Hook up LocalNtpSource to OneGoogleBarService

Hook up LocalNtpSource to OneGoogleBarService

Hidden behind a new feature "OneGoogleBarOnLocalNtp".

BUG=583292
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2819553003
Cr-Commit-Position: refs/heads/master@{#467961}
Committed: https://chromium.googlesource.com/chromium/src/+/d7182375deee39e27a7327408ad3173ee490786f

[Marc Treib, 2017-04-13 16:59:54]

Description was changed from

==========
WIP: Hook up LocalNtpSource to OneGoogleBarService

BUG=583292
==========

to

==========
WIP: Hook up LocalNtpSource to OneGoogleBarService

BUG=583292
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Marc Treib, 2017-04-26 13:33:15]

Description was changed from

==========
WIP: Hook up LocalNtpSource to OneGoogleBarService

BUG=583292
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Hook up LocalNtpSource to OneGoogleBarService

Hidden behind a new feature "OneGoogleBarOnLocalNtp".

BUG=583292
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Marc Treib, 2017-04-26 13:33:22]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 13:33:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 14:40:48]

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

[commit-bot: I haz the power, 2017-04-26 14:40:49]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Marc Treib, 2017-04-26 15:32:09]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 15:32:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marc Treib, 2017-04-26 16:27:38]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 16:28:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 18:04:01]

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

[commit-bot: I haz the power, 2017-04-26 18:04:02]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-04-27 09:34:46]

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

[Marc Treib, 2017-04-27 09:34:47]

This is the one that makes it all work! Of course, there's a bunch of things
still missing:
- Integration tests
- Metrics
- Probably some UI polish, e.g. making it fade in

I believe all of these can wait for other CLs, so PTAL!

[Marc Treib, 2017-04-27 09:35:13]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-27 09:35:23]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-27 10:41:46]

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

[commit-bot: I haz the power, 2017-04-27 10:41:47]

Dry run: This issue passed the CQ dry run.

[sfiera, 2017-04-27 12:16:07]

I would really like to see some kind of test in place from the beginning. It
wouldn't really have to test anything interesting, just serve as proof that you
*can* write tests for the interesting stuff.

(If you want to break out the OneGoogleBarService part as a separate CL, that
part looks independent and ready to submit)

https://codereview.chromium.org/2819553003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/local_ntp/local_ntp.html (right):

https://codereview.chromium.org/2819553003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/local_ntp/local_ntp.html:43: Note: Listed after
ntp-contents, so that it'll get drawn on top. -->
Can you use z-index and avoid this subtlety?

https://codereview.chromium.org/2819553003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/2819553003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/local_ntp/local_ntp.js:688: * doesn't block the main
page load. It expects a global variable named "og"
Can you pass og.html and og.end_of_body_html instead? Up where the script is
created, expecting a global variable with certain properties is natural. Here,
it would be nice for the function to behave like a normal function.

Alternately, did you consider the JSONP approach, where the content of the
script is "injectOneGoogleBar({…})"?

