[webkit-reviews] review granted: [Bug 76035] Add state attribute to history's dom interface. : [Attachment 122860] history.state part, reworked

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted 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 122860: history.state part, reworked
https://bugs.webkit.org/attachment.cgi?id=122860&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.