Bug 54036 – didChangeBackForwardList should include some context about what changed

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

[Brady Eidson]

In the WKPage loader client, didChangeBackForwardList should include some context about what changed.

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

[Brady Eidson]

Created attachment 81696 [details]
Patch v1

[Darin Adler]

Comment on attachment 81696 [details]
Patch v1

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

> Source/WebKit2/UIProcess/WebBackForwardList.cpp:67
> +    Vector<RefPtr<APIObject> > removed;

I think removedItems would be a better name.

> Source/WebKit2/UIProcess/WebLoaderClient.cpp:214
> -void WebLoaderClient::didChangeBackForwardList(WebPageProxy* page)
> +void WebLoaderClient::didChangeBackForwardList(WebPageProxy* page, WebBackForwardListItem* addedItem, Vector<RefPtr<APIObject> >* removedItems)

const Vector* would be better.

Why APIObject in the vector instead of WebBackForwardListItem? Is that to make it work with ImmutableArray?

Why RefPtr in the vector but not for addedItem? Could there be a race where addedItem needs to be ref'd too?

> Source/WebKit2/UIProcess/WebLoaderClient.cpp:219
> +    RefPtr<ImmutableArray> removed;

removedItemsArray is probably a better name than "removed".

[Brady Eidson]

(In reply to comment #2)
> (From update of attachment 81696 [details])
>
> > Source/WebKit2/UIProcess/WebLoaderClient.cpp:214
> > -void WebLoaderClient::didChangeBackForwardList(WebPageProxy* page)
> > +void WebLoaderClient::didChangeBackForwardList(WebPageProxy* page, WebBackForwardListItem* addedItem, Vector<RefPtr<APIObject> >* removedItems)
> 
> const Vector* would be better.

Can't, because ImmutableArray::adopt() actually inherits the vector buffer.

> Why APIObject in the vector instead of WebBackForwardListItem? Is that to make it work with ImmutableArray?

Yes - ImmutableArray is for WK* API vending, and it only knows how to adopt generic Vector<RefPtr<APIObject> >s.

> Why RefPtr in the vector but not for addedItem? 

RefPtr<> in the Vector because that's what ImmutableArray works with.

> Could there be a race where addedItem needs to be ref'd too?

Nope, it's solidly reff'ed in the WebBackForwardList itself when this client call is made.

Thanks!

[Brady Eidson]

r77978