[Webkit-unassigned] [Bug 93368] [CSSRegions]Expose WebKitNamedFlowCollection interface through document.getNamedFlows()

Mon Aug 20 13:41:02 PDT 2012

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

Ryosuke Niwa <rniwa at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #159139|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |

--- Comment #15 from Ryosuke Niwa <rniwa at webkit.org>  2012-08-20 13:41:36 PST ---
(From update of attachment 159139)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.