[Extensions UI] Clean up extension icon generation

[Extensions UI] Clean up extension icon generation

Clean up extension icon generation so that a single object
handles drawing the icon and the badge, and we don't need
the extra methods like PaintExtra(). In addition to being a
nice cleanup, this is all but necessary for the next steps
in the "want to act" treatment.  And as a bonus, it should
also fix bugs stemming from having different icons on the
toolbar and in the (chevron) overflow menu.

BUG=417441
TBR=sky@chromium.org (extension actions reviewed by Finnur)

Committed: https://crrev.com/3a57a7ffd210addc2dc13567366208968a9c0292
Cr-Commit-Position: refs/heads/master@{#337476}

[Devlin, 2015-07-01 23:14:04]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-07-01 23:14:10]

Patchset #1 (id:20001) has been deleted

[Devlin, 2015-07-01 23:14:16]

Patchset #1 (id:40001) has been deleted

[Devlin, 2015-07-01 23:14:22]

Patchset #1 (id:60001) has been deleted

[Devlin, 2015-07-01 23:16:45]

rdevlin.cronin@chromium.org changed reviewers:
+ finnur@chromium.org

[Devlin, 2015-07-01 23:16:56]

rdevlin.cronin@chromium.org changed reviewers:
+ avi@chromium.org

[Devlin, 2015-07-01 23:17:45]

Finnur, please take a look at everything but the cocoa.  Avi, cocoa's all yours.
:)

Thursday and Friday of this week are holidays in the US, so no rush on this.

[Avi (use Gerrit), 2015-07-02 00:33:23]

lgtm

nits but nothing horrible.

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:48: //
by an empty skia.
"have a blank skia"? "empty skia"?

s/skia/canvas/ perhaps?

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:406:
gfx::Size(NSWidth(frame), NSHeight(frame)));
CGSizeFromNSSize(frame.size) should do it.

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:102: NSSize
ns_size = NSMakeSize(size.width(), size.height());
Hmm. NSSizeFromCGSize(size.ToCGSize()), though that might force a linewrap.

[Finnur, 2015-07-06 10:27:09]

LGTM, seasoned with a nit and a question.

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:74:
gfx::Size size(29, 29);  // Size of browser actions buttons.
nit: const-ify.

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right):

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_action_view.cc:40: const int
kBorderInset = 0;
nit: Is the inset not 0 by default?

[Devlin, 2015-07-06 19:16:07]

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:48: //
by an empty skia.
On 2015/07/02 00:33:23, Avi wrote:
> "have a blank skia"? "empty skia"?
> 
> s/skia/canvas/ perhaps?

For "blank", yeah, blank canvas is better.  For "empty", I think skia is right,
but I'll change to "null", since that matches the isNull terminology in the skia
api.  (This comment is just trying to explain why it's not enough to just
super-impose it with an ImageSkia()).

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:74:
gfx::Size size(29, 29);  // Size of browser actions buttons.
On 2015/07/06 10:27:09, Finnur wrote:
> nit: const-ify.

Done.

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/extensions/browser_action_button.mm (right):

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/extensions/browser_action_button.mm:406:
gfx::Size(NSWidth(frame), NSHeight(frame)));
On 2015/07/02 00:33:23, Avi wrote:
> CGSizeFromNSSize(frame.size) should do it.

Dang, I was looking for that method, but couldn't remember the magical
combination of terms.  And actually, it looks like CGSizeFromNSSize doesn't
really exist either... but apparently, NSRect.size returns CGSize, so we can
just use that.  Ain't Cocoa fun. ;)

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm (right):

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/page_action_decoration.mm:102: NSSize
ns_size = NSMakeSize(size.width(), size.height());
On 2015/07/02 00:33:23, Avi wrote:
> Hmm. NSSizeFromCGSize(size.ToCGSize()), though that might force a linewrap.

Done.

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right):

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_action_view.cc:40: const int
kBorderInset = 0;
On 2015/07/06 10:27:09, Finnur wrote:
> nit: Is the inset not 0 by default?

Not for a LabelButtonBorder, no - it's all handled in
LabelButtonBorder::GetDefaultInsetsForStyle().

[Devlin, 2015-07-06 19:16:11]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-07-06 19:16:12]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1214243003/#ps90001
(title: "Finnur's and Avi's")

[commit-bot: I haz the power, 2015-07-06 19:16:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1214243003/90001

[commit-bot: I haz the power, 2015-07-06 19:28:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-06 19:28:51]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Avi (use Gerrit), 2015-07-06 19:33:30]

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:48: //
by an empty skia.
On 2015/07/06 19:16:07, Devlin wrote:
> On 2015/07/02 00:33:23, Avi wrote:
> > "have a blank skia"? "empty skia"?
> > 
> > s/skia/canvas/ perhaps?
> 
> For "blank", yeah, blank canvas is better.  For "empty", I think skia is
right,
> but I'll change to "null", since that matches the isNull terminology in the
skia
> api.  (This comment is just trying to explain why it's not enough to just
> super-impose it with an ImageSkia()).

What is a "skia"? That's my question. ImageSkia is a Skia (the graphics
library)-backed Image.

But your use of the word "skia" as a noun is weird, because it's clearly not a
reference to the graphics library, but I can't figure out what it _is_ a
reference to.

[Devlin, 2015-07-06 19:34:21]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-07-06 19:34:31]

The CQ bit was unchecked by rdevlin.cronin@chromium.org

[Devlin, 2015-07-06 19:35:52]

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/1214243003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:48: //
by an empty skia.
On 2015/07/06 19:33:30, Avi wrote:
> On 2015/07/06 19:16:07, Devlin wrote:
> > On 2015/07/02 00:33:23, Avi wrote:
> > > "have a blank skia"? "empty skia"?
> > > 
> > > s/skia/canvas/ perhaps?
> > 
> > For "blank", yeah, blank canvas is better.  For "empty", I think skia is
> right,
> > but I'll change to "null", since that matches the isNull terminology in the
> skia
> > api.  (This comment is just trying to explain why it's not enough to just
> > super-impose it with an ImageSkia()).
> 
> What is a "skia"? That's my question. ImageSkia is a Skia (the graphics
> library)-backed Image.
> 
> But your use of the word "skia" as a noun is weird, because it's clearly not a
> reference to the graphics library, but I can't figure out what it _is_ a
> reference to.

That's fair.  s/skia/ImageSkia.

[Devlin, 2015-07-06 20:13:57]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-07-06 20:13:57]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org,
avi@chromium.org
Link to the patchset: https://codereview.chromium.org/1214243003/#ps110001
(title: "s/skia/ImageSkia")

[commit-bot: I haz the power, 2015-07-06 20:14:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1214243003/110001

[commit-bot: I haz the power, 2015-07-06 21:22:53]

Committed patchset #3 (id:110001)

[commit-bot: I haz the power, 2015-07-06 21:24:44]

Patchset 3 (id:??) landed as
https://crrev.com/3a57a7ffd210addc2dc13567366208968a9c0292
Cr-Commit-Position: refs/heads/master@{#337476}

[Avi (use Gerrit), 2015-07-06 23:20:06]

https://codereview.chromium.org/1214243003/diff/110001/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/1214243003/diff/110001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:46: //
An ImageSkia source that will do nothing (i.e., have a blank skia). We need
... have a blank what?

You could totally just omit the parenthetical comment.