New issue
Advanced search Search tips

Issue 583292 link

Starred by 7 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Feature

Blocked on:
issue 704518
issue 740462
issue 753383

Blocking:
issue 583289
issue 521698
issue 693085
issue 696242
issue 708970



Sign in to add a comment

[Local NTP] Support the OneGoogleBar

Project Member Reported by treib@chromium.org, Feb 2 2016

Issue description

Add support for showing the OneGoogleBar on the local NTP, by using a new API that the OneGoogle team is working on.

Design doc: go/ntp-ogb
 

Comment 1 by treib@chromium.org, Feb 2 2016

Add the OneGoogleBar to the local NTP.

Comment 2 by treib@chromium.org, Mar 30 2016

Cc: -treib@chromium.org
Owner: treib@chromium.org
Status: Assigned (was: Available)

Comment 3 by treib@chromium.org, Mar 30 2016

Status: Started (was: Assigned)

Comment 4 by treib@chromium.org, Jul 18 2016

Status: ExternalDependency (was: Started)
Blocked on support from OGB for now.

Comment 5 by fi...@chromium.org, Aug 9 2016

Labels: zine-ntp-pe zine-triaged

Comment 6 by treib@chromium.org, Jan 24 2017

Status: Started (was: ExternalDependency)
I'm working with OneGoogle folks on this.

Comment 7 by treib@chromium.org, Jan 24 2017

Blocking: 521698

Comment 8 by treib@chromium.org, Feb 16 2017

Blocking: 693085

Comment 9 by treib@chromium.org, Mar 23 2017

Blockedon: 704518
Labels: -Restrict-View-Google
Blocking: 708970
Description: Show this description
Project Member

Comment 14 by bugdroid1@chromium.org, Apr 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2edac8fe131cb58ae2bc37c007cc0b3c56f6fc0e

commit 2edac8fe131cb58ae2bc37c007cc0b3c56f6fc0e
Author: treib <treib@chromium.org>
Date: Wed Apr 26 10:29:39 2017

Local NTP: Introduce OneGoogleBarService

BUG= 583292 

Review-Url: https://codereview.chromium.org/2811353002
Cr-Commit-Position: refs/heads/master@{#467286}

[modify] https://crrev.com/2edac8fe131cb58ae2bc37c007cc0b3c56f6fc0e/chrome/browser/BUILD.gn
[modify] https://crrev.com/2edac8fe131cb58ae2bc37c007cc0b3c56f6fc0e/chrome/browser/search/one_google_bar/one_google_bar_data.cc
[modify] https://crrev.com/2edac8fe131cb58ae2bc37c007cc0b3c56f6fc0e/chrome/browser/search/one_google_bar/one_google_bar_data.h
[modify] https://crrev.com/2edac8fe131cb58ae2bc37c007cc0b3c56f6fc0e/chrome/browser/search/one_google_bar/one_google_bar_fetcher.h
[modify] https://crrev.com/2edac8fe131cb58ae2bc37c007cc0b3c56f6fc0e/chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.h
[add] https://crrev.com/2edac8fe131cb58ae2bc37c007cc0b3c56f6fc0e/chrome/browser/search/one_google_bar/one_google_bar_service.cc
[add] https://crrev.com/2edac8fe131cb58ae2bc37c007cc0b3c56f6fc0e/chrome/browser/search/one_google_bar/one_google_bar_service.h
[add] https://crrev.com/2edac8fe131cb58ae2bc37c007cc0b3c56f6fc0e/chrome/browser/search/one_google_bar/one_google_bar_service_factory.cc
[add] https://crrev.com/2edac8fe131cb58ae2bc37c007cc0b3c56f6fc0e/chrome/browser/search/one_google_bar/one_google_bar_service_factory.h
[add] https://crrev.com/2edac8fe131cb58ae2bc37c007cc0b3c56f6fc0e/chrome/browser/search/one_google_bar/one_google_bar_service_observer.h
[add] https://crrev.com/2edac8fe131cb58ae2bc37c007cc0b3c56f6fc0e/chrome/browser/search/one_google_bar/one_google_bar_service_unittest.cc
[modify] https://crrev.com/2edac8fe131cb58ae2bc37c007cc0b3c56f6fc0e/chrome/test/BUILD.gn

Project Member

Comment 15 by bugdroid1@chromium.org, Apr 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d7182375deee39e27a7327408ad3173ee490786f

commit d7182375deee39e27a7327408ad3173ee490786f
Author: treib <treib@chromium.org>
Date: Fri Apr 28 11:39:11 2017

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}

