Bug 93368

Summary: [CSSRegions]Expose WebKitNamedFlowCollection interface through document.getNamedFlows()
Product: WebKit Reporter: Andrei Onea <andreionea3000>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric, gns, 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 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+

Description Andrei Onea 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.
Comment 1 Andrei Onea 2012-08-07 08:25:10 PDT
Created attachment 156943 [details]
Patch
Comment 2 Gyuyoung Kim 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.
Comment 3 Andrei Onea 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/
Comment 4 Andrei Onea 2012-08-17 02:28:46 PDT
Created attachment 159055 [details]
Patch
Comment 5 Andrei Onea 2012-08-17 05:59:36 PDT
Created attachment 159106 [details]
Patch
Comment 6 Early Warning System Bot 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
Comment 7 Early Warning System Bot 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
Comment 8 Build Bot 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
Comment 9 Andrei Onea 2012-08-17 07:32:33 PDT
Created attachment 159122 [details]
patch
Comment 10 WebKit Review Bot 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
Comment 11 WebKit Review Bot 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
Comment 12 Early Warning System Bot 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
Comment 13 Early Warning System Bot 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
Comment 14 Andrei Onea 2012-08-17 09:20:41 PDT
Created attachment 159139 [details]
patch
Comment 15 Ryosuke Niwa 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.
Comment 16 Andrei Onea 2012-08-22 08:15:58 PDT
Created attachment 159937 [details]
Patch
Comment 17 Early Warning System Bot 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
Comment 18 Early Warning System Bot 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
Comment 19 Build Bot 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
Comment 20 Ryosuke Niwa 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]
Comment 21 Andrei Onea 2012-08-24 02:39:27 PDT
Created attachment 160366 [details]
Patch
Comment 22 Build Bot 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
Comment 23 Andrei Onea 2012-08-24 04:45:11 PDT
Created attachment 160396 [details]
Patch
Comment 24 Build Bot 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
Comment 25 Andrei Onea 2012-08-24 06:10:35 PDT
Created attachment 160409 [details]
Patch
Comment 26 Ryosuke Niwa 2012-08-24 13:15:17 PDT
I'm going to land this patch manually to correctly rename WebKitNamedFlowCollection.h/cpp with "svn mv".
Comment 27 Ryosuke Niwa 2012-08-24 14:45:53 PDT
Committed r126627: <http://trac.webkit.org/changeset/126627>