[webkit-reviews] review denied: [Bug 76035] Add state attribute to history's dom interface. : [Attachment 121987] proposed patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Jan 11 00:33:53 PST 2012
Adam Barth <abarth at webkit.org> has denied Pablo Flouret <pablof at motorola.com>'s
request for review:
Bug 76035: Add state attribute to history's dom interface.
https://bugs.webkit.org/show_bug.cgi?id=76035
Attachment 121987: proposed patch
https://bugs.webkit.org/attachment.cgi?id=121987&action=review
------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=121987&action=review
Thanks for working on this bug. One issue below:
> Source/WebCore/bindings/js/JSHistoryCustom.cpp:170
> + History* history = static_cast<History*>(impl());
> + return history->stateObject()->deserialize(exec, globalObject(), 0);
I think this isn't quite right. Every time we call this getter, we're going to
deserialize the state object again. That means
history.state !== history.state
which doesn't seem to match the spec. If that's what other browsers do, then
we can change the spec, but it's more likely that we should cache the result of
deserializing the SerializedScriptValue and aways return the same value.
Would you be willing to test Firefox and/or IE to see whether they create a new
deserialization every time you call the getter?
More information about the webkit-reviews
mailing list