[webkit-reviews] review granted: [Bug 47355] Change WK2 session restoring to restore the full back/forward list : [Attachment 78198] Finish it up! (v1)

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Brady Eidson <beidson at apple.com>'s
request for review:
Bug 47355: Change WK2 session restoring to restore the full back/forward list
https://bugs.webkit.org/show_bug.cgi?id=47355

Attachment 78198: Finish it up!  (v1)
https://bugs.webkit.org/attachment.cgi?id=78198&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=78198&action=review

Great! Very exciting.

> WebKit2/UIProcess/WebProcessProxy.cpp:195
> +    std::pair<WebBackForwardListItemMap::iterator, bool> result =
m_backForwardListItemMap.add(item->itemID(), 0);
> +    
> +    // This item was just created by the UIProcess and is being added to the
map for the first time
> +    // so we should not already have an item for this ID.
> +    ASSERT(result.second);
> +    
> +    result.first->second = item;

I would write this like this:

    ASSERT(!m_backForwardListItemMap.contains(item->itemID());

    m_backForwardListItemMap.set(item->itemID(), item);

It's better to avoid all that pair stuff if you don’t need it.

> WebKit2/UIProcess/cf/WebPageProxyCF.cpp:132
> +    WebProcessProxy* webProcess = process();

Not sure it’s helpful to put that in a local variable. But if you do, I suggest
just naming it process:

    WebProcessProxy* process = this->process();

And I suggest using it on the line below where RestoreSession is sent.

> WebKit2/WebProcess/WebPage/WebBackForwardListProxy.cpp:86
> +    // UIProcess itemIDs should be even...

Why the three dots? Just one period should do.

> WebKit2/WebProcess/WebPage/WebBackForwardListProxy.h:43
> +    static void addHistoryItemFromUIProcess(uint64_t itemID,
PassRefPtr<WebCore::HistoryItem>);

I don’t think this function needs “history” in its name.

> WebKit2/WebProcess/WebPage/WebPage.cpp:856
> +    const BackForwardListItemVector& list(sessionState.list());

I prefer assignment-style syntax to construction style.