Bug 47355

Summary: Change WK2 session restoring to restore the full back/forward list
Product: WebKit Reporter: Brady Eidson <beidson>
Component: WebKit2Assignee: Brady Eidson <beidson>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, sam
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Send session state from UI -> Web process (v1)
beidson: commit-queue-
Send session state from UI -> Web process (v2)
sam: review+, beidson: commit-queue-
Finish it up! (v1) darin: review+, beidson: commit-queue-

Description Brady Eidson 2010-10-07 09:24:16 PDT
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.
Comment 1 Brady Eidson 2011-01-05 16:47:50 PST
In radar as <rdar://problem/8261624>
Comment 2 Brady Eidson 2011-01-05 16:50:58 PST
Created attachment 78062 [details]
Send session state from UI -> Web process (v1)
Comment 3 Build Bot 2011-01-05 17:10:38 PST
Attachment 78062 [details] did not build on win:
Build output: http://queues.webkit.org/results/7257404
Comment 4 Brady Eidson 2011-01-05 17:15:04 PST
Created attachment 78076 [details]
Send session state from UI -> Web process (v2)
Comment 5 Sam Weinig 2011-01-05 17:19:59 PST
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.
Comment 6 Brady Eidson 2011-01-05 17:34:34 PST
http://trac.webkit.org/changeset/75121

Working on the last chunk right now.
Comment 7 Brady Eidson 2011-01-06 18:29:44 PST
Created attachment 78198 [details]
Finish it up!  (v1)
Comment 8 Darin Adler 2011-01-06 19:51:39 PST
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.
Comment 9 Brady Eidson 2011-01-07 08:53:28 PST
(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!
Comment 10 Brady Eidson 2011-01-07 08:57:05 PST
r75246