Allow high-res bitmaps to be passed in from notifications API.

Allow high-res bitmaps to be passed in from notifications API.

Previously the custom binding in JS would prevent an image that's larger
than the size of the template from being sent to the browser process.
This relaxes the maximum to be the largest supported scale factor of the machine.

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

[dewittj, 2014-04-24 18:31:34]

ptal

[dewittj, 2014-04-25 21:48:25]

ping

[Dmitry Titov, 2014-04-29 01:55:28]

Cool, this will let developers to pass larger images w/o us scaling them in the
bindings code...

LGTM

[dewittj, 2014-04-29 16:28:17]

kalman: mind reviewing the custom bindings and owners changes?

[not at google - send to devlin, 2014-04-29 16:58:56]

is there a way to test this?

https://codereview.chromium.org/256503004/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/256503004/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/notifications/notifications_api.cc:262: //
TODO(dewittj): Return error if this fails.
yes please

https://codereview.chromium.org/256503004/diff/20001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/notifications_custom_bindings.js
(right):

https://codereview.chromium.org/256503004/diff/20001/chrome/renderer/resource...
chrome/renderer/resources/extensions/notifications_custom_bindings.js:29: // max
scale factor.
what is the "max scale factor"? without actually really understanding what this
code is doing, why does the renderer need to care at all if the browser is doing
validation?

[dewittj, 2014-04-29 17:01:30]

On 2014/04/29 16:58:56, kalman wrote:
> is there a way to test this?
Yes, there should be a way.

https://codereview.chromium.org/256503004/diff/20001/chrome/renderer/resource...
> chrome/renderer/resources/extensions/notifications_custom_bindings.js:29: //
max
> scale factor.
> what is the "max scale factor"? without actually really understanding what
this
> code is doing, why does the renderer need to care at all if the browser is
doing
> validation?

