[Mac] Desaturate the Favicon for Network Errors

[Mac] Desaturate the Favicon for Network Errors

BUG=644369
Committed: https://crrev.com/891bc75c4c20d091b55ca5267e547d9f9749155d
Cr-Commit-Position: refs/heads/master@{#419854}

[spqchan, 2016-09-08 00:33:39]

spqchan@chromium.org changed reviewers:
+ shrike@chromium.org

[spqchan, 2016-09-08 00:33:39]

PTAL

[shrike, 2016-09-08 16:21:58]

Hi spqchan@, what's a good URL (or steps) to use to test this change?

[spqchan, 2016-09-08 16:31:56]

On 2016/09/08 16:21:58, shrike wrote:
> Hi spqchan@, what's a good URL (or steps) to use to test this change?

1) Go to any URL with a favicon (youtube.com) and wait for the favicon to load
2) Turn off wifi
3) Refresh

[shrike, 2016-09-08 16:40:39]

lgtm

[spqchan, 2016-09-08 16:46:23]

spqchan@chromium.org changed reviewers:
+ thakis@chromium.org

[spqchan, 2016-09-08 16:46:24]

Thanks!

+thakis for OWNERS

[Nico, 2016-09-08 20:54:12]

https://codereview.chromium.org/2321833002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tab_contents/favicon_util_mac.mm (right):

https://codereview.chromium.org/2321833002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_contents/favicon_util_mac.mm:53:
gfx::ImageSkiaOperations::CreateHSLShiftedImage(*image_skia, shift);
it feels like this should be code that's shared between ports (a function that
takes a naventry and a page type, and that returns the gfx::ImageSkia to use)

[spqchan, 2016-09-12 17:10:36]

spqchan@chromium.org changed reviewers:
+ edwardjung@chromium.org

[spqchan, 2016-09-12 17:10:37]

Good point. I moved the favicon desaturating logic out into the GetFavicon.
Unfortunately, I'm unable to test it on Views right now since the MacViews gn
build is currently broken. 

However, edwardjung, can you take a look to see if it's alright? I reverted your
CL and moved your logic into content_favicon_driver.cc

[edwardjung, 2016-09-13 11:40:44]

On 2016/09/12 17:10:37, spqchan wrote:
> Good point. I moved the favicon desaturating logic out into the GetFavicon.
> Unfortunately, I'm unable to test it on Views right now since the MacViews gn
> build is currently broken. 
> 
> However, edwardjung, can you take a look to see if it's alright? I reverted
your
> CL and moved your logic into content_favicon_driver.cc

LGTM, if the build looks the same as currently in canary. Thanks for the
refactor. That's probably what I should have done initially.

[spqchan, 2016-09-14 21:14:09]

On 2016/09/13 11:40:44, edwardjung wrote:
> On 2016/09/12 17:10:37, spqchan wrote:
> > Good point. I moved the favicon desaturating logic out into the GetFavicon.
> > Unfortunately, I'm unable to test it on Views right now since the MacViews
gn
> > build is currently broken. 
> > 
> > However, edwardjung, can you take a look to see if it's alright? I reverted
> your
> > CL and moved your logic into content_favicon_driver.cc
> 
> LGTM, if the build looks the same as currently in canary. Thanks for the
> refactor. That's probably what I should have done initially.

Awesome, thanks! Just tested it out on Views and it looks fine too. Nico, can
you have a look and let me know if this looks alright?

[Nico, 2016-09-15 16:58:37]

lgtm, thanks.

the desaturated version never gets stored in the favicon db, right? it happens
after the favicon db load, and a desaturated image will never be stored there?

https://codereview.chromium.org/2321833002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tab_contents/favicon_util_mac.mm (right):

https://codereview.chromium.org/2321833002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_contents/favicon_util_mac.mm:34: // The |image|
could be nil if the bitmap is null. In that case, fallback
Is this still true now that you switched the call above from AsNSImage() to
ToNSImage()? I though the main difference between the two is that the former
asserts when called on an empty bitmap, while the latter returns nil there.

https://codereview.chromium.org/2321833002/diff/40001/components/favicon/cont...
File components/favicon/content/content_favicon_driver.cc (right):

