| Summary: | Move the 'quotes' CSS property to the new StyleBuilder | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Sam Weinig <sam> | ||||||||
| Component: | New Bugs | Assignee: | Sam Weinig <sam> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | cdumez | ||||||||
| Priority: | P2 | ||||||||||
| Version: | 528+ (Nightly build) | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Sam Weinig
2014-11-16 20:58:37 PST
Created attachment 241691 [details]
Patch
Created attachment 241693 [details]
Patch
Created attachment 241709 [details]
Patch
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? Fixed in http://trac.webkit.org/changeset/176369. |