[Android] Make BookmarkModelObserver more convenient.

[Android] Make BookmarkModelObserver more convenient.

Often, we don't need all the  bookmarksBridge#BookmarkModelObserver
events. Especially, it's common to skip listening on extensive
bookmark changes. So make it built-in and also provide fall-back
implementations so that listeners don't have to implement all the
stub methods.

BUG=386785
Committed: https://crrev.com/a4eb244baaac411fe27fb2282e62a80ee20fe1ff
Cr-Commit-Position: refs/heads/master@{#294318}

[Kibeom Kim (inactive), 2014-09-08 10:46:57]

kkimlabs@chromium.org changed reviewers:
+ ianwen@chromium.org, newt@chromium.org, tedchoc@chromium.org

[Kibeom Kim (inactive), 2014-09-08 10:46:58]

downstream : https://chrome-internal-review.googlesource.com/#/c/175423/

[Ted C, 2014-09-08 15:58:31]

https://codereview.chromium.org/552743002/diff/1/chrome/android/java/src/org/...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/552743002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:128: */
Can all of these functional changes just be incorporated into the existing
BookmarkModelObserver?

I don't think we should do it this way regardless.  If anything, we should
define a base observer that people can extend without implementing all the
methods.  Upstream, I believe we were told to call them
Empty<OtherInterfaceName>.

If we do need these different implementations, all these functional changes
could be isolated into a separate base class that prevents this class from
knowing about it.

[Kibeom Kim (inactive), 2014-09-09 01:20:35]

On 2014/09/08 15:58:31, Ted C wrote:
>
https://codereview.chromium.org/552743002/diff/1/chrome/android/java/src/org/...
> File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
> (right):
> 
>
https://codereview.chromium.org/552743002/diff/1/chrome/android/java/src/org/...
> chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:128:
*/
> Can all of these functional changes just be incorporated into the existing
> BookmarkModelObserver?
> 
> I don't think we should do it this way regardless.  If anything, we should
> define a base observer that people can extend without implementing all the
> methods.  Upstream, I believe we were told to call them
> Empty<OtherInterfaceName>.
> 
> If we do need these different implementations, all these functional changes
> could be isolated into a separate base class that prevents this class from
> knowing about it.

I tried a different way
https://chrome-internal-review.googlesource.com/#/c/175591

[Kibeom Kim (inactive), 2014-09-10 01:16:16]

Downstream example: https://chrome-internal-review.googlesource.com/#/c/175776/

If this looks OK, I'll finish off the downstream code.

https://codereview.chromium.org/552743002/diff/1/chrome/android/java/src/org/...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/552743002/diff/1/chrome/android/java/src/org/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:128: */
On 2014/09/08 15:58:30, Ted C wrote:
> Can all of these functional changes just be incorporated into the existing
> BookmarkModelObserver?
> 
> I don't think we should do it this way regardless.  If anything, we should
> define a base observer that people can extend without implementing all the
> methods.  Upstream, I believe we were told to call them
> Empty<OtherInterfaceName>.
> 
> If we do need these different implementations, all these functional changes
> could be isolated into a separate base class that prevents this class from
> knowing about it.

Done.

[Kibeom Kim (inactive), 2014-09-10 21:13:11]

(Just in case the multiple CLs were confusing) This is the latest attempt and
ready for review.

[Ian Wen, 2014-09-10 21:48:51]

Naming of the new functions is a bit complicated but I cannot come up with
better names either...

https://codereview.chromium.org/552743002/diff/20001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/552743002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:53: *
to the bookmark model. It's a mandatory to implement one method,
s It's a mandatory/It's mandatory

https://codereview.chromium.org/552743002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:80:
public void bookmarkNodeMovedNotDuringExtensiveChange(
I wonder if we could rename these functions to be something simpler. Now when I
am reading these function names, all I get is NotDuringExtensiveChange and every
function looks the same. How about keeping original names, say
bookmarkNodeMoved, and write comments to warn people that these functions will
only be called not during extensive change period?

[Ted C, 2014-09-10 21:57:40]

https://codereview.chromium.org/552743002/diff/20001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/552743002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:74: *
Invoked when a node has moved. But only when it's not during extensive change.
I wouldn't include "But only when it's not during extensive change." for all
these methods.

I would just document the behavior in the class level description and just put a
extra documentation on bookmarkNodeRemoved.

https://codereview.chromium.org/552743002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:80:
public void bookmarkNodeMovedNotDuringExtensiveChange(
I don't think we need these NotDuringExtensiveChange as the documentation should
be sufficient.

https://codereview.chromium.org/552743002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:133: * 
callback methods or it wasn't handled by other callback methods. Also note that
this
This behavior is odd to me.  I fear that this will introduce multiple calls
because it requires people to specifically not call super.<whatever> because
that will end up calling this.

Do we really need this everything funnels into a single place behavior?  Is it
really that common of a pattern?  I think in most places people should only be
acting on a subset of these functions so this seems like unnecessary overkill. 
In general, you'd rather have updates be very targeted and not just bulk
updates.

https://codereview.chromium.org/552743002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:149:
mNativeBookmarksBridge = nativeInit(profile);
I think you should expose a private native call to
BookmarkModel::IsDoingExtensiveChanges and set
mExtensiveBookmarkChangesHappening equal to that to ensure your logic in the
rest of the file is sane.

https://codereview.chromium.org/552743002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:400: if
(!mExtensiveBookmarkChangesHappening) {
for all these i would early exit

if (mExtensiveBookmarkChangesHappening) return;

[Kibeom Kim (inactive), 2014-09-10 22:39:16]

https://codereview.chromium.org/552743002/diff/20001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/552743002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:53: *
to the bookmark model. It's a mandatory to implement one method,
On 2014/09/10 21:48:51, Ian Wen wrote:
> s It's a mandatory/It's mandatory

Done.

https://codereview.chromium.org/552743002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:74: *
Invoked when a node has moved. But only when it's not during extensive change.
On 2014/09/10 21:57:40, Ted C wrote:
> I wouldn't include "But only when it's not during extensive change." for all
> these methods.
> 
> I would just document the behavior in the class level description and just put
a
> extra documentation on bookmarkNodeRemoved.

Done.

https://codereview.chromium.org/552743002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:133: * 
callback methods or it wasn't handled by other callback methods. Also note that
this
On 2014/09/10 21:57:39, Ted C wrote:
> This behavior is odd to me.  I fear that this will introduce multiple calls
> because it requires people to specifically not call super.<whatever> because
> that will end up calling this.
> 
> Do we really need this everything funnels into a single place behavior?  Is it
> really that common of a pattern?  I think in most places people should only be
> acting on a subset of these functions so this seems like unnecessary overkill.

> In general, you'd rather have updates be very targeted and not just bulk
> updates.

I think it should be a common pattern. First, we cannot avoid implementing very
general bookmarkModelChanged() event, because there are events not covered by
specific ones(e.g. partner update), and also now bulk-update. So I made it
abstract.

After that, all the other methods are optional to implement for more
optimization. But since bookmark changes won't happen that frequently, it's not
high-priority to optimize each specific path, and I've found myself funneling
all the events to one function repeatedly. (e.g., EnhancedBookmarkTitleBar)

One more potential advantage for funneling is when we add a new event. Say, if
we are adding bookmarkImageChanged() event. Then even if listeners don't handle
that particular case, it will function correctly by funneling.

Though, I agree that not preventing super.<whatever> is not good, and this is a
trade-off.

https://codereview.chromium.org/552743002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:149:
mNativeBookmarksBridge = nativeInit(profile);
On 2014/09/10 21:57:39, Ted C wrote:
> I think you should expose a private native call to
> BookmarkModel::IsDoingExtensiveChanges and set
> mExtensiveBookmarkChangesHappening equal to that to ensure your logic in the
> rest of the file is sane.

Done. (good point!)

https://codereview.chromium.org/552743002/diff/20001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:400: if
(!mExtensiveBookmarkChangesHappening) {
On 2014/09/10 21:57:40, Ted C wrote:
> for all these i would early exit
> 
> if (mExtensiveBookmarkChangesHappening) return;

Done.

[Ted C, 2014-09-10 23:52:29]

lgtm  -- I still find the behavior odd, but we can revisit this if it becomes an
issue.

https://codereview.chromium.org/552743002/diff/40001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/552743002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:54: *
bookmarkModelChangedNotDuringExtensiveChange. Other methods are optional and if
they aren't
update comment to new names

https://codereview.chromium.org/552743002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:68:
public void bookmarkNodeRemoved(BookmarkItem parent, int oldIndex, BookmarkItem
node,
move this down to be next to the removed call below.

[Kibeom Kim (inactive), 2014-09-10 23:57:18]

https://codereview.chromium.org/552743002/diff/40001/chrome/android/java/src/...
File chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java
(right):

https://codereview.chromium.org/552743002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:54: *
bookmarkModelChangedNotDuringExtensiveChange. Other methods are optional and if
they aren't
On 2014/09/10 23:52:28, Ted C wrote:
> update comment to new names

Done.

https://codereview.chromium.org/552743002/diff/40001/chrome/android/java/src/...
chrome/android/java/src/org/chromium/chrome/browser/BookmarksBridge.java:68:
public void bookmarkNodeRemoved(BookmarkItem parent, int oldIndex, BookmarkItem
node,
On 2014/09/10 23:52:28, Ted C wrote:
> move this down to be next to the removed call below.

Done.

[Kibeom Kim (inactive), 2014-09-11 02:43:53]

The CQ bit was checked by kkimlabs@chromium.org

[commit-bot: I haz the power, 2014-09-11 02:45:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/552743002/80001

[commit-bot: I haz the power, 2014-09-11 03:53:47]

Committed patchset #5 (id:80001) as 9b58ac748786a3ee36a507b7137412c992887048

[commit-bot: I haz the power, 2014-09-11 04:00:16]

Patchset 5 (id:??) landed as
https://crrev.com/a4eb244baaac411fe27fb2282e62a80ee20fe1ff
Cr-Commit-Position: refs/heads/master@{#294318}