RESOLVED FIXED138788
Move the 'quotes' CSS property to the new StyleBuilder 
https://bugs.webkit.org/show_bug.cgi?id=138788
Summary Move the 'quotes' CSS property to the new StyleBuilder
Sam Weinig
Reported 2014-11-16 20:58:37 PST
Move the 'quotes' CSS property to the new StyleBuilder
Attachments
Patch (11.29 KB, patch)
2014-11-16 21:02 PST, Sam Weinig 		no flags
DetailsFormatted DiffDiff
Patch (11.29 KB, patch)
2014-11-16 21:43 PST, Sam Weinig 		no flags
DetailsFormatted DiffDiff
Patch (11.24 KB, patch)
2014-11-17 07:03 PST, Sam Weinig 		cdumez: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Sam Weinig
Comment 1 2014-11-16 21:02:39 PST
Created attachment 241691 [details] Patch
Sam Weinig
Comment 2 2014-11-16 21:43:08 PST
Created attachment 241693 [details] Patch
Sam Weinig
Comment 3 2014-11-17 07:03:52 PST
Created attachment 241709 [details] Patch
Chris Dumez
Comment 4 2014-11-17 09:44:56 PST
Comment on attachment 241709 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=241709&action=review r=me with nits. > Source/WebCore/css/StyleBuilderConverter.h:429 > + ASSERT(is<CSSValueList>(value)); This is not needed. There is already such assert in downcast<CSSValueList>(). > Source/WebCore/css/StyleBuilderConverter.h:431 > + Vector<std::pair<String, String>> quotes; Maybe we could even reserve capacity: quotes.reserveInitialCapacity(list.length() / 2); > Source/WebCore/css/StyleBuilderConverter.h:432 > + for (size_t i = 0; i < list.length(); i += 2) { nit: could be unsigned as Vector::m_size is unsigned. > Source/WebCore/css/StyleBuilderConverter.h:437 > + continue; nit: Looks like we should even break; here. > LayoutTests/fast/css/content/content-quotes-07.html:1 > +<html> nit: Missing <!DOCTYPE html> (to make sure we run in strict mode). > LayoutTests/fast/css/content/content-quotes-07.html:3 > + <style type="text/css"> Nit: we don't need to specify the type as this is the default. > LayoutTests/fast/css/content/content-quotes-07.html:6 > + <script type="text/javascript"> Nit: we don't need to specify the type as this is the default. > LayoutTests/fast/css/content/content-quotes-07.html:7 > + if (window.testRunner) { Your call, but I think it would be nice to use js-test-pre.js framework. > LayoutTests/fast/css/content/content-quotes-07.html:13 > + testWidth = window.getComputedStyle(document.getElementById("testContainer"), null).getPropertyValue("width"); nit: var testWidth = ... > LayoutTests/fast/css/content/content-quotes-07.html:14 > + referenceWidth = window.getComputedStyle(document.getElementById("reference"), null).getPropertyValue("width"); nit: var referenceWidth = ... > LayoutTests/fast/css/content/content-quotes-07.html:41 > + <p>========Marker3========</p> Marker3? > LayoutTests/fast/css/content/content-quotes-07.html:5 > + body { quotes: baseline; } /* This should be ignored, as "baseline" is not a vaild value for quotes */ typo: "valid" BTW, I don't get why Bugzilla shows LayoutTests/fast/css/content/content-quotes-07.html twice in this patch?
Sam Weinig
Comment 5 2014-11-19 17:14:17 PST
Fixed in http://trac.webkit.org/changeset/176369.
Note You need to log in before you can comment on or make changes to this bug.