Remove ShowDesktopNotificationHostMsgParams in favor of PlatformNotificationData.

Remove ShowDesktopNotificationHostMsgParams in favor of PlatformNotificationData.

This patch removes all uses of ShowDesktopNotificationHostMsgParams and replaces
it with a more directed PlatformNotificationData. The PlatformNotificationData
structure has the same content as the WebNotificationData structure in the Blink
API, and included are canonical conversion functions between the two.

The reason for this change is two-folded:
  (1) The content/ layer now has the ability to initialize Web Notification
      objects, whereas this previously could only be done by Blink. This means
      that we have to carry all associated data around.

      The primary user for this are Persistent Notifications, which can outlive
      the page they were created by (and thereby the JavaScript objects).

  (2) The Web Notification specification is being updated with new features, a
      number of which will eventually have to be known to the browser process.
      This makes future plumbing significantly clearer.

Also, the *Desktop*Notification.. name is outdated.

BUG=432527
Committed: https://crrev.com/7329c2c1df2c08bf1dd7600731cc37a572181459
Cr-Commit-Position: refs/heads/master@{#308080}

[Peter Beverloo, 2014-12-10 17:24:50]

peter@chromium.org changed reviewers:
+ mvanouwerkerk@chromium.org

[Peter Beverloo, 2014-12-10 17:24:51]

+mvanouwerkerk to judge my sanity.

[Michael van Ouwerkerk, 2014-12-10 19:14:07]

lgtm

Holy plumbing batman!

https://codereview.chromium.org/794633002/diff/1/content/public/common/platfo...
File content/public/common/platform_notification_data.h (right):

https://codereview.chromium.org/794633002/diff/1/content/public/common/platfo...
content/public/common/platform_notification_data.h:5: #ifndef
CONTENT_PUBLIC_COMMON_PERSISTENT_NOTIFICATION_DATA_H_
Please match the file name.

[Peter Beverloo, 2014-12-10 19:44:38]

peter@chromium.org changed reviewers:
+ dewittj@chromium.org, mkwst@chromium.org

[Peter Beverloo, 2014-12-10 19:44:39]

+dewittj for notifications
+mkwst since I promised to do this

Non-plumbing bits are in
content/child/notifications/notification_data_conversions*.

https://codereview.chromium.org/794633002/diff/1/content/public/common/platfo...
File content/public/common/platform_notification_data.h (right):

https://codereview.chromium.org/794633002/diff/1/content/public/common/platfo...
content/public/common/platform_notification_data.h:5: #ifndef
CONTENT_PUBLIC_COMMON_PERSISTENT_NOTIFICATION_DATA_H_
On 2014/12/10 19:14:07, Michael van Ouwerkerk wrote:
> Please match the file name.

Done.

[Peter Beverloo, 2014-12-10 19:46:27]

peter@chromium.org changed reviewers:
+ avi@chromium.org

[Peter Beverloo, 2014-12-10 19:46:28]

+avi for content API;

show_desktop_notification_params.h for platform_notification_data.h swap-out.

[Avi (use Gerrit), 2014-12-10 19:56:39]

content/public LGTM

[dewittj, 2014-12-10 21:57:49]

https://codereview.chromium.org/794633002/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.h (right):

https://codereview.chromium.org/794633002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.h:84: const SkBitmap&
icon,
Is it possible to use gfx::Image in this API so that only the consumers need to
change when we support multiple scale factors?

[Peter Beverloo, 2014-12-10 22:06:37]

https://codereview.chromium.org/794633002/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.h (right):

https://codereview.chromium.org/794633002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.h:84: const SkBitmap&
icon,
On 2014/12/10 21:57:49, dewittj wrote:
> Is it possible to use gfx::Image in this API so that only the consumers need
to
> change when we support multiple scale factors?

Firstly, this call is going away entirely after
https://codereview.chromium.org/787193002/, but you're talking in general.

There are two parts to this:

  (1) The current conversion from SkBitmap to gfx::Image, which only supports 1x
scale. PlatformNotificationServiceImpl::CreateNotificationFromData will be the
only place where this conversion gets done, regardless of the code path. We
cannot pass gfx::Images over IPC.

  (2) Allowing developers to specify multiple icons, depending on resolution.
We'll make the selection in the NotificationImageLoader and only pass through a
single SkBitmap to the browser process, whilst storing all available images in
the PlatformNotificationData.

Does this answer your question?

[dewittj, 2014-12-10 22:13:11]

On 2014/12/10 22:06:37, Peter Beverloo wrote:
>
https://codereview.chromium.org/794633002/diff/20001/chrome/browser/notificat...
> File chrome/browser/notifications/desktop_notification_service.h (right):
> 
>
https://codereview.chromium.org/794633002/diff/20001/chrome/browser/notificat...
> chrome/browser/notifications/desktop_notification_service.h:84: const
SkBitmap&
> icon,
> On 2014/12/10 21:57:49, dewittj wrote:
> > Is it possible to use gfx::Image in this API so that only the consumers need
> to
> > change when we support multiple scale factors?
> 
> Firstly, this call is going away entirely after
> https://codereview.chromium.org/787193002/, but you're talking in general.
> 
> There are two parts to this:
> 
>   (1) The current conversion from SkBitmap to gfx::Image, which only supports
1x
> scale. PlatformNotificationServiceImpl::CreateNotificationFromData will be the
> only place where this conversion gets done, regardless of the code path. We
> cannot pass gfx::Images over IPC.
> 
>   (2) Allowing developers to specify multiple icons, depending on resolution.
> We'll make the selection in the NotificationImageLoader and only pass through
a
> single SkBitmap to the browser process, whilst storing all available images in
> the PlatformNotificationData.
> 
> Does this answer your question?

The issue with (2) is that scale factor can change dynamically on some platforms
(like ChromeOS) so having bitmaps for all scale factors preloaded in the browser
process is a nice feature; ctrl-shift-+ would then behave nicely without any
lag.

[Peter Beverloo, 2014-12-10 22:38:52]

On 2014/12/10 22:13:11, dewittj wrote:
> On 2014/12/10 22:06:37, Peter Beverloo wrote:
> >
>
https://codereview.chromium.org/794633002/diff/20001/chrome/browser/notificat...
> > File chrome/browser/notifications/desktop_notification_service.h (right):
> > 
> >
>
https://codereview.chromium.org/794633002/diff/20001/chrome/browser/notificat...
> > chrome/browser/notifications/desktop_notification_service.h:84: const
> SkBitmap&
> > icon,
> > On 2014/12/10 21:57:49, dewittj wrote:
> > > Is it possible to use gfx::Image in this API so that only the consumers
need
> > to
> > > change when we support multiple scale factors?
> > 
> > Firstly, this call is going away entirely after
> > https://codereview.chromium.org/787193002/, but you're talking in general.
> > 
> > There are two parts to this:
> > 
> >   (1) The current conversion from SkBitmap to gfx::Image, which only
supports
> 1x
> > scale. PlatformNotificationServiceImpl::CreateNotificationFromData will be
the
> > only place where this conversion gets done, regardless of the code path. We
> > cannot pass gfx::Images over IPC.
> > 
> >   (2) Allowing developers to specify multiple icons, depending on
resolution.
> > We'll make the selection in the NotificationImageLoader and only pass
through
> a
> > single SkBitmap to the browser process, whilst storing all available images
in
> > the PlatformNotificationData.
> > 
> > Does this answer your question?
> 
> The issue with (2) is that scale factor can change dynamically on some
platforms
> (like ChromeOS) so having bitmaps for all scale factors preloaded in the
browser
> process is a nice feature; ctrl-shift-+ would then behave nicely without any
> lag.

I think that's part of a larger discussion, orthogonal to this patch.

  1) Loading all images may be too expensive in bandwidth constrained
