Add custom icon support to GlobalError. Show extension icon in permissions increase bubble.

Add custom icon support to GlobalError. Show extension icon in permissions increase bubble.

http://i.imgur.com/VHG6Zbl.png

BUG=229102
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202795

[Yoyo Zhou, 2013-05-23 23:54:37]

sail: please review GlobalError and its views, especially GlobalError interface
changes
mpcomplete: please review extension_disabled_ui

[Matt Perry, 2013-05-24 00:24:03]

https://chromiumcodereview.appspot.com/15745022/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://chromiumcodereview.appspot.com/15745022/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_disabled_ui.cc:356:
base::Bind(&AddExtensionDisabledErrorWithIcon, service, extension));
What happens if |service| or |extension| get deleted before the task is run? I
think you need to:
- Use a WeakPtr for the ExtensionService.
- Pass the extension ID and look it up again when the task runs.

[Yoyo Zhou, 2013-05-24 00:47:11]

https://chromiumcodereview.appspot.com/15745022/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://chromiumcodereview.appspot.com/15745022/diff/1/chrome/browser/extensi...
chrome/browser/extensions/extension_disabled_ui.cc:356:
base::Bind(&AddExtensionDisabledErrorWithIcon, service, extension));
On 2013/05/24 00:24:04, Matt Perry wrote:
> What happens if |service| or |extension| get deleted before the task is run? I
> think you need to:
> - Use a WeakPtr for the ExtensionService.
> - Pass the extension ID and look it up again when the task runs.

Done.

[Matt Perry, 2013-05-24 00:54:44]

lgtm

