93368 – [CSSRegions]Expose WebKitNamedFlowCollection interface through document.getNamedFlows()

RESOLVED FIXED 93368

[CSSRegions]Expose WebKitNamedFlowCollection interface through document.getNamedFlows()

https://bugs.webkit.org/show_bug.cgi?id=93368

Summary [CSSRegions]Expose WebKitNamedFlowCollection interface through document.getNa...

Andrei Onea

Reported 2012-08-07 07:21:50 PDT

The WebKitNamedFlowCollection class is already implemented, we now need to expose it through document.webkitNamedFlows. Also, to allow testing, add the length property. http://www.w3.org/TR/css3-regions/#dom-named-flow-collection.

Attachments
Patch (26.98 KB, patch) 2012-08-07 08:25 PDT, Andrei Onea	no flags	Details Formatted Diff Diff
Patch (39.30 KB, patch) 2012-08-17 02:28 PDT, Andrei Onea	no flags	Details Formatted Diff Diff
Patch (37.95 KB, patch) 2012-08-17 05:59 PDT, Andrei Onea	webkit-ews: commit-queue-	Details Formatted Diff Diff
patch (38.39 KB, patch) 2012-08-17 07:32 PDT, Andrei Onea	webkit.review.bot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from gce-cr-linux-01 (346.25 KB, application/zip) 2012-08-17 08:47 PDT, WebKit Review Bot	no flags	Details
patch (38.87 KB, patch) 2012-08-17 09:20 PDT, Andrei Onea	no flags	Details Formatted Diff Diff
Patch (73.12 KB, patch) 2012-08-22 08:15 PDT, Andrei Onea	no flags	Details Formatted Diff Diff
Patch (73.92 KB, patch) 2012-08-24 02:39 PDT, Andrei Onea	no flags	Details Formatted Diff Diff
Patch (74.11 KB, patch) 2012-08-24 04:45 PDT, Andrei Onea	buildbot: commit-queue-	Details Formatted Diff Diff
Patch (73.02 KB, patch) 2012-08-24 06:10 PDT, Andrei Onea	rniwa: review+	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Andrei Onea

Comment 1 2012-08-07 08:25:10 PDT

Created attachment 156943 [details] Patch

Gyuyoung Kim

Comment 2 2012-08-12 22:08:20 PDT

Comment on attachment 156943 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=156943&action=review > Source/WebCore/dom/WebKitNamedFlowCollection.cpp:93 > + return 0; In my humble opinion, early return is more close WebKit style. if (!m_document) return 0; m_document->updateLayoutIgnorePendingStylesheets(); In addition, it seems it is good to add an empty line.

Andrei Onea

Comment 3 2012-08-16 08:49:14 PDT

The editor's draft has changed the way NamedFlowCollection is exposed. Now, it is a static snapshot of the NamedFlow's in the CREATED state, and is exposed via the Document.getNamedFlows() function. http://dev.w3.org/csswg/css3-regions/

Andrei Onea

Comment 4 2012-08-17 02:28:46 PDT

Created attachment 159055 [details] Patch

Andrei Onea

Comment 5 2012-08-17 05:59:36 PDT

Created attachment 159106 [details] Patch

Early Warning System Bot

Comment 6 2012-08-17 06:46:42 PDT

Comment on attachment 159106 [details] Patch Attachment 159106 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13529055

Early Warning System Bot

Comment 7 2012-08-17 06:56:33 PDT

Comment on attachment 159106 [details] Patch Attachment 159106 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13518624

Build Bot

Comment 8 2012-08-17 07:10:56 PDT

Comment on attachment 159106 [details] Patch Attachment 159106 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13516804

Andrei Onea

Comment 9 2012-08-17 07:32:33 PDT

Created attachment 159122 [details] patch

WebKit Review Bot

Comment 10 2012-08-17 08:47:25 PDT

Comment on attachment 159122 [details] patch Attachment 159122 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13519466 New failing tests: animations/suspend-resume-animation-events.html

WebKit Review Bot

Comment 11 2012-08-17 08:47:31 PDT

Created attachment 159133 [details] Archive of layout-test-results from gce-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Early Warning System Bot

Comment 12 2012-08-17 09:05:50 PDT

Comment on attachment 159122 [details] patch Attachment 159122 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13516838

Early Warning System Bot

Comment 13 2012-08-17 09:15:03 PDT

Comment on attachment 159122 [details] patch Attachment 159122 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13514862

Andrei Onea

Comment 14 2012-08-17 09:20:41 PDT

Created attachment 159139 [details] patch

Ryosuke Niwa

Comment 15 2012-08-20 13:41:36 PDT

