Bug 66642 – [CSSRegions][CSSOM] Implement NamedFlow interface

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Alexandru Chiculita]

Implement the NamedFlow interface from the spec. Add the flowWithName on the Document idl.

http://www.w3.org/TR/css3-regions/#the-namedflow-interface

[Radar WebKit Bug Importer]

<rdar://problem/10218110>

[Mihnea Ovidenie]

Created attachment 116676 [details]
Patch

[WebKit Review Bot]

Attachment 116676 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/WebCore.vcproj/WebCore.vcproj:23931:  mismatched tag  [xml/syntax] [5]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adam Barth]

Comment on attachment 116676 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=116676&action=review

> Source/WebCore/dom/Document.idl:250
> +        WebKitNamedFlow getFlowByName(in DOMString name);

Did you mean to prefix this API?

[Early Warning System Bot]

Comment on attachment 116676 [details]
Patch

Attachment 116676 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10671036

[Mihnea Ovidenie]

(In reply to comment #4)
> (From update of attachment 116676 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116676&action=review
> 
> > Source/WebCore/dom/Document.idl:250
> > +        WebKitNamedFlow getFlowByName(in DOMString name);
> 
> Did you mean to prefix this API?

Yes, that was my intention.

[Csaba Osztrogonác]

(In reply to comment #5)
> (From update of attachment 116676 [details])
> Attachment 116676 [details] did not pass qt-ews (qt):
> Output: http://queues.webkit.org/results/10671036

You missed to add the new source to WebKit/Source/WebCore/Target.pri

[Mihnea Ovidenie]

Created attachment 116754 [details]
Patch 2

I have followed Adam's suggestion and prefixed the method in Document.idl with webkit keyword.

[Mihnea Ovidenie]

Created attachment 116763 [details]
Patch 3

Updated expected results to take into account the webkit prefix.

[Mihnea Ovidenie]

Regarding the lifetime of the NamedFlow object, there is an open bug at http://www.w3.org/Bugs/Public/show_bug.cgi?id=14948. In the current patch, when you ask for a NamedFlow object for a flow thread that does not exist, you get a null object.

[Mihnea Ovidenie]

Comment on attachment 116763 [details]
Patch 3

No need for auto commit at this moment.

[Mihnea Ovidenie]

Comment on attachment 116763 [details]
Patch 3

I need to rework it.

[Mihnea Ovidenie]

Created attachment 116906 [details]
Patch 4

The first time the user asks for a named flow object, it gets a valid js object even if we do not yet have defined a flow thread in the document. The properties of the NamedFlow object will reflect the properties of the flow thread.

[Andreas Kling]

Comment on attachment 116906 [details]
Patch 4

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().

[Mihnea Ovidenie]

Created attachment 117861 [details]
Patch 5

[Mihnea Ovidenie]

Created attachment 117862 [details]
Patch 5

With the right characters in the license text.

[Mihnea Ovidenie]

Created attachment 118023 [details]
Patch 6

Fixing the Win build also.

[Dave Hyatt]

Comment on attachment 118023 [details]
Patch 6

r=me

[WebKit Review Bot]

Comment on attachment 118023 [details]
Patch 6

Rejecting attachment 118023 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
 at 127 with fuzz 2 (offset 4 lines).
Hunk #4 succeeded at 186 (offset 6 lines).
patching file Source/WebCore/rendering/RenderObject.cpp
patching file Source/WebCore/rendering/RenderView.cpp
Hunk #1 succeeded at 879 (offset 1 line).
Hunk #2 succeeded at 894 (offset 1 line).
patching file Source/WebCore/rendering/RenderView.h
Hunk #1 succeeded at 172 (offset 1 line).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'David Hyatt', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/10750244

[Mihnea Ovidenie]

Created attachment 118360 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 118360 [details]
Patch for landing

Clearing flags on attachment: 118360

Committed r102333: <http://trac.webkit.org/changeset/102333>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.