Bug 47355 – Change WK2 session restoring to restore the full back/forward list

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

Change WK2 session restoring to restore the full back/forward list.

In https://bugs.webkit.org/show_bug.cgi?id=47354 we'll finally be hooking up the basic session state saving/restoring mechanism, and it will save the back/forward list.  It won't yet restore it - that added work is tracked by this bug.

[Brady Eidson]

In radar as <rdar://problem/8261624>

[Brady Eidson]

Created attachment 78062 [details]
Send session state from UI -> Web process (v1)

[Build Bot]

Attachment 78062 [details] did not build on win:
Build output: http://queues.webkit.org/results/7257404

[Brady Eidson]

Created attachment 78076 [details]
Send session state from UI -> Web process (v2)

[Sam Weinig]

Comment on attachment 78076 [details]
Send session state from UI -> Web process (v2) 

View in context: https://bugs.webkit.org/attachment.cgi?id=78076&action=review

> WebKit2/UIProcess/cf/WebBackForwardListCF.cpp:43
> +static uint64_t generateWebBackForwardItemID()
> +{
> +    // These IDs exist in the UIProcess for items created by the UIProcess.
> +    // The IDs generated here need to never collide with the IDs created in WebBackForwardListProxy in the WebProcess.
> +    // We accomplish this by started from the max and counting down, while the WebProcess starts from 1 and counts up.
> +    static uint64_t uniqueHistoryItemID = ULONG_LONG_MAX - 1;
> +    return uniqueHistoryItemID--;
> +}

I prefer the odd/even approach here.

> WebKit2/WebProcess/WebPage/WebPage.h:339
> +    void restoreSession(const SessionState&);

Fix paragraphing.

> WebKit2/WebProcess/WebPage/WebPage.messages.in:105
> +    # Speling and grammar.

This is not related. Please don't check it in.  Also, you suck.

[Brady Eidson]

http://trac.webkit.org/changeset/75121

Working on the last chunk right now.

[Brady Eidson]

Created attachment 78198 [details]
Finish it up!  (v1)

[Darin Adler]

Comment on attachment 78198 [details]
Finish it up!  (v1)

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.

[Brady Eidson]

(In reply to comment #8)
> (From update of attachment 78198 [details])
> 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:
>...
> It's better to avoid all that pair stuff if you don’t need it.

Yup.

> > 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.

Compiler doesn't want me to have a local variable named the same as a current-scoped function.

But I'll just remove it.  An earlier iteration of the code called process() about three times as much as this patch, which is where the temporary is from.  I'll just remove it.

> > WebKit2/WebProcess/WebPage/WebPage.cpp:856
> > +    const BackForwardListItemVector& list(sessionState.list());
> 
> I prefer assignment-style syntax to construction style.

Oh FINE.

Thanks for the review!

[Brady Eidson]

r75246