Introduce the PlatformNotificationContext class.

Introduce the PlatformNotificationContext class.

This class provides an interface, to be used on the IO thread, on top
of the NotificationDatabase class. It abstracts away the thread
requirements of the database class and is responsible for managing the
status and the lifetime of the database.

Right now it's only used in unit tests, but it will eventually live as
a member of StoragePartitionImpl, and will be used by the notification
message filter.

Design document:
  http://goo.gl/TciXVp

BUG=447628
Committed: https://crrev.com/8f7612681b0b45c99010e90d5a0b7c1d80d9de8d
Cr-Commit-Position: refs/heads/master@{#320781}

[Peter Beverloo, 2015-03-13 16:20:58]

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

[Peter Beverloo, 2015-03-13 16:20:59]

+bauerb

This patch depends on:
  https://codereview.chromium.org/999933002/

[Bernhard Bauer, 2015-03-13 16:51:18]

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context.cc (right):

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.cc:49:
NotificationDatabaseData database_data;
DCHECK you're on the right thread? (Also below)

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context.h (right):

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.h:31: : public
base::RefCountedThreadSafe<PlatformNotificationContext> {
Is there a particular reason this class needs to be refcounted? If it's just to
be able to post tasks to other threads, an alternative might be to split this
class up into an outer class that lives on the IO thread, and an inner "Core"
that lives on the task runner thread.

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.h:60: protected:
Why is this protected?

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.h:67: //
|success_closure| will be invoked on a the thread which must be used to
The "the thread which must be used to access |database_|" is just the
|task_runner_| thread, right?

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.h:98:
scoped_refptr<base::SequencedTaskRunner> task_runner);
Pass by const-ref?

[Peter Beverloo, 2015-03-13 16:57:07]

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context.cc (right):

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.cc:49:
NotificationDatabaseData database_data;
On 2015/03/13 16:51:18, Bernhard Bauer wrote:
> DCHECK you're on the right thread? (Also below)

All the public methods in database_ have the appropriate DCHECKs as the very
first line in their implementation.

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context.h (right):

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.h:31: : public
base::RefCountedThreadSafe<PlatformNotificationContext> {
On 2015/03/13 16:51:18, Bernhard Bauer wrote:
> Is there a particular reason this class needs to be refcounted? If it's just
to
> be able to post tasks to other threads, an alternative might be to split this
> class up into an outer class that lives on the IO thread, and an inner "Core"
> that lives on the task runner thread.

It will be owned by StoragePartitionImpl (like the other storage contexts), but
the NotificationMessageFilter will also want to access the class.

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/st...

Note that the definition of the public methods and types will eventually move to
the content/ API, but I'd like to do that when the churn is over.

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.h:60: protected:
On 2015/03/13 16:51:18, Bernhard Bauer wrote:
> Why is this protected?

Typo. Fixed!

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.h:67: //
|success_closure| will be invoked on a the thread which must be used to
On 2015/03/13 16:51:18, Bernhard Bauer wrote:
> The "the thread which must be used to access |database_|" is just the
> |task_runner_| thread, right?

Done.

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.h:98:
scoped_refptr<base::SequencedTaskRunner> task_runner);
On 2015/03/13 16:51:18, Bernhard Bauer wrote:
> Pass by const-ref?

Done.

[Bernhard Bauer, 2015-03-13 17:24:02]

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context.cc (right):

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.cc:49:
NotificationDatabaseData database_data;
On 2015/03/13 16:57:07, Peter Beverloo wrote:
> On 2015/03/13 16:51:18, Bernhard Bauer wrote:
> > DCHECK you're on the right thread? (Also below)
> 
> All the public methods in database_ have the appropriate DCHECKs as the very
> first line in their implementation.

Those do, but there are also methods like LazyInitialize, and in general DCHECKs
also serve as documentation about which method runs on which thread (which you
diligently explain in the header, but that's distant as well).

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context.h (right):

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.h:31: : public
base::RefCountedThreadSafe<PlatformNotificationContext> {
On 2015/03/13 16:57:07, Peter Beverloo wrote:
> On 2015/03/13 16:51:18, Bernhard Bauer wrote:
> > Is there a particular reason this class needs to be refcounted? If it's just
> to
> > be able to post tasks to other threads, an alternative might be to split
this
> > class up into an outer class that lives on the IO thread, and an inner
"Core"
> > that lives on the task runner thread.
> 
> It will be owned by StoragePartitionImpl (like the other storage contexts),
but
> the NotificationMessageFilter will also want to access the class.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/st...
> 
> Note that the definition of the public methods and types will eventually move
to
> the content/ API, but I'd like to do that when the churn is over.

Urgh, so many refcounted classes :-( But fair enough (assuming that
NotificationMessageFilter will keep a reference to it).

https://codereview.chromium.org/1006493005/diff/40001/content/browser/notific...
File content/browser/notifications/platform_notification_context.cc (right):

https://codereview.chromium.org/1006493005/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context.cc:123:
database_.reset(new NotificationDatabase(GetDatabasePath()));
Hm, I think you might be able to simplify both this class and the database a
little bit if you construct the database on the database thread (in
OpenDatabase, lazily) and just directly jump to it here.

[Peter Beverloo, 2015-03-13 20:43:31]

All done!

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
File content/browser/notifications/platform_notification_context.cc (right):

https://codereview.chromium.org/1006493005/diff/1/content/browser/notificatio...
content/browser/notifications/platform_notification_context.cc:49:
NotificationDatabaseData database_data;
On 2015/03/13 17:24:01, Bernhard (OOO until Mar 23) wrote:
> On 2015/03/13 16:57:07, Peter Beverloo wrote:
> > On 2015/03/13 16:51:18, Bernhard Bauer wrote:
> > > DCHECK you're on the right thread? (Also below)
> > 
> > All the public methods in database_ have the appropriate DCHECKs as the very
> > first line in their implementation.
> 
> Those do, but there are also methods like LazyInitialize, and in general
DCHECKs
> also serve as documentation about which method runs on which thread (which you
> diligently explain in the header, but that's distant as well).

Done.

https://codereview.chromium.org/1006493005/diff/40001/content/browser/notific...
File content/browser/notifications/platform_notification_context.cc (right):

https://codereview.chromium.org/1006493005/diff/40001/content/browser/notific...
content/browser/notifications/platform_notification_context.cc:123:
database_.reset(new NotificationDatabase(GetDatabasePath()));
On 2015/03/13 17:24:01, Bernhard (OOO until Mar 23) wrote:
> Hm, I think you might be able to simplify both this class and the database a
> little bit if you construct the database on the database thread (in
> OpenDatabase, lazily) and just directly jump to it here.

Done.

[Bernhard Bauer, 2015-03-13 21:00:17]

LGTM!

https://codereview.chromium.org/1006493005/diff/60001/content/browser/notific...
File content/browser/notifications/notification_database.h (right):

https://codereview.chromium.org/1006493005/diff/60001/content/browser/notific...
content/browser/notifications/notification_database.h:28: // This class can be
constructed on any thread, but method calls must only be
You could tighten this restriction now, and not detach the sequence checker in
the constructor.

[Peter Beverloo, 2015-03-16 12:06:22]

https://codereview.chromium.org/1006493005/diff/60001/content/browser/notific...
File content/browser/notifications/notification_database.h (right):

https://codereview.chromium.org/1006493005/diff/60001/content/browser/notific...
content/browser/notifications/notification_database.h:28: // This class can be
constructed on any thread, but method calls must only be
On 2015/03/13 21:00:17, Bernhard (OOO until Mar 23) wrote:
> You could tighten this restriction now, and not detach the sequence checker in
> the constructor.

Done! I also added some more information about the uniqueness of notification
ids to the class-level comment.

[johnme, 2015-03-16 16:54:42]

johnme@chromium.org changed reviewers:
+ johnme@chromium.org

[johnme, 2015-03-16 16:54:43]

https://codereview.chromium.org/1006493005/diff/80001/content/browser/notific...
File content/browser/notifications/platform_notification_context.cc (right):

https://codereview.chromium.org/1006493005/diff/80001/content/browser/notific...
content/browser/notifications/platform_notification_context.cc:158:
BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, failure_closure);
Surely you should also reset database_ to nullptr, or else subsequent calls to
OpenDatabase will early out and run the success_closure?

https://codereview.chromium.org/1006493005/diff/80001/content/browser/notific...
File content/browser/notifications/platform_notification_context.h (right):

https://codereview.chromium.org/1006493005/diff/80001/content/browser/notific...
content/browser/notifications/platform_notification_context.h:28: // Represents
the per-BrowserContext data for persistent Web Notifications.
http://goo.gl/TciXVp says it's per-StoragePartitionImpl not per-BrowserContext.
Which is it?

https://codereview.chromium.org/1006493005/diff/80001/content/browser/notific...
content/browser/notifications/platform_notification_context.h:29: // The methods
defined in this interface must only be called on the IO thread.
Except OpenDatabase, DoRead and DoWrite. Add "unless otherwise specified"?

https://codereview.chromium.org/1006493005/diff/80001/content/browser/notific...
content/browser/notifications/platform_notification_context.h:39:
base::Callback<void(bool /* success */,
Nit: you don't need to comment out the parameter names (ditto below).

https://codereview.chromium.org/1006493005/diff/80001/content/browser/notific...
content/browser/notifications/platform_notification_context.h:75: // thread,
otherwise |failure_closure_ will be invoked on the IO thread.
Nit: |failure_closure_|

[Peter Beverloo, 2015-03-16 17:55:01]

https://codereview.chromium.org/1006493005/diff/80001/content/browser/notific...
File content/browser/notifications/platform_notification_context.cc (right):

https://codereview.chromium.org/1006493005/diff/80001/content/browser/notific...
content/browser/notifications/platform_notification_context.cc:158:
BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, failure_closure);
On 2015/03/16 16:54:43, johnme wrote:
> Surely you should also reset database_ to nullptr, or else subsequent calls to
> OpenDatabase will early out and run the success_closure?

Done.

https://codereview.chromium.org/1006493005/diff/80001/content/browser/notific...
File content/browser/notifications/platform_notification_context.h (right):

https://codereview.chromium.org/1006493005/diff/80001/content/browser/notific...
content/browser/notifications/platform_notification_context.h:28: // Represents
the per-BrowserContext data for persistent Web Notifications.
On 2015/03/16 16:54:43, johnme wrote:
> http://goo.gl/TciXVp says it's per-StoragePartitionImpl not
per-BrowserContext.
> Which is it?

It's per storage partition. Amended.

https://codereview.chromium.org/1006493005/diff/80001/content/browser/notific...
content/browser/notifications/platform_notification_context.h:29: // The methods
defined in this interface must only be called on the IO thread.
On 2015/03/16 16:54:43, johnme wrote:
> Except OpenDatabase, DoRead and DoWrite. Add "unless otherwise specified"?

Changed it to "The public methods" instead.

https://codereview.chromium.org/1006493005/diff/80001/content/browser/notific...
content/browser/notifications/platform_notification_context.h:39:
base::Callback<void(bool /* success */,
On 2015/03/16 16:54:43, johnme wrote:
> Nit: you don't need to comment out the parameter names (ditto below).

Technically I don't, but If I read "base::Callback<void(bool, int64_t)>" I don't
know what I'm going to get. I include them for readability purposes.

https://codereview.chromium.org/1006493005/diff/80001/content/browser/notific...
content/browser/notifications/platform_notification_context.h:75: // thread,
otherwise |failure_closure_ will be invoked on the IO thread.
On 2015/03/16 16:54:43, johnme wrote:
> Nit: |failure_closure_|

Done.

[johnme, 2015-03-16 19:05:11]

lgtm

[Peter Beverloo, 2015-03-16 19:05:29]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-03-16 19:05:29]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1006493005/#ps100001
(title: "y")

[commit-bot: I haz the power, 2015-03-16 19:06:20]

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

[commit-bot: I haz the power, 2015-03-16 20:11:24]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-03-16 20:11:51]

Patchset 6 (id:??) landed as
https://crrev.com/8f7612681b0b45c99010e90d5a0b7c1d80d9de8d
Cr-Commit-Position: refs/heads/master@{#320781}