Bug 31915 – [Android] notifyHistoryItemChanged() should pass a pointer to the HistoryItem that changed.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ben Murdoch]

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

On Android the Java side HistoryItem needs to be kept in sync with the WebCore version. Passing the HistoryItem that changed to the static notifyHistoryItemChanged function allows us to achieve this in a straightforward manner.

[Ben Murdoch]

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

Brady, in response to your comments on the "master bug":

> (From update of attachment 43840 [details])
> 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.

Great! :)

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

We manage a Java side version of the HistoryItem class that needs to be kept in sync with the WebCore version, and this synchronization is achieved by our implementation of the notifyHistoryItemChanged function. We need to be notified when any changes occur to the WebCore version of the class which is why we have the extra calls to the function in setDocumentState etc. Right now it is only Android that needs these extra callbacks and I tried to avoid using #if PLATFORM(ANDROID) guards by passing the extra "kind of mutation" parameter. That way the implementation of notifyHistoryItemChanged can switch on the kind of mutation and only act if it needs to (for example, see the change to WebHistoryItem.mm, which does not react to calls from the setDocumentState and the other Android only callsites). If the extra calls are too expensive, would you be happy to accept them guarded with #if PLATFORM(ANDROID)? (If so there would be no need for the additional kind-of-mutation parameter).

Thanks, Ben

[Ben Murdoch]

Created attachment 44024 [details]
Proposed patch

This patch implements the changes discussed above to help us manage History on Android.

[Adam Barth]

Attachment 44024 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Done processing WebCore/history/HistoryItem.h
Done processing WebCore/history/HistoryItem.cpp
WebKit/mac/History/WebHistoryItemInternal.h:31:  Missing spaces around /  [whitespace/operators] [3]
Done processing WebKit/mac/History/WebHistoryItemInternal.h
Total errors found: 1

[Ben Murdoch]

(In reply to comment #3)
> Attachment 44024 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> Done processing WebCore/history/HistoryItem.h
> Done processing WebCore/history/HistoryItem.cpp
> WebKit/mac/History/WebHistoryItemInternal.h:31:  Missing spaces around / 
> [whitespace/operators] [3]
> Done processing WebKit/mac/History/WebHistoryItemInternal.h
> Total errors found: 1

The line this refers to is a #import directive with a '/' in the pathname, so I think we're OK ...

[Eric Seidel (no email)]

(In reply to comment #4)
> The line this refers to is a #import directive with a '/' in the pathname, so I
> think we're OK ...

Filed bug 32022 about the false positive.

[Brady Eidson]

(In reply to comment #1)
> (In reply to comment #4 on https://bugs.webkit.org/show_bug.cgi?id=31867)
> 
> Brady, in response to your comments on the "master bug":
> 
> > (From update of attachment 43840 [details] [details])
> > 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.
> 
> Great! :)
> 
> > But, do you actually need to be notified of the individual KIND of mutation on
> > a HistoryItem?  What is the use-case here?
> > ...This could cause a flurry of notifications that are
> > a pointless performance hit.
> 
> We manage a Java side version of the HistoryItem class that needs to be kept in
> sync with the WebCore version, and this synchronization is achieved by our
> implementation of the notifyHistoryItemChanged function. We need to be notified
> when any changes occur to the WebCore version of the class which is why we have
> the extra calls to the function in setDocumentState etc. Right now it is only
> Android that needs these extra callbacks and I tried to avoid using #if
> PLATFORM(ANDROID) guards by passing the extra "kind of mutation" parameter.
> That way the implementation of notifyHistoryItemChanged can switch on the kind
> of mutation and only act if it needs to (for example, see the change to
> WebHistoryItem.mm, which does not react to calls from the setDocumentState and
> the other Android only callsites). If the extra calls are too expensive, would
> you be happy to accept them guarded with #if PLATFORM(ANDROID)? (If so there
> would be no need for the additional kind-of-mutation parameter).
>

The "kind of mutation" stuff is ugly, making the code harder to follow and the compiled code slower/more complex.

The notification mechanism is meant to support API, and API clients will never have an interest in tracking the kinds of things you need to keep in sync.

I'd much rather the new calls be #if ANDROID'ed and the kind-of-mutation parameter removed.

[Brady Eidson]

Comment on attachment 44024 [details]
Proposed patch

Per my comments above, r-

[Ben Murdoch]

(In reply to comment #6)

> 
> The "kind of mutation" stuff is ugly, making the code harder to follow and the
> compiled code slower/more complex.
> 
> The notification mechanism is meant to support API, and API clients will never
> have an interest in tracking the kinds of things you need to keep in sync.
> 
> I'd much rather the new calls be #if ANDROID'ed and the kind-of-mutation
> parameter removed.

OK, I can upload a new patch using ANDROID guards. Thanks!

[Ben Murdoch]

Created attachment 44176 [details]
New patch with ANDROID guards.

[Ben Murdoch]

Landed as r51630.