[modify] https://crrev.com/d7182375deee39e27a7327408ad3173ee490786f/chrome/browser/resources/local_ntp/local_ntp.css
[modify] https://crrev.com/d7182375deee39e27a7327408ad3173ee490786f/chrome/browser/resources/local_ntp/local_ntp.html
[modify] https://crrev.com/d7182375deee39e27a7327408ad3173ee490786f/chrome/browser/resources/local_ntp/local_ntp.js
[modify] https://crrev.com/d7182375deee39e27a7327408ad3173ee490786f/chrome/browser/search/local_ntp_source.cc
[modify] https://crrev.com/d7182375deee39e27a7327408ad3173ee490786f/chrome/browser/search/local_ntp_source.h
[modify] https://crrev.com/d7182375deee39e27a7327408ad3173ee490786f/chrome/browser/search/one_google_bar/one_google_bar_service.cc
[modify] https://crrev.com/d7182375deee39e27a7327408ad3173ee490786f/chrome/browser/search/one_google_bar/one_google_bar_service.h
[modify] https://crrev.com/d7182375deee39e27a7327408ad3173ee490786f/chrome/browser/search/one_google_bar/one_google_bar_service_observer.h
[modify] https://crrev.com/d7182375deee39e27a7327408ad3173ee490786f/chrome/browser/search/one_google_bar/one_google_bar_service_unittest.cc
[modify] https://crrev.com/d7182375deee39e27a7327408ad3173ee490786f/chrome/browser/ui/search/local_ntp_browsertest.cc

Project Member

Comment 17 by bugdroid1@chromium.org, May 3 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8758b479e0b41c9311ded11ef2a2d4ffd4d59d4c

commit 8758b479e0b41c9311ded11ef2a2d4ffd4d59d4c
Author: treib <treib@chromium.org>
Date: Wed May 03 10:47:21 2017

Local NTP: Allow iframes from *.google.com
if OneGoogleBarOnLocalNtp is enabled.
This achieves two things:
- More robust if things change server-side
- Allows staging instances

BUG= 583292 

