Bug 138788

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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	cdumez: review+

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	Details Formatted Diff Diff
Patch (11.29 KB, patch) 2014-11-16 21:43 PST, Sam Weinig	no flags	Details Formatted Diff Diff
Patch (11.24 KB, patch) 2014-11-17 07:03 PST, Sam Weinig	cdumez: review+	Details Formatted Diff Diff
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.