Bug 31914 – [Android] The FrameLoaderClient is unaware of BackForwardList changes.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ben Murdoch]

Master bug for upstreaming Android history changes: https://bugs.webkit.org/show_bug.cgi?id=31867

We would like a mechanism to notify the FrameLoaderClient of changes to the BackForwardList. On Android the BackForwardList is implemented as a Java class so we need a mechanism to keep the Java version in sync with the WebCore version.

[Ben Murdoch]

Brady, addressing your comments from the original "master bug":

(In reply to comment #4 on https://bugs.webkit.org/show_bug.cgi?id=31867)

> Are you trying to be notified with the back/forward list changes?

Yes, exactly.

> Other apps all seem to handle this without a specific notification.  Safari and
> Chrome both manage to refresh their back/forward buttons at appropriate times
> based on other delegate callbacks.

On Android, the BackForwardList and HistoryItem objects have counterparts that live on the Java side that we keep in sync with the WebCore versions of the objects via JNI. The bulk of the code to implement this is in WebKit/android, which we plan to upstream in due course. As I understand Chromium would have a similar syncing issue due to their sandboxing model but looking in their implementation in BackForwardList.h and BackForwardListChromium.cpp, they essentially reimplement the class. Our model doesn't require us to reimplement the class, but the addition of the callback when the BackForwardList changes simplifies our task.

> That said, I can see the value in having these callbacks.  If this is the
> problem you're trying to solve, then only the FrameLoaderClient changes are
> needed.
> 
> Of those changes, dispatchDidRemoveHistoryItem() confuses me.  It has an int
> parameter, but the parameter is always 0.  What is it for...?

Ah, that's my bad. I have removed the parameter.

> New methods added to FrameLoaderClient should be pure virtual, with empty
> implementations added to all of the implementations.  I know there's a few
> empty/default implementations in FrameLoaderClient.h but they are incorrect and
> should have been rejected.

OK. I was working on the precedent of those methods, and to save myself the task of providing default, empty implementations for each FrameLoaderClient implementation :) I will correct this in the next patch I upload and provide those default implementations to each implementation.

(In reply to comment #5 on https://bugs.webkit.org/show_bug.cgi?id=31867)
> More thoughts, also:
> 
> dispatchDidAddHistoryItem() and dispatchDidRemoveHistoryItem() are too generic
> of names.  We've been fighting confusion of what is "History" for the entire
> run of the project, and we shouldn't add more confusion.  They should be
> renamed to make it clear that the HistoryItem represents a back/forward entry
> and was added/removed from a BackForwardList.
> 
> Similarly, dispatchDidChangeHistoryIndex() could probably be called
> dispatchDidChangeBackForwardIndex()

Good idea. I have renamed the functions.

> Also, dispatchDidChangeHistoryIndex() includes a BackForwardList parameter, but
> didAdd/Remove do not.  As it turns out, the parameter isn't required.
>
> Either all 3 should include the parameter, or not of them should.  My vote goes
> for none of them.  Most (all?) FrameLoaderClient implementations know which
> Frame/Page they represent, and as such they don't need to be told which
> BackForwardList was affected.

Yes, we can access it in the FrameLoaderClient through the Page member, it was just a convenience having it as a parameter. I've changed it to not be passed.


I've got a new patch in the works to address these issues and will upload very shortly. Thanks for the review!!

[Ben Murdoch]

Created attachment 43956 [details]
Patch

This patch adds the callbacks to the FrameLoaderClient, the callsites to BackForwardList and empty implementations for each FrameLoaderClient implementation that lives upstream. The Android implementation of the callbacks will be upstreamed in due course, separate to this patch/bug.

[Adam Barth]

style-queue ran check-webkit-style on attachment 43956 [details] without any errors.

[Brady Eidson]

Comment on attachment 43956 [details]
Patch

I hate to see dead code added like this, but understand the iterative aspect of this process.  r+

[Ben Murdoch]

Landed as r51629.