Bug 31867 – [Android] WebCore/History code needs upstreaming.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ben Murdoch]

This patch adds the WebCore/history/android directory which contains some Android specific history implementation.

We also require calls to the notifyHistoryItemChanged function at points that other platforms do not, and vice versa so this patch adds an extra enum parameter to that function so we can determine what action has caused the history to be changed. Then in the platform specific implementation of the function we can switch on that enum value to determine whether or not the history should be updated. Currently as far as I can tell Mac is the only other port that redefines notifyHistoryItemChanged and I've updated it's logic so there should be no change in functionality.

[Ben Murdoch]

Created attachment 43840 [details]
Patch.

[Eric Seidel (no email)]

This change needs more explanation.  I'm not sure what you're trying to accomplish here and why these notifications are needed for Andriod but not any other port.

[Ben Murdoch]

(In reply to comment #2)
> This change needs more explanation.  I'm not sure what you're trying to
> accomplish here and why these notifications are needed for Andriod but not any
> other port.

The notfication mechanism at the moment is only used by the Mac port and the difference is that on Android we consider the history to have changed in different circumstances to when the Mac does. This is to help us work around some difficult issues relating to accessing the desktop version of a website that then goes through various redirects to forward Android to a mobile version of the site whilst still maintaining a useful history.

Instead of littering the code in HistoryItem with #if PLATFORM(ANDROID), I thought it would be a nicer solution to allow each platform that uses this notification mechanism to decide in it's platform specific callback whether to respond to the notification. The changes to WebKit/mac/WebHistoryItem.mm should reflect this on the Mac and the Android callback will be upstreamed when we send the code in WebKit/android.

I hope this helps to clarify the changes.

Thanks, Ben

[Brady Eidson]

Comment on attachment 43840 [details]
Patch.

r-

The title of this bug doesn't actually say what this bug is about, and the comments don't actually say what *precise* problem you're trying to solve.

As a result, I see a patch that seems to overreach and not have a clear purpose.

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

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.  

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

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.

I like adding the HistoryItem parameter to notifyHistoryItemChanged - that will actually help us fulfill an API contract that we've basically been ignoring for awhile by not including the WebHistoryItem that changed.

But, do you actually need to be notified of the individual KIND of mutation on a HistoryItem?  What is the use-case here?

Also, adding new notifications for things like document state, target item, and children items seems misguided.  These are all about state internal to WebCore.  Is there a reason your client needs to know about them?  Also, these things change A LOT more often than the other ones.  Rarely does a history item have it's title or URL changed, for example, but OFTEN might it have child items added during a navigation.  This could cause a flurry of notifications that are a pointless performance hit.

[Brady Eidson]

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()

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.

[Ben Murdoch]

Hi Brady,

First thank you very much for the thorough review and comments. To make sure I can address each of your points (and make life easier for both of us :)) I propose that I split this patch up into more manageable chunks (with a new bug for each), and use this bug as a "master bug" to track overall progress.

I think we could proceed in this order:
1) A patch to add the Android specific files to history/android, and the code protected by PLATFORM(ANDROID) in history/HistoryItem.h

2) A patch to add the BackForwardList callbacks to FrameLoaderClient (pure virtual) and default code to each of the implementations of FrameLoaderClient, and also the changes to BackForwardList.cpp to call those callbacks.

3) The changes to notifyHistoryItemChanged and the points where it is called from in HistoryItem.cpp, and any changes to current usage of the notifyHistoryItemChanged function in platform code for example the changes to WebKit/mac/History/WebHistoryItem.mm

I'll update this bug with the bug numbers of the smaller patches and address your comments in the relevant bugs.

Thanks, Ben

[Ben Murdoch]

Sub-bugs:

1) https://bugs.webkit.org/show_bug.cgi?id=31913
2) https://bugs.webkit.org/show_bug.cgi?id=31914
3) https://bugs.webkit.org/show_bug.cgi?id=31915

[Ben Murdoch]

Comment on attachment 43840 [details]
Patch.

Marking the original large patch obsolete as I have split it up into smaller parts in each of the sub bugs.

[Ben Murdoch]

Patches for each of the sub bugs have landed so closing this bug.