[Webkit-unassigned] [Bug 76035] Add state attribute to history's dom interface.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=76035


Brady Eidson <beidson at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #122471|review?                     |review-
               Flag|                            |




--- Comment #17 from Brady Eidson <beidson at apple.com>  2012-01-13 11:48:26 PST ---
(From update of attachment 122471)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.