WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
31915
[Android] notifyHistoryItemChanged() should pass a pointer to the HistoryItem that changed.
https://bugs.webkit.org/show_bug.cgi?id=31915
Summary
[Android] notifyHistoryItemChanged() should pass a pointer to the HistoryItem...
Ben Murdoch
Reported
2009-11-26 08:29:07 PST
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.
Attachments
Proposed patch
(8.07 KB, patch)
2009-11-30 09:18 PST
,
Ben Murdoch
beidson
: review-
Details
Formatted Diff
Diff
New patch with ANDROID guards.
(6.96 KB, patch)
2009-12-02 12:59 PST
,
Ben Murdoch
beidson
: review+
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Ben Murdoch
Comment 1
2009-11-30 07:57:17 PST
(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
Comment 2
2009-11-30 09:18:17 PST
Created
attachment 44024
[details]
Proposed patch This patch implements the changes discussed above to help us manage History on Android.
Adam Barth
Comment 3
2009-11-30 12:52:05 PST
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
Comment 4
2009-12-01 07:49:26 PST
(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)
Comment 5
2009-12-01 07:58:07 PST
(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
Comment 6
2009-12-02 11:35:59 PST
(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 7
2009-12-02 11:36:25 PST
Comment on
attachment 44024
[details]
Proposed patch Per my comments above, r-
Ben Murdoch
Comment 8
2009-12-02 11:40:03 PST
(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
Comment 9
2009-12-02 12:59:48 PST
Created
attachment 44176
[details]
New patch with ANDROID guards.
Ben Murdoch
Comment 10
2009-12-03 04:39:36 PST
Landed as
r51630
.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug