Bug 66642 - [CSSRegions][CSSOM] Implement NamedFlow interface
Summary: [CSSRegions][CSSOM] Implement NamedFlow interface
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mihnea Ovidenie
URL:
Keywords: InRadar
Depends on:
Blocks: 57312
  Show dependency treegraph
 
Reported: 2011-08-21 23:18 PDT by Alexandru Chiculita
Modified: 2011-12-08 06:23 PST (History)
9 users (show)

See Also:


Attachments
Patch (49.80 KB, patch)
2011-11-27 11:33 PST, Mihnea Ovidenie
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Patch 2 (52.31 KB, patch)
2011-11-28 07:06 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 3 (52.47 KB, patch)
2011-11-28 08:22 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 4 (52.72 KB, patch)
2011-11-29 00:09 PST, Mihnea Ovidenie
kling: review-
Details | Formatted Diff | Diff
Patch 5 (57.39 KB, patch)
2011-12-05 02:45 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 5 (57.38 KB, patch)
2011-12-05 02:49 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch 6 (56.79 KB, patch)
2011-12-06 04:16 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff
Patch for landing (59.36 KB, patch)
2011-12-08 04:07 PST, Mihnea Ovidenie
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexandru Chiculita 2011-08-21 23:18:04 PDT
Implement the NamedFlow interface from the spec. Add the flowWithName on the Document idl.

http://www.w3.org/TR/css3-regions/#the-namedflow-interface
Comment 1 Radar WebKit Bug Importer 2011-09-30 16:01:49 PDT
<rdar://problem/10218110>
Comment 2 Mihnea Ovidenie 2011-11-27 11:33:35 PST
Created attachment 116676 [details]
Patch
Comment 3 WebKit Review Bot 2011-11-27 11:41:22 PST
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.
Comment 4 Adam Barth 2011-11-27 11:48:53 PST
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?
Comment 5 Early Warning System Bot 2011-11-27 11:54:53 PST
Comment on attachment 116676 [details]
Patch

Attachment 116676 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10671036
Comment 6 Mihnea Ovidenie 2011-11-27 22:16:22 PST
(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.
Comment 7 Csaba Osztrogonác 2011-11-28 01:40:34 PST
(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
Comment 8 Mihnea Ovidenie 2011-11-28 07:06:17 PST
Created attachment 116754 [details]
Patch 2

I have followed Adam's suggestion and prefixed the method in Document.idl with webkit keyword.
Comment 9 Mihnea Ovidenie 2011-11-28 08:22:07 PST
Created attachment 116763 [details]
Patch 3

Updated expected results to take into account the webkit prefix.
Comment 10 Mihnea Ovidenie 2011-11-28 08:27:53 PST
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.
Comment 11 Mihnea Ovidenie 2011-11-28 09:14:53 PST
Comment on attachment 116763 [details]
Patch 3

No need for auto commit at this moment.
Comment 12 Mihnea Ovidenie 2011-11-28 10:55:20 PST
Comment on attachment 116763 [details]
Patch 3

I need to rework it.
Comment 13 Mihnea Ovidenie 2011-11-29 00:09:33 PST
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.
Comment 14 Andreas Kling 2011-12-04 08:55:03 PST
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().
Comment 15 Mihnea Ovidenie 2011-12-05 02:45:18 PST
Created attachment 117861 [details]
Patch 5
Comment 16 Mihnea Ovidenie 2011-12-05 02:49:46 PST
Created attachment 117862 [details]
Patch 5

With the right characters in the license text.
Comment 17 Mihnea Ovidenie 2011-12-06 04:16:38 PST
Created attachment 118023 [details]
Patch 6

Fixing the Win build also.
Comment 18 Dave Hyatt 2011-12-07 11:46:37 PST
Comment on attachment 118023 [details]
Patch 6

r=me
Comment 19 WebKit Review Bot 2011-12-07 12:08:23 PST
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
Comment 20 Mihnea Ovidenie 2011-12-08 04:07:54 PST
Created attachment 118360 [details]
Patch for landing
Comment 21 WebKit Review Bot 2011-12-08 06:23:29 PST
Comment on attachment 118360 [details]
Patch for landing

Clearing flags on attachment: 118360

Committed r102333: <http://trac.webkit.org/changeset/102333>
Comment 22 WebKit Review Bot 2011-12-08 06:23:36 PST
All reviewed patches have been landed.  Closing bug.