environments. As an example, we ship all resources in five formats on Android
(MDPI, HDPI, XHDPI, XXHDPI, XXXHDPI) - that's a huge binary size cost. Even if
developers only specify 3 of those (1x - MDPI, 2x - XHDPI, 3x - XXXHDPI), the
bandwidth costs could be significant.

  2) It may not just be about resolution, but also intended display size of the
image, or perhaps the shape (think watches).

  3) We don't actually have any Web-exposed APIs for this yet.

[dewittj, 2014-12-10 22:47:52]

ok, needs more design/discussion elsewhere :)

lgtm

[Peter Beverloo, 2014-12-11 19:02:44]

peter@chromium.org changed reviewers:
+ tsepez@chromium.org

[Peter Beverloo, 2014-12-11 19:02:45]

+Tom, mind taking a look to off-load the sick mkwst?

[Tom Sepez, 2014-12-11 22:12:23]

Messages LGTM with optional nit.

https://codereview.chromium.org/794633002/diff/60001/content/public/common/pl...
File content/public/common/platform_notification_data.h (right):

https://codereview.chromium.org/794633002/diff/60001/content/public/common/pl...
content/public/common/platform_notification_data.h:26: };
nit: prefer the pattern where

enum NotificationDirection {
    NotificationDirectionLeftToRight,
    NotificationDirectionRightToLeft
    NotificationDirectionLast = NotificationDirectionRightToLeft
}

and NotificationDirectionLast is used in the IPC declarations.

[Peter Beverloo, 2014-12-12 12:05:56]

https://codereview.chromium.org/794633002/diff/60001/content/public/common/pl...
File content/public/common/platform_notification_data.h (right):

https://codereview.chromium.org/794633002/diff/60001/content/public/common/pl...
content/public/common/platform_notification_data.h:26: };
On 2014/12/11 22:12:23, Tom Sepez wrote:
> nit: prefer the pattern where
> 
> enum NotificationDirection {
>     NotificationDirectionLeftToRight,
>     NotificationDirectionRightToLeft
>     NotificationDirectionLast = NotificationDirectionRightToLeft
> }
> 
> and NotificationDirectionLast is used in the IPC declarations.

Done.

[Peter Beverloo, 2014-12-12 12:06:01]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2014-12-12 12:06:49]

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

[commit-bot: I haz the power, 2014-12-12 14:43:08]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-12-12 14:44:17]

Patchset 5 (id:??) landed as
https://crrev.com/7329c2c1df2c08bf1dd7600731cc37a572181459
Cr-Commit-Position: refs/heads/master@{#308080}

[ciqiguopimin, 2015-01-07 09:01:58]

On 2014/12/10 19:46:28, Peter Beverloo wrote:
> +avi for content API;
> 
> show_desktop_notification_params.h for platform_notification_data.h swap-out.

really? do you mean the file content_browser_client.h

 // Show a desktop notification. If |cancel_callback| is non-null, it's set to
  // a callback which can be used to cancel the notification.
  virtual void ShowDesktopNotification(
      const ShowDesktopNotificationHostMsgParams& params,
      BrowserContext* browser_context,
      int render_process_id,
      scoped_ptr<DesktopNotificationDelegate> delegate,
      base::Closure* cancel_callback) {}