Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2217273002:
Desaturate the favicon shown in the tab when a network error is encountered (Closed)
|
Created:
4 years, 4 months ago by edwardjung Modified:
4 years, 4 months ago Reviewers:
sky CC:
chromium-reviews, tfarina, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDesaturate the favicon shown in the tab when a network error is encountered.
BUG=630190
Committed: https://crrev.com/0fb4518d83764eb357d3e043c529ae6aeff99ff9
Cr-Commit-Position: refs/heads/master@{#411956}
Patch Set 1 #Patch Set 2 : Merge #
Total comments: 4
Patch Set 3 : Added change for pinned tabs, reused NetworkState enum for net errors #
Total comments: 8
Patch Set 4 : Update desaturation method #Patch Set 5 : Correct style #Patch Set 6 : & #
Total comments: 2
Patch Set 7 : Remove else #
Messages
Total messages: 28 (18 generated)
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Desaturate the favicon when encountering a network error BUG=630190 ========== to ========== Desaturate the favicon shown in the tab when a network error is encountered. BUG=630190 ==========
edwardjung@chromium.org changed reviewers: + sky@chromium.org
sky PTAL at this UI tweak to tab favicons. Windows and Linux screenshots are attached to the bug. Thanks.
Neat effect! I can't quite tell from the bug if ux has signed off on this yet, have they? https://codereview.chromium.org/2217273002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2217273002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1597: icon_canvas.DrawImageInt(favicon_, 0, 0); You'll need to update this too. https://codereview.chromium.org/2217273002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_renderer_data.h (right): https://codereview.chromium.org/2217273002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_renderer_data.h:48: bool network_error; Can this be merged into the NetworkState enum above?
Yes this was approved by UI review. I tied up the loose ends regarding the feedback from the designers with an offline chat with ainslie. Added an update #21. https://bugs.chromium.org/p/chromium/issues/detail?id=630190#c21 https://codereview.chromium.org/2217273002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2217273002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:1597: icon_canvas.DrawImageInt(favicon_, 0, 0); On 2016/08/11 20:46:21, sky wrote: > You'll need to update this too. Thanks, missed this, although I'm not sure when a pinned net error tab would get a title update. https://codereview.chromium.org/2217273002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab_renderer_data.h (right): https://codereview.chromium.org/2217273002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab_renderer_data.h:48: bool network_error; On 2016/08/11 20:46:21, sky wrote: > Can this be merged into the NetworkState enum above? Done. Although not sure it's as clean as we now have to check against two states in tab.cc
https://codereview.chromium.org/2217273002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right): https://codereview.chromium.org/2217273002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:61: if (contents) { This is mildly awkward because of the !contents 1 line above. How about slightly restructuring to: if (!contents) return NONE; if (!contents->IsLoadingToDifferentDocument()) ... https://codereview.chromium.org/2217273002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2217273002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:586: gfx::Rect bounds) { const gfx::Rect& https://codereview.chromium.org/2217273002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:590: SkScalar desaturate[20] = {0.3086f, 0.6094f, 0.0820f, 0.0f, 0.0f, These constants look rather magical. Are they defined as a constants some where? https://codereview.chromium.org/2217273002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:967: // If the network state is none and hasn't changed, do nothing. Otherwise we update comment
Updated, thanks. https://codereview.chromium.org/2217273002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right): https://codereview.chromium.org/2217273002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:61: if (contents) { On 2016/08/12 15:29:25, sky wrote: > This is mildly awkward because of the !contents 1 line above. How about slightly > restructuring to: > > if (!contents) > return NONE; > > if (!contents->IsLoadingToDifferentDocument()) > ... Done. https://codereview.chromium.org/2217273002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/tabs/tab.cc (right): https://codereview.chromium.org/2217273002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:586: gfx::Rect bounds) { On 2016/08/12 15:29:25, sky wrote: > const gfx::Rect& Done. https://codereview.chromium.org/2217273002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:590: SkScalar desaturate[20] = {0.3086f, 0.6094f, 0.0820f, 0.0f, 0.0f, On 2016/08/12 15:29:25, sky wrote: > These constants look rather magical. Are they defined as a constants some where? I've switched to use CreateHSLShiftedImage which has easier to understand parameters using a HSL shift. https://codereview.chromium.org/2217273002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/tabs/tab.cc:967: // If the network state is none and hasn't changed, do nothing. Otherwise we On 2016/08/12 15:29:25, sky wrote: > update comment Done.
LGTM https://codereview.chromium.org/2217273002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right): https://codereview.chromium.org/2217273002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:67: else no else after a return (see chromium style guide).
https://codereview.chromium.org/2217273002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc (right): https://codereview.chromium.org/2217273002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/tabs/browser_tab_strip_controller.cc:67: else On 2016/08/12 21:43:11, sky wrote: > no else after a return (see chromium style guide). Done.
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by edwardjung@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by edwardjung@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/2217273002/#ps120001 (title: "Remove else")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Desaturate the favicon shown in the tab when a network error is encountered. BUG=630190 ========== to ========== Desaturate the favicon shown in the tab when a network error is encountered. BUG=630190 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Desaturate the favicon shown in the tab when a network error is encountered. BUG=630190 ========== to ========== Desaturate the favicon shown in the tab when a network error is encountered. BUG=630190 Committed: https://crrev.com/0fb4518d83764eb357d3e043c529ae6aeff99ff9 Cr-Commit-Position: refs/heads/master@{#411956} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0fb4518d83764eb357d3e043c529ae6aeff99ff9 Cr-Commit-Position: refs/heads/master@{#411956} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