https://codereview.chromium.org/2819553003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/local_ntp/local_ntp.js:696: inHeadStyle.href =
'chrome-search://local-ntp/one-google-in-head.css';
Can you use a "directory" for the one-google files?
(chrome-search://local-ntp/one-google/in-head.css &c.)

https://codereview.chromium.org/2819553003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/local_ntp/local_ntp.js:699: inHeadStyle.onload =
function() {
Is the injection order a restriction of the API? Here, you're waiting for some
CSS and JS to load before proceeding to the next thing, and I don't know what's
necessary.

[Marc Treib, 2017-04-27 14:00:56]

Comments addressed, except for adding a test which I'll work on now.

Unless you think this CL is getting too large, I'll leave the
OneGoogleBarService part in. There's no particular hurry to get that landed.

https://codereview.chromium.org/2819553003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/local_ntp/local_ntp.html (right):

https://codereview.chromium.org/2819553003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/local_ntp/local_ntp.html:43: Note: Listed after
ntp-contents, so that it'll get drawn on top. -->
On 2017/04/27 12:16:07, sfiera wrote:
> Can you use z-index and avoid this subtlety?

Done.

https://codereview.chromium.org/2819553003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/2819553003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/local_ntp/local_ntp.js:688: * doesn't block the main
page load. It expects a global variable named "og"
On 2017/04/27 12:16:07, sfiera wrote:
> Can you pass og.html and og.end_of_body_html instead? Up where the script is
> created, expecting a global variable with certain properties is natural. Here,
> it would be nice for the function to behave like a normal function.

Good idea, done.

> Alternately, did you consider the JSONP approach, where the content of the
> script is "injectOneGoogleBar({…})"?

I did, but that feels even more magical to me than the global variable. Maybe
I'm just not used to The JavaScript Way?

https://codereview.chromium.org/2819553003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/local_ntp/local_ntp.js:696: inHeadStyle.href =
'chrome-search://local-ntp/one-google-in-head.css';
On 2017/04/27 12:16:07, sfiera wrote:
> Can you use a "directory" for the one-google files?
> (chrome-search://local-ntp/one-google/in-head.css &c.)

Sure, done.

https://codereview.chromium.org/2819553003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/local_ntp/local_ntp.js:699: inHeadStyle.onload =
function() {
On 2017/04/27 12:16:07, sfiera wrote:
> Is the injection order a restriction of the API? Here, you're waiting for some
> CSS and JS to load before proceeding to the next thing, and I don't know
what's
> necessary.

Most of this is mandated by the API, yes. The two "in-head" things could load in
parallel I think, but I don't know a convenient way of writing that, so it
doesn't seem worth the trouble. WDYT?

[Marc Treib, 2017-04-28 09:07:50]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-28 09:08:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marc Treib, 2017-04-28 09:08:12]

Some first tests are here now, PTAL!

[commit-bot: I haz the power, 2017-04-28 10:24:25]

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

[commit-bot: I haz the power, 2017-04-28 10:24:25]

Dry run: This issue passed the CQ dry run.

[sfiera, 2017-04-28 11:17:57]

LGTM

https://codereview.chromium.org/2819553003/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/local_ntp/local_ntp.js (right):

https://codereview.chromium.org/2819553003/diff/140001/chrome/browser/resourc...
chrome/browser/resources/local_ntp/local_ntp.js:688: * doesn't block the main
page load. It expects a global variable named "og"
On 2017/04/27 14:00:56, Marc Treib wrote:
> On 2017/04/27 12:16:07, sfiera wrote:
> > Alternately, did you consider the JSONP approach, where the content of the
> > script is "injectOneGoogleBar({…})"?
> 
> I did, but that feels even more magical to me than the global variable. Maybe
> I'm just not used to The JavaScript Way?

Doesn't feel more or less magical to me, it's just a question of whether og is
the magical global or injectOneGoogleBar is the magical global :) Anyway, I'm
happy with the new state.

[Marc Treib, 2017-04-28 11:35:12]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2017-04-28 11:35:32]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-28 11:39:15]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1493379312498200,
"parent_rev": "4a8be6cb76f35f025a3969bf074118114e607ce5", "commit_rev":
"d7182375deee39e27a7327408ad3173ee490786f"}

[commit-bot: I haz the power, 2017-04-28 11:39:31]

Description was changed from

==========
Hook up LocalNtpSource to OneGoogleBarService

Hidden behind a new feature "OneGoogleBarOnLocalNtp".

BUG=583292
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Hook up LocalNtpSource to OneGoogleBarService

Hidden behind a new feature "OneGoogleBarOnLocalNtp".

BUG=583292
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2819553003
Cr-Commit-Position: refs/heads/master@{#467961}
Committed:
https://chromium.googlesource.com/chromium/src/+/d7182375deee39e27a7327408ad3...
==========

[commit-bot: I haz the power, 2017-04-28 11:39:32]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/d7182375deee39e27a7327408ad3...