I'm mostly trying to keep things as sane as possible on both ends.  I guess it
seemed like I should minimize the amount of data going over IPC (and therefore
the amount of time the browser process has to spend manipulating the bitmaps
once they're across the boundary).  Is it not worth the effort?  Should I have a
max size of 5MB or something?

[not at google - send to devlin, 2014-04-29 17:12:51]

On 2014/04/29 17:01:30, dewittj wrote:
> On 2014/04/29 16:58:56, kalman wrote:
> > is there a way to test this?
> Yes, there should be a way.

Thanks.

> 
> I'm mostly trying to keep things as sane as possible on both ends.  I guess it
> seemed like I should minimize the amount of data going over IPC (and therefore
> the amount of time the browser process has to spend manipulating the bitmaps
> once they're across the boundary).  Is it not worth the effort?  Should I have
a
> max size of 5MB or something?

Ah ok, now I understand why you have that code. It makes sense. While there are
plenty of other ways for extensions to send large amounts of data over IPC
(store lots of stuff, send a large message, construct a really really long
string and pass as an ID to <basically any API>) the chance for a mistake seems
likely for notifications? Like they use some huge hi-res image without scaling
it themselves?

Are you automatically scaling?
Is the behaviour documented somewhere?

A better approach would be to actually tell the renderer its configuration via
an IPC or something so that it can do the scaling and the browser just
validates? Maybe... later, depends on how urgent this CL is.

[dewittj, 2014-04-29 17:18:19]

> Are you automatically scaling?
Yes, if the notification doesn't fit the size constraints it's resized when
drawing into the canvas.

> Is the behaviour documented somewhere?
No, not really.  It fits with the normal JS idiom of image tags; they might have
a size and the image is scaled to fit that size.  We really owe a full document
about notifications UI and API details, but jstark@ and I have been waiting for
a bit of stability on Android to flesh it out.  I think we'll do this soon-ish
(like this q?).

> A better approach would be to actually tell the renderer its configuration via
> an IPC or something so that it can do the scaling and the browser just
> validates? Maybe... later, depends on how urgent this CL is.

I would be really happy to have that.  Is there precedent?  There are several
constants in the custom binding that I'd love to remove the hard-coding for. 
Can i refer to constants within ui/message_center from within a
notifications_natives.cc?

[not at google - send to devlin, 2014-04-29 17:24:11]

> I would be really happy to have that.  Is there precedent?

There are a couple of things we send down, have a look at how the chrome channel
is populated:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

> There are several
> constants in the custom binding that I'd love to remove the hard-coding for. 
> Can i refer to constants within ui/message_center from within a
> notifications_natives.cc?

It looks like the renderer does refer to bits of ui/ but I don't know whether
that covers message_center. I'm not the right person to ask.

[dewittj, 2014-05-15 22:04:08]

ptal, let me know if this is the right way to go aboutt things.

[not at google - send to devlin, 2014-05-15 23:40:18]

nice. lgtm generally with a couple of comments, Dmitry should probably have
another look.

https://codereview.chromium.org/256503004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/256503004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/notifications/notifications_api.cc:251:
scoped_ptr<base::DictionaryValue> bitmapInfo =
bitmap_info

https://codereview.chromium.org/256503004/diff/80001/chrome/renderer/extensio...
File chrome/renderer/extensions/notifications_native_handler.h (right):

https://codereview.chromium.org/256503004/diff/80001/chrome/renderer/extensio...
chrome/renderer/extensions/notifications_native_handler.h:21:
DISALLOW_COPY_AND_ASSIGN(NotificationsNativeHandler);
put this in private:

https://codereview.chromium.org/256503004/diff/80001/chrome/renderer/extensio...
chrome/renderer/extensions/notifications_native_handler.h:24: void
GetNotificationImageSizes(
document arguments and return value

https://codereview.chromium.org/256503004/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/notifications_test_util.js (right):

https://codereview.chromium.org/256503004/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/extensions/notifications_test_util.js:5: //
NotificationsTestUtil contains stubs for the global classes and
hm we might be able to fix this at some point. it'd be really interesting to
properly support a js test framework - i had no idea gtestjs existed.

https://codereview.chromium.org/256503004/diff/80001/ui/message_center/messag...
File ui/message_center/message_center_style.cc (right):

https://codereview.chromium.org/256503004/diff/80001/ui/message_center/messag...
ui/message_center/message_center_style.cc:62:
dict->SetInteger("buttonIcon.height", kNotificationButtonIconSize);
why does this return as a dictionary? seems kind of awkward (particularly at the
call site) vs a struct.

or just not worry at all.

I can see why you did it I suppose but seems rather API specific?

[dewittj, 2014-05-16 20:05:20]

https://codereview.chromium.org/256503004/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/256503004/diff/20001/chrome/browser/extension...
chrome/browser/extensions/api/notifications/notifications_api.cc:262: //
TODO(dewittj): Return error if this fails.
On 2014/04/29 16:58:57, kalman wrote:
> yes please

Done.

https://codereview.chromium.org/256503004/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/256503004/diff/80001/chrome/browser/extension...
chrome/browser/extensions/api/notifications/notifications_api.cc:251:
scoped_ptr<base::DictionaryValue> bitmapInfo =
On 2014/05/15 23:40:18, kalman wrote:
> bitmap_info

Done.

https://codereview.chromium.org/256503004/diff/80001/chrome/renderer/extensio...
File chrome/renderer/extensions/notifications_native_handler.h (right):

https://codereview.chromium.org/256503004/diff/80001/chrome/renderer/extensio...
chrome/renderer/extensions/notifications_native_handler.h:21:
DISALLOW_COPY_AND_ASSIGN(NotificationsNativeHandler);
On 2014/05/15 23:40:18, kalman wrote:
> put this in private:

duh.  Thanks.

https://codereview.chromium.org/256503004/diff/80001/chrome/renderer/extensio...
chrome/renderer/extensions/notifications_native_handler.h:24: void
GetNotificationImageSizes(
On 2014/05/15 23:40:18, kalman wrote:
> document arguments and return value

Done.

https://codereview.chromium.org/256503004/diff/80001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/notifications_test_util.js (right):

https://codereview.chromium.org/256503004/diff/80001/chrome/renderer/resource...
chrome/renderer/resources/extensions/notifications_test_util.js:5: //
NotificationsTestUtil contains stubs for the global classes and
On 2014/05/15 23:40:18, kalman wrote:
> hm we might be able to fix this at some point. it'd be really interesting to
> properly support a js test framework - i had no idea gtestjs existed.

Yeah, a proper set of fake objects could be really useful if we decide to do
more tests in the custom bindings.  I just recently discovered it; previously
bindings code was tricky to test.  At the very least we could add an empty test
that just compiles all the custom bindings code :)

https://codereview.chromium.org/256503004/diff/80001/ui/message_center/messag...
File ui/message_center/message_center_style.cc (right):

https://codereview.chromium.org/256503004/diff/80001/ui/message_center/messag...
ui/message_center/message_center_style.cc:62:
dict->SetInteger("buttonIcon.height", kNotificationButtonIconSize);
On 2014/05/15 23:40:18, kalman wrote:
> why does this return as a dictionary? seems kind of awkward (particularly at
the
> call site) vs a struct.
> 
> or just not worry at all.
> 
> I can see why you did it I suppose but seems rather API specific?

Mostly because it's easy to convert to a v8::Object.  I refactored this a bit;
if you think that it can be done better without the v8_value_converter then let
me know; I don't really understand how to create/manipulate v8 objects (context?
isolate? scope? etc) so I might just be adding one too many layers in there.

[not at google - send to devlin, 2014-05-16 20:16:03]

still lgtm

https://codereview.chromium.org/256503004/diff/100001/chrome/renderer/extensi...
File chrome/renderer/extensions/notifications_native_handler.cc (right):

https://codereview.chromium.org/256503004/diff/100001/chrome/renderer/extensi...
chrome/renderer/extensions/notifications_native_handler.cc:44:
content::V8ValueConverter::create());
you'd use the v8 APIs like so:

the result is an object...
v8::Local<v8::Object> dict = v8::Object::New(isolate());  // maybe GetIsolate()?

then set each property...
dict->Set(v8::String::New(isolate(), "scaleFactor"),
          v8::Double::New(isolate(), scale_factor);
// uhoh need to create a dictionary now. I give up.


so that dictionary thing makes using a Value.. easier. However have a look at
value_builder (extensions/common/value_builder.h) as a neat way of doing this;
it doesn't do path expansion so you'd need to manually do that too.

[Dmitry Titov, 2014-05-16 22:21:41]

still lgtm, with a nit:

https://codereview.chromium.org/256503004/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/256503004/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/api/notifications/notifications_api.cc:45: "Unable to
successfully decode the input icon.  Please check your source "
This message may be misleading, it reads like there is an issue with the image
encoding... If we want a single message for all 'false' returns from this
function, then may be something like "Unable to use the image provided"?
Even better, rejecting otherwise nice image because of its size might deserve a
separate message, not necessarily now.

[dewittj, 2014-05-19 16:58:32]

On 2014/05/16 22:21:41, Dmitry Titov wrote:
> still lgtm, with a nit:
> 
>
https://codereview.chromium.org/256503004/diff/100001/chrome/browser/extensio...
> File chrome/browser/extensions/api/notifications/notifications_api.cc (right):
> 
>
https://codereview.chromium.org/256503004/diff/100001/chrome/browser/extensio...
> chrome/browser/extensions/api/notifications/notifications_api.cc:45: "Unable
to
> successfully decode the input icon.  Please check your source "
> This message may be misleading, it reads like there is an issue with the image
> encoding... If we want a single message for all 'false' returns from this
> function, then may be something like "Unable to use the image provided"?
> Even better, rejecting otherwise nice image because of its size might deserve
a
> separate message, not necessarily now.

FYI, we should never get a size rejection unless the renderer is compromised in
some way.

[dewittj, 2014-05-20 17:00:56]

I took this opportunity (failing tests) to do a bunch of cleanup.
* Call SetError for all failure cases in notifications so the user gets better
than "undefined error"
* Rewrote the api tests as an ExtensionTest so that we can properly validate the
notifications icon bitmap
* Wrote them in promises style which produces startlingly readable tests.

[dewittj, 2014-05-20 19:12:15]

The CQ bit was checked by dewittj@chromium.org

[dewittj, 2014-05-20 19:12:32]

On 2014/05/20 19:12:15, dewittj wrote:
> The CQ bit was checked by mailto:dewittj@chromium.org

I hit CQ please ping me if you are writing comments.

[commit-bot: I haz the power, 2014-05-20 19:12:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dewittj@chromium.org/256503004/150001

[commit-bot: I haz the power, 2014-05-21 00:09:21]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-21 01:05:16]

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

[commit-bot: I haz the power, 2014-05-21 01:05:16]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/7183)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/10419)

[dewittj, 2014-05-21 21:51:57]

fyi: I had to move the new GetNotificationBitmapSizes function into
chrome/common/extensions/api/notifications/ so that chrome_child.dll could get
the symbol without depending on ui/message_center.  Adding to CQ but if that is
the wrong place lmk.

[dewittj, 2014-05-22 13:49:51]

The CQ bit was checked by dewittj@chromium.org

[commit-bot: I haz the power, 2014-05-22 13:50:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dewittj@chromium.org/256503004/170001

[commit-bot: I haz the power, 2014-05-22 17:14:21]

Change committed as 272211