Bug 76035

Created attachment 121987 [details] proposed patch

Comment on attachment 121987 [details] proposed patch 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?

Good point, i'll check.

Firefox caches the value indeed. Is there a preferred way to cache the value? Should i save the deserialized ScriptValue in the History object directly or are there other things i should take into consideration? (i saw some references to CachedAttribute in some IDLs, would that be of use here?) Pardon the seemingly obvious questions, i'm fairly new to webkit. :-)

(In reply to comment #2) > (From update of attachment 121987 [details]) > 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. > I think it's even more complicated than this. In addition to the requirement that "history.state == history.state" I think the identity has to match with the object in the pop state event. I think the following JS code is valid: // Create a new object. var stateObject = new Array; // Use it as the state object in a replaceState. This clones the object. history.replaceState(stateObject, null, null); // Since the actual history.state is a structured clone, it should not match our original object. ASSERT(history.state != stateObject); // Now let's refetch a copy of history.state to store; stateObject = history.state; // Now let's do a pushstate to add a history entry. history.pushState(null, null, null); // Now let's go back to our original history entry which has a state object that we've stored a reference to already. history.back(); // Now a handler for the popstate event. onpopstate = function(event) { // Our stored reference to stateObject should match the current state object. ASSERT(history.state == stateObject); // AND our stored reference to stateObject should match the state object in this popstate event. ASSERT(event.state == stateObject); // For good measure, the event's state object and the current state object should also both match. ASSERT(event.state == history.state); }

(In reply to comment #5) > I think it's even more complicated than this. In addition to the requirement that "history.state == history.state" I think the identity has to match with the object in the pop state event. I think the following JS code is valid: > ... By the way, my suggestion to make that all happen is to not only store the structured clone on the HistoryItem, but to also store a create-on-read deserialized version of the cloned object on the HistoryItem... then all access to the state object goes through the HistoryItem. My instinct says it's possible either WebKit2's out of process history or Chrome's out of process history will cause a hiccup here. I can't say for sure without inspecting code a little closer which I don't have time to do right now.

(In reply to comment #5) > I think it's even more complicated than this. In addition to the requirement that "history.state == history.state" I think the identity has to match with the object in the pop state event. I think the following JS code is valid: Here's an updated snippet of code with slight tweaks: // Create a new object. var stateObject = new Array; // Use it as the state object in a replaceState. This clones the object. history.replaceState(stateObject, null, null); // Since the actual history.state is a structured clone, it should not match our original object. ASSERT(history.state != stateObject); // Now let's refetch a copy of history.state to store; stateObject = history.state; // Our reference and the history.state itself should be the same. This is now Adam's original assertion. ASSERT(stateObject == history.state); // Now let's do a pushstate to add a history entry. history.pushState(null, null, null); // Now add a handler for the popstate event. onpopstate = function(event) { // Our stored reference to stateObject should match the current state object. ASSERT(history.state == stateObject); // AND our stored reference to stateObject should match the state object in this popstate event. ASSERT(event.state == stateObject); // For good measure, the event's state object and the current state object should also both match. ASSERT(event.state == history.state); } // Now let's go back to our original history entry which has a state object that we've stored a reference to already. // This will fire our popstate event handler above. history.back();

> By the way, my suggestion to make that all happen is to not only store the structured clone on the HistoryItem, but to also store a create-on-read deserialized version of the cloned object on the HistoryItem... then all access to the state object goes through the HistoryItem. That makes sense to me. > My instinct says it's possible either WebKit2's out of process history or Chrome's out of process history will cause a hiccup here. I can't say for sure without inspecting code a little closer which I don't have time to do right now. Let's be careful about that in reviewing the patch. Pablo, do you want to try Brady's approach?

Yeah, i'll give it a shot and shout if i need some help. Any pointers as to where to look for potential issues with the out-of-process stuff?

(In reply to comment #9) > Yeah, i'll give it a shot and shout if i need some help. > > Any pointers as to where to look for potential issues with the out-of-process stuff? I would suggest to not worry about it for now. If OOP stuff is actually a hiccup, it will be an additional task on top of the WebCore work so it shouldn't effect how you start out. Just get a layout test going that covers what you covered before and my new code snippet, then hack WebCore till it works. We'll revisit OOP later.

In the callback: ASSERT(history.state == stateObject); No, because history.state is a new clone, and stateObject an old clone. ASSERT(event.state == stateObject); No, same reason (same two clones as the previous assert). See "traverse the history", step 10, and notice how that variable is used in steps 11 and 14. The others look right.

(In reply to comment #11) > In the callback: > > ASSERT(history.state == stateObject); > > No, because history.state is a new clone, and stateObject an old clone. > > ASSERT(event.state == stateObject); > > No, same reason (same two clones as the previous assert). > > See "traverse the history", step 10, and notice how that variable is used in steps 11 and 14. > > The others look right. I see my search for "structured clone" in the spec failed! I was also kind of surprised that it wasn't a new structured clone. This makes more sense. Updated code snippet: // Create a new object. var stateObject = new Array; // Use it as the state object in a replaceState. This clones the object. history.replaceState(stateObject, null, null); // Since the actual history.state is a structured clone, it should not match our original object. ASSERT(history.state != stateObject); // Now let's refetch a copy of history.state to store; stateObject = history.state; // Our reference and the history.state itself should be the same. This is now Adam's original assertion. ASSERT(stateObject == history.state); // Now let's do a pushstate to add a history entry. history.pushState(null, null, null); // Now add a handler for the popstate event. onpopstate = function(event) { // The event's state object and the current state object should match. ASSERT(event.state == history.state); // Our stored reference to stateObject will not match the current state object, as it is a structured clone of the history item's state object. ASSERT(history.state != stateObject); // Our stored reference to stateObject will not match the state object in this pop state event, as it is the same as history.state which is a structured clone of the history item's state object. ASSERT(event.state != stateObject); } // Now let's go back to our original history entry which has a state object that we've stored a reference to already. // This will fire our popstate event handler above. history.back();

I'm not sure create-on-read is fully feasible. I'm looking at the PopStateEvent part and it doesn't get the state object directly from the HistoryItem, it gets the serialized state through its constructor when it's created, eventually passed down from FrameLoader and Document, which in some cases save the value in m_pendingStateObject and dispatch the event after some things happen (document finishes loading, etc). Would it be safe to keep a frame in the event instead and get the state directly from the HistoryItem? I suppose, for starters, {FrameLoader,Document}::m_pendingStateObject should always be the same as currentItem()->stateObject(). Are there any other potential issues? for instance, can the event survive a history navigation and then get a different state object via currentItem()->stateObject()?. (And might there be race conditions in OOP between the points where you get the state object, serialize it and store that back in the HistoryItem?) Should we just save both serialized and deserialized values from the get go (useless memory usage, i guess)?, or just serialize to check for errors, and just keep and pass around the deserialized one? Any thoughts/ideas?

Maybe we should tackle the PopStateEvent aspects in a second patch? If we focus on history.state first, it sounds like create-on-read should be possible.

(In reply to comment #14) > Maybe we should tackle the PopStateEvent aspects in a second patch? If we focus on history.state first, it sounds like create-on-read should be possible. Agreed. I think create-on-read will be possible for both, but that the PopStateEvent case is more complicated. Don't let it hold you up. Since we're basically adding a new feature it really shouldn't be controversial to do it in stages.

Created attachment 122471 [details] use only one deserialization for history.state

Comment on attachment 122471 [details] use only one deserialization for history.state View in context: https://bugs.webkit.org/attachment.cgi?id=122471&action=review Good first swipe, but I think there's some straightforward ways to make it much cleaner. > Source/WebCore/bindings/js/JSHistoryCustom.cpp:181 > +JSValue JSHistory::state(ExecState* exec) const > +{ > + History* history = static_cast<History*>(impl()); > + > + ScriptValue state = history->state(); > + > + if (state.hasNoValue()) { > + state = ScriptValue(exec->globalData(), history->serializedState()->deserialize(exec, globalObject(), 0)); > + history->setState(state); > + } > + > + return state.jsValue(); > +} > + You're putting logic in the JS wrapper that belongs in the dom object itself (History.cpp) > Source/WebCore/bindings/v8/custom/V8HistoryCustom.cpp:58 > +v8::Handle<v8::Value> V8History::stateAccessorGetter(v8::Local<v8::String> name, const v8::AccessorInfo& info) > +{ > + History* history = V8History::toNative(info.Holder()); > + > + ScriptValue state = history->state(); > + > + if (state.hasNoValue()) { > + state = ScriptValue(history->serializedState()->deserialize()); > + history->setState(state); > + } > + > + return state.v8Value(); > +} > + And as a result, you have to dupe it here. > Source/WebCore/history/HistoryItem.cpp:472 > void HistoryItem::setStateObject(PassRefPtr<SerializedScriptValue> object) > { > m_stateObject = object; > + clearDeserializedStateObject(); > +} > + > +void HistoryItem::setDeserializedStateObject(ScriptValue& object) > +{ > + m_deserializedStateObject = object; > +} > + > +void HistoryItem::clearDeserializedStateObject() > +{ > + m_deserializedStateObject = ScriptValue(); > } HistoryItem itself should be the keeper of the deserializedStateObject. All it needs is a pointer to that object and a single new method ::deserializedStateObject() That method is what will create-on-read the ScriptValue. > Source/WebCore/page/History.cpp:120 > +SerializedScriptValue* History::serializedState() const > +{ > + if (m_frame) { > + HistoryItem* historyItem = m_frame->loader()->history()->currentItem(); > + if (historyItem && historyItem->stateObject()) > + return historyItem->stateObject(); > + } > + > + return SerializedScriptValue::nullValue(); > +} You don't need this. > Source/WebCore/page/History.cpp:131 > +ScriptValue History::state() const > +{ > + if (m_frame) { > + HistoryItem* historyItem = m_frame->loader()->history()->currentItem(); > + if (historyItem) > + return historyItem->deserializedStateObject(); > + } > + > + return ScriptValue(); > +} This can be as simple as if (m_frame) { if (HistoryItem* item = m_frame->loader()->history()->currentItem()) return item->deserializedStateObject(); } return ScriptValue(); > Source/WebCore/page/History.cpp:141 > +void History::setState(ScriptValue& object) > +{ > + if (m_frame) { > + HistoryItem* historyItem = m_frame->loader()->history()->currentItem(); > + if (historyItem) > + historyItem->setDeserializedStateObject(object); > + } > + > +} You don't need this. > Source/WebCore/page/History.idl:40 > + readonly attribute [CustomGetter] DOMObject state; This really doesn't need to be custom. The create-on-read logic should be in History/HistoryItem code, and you can use a generated wrapper.

> > Source/WebCore/page/History.idl:40 > > + readonly attribute [CustomGetter] DOMObject state; > > This really doesn't need to be custom. The create-on-read logic should be in History/HistoryItem code, and you can use a generated wrapper. If you need a ScriptState to deserialize the SerializedScriptValue, you should be able to use the [CallWith=ScriptState] attribute in the IDL. (Hopefully that works with attributes.)

Created attachment 122518 [details] new try based on brady's suggestions

I didn't see a way to set a value on a ScriptValue after creation, so i went with a reference instead of a pointer, if you prefer some other way, let me know. Also, there isn't support for CallWith on attributes in the generators so i went with a JSCCustomGetter that just passes off the ScriptState, i can try to add support for CallWith for attributes if you prefer, but perl isn't one of my strong suits, so ymmv :-)

Comment on attachment 122518 [details] new try based on brady's suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=122518&action=review > Source/WebCore/page/History.cpp:112 > +ScriptValue History::state(ScriptState* exec) const exec -> scriptState > Source/WebCore/page/History.cpp:114 > + if (m_frame) { Prefer early exist. You can just say: if (!m_frame) return ScriptValue(); etc > Source/WebCore/page/History.cpp:116 > + ScriptValue& state = historyItem->deserializedStateObject(); This is kind of subtle given that you're assign to a non-const reference. Maybe you should have historyItem do this work? > Source/WebCore/page/History.cpp:123 > +#if USE(V8) > + state = ScriptValue(historyItem->stateObject()->deserialize()); > +#else > + ASSERT(exec); > + state = ScriptValue::deserialize(exec, historyItem->stateObject()); > +#endif I would just add a ScriptValue::deserialize to the V8 version of SerializedScriptValue that accepts a ScriptState so you don't need this ifdef.

Comment on attachment 122518 [details] new try based on brady's suggestions Attachment 122518 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11157791

Comment on attachment 122518 [details] new try based on brady's suggestions View in context: https://bugs.webkit.org/attachment.cgi?id=122518&action=review >> Source/WebCore/page/History.cpp:116 >> + ScriptValue& state = historyItem->deserializedStateObject(); > > This is kind of subtle given that you're assign to a non-const reference. > > Maybe you should have historyItem do this work? Not maybe - definitely!

(In reply to comment #20) > I didn't see a way to set a value on a ScriptValue after creation, so i went with a reference instead of a pointer, if you prefer some other way, let me know. The references kinda muck up the code and make HistoryItem bigger than it needs to be. An alternate suggestions is OwnPtr<ScriptValue> and use "new ScriptValue" when it's time to create it. > Also, there isn't support for CallWith on attributes in the generators so i went with a JSCCustomGetter that just passes off the ScriptState, i can try to add support for CallWith for attributes if you prefer, but perl isn't one of my strong suits, so ymmv :-) We should add support for CallWith in attributes. It might come in handy in the future, and it is something that is easily scriptable in principle. The JSCustom* code should be used only for truly one-off squirrley behavior that can't be scripted.

Created attachment 122586 [details] add [CallWith] support for attributes and cleanup from review comments

Comment on attachment 122586 [details] add [CallWith] support for attributes and cleanup from review comments Attachment 122586 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11252064 New failing tests: fast/loader/stateobjects/pushstate-state-attribute-object-types.html fast/dom/Window/window-appendages-cleared.html fast/loader/stateobjects/pushstate-state-attribute-only-one-deserialization.html

Comment on attachment 122586 [details] add [CallWith] support for attributes and cleanup from review comments Attachment 122586 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11135077

I guess an EmptyScriptState won't do, how do i get a proper one?

@haraken: Would you be willing to look at the CodeGenerator parts of this change? @Pablo: You might consider making the code generator parts of this change in a separate patch. It will make reviewing easier. (We can use run-bindings-tests to test that part of the change in isolation.)

Comment on attachment 122586 [details] add [CallWith] support for attributes and cleanup from review comments View in context: https://bugs.webkit.org/attachment.cgi?id=122586&action=review > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2039 > + push(@implContent, " ScriptExecutionContext* scriptContext = static_cast<JSDOMGlobalObject*>(exec->lexicalGlobalObject())->scriptExecutionContext();\n"); Don't we need to check if the returned scriptContext is valid or not? > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2042 > + } Could you use GenerateAttributeCallWith? > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:908 > + my $callWithArg; This declaration can be inside the following if block. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:945 > + if ($hasScriptState) { Maybe we can clean up the code by writing '$callWith eq "ScriptState"' here, and remove $hasScriptState. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1142 > + my $hasScriptState = 0; Remove this. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1175 > + my $callWithArg; This declaration can be inside the following if block. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1179 > + unshift(@arguments, $callWithArg); push here, instead of unshift. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1181 > + if ($callWith eq "ScriptState") { push(@implContentDecls, " if (state.hadException())\n"); ...; } is necessary, isn't it? > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1493 > + push(@$outputArray, " EmptyScriptState state;\n"); Nit: Maybe rename to ScriptStateHolder, or just ScriptState. EmptyScriptState might be confusing. (I know other places are using EmptyScriptState though) It's up to you. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1500 > + push(@$outputArray, " return v8::Undefined();\n"); I am not sure but do you really want to skip this return for setters?

Created attachment 122857 [details] [CallWith] support for attributes in jsc/v8 generators

Attachment 122857 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:1110: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:1123: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:1137: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:1154: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:1173: Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html). [readability/pass_ptr] [5] Source/WebCore/bindings/scripts/test/GObject/WebKitDOMTestObj.cpp:1190: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 6 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 122860 [details] history.state part, reworked

Comment on attachment 122860 [details] history.state part, reworked View in context: https://bugs.webkit.org/attachment.cgi?id=122860&action=review I'll look more in depth tomorrow, but a few surface comments before I head home for the night. > Source/WebCore/history/HistoryItem.cpp:473 > + else { > + ScriptValue deserializedStateObject = ScriptValue::deserialize(scriptState, m_stateObject.get()); > + m_deserializedStateObject = adoptPtr(new ScriptValue(deserializedStateObject)); > + } Really a shame ScriptValue's can't be more easily deserialized to the heap. You *could* overload ScriptValue::deserialize to add a, OwnPtr<> version but I won't request that for this patch. An alternate here would be: m_deserializedStateObject = adoptPtr(new ScriptValue(ScriptValue::deserialize(scriptState, m_stateObject.get()))); con: looks a bit uglier pro: don't have to have faith that the compiler will optimize out the unnecessary temporary > Source/WebCore/page/History.h:32 > +#include "ScriptState.h" > +#include "ScriptValue.h" Have to include ScriptValue.h, but can forward declare `class ScriptState`

Comment on attachment 122860 [details] history.state part, reworked Attachment 122860 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/11196484

Comment on attachment 122860 [details] history.state part, reworked Attachment 122860 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11211426

(In reply to comment #34) > (From update of attachment 122860 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122860&action=review > > > Source/WebCore/page/History.h:32 > > +#include "ScriptState.h" > > +#include "ScriptValue.h" > > Have to include ScriptValue.h, but can forward declare `class ScriptState` ScriptState is a typedef for JSC, so i can't forward declare. I should probably add the generator changes to the history.state patch as well so all the ews build don't fail, right?

Comment on attachment 122857 [details] [CallWith] support for attributes in jsc/v8 generators View in context: https://bugs.webkit.org/attachment.cgi?id=122857&action=review r+ for the CodeGenerator change. > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2037 > + my $callWithArg = GenerateAttributeCallWith($callWith, \@implContent, 1); > + unshift(@arguments, $callWithArg); Nit: You can just write unshift(@arguments, GenerateAttributeCallWith($callWith, \@implContent, 1)); > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:2396 > + my $returnVoid = shift || 0; Nit: || 0 is not necessary > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:902 > + my $callWith = $attribute->signature->extendedAttributes->{"CallWith"} || ""; Nit: || "" is not necessary. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:909 > + my $callWithArg = GenerateAttributeCallWith($callWith, \@implContentDecls); > + push(@arguments, $callWithArg); Nit: You can just write push(@arguments, GenerateAttributeCallWith($callWith, \@implContentDecls)); > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1145 > + my $callWith = $attribute->signature->extendedAttributes->{"CallWith"} || ""; Nit: || "" is not necessary. > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1174 > + my $callWithArg = GenerateAttributeCallWith($callWith, \@implContentDecls, 1); > + push(@arguments, $callWithArg); Nit: You can just write push(@arguments, GenerateAttributeCallWith($callWith, \@implContentDecls, 1)); > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:1488 > + my $returnVoid = shift || 0; Nit: || 0 is not necessary.

(In reply to comment #38) > (From update of attachment 122857 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122857&action=review > > r+ for the CodeGenerator change. > > > Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:902 > > + my $callWith = $attribute->signature->extendedAttributes->{"CallWith"} || ""; > > Nit: || "" is not necessary. I get a whole bunch of warnings like "Use of uninitialized value $callWith in string eq at CodeGeneratorV8.pm" if i don't do that one, maybe i'm doing something wrong? (i seem to recall the same thing happening for the other ones, but they don't appear anymore, weird).

Comment on attachment 122857 [details] [CallWith] support for attributes in jsc/v8 generators View in context: https://bugs.webkit.org/attachment.cgi?id=122857&action=review >>> Source/WebCore/bindings/scripts/CodeGeneratorV8.pm:902 >>> + my $callWith = $attribute->signature->extendedAttributes->{"CallWith"} || ""; >> >> Nit: || "" is not necessary. > > I get a whole bunch of warnings like "Use of uninitialized value $callWith in string eq at CodeGeneratorV8.pm" if i don't do that one, maybe i'm doing something wrong? (i seem to recall the same thing happening for the other ones, but they don't appear anymore, weird). Ah, you're right. The warning appears because "$callWith eq ..." is used at the line 942. Then let's keep it as-is.

Comment on attachment 122860 [details] history.state part, reworked Attachment 122860 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11282044

(In reply to comment #33) > Created an attachment (id=122860) [details] > history.state part, reworked Maybe you can split this bug into two bugs, one for implementing the [CallWith] IDL in CodeGenerators, and the other for the history.state.

(In reply to comment #42) > (In reply to comment #33) > > Created an attachment (id=122860) [details] [details] > > history.state part, reworked > > Maybe you can split this bug into two bugs, one for implementing the [CallWith] IDL in CodeGenerators, and the other for the history.state. Yah, I think we should do that. File a new bug for the [CallWith] work and have it block this bug.

Comment on attachment 122860 [details] history.state part, reworked Attachment 122860 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11283089

Comment on attachment 122857 [details] [CallWith] support for attributes in jsc/v8 generators Filed https://bugs.webkit.org/show_bug.cgi?id=76517 for the [CallWith] changes.

Comment on attachment 122860 [details] history.state part, reworked View in context: https://bugs.webkit.org/attachment.cgi?id=122860&action=review Thanks. This is looking great! > Source/WebCore/history/HistoryItem.cpp:465 > +ScriptValue* HistoryItem::getOrCreateDeserializedStateObject(ScriptState* scriptState) This function could be made slightly more elegant. If you want to iterate once more, here are some changes I would make. (If you've had enough iterations, this version is fine too.) > Source/WebCore/history/HistoryItem.cpp:467 > + if (!m_deserializedStateObject) { WebKit prefers early return. So you can just say: if (m_deserializedStateObject) return m_deserializedStateObject.get(); and save indent space. > Source/WebCore/history/HistoryItem.cpp:472 > + ScriptValue deserializedStateObject = ScriptValue::deserialize(scriptState, m_stateObject.get()); > + m_deserializedStateObject = adoptPtr(new ScriptValue(deserializedStateObject)); If you take Brady's suggestion, that makes this stanza one line as well, which means you can use the ternary operator: m_deserializedStateObject= m_stateObject ? adoptPtr(new ScriptValue(ScriptValue::null(scriptState))) : adoptPtr(new ScriptValue(ScriptValue::null(scriptState))); > Source/WebCore/page/History.cpp:118 > + I would skip this blank line.

Created attachment 123150 [details] review comments fixed

Comment on attachment 123150 [details] review comments fixed View in context: https://bugs.webkit.org/attachment.cgi?id=123150&action=review > Source/WebCore/page/History.idl:40 > + readonly attribute [CallWith=ScriptState] DOMObject state; Why isn't this SerializedScriptValue as we do for message.data? (I'm genuinely curious what the different required semantic is)

Comment on attachment 123150 [details] review comments fixed View in context: https://bugs.webkit.org/attachment.cgi?id=123150&action=review I would r+ now based on the premise that the comment is just obsolete and accidentally left behind. But we also need to answer Oliver's question. > LayoutTests/fast/loader/stateobjects/pushstate-state-attribute-only-one-deserialization.html:47 > + shouldBeTrue("event.state === history.state"); // This fails for now, needs to be fixed. Does this still fail or is this just an obsolete comment?

(In reply to comment #49) > (From update of attachment 123150 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123150&action=review > > I would r+ now based on the premise that the comment is just obsolete and accidentally left behind. But we also need to answer Oliver's question. > > > LayoutTests/fast/loader/stateobjects/pushstate-state-attribute-only-one-deserialization.html:47 > > + shouldBeTrue("event.state === history.state"); // This fails for now, needs to be fixed. > > Does this still fail or is this just an obsolete comment? Still fails, i'll look at the PopStateEvent issue in a later bug (per comments #14 / #15). Do you want me to get rid of that line?

(In reply to comment #50) > (In reply to comment #49) > > (From update of attachment 123150 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=123150&action=review > > > > I would r+ now based on the premise that the comment is just obsolete and accidentally left behind. But we also need to answer Oliver's question. > > > > > LayoutTests/fast/loader/stateobjects/pushstate-state-attribute-only-one-deserialization.html:47 > > > + shouldBeTrue("event.state === history.state"); // This fails for now, needs to be fixed. > > > > Does this still fail or is this just an obsolete comment? > > Still fails, i'll look at the PopStateEvent issue in a later bug (per comments #14 / #15). Do you want me to get rid of that line? Oh duh. We should have a separate bug filed about that specific issue. Oliver and I have been chatting on IRC and neither of us is convinced one way or the other that DOMObject or SerializedScriptValue is necessarily the right way to go. We want to talk it over in person to make sure we're on the same page. Assuming there's no pressing need to get this in RIGHT NOW, let's hold off for now.

There's no immediate need to land this, but wouldn't using SerializedScriptValue take us back to the original problem? Namely, that we'd be deserializing each time and history.state === history.state would never be true? Or am i missing something about the inner workings of SerializedScriptValue in idl code?

(In reply to comment #52) > There's no immediate need to land this, but wouldn't using SerializedScriptValue take us back to the original problem? Namely, that we'd be deserializing each time and history.state === history.state would never be true? Or am i missing something about the inner workings of SerializedScriptValue in idl code? That's my understanding as well but Oliver isn't convinced. We'll clear it up tomorrow when we can be in the same room with a white board and markers. :)

You could just slap the CachedAttribute attribute on the property and then (provided you're not doing any custom getters or anything the caching should automatic and correct) with SerislizedScriptValue

(In reply to comment #54) > You could just slap the CachedAttribute attribute on the property and then (provided you're not doing any custom getters or anything the caching should automatic and correct) with SerislizedScriptValue How could that guarantee that the object returned from both PopStateEvent.state and History.state are the same?

Comment on attachment 123150 [details] review comments fixed Attachment 123150 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11169996 New failing tests: fast/dom/Window/window-appendages-cleared.html

(In reply to comment #55) > (In reply to comment #54) > > You could just slap the CachedAttribute attribute on the property and then (provided you're not doing any custom getters or anything the caching should automatic and correct) with SerislizedScriptValue > > How could that guarantee that the object returned from both PopStateEvent.state and History.state are the same? Admittedly, this patch doesn't even do that yet. But it's something we'll need to fix. Let's just talk tomorrow.

Comment on attachment 123150 [details] review comments fixed Attachment 123150 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11247171

*** Bug 70876 has been marked as a duplicate of this bug. ***

> Let's just talk tomorrow. Any further thoughts?

(In reply to comment #60) > > Let's just talk tomorrow. > > Any further thoughts? Thanks for reminding us! Oliver tried to track me down today but it was at a busy time. We'll figure this out tomorrow.

Oliver and I talked it over. Here's how we thing we should make this work. - Currently the attribute on PopStateEvent.idl is a DOMObject. This is wrong and it needs to be a SerializedScriptValue. - The new attribute should not be a DOMObject. It needs to be a SerializedScriptValue. - We need to use the IDL attributes "CachedAttribute, CustomGetter". CachedAttribute is what makes the JS wrapper for the SerializedScriptValue cached so it hangs around. - We need to change PopStateEvent.h/cpp to be created with a History object, or at the very least a Frame object. - JSPopStateEvent::state will grab the relevant History object from the PopStateEvent implementationg, do a cached lookup for the JSHistory wrapper of that History, and just call its JSHistory::state. Oliver, did I get the plan right?

(In reply to comment #62) > Oliver and I talked it over. Here's how we thing we should make this work. > > - Currently the attribute on PopStateEvent.idl is a DOMObject. This is wrong and it needs to be a SerializedScriptValue. > - The new attribute should not be a DOMObject. It needs to be a SerializedScriptValue. > - We need to use the IDL attributes "CachedAttribute, CustomGetter". CachedAttribute is what makes the JS wrapper for the SerializedScriptValue cached so it hangs around. It gives your a correctly marked field to store the property in -- m_state (or m_whateverTheAttributeIs) > - We need to change PopStateEvent.h/cpp to be created with a History object, or at the very least a Frame object. > - JSPopStateEvent::state will grab the relevant History object from the PopStateEvent implementationg, do a cached lookup for the JSHistory wrapper of that History, and just call its JSHistory::state. toJS(callFrame, whateverMyHistoryObject) should give you the required foo. > > Oliver, did I get the plan right? Yup

Alright, i'll give that plan a shot this week, thanks!

Created attachment 124811 [details] First attempt at a full proper solution

Comment on attachment 124811 [details] First attempt at a full proper solution Attachment 124811 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11393096

Comment on attachment 124811 [details] First attempt at a full proper solution View in context: https://bugs.webkit.org/attachment.cgi?id=124811&action=review r- due to removed PopStateEvent constructor. It is good to show the whole image of your intended change, but it might be better to split the big patch into sub-patches for review, so that each sub-patch is small enough to confirm that it is correct. > Source/WebCore/dom/PopStateEvent.idl:31 > + readonly attribute [CustomGetter] DOMObject state; Why did you remove [ConstructorTemplate=Event] and [InitializedByConstructor]? Then I am afraid that "new PopStateEvent()" wouldn't work.

Ok, let me make a new bug for the popstate stuff to make things a bit easier. (In reply to comment #67) > (From update of attachment 124811 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124811&action=review > > r- due to removed PopStateEvent constructor. > > It is good to show the whole image of your intended change, but it might be better to split the big patch into sub-patches for review, so that each sub-patch is small enough to confirm that it is correct. > > > Source/WebCore/dom/PopStateEvent.idl:31 > > + readonly attribute [CustomGetter] DOMObject state; > > Why did you remove [ConstructorTemplate=Event] and [InitializedByConstructor]? Then I am afraid that "new PopStateEvent()" wouldn't work. I might've been too eager in removing stuff wrt ConstructorTemplate, i'll put it back in. Now that the state is taken directly from the history object how would InitializedByConstructor work in this case?

(In reply to comment #68) > I might've been too eager in removing stuff wrt ConstructorTemplate, i'll put it back in. Now that the state is taken directly from the history object how would InitializedByConstructor work in this case? I am still not sure why we need to (or whether we should) take |state| directly from the history object. The spec (http://dev.w3.org/html5/spec/Overview.html#popstateevent) says that |state| is a readonly attribute of PopStateEvent and "The |state| attribute must return the value it was initialized to". So I think it is natural to keep |state| (not in a history object but) in a PopStateEvent object. If you want to cache the deserialized |state| somewhere for performance, you can cache it in the PopStateEvent object.

(In reply to comment #69) > (In reply to comment #68) > > I might've been too eager in removing stuff wrt ConstructorTemplate, i'll put it back in. Now that the state is taken directly from the history object how would InitializedByConstructor work in this case? > > I am still not sure why we need to (or whether we should) take |state| directly from the history object. The spec (http://dev.w3.org/html5/spec/Overview.html#popstateevent) says that |state| is a readonly attribute of PopStateEvent and "The |state| attribute must return the value it was initialized to". So I think it is natural to keep |state| (not in a history object but) in a PopStateEvent object. If you want to cache the deserialized |state| somewhere for performance, you can cache it in the PopStateEvent object. See the discussion above on how PopStateEvent.state and history.state have to refer to the same object.

(In reply to comment #70) > (In reply to comment #69) > > (In reply to comment #68) > > See the discussion above on how PopStateEvent.state and history.state have to refer to the same object. Ah, I got it. > > > I might've been too eager in removing stuff wrt ConstructorTemplate, i'll put it back in. Now that the state is taken directly from the history object how would InitializedByConstructor work in this case? Then, I guess you need to write custom Event constructor. You can write bindings/js/JSPopStateEventConstructorCustom.cpp by customizing constructJSPopStateEvent() and fillPopStateEventInit() in WebKitBuild/Debug/DerivedSources/WebCore/JSPopStateEvent.cpp.

(In reply to comment #71) > (In reply to comment #70) > > (In reply to comment #69) > Then, I guess you need to write custom Event constructor. You can write bindings/js/JSPopStateEventConstructorCustom.cpp by customizing constructJSPopStateEvent() and fillPopStateEventInit() in WebKitBuild/Debug/DerivedSources/WebCore/JSPopStateEvent.cpp. The IDL would be interface PopStateEvent [ CustomConstructor ] : Event { readonly attribute [CustomGetter] DOMObject state; };

Created attachment 124856 [details] history.state

Comment on attachment 124856 [details] history.state View in context: https://bugs.webkit.org/attachment.cgi?id=124856&action=review r- due to history->stateChanged() > Source/WebCore/bindings/js/JSHistoryCustom.cpp:172 > + if (!cachedValue.isEmpty() && !history->stateChanged()) I guess this might be dangerous. What happens if another call path updates history.state? For example, (1) JSHistory::state() caches 1111 in |m_state|. (2) Another call path updates history.state to 2222. (3) Another call path calls History::state(), which returns 2222. (4) JSHistory::state() is called again. It calls history->stateChanged() and it returns false. Consequently, JSHistory::state() will return the cached 1111. > Source/WebCore/page/History.idl:40 > + readonly attribute [CachedAttribute, Custom] SerializedScriptValue state; [CachedAttribute] is not necessary, since the getter and setter are written as a custom getter and setter.

(In reply to comment #74) > (From update of attachment 124856 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124856&action=review > > r- due to history->stateChanged() > > > Source/WebCore/bindings/js/JSHistoryCustom.cpp:172 > > + if (!cachedValue.isEmpty() && !history->stateChanged()) > > I guess this might be dangerous. What happens if another call path updates history.state? For example, > > (1) JSHistory::state() caches 1111 in |m_state|. > (2) Another call path updates history.state to 2222. > (3) Another call path calls History::state(), which returns 2222. > (4) JSHistory::state() is called again. It calls history->stateChanged() and it returns false. Consequently, JSHistory::state() will return the cached 1111. Is the relationship between a History and a {JS,V8}History not 1-to-1? On what situation would there be multiple JSHistory objects using the same History object? Maybe i can make a test for that. > > Source/WebCore/page/History.idl:40 > > + readonly attribute [CachedAttribute, Custom] SerializedScriptValue state; > > [CachedAttribute] is not necessary, since the getter and setter are written as a custom getter and setter. [CachedAttribute] declares a m_state member that's used in the custom getter to store the value (in JSC).

Comment on attachment 124856 [details] history.state View in context: https://bugs.webkit.org/attachment.cgi?id=124856&action=review >>> Source/WebCore/bindings/js/JSHistoryCustom.cpp:172 >>> + if (!cachedValue.isEmpty() && !history->stateChanged()) >> >> I guess this might be dangerous. What happens if another call path updates history.state? For example, >> >> (1) JSHistory::state() caches 1111 in |m_state|. >> (2) Another call path updates history.state to 2222. >> (3) Another call path calls History::state(), which returns 2222. >> (4) JSHistory::state() is called again. It calls history->stateChanged() and it returns false. Consequently, JSHistory::state() will return the cached 1111. > > Is the relationship between a History and a {JS,V8}History not 1-to-1? On what situation would there be multiple JSHistory objects using the same History object? Maybe i can make a test for that. Sorry, the above example was wrong. But if PopStateEvent.state and history.state have to refer to the same object, isn't there any possibility like this? (sorry if I am still wrong) (1) history.state // JSHistory::state() caches 1111 in |m_state|. (2) history.pushState(2222, "foo") // history.state is updated to 2222. (3) popStateEvent.state // History::state() changes |m_lastStateObjectRequested| from 1111 to 2222. (4) history.state // history->stateChanged() returns false, and history.state returns 1111. >>> Source/WebCore/page/History.idl:40 >>> + readonly attribute [CachedAttribute, Custom] SerializedScriptValue state; >> >> [CachedAttribute] is not necessary, since the getter and setter are written as a custom getter and setter. > > [CachedAttribute] declares a m_state member that's used in the custom getter to store the value (in JSC). Makes sense.

(In reply to comment #76) > (From update of attachment 124856 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=124856&action=review > > >>> Source/WebCore/bindings/js/JSHistoryCustom.cpp:172 > >>> + if (!cachedValue.isEmpty() && !history->stateChanged()) > >> > >> I guess this might be dangerous. What happens if another call path updates history.state? For example, > >> > >> (1) JSHistory::state() caches 1111 in |m_state|. > >> (2) Another call path updates history.state to 2222. > >> (3) Another call path calls History::state(), which returns 2222. > >> (4) JSHistory::state() is called again. It calls history->stateChanged() and it returns false. Consequently, JSHistory::state() will return the cached 1111. > > > > Is the relationship between a History and a {JS,V8}History not 1-to-1? On what situation would there be multiple JSHistory objects using the same History object? Maybe i can make a test for that. > > Sorry, the above example was wrong. But if PopStateEvent.state and history.state have to refer to the same object, isn't there any possibility like this? (sorry if I am still wrong) > > (1) history.state // JSHistory::state() caches 1111 in |m_state|. > (2) history.pushState(2222, "foo") // history.state is updated to 2222. > (3) popStateEvent.state // History::state() changes |m_lastStateObjectRequested| from 1111 to 2222. > (4) history.state // history->stateChanged() returns false, and history.state returns 1111. In that particular case, pushState will reset the cached value (which is probably superfluous), but let's say it was history.back() or the user with the back button, JSPopStateEvent::state is going through JSHistory::state, not History::state directly, so if there's a 1-to-1 mapping, the only one calling History::state is JSHistory::state and everything should be fine (as far as i can see). In any case, it's a good observation, i can modify the deserialization test slightly to cover this, or make a new test to make it more explicit.

(In reply to comment #77) > In that particular case, pushState will reset the cached value (which is probably superfluous), but let's say it was history.back() or the user with the back button, JSPopStateEvent::state is going through JSHistory::state, not History::state directly, so if there's a 1-to-1 mapping, the only one calling History::state is JSHistory::state and everything should be fine (as far as i can see). Makes sense! Thanks for the clarification. > In any case, it's a good observation, i can modify the deserialization test slightly to cover this, or make a new test to make it more explicit. Good idea.

Comment on attachment 124856 [details] history.state Thanks for the patch!

Created attachment 125274 [details] Patch. Same patch as before, to get another EWS run

Comment on attachment 125274 [details] Patch. You can use "--no-review --no-obsolete" option on the ./webkit-patch command in order to avoid invalidating r+ that you got before.

Comment on attachment 125274 [details] Patch. Attachment 125274 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11423168

Comment on attachment 125274 [details] Patch. Attachment 125274 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11420223 New failing tests: fast/loader/stateobjects/state-attribute-object-types.html fast/dom/Window/window-appendages-cleared.html

I can't reproduce the chromium test failure here on my machine on a mac chromium port build... As to the gtk thing, i'm not sure what i should do, i see that, for instance, MessageEvent.idl only does the custom bindings stuff for js, should something like that be done here as well?

(In reply to comment #84) > I can't reproduce the chromium test failure here on my machine on a mac chromium port build... > > As to the gtk thing, i'm not sure what i should do, i see that, for instance, MessageEvent.idl only does the custom bindings stuff for js, should something like that be done here as well? Here are the results on my local Chromium Linux: http://haraken.info/null/window-appendages-cleared-diff.txt http://haraken.info/null/state-attribute-object-types-diff.txt Can you fix it?

With chromium I've found it easier to just commit and rebaseline results using garden-o-matic. The other bots are more problematic. We used to have the EWS bots upload failed test results, but we don't anymore, sadly.

(In reply to comment #85) > (In reply to comment #84) > > I can't reproduce the chromium test failure here on my machine on a mac chromium port build... > > > > As to the gtk thing, i'm not sure what i should do, i see that, for instance, MessageEvent.idl only does the custom bindings stuff for js, should something like that be done here as well? > > Here are the results on my local Chromium Linux: > > http://haraken.info/null/window-appendages-cleared-diff.txt > http://haraken.info/null/state-attribute-object-types-diff.txt > > Can you fix it? Yeah, i can fix those. Any thoughts on the gtk issues?

(In reply to comment #87) > > http://haraken.info/null/state-attribute-object-types-diff.txt > > > > Can you fix it? > > Yeah, i can fix those. Thanks, you can fix it so that the result won't depend on a time zone. > Any thoughts on the gtk issues? Maybe you need to modify CodeGeneratorGObject.pm so that it supports SerializedScriptValue with [CachedAttribute] IDL.

(In reply to comment #88) > > > Any thoughts on the gtk issues? > > Maybe you need to modify CodeGeneratorGObject.pm so that it supports SerializedScriptValue with [CachedAttribute] IDL. That looks non-trivial (at least for someone with zero knowledge of GObject, like me). I haven't even managed to build the gtk port on my mac. Might someone else be persuaded to add that support? or maybe there's some other temporary workaround that we could use for now?

(In reply to comment #89) > (In reply to comment #88) > > > > > Any thoughts on the gtk issues? > > > > Maybe you need to modify CodeGeneratorGObject.pm so that it supports SerializedScriptValue with [CachedAttribute] IDL. > > That looks non-trivial (at least for someone with zero knowledge of GObject, like me). I haven't even managed to build the gtk port on my mac. > > Might someone else be persuaded to add that support? or maybe there's some other temporary workaround that we could use for now? OK, I'll make a change in bug 78059, which might solve your issue.

> Might someone else be persuaded to add that support? or maybe there's some other temporary workaround that we could use for now? We could ifdef this feature not to be available in LANGUAGE_GOBJECT (or whatever the preprocessor variable is called).

(In reply to comment #90) > (In reply to comment #89) > > (In reply to comment #88) > > > > > > > Any thoughts on the gtk issues? > > > > > > Maybe you need to modify CodeGeneratorGObject.pm so that it supports SerializedScriptValue with [CachedAttribute] IDL. > > > > That looks non-trivial (at least for someone with zero knowledge of GObject, like me). I haven't even managed to build the gtk port on my mac. > > > > Might someone else be persuaded to add that support? or maybe there's some other temporary workaround that we could use for now? > > OK, I'll make a change in bug 78059, which might solve your issue. Thanks! I'll fix the tests and upload a new patch once that one has landed.

Created attachment 126022 [details] Fixed tests

Let's wait for the bot results:-)

All green this time :D

Comment on attachment 126022 [details] Fixed tests Clearing flags on attachment: 126022 Committed r107058: <http://trac.webkit.org/changeset/107058>

All reviewed patches have been landed. Closing bug.