New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 862786 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jul 27
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Revise HomepageClinet.isNewTabPageUsedAsHomepage

Project Member Reported by romax@chromium.org, Jul 11

Issue description

In the CL: https://chromium-review.googlesource.com/c/chromium/src/+/1132613, quoting a comment from twellington@:
"""
Not for this patch, but in perhaps a next, for our new purposes, should this evolve into isHomePageNTPorGoogle (or something like that)?
"""

Since we probably do not want to pin google.com as a tile.
 
Seems like this is can be combined with other methods in HomepageClient interface and live on Java side only. I think we can add the check in isHomepageTileEnabled (from isHomepageEnabled) and eliminate HomepageClient.isNewTabPageUsedAsHomepage().
Labels: -Pri-3 Pri-2
Owner: romax@chromium.org
Status: Assigned (was: Untriaged)
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 18

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

commit ddf0db038974356eb6f09cc36f799278639ee0d0
Author: Yafei Duan <romax@chromium.org>
Date: Wed Jul 18 17:21:47 2018

[NTP tiles] Removing redundant interface from HomepageClient.

Removing the isNewTabPageUsedAsHomepage from HomepageClient interface,
since it can be combined with isHomepageEnabled and was only used in a
check on native side along with isHomepageEnabled. So those two methods
are combined on Java side to make interface simpler.

Bug:  862786 
Change-Id: I32f112590afe206da8d3d2264e36202d8cf2b9ed
Reviewed-on: https://chromium-review.googlesource.com/1140691
Reviewed-by: Theresa <twellington@chromium.org>
Reviewed-by: Friedrich Horschig <fhorschig@chromium.org>
Commit-Queue: Yafei Duan <romax@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576106}
[modify] https://crrev.com/ddf0db038974356eb6f09cc36f799278639ee0d0/chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSites.java
[modify] https://crrev.com/ddf0db038974356eb6f09cc36f799278639ee0d0/chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java
[modify] https://crrev.com/ddf0db038974356eb6f09cc36f799278639ee0d0/chrome/browser/android/ntp/most_visited_sites_bridge.cc
[modify] https://crrev.com/ddf0db038974356eb6f09cc36f799278639ee0d0/components/ntp_tiles/most_visited_sites.cc
[modify] https://crrev.com/ddf0db038974356eb6f09cc36f799278639ee0d0/components/ntp_tiles/most_visited_sites.h
[modify] https://crrev.com/ddf0db038974356eb6f09cc36f799278639ee0d0/components/ntp_tiles/most_visited_sites_unittest.cc

Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 27

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

commit ef042f5e7aeabe3baaf865a5d089b5255af64f30
Author: Yafei Duan <romax@chromium.org>
Date: Fri Jul 27 21:05:19 2018

[NTP tiles] Don't enable homepage tile if home page url is google.com.

Homepage tile should not be added to Most Visited tiles if the homepage
url is google.com.

Bug:  862786 
Change-Id: Icb42ce4fb5f0c26bece80b7a7dd82bfd8526bcb2
Reviewed-on: https://chromium-review.googlesource.com/1152298
Reviewed-by: Theresa <twellington@chromium.org>
Commit-Queue: Yafei Duan <romax@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578788}
[modify] https://crrev.com/ef042f5e7aeabe3baaf865a5d089b5255af64f30/chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java

Status: Fixed (was: Started)
Hi all,

In order to allow for some non-NTP Home page continuity for users as we launch this, can we please revert this change, such that any non-NTP Home page (including google.com and DSEs) gets pinned to the 1st Most Visited tile?
reverted change mentioned in #5, so that google.com will be pinned as a tile if it's set as the home page url.
Project Member

Comment 9 by bugdroid1@chromium.org, Jul 31

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

commit 4d7bb41c2908b8200a9caf9c59742937549b58ea
Author: Yafei Duan <romax@chromium.org>
Date: Tue Jul 31 23:36:57 2018

Revert "[NTP tiles] Don't enable homepage tile if home page url is google.com."

This reverts commit ef042f5e7aeabe3baaf865a5d089b5255af64f30.

Reason for revert: We don't want to skip pinning google.com as tile, per discussion with nickrad@.

Original change's description:
> [NTP tiles] Don't enable homepage tile if home page url is google.com.
> 
> Homepage tile should not be added to Most Visited tiles if the homepage
> url is google.com.
> 
> Bug:  862786 
> Change-Id: Icb42ce4fb5f0c26bece80b7a7dd82bfd8526bcb2
> Reviewed-on: https://chromium-review.googlesource.com/1152298
> Reviewed-by: Theresa <twellington@chromium.org>
> Commit-Queue: Yafei Duan <romax@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#578788}

TBR=twellington@chromium.org,romax@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  862786 
Change-Id: Ifca7d377d5b3ff527cddbecafc63b3909c41cd53
Reviewed-on: https://chromium-review.googlesource.com/1157385
Reviewed-by: Yafei Duan <romax@chromium.org>
Commit-Queue: Yafei Duan <romax@chromium.org>
Cr-Commit-Position: refs/heads/master@{#579620}
[modify] https://crrev.com/4d7bb41c2908b8200a9caf9c59742937549b58ea/chrome/android/java/src/org/chromium/chrome/browser/suggestions/MostVisitedSitesBridge.java

Sign in to add a comment