https://codereview.chromium.org/2321833002/diff/40001/components/favicon/cont...
components/favicon/content/content_favicon_driver.cc:37: if (image) {
Can this ever be not true? I thought ToImageSkia() never returned null (?)

[spqchan, 2016-09-15 22:07:03]

spqchan@chromium.org changed reviewers:
+ brettw@chromium.org

[spqchan, 2016-09-15 22:07:05]

That's a good point. AFAIK, it's fine but it's pretty scary. I added a param to
GetFavicon() so hopefully this will be less risky. WDYT?

+brettw for OWNERS of components/favicon

https://codereview.chromium.org/2321833002/diff/40001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/tab_contents/favicon_util_mac.mm (right):

https://codereview.chromium.org/2321833002/diff/40001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/tab_contents/favicon_util_mac.mm:34: // The |image|
could be nil if the bitmap is null. In that case, fallback
On 2016/09/15 16:58:37, Nico wrote:
> Is this still true now that you switched the call above from AsNSImage() to
> ToNSImage()? I though the main difference between the two is that the former
> asserts when called on an empty bitmap, while the latter returns nil there.

Good point, the change was accidental

https://codereview.chromium.org/2321833002/diff/40001/components/favicon/cont...
File components/favicon/content/content_favicon_driver.cc (right):

https://codereview.chromium.org/2321833002/diff/40001/components/favicon/cont...
components/favicon/content/content_favicon_driver.cc:37: if (image) {
On 2016/09/15 16:58:37, Nico wrote:
> Can this ever be not true? I thought ToImageSkia() never returned null (?)

Sure thing, I was just cautious

[spqchan, 2016-09-19 19:45:03]

spqchan@chromium.org changed reviewers:
+ sky@chromium.org
- brettw@chromium.org

[spqchan, 2016-09-19 19:45:04]

+sky for compontents/favicon owner

[sky, 2016-09-19 19:59:18]

The desaturation should be moved to the place that wants to show it, not the
core favicon ode.

[sky, 2016-09-19 19:59:36]

On 2016/09/19 19:59:18, sky wrote:
> The desaturation should be moved to the place that wants to show it, not the
> core favicon ode.

'code' that is.

[spqchan, 2016-09-19 20:49:09]

On 2016/09/19 19:59:36, sky wrote:
> On 2016/09/19 19:59:18, sky wrote:
> > The desaturation should be moved to the place that wants to show it, not the
> > core favicon ode.
> 
> 'code' that is.

That's a good point. However, if we want to share the code between platforms, I
can't really move it into the place that wants to show it.
Do you have any suggestions where I can put this? Perhaps I can add a helper
method in "components/favicon_base/favicon_util.h" that will desaturate the
favicon if the current entry contains a navigation error?

[sky, 2016-09-19 22:46:31]

On 2016/09/19 20:49:09, spqchan wrote:
> On 2016/09/19 19:59:36, sky wrote:
> > On 2016/09/19 19:59:18, sky wrote:
> > > The desaturation should be moved to the place that wants to show it, not
the
> > > core favicon ode.
> > 
> > 'code' that is.
> 
> That's a good point. However, if we want to share the code between platforms,
I
> can't really move it into the place that wants to show it.
> Do you have any suggestions where I can put this? Perhaps I can add a helper
> method in "components/favicon_base/favicon_util.h" that will desaturate the
> favicon if the current entry contains a navigation error?

Are the only consumers of this code in chrome/browser/ui/* ? If so, how about
chrome/browser/ui/tabs?

[spqchan, 2016-09-20 00:26:07]

On 2016/09/19 22:46:31, sky wrote:
> On 2016/09/19 20:49:09, spqchan wrote:
> > On 2016/09/19 19:59:36, sky wrote:
> > > On 2016/09/19 19:59:18, sky wrote:
> > > > The desaturation should be moved to the place that wants to show it, not
> the
> > > > core favicon ode.
> > > 
> > > 'code' that is.
> > 
> > That's a good point. However, if we want to share the code between
platforms,
> I
> > can't really move it into the place that wants to show it.
> > Do you have any suggestions where I can put this? Perhaps I can add a helper
> > method in "components/favicon_base/favicon_util.h" that will desaturate the
> > favicon if the current entry contains a navigation error?
> 
> Are the only consumers of this code in chrome/browser/ui/* ? If so, how about
> chrome/browser/ui/tabs?

Sounds good! I can put it inside chrome/browser/ui/tabs/tab_utils

[sky, 2016-09-20 02:36:50]

tab_utils is too easy to become a dumping ground. How about
favicon_utils? A little more specific.

On Mon, Sep 19, 2016 at 5:26 PM,  <spqchan@chromium.org> wrote:
> On 2016/09/19 22:46:31, sky wrote:
>> On 2016/09/19 20:49:09, spqchan wrote:
>> > On 2016/09/19 19:59:36, sky wrote:
>> > > On 2016/09/19 19:59:18, sky wrote:
>> > > > The desaturation should be moved to the place that wants to show it,
>> > > > not
>> the
>> > > > core favicon ode.
>> > >
>> > > 'code' that is.
>> >
>> > That's a good point. However, if we want to share the code between
> platforms,
>> I
>> > can't really move it into the place that wants to show it.
>> > Do you have any suggestions where I can put this? Perhaps I can add a
>> > helper
>> > method in "components/favicon_base/favicon_util.h" that will desaturate
>> > the
>> > favicon if the current entry contains a navigation error?
>>
>> Are the only consumers of this code in chrome/browser/ui/* ? If so, how
>> about
>> chrome/browser/ui/tabs?
>
> Sounds good! I can put it inside chrome/browser/ui/tabs/tab_utils
>
> https://codereview.chromium.org/2321833002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[spqchan, 2016-09-20 02:39:17]

On 2016/09/20 02:36:50, sky wrote:
> tab_utils is too easy to become a dumping ground. How about
> favicon_utils? A little more specific.
> 
> On Mon, Sep 19, 2016 at 5:26 PM,  <mailto:spqchan@chromium.org> wrote:
> > On 2016/09/19 22:46:31, sky wrote:
> >> On 2016/09/19 20:49:09, spqchan wrote:
> >> > On 2016/09/19 19:59:36, sky wrote:
> >> > > On 2016/09/19 19:59:18, sky wrote:
> >> > > > The desaturation should be moved to the place that wants to show it,
> >> > > > not
> >> the
> >> > > > core favicon ode.
> >> > >
> >> > > 'code' that is.
> >> >
> >> > That's a good point. However, if we want to share the code between
> > platforms,
> >> I
> >> > can't really move it into the place that wants to show it.
> >> > Do you have any suggestions where I can put this? Perhaps I can add a
> >> > helper
> >> > method in "components/favicon_base/favicon_util.h" that will desaturate
> >> > the
> >> > favicon if the current entry contains a navigation error?
> >>
> >> Are the only consumers of this code in chrome/browser/ui/* ? If so, how
> >> about
> >> chrome/browser/ui/tabs?
> >
> > Sounds good! I can put it inside chrome/browser/ui/tabs/tab_utils
> >
> > https://codereview.chromium.org/2321833002/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

Sure, I can do that

[spqchan, 2016-09-20 19:53:14]

PTAL

[sky, 2016-09-20 20:15:06]

LGTM - Thanks!

[spqchan, 2016-09-20 20:17:38]

The CQ bit was checked by spqchan@chromium.org

[spqchan, 2016-09-20 20:17:39]

The patchset sent to the CQ was uploaded after l-g-t-m from shrike@chromium.org,
edwardjung@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2321833002/#ps120001
(title: "Fix for Views")

[commit-bot: I haz the power, 2016-09-20 20:18:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 21:10:31]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-20 21:11:47]

Description was changed from

==========
[Mac] Desaturate the Favicon for Network Errors

BUG=644369
==========

to

==========
[Mac] Desaturate the Favicon for Network Errors

BUG=644369

Committed: https://crrev.com/891bc75c4c20d091b55ca5267e547d9f9749155d
Cr-Commit-Position: refs/heads/master@{#419854}
==========

[commit-bot: I haz the power, 2016-09-20 21:11:48]

Patchset 7 (id:??) landed as
https://crrev.com/891bc75c4c20d091b55ca5267e547d9f9749155d
Cr-Commit-Position: refs/heads/master@{#419854}