[NOT FOR REVIEW] Significantly simplify the PlatformNotificationService //content API.

Significantly simplify the PlatformNotificationService //content API.

BUG=

[Peter Beverloo, 2015-04-29 14:50:09]

peter@chromium.org changed reviewers:
+ avi@chromium.org, dewittj@chromium.org, johnme@chromium.org,
mvanouwerkerk@chromium.org, ohrn@opera.com

[Peter Beverloo, 2015-04-29 14:50:09]

Hey all -

**This is not for review**. I have some specific questions about the taken
approach before I work this out properly (notably in //chrome) and start landing
it as a series of patches.

In a nutshell - this allows us to move much more notification logic down to
//content and significantly simplify the PlatformNotificationService
implementation (see LayoutTestNotificationManager). It will also fix standing
bugs, such as clean notification replacement on desktop, by having deterministic
ID generation.

- Avi: Are the DEPS additions OK from a //content point of view? We now only
expose exactly what the //content embedder needs.

- Justin: This will allow us to get rid of NotificationObjectProxy, random IDs,
cancel closures, much of the PlatformNotificationServiceImpl pipeline and in the
longer term NotificationDelegate. Do you have any high-level opinions?

- Fredrik, John, Michael: FYI

Thanks :-)

[Avi (use Gerrit), 2015-04-29 15:24:39]

On 2015/04/29 14:50:09, Peter Beverloo wrote:
> - Avi: Are the DEPS additions OK from a //content point of view? We now only
> expose exactly what the //content embedder needs.

I'm looking at those two files, and I'm not comfortable there. The other ui/
files in the DEPS are for low-level basics (doing ui string lookup, etc) but the
message_center files are complicated UI things with tons of implementation
detail.

What exactly do we need from those files? Can we pull the basics of what we'd
need from a spec standpoint into content, and rebuild the ui/ data structures
atop them?

[Peter Beverloo, 2015-04-29 15:50:00]

On 2015/04/29 15:24:39, Avi wrote:
> On 2015/04/29 14:50:09, Peter Beverloo wrote:
> > - Avi: Are the DEPS additions OK from a //content point of view? We now only
> > expose exactly what the //content embedder needs.
> 
> I'm looking at those two files, and I'm not comfortable there. The other ui/
> files in the DEPS are for low-level basics (doing ui string lookup, etc) but
the
> message_center files are complicated UI things with tons of implementation
> detail.

Right, that's fair.

> What exactly do we need from those files? Can we pull the basics of what we'd
> need from a spec standpoint into content, and rebuild the ui/ data structures
> atop them?

This is where my mental model of layering seems to be off. What is the
dependency relationship between //ui and //content?

The message center is used for several non-Web Notification purposes, so having
that depend on //content does not feel right. It's a very self-contained
component right now. (But I may very well be wrong here!)

An alternative approach would be to have two new classes (~ naming):
  (1) NotificationPresentationData, which can be cleanly converted to a
mc::Notification by //chrome.
  (2) NotificationPresentationDelegate, which defines the methods of a
mc::NotificationDelegate used by //content. (Using a proxy.)


Additionally, we can probably drop PlatformNotificationData from the public
//content API after this change and reserve that for IPC and storage internal to
//content.

Does that sound better?

[Avi (use Gerrit), 2015-04-29 16:07:37]

On 2015/04/29 15:50:00, Peter Beverloo wrote:
> This is where my mental model of layering seems to be off. What is the
> dependency relationship between //ui and //content?

It's complicated. (That means I don't fully understand it myself.)

From what I understand, content grabs core graphics stuff from ui, like ui/gfx,
ui/aura, ui/base, etc, but that's about it. 

>   (1) NotificationPresentationData, which can be cleanly converted to a
> mc::Notification by //chrome.

Which would be the basic data specified by the spec?

>   (2) NotificationPresentationDelegate, which defines the methods of a
> mc::NotificationDelegate used by //content. (Using a proxy.)

The idea here is that the content layer has to have to have a way to say
"present this thing", and the embedder implements that way?

That sounds like a reasonable approach.

[Peter Beverloo, 2015-04-29 16:15:13]

On 2015/04/29 16:07:37, Avi wrote:
> On 2015/04/29 15:50:00, Peter Beverloo wrote:
> > This is where my mental model of layering seems to be off. What is the
> > dependency relationship between //ui and //content?
> 
> It's complicated. (That means I don't fully understand it myself.)
> 
> From what I understand, content grabs core graphics stuff from ui, like
ui/gfx,
> ui/aura, ui/base, etc, but that's about it. 
> 
> >   (1) NotificationPresentationData, which can be cleanly converted to a
> > mc::Notification by //chrome.
> 
> Which would be the basic data specified by the spec?

A sub-set of that - strictly the data necessary to display the notification. To
illustrate:

  * gfx::Image for the icon, but no image URL. (Similar with other resources
when we support that, e.g. sounds.)
  * No "data" (author-provided data), tags, languages / direction hints which we
don't use.

> >   (2) NotificationPresentationDelegate, which defines the methods of a
> > mc::NotificationDelegate used by //content. (Using a proxy.)
> 
> The idea here is that the content layer has to have to have a way to say
> "present this thing", and the embedder implements that way?
> 
> That sounds like a reasonable approach.

Yes. "Present this thing", and "tell me when something happened to it."

[Avi (use Gerrit), 2015-04-29 16:30:59]

On 2015/04/29 16:15:13, Peter Beverloo wrote:
> On 2015/04/29 16:07:37, Avi wrote:
> > On 2015/04/29 15:50:00, Peter Beverloo wrote:
> > > This is where my mental model of layering seems to be off. What is the
> > > dependency relationship between //ui and //content?
> > 
> > It's complicated. (That means I don't fully understand it myself.)
> > 
> > From what I understand, content grabs core graphics stuff from ui, like
> ui/gfx,
> > ui/aura, ui/base, etc, but that's about it. 
> > 
> > >   (1) NotificationPresentationData, which can be cleanly converted to a
> > > mc::Notification by //chrome.
> > 
> > Which would be the basic data specified by the spec?
> 
> A sub-set of that - strictly the data necessary to display the notification.
To
> illustrate:
> 
>   * gfx::Image for the icon, but no image URL. (Similar with other resources
> when we support that, e.g. sounds.)
>   * No "data" (author-provided data), tags, languages / direction hints which
we
> don't use.
> 
> > >   (2) NotificationPresentationDelegate, which defines the methods of a
> > > mc::NotificationDelegate used by //content. (Using a proxy.)
> > 
> > The idea here is that the content layer has to have to have a way to say
> > "present this thing", and the embedder implements that way?
> > 
> > That sounds like a reasonable approach.
> 
> Yes. "Present this thing", and "tell me when something happened to it."

Yeah, that sounds like something good. ContentBrowserClient, probably.

[dewittj, 2015-05-02 18:43:29]

https://codereview.chromium.org/1116693002/diff/1/content/public/browser/plat...
File content/public/browser/platform_notification_service.h (right):

https://codereview.chromium.org/1116693002/diff/1/content/public/browser/plat...
content/public/browser/platform_notification_service.h:47: // Displays the
|notification| to the user. This method must be called on the
I like the simplification.  If layering is an issue we can always use another
struct besides m_c::Notification and teach notifications how to use them.

[Michael van Ouwerkerk, 2015-06-23 16:16:27]

mvanouwerkerk@chromium.org changed reviewers:
- mvanouwerkerk@chromium.org

[dewittj, 2015-08-31 16:02:07]

dewittj@chromium.org changed reviewers:
- dewittj@chromium.org