[NTP Snippets] Merge SnippetsController into SnippetsBridge

[NTP Snippets] Merge SnippetsController into SnippetsBridge

BUG=none

Committed: https://crrev.com/e80795399b9313fc3e039d17535164b65dbb874f
Cr-Commit-Position: refs/heads/master@{#386068}

[Marc Treib, 2016-04-07 17:30:22]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org

[Marc Treib, 2016-04-07 17:30:22]

Aaand one more. This is the last one for today! :D

[Bernhard Bauer, 2016-04-08 10:53:39]

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

https://codereview.chromium.org/1871583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:84:
@VisibleForTesting
Is this really visible for testing? It kind of looks like this is more protected
so it can be overridden in a subclass?

https://codereview.chromium.org/1871583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:86:
SnippetsBridge bridge = new SnippetsBridge(Profile.getLastUsedProfile());
It's a bit weird that we now create a new SnippetsBridge, only to destroy it
right afterwards. Really, we only need state to keep track of observers, which
we don't have here, so we could just make fetchSnippets() a static method.

(We could also get the last used profile on the native side rather than here;
either works now and would break if we should ever move to multi-profile on
Android, so I don't see one as better than the other, so we could use what is
more convenient.)

[Marc Treib, 2016-04-08 11:43:43]

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

https://codereview.chromium.org/1871583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:84:
@VisibleForTesting
On 2016/04/08 10:53:39, Bernhard Bauer wrote:
> Is this really visible for testing? It kind of looks like this is more
protected
> so it can be overridden in a subclass?

In a subclass that's used for testing :)
This has a higher visibility than it should have, for testing purposes. That's
what the VisibleForTesting annotation means, right?

https://codereview.chromium.org/1871583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:86:
SnippetsBridge bridge = new SnippetsBridge(Profile.getLastUsedProfile());
On 2016/04/08 10:53:39, Bernhard Bauer wrote:
> It's a bit weird that we now create a new SnippetsBridge, only to destroy it
> right afterwards. Really, we only need state to keep track of observers, which
> we don't have here, so we could just make fetchSnippets() a static method.
> 
> (We could also get the last used profile on the native side rather than here;
> either works now and would break if we should ever move to multi-profile on
> Android, so I don't see one as better than the other, so we could use what is
> more convenient.)

Agreed that it's a bit weird to create and immediately destroy a Bridge, but
this seemed like the least weird thing overall...
Oh well. I've added a static fetchSnippets. I've kept the non-static one - it'd
be weird having to pass in a profile when you already have a Bridge instance.

[Bernhard Bauer, 2016-04-08 13:04:20]

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

https://codereview.chromium.org/1871583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:84:
@VisibleForTesting
On 2016/04/08 11:43:42, Marc Treib wrote:
> On 2016/04/08 10:53:39, Bernhard Bauer wrote:
> > Is this really visible for testing? It kind of looks like this is more
> protected
> > so it can be overridden in a subclass?
> 
> In a subclass that's used for testing :)
> This has a higher visibility than it should have, for testing purposes. That's
> what the VisibleForTesting annotation means, right?

I guess... It's just that I mostly tend to read this as, "this is called from
somewhere else for testing". But you're right, other places in this file do the
same thing. Ok, carry on ☺

https://codereview.chromium.org/1871583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:86:
SnippetsBridge bridge = new SnippetsBridge(Profile.getLastUsedProfile());
On 2016/04/08 11:43:42, Marc Treib wrote:
> On 2016/04/08 10:53:39, Bernhard Bauer wrote:
> > It's a bit weird that we now create a new SnippetsBridge, only to destroy it
> > right afterwards. Really, we only need state to keep track of observers,
which
> > we don't have here, so we could just make fetchSnippets() a static method.
> > 
> > (We could also get the last used profile on the native side rather than
here;
> > either works now and would break if we should ever move to multi-profile on
> > Android, so I don't see one as better than the other, so we could use what
is
> > more convenient.)
> 
> Agreed that it's a bit weird to create and immediately destroy a Bridge, but
> this seemed like the least weird thing overall...
> Oh well. I've added a static fetchSnippets. I've kept the non-static one -
it'd
> be weird having to pass in a profile when you already have a Bridge instance.

That's why I meant we could just get the last used profile on the native side.
Also, we don't really have a case yet where we keep a SnippetsBridge around for
longer and call fetchSnippets() on it, right?

[Marc Treib, 2016-04-08 13:31:50]

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

https://codereview.chromium.org/1871583002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeBackgroundService.java:86:
SnippetsBridge bridge = new SnippetsBridge(Profile.getLastUsedProfile());
On 2016/04/08 13:04:19, Bernhard Bauer wrote:
> On 2016/04/08 11:43:42, Marc Treib wrote:
> > On 2016/04/08 10:53:39, Bernhard Bauer wrote:
> > > It's a bit weird that we now create a new SnippetsBridge, only to destroy
it
> > > right afterwards. Really, we only need state to keep track of observers,
> which
> > > we don't have here, so we could just make fetchSnippets() a static method.
> > > 
> > > (We could also get the last used profile on the native side rather than
> here;
> > > either works now and would break if we should ever move to multi-profile
on
> > > Android, so I don't see one as better than the other, so we could use what
> is
> > > more convenient.)
> > 
> > Agreed that it's a bit weird to create and immediately destroy a Bridge, but
> > this seemed like the least weird thing overall...
> > Oh well. I've added a static fetchSnippets. I've kept the non-static one -
> it'd
> > be weird having to pass in a profile when you already have a Bridge
instance.
> 
> That's why I meant we could just get the last used profile on the native side.
> Also, we don't really have a case yet where we keep a SnippetsBridge around
for
> longer and call fetchSnippets() on it, right?

Not yet, though I expect we will soon enough.
Alright, fair enough. I've removed the non-static fetchSnippets, and moved the
GetLastUsedProfile to native.

[Bernhard Bauer, 2016-04-08 13:33:52]

Thanks, that looks much nicer to me! LGTM

[Marc Treib, 2016-04-08 13:34:26]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2016-04-08 13:34:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1871583002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1871583002/60001

[commit-bot: I haz the power, 2016-04-08 14:33:01]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-04-08 14:34:13]

Description was changed from

==========
[NTP Snippets] Merge SnippetsController into SnippetsBridge

BUG=none
==========

to

==========
[NTP Snippets] Merge SnippetsController into SnippetsBridge

BUG=none

Committed: https://crrev.com/e80795399b9313fc3e039d17535164b65dbb874f
Cr-Commit-Position: refs/heads/master@{#386068}
==========

[commit-bot: I haz the power, 2016-04-08 14:34:15]

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