Review-Url: https://codereview.chromium.org/2856133002
Cr-Commit-Position: refs/heads/master@{#468933}

[modify] https://crrev.com/8758b479e0b41c9311ded11ef2a2d4ffd4d59d4c/chrome/browser/search/local_ntp_source.cc

Project Member

Comment 19 by bugdroid1@chromium.org, May 10 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2b22fb640f88f055ab951a8fdc18bf073a2af90f

commit 2b22fb640f88f055ab951a8fdc18bf073a2af90f
Author: treib <treib@chromium.org>
Date: Wed May 10 16:12:09 2017

Local NTP: Update the scope for the OneGoogle API

BUG= 583292 

Review-Url: https://codereview.chromium.org/2872313002
Cr-Commit-Position: refs/heads/master@{#470594}

[modify] https://crrev.com/2b22fb640f88f055ab951a8fdc18bf073a2af90f/chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc

Project Member

Comment 21 by bugdroid1@chromium.org, May 11 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/377048614cb8970f8c621ebb059deb223b1c8b00

commit 377048614cb8970f8c621ebb059deb223b1c8b00
Author: treib <treib@chromium.org>
Date: Thu May 11 14:37:44 2017

OneGoogleBarFetcher: replace cmdline flags with feature params

BUG= 583292 

Review-Url: https://codereview.chromium.org/2877763002
Cr-Commit-Position: refs/heads/master@{#470950}

[modify] https://crrev.com/377048614cb8970f8c621ebb059deb223b1c8b00/chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc

Project Member

Comment 22 by bugdroid1@chromium.org, May 20 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d2867779401995106d046e3c7b0c926a9aa6b3e2

commit d2867779401995106d046e3c7b0c926a9aa6b3e2
Author: treib <treib@chromium.org>
Date: Sat May 20 00:01:08 2017

Tab.NewTabOnload.Local histogram: Add split into Google/Other

With various changes coming to the Google local NTP, it'll be useful to
be able to differentiate it from the non-Google local NTP.

(Also, take over ownership of the histogram from people who don't work
in this area anymore.)

BUG= 583292 

Review-Url: https://codereview.chromium.org/2897523004
Cr-Commit-Position: refs/heads/master@{#473389}

[modify] https://crrev.com/d2867779401995106d046e3c7b0c926a9aa6b3e2/chrome/browser/ui/search/search_tab_helper.cc
[modify] https://crrev.com/d2867779401995106d046e3c7b0c926a9aa6b3e2/tools/metrics/histograms/histograms.xml

Comment 23 by roy...@google.com, May 31 2017

Is this issue resolved now ?
I'm not sure if Marc has any other plans before closing this out (he's OOO) but it's implemented and you can try it out with this flag:
chrome://flags/#one-google-bar-on-local-ntp

You probably would want chrome://flags/#use-google-local-ntp enabled too, though even if it's not you can navigate directly to chrome-search://local-ntp/local-ntp.html
(ah, and you'd need to be on Canary or Dev. I think the Beta release that contains it is scheduled for late next week)
Project Member

Comment 26 by bugdroid1@chromium.org, Jun 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dca221e3fe17969f0ebd5f612b77cd0be0ff1fb3

commit dca221e3fe17969f0ebd5f612b77cd0be0ff1fb3
Author: Marc Treib <treib@chromium.org>
Date: Mon Jun 12 15:37:34 2017

Tab.NewTabOnload histogram: Remove .LocalGoogle vs .LocalOther split

This essentially reverts crrev.com/2897523004. The split introduced
there doesn't work: The "Google vs. non-Google" check is based on the
URL, but the local NTP URL is always the same.

Bug:  583292 
Change-Id: Ia7854a0c694c11d86ebaa853849a29e04dd81dde
Reviewed-on: https://chromium-review.googlesource.com/527794
Reviewed-by: Chris Pickel <sfiera@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478632}
[modify] https://crrev.com/dca221e3fe17969f0ebd5f612b77cd0be0ff1fb3/chrome/browser/ui/search/search_tab_helper.cc
[modify] https://crrev.com/dca221e3fe17969f0ebd5f612b77cd0be0ff1fb3/tools/metrics/histograms/histograms.xml

Project Member

Comment 27 by bugdroid1@chromium.org, Jun 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc5a62447efda4feda47694477b17ff7296a8454

commit fc5a62447efda4feda47694477b17ff7296a8454
Author: Marc Treib <treib@chromium.org>
Date: Mon Jun 12 17:50:51 2017

NewTabPage.LoadTime histograms: Add splits for .Google vs .Other

Semi-related cleanup: Merge the DefaultSearchProviderIsGoogle
implementations between search.cc and local_ntp_source.cc.

Bug:  583292 
Change-Id: I47f0a2b1eeb2dfd6e117e81b332a44f09b7e8ee8
Reviewed-on: https://chromium-review.googlesource.com/527437
Reviewed-by: Chris Pickel <sfiera@chromium.org>
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#478672}
[modify] https://crrev.com/fc5a62447efda4feda47694477b17ff7296a8454/chrome/browser/search/local_ntp_source.cc
[modify] https://crrev.com/fc5a62447efda4feda47694477b17ff7296a8454/chrome/browser/search/search.cc
[modify] https://crrev.com/fc5a62447efda4feda47694477b17ff7296a8454/chrome/browser/search/search.h
[modify] https://crrev.com/fc5a62447efda4feda47694477b17ff7296a8454/chrome/browser/ui/search/ntp_user_data_logger.cc
[modify] https://crrev.com/fc5a62447efda4feda47694477b17ff7296a8454/chrome/browser/ui/search/ntp_user_data_logger.h
[modify] https://crrev.com/fc5a62447efda4feda47694477b17ff7296a8454/chrome/browser/ui/search/ntp_user_data_logger_unittest.cc
[modify] https://crrev.com/fc5a62447efda4feda47694477b17ff7296a8454/tools/metrics/histograms/histograms.xml

Project Member

Comment 28 by bugdroid1@chromium.org, Jun 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9c2b79657ad9120f2480067166bec92d0dccc24a

commit 9c2b79657ad9120f2480067166bec92d0dccc24a
Author: Marc Treib <treib@chromium.org>
Date: Fri Jun 23 13:28:01 2017

Local NTP OneGoogleBar: In case of network errors, keep the cached data

Before, we'd clear the cached data when the refresh request failed due
to e.g. missing network connectivity. Now, we keep the cache in that
case. We still clear the cache if the server responds with an HTTP
error, or when it returns invalid/unexpected data.

Bug:  583292 
Change-Id: I974ec774591d77bb11417a7cafdb511934250fcb
Reviewed-on: https://chromium-review.googlesource.com/541357
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481866}
[modify] https://crrev.com/9c2b79657ad9120f2480067166bec92d0dccc24a/chrome/browser/search/one_google_bar/one_google_bar_fetcher.h
[modify] https://crrev.com/9c2b79657ad9120f2480067166bec92d0dccc24a/chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc
[modify] https://crrev.com/9c2b79657ad9120f2480067166bec92d0dccc24a/chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.h
[modify] https://crrev.com/9c2b79657ad9120f2480067166bec92d0dccc24a/chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl_unittest.cc
[modify] https://crrev.com/9c2b79657ad9120f2480067166bec92d0dccc24a/chrome/browser/search/one_google_bar/one_google_bar_service.cc
[modify] https://crrev.com/9c2b79657ad9120f2480067166bec92d0dccc24a/chrome/browser/search/one_google_bar/one_google_bar_service.h
[modify] https://crrev.com/9c2b79657ad9120f2480067166bec92d0dccc24a/chrome/browser/search/one_google_bar/one_google_bar_service_observer.h
[modify] https://crrev.com/9c2b79657ad9120f2480067166bec92d0dccc24a/chrome/browser/search/one_google_bar/one_google_bar_service_unittest.cc
[modify] https://crrev.com/9c2b79657ad9120f2480067166bec92d0dccc24a/chrome/browser/ui/search/local_ntp_browsertest.cc

Comment 29 by treib@chromium.org, Jul 10 2017

Blockedon: 740462
Project Member

Comment 30 by bugdroid1@chromium.org, Aug 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5fe4e133bd914b19a5d87c4a87de3a425c1dffbd

commit 5fe4e133bd914b19a5d87c4a87de3a425c1dffbd
Author: Marc Treib <treib@chromium.org>
Date: Wed Aug 02 07:44:47 2017

OneGoogleBar on local NTP: Simplify injection into the page

Before this CL, the OGB was served through multiple "files" under
chrome-search://local-ntp/one-google/. After this CL, all the data is
sent in a single JS dictionary. This simplifies the injection (no more
waiting for all the individual files to load), and it ensures that all
the data actually comes from the same API response.

Bug:  583292 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I50a2a6f61a1ac59fef4f908ff6e009f485e61096
Reviewed-on: https://chromium-review.googlesource.com/596051
Reviewed-by: Chris Pickel <sfiera@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491301}
[modify] https://crrev.com/5fe4e133bd914b19a5d87c4a87de3a425c1dffbd/chrome/browser/resources/local_ntp/local_ntp.js
[modify] https://crrev.com/5fe4e133bd914b19a5d87c4a87de3a425c1dffbd/chrome/browser/search/local_ntp_source.cc

Blockedon: 753383
Project Member

Comment 32 by bugdroid1@chromium.org, Aug 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e4e6dc2fc6767b08bca551ad058459ccc08a263b

commit e4e6dc2fc6767b08bca551ad058459ccc08a263b
Author: Marc Treib <treib@chromium.org>
Date: Wed Aug 09 09:35:06 2017

OneGoogleBarFetcher: abandon ongoing request on Refresh

It's possible that something changed in the meantime, e.g. cookies, so
it's safer to start a fresh request.

Bug:  583292 
Change-Id: I848c09d0fa3430efee4e4a499a5d936a23170c26
Reviewed-on: https://chromium-review.googlesource.com/606287
Reviewed-by: Chris Pickel <sfiera@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492920}
[modify] https://crrev.com/e4e6dc2fc6767b08bca551ad058459ccc08a263b/chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.cc
[modify] https://crrev.com/e4e6dc2fc6767b08bca551ad058459ccc08a263b/chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl.h
[modify] https://crrev.com/e4e6dc2fc6767b08bca551ad058459ccc08a263b/chrome/browser/search/one_google_bar/one_google_bar_fetcher_impl_unittest.cc

