[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
bugzilla-daemon at webkit.org
Fri Jan 13 11:48:26 PST 2012
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.
More information about the webkit-reviews
mailing list