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

See Also:


Attachments
proposed patch (8.75 KB, patch)
2012-01-11 00:10 PST, Pablo Flouret
no flags 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 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 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 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 Details | Formatted Diff | Diff
history.state part, reworked (18.75 KB, patch)
2012-01-17 18:40 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff
review comments fixed (18.63 KB, patch)
2012-01-19 11:06 PST, Pablo Flouret
no flags 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 Details | Formatted Diff | Diff
history.state (15.66 KB, patch)
2012-01-31 17:11 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff
Patch. (15.66 KB, patch)
2012-02-03 00:03 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff
Fixed tests (16.34 KB, patch)
2012-02-08 00:17 PST, Pablo Flouret
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Pablo Flouret 2012-01-11 00:10:44 PST
Created attachment 121987 [details]
proposed patch
Comment 2 Adam Barth 2012-01-11 00:33:54 PST
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?
Comment 3 Pablo Flouret 2012-01-11 00:36:12 PST
Good point, i'll check.
Comment 4 Pablo Flouret 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 Brady Eidson 2012-01-11 12:53:07 PST
(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);
 }
Comment 6 Brady Eidson 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 Brady Eidson 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 Adam Barth 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 Pablo Flouret 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 Brady Eidson 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 Ian 'Hixie' Hickson 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 Brady Eidson 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 Pablo Flouret 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 Adam Barth 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 Brady Eidson 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 Pablo Flouret 2012-01-13 11:20:15 PST
Created attachment 122471 [details]
use only one deserialization for history.state
Comment 17 Brady Eidson 2012-01-13 11:48:26 PST
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.
Comment 18 Adam Barth 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 Pablo Flouret 2012-01-13 16:11:57 PST
Created attachment 122518 [details]
new try based on brady's suggestions
Comment 20 Pablo Flouret 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 Adam Barth 2012-01-13 16:34:32 PST
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 22 Gustavo Noronha (kov) 2012-01-13 16:43:54 PST
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 23 Brady Eidson 2012-01-13 16:50:58 PST
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!
Comment 24 Brady Eidson 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 Pablo Flouret 2012-01-15 20:33:29 PST
Created attachment 122586 [details]
add [CallWith] support for attributes and cleanup from review comments
Comment 26 WebKit Review Bot 2012-01-15 21:15:26 PST
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 27 Collabora GTK+ EWS bot 2012-01-16 00:01:29 PST
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
Comment 28 Pablo Flouret 2012-01-16 00:09:24 PST
I guess an EmptyScriptState won't do, how do i get a proper one?
Comment 29 Adam Barth 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 Kentaro Hara 2012-01-17 02:32:16 PST
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?
Comment 31 Pablo Flouret 2012-01-17 18:31:56 PST
Created attachment 122857 [details]
[CallWith] support for attributes in jsc/v8 generators
Comment 32 WebKit Review Bot 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 Pablo Flouret 2012-01-17 18:40:47 PST
Created attachment 122860 [details]
history.state part, reworked
Comment 34 Brady Eidson 2012-01-17 18:52:26 PST
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 35 Early Warning System Bot 2012-01-17 19:15:53 PST
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 36 Gyuyoung Kim 2012-01-17 19:21:20 PST
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
Comment 37 Pablo Flouret 2012-01-17 20:41:43 PST
(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 38 Kentaro Hara 2012-01-17 20:45:08 PST
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.
Comment 39 Pablo Flouret 2012-01-17 21:02:15 PST
(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 40 Kentaro Hara 2012-01-17 21:07:53 PST
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 41 WebKit Review Bot 2012-01-17 21:12:38 PST
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
Comment 42 Kentaro Hara 2012-01-17 21:14:50 PST
(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.
Comment 43 Brady Eidson 2012-01-17 22:49:00 PST
(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 44 Gustavo Noronha (kov) 2012-01-17 22:53:45 PST
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 45 Pablo Flouret 2012-01-17 23:07:27 PST
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 46 Adam Barth 2012-01-18 23:46:14 PST
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.
Comment 47 Pablo Flouret 2012-01-19 11:06:11 PST
Created attachment 123150 [details]
review comments fixed
Comment 48 Oliver Hunt 2012-01-19 11:08:35 PST
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 49 Brady Eidson 2012-01-19 11:12:05 PST
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?
Comment 50 Pablo Flouret 2012-01-19 11:21:19 PST
(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?
Comment 51 Brady Eidson 2012-01-19 11:26:43 PST
(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.
Comment 52 Pablo Flouret 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 Brady Eidson 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 Oliver Hunt 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 Brady Eidson 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 WebKit Review Bot 2012-01-19 12:26:50 PST
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
Comment 57 Brady Eidson 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 Collabora GTK+ EWS bot 2012-01-19 14:39:21 PST
Comment on attachment 123150 [details]
review comments fixed

Attachment 123150 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11247171
Comment 59 Adam Barth 2012-01-20 15:31:49 PST
*** Bug 70876 has been marked as a duplicate of this bug. ***
Comment 60 Adam Barth 2012-01-24 21:44:15 PST
> Let's just talk tomorrow.

Any further thoughts?
Comment 61 Brady Eidson 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 Brady Eidson 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 Oliver Hunt 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 Pablo Flouret 2012-01-25 17:14:06 PST
Alright, i'll give that plan a shot this week, thanks!
Comment 65 Pablo Flouret 2012-01-31 13:31:11 PST
Created attachment 124811 [details]
First attempt at a full proper solution
Comment 66 Philippe Normand 2012-01-31 13:56:54 PST
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 67 Kentaro Hara 2012-01-31 16:06:00 PST
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.
Comment 68 Pablo Flouret 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])
> 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 Kentaro Hara 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 Brady Eidson 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 Kentaro Hara 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 Kentaro Hara 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 Pablo Flouret 2012-01-31 17:11:00 PST
Created attachment 124856 [details]
history.state
Comment 74 Kentaro Hara 2012-01-31 17:30:45 PST
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.
Comment 75 Pablo Flouret 2012-01-31 17:54:43 PST
(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 76 Kentaro Hara 2012-01-31 18:19:00 PST
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.
Comment 77 Pablo Flouret 2012-01-31 19:30:47 PST
(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.
Comment 78 Kentaro Hara 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 Kentaro Hara 2012-02-02 15:27:29 PST
Comment on attachment 124856 [details]
history.state

Thanks for the patch!
Comment 80 Pablo Flouret 2012-02-03 00:03:45 PST
Created attachment 125274 [details]
Patch.

Same patch as before, to get another EWS run
Comment 81 Kentaro Hara 2012-02-03 00:06:55 PST
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 82 Gustavo Noronha (kov) 2012-02-03 00:53:40 PST
Comment on attachment 125274 [details]
Patch.

Attachment 125274 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11423168
Comment 83 WebKit Review Bot 2012-02-03 01:24:37 PST
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
Comment 84 Pablo Flouret 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 Kentaro Hara 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 Eric Seidel 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 Pablo Flouret 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 Kentaro Hara 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 Pablo Flouret 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 Kentaro Hara 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 Adam Barth 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 Pablo Flouret 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 Pablo Flouret 2012-02-08 00:17:07 PST
Created attachment 126022 [details]
Fixed tests
Comment 94 Kentaro Hara 2012-02-08 00:18:41 PST
Let's wait for the bot results:-)
Comment 95 Pablo Flouret 2012-02-08 00:46:26 PST
All green this time :D
Comment 96 WebKit Review Bot 2012-02-08 02:39:27 PST
Comment on attachment 126022 [details]
Fixed tests

Clearing flags on attachment: 126022

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