Comment on attachment 159139 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=159139&action=review > LayoutTests/fast/regions/webkit-named-flow-collection.html:1 > +<html> Missing DOCTYPE. > LayoutTests/fast/regions/webkit-named-flow-collection.html:3 > + <head> > + </head> We don't need head. > LayoutTests/fast/regions/webkit-named-flow-collection.html:4 > +<script src="../../fast/js/resources/js-test-pre.js"></script> This should be in the body element. > LayoutTests/fast/regions/webkit-named-flow-collection.html:11 > + description("Tests WebKitNamedFlowCollection interface") > + if (window.testRunner) { > + testRunner.dumpAsText(); > + } No need to indent the entire script. Also, no curly brackets around a single statement. > LayoutTests/fast/regions/webkit-named-flow-collection.html:19 > + // Test namedFlows.length function > + > + // There aren't any named flows in the document We should either delete these comments or put in debug so that they appear in expected result. Otherwise the output doesn't make much sense. > LayoutTests/fast/regions/webkit-named-flow-collection.html:26 > + region1.style.webkitFlowFrom = "flow1"; > + document.body.appendChild(region1); > + namedFlows = document.webkitGetNamedFlows(); It'll be better if this code was in shouldBe as in: shouldBe("region1.style.webkitFlowFrom = 'flow1'; document.webkitGetNamedFlows().length", "1"); > LayoutTests/fast/regions/webkit-named-flow-collection.html:45 > + // Add another named flow, there will be two in total > + region2.style.webkitFlowFrom = "flow2"; > + document.body.appendChild(region2); > + namedFlows = document.webkitGetNamedFlows(); > + shouldBe("namedFlows.length", "2"); > + > + // Remove first named flow, one will remain > + region1.style.webkitFlowFrom = ""; > + document.body.removeChild(region1); > + namedFlows = document.webkitGetNamedFlows(); > + shouldBe("namedFlows.length", "1"); > + > + // Remove remaining named flow, there will be no named flows remaining > + region2.style.webkitFlowFrom = ""; > + document.body.removeChild(region2); > + namedFlows = document.webkitGetNamedFlows(); > + shouldBe("namedFlows.length", "0"); Ditto about these tests cases. > Source/WebCore/dom/StaticWebKitNamedFlowCollection.h:42 > +class StaticWebKitNamedFlowCollection : public WebKitNamedFlowCollection { Why do we have WebKitNamedFlowCollection and StaticWebKitNamedFlowCollection? It appears that WebKitNamedFlowCollection is used internally in WebCore? I don't think we should be using inheritance here. The internal representation and the external representation doesn't need to be the same, and if WebKitNamedFlowCollection is live, then it's probably create a new class and put member functions there. If we can share some code between the two, then we should be creating a super class between the two instead. Also, WebKitNamedFlowCollection should be renamed to something else if were intended to be used internally. r- because we need to sort of the design issues first. > Source/WebCore/dom/StaticWebKitNamedFlowCollection.h:48 > + unsigned long length() const; We need item() and namedItem(). > Source/WebCore/dom/WebKitNamedFlowCollection.cpp:50 > + m_namedFlows.add(*it); Wrong indentation.

Andrei Onea

Comment 16 2012-08-22 08:15:58 PDT

Created attachment 159937 [details] Patch

Early Warning System Bot

Comment 17 2012-08-22 08:40:55 PDT

Comment on attachment 159937 [details] Patch Attachment 159937 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13568255

Early Warning System Bot

Comment 18 2012-08-22 08:43:07 PDT

Comment on attachment 159937 [details] Patch Attachment 159937 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13570254

Build Bot

Comment 19 2012-08-22 08:50:26 PDT

Comment on attachment 159937 [details] Patch Attachment 159937 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13563293

Ryosuke Niwa

Comment 20 2012-08-23 14:39:12 PDT

Comment on attachment 159937 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159937&action=review > Source/WebCore/bindings/gobject/GNUmakefile.am:258 > + DerivedSources/webkit/WebKitDOMWebKitNamedFlowCollection.cpp \ Wrong file name. > Source/WebCore/bindings/gobject/GNUmakefile.am:392 > + DerivedSources/webkit/WebKitDOMWebKitNamedFlowCollection.h \ Ditto. > Source/WebCore/dom/DOMNamedFlowCollection.cpp:52 > + for (unsigned long i = 0; i < index; ++it, ++i) { } I'd prefer if you had incremented "it" as a statement in the for so that we don't have to have an awkward { }. > Source/WebCore/dom/DOMNamedFlowCollection.idl:35 > + interface [ > + IndexedGetter, > + NamedGetter, > + InterfaceName=WebKitNamedFlowCollection, > + JSGenerateToJSObject Shouldn't we have Conditional=CSS_REGIONS > Source/WebCore/dom/Document.idl:274 > #if defined(ENABLE_CSS_REGIONS) && ENABLE_CSS_REGIONS > WebKitNamedFlow webkitGetFlowByName(in DOMString name); > + DOMNamedFlowCollection webkitGetNamedFlows(); > #endif Please use [Conditional=CSS_REGIONS]

Andrei Onea

Comment 21 2012-08-24 02:39:27 PDT

Created attachment 160366 [details] Patch

Build Bot

Comment 22 2012-08-24 03:29:08 PDT

Comment on attachment 160366 [details] Patch Attachment 160366 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13592109

Andrei Onea

Comment 23 2012-08-24 04:45:11 PDT

Created attachment 160396 [details] Patch

Build Bot

Comment 24 2012-08-24 05:50:28 PDT

Comment on attachment 160396 [details] Patch Attachment 160396 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13591210

Andrei Onea

Comment 25 2012-08-24 06:10:35 PDT

Created attachment 160409 [details] Patch

Ryosuke Niwa

Comment 26 2012-08-24 13:15:17 PDT

I'm going to land this patch manually to correctly rename WebKitNamedFlowCollection.h/cpp with "svn mv".

Ryosuke Niwa

Comment 27 2012-08-24 14:45:53 PDT

Committed r126627: <http://trac.webkit.org/changeset/126627>

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component CSS

Assignee

Nobody

Reported

2012-08-07 07:21 PDT

Modified

2012-08-24 14:45 PDT History

CC List

14 users Show

URL

Keywords

Depends on

Blocks

57312

Dependencies

tree graph