Bug 107301 - methods on window.internals shouldn't pass in a document
Summary: methods on window.internals shouldn't pass in a document
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 107757
Blocks:
  Show dependency treegraph
 
Reported: 2013-01-18 10:26 PST by Tony Chang
Modified: 2013-10-28 09:14 PDT (History)
10 users (show)

See Also:


Attachments
WIP patch for EWS testing (574.49 KB, patch)
2013-10-22 09:18 PDT, Afonso Costa
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-09 for mac-mountainlion-wk2 (458.10 KB, application/zip)
2013-10-22 10:07 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (482.16 KB, application/zip)
2013-10-22 10:30 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (478.26 KB, application/zip)
2013-10-22 11:28 PDT, Build Bot
no flags Details
WIP patch (v2) (574.41 KB, patch)
2013-10-22 14:48 PDT, Afonso Costa
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (640.23 KB, application/zip)
2013-10-22 16:09 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (520.08 KB, application/zip)
2013-10-22 16:45 PDT, Build Bot
no flags Details
WIP patch (v3) (377.20 KB, patch)
2013-10-24 08:00 PDT, Afonso Costa
no flags Details | Formatted Diff | Diff
Proposed patch (377.20 KB, patch)
2013-10-24 11:35 PDT, Afonso Costa
ap: review+
Details | Formatted Diff | Diff
Patch (378.34 KB, patch)
2013-10-24 15:26 PDT, Afonso Costa
ap: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (v2) (378.34 KB, patch)
2013-10-28 07:01 PDT, Afonso Costa
no flags Details | Formatted Diff | Diff
Proposed patch (v3) (378.29 KB, patch)
2013-10-28 08:27 PDT, Afonso Costa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2013-01-18 10:26:45 PST
Since each internals instance is associated with a document, this isn't necessary.
Comment 1 Afonso Costa 2013-10-18 06:20:59 PDT
Hi,

I'm working on this issue. I'll submit a patch soon.
Comment 2 Afonso Costa 2013-10-22 09:18:34 PDT
Created attachment 214862 [details]
WIP patch for EWS testing
Comment 3 Build Bot 2013-10-22 10:07:31 PDT
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
Comment 4 Build Bot 2013-10-22 10:07:34 PDT
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 5 Build Bot 2013-10-22 10:30:02 PDT
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
Comment 6 Build Bot 2013-10-22 10:30:05 PDT
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 7 Build Bot 2013-10-22 11:28:07 PDT
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
Comment 8 Build Bot 2013-10-22 11:28:10 PDT
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
Comment 9 Afonso Costa 2013-10-22 14:48:10 PDT
Created attachment 214887 [details]
WIP patch (v2)
Comment 10 Build Bot 2013-10-22 16:09:46 PDT
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
Comment 11 Build Bot 2013-10-22 16:09:49 PDT
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 12 Build Bot 2013-10-22 16:45:25 PDT
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
Comment 13 Build Bot 2013-10-22 16:45:27 PDT
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
Comment 14 Afonso Costa 2013-10-24 08:00:48 PDT
Created attachment 215064 [details]
WIP patch (v3)
Comment 15 Afonso Costa 2013-10-24 11:35:37 PDT
Created attachment 215084 [details]
Proposed patch
Comment 16 Alexey Proskuryakov 2013-10-24 13:29:40 PDT
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 17 Afonso Costa 2013-10-24 14:10:00 PDT
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?
Comment 18 Afonso Costa 2013-10-24 15:26:53 PDT
Created attachment 215110 [details]
Patch

Fixed patch according to Alexey's comments.
Comment 19 Alexey Proskuryakov 2013-10-24 16:16:11 PDT
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.
Comment 20 Afonso Costa 2013-10-28 06:56:19 PDT
(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.
Comment 21 Afonso Costa 2013-10-28 07:01:31 PDT
Created attachment 215302 [details]
Proposed patch (v2)

Fixed code comments according to Alexey's suggestion.
Comment 22 Afonso Costa 2013-10-28 08:27:01 PDT
Created attachment 215309 [details]
Proposed patch (v3)

Patch was rebased with branch master.
Comment 23 WebKit Commit Bot 2013-10-28 09:14:06 PDT
Comment on attachment 215309 [details]
Proposed patch (v3)

Clearing flags on attachment: 215309

Committed r158113: <http://trac.webkit.org/changeset/158113>
Comment 24 WebKit Commit Bot 2013-10-28 09:14:11 PDT
All reviewed patches have been landed.  Closing bug.