| 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: | P2 | ||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||
| Bug Blocks: | 57312 | ||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||
|
Description
Andrei Onea
2012-08-07 07:21:50 PDT
Created attachment 156943 [details]
Patch
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. 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/ Created attachment 159055 [details]
Patch
Created attachment 159106 [details]
Patch
Comment on attachment 159106 [details] Patch Attachment 159106 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13529055 Comment on attachment 159106 [details] Patch Attachment 159106 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13518624 Comment on attachment 159106 [details] Patch Attachment 159106 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13516804 Created attachment 159122 [details]
patch
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 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
Comment on attachment 159122 [details] patch Attachment 159122 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13516838 Comment on attachment 159122 [details] patch Attachment 159122 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13514862 Created attachment 159139 [details]
patch
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. Created attachment 159937 [details]
Patch
Comment on attachment 159937 [details] Patch Attachment 159937 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13568255 Comment on attachment 159937 [details] Patch Attachment 159937 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13570254 Comment on attachment 159937 [details] Patch Attachment 159937 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13563293 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] Created attachment 160366 [details]
Patch
Comment on attachment 160366 [details] Patch Attachment 160366 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13592109 Created attachment 160396 [details]
Patch
Comment on attachment 160396 [details] Patch Attachment 160396 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13591210 Created attachment 160409 [details]
Patch
I'm going to land this patch manually to correctly rename WebKitNamedFlowCollection.h/cpp with "svn mv". Committed r126627: <http://trac.webkit.org/changeset/126627> |