Bug 93368

Summary:

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

Product:

WebKit

Reporter:

Andrei Onea <andreionea3000>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dglazkov, eric, gustavo, gyuyoung.kim, haraken, japhet, ojan, philn, rakuco, rniwa, vestbo, webkit.review.bot, xan.lopez

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

57312

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	webkit-ews: commit-queue-
patch	webkit.review.bot: commit-queue-
Archive of layout-test-results from gce-cr-linux-01	none
patch	none
Patch	none
Patch	none
Patch	buildbot: commit-queue-
Patch	rniwa: review+

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