RESOLVED FIXED 77493
PopStateEvent.state should use the same object as history.state
https://bugs.webkit.org/show_bug.cgi?id=77493
Summary PopStateEvent.state should use the same object as history.state
Pablo Flouret
Reported 2012-01-31 16:30:52 PST
When state is added to history's dom interface, PopStateEvent should use that same deserialization for the state attribute.
Attachments
Patch (17.55 KB, patch)
2012-02-08 17:30 PST, Pablo Flouret
abarth: review-
Patch (20.49 KB, patch)
2012-02-10 15:31 PST, Pablo Flouret
no flags
Got rid of gotos (21.01 KB, patch)
2012-02-15 17:36 PST, Pablo Flouret
no flags
Use V8HiddenPropertyName (23.95 KB, patch)
2012-02-22 16:24 PST, Pablo Flouret
no flags
Pablo Flouret
Comment 1 2012-02-08 17:30:54 PST
Adam Barth
Comment 2 2012-02-08 17:37:55 PST
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.
Pablo Flouret
Comment 3 2012-02-08 22:40:42 PST
Yeah, and i found other issues as well, so i'll make more tests and maybe rethink it a bit.
Pablo Flouret
Comment 4 2012-02-09 11:12:20 PST
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.
Pablo Flouret
Comment 5 2012-02-10 15:31:09 PST
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.
Pablo Flouret
Comment 6 2012-02-10 15:33:30 PST
(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.
Brady Eidson
Comment 7 2012-02-10 15:40:05 PST
(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.
Pablo Flouret
Comment 8 2012-02-15 15:27:06 PST
Ping?
Adam Barth
Comment 9 2012-02-15 15:39:38 PST
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?
Pablo Flouret
Comment 10 2012-02-15 17:36:44 PST
Created attachment 127283 [details] Got rid of gotos
Pablo Flouret
Comment 11 2012-02-22 10:59:15 PST
Ping again? :-)
Adam Barth
Comment 12 2012-02-22 15:11:11 PST
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 ?
Adam Barth
Comment 13 2012-02-22 15:11:27 PST
Sorry for the delay in reviewing. One nit above.
Pablo Flouret
Comment 14 2012-02-22 15:17:16 PST
(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.
Pablo Flouret
Comment 15 2012-02-22 16:24:09 PST
Created attachment 128324 [details] Use V8HiddenPropertyName
Kentaro Hara
Comment 16 2012-02-22 16:29:32 PST
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"'
WebKit Review Bot
Comment 17 2012-02-22 19:05:39 PST
Comment on attachment 128324 [details] Use V8HiddenPropertyName Clearing flags on attachment: 128324 Committed r108596: <http://trac.webkit.org/changeset/108596>
WebKit Review Bot
Comment 18 2012-02-22 19:05:47 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.