Set Extension Infobars to have a fixed height determined by the developer (clamped to sane min an...

Set Extension Infobars to have a fixed height determined by the developer (clamped to sane min and max values).

BUG=92656
TEST=This needs to be tested with an Extension, which I can provide.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97132

[asargent_no_longer_on_chrome, 2011-08-15 17:23:22]

http://codereview.chromium.org/7635019/diff/8001/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7635019/diff/8001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:2168: "description": "The height
(in pixels) of the infobar to show. A value of 0 means: use default infobar
height.",
I don't understand why we want to make the property required if 0 means "use the
default." Wouldn't it make more sense to have the property be optional, and if
it isn't specified that means use the default?

As for the minimum if they do specify a height, I guess we could allow anything
down to 0 in the schema here but include in the note that we won't actually use
anything smaller than 16 or 24 or whatever the icon size is (I guess it's
platform depenendent?).

[Finnur, 2011-08-15 19:05:24]

Please take another look.

http://codereview.chromium.org/7635019/diff/8001/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7635019/diff/8001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:2168: "description": "The height
(in pixels) of the infobar to show. A value of 0 means: use default infobar
height.",
Yeah, it is platform dependent. But making it optional is the way to go, though.

On 2011/08/15 17:23:22, Antony Sargent wrote:
> I don't understand why we want to make the property required if 0 means "use
the
> default." Wouldn't it make more sense to have the property be optional, and if
> it isn't specified that means use the default?
> 
> As for the minimum if they do specify a height, I guess we could allow
anything
> down to 0 in the schema here but include in the note that we won't actually
use
> anything smaller than 16 or 24 or whatever the icon size is (I guess it's
> platform depenendent?).
>

[asargent_no_longer_on_chrome, 2011-08-15 19:52:33]

LGTM. 

Note that you might have to get some views/gtk OWNERS to review as well.

[Finnur, 2011-08-16 14:30:03]

Looks like I need three owners since the three areas don't have overlapping
owners...

avi, can you cover browser\tab_contents?
estade, can you cover browser\ui\gtk?
and
sky, can you cover browser\ui\views?

Fortunately, it is code deletion and not a lot, so it should be relative simple.
:)

[Avi (use Gerrit), 2011-08-16 14:32:38]

tab contents LGTM

[Evan Stade, 2011-08-16 18:35:03]

gtk lgtm

http://codereview.chromium.org/7635019/diff/15001/chrome/browser/ui/gtk/infob...
File chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc (right):

http://codereview.chromium.org/7635019/diff/15001/chrome/browser/ui/gtk/infob...
chrome/browser/ui/gtk/infobars/extension_infobar_gtk.cc:59: OnImageLoaded(NULL,
icon_resource, 0);  // |image|, ..., |index|.
this comment doesn't really help at all, I'd just delete it

[sky, 2011-08-17 14:00:45]

LGTM