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

Issue 853780 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 6
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocked on:
issue 856850
issue 856852
issue 856853

Blocking:
issue 850339



Sign in to add a comment

[New Tab Page] Use a single source to return all icons.

Project Member Reported by ma...@chromium.org, Jun 18 2018

Issue description

The priority order for icons should be 

1) local favicon
2) server favicon
3) Local fallback (letter in a circle).
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 18 2018

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

commit 833b9e10405a56bb346c9b46a62f5e53a64d8469
Author: Mathieu Perreault <mathp@chromium.org>
Date: Mon Jun 18 19:29:29 2018

[NTP] Fix copy paste error.

Code was referencing undefined variables.

Bug:  853780 
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: Ie50c336424a45f780b854b35074b788be65a9368
Reviewed-on: https://chromium-review.googlesource.com/1104779
Reviewed-by: Moe Ahmadi <mahmadi@chromium.org>
Commit-Queue: Mathieu Perreault <mathp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568106}
[modify] https://crrev.com/833b9e10405a56bb346c9b46a62f5e53a64d8469/chrome/browser/resources/local_ntp/most_visited_single.js

Labels: zine-triaged
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 21 2018

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

commit 77c1f66815af0633a2d4efd38274269551b538ad
Author: Mathieu Perreault <mathp@chromium.org>
Date: Thu Jun 21 23:06:26 2018

[New Tab Page] Create a source for NTP icons.

For privacy reasons, the native code will have to choose which icon
is served within a single source (so the implementer cannot deduct
history). The priority order is the local favicon, the server favicon
or a letter+circle fallback. This CL implements the first and the
last of those, and further CLs will improve the logic.

Bug:  853780 
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: I6f56d02f3c953964e4968f191b8fb3177998c962
Reviewed-on: https://chromium-review.googlesource.com/1105508
Commit-Queue: Mathieu Perreault <mathp@chromium.org>
Reviewed-by: Scott Chen <scottchen@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569414}
[modify] https://crrev.com/77c1f66815af0633a2d4efd38274269551b538ad/chrome/browser/BUILD.gn
[modify] https://crrev.com/77c1f66815af0633a2d4efd38274269551b538ad/chrome/browser/resources/local_ntp/most_visited_single.css
[modify] https://crrev.com/77c1f66815af0633a2d4efd38274269551b538ad/chrome/browser/resources/local_ntp/most_visited_single.js
[modify] https://crrev.com/77c1f66815af0633a2d4efd38274269551b538ad/chrome/browser/search/instant_service.cc
[add] https://crrev.com/77c1f66815af0633a2d4efd38274269551b538ad/chrome/browser/search/ntp_icon_source.cc
[add] https://crrev.com/77c1f66815af0633a2d4efd38274269551b538ad/chrome/browser/search/ntp_icon_source.h
[modify] https://crrev.com/77c1f66815af0633a2d4efd38274269551b538ad/chrome/common/webui_url_constants.cc
[modify] https://crrev.com/77c1f66815af0633a2d4efd38274269551b538ad/chrome/common/webui_url_constants.h

Cc: pnangunoori@chromium.org
Labels: Needs-Feedback
mathp@ -- Could you please provide any sample steps to verify this fix manually. So that we can verify and add TE-Verified labels from our end.

Thanks!
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 22 2018

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

commit 42eac6f0920315fa30534384fae5a1568f6a8b9d
Author: Mathieu Perreault <mathp@chromium.org>
Date: Fri Jun 22 20:58:53 2018

[NTP] Draw the local favicon image inside a circle.

The chrome-search://ntpicon source will always return an image
that is 40dp: either the local favicon (16dp) drawn in the middle
of a circle, or a fallback letter in a circle.

This simplifies the css and addresses a potential privacy concern.

