[webkit-reviews] review denied: [Bug 76035] Add state attribute to history's dom interface. : [Attachment 122471] use only one deserialization for history.state

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Pablo Flouret
<pablof at motorola.com>'s request for review:
Bug 76035: Add state attribute to history's dom interface.
https://bugs.webkit.org/show_bug.cgi?id=76035

Attachment 122471: use only one deserialization for history.state
https://bugs.webkit.org/attachment.cgi?id=122471&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
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.