[webkit-reviews] review denied: [Bug 93368] [CSSRegions]Expose WebKitNamedFlowCollection interface through document.getNamedFlows() : [Attachment 159139] patch

Mon Aug 20 13:40:58 PDT 2012

Ryosuke Niwa <rniwa at webkit.org> has denied Andrei Onea <onea at adobe.com>'s
request for review:
Bug 93368: [CSSRegions]Expose WebKitNamedFlowCollection interface through
document.getNamedFlows()
https://bugs.webkit.org/show_bug.cgi?id=93368

Attachment 159139: patch
https://bugs.webkit.org/attachment.cgi?id=159139&action=review

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
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.