Add NotificationOptions

Add NotificationOptions

Use IDL dictionary (NotificationOptions) instead of generic
Dictionary. Tests under fast/notifications are passed.

BUG=403150
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181879

[bashi, 2014-09-10 14:23:24]

bashi@chromium.org changed reviewers:
+ haraken@chromium.org, peter@chromium.org

[bashi, 2014-09-10 14:23:24]

Peter, Haraken-san,

Could you please review this?
This CL depends on https://codereview.chromium.org/547383004/ to build(GYP/GN
stuff), but ready for review.

Here is a document which describes how IDL dictionaries work in Blink:
https://docs.google.com/document/d/1mRB5zbfHd0lX2Y_Hr7g6grzP_L4Xc6_gBRjL-AE7s...

[Peter Beverloo, 2014-09-10 14:30:34]

This is great, thanks!!

https://codereview.chromium.org/556193002/diff/1/Source/modules/notifications...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/556193002/diff/1/Source/modules/notifications...
Source/modules/notifications/Notification.cpp:51: if (options->hasBody())
The specification defines "tag" and "body" as defaulting to empty strings as
well, can we avoid having these branches?

https://codereview.chromium.org/556193002/diff/1/Source/modules/notifications...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/556193002/diff/1/Source/modules/notifications...
Source/modules/notifications/Notification.h:46: class NotificationOptions;
Alphabetical order please.

https://codereview.chromium.org/556193002/diff/1/Source/modules/notifications...
File Source/modules/notifications/Notification.idl (right):

https://codereview.chromium.org/556193002/diff/1/Source/modules/notifications...
Source/modules/notifications/Notification.idl:35: Constructor(DOMString title,
[Default=Undefined] optional NotificationOptions options),
What does [Default=Undefined] mean in this case? We pass a NotificationOptions*
to Notification::create(), does that mean that the pointer value can be NULL? We
don't have any NULL checks there.

https://codereview.chromium.org/556193002/diff/1/Source/modules/notifications...
File Source/modules/notifications/NotificationOptions.idl (right):

https://codereview.chromium.org/556193002/diff/1/Source/modules/notifications...
Source/modules/notifications/NotificationOptions.idl:5: //
http://www.w3.org/TR/notifications/#notification
nit: We follow the object in the WHATWG spec:
  http://notifications.spec.whatwg.org/#api

[bashi, 2014-09-10 15:02:01]

Thank you for the quick review!

https://codereview.chromium.org/556193002/diff/1/Source/modules/notifications...
File Source/modules/notifications/Notification.cpp (right):

https://codereview.chromium.org/556193002/diff/1/Source/modules/notifications...
Source/modules/notifications/Notification.cpp:51: if (options->hasBody())
On 2014/09/10 14:30:34, Peter Beverloo wrote:
> The specification defines "tag" and "body" as defaulting to empty strings as
> well, can we avoid having these branches?

Done. I was looking at http://www.w3.org/TR/notifications/#notification, which
doesn't say "tag" and "body" have default values. I should have referred to
http://notifications.spec.whatwg.org/#api, which is what we are implementing.

https://codereview.chromium.org/556193002/diff/1/Source/modules/notifications...
File Source/modules/notifications/Notification.h (right):

https://codereview.chromium.org/556193002/diff/1/Source/modules/notifications...
Source/modules/notifications/Notification.h:46: class NotificationOptions;
On 2014/09/10 14:30:34, Peter Beverloo wrote:
> Alphabetical order please.

Done.

https://codereview.chromium.org/556193002/diff/1/Source/modules/notifications...
File Source/modules/notifications/Notification.idl (right):

https://codereview.chromium.org/556193002/diff/1/Source/modules/notifications...
Source/modules/notifications/Notification.idl:35: Constructor(DOMString title,
[Default=Undefined] optional NotificationOptions options),
On 2014/09/10 14:30:34, Peter Beverloo wrote:
> What does [Default=Undefined] mean in this case? We pass a
NotificationOptions*
> to Notification::create(), does that mean that the pointer value can be NULL?
We
> don't have any NULL checks there.

Good catch! [Default=Undefined] should have been removed. IDL dictionaries never
become undefined. Removed.

https://codereview.chromium.org/556193002/diff/1/Source/modules/notifications...
File Source/modules/notifications/NotificationOptions.idl (right):

https://codereview.chromium.org/556193002/diff/1/Source/modules/notifications...
Source/modules/notifications/NotificationOptions.idl:5: //
http://www.w3.org/TR/notifications/#notification
On 2014/09/10 14:30:34, Peter Beverloo wrote:
> nit: We follow the object in the WHATWG spec:
>   http://notifications.spec.whatwg.org/#api

Ah, I was looking at a different spec.. Changed.

[Peter Beverloo, 2014-09-10 15:07:17]

lgtm

[haraken, 2014-09-10 15:24:57]

LGTM

[bashi, 2014-09-12 04:25:51]

Rebased (Pointers -> References)

[bashi, 2014-09-12 04:38:01]

The CQ bit was checked by bashi@chromium.org

[commit-bot: I haz the power, 2014-09-12 04:38:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/556193002/60001

[commit-bot: I haz the power, 2014-09-12 05:29:31]

Committed patchset #4 (id:60001) as 181879