Bug 76035 – Add state attribute to history's dom interface.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Pablo Flouret]

http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#dom-history-state

[Pablo Flouret]

Created attachment 121987 [details]
proposed patch

[Adam Barth]

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?

[Pablo Flouret]

Good point, i'll check.

[Pablo Flouret]

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. :-)

[Brady Eidson]

(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);
 }

[Brady Eidson]

(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.

[Brady Eidson]

(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();

[Adam Barth]

> 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?

[Pablo Flouret]

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?

[Brady Eidson]

(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.

[Ian 'Hixie' Hickson]

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.

[Brady Eidson]

(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();

[Pablo Flouret]

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?

[Adam Barth]

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.

[Brady Eidson]

(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.

[Pablo Flouret]

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

[Brady Eidson]

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.

[Adam Barth]

> > 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.)

[Pablo Flouret]

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

[Pablo Flouret]

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 :-)

[Adam Barth]

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.

[Gustavo Noronha (kov)]

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

[Brady Eidson]

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!

[Brady Eidson]

(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.

[Pablo Flouret]

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

[WebKit Review Bot]

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

[Collabora GTK+ EWS bot]

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

[Pablo Flouret]

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

[Adam Barth]

@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.)

[Kentaro Hara]

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?

[Pablo Flouret]

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

[WebKit Review Bot]

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.

[Pablo Flouret]

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

[Brady Eidson]

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`

[Early Warning System Bot]

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

[Gyuyoung Kim]

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

[Pablo Flouret]

(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?

[Kentaro Hara]

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.

[Pablo Flouret]

(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).

[Kentaro Hara]

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.

[WebKit Review Bot]

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

[Kentaro Hara]

(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.

[Brady Eidson]

(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.

[Gustavo Noronha (kov)]

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

[Pablo Flouret]

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.

[Adam Barth]

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.

[Pablo Flouret]

Created attachment 123150 [details]
review comments fixed

[Oliver Hunt]

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)

[Brady Eidson]

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?

[Pablo Flouret]

(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?

[Brady Eidson]

(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.

[Pablo Flouret]

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?

[Brady Eidson]

(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.  :)

[Oliver Hunt]

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

[Brady Eidson]

(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?

[WebKit Review Bot]

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

[Brady Eidson]

(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.

[Collabora GTK+ EWS bot]

Comment on attachment 123150 [details]
review comments fixed

Attachment 123150 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11247171

[Adam Barth]

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

[Adam Barth]

> Let's just talk tomorrow.

Any further thoughts?

[Brady Eidson]

(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.

[Brady Eidson]

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?

[Oliver Hunt]

(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

[Pablo Flouret]

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

[Pablo Flouret]

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

[Philippe Normand]

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

[Kentaro Hara]

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.

[Pablo Flouret]

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?

[Kentaro Hara]

(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.

[Brady Eidson]

(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.

[Kentaro Hara]

(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.

[Kentaro Hara]

(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;
    };

[Pablo Flouret]

Created attachment 124856 [details]
history.state

[Kentaro Hara]

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.

[Pablo Flouret]

(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).

[Kentaro Hara]

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.

[Pablo Flouret]

(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.

[Kentaro Hara]

(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.

[Kentaro Hara]

Comment on attachment 124856 [details]
history.state

Thanks for the patch!

[Pablo Flouret]

Created attachment 125274 [details]
Patch.

Same patch as before, to get another EWS run

[Kentaro Hara]

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.

[Gustavo Noronha (kov)]

Comment on attachment 125274 [details]
Patch.

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

[WebKit Review Bot]

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

[Pablo Flouret]

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?

[Kentaro Hara]

(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?

[Eric Seidel (no email)]

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.

[Pablo Flouret]

(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?

[Kentaro Hara]

(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.

[Pablo Flouret]

(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?

[Kentaro Hara]

(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.

[Adam Barth]

> 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).

[Pablo Flouret]

(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.

[Pablo Flouret]

Created attachment 126022 [details]
Fixed tests

[Kentaro Hara]

Let's wait for the bot results:-)

[Pablo Flouret]

All green this time :D

[WebKit Review Bot]

Comment on attachment 126022 [details]
Fixed tests

Clearing flags on attachment: 126022

Committed r107058: <http://trac.webkit.org/changeset/107058>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.