Bug 137906 – Don't create cached functions for HTMLDocument.write*()

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Michael Saboff]

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.

[Michael Saboff]

Created attachment 240172 [details]
Patch

[Darin Adler]

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.

[Michael Saboff]

(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*)"

[Michael Saboff]

Committed r174985: <http://trac.webkit.org/changeset/174985>

[Geoffrey Garen]

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.

[Michael Saboff]

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

[Darin Adler]

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

[Michael Saboff]

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

[Simon Fraser (smfr)]

I think this caused bug 138082.