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

Issue 860245 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Last visit > 30 days ago
Closed: Jul 26
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: iOS
Pri: 1
Type: Bug
Q2



Sign in to add a comment

Use new default favicon asset in tab grid

Project Member Reported by pschaffner@chromium.org, Jul 4

Issue description

Labels: -Pri-2 M-69 Pri-1
Status: Available (was: Untriaged)
This is for the tiles in the tab grid. To reproduce, just open a New tab and see how the thumbnail looks like in tab grid. There is no placeholder/default icon when we don't have a favicon. 

Please assign an owner as you see fit. 
Cc: -edchin@chromium.org
Labels: S-TabGrid-Polish MS-Tab-Grid Q2
Owner: edchin@chromium.org
Status: Assigned (was: Available)
Status: Started (was: Assigned)
CL in-flight.
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 13

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

commit c25d842f0a5ee554730a6c1202806ede3c337d76
Author: edchin <edchin@chromium.org>
Date: Fri Jul 13 21:35:37 2018

[ios] Improve favicon in tab grid

The tab grid cell titles were previously being updated too late
on "page loaded", rather than earlier on "title changed".

This CL also sets a default icon for cells.

This CL also removes an old snapshot in the tab grid once the
title has changed, so that the title and snapshot are in sync.

Video:
https://drive.google.com/open?id=1NvSuQ039jsl0YTqO6ua_IREk7f2Z807C


Bug:  860245 , 853569
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Ic3317ddc50ceb16fe0d89b41e8b26fc2bac203e9
Reviewed-on: https://chromium-review.googlesource.com/1136836
Reviewed-by: Rohit Rao <rohitrao@chromium.org>
Reviewed-by: edchin <edchin@chromium.org>
Commit-Queue: edchin <edchin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#575078}
[modify] https://crrev.com/c25d842f0a5ee554730a6c1202806ede3c337d76/ios/chrome/browser/ui/tab_grid/tab_grid_mediator.mm

Judging from the video, it looks like the "default icon for cells" mentioned in the last commit message is the old one. Make sure to use the new one provided in  issue 847795  and let me know if you need a unique asset cut.
In my opinion, the icon is slightly too big and it looks very prominent in incognito as the background for the asset is white.
Owner: martijnb@chromium.org
I'm temporarily assigning this to Martijn to give guidance oh how it should look/be tinted with the new fallback icon.
These are the tweaks that we would like to do:

- Scaling down all the tile favicons from 22pt to 16pt from the center origin. As a result, the favicon will have a padding of 4pt (blue in the attachment) around all the sides. 

- Remove the grey color that currently appears behind favicon, use transparency instead.

- Replace the favicon fallback icon with this asset: 
https://drive.google.com/corp/drive/u/0/folders/16YQcGUML_VMBk-Ln4DzAgsIz1v7sdmzY

Tinting icon non-incognito: #000000, 30% alpha
Tinting icon incognito: #ffffff, 40% alpha

- Add 4pt spacing between tile title and X button, see  issue 864512 


FaviconFallbackTiles.png
112 KB View Download
Cc: edchin@chromium.org ghendel@chromium.org
martijnb@ - Does comment 9 represent all the input you have? If so, can you reassign this to someone else to make the code change for this?
Cc: -marq@chromium.org
Owner: marq@chromium.org
Cc: marq@chromium.org
Owner: kkhorimoto@chromium.org
Status: Assigned (was: Started)
Load-shedding to kkhorimoto@. 

This should be straightforward. It might be easier to get two new assets instead of one that's tinted for regular or incognito, because the favicon images are passed as images over the grid consumer interface.
Owner: lod@chromium.org
re-load-shedding to lod@
Status: Started (was: Assigned)
The title to close button spacing was fixed here https://bugs.chromium.org/p/chromium/issues/detail?id=862343
Project Member

Comment 16 by bugdroid1@chromium.org, Jul 26

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

