Bug 137906

Created attachment 240172 [details] Patch

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.

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

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

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.

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

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

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

I think this caused bug 138082.