Remove most of the dependencies on //chrome/...

Remove most of the dependencies on //chrome/...

Pull them in from the Java bridge.

Remaining are SupervisedUserService and FileDownloader. The former will
require more non-trivial work, and the latter doesn't really make sense
to stub out.

Put URLDataSource registration in bridge. It won't fit into a component,
and it can only be allowed to happen on Android, because other code is
responsible for it on other platforms. It doesn't really belong in
either place, but it has to go somewhere for now.

BUG=603026
Committed: https://crrev.com/7e952d4bf6eb67eee34c97264eed9d65b323fd75
Cr-Commit-Position: refs/heads/master@{#391556}

[sfiera, 2016-05-02 10:27:36]

Description was changed from

==========
Remove most of the dependencies on //chrome/...

Pull them in from the Java bridge, or in webui code.

Remaining are SupervisedUserService and FileDownloader. The former will
require more non-trivial work, and the latter doesn't really make sense
to stub out.

Put URLDataSource registration in bridge. It won't fit into a component,
and it can only be allowed to happen on Android, because other code is
responsible for it on other platforms. It doesn't really belong in
either place, but it has to go somewhere for now.

BUG=603026
==========

to

==========
Remove most of the dependencies on //chrome/...

Pull them in from the Java bridge.

Remaining are SupervisedUserService and FileDownloader. The former will
require more non-trivial work, and the latter doesn't really make sense
to stub out.

Put URLDataSource registration in bridge. It won't fit into a component,
and it can only be allowed to happen on Android, because other code is
responsible for it on other platforms. It doesn't really belong in
either place, but it has to go somewhere for now.

BUG=603026
==========

[sfiera, 2016-05-02 11:19:22]

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

[Marc Treib, 2016-05-02 11:30:40]

LGTM

https://codereview.chromium.org/1941713002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/most_visited_sites.cc (right):

https://codereview.chromium.org/1941713002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/most_visited_sites.cc:188: scoped_observer_(this),
mv_source_(SUGGESTIONS_SERVICE),
nit: I think we can save a line here.

https://codereview.chromium.org/1941713002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/most_visited_sites.h (right):

https://codereview.chromium.org/1941713002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/most_visited_sites.h:99: bool is_child,
profile_is_child, or is_child_account, or something like that? Without context
it's not clear what this means.

Also, FYI: The "is_child" state can change during runtime, so if we go to a
model where we don't create a MostVisitedSites instance per NTP (which we'll
probably want once there's more than one user), then we'll have to find another
way to query this.

[sfiera, 2016-05-03 13:26:12]

I was thinking more, and I realized that the Chromium Way here would be to add a
MostVisitedSitesFactory, not to inject everything by hand. Is that correct?

I guess that could go in a follow-up CL. Preferences?

https://codereview.chromium.org/1941713002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/most_visited_sites.cc (right):

https://codereview.chromium.org/1941713002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/most_visited_sites.cc:188: scoped_observer_(this),
mv_source_(SUGGESTIONS_SERVICE),
On 2016/05/02 11:30:40, Marc Treib wrote:
> nit: I think we can save a line here.

Done.

https://codereview.chromium.org/1941713002/diff/1/chrome/browser/android/ntp/...
File chrome/browser/android/ntp/most_visited_sites.h (right):

https://codereview.chromium.org/1941713002/diff/1/chrome/browser/android/ntp/...
chrome/browser/android/ntp/most_visited_sites.h:99: bool is_child,
On 2016/05/02 11:30:40, Marc Treib wrote:
> profile_is_child, or is_child_account, or something like that? Without context
> it's not clear what this means.
> 
> Also, FYI: The "is_child" state can change during runtime, so if we go to a
> model where we don't create a MostVisitedSites instance per NTP (which we'll
> probably want once there's more than one user), then we'll have to find
another
> way to query this.

Done.

[Marc Treib, 2016-05-03 13:32:55]

We'll definitely need a factory once we change this to a KeyedService. Before
that, and while there's only a single client, I'd keep it as-is.

[sfiera, 2016-05-04 08:18:25]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2016-05-04 08:18:25]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/1941713002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2016-05-04 08:18:37]

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

[sfiera, 2016-05-04 08:24:33]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2016-05-04 08:24:33]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/1941713002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2016-05-04 08:24:41]

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

[sfiera, 2016-05-04 09:18:48]

Marc, is the Android failure transient? I can't tell what's going on there.

[sfiera, 2016-05-04 09:19:50]

Or wait, it looks like it's retrying.