Bug:  853780 
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: I1eccb79f79ce03434e5af78b48a4ae1ed6fe836f
Reviewed-on: https://chromium-review.googlesource.com/1111912
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: Mathieu Perreault <mathp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569768}
[modify] https://crrev.com/42eac6f0920315fa30534384fae5a1568f6a8b9d/chrome/browser/resources/local_ntp/most_visited_single.css
[modify] https://crrev.com/42eac6f0920315fa30534384fae5a1568f6a8b9d/chrome/browser/resources/local_ntp/most_visited_single.js
[modify] https://crrev.com/42eac6f0920315fa30534384fae5a1568f6a8b9d/chrome/browser/search/ntp_icon_source.cc

Comment 6 by ma...@chromium.org, Jun 27 2018

Blockedon: 856850

Comment 7 by ma...@chromium.org, Jun 27 2018

Blockedon: 856852

Comment 8 by ma...@chromium.org, Jun 27 2018

Blockedon: 856853

Comment 9 by ma...@chromium.org, Jun 27 2018

Status: Assigned (was: Started)
Blocking: 850339
Project Member

Comment 11 by bugdroid1@chromium.org, Jul 10

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

commit 636040e90753c20bd7bf942a3118c5aab19c6b91
Author: Mathieu Perreault <mathp@chromium.org>
Date: Tue Jul 10 18:17:27 2018

[New Tab Page] Specify an image size for the icons

The image size should be 40dp, which is relevant for scale factors other than
1.0 (the returned image will be > 40px but needs to be scaled down).

Regression was introduced in https://chromium.googlesource.com/chromium/src/+/e74260d1dac116a0950c18446b066f2bc5434a01%5E%21/#F0
where an equivalent styling for non-fallbacks limiting to 40dp height/width was
removed.

Bug:  853780 
Cq-Include-Trybots: luci.chromium.try:closure_compilation
Change-Id: Ib93a30faa5fb6573218a3f3fd5467ebfafccb7c0
Reviewed-on: https://chromium-review.googlesource.com/1131594
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Commit-Queue: Mathieu Perreault <mathp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573813}
[modify] https://crrev.com/636040e90753c20bd7bf942a3118c5aab19c6b91/chrome/browser/resources/local_ntp/most_visited_single.css

Project Member

Comment 12 by bugdroid1@chromium.org, Jul 23

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

commit 6b5891cb32f6ed4da4a8d969582d04312ddba2af
Author: Mathieu Perreault <mathp@chromium.org>
Date: Mon Jul 23 22:22:38 2018

[New Tab] Have default background colors for tiles with no favicon.

Bug:  853780 
Test: ntp_render_browsertests
Change-Id: I57d4321f6a72d1171066b1578128117c91521bfd
Reviewed-on: https://chromium-review.googlesource.com/1145693
Commit-Queue: Mathieu Perreault <mathp@chromium.org>
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577298}
[modify] https://crrev.com/6b5891cb32f6ed4da4a8d969582d04312ddba2af/chrome/browser/search/ntp_icon_source.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Jul 25

Labels: merge-merged-3497
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dba741d1e5f814e8cf0571b0a75826dc220a7b41

commit dba741d1e5f814e8cf0571b0a75826dc220a7b41
Author: Mathieu Perreault <mathp@chromium.org>
Date: Wed Jul 25 01:30:38 2018

[New Tab] Have default background colors for tiles with no favicon.

TBR=mathp@chromium.org

(cherry picked from commit 6b5891cb32f6ed4da4a8d969582d04312ddba2af)

Bug:  853780 
Test: ntp_render_browsertests
Change-Id: I57d4321f6a72d1171066b1578128117c91521bfd
Reviewed-on: https://chromium-review.googlesource.com/1145693
Commit-Queue: Mathieu Perreault <mathp@chromium.org>
Reviewed-by: Fernando Serboncini <fserb@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#577298}
Reviewed-on: https://chromium-review.googlesource.com/1149116
Reviewed-by: Mathieu Perreault <mathp@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#60}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/dba741d1e5f814e8cf0571b0a75826dc220a7b41/chrome/browser/search/ntp_icon_source.cc

Cc: yyushkina@chromium.org
Status: Fixed (was: Assigned)

Sign in to add a comment