Since each internals instance is associated with a document, this isn't necessary.
Hi, I'm working on this issue. I'll submit a patch soon.
Created attachment 214862 [details] WIP patch for EWS testing
Comment on attachment 214862 [details] WIP patch for EWS testing Attachment 214862 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/8858343 New failing tests: fast/dom/nodesFromRect/nodesFromRect-inner-documents.html compositing/visible-rect/iframe-no-layers.html platform/mac-wk2/tiled-drawing/tiled-drawing-scroll-position-page-cache-restoration.html
Created attachment 214868 [details] Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-09 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 214862 [details] WIP patch for EWS testing Attachment 214862 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/8898303 New failing tests: fast/dom/nodesFromRect/nodesFromRect-inner-documents.html compositing/visible-rect/iframe-no-layers.html
Created attachment 214869 [details] Archive of layout-test-results from webkit-ews-02 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-02 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 214862 [details] WIP patch for EWS testing Attachment 214862 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/9048014 New failing tests: fast/dom/nodesFromRect/nodesFromRect-inner-documents.html compositing/visible-rect/iframe-no-layers.html
Created attachment 214871 [details] Archive of layout-test-results from webkit-ews-05 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-05 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 214887 [details] WIP patch (v2)
Comment on attachment 214887 [details] WIP patch (v2) Attachment 214887 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/6078020 New failing tests: fast/dom/nodesFromRect/nodesFromRect-inner-documents.html compositing/visible-rect/iframe-no-layers.html
Created attachment 214900 [details] Archive of layout-test-results from webkit-ews-04 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-04 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 214887 [details] WIP patch (v2) Attachment 214887 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6078026 New failing tests: fast/dom/nodesFromRect/nodesFromRect-inner-documents.html compositing/visible-rect/iframe-no-layers.html platform/mac-wk2/tiled-drawing/tiled-drawing-scroll-position-page-cache-restoration.html
Created attachment 214903 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 215064 [details] WIP patch (v3)
Created attachment 215084 [details] Proposed patch
Comment on attachment 215084 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=215084&action=review > Source/WebCore/ChangeLog:11 > + Each 'Internals' instance is associated with a 'Document'. So, it > + is not necessary to pass a document as argument. Only nodesFromRect and > + layerTreeAsText methods were kept because, in some Layout Tests, the > + 'Document' object is not the same used by Internals::contextDocument. Can you add comments in these functions explaining why they take the document argument? > Source/WebCore/ChangeLog:13 > + No new tests. No reason to add this line, it's pretty obvious to anyone who would care why there are no new tests.
Comment on attachment 215084 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=215084&action=review >> Source/WebCore/ChangeLog:11 >> + 'Document' object is not the same used by Internals::contextDocument. > > Can you add comments in these functions explaining why they take the document argument? Ok, but would it be interesting to put a FIXME or a simple comment?
Created attachment 215110 [details] Patch Fixed patch according to Alexey's comments.
Comment on attachment 215110 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215110&action=review This patch doesn't apply, so commit queue won't be able to land it. > Source/WebCore/testing/Internals.cpp:1538 > +//The 'document' parameter was not removed because the document instance (got from > +//contextDocument()) is not the same as the one passed in the Layout Tests > +//(window.document, for example). Some nitpicks: 1.There should be spaces after "//". 2. Saying why the argument was not removed is not the best comment that can be made. A person reading likely won't care about the history (and won't even know that we used to have the argument in other functions before). A comment should talk about current state, not about history. I'd have said something like: // FIXME: Remove the document argument. It is almost always the same as contextDocument(), with the exception of a few tests that pass a different document, and could just make the call through another Internals instance instead.
(In reply to comment #19) > (From update of attachment 215110 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=215110&action=review > > This patch doesn't apply, so commit queue won't be able to land it. > > > Source/WebCore/testing/Internals.cpp:1538 > > +//The 'document' parameter was not removed because the document instance (got from > > +//contextDocument()) is not the same as the one passed in the Layout Tests > > +//(window.document, for example). > > Some nitpicks: > > 1.There should be spaces after "//". > > 2. Saying why the argument was not removed is not the best comment that can be made. A person reading likely won't care about the history (and won't even know that we used to have the argument in other functions before). A comment should talk about current state, not about history. > > I'd have said something like: > > // FIXME: Remove the document argument. It is almost always the same as contextDocument(), with the exception of a few tests that pass a different document, and could just make the call through another Internals instance instead. Thank you Alexey for review. I'll submit a new version ASAP.
Created attachment 215302 [details] Proposed patch (v2) Fixed code comments according to Alexey's suggestion.
Created attachment 215309 [details] Proposed patch (v3) Patch was rebased with branch master.
Comment on attachment 215309 [details] Proposed patch (v3) Clearing flags on attachment: 215309 Committed r158113: <http://trac.webkit.org/changeset/158113>
All reviewed patches have been landed. Closing bug.