commit d7a28ff693b08cc532ffafe9d5af3c0b486fa6ee
Author: Elodie Banel <lod@google.com>
Date: Thu Jul 26 15:57:23 2018

UI polish for favicons in tabgrid

Displaying favicons with clear background, shrinking down to 16pt, and
changing default icon.

Bug:  860245 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I75e75d0bb21784441561e79a623002fdc2c95ae8
Reviewed-on: https://chromium-review.googlesource.com/1151111
Commit-Queue: Elodie Banel <lod@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578313}
[modify] https://crrev.com/d7a28ff693b08cc532ffafe9d5af3c0b486fa6ee/ios/chrome/browser/ui/favicon/BUILD.gn
[modify] https://crrev.com/d7a28ff693b08cc532ffafe9d5af3c0b486fa6ee/ios/chrome/browser/ui/favicon/resources/BUILD.gn
[add] https://crrev.com/d7a28ff693b08cc532ffafe9d5af3c0b486fa6ee/ios/chrome/browser/ui/favicon/resources/default_world_favicon_incognito.imageset/Contents.json
[add] https://crrev.com/d7a28ff693b08cc532ffafe9d5af3c0b486fa6ee/ios/chrome/browser/ui/favicon/resources/default_world_favicon_incognito.imageset/default_world_favicon_incognito.png
[add] https://crrev.com/d7a28ff693b08cc532ffafe9d5af3c0b486fa6ee/ios/chrome/browser/ui/favicon/resources/default_world_favicon_incognito.imageset/default_world_favicon_incognito@2x.png
[add] https://crrev.com/d7a28ff693b08cc532ffafe9d5af3c0b486fa6ee/ios/chrome/browser/ui/favicon/resources/default_world_favicon_incognito.imageset/default_world_favicon_incognito@3x.png
[add] https://crrev.com/d7a28ff693b08cc532ffafe9d5af3c0b486fa6ee/ios/chrome/browser/ui/favicon/resources/default_world_favicon_regular.imageset/Contents.json
[add] https://crrev.com/d7a28ff693b08cc532ffafe9d5af3c0b486fa6ee/ios/chrome/browser/ui/favicon/resources/default_world_favicon_regular.imageset/default_world_favicon_regular.png
[add] https://crrev.com/d7a28ff693b08cc532ffafe9d5af3c0b486fa6ee/ios/chrome/browser/ui/favicon/resources/default_world_favicon_regular.imageset/default_world_favicon_regular@2x.png
[add] https://crrev.com/d7a28ff693b08cc532ffafe9d5af3c0b486fa6ee/ios/chrome/browser/ui/favicon/resources/default_world_favicon_regular.imageset/default_world_favicon_regular@3x.png
[modify] https://crrev.com/d7a28ff693b08cc532ffafe9d5af3c0b486fa6ee/ios/chrome/browser/ui/tab_grid/grid/grid_cell.mm
[modify] https://crrev.com/d7a28ff693b08cc532ffafe9d5af3c0b486fa6ee/ios/chrome/browser/ui/tab_grid/grid/grid_constants.mm
[modify] https://crrev.com/d7a28ff693b08cc532ffafe9d5af3c0b486fa6ee/ios/chrome/browser/ui/tab_grid/tab_grid_mediator.mm

Status: Fixed (was: Started)
Labels: Merge-TBD
Status: Verified (was: Fixed)
Verified on chrome canary version 70.0.3508.0 on iPhone 8 plus with iOS 11.4.1, 12 beta 4 following the steps mentioned in comment #5 video.  
As per comment #7 verified "default icon for cells" and it is smaller. 

Verified issue mentioned in comment #15, no overlapping is noticed between page title and "x" button.
Please add merge request label if this needs to be merged to M69.
Labels: Merge-Request-69
Labels: -Merge-TBD
Project Member

Comment 23 by sheriffbot@chromium.org, Aug 2