[commit-bot: I haz the power, 2016-05-04 10:08:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-05-04 10:08:45]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[sfiera, 2016-05-04 16:32:59]

Failed because I hadn't initialized all variables. In general, I probably would
have declared:

  PrefService* const prefs_;
  const TemplateURLService* const template_url_service_;                        
     
  variations::VariationsService* const variations_service_;                     
     

That would have caught my bug (there's a warning for uninitialized const fields)
but I didn't because Chromium code seems to be inconstinent and I didn't want to
be inconsistent.

Is my observation correct? Or is more const more good?

[sfiera, 2016-05-04 17:00:48]

The CQ bit was checked by sfiera@chromium.org

[sfiera, 2016-05-04 17:00:48]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org
Link to the patchset: https://codereview.chromium.org/1941713002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-05-04 17:01:04]

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

[commit-bot: I haz the power, 2016-05-04 17:56:46]

Description was changed from

==========
Remove most of the dependencies on //chrome/...

Pull them in from the Java bridge.

Remaining are SupervisedUserService and FileDownloader. The former will
require more non-trivial work, and the latter doesn't really make sense
to stub out.

Put URLDataSource registration in bridge. It won't fit into a component,
and it can only be allowed to happen on Android, because other code is
responsible for it on other platforms. It doesn't really belong in
either place, but it has to go somewhere for now.

BUG=603026
==========

to

==========
Remove most of the dependencies on //chrome/...

Pull them in from the Java bridge.

Remaining are SupervisedUserService and FileDownloader. The former will
require more non-trivial work, and the latter doesn't really make sense
to stub out.

Put URLDataSource registration in bridge. It won't fit into a component,
and it can only be allowed to happen on Android, because other code is
responsible for it on other platforms. It doesn't really belong in
either place, but it has to go somewhere for now.

BUG=603026
==========

[commit-bot: I haz the power, 2016-05-04 17:56:47]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-04 17:59:14]

Description was changed from

==========
Remove most of the dependencies on //chrome/...

Pull them in from the Java bridge.

Remaining are SupervisedUserService and FileDownloader. The former will
require more non-trivial work, and the latter doesn't really make sense
to stub out.

Put URLDataSource registration in bridge. It won't fit into a component,
and it can only be allowed to happen on Android, because other code is
responsible for it on other platforms. It doesn't really belong in
either place, but it has to go somewhere for now.

BUG=603026
==========

to

==========
Remove most of the dependencies on //chrome/...

Pull them in from the Java bridge.

Remaining are SupervisedUserService and FileDownloader. The former will
require more non-trivial work, and the latter doesn't really make sense
to stub out.

Put URLDataSource registration in bridge. It won't fit into a component,
and it can only be allowed to happen on Android, because other code is
responsible for it on other platforms. It doesn't really belong in
either place, but it has to go somewhere for now.

BUG=603026

Committed: https://crrev.com/7e952d4bf6eb67eee34c97264eed9d65b323fd75
Cr-Commit-Position: refs/heads/master@{#391556}
==========

[commit-bot: I haz the power, 2016-05-04 17:59:15]

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

[Marc Treib, 2016-05-05 16:22:55]

On 2016/05/04 16:32:59, sfiera wrote:
> Failed because I hadn't initialized all variables. In general, I probably
would
> have declared:
> 
>   PrefService* const prefs_;
>   const TemplateURLService* const template_url_service_;                      
 
>      
>   variations::VariationsService* const variations_service_;                   
 
>      
> 
> That would have caught my bug (there's a warning for uninitialized const
fields)
> but I didn't because Chromium code seems to be inconstinent and I didn't want
to
> be inconsistent.

Well, clearly code review should have caught this bug too! ;)

> Is my observation correct? Or is more const more good?

I'd say "both" - const members are fairly rare in Chrome (though not completely
unheard of), but I'd say they're generally a good thing. Often you do need to
change them in some cases though, e.g. in tests.

I kinda wonder why we don't have a warning for uninitialized members, const or
not. Okay, for the non-const case, it's not necessarily a bug, but still...

[sfiera, 2016-05-06 07:15:56]

I think I might have had to rebase before submitting, and resolved a conflict
wrong*, so it could have been after LGTM.

The problem with warnings is that you have to have a really low false positive
rate or else they become noise. I think that -Werror and zero false positives is
the only way to go, but Chromium doesn't set -Werror, does it?

* I prefer one initialization per line, which is a lot easier to merge, but I
know that's not going to happen.