[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


Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #122860|review?                     |review+
               Flag|                            |




--- Comment #46 from Adam Barth <abarth at webkit.org>  2012-01-18 23:46:14 PST ---
(From update of attachment 122860)
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.

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