https://chromiumcodereview.appspot.com/15745022/diff/16002/chrome/browser/ext...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://chromiumcodereview.appspot.com/15745022/diff/16002/chrome/browser/ext...
chrome/browser/extensions/extension_disabled_ui.cc:338: void
AddExtensionDisabledErrorWithIcon(base::WeakPtr<ExtensionService> service,
Is Bind smart enough to handle WeakPtrs as arguments? As in, if this method
accepts a raw |ExtensionService*|, does it compile? If so, I believe Bind will
handle cancelling the callback when the WeakPtr dies. (It works for WeakPtrs as
|this| pointers.)

[sail, 2013-05-24 22:15:02]

Looks good overall!

https://codereview.chromium.org/15745022/diff/16002/chrome/browser/ui/global_...
File chrome/browser/ui/global_error/global_error.h (right):

https://codereview.chromium.org/15745022/diff/16002/chrome/browser/ui/global_...
chrome/browser/ui/global_error/global_error.h:59: virtual gfx::Image*
GetBubbleViewCustomIcon();
Return by value instead. Callers can use IsEmpty() to check for a invalid image.

https://codereview.chromium.org/15745022/diff/16002/chrome/browser/ui/global_...
chrome/browser/ui/global_error/global_error.h:61: int
GetBubbleViewIconResourceID();
this should be removed.

[Yoyo Zhou, 2013-05-28 18:21:33]

https://codereview.chromium.org/15745022/diff/16002/chrome/browser/ui/global_...
File chrome/browser/ui/global_error/global_error.h (right):

https://codereview.chromium.org/15745022/diff/16002/chrome/browser/ui/global_...
chrome/browser/ui/global_error/global_error.h:61: int
GetBubbleViewIconResourceID();
On 2013/05/24 22:15:02, sail wrote:
> this should be removed.

What do you think about making it a static GetDefaultBubbleViewIconResourceID
and using that if the custom icon IsEmpty?

[sail, 2013-05-28 18:39:50]

https://codereview.chromium.org/15745022/diff/16002/chrome/browser/ui/global_...
File chrome/browser/ui/global_error/global_error.h (right):

https://codereview.chromium.org/15745022/diff/16002/chrome/browser/ui/global_...
chrome/browser/ui/global_error/global_error.h:61: int
GetBubbleViewIconResourceID();
On 2013/05/28 18:21:34, Yoyo Zhou wrote:
> On 2013/05/24 22:15:02, sail wrote:
> > this should be removed.
> 
> What do you think about making it a static GetDefaultBubbleViewIconResourceID
> and using that if the custom icon IsEmpty?

I think it's best to just have a simple API. I don't think having to fallback to
to the resource ID is helpful.

[Yoyo Zhou, 2013-05-28 21:58:02]

https://codereview.chromium.org/15745022/diff/16002/chrome/browser/ui/global_...
File chrome/browser/ui/global_error/global_error.h (right):

https://codereview.chromium.org/15745022/diff/16002/chrome/browser/ui/global_...
chrome/browser/ui/global_error/global_error.h:59: virtual gfx::Image*
GetBubbleViewCustomIcon();
On 2013/05/24 22:15:02, sail wrote:
> Return by value instead. Callers can use IsEmpty() to check for a invalid
image.

Done.

https://codereview.chromium.org/15745022/diff/16002/chrome/browser/ui/global_...
chrome/browser/ui/global_error/global_error.h:61: int
GetBubbleViewIconResourceID();
On 2013/05/28 18:39:50, sail wrote:
> On 2013/05/28 18:21:34, Yoyo Zhou wrote:
> > On 2013/05/24 22:15:02, sail wrote:
> > > this should be removed.
> > 
> > What do you think about making it a static
GetDefaultBubbleViewIconResourceID
> > and using that if the custom icon IsEmpty?
> 
> I think it's best to just have a simple API. I don't think having to fallback
to
> to the resource ID is helpful.

OK. I changed the API to gfx::Image GetBubbleViewIcon().

[Yoyo Zhou, 2013-05-28 22:04:31]

+sky for OWNERS approval

[sail, 2013-05-28 22:41:22]

LGTM! Looks great.

https://codereview.chromium.org/15745022/diff/38001/chrome/browser/ui/global_...
File chrome/browser/ui/global_error/global_error.h (right):

https://codereview.chromium.org/15745022/diff/38001/chrome/browser/ui/global_...
chrome/browser/ui/global_error/global_error.h:58: virtual gfx::Image
GetBubbleViewIcon() = 0;
Any reason not have a default implementation return
GetNativeImageNamed(IDR_INPUT_ALERT)?

[Yoyo Zhou, 2013-05-28 22:41:42]

mpcomplete: some changes in extension_disabled_ui to show the default
app/extension icon for apps/extensions with no icon.

[Matt Perry, 2013-05-28 22:50:40]

lgtm

https://codereview.chromium.org/15745022/diff/40009/chrome/browser/extensions...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://codereview.chromium.org/15745022/diff/40009/chrome/browser/extensions...
chrome/browser/extensions/extension_disabled_ui.cc:49: static int kIconSize =
extension_misc::EXTENSION_ICON_SMALL;
nit: const int

[Yoyo Zhou, 2013-05-28 22:56:43]

https://codereview.chromium.org/15745022/diff/38001/chrome/browser/ui/global_...
File chrome/browser/ui/global_error/global_error.h (right):

https://codereview.chromium.org/15745022/diff/38001/chrome/browser/ui/global_...
chrome/browser/ui/global_error/global_error.h:58: virtual gfx::Image
GetBubbleViewIcon() = 0;
On 2013/05/28 22:41:22, sail wrote:
> Any reason not have a default implementation return
> GetNativeImageNamed(IDR_INPUT_ALERT)?

Nope, that's a much better plan. I was just having trouble understanding your
earlier comment.

https://codereview.chromium.org/15745022/diff/40009/chrome/browser/extensions...
File chrome/browser/extensions/extension_disabled_ui.cc (right):

https://codereview.chromium.org/15745022/diff/40009/chrome/browser/extensions...
chrome/browser/extensions/extension_disabled_ui.cc:49: static int kIconSize =
extension_misc::EXTENSION_ICON_SMALL;
On 2013/05/28 22:50:40, Matt Perry wrote:
> nit: const int

Oops, done.

[sky, 2013-05-29 00:23:29]

LGTM

[sail, 2013-05-29 00:32:52]

second LGTM!

[commit-bot: I haz the power, 2013-05-29 00:33:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yoz@chromium.org/15745022/58001

[commit-bot: I haz the power, 2013-05-29 07:20:22]

Change committed as 202795