Comment 33 by treib@chromium.org, Aug 21 2017

Labels: M-62

Comment 34 by treib@chromium.org, Aug 23 2017

Blocking: 696242

Comment 35 by treib@chromium.org, Aug 24 2017

This is done. Keeping the bug open until it's default-enabled on trunk.
Project Member

Comment 36 by bugdroid1@chromium.org, Oct 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/299dc40a51976bd26f7845ea1231111bb18faae0

commit 299dc40a51976bd26f7845ea1231111bb18faae0
Author: Marc Treib <treib@chromium.org>
Date: Mon Oct 02 16:08:51 2017

Local NTP: Load OGB only after the tiles have loaded

Turns out injectOneGoogleBar takes ~100ms. Let's not do that before the
tiles have finished loading, otherwise we risk slowing that down.

Bug:  583292 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: I99c908bc0486cbae2fd3b50bf5945533acfd4c3f
Reviewed-on: https://chromium-review.googlesource.com/695302
Reviewed-by: Chris Pickel <sfiera@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505632}
[modify] https://crrev.com/299dc40a51976bd26f7845ea1231111bb18faae0/chrome/browser/resources/local_ntp/local_ntp.js

Project Member

Comment 37 by bugdroid1@chromium.org, Oct 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/89b5c3ba0427dd52df552d63238f3a0c4377861a

