Summary: | [Android] notifyHistoryItemChanged() should pass a pointer to the HistoryItem that changed. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ben Murdoch <benm> | ||||||
Component: | WebCore Misc. | Assignee: | Ben Murdoch <benm> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, android-webkit-unforking, beidson, eric | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | PC | ||||||||
OS: | OS X 10.5 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 31867 | ||||||||
Attachments: |
|
Description
Ben Murdoch
2009-11-26 08:29:07 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 Created attachment 44024 [details]
Proposed patch
This patch implements the changes discussed above to help us manage History on Android.
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
(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 ... (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. (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. Comment on attachment 44024 [details]
Proposed patch
Per my comments above, r-
(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! Created attachment 44176 [details]
New patch with ANDROID guards.
Landed as r51630. |