Bug 76035 - Add state attribute to history's dom interface.
: Add state attribute to history's dom interface.
Status: RESOLVED FIXED
: WebKit
HTML DOM
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 76517 77165 77295
: 76198 77493
  Show dependency treegraph
 
Reported: 2012-01-10 23:47 PST by
Modified: 2012-02-08 02:39 PST (History)


Attachments
proposed patch (8.75 KB, patch)
2012-01-11 00:10 PST, Pablo Flouret
no flags Review Patch | Details | Formatted Diff | Diff
use only one deserialization for history.state (17.35 KB, patch)
2012-01-13 11:20 PST, Pablo Flouret
no flags Review Patch | Details | Formatted Diff | Diff
new try based on brady's suggestions (16.85 KB, patch)
2012-01-13 16:11 PST, Pablo Flouret
no flags Review Patch | Details | Formatted Diff | Diff
add [CallWith] support for attributes and cleanup from review comments (46.46 KB, patch)
2012-01-15 20:33 PST, Pablo Flouret
no flags Review Patch | Details | Formatted Diff | Diff
[CallWith] support for attributes in jsc/v8 generators (53.60 KB, patch)
2012-01-17 18:31 PST, Pablo Flouret
no flags Review Patch | Details | Formatted Diff | Diff
history.state part, reworked (18.75 KB, patch)
2012-01-17 18:40 PST, Pablo Flouret
no flags Review Patch | Details | Formatted Diff | Diff
review comments fixed (18.63 KB, patch)
2012-01-19 11:06 PST, Pablo Flouret
no flags Review Patch | Details | Formatted Diff | Diff
First attempt at a full proper solution (31.11 KB, patch)
2012-01-31 13:31 PST, Pablo Flouret
no flags Review Patch | Details | Formatted Diff | Diff
history.state (15.66 KB, patch)
2012-01-31 17:11 PST, Pablo Flouret
no flags Review Patch | Details | Formatted Diff | Diff
Patch. (15.66 KB, patch)
2012-02-03 00:03 PST, Pablo Flouret
no flags Review Patch | Details | Formatted Diff | Diff
Fixed tests (16.34 KB, patch)
2012-02-08 00:17 PST, Pablo Flouret
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


------- Comment #1 From 2012-01-11 00:10:44 PST -------
Created an attachment (id=121987) [details]
proposed patch
------- Comment #2 From 2012-01-11 00:33:54 PST -------
(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.

Would you be willing to test Firefox and/or IE to see whether they create a new deserialization every time you call the getter?
------- Comment #3 From 2012-01-11 00:36:12 PST -------
Good point, i'll check.
------- Comment #4 From 2012-01-11 12:06:21 PST -------
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. :-)
------- Comment #5 From 2012-01-11 12:53:07 PST -------
(In reply to comment #2)
> (From update of attachment 121987 [details] [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);
 }
------- Comment #6 From 2012-01-11 12:57:10 PST -------
(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.
------- Comment #7 From 2012-01-11 13:00:29 PST -------
(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();
------- Comment #8 From 2012-01-11 13:03:27 PST -------
> 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?
------- Comment #9 From 2012-01-11 13:06:02 PST -------
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?
------- Comment #10 From 2012-01-11 13:13:28 PST -------
(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.
------- Comment #11 From 2012-01-11 14:33:51 PST -------
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.
------- Comment #12 From 2012-01-11 14:53:34 PST -------
(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();
------- Comment #13 From 2012-01-12 14:45:19 PST -------
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?
------- Comment #14 From 2012-01-12 14:54:33 PST -------
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.
------- Comment #15 From 2012-01-12 15:39:00 PST -------
(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.
------- Comment #16 From 2012-01-13 11:20:15 PST -------
Created an attachment (id=122471) [details]
use only one deserialization for history.state
------- Comment #17 From 2012-01-13 11:48:26 PST -------
(From update of attachment 122471 [details])
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.
------- Comment #18 From 2012-01-13 12:24:38 PST -------
> > 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.)
------- Comment #19 From 2012-01-13 16:11:57 PST -------
Created an attachment (id=122518) [details]
new try based on brady's suggestions
------- Comment #20 From 2012-01-13 16:14:47 PST -------
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 #21 From 2012-01-13 16:34:32 PST -------
(From update of attachment 122518 [details])
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 #22 From 2012-01-13 16:43:54 PST -------
(From update of attachment 122518 [details])
Attachment 122518 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11157791
------- Comment #23 From 2012-01-13 16:50:58 PST -------
(From update of attachment 122518 [details])
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!
------- Comment #24 From 2012-01-13 17:00:09 PST -------
(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.
------- Comment #25 From 2012-01-15 20:33:29 PST -------
Created an attachment (id=122586) [details]
add [CallWith] support for attributes and cleanup from review comments
------- Comment #26 From 2012-01-15 21:15:26 PST -------
(From update of attachment 122586 [details])
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 #27 From 2012-01-16 00:01:29 PST -------
(From update of attachment 122586 [details])
Attachment 122586 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11135077
------- Comment #28 From 2012-01-16 00:09:24 PST -------
I guess an EmptyScriptState won't do, how do i get a proper one?
------- Comment #29 From 2012-01-17 01:09:35 PST -------
@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 #30 From 2012-01-17 02:32:16 PST -------
(From update of attachment 122586 [details])
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?
------- Comment #31 From 2012-01-17 18:31:56 PST -------
Created an attachment (id=122857) [details]
[CallWith] support for attributes in jsc/v8 generators
------- Comment #32 From 2012-01-17 18:34:56 PST -------
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.
------- Comment #33 From 2012-01-17 18:40:47 PST -------
Created an attachment (id=122860) [details]
history.state part, reworked
------- Comment #34 From 2012-01-17 18:52:26 PST -------
(From update of attachment 122860 [details])
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 #35 From 2012-01-17 19:15:53 PST -------
(From update of attachment 122860 [details])
Attachment 122860 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11196484
------- Comment #36 From 2012-01-17 19:21:20 PST -------
(From update of attachment 122860 [details])
Attachment 122860 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11211426
------- Comment #37 From 2012-01-17 20:41:43 PST -------
(In reply to comment #34)
> (From update of attachment 122860 [details] [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 #38 From 2012-01-17 20:45:08 PST -------
(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/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.
------- Comment #39 From 2012-01-17 21:02:15 PST -------
(In reply to comment #38)
> (From update of attachment 122857 [details] [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 #40 From 2012-01-17 21:07:53 PST -------
(From update of attachment 122857 [details])
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 #41 From 2012-01-17 21:12:38 PST -------
(From update of attachment 122860 [details])
Attachment 122860 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11282044
------- Comment #42 From 2012-01-17 21:14:50 PST -------
(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.
------- Comment #43 From 2012-01-17 22:49:00 PST -------
(In reply to comment #42)
> (In reply to comment #33)
> > Created an attachment (id=122860) [details] [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 #44 From 2012-01-17 22:53:45 PST -------
(From update of attachment 122860 [details])
Attachment 122860 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11283089
------- Comment #45 From 2012-01-17 23:07:27 PST -------
(From update of attachment 122857 [details])
Filed https://bugs.webkit.org/show_bug.cgi?id=76517 for the [CallWith] changes.
------- Comment #46 From 2012-01-18 23:46:14 PST -------
(From update of attachment 122860 [details])
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.
------- Comment #47 From 2012-01-19 11:06:11 PST -------
Created an attachment (id=123150) [details]
review comments fixed
------- Comment #48 From 2012-01-19 11:08:35 PST -------
(From update of attachment 123150 [details])
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 #49 From 2012-01-19 11:12:05 PST -------
(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?
------- Comment #50 From 2012-01-19 11:21:19 PST -------
(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?
------- Comment #51 From 2012-01-19 11:26:43 PST -------
(In reply to comment #50)
> (In reply to comment #49)
> > (From update of attachment 123150 [details] [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.
------- Comment #52 From 2012-01-19 11:40:53 PST -------
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?
------- Comment #53 From 2012-01-19 11:45:54 PST -------
(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.  :)
------- Comment #54 From 2012-01-19 12:09:10 PST -------
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
------- Comment #55 From 2012-01-19 12:25:32 PST -------
(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 #56 From 2012-01-19 12:26:50 PST -------
(From update of attachment 123150 [details])
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
------- Comment #57 From 2012-01-19 12:29:33 PST -------
(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 #58 From 2012-01-19 14:39:21 PST -------
(From update of attachment 123150 [details])
Attachment 123150 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11247171
------- Comment #59 From 2012-01-20 15:31:49 PST -------
*** Bug 70876 has been marked as a duplicate of this bug. ***
------- Comment #60 From 2012-01-24 21:44:15 PST -------
> Let's just talk tomorrow.

Any further thoughts?
------- Comment #61 From 2012-01-24 22:59:25 PST -------
(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.
------- Comment #62 From 2012-01-25 17:07:04 PST -------
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?
------- Comment #63 From 2012-01-25 17:09:33 PST -------
(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
------- Comment #64 From 2012-01-25 17:14:06 PST -------
Alright, i'll give that plan a shot this week, thanks!
------- Comment #65 From 2012-01-31 13:31:11 PST -------
Created an attachment (id=124811) [details]
First attempt at a full proper solution
------- Comment #66 From 2012-01-31 13:56:54 PST -------
(From update of attachment 124811 [details])
Attachment 124811 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11393096
------- Comment #67 From 2012-01-31 16:06:00 PST -------
(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.
------- Comment #68 From 2012-01-31 16:23:55 PST -------
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] [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?
------- Comment #69 From 2012-01-31 16:38:47 PST -------
(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.
------- Comment #70 From 2012-01-31 16:49:26 PST -------
(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.
------- Comment #71 From 2012-01-31 16:58:30 PST -------
(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.
------- Comment #72 From 2012-01-31 17:00:59 PST -------
(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;
    };
------- Comment #73 From 2012-01-31 17:11:00 PST -------
Created an attachment (id=124856) [details]
history.state
------- Comment #74 From 2012-01-31 17:30:45 PST -------
(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.

> 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.
------- Comment #75 From 2012-01-31 17:54:43 PST -------
(In reply to comment #74)
> (From update of attachment 124856 [details] [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 #76 From 2012-01-31 18:19:00 PST -------
(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.

>>> 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.
------- Comment #77 From 2012-01-31 19:30:47 PST -------
(In reply to comment #76)
> (From update of attachment 124856 [details] [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.
------- Comment #78 From 2012-01-31 19:40:38 PST -------
(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 #79 From 2012-02-02 15:27:29 PST -------
(From update of attachment 124856 [details])
Thanks for the patch!
------- Comment #80 From 2012-02-03 00:03:45 PST -------
Created an attachment (id=125274) [details]
Patch.

Same patch as before, to get another EWS run
------- Comment #81 From 2012-02-03 00:06:55 PST -------
(From update of attachment 125274 [details])
You can use "--no-review --no-obsolete" option on the ./webkit-patch command in order to avoid invalidating r+ that you got before.
------- Comment #82 From 2012-02-03 00:53:40 PST -------
(From update of attachment 125274 [details])
Attachment 125274 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11423168
------- Comment #83 From 2012-02-03 01:24:37 PST -------
(From update of attachment 125274 [details])
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
------- Comment #84 From 2012-02-05 04:31:15 PST -------
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?
------- Comment #85 From 2012-02-05 16:20:18 PST -------
(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?
------- Comment #86 From 2012-02-06 09:52:37 PST -------
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.
------- Comment #87 From 2012-02-06 13:03:31 PST -------
(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?
------- Comment #88 From 2012-02-06 16:05:57 PST -------
(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.
------- Comment #89 From 2012-02-07 17:13:53 PST -------
(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?
------- Comment #90 From 2012-02-07 17:22:45 PST -------
(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.
------- Comment #91 From 2012-02-07 17:24:46 PST -------
> 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).
------- Comment #92 From 2012-02-07 17:34:26 PST -------
(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.
------- Comment #93 From 2012-02-08 00:17:07 PST -------
Created an attachment (id=126022) [details]
Fixed tests
------- Comment #94 From 2012-02-08 00:18:41 PST -------
Let's wait for the bot results:-)
------- Comment #95 From 2012-02-08 00:46:26 PST -------
All green this time :D
------- Comment #96 From 2012-02-08 02:39:27 PST -------
(From update of attachment 126022 [details])
Clearing flags on attachment: 126022

Committed r107058: <http://trac.webkit.org/changeset/107058>
------- Comment #97 From 2012-02-08 02:39:40 PST -------
All reviewed patches have been landed.  Closing bug.