commit 89b5c3ba0427dd52df552d63238f3a0c4377861a
Author: Marc Treib <treib@chromium.org>
Date: Wed Oct 04 09:13:00 2017

Enable OneGoogleBarOnLocalNtp by default

and remove the fieldtrial testing config

Bug:  583292 
Change-Id: I35385a4cc12fc00353c258d4d1311d96e2950ce6
Reviewed-on: https://chromium-review.googlesource.com/692854
Reviewed-by: Robert Kaplow <rkaplow@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506344}
[modify] https://crrev.com/89b5c3ba0427dd52df552d63238f3a0c4377861a/chrome/common/chrome_features.cc
[modify] https://crrev.com/89b5c3ba0427dd52df552d63238f3a0c4377861a/testing/variations/fieldtrial_testing_config.json

Project Member

Comment 38 by bugdroid1@chromium.org, Oct 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/250a2041e9bbfdf4858e48feb75f44abd438460c

commit 250a2041e9bbfdf4858e48feb75f44abd438460c
Author: Marc Treib <treib@chromium.org>
Date: Wed Oct 04 09:22:39 2017

Local NTP: Add histogram NewTabPage.OneGoogleBar.ShownTime

Bug:  583292 
Cq-Include-Trybots: master.tryserver.chromium.linux:closure_compilation
Change-Id: Ia6de50ccd0393c37a946765ebb1d530b2e1059cb
Reviewed-on: https://chromium-review.googlesource.com/695343
Reviewed-by: Chris Pickel <sfiera@chromium.org>
Reviewed-by: Jesse Doherty <jwd@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506346}
[modify] https://crrev.com/250a2041e9bbfdf4858e48feb75f44abd438460c/chrome/browser/resources/local_ntp/local_ntp.js
[modify] https://crrev.com/250a2041e9bbfdf4858e48feb75f44abd438460c/chrome/browser/ui/search/ntp_user_data_logger.cc
[modify] https://crrev.com/250a2041e9bbfdf4858e48feb75f44abd438460c/chrome/common/search/ntp_logging_events.h
[modify] https://crrev.com/250a2041e9bbfdf4858e48feb75f44abd438460c/tools/metrics/histograms/histograms.xml

Comment 39 by treib@chromium.org, Oct 11 2017

Status: Fixed (was: Started)

Sign in to add a comment