Making icon url for buttons in notification optional.

Making icon url for buttons in notification optional.

The icon url's for buttons are optional. So notification will have the
buttons without button icon also.

BUG=501214
Committed: https://crrev.com/e3599a679d6a9ba80fecc1fa371a8c5f51803ed0
Cr-Commit-Position: refs/heads/master@{#335616}

[Deepak, 2015-06-17 12:09:44]

deepak.m1@samsung.com changed reviewers:
+ dewittj@chromium.org

[Deepak, 2015-06-17 12:09:44]

As icon url for the buttons in notification is optional, so adding button info
in vector should not based on button's icon url. Buttons should come when icon
url is not specified.

This is because in https://codereview.chromium.org/1135213004 
addition of button become dependent on icon url of button.
My apologies, I have done changes for fix this.

Please review.

[Deepak, 2015-06-17 12:12:04]

deepak.m1@samsung.com changed reviewers:
+ ajith.v@chromium.org

[Deepak, 2015-06-18 12:46:18]

As icon url for the buttons in notification is optional, so adding button info
in vector should not based on button's icon url. Buttons should come when icon
url is not specified.

This is because in https://codereview.chromium.org/1135213004 
addition of button become dependent on icon url of button.
My apologies, I have done changes for fix this.

Please review.

[Deepak, 2015-06-18 14:51:10]

deepak.m1@samsung.com changed reviewers:
+ peter@chromium.org

[Deepak, 2015-06-19 13:04:37]

@peter
Please review this small correction change.

[Deepak, 2015-06-22 11:52:05]

@peter
Please review this small correction change.

[Peter Beverloo, 2015-06-22 13:00:12]

This is in line with the documentation:
  https://developer.chrome.com/extensions/notifications#type-NotificationOptions

So that looks fine to me. Thank you!!

Would you consider adding a test?

https://codereview.chromium.org/1188213002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/notifications/notifications_api.cc (right):

https://codereview.chromium.org/1188213002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/notifications/notifications_api.cc:297: // As icon
url for button is optional so icon_bitmap_ptr can be null.
nit: grammer. I personally wouldn't bother with the comment at all.

[Deepak, 2015-06-22 16:52:28]

@peter
Thanks for review.
Test case added.
PTAL.

[Peter Beverloo, 2015-06-22 16:58:14]

+dewittj for extension API

Is there really no more appropriate test to put this under?

[dewittj, 2015-06-22 18:40:39]

lgtm

peter, what do you mean by a better test?

[Peter Beverloo, 2015-06-22 18:53:01]

On 2015/06/22 18:40:39, dewittj wrote:
> lgtm
> 
> peter, what do you mean by a better test?

Whether buttons require icons or not has little to do with partial updates.
Nonetheless, it is coverage :-).

[Deepak, 2015-06-23 02:49:08]

On 2015/06/22 18:53:01, Peter Beverloo wrote:
> On 2015/06/22 18:40:39, dewittj wrote:
> > lgtm
> > 
> > peter, what do you mean by a better test?
> 
> Whether buttons require icons or not has little to do with partial updates.
> Nonetheless, it is coverage :-).

@dewittj and @peter
Thanks for review.
Modified test case is just to provide coverage :)

[Deepak, 2015-06-23 02:49:13]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2015-06-23 02:49:40]

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

[commit-bot: I haz the power, 2015-06-23 02:55:59]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-06-23 02:58:01]

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