Use new default favicon asset in tab grid |
||||||||||||||||
Issue descriptionSee Issue 847795
,
Jul 4
,
Jul 13
,
Jul 13
CL in-flight.
,
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
,
Jul 16
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.
,
Jul 17
In my opinion, the icon is slightly too big and it looks very prominent in incognito as the background for the asset is white.
,
Jul 17
I'm temporarily assigning this to Martijn to give guidance oh how it should look/be tinted with the new fallback icon.
,
Jul 17
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
,
Jul 20
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?
,
Jul 20
,
Jul 24
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.
,
Jul 25
re-load-shedding to lod@
,
Jul 26
I exported the assets pre-tinted here: https://drive.google.com/corp/drive/u/0/folders/1aiZNckPquXL1dwiWNmncraDJ1WN6W01N
,
Jul 26
The title to close button spacing was fixed here https://bugs.chromium.org/p/chromium/issues/detail?id=862343
,
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
,
Jul 26
,
Jul 26
,
Jul 31
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.
,
Jul 31
Please add merge request label if this needs to be merged to M69.
,
Aug 1
,
Aug 1
,
Aug 2
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
,
Aug 3
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
,
Aug 8
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 |
||||||||||||||||
Comment 1 by mard...@chromium.org
, Jul 4Status: Available (was: Untriaged)