[webkit-reviews] review denied: [Bug 66642] [CSSRegions][CSSOM] Implement NamedFlow interface : [Attachment 116906] Patch 4

[bugzilla-daemon at webkit.org]

Andreas Kling <kling at webkit.org> has denied Mihnea Ovidenie
<mihnea at adobe.com>'s request for review:
Bug 66642: [CSSRegions][CSSOM] Implement NamedFlow interface
https://bugs.webkit.org/show_bug.cgi?id=66642

Attachment 116906: Patch 4
https://bugs.webkit.org/attachment.cgi?id=116906&action=review

------- Additional Comments from Andreas Kling <kling at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116906&action=review


Looks generally good, but I'd like to see another iteration, mostly because of
the tests.

> LayoutTests/fast/regions/webkit-named-flow-existing-flow-expected.txt:1
> +PASS

We should strive to avoid adding new tests that simply dump "PASS" or "FAIL".
If/when such a test breaks in the future, it will be very helpful if the test
is more verbose about what's being tested.

> Source/WebCore/dom/Document.cpp:1000
> +    if (!renderer() || !renderer()->view())
> +	   return 0;
> +
> +    RenderFlowThread* flowThread =
renderer()->view()->renderFlowThreadWithName(flowName);

Nit: RenderObject::view() is an out-of-line call, so this would be slightly
more efficient like so:
if (!renderer())
    return 0;
if (RenderView* view = renderer()->view())
    return view->renderFlowThreadWithName(flowName);
return 0;

> Source/WebCore/dom/Document.h:355
> +    WebKitNamedFlow* webkitGetFlowByName(const String&);

This should return a PassRefPtr<WebKitNamedFlow> rather than a raw pointer.

> Source/WebCore/dom/WebKitNamedFlow.cpp:16
> +* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDER “AS IS” AND ANY

Looks like we have some text encoding problem with the apostrophes here.

> Source/WebCore/dom/WebKitNamedFlow.h:16
> +* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDER “AS IS” AND ANY

Ditto.

> Source/WebCore/dom/WebKitNamedFlow.h:42
> +	   return adoptRef(new WebKitNamedFlow());

Style nit: No need for () with argument-less constructors.

> Source/WebCore/dom/WebKitNamedFlow.idl:16
> +* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDER “AS IS” AND ANY

Text encoding again.

> Source/WebCore/rendering/RenderFlowThread.cpp:54
> +    , m_namedFlow(0)

Unnecessary, a RefPtr is null by default.

> Source/WebCore/rendering/RenderFlowThread.h:78
> +    bool hasChildren() const { return m_flowThreadChildList.size(); }

I think this would read slightly nicer as:
return !m_flowThreadChildList.isEmpty();

> Source/WebCore/rendering/RenderFlowThread.h:126
> +    WebKitNamedFlow* getFlowByName();

This should be called "namedFlow()" instead, as "getFlowByName()" implies that
it takes a name argument.

> Source/WebCore/rendering/RenderView.h:177
> +    // Does not create a render flow thread if one with the name does not
exist.
> +    RenderFlowThread* getFlowThreadWithName(const AtomicString& flowThread)
const;

This name is too similar to "renderFlowThreadWithName", we need something
better.
A common pattern in WebKit is having the version that is guaranteed to return a
value called "ensureFoo()", and the one that only returns it if one exists
"foo()".
In other words, the current renderFlowThreadWithName() would be renamed to
ensureRenderFlowThreadWithName() and the new one would be simply
renderFlowThreadWithName().