[webkit-reviews] review denied: [Bug 31867] [Android] WebCore/History code needs upstreaming. : [Attachment 43840] Patch.

[bugzilla-daemon at webkit.org]

Brady Eidson <beidson at apple.com> has denied Ben Murdoch <benm at google.com>'s
request for review:
Bug 31867: [Android] WebCore/History code needs upstreaming.
https://bugs.webkit.org/show_bug.cgi?id=31867

Attachment 43840: Patch.
https://bugs.webkit.org/attachment.cgi?id=43840&action=review

------- Additional Comments from Brady Eidson <beidson at apple.com>
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.