Labels: -Merge-Request-69 Hotlist-Merge-Approved Merge-Approved-69
Your change meets the bar and is auto-approved for M69. Please go ahead and merge the CL to branch 3497 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 24 by bugdroid1@chromium.org, Aug 3

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

commit 7bbca97bf411ff0d42ebc39078cec2281b95400c
Author: Elodie Banel <lod@google.com>
Date: Fri Aug 03 11:48:06 2018

UI polish for favicons in tabgrid

Displaying favicons with clear background, shrinking down to 16pt, and
changing default icon.

Bug:  860245 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: I75e75d0bb21784441561e79a623002fdc2c95ae8
Reviewed-on: https://chromium-review.googlesource.com/1151111
Commit-Queue: Elodie Banel <lod@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#578313}(cherry picked from commit d7a28ff693b08cc532ffafe9d5af3c0b486fa6ee)
Reviewed-on: https://chromium-review.googlesource.com/1162022
Reviewed-by: Elodie Banel <lod@chromium.org>
Cr-Commit-Position: refs/branch-heads/3497@{#376}
Cr-Branched-From: 271eaf50594eb818c9295dc78d364aea18c82ea8-refs/heads/master@{#576753}
[modify] https://crrev.com/7bbca97bf411ff0d42ebc39078cec2281b95400c/ios/chrome/browser/ui/favicon/BUILD.gn
[modify] https://crrev.com/7bbca97bf411ff0d42ebc39078cec2281b95400c/ios/chrome/browser/ui/favicon/resources/BUILD.gn
[add] https://crrev.com/7bbca97bf411ff0d42ebc39078cec2281b95400c/ios/chrome/browser/ui/favicon/resources/default_world_favicon_incognito.imageset/Contents.json
[add] https://crrev.com/7bbca97bf411ff0d42ebc39078cec2281b95400c/ios/chrome/browser/ui/favicon/resources/default_world_favicon_incognito.imageset/default_world_favicon_incognito.png
[add] https://crrev.com/7bbca97bf411ff0d42ebc39078cec2281b95400c/ios/chrome/browser/ui/favicon/resources/default_world_favicon_incognito.imageset/default_world_favicon_incognito@2x.png
[add] https://crrev.com/7bbca97bf411ff0d42ebc39078cec2281b95400c/ios/chrome/browser/ui/favicon/resources/default_world_favicon_incognito.imageset/default_world_favicon_incognito@3x.png
[add] https://crrev.com/7bbca97bf411ff0d42ebc39078cec2281b95400c/ios/chrome/browser/ui/favicon/resources/default_world_favicon_regular.imageset/Contents.json
[add] https://crrev.com/7bbca97bf411ff0d42ebc39078cec2281b95400c/ios/chrome/browser/ui/favicon/resources/default_world_favicon_regular.imageset/default_world_favicon_regular.png
[add] https://crrev.com/7bbca97bf411ff0d42ebc39078cec2281b95400c/ios/chrome/browser/ui/favicon/resources/default_world_favicon_regular.imageset/default_world_favicon_regular@2x.png
[add] https://crrev.com/7bbca97bf411ff0d42ebc39078cec2281b95400c/ios/chrome/browser/ui/favicon/resources/default_world_favicon_regular.imageset/default_world_favicon_regular@3x.png
[modify] https://crrev.com/7bbca97bf411ff0d42ebc39078cec2281b95400c/ios/chrome/browser/ui/tab_grid/grid/grid_cell.mm
[modify] https://crrev.com/7bbca97bf411ff0d42ebc39078cec2281b95400c/ios/chrome/browser/ui/tab_grid/grid/grid_constants.mm
[modify] https://crrev.com/7bbca97bf411ff0d42ebc39078cec2281b95400c/ios/chrome/browser/ui/tab_grid/tab_grid_mediator.mm

Verified on M69.0.3497.31 beta
iOS: 10.3.3, 12.0 beta#6
iPhone7Plus, iPhoneX, iPad Pro

Sign in to add a comment