Bug 137906 - Don't create cached functions for HTMLDocument.write*()
Summary: Don't create cached functions for HTMLDocument.write*()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 136724 136901 137907
  Show dependency treegraph
 
Reported: 2014-10-20 18:29 PDT by Michael Saboff
Modified: 2014-10-29 11:32 PDT (History)
3 users (show)

See Also:


Attachments
Patch (2.69 KB, patch)
2014-10-20 18:50 PDT, Michael Saboff
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2014-10-20 18:29:03 PDT
For the same reasons outline in <https://bugs.webkit.org/show_bug.cgi?id=137839> - "Don't create cached functions that access lexicalGlobalObject()", create uncached version of write() and writeln().  These function should have been part of that bug, but are handled here.
Comment 1 Michael Saboff 2014-10-20 18:50:50 PDT
Created attachment 240172 [details]
Patch
Comment 2 Darin Adler 2014-10-21 08:44:16 PDT
Comment on attachment 240172 [details]
Patch

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

> Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:69
> +    if (equal(propertyName.publicName(), "write")) {

Given that this idiom is a good one, I think we should add:

    bool operator==(PropertyName, const char*);

So we don't have to write this longer sequence at each call site. I also think it’s possibly slightly more efficient to save one branch and use uid() instead of publicName() since the empty uniques would never compare equal to a non-empty string. But that's the kind of thing that I would like the person writing the override to worry about rather than having to worry about it at each call site.
Comment 3 Michael Saboff 2014-10-21 08:52:03 PDT
(In reply to comment #2)
> Comment on attachment 240172 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240172&action=review
> 
> > Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:69
> > +    if (equal(propertyName.publicName(), "write")) {
> 
> Given that this idiom is a good one, I think we should add:
> 
>     bool operator==(PropertyName, const char*);
> 
> So we don't have to write this longer sequence at each call site. I also
> think it’s possibly slightly more efficient to save one branch and use uid()
> instead of publicName() since the empty uniques would never compare equal to
> a non-empty string. But that's the kind of thing that I would like the
> person writing the override to worry about rather than having to worry about
> it at each call site.

Thanks for the review.

I'll make the suggested changes, including using uid() in another patch.  Tracked in <https://bugs.webkit.org/show_bug.cgi?id=137925> - "Add operator==(PropertyName, char*)"
Comment 4 Michael Saboff 2014-10-21 10:19:45 PDT
Committed r174985: <http://trac.webkit.org/changeset/174985>
Comment 5 Geoffrey Garen 2014-10-21 10:47:02 PDT
This is the kind of subtle change where the test case is more valuable than the code change. Please add a test for this, or note in the ChangeLog which tests already cover this.
Comment 6 Michael Saboff 2014-10-21 11:12:02 PDT
(In reply to comment #5)
> This is the kind of subtle change where the test case is more valuable than
> the code change. Please add a test for this, or note in the ChangeLog which
> tests already cover this.

This change is covered by the tests:
LayoutTests/http/tests/security/aboutBlank/security-context-grandchildren-write-lexical.html
LayoutTests/http/tests/security/aboutBlank/security-context-grandchildren-writeln-lexical.html
LayoutTests/http/tests/security/aboutBlank/security-context-write.html
LayoutTests/http/tests/security/aboutBlank/security-context-writeln.html
LayoutTests/http/tests/xmlhttprequest/request-from-popup.html
LayoutTests/http/tests/navigation/new-window-redirect-history.html
LayoutTests/http/tests/misc/window-open-then-write.html

I'll update the change log accordingly.
Comment 7 Darin Adler 2014-10-21 13:59:41 PDT
(In reply to comment #6)
> This change is covered by the tests:

Sorry, I don’t understand. Were those tests failing before? I didn’t see a patch to the expected results to expect success instead of failure.
Comment 8 Michael Saboff 2014-10-21 14:11:02 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > This change is covered by the tests:
> 
> Sorry, I don’t understand. Were those tests failing before? I didn’t see a
> patch to the expected results to expect success instead of failure.

This change was split out from <https://bugs.webkit.org/show_bug.cgi?id=137907> similar to how <https://bugs.webkit.org/show_bug.cgi?id=137839> was split out.  This change is needed for <https://bugs.webkit.org/show_bug.cgi?id=137907> or the tests listed above fail.
Comment 9 Simon Fraser (smfr) 2014-10-29 11:32:45 PDT
I think this caused bug 138082.