Bug 77493

Created attachment 126201 [details] Patch

Comment on attachment 126201 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126201&action=review > Source/WebCore/bindings/js/JSPopStateEventCustom.cpp:53 > + return jsHistory->get(exec, Identifier(exec, "state")); If the web page installs a getter on the history object, does this call that getter? (Seems like something we should test.) > Source/WebCore/bindings/v8/custom/V8PopStateEventCustom.cpp:55 > + return toV8(history).As<v8::Object>()->Get(v8::String::NewSymbol("state")); I would think you'd want to get the hidden property rather than the real property. The hidden property can't be manipulated by the web page where as the real property can.

Yeah, and i found other issues as well, so i'll make more tests and maybe rethink it a bit.

It seems the existing behavior for PopStateEvent.state is not totally correct, or at least different to the other browsers, due to history.back et al being asynchronous. Consider: <script> history.pushState(1, "", ""); history.pushState(2, "", ""); window.onpopstate = function (e) { alert(e.state); } history.back(); history.pushState(3, "", ""); </script> Webkit alerts 2, gecko alerts 1, presto doesn't even fire the event (pushState(3) cancels the navigation?) I'll try to find a better solution to the whole thing and spam some lists in the meantime to see what should happen here. In the meantime ignore the patch, it's very wrong.

Created attachment 126594 [details] Patch New try. The bindings code is a bit fragile, i wonder if the first approach in 76035 (keeping the ScriptValue in HistoryItem) wasn't cleaner after all.

(In reply to comment #2) > (From update of attachment 126201 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=126201&action=review > > > Source/WebCore/bindings/js/JSPopStateEventCustom.cpp:53 > > + return jsHistory->get(exec, Identifier(exec, "state")); > > If the web page installs a getter on the history object, does this call that getter? (Seems like something we should test.) > Apparently the attribute in JSC isn't configurable so there would be no way to set a custom getter, right? I modified it to call jsHistory->state() directly to be sure.

(In reply to comment #6) > (In reply to comment #2) > > (From update of attachment 126201 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=126201&action=review > > > > > Source/WebCore/bindings/js/JSPopStateEventCustom.cpp:53 > > > + return jsHistory->get(exec, Identifier(exec, "state")); > > > > If the web page installs a getter on the history object, does this call that getter? (Seems like something we should test.) > > > > Apparently the attribute in JSC isn't configurable so there would be no way to set a custom getter, right? I modified it to call jsHistory->state() directly to be sure. The History IDL says history.state is readonly. I don't think a page can install a custom getter.

Ping?

Comment on attachment 126594 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=126594&action=review This feels more complicated than it should be, but I don't immediately see a way to simplify it. Can we try for a version that doesn't use goto? > Source/WebCore/bindings/js/JSPopStateEventCustom.cpp:50 > + History* history; > + bool isSameState; Generally we like to declare variables at the same time as they're initialized. Here, we should move the History* history declaration to line 57 > Source/WebCore/bindings/js/JSPopStateEventCustom.cpp:54 > + goto saveResultAndReturnIt; There's got to be a better way to structure this code than to use goto. Perhaps use a helper function that returns the proper result?

Created attachment 127283 [details] Got rid of gotos

Ping again? :-)

Comment on attachment 127283 [details] Got rid of gotos View in context: https://bugs.webkit.org/attachment.cgi?id=127283&action=review > Source/WebCore/bindings/v8/custom/V8PopStateEventCustom.cpp:53 > + v8::Handle<v8::String> propertyName = v8::String::NewSymbol("state"); Why not use V8HiddenPropertyNames ?

Sorry for the delay in reviewing. One nit above.

(In reply to comment #12) > (From update of attachment 127283 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127283&action=review > > > Source/WebCore/bindings/v8/custom/V8PopStateEventCustom.cpp:53 > > + v8::Handle<v8::String> propertyName = v8::String::NewSymbol("state"); > > Why not use V8HiddenPropertyNames ? Wasn't aware of its existence, will use, thanks.

Created attachment 128324 [details] Use V8HiddenPropertyName

Comment on attachment 128324 [details] Use V8HiddenPropertyName One good way not to invalidate the previous r+ is as follows: - Manually change the "Reviewed by NOBODY" line to "Reviewed by Adam Barth" - Upload the patch by './webkit-patch upload ... --no-review --no-obsolete --request-commit -m "patch for landing"'

Comment on attachment 128324 [details] Use V8HiddenPropertyName Clearing flags on attachment: 128324 Committed r108596: <http://trac.webkit.org/changeset/108596>

All reviewed patches have been landed. Closing bug.