Bug 184750 – Add globally-unique HistoryItem identifiers (and have WebKit2 adopt them)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brady Eidson]

Add globally-unique HistoryItem identifiers (and have WebKit2 adopt them)

With process swapping, the WebKit assumption that "back/forward items belong to a process" has been invalidated.

So we need to uniquely identify all history items across all processes so there will never be a collision in the global UIProcess namespace.

This also lets us get rid of the "per-WebProcess WebBackForwardListItem registry"

[Brady Eidson]

Created attachment 338309 [details]
Patch

[Brady Eidson]

Assume I'll fix the GTK build failure sensibly - Should still be ready for review

[Ryosuke Niwa]

Comment on attachment 338309 [details]
Patch

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

> Source/WebCore/history/BackForwardItemIdentifier.h:102
> +    static void constructDeletedValue(WebCore::BackForwardItemIdentifier& slot) { slot.processIdentifier = makeObjectIdentifier<WebCore::ProcessIdentifierType>(std::numeric_limits<uint64_t>::max()); }

Please use ObjectIdentifier<WebCore::ProcessIdentifierType>::hashTableDeletedValue().
Alternatively, define a constructor for HashTableDeletedValueType & add isHashTableDeletedValue to BackForwardItemIdentifier?

> Source/WebKit/UIProcess/WebBackForwardList.cpp:422
> +        backForwardListItemState.identifier = { Process::identifier(), generateObjectIdentifier<BackForwardItemIdentifier::ItemIdentifierType>() };

Neat!

> Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:-158
> -    updateBackForwardItem(itemID, m_page->pageID(), item.ptr());

Why is it okay to stop calling updateBackForwardItem here? Could you clarify in the change log?

[Brady Eidson]

(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 338309 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338309&action=review
> 
> > Source/WebCore/history/BackForwardItemIdentifier.h:102
> > +    static void constructDeletedValue(WebCore::BackForwardItemIdentifier& slot) { slot.processIdentifier = makeObjectIdentifier<WebCore::ProcessIdentifierType>(std::numeric_limits<uint64_t>::max()); }
> 
> Please use
> ObjectIdentifier<WebCore::ProcessIdentifierType>::hashTableDeletedValue().
> Alternatively, define a constructor for HashTableDeletedValueType & add
> isHashTableDeletedValue to BackForwardItemIdentifier?
> 

Sure.

> 
> > Source/WebKit/WebProcess/WebPage/WebBackForwardListProxy.cpp:-158
> > -    updateBackForwardItem(itemID, m_page->pageID(), item.ptr());
> 
> Why is it okay to stop calling updateBackForwardItem here? Could you clarify
> in the change log?

Previously, adding an item was "add or update to the process" followed by "associate that item to the web page"

Now it's one step "Add a new item to the page"

I'll put this in the changelog

[Brady Eidson]

Created attachment 338327 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Brady Eidson]

Created attachment 338332 [details]
Patch

[Brady Eidson]

Created attachment 338334 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 338334 [details]
Patch

Clearing flags on attachment: 338334

Committed r230812: <https://trac.webkit.org/changeset/230812>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39571288>

[Sam Weinig]

Two quick follow up questions:
1) I seem to remember that our long term goal was to try and make it so that back/forward list items were always created in the UIProcess, making among other things, the globally unique id problem trivial. Is that still a goal?

2) What is the reason that WebCore still uses the term HistoryItem? Should we change the name to BackForwardItem (or something like that) at some point (perhaps along with HistoryController)?

[Brady Eidson]

(In reply to Sam Weinig from comment #12)
> Two quick follow up questions:
> 1) I seem to remember that our long term goal was to try and make it so that
> back/forward list items were always created in the UIProcess, making among
> other things, the globally unique id problem trivial. Is that still a goal?

Sure!

> 2) What is the reason that WebCore still uses the term HistoryItem? Should
> we change the name to BackForwardItem (or something like that) at some point
> (perhaps along with HistoryController)?

There's no reason to be HistoryItem instead of BackForwardListItem.

I considered it here, but didn't do it because it would've taken half a day of fighting will all the platforms and all the build systems.