Bug 66642

<rdar://problem/10218110>

Created attachment 116676 [details] Patch

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 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 on attachment 116676 [details] Patch Attachment 116676 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/10671036

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

(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

Created attachment 116754 [details] Patch 2 I have followed Adam's suggestion and prefixed the method in Document.idl with webkit keyword.

Created attachment 116763 [details] Patch 3 Updated expected results to take into account the webkit prefix.

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 on attachment 116763 [details] Patch 3 No need for auto commit at this moment.

Comment on attachment 116763 [details] Patch 3 I need to rework it.

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

Created attachment 117861 [details] Patch 5

Created attachment 117862 [details] Patch 5 With the right characters in the license text.

Created attachment 118023 [details] Patch 6 Fixing the Win build also.

Comment on attachment 118023 [details] Patch 6 r=me

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

Created attachment 118360 [details] Patch for landing

Comment on attachment 118360 [details] Patch for landing Clearing flags on attachment: 118360 Committed r102333: <http://trac.webkit.org/changeset/102333>

All reviewed patches have been landed. Closing bug.