chromeos: Notify about Chrome note-taking apps.

chromeos: Notify about Chrome note-taking apps.

Make NoteTakingHelper notify its observers when Chrome
note-taking apps are loaded or unloaded.

BUG=633596
Committed: https://crrev.com/e97a722caafc2d3a8b807ee115bfb307f7d2cfd9
Cr-Commit-Position: refs/heads/master@{#440174}

[Daniel Erat, 2016-12-20 00:13:58]

The CQ bit was checked by derat@chromium.org to run a CQ dry run

[Daniel Erat, 2016-12-20 00:14:14]

derat@chromium.org changed reviewers:
+ jdufault@chromium.org

[commit-bot: I haz the power, 2016-12-20 00:14:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 00:45:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 00:46:00]

Dry run: This issue passed the CQ dry run.

[Daniel Erat, 2016-12-20 17:14:45]

The CQ bit was checked by derat@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 17:15:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Daniel Erat, 2016-12-20 17:15:50]

upon testing on a device with the followup change to add the setting, i realized
that this code would probably work better if i actually registered as an
ExtensionRegistry observer instead of just implementing the interface. :-P doing
so resulted in some more profile-related test code.

[commit-bot: I haz the power, 2016-12-20 18:38:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 18:38:16]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Daniel Erat, 2016-12-20 19:39:35]

The CQ bit was checked by derat@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 19:40:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 20:19:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-20 20:19:21]

Dry run: This issue passed the CQ dry run.

[Daniel Erat, 2016-12-21 16:13:27]

derat@chromium.org changed reviewers:
+ xiyuan@chromium.org

[Daniel Erat, 2016-12-21 16:13:28]

adding xiyuan as a reviewer since you're the gardener right now, jacob. :-)

as background: this class will be used by settings to get a list of chrome
note-taking apps (currently just whitelisted google keep IDs) and provide
notifications when the apps are updated. this change makes it watch for
extensions being added or removed.

[xiyuan, 2016-12-21 16:46:51]

Just one request.

https://codereview.chromium.org/2587913006/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/note_taking_helper.cc (right):

https://codereview.chromium.org/2587913006/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/note_taking_helper.cc:264:
extensions::ExtensionRegistry::Get(profile)->AddObserver(this);
Suggest to use base::ScopedObserver. It can track multiple sources
(ExtensionRegistry).

[Daniel Erat, 2016-12-21 17:04:53]

https://codereview.chromium.org/2587913006/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/note_taking_helper.cc (right):

https://codereview.chromium.org/2587913006/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/note_taking_helper.cc:264:
extensions::ExtensionRegistry::Get(profile)->AddObserver(this);
On 2016/12/21 16:46:51, xiyuan wrote:
> Suggest to use base::ScopedObserver. It can track multiple sources
> (ExtensionRegistry).

thanks, i'd forgotten about ScopedObserver and additionally didn't know it could
handle multiple sources! :-)

[Daniel Erat, 2016-12-21 17:04:58]

The CQ bit was checked by derat@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 17:05:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiyuan, 2016-12-21 17:11:17]

lgtm

https://codereview.chromium.org/2587913006/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/note_taking_helper.cc (right):

https://codereview.chromium.org/2587913006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/note_taking_helper.cc:363:
CHECK(!extension_registry_observer_.IsObserving(registry));
nit: DCHECK should be enough. NOTIFICATION_PROFILE_ADDED should only be fired
once for a profile.

[commit-bot: I haz the power, 2016-12-21 17:44:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-21 17:44:59]

Dry run: This issue passed the CQ dry run.

[Daniel Erat, 2016-12-21 18:08:52]

https://codereview.chromium.org/2587913006/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/note_taking_helper.cc (right):

https://codereview.chromium.org/2587913006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/note_taking_helper.cc:363:
CHECK(!extension_registry_observer_.IsObserving(registry));
On 2016/12/21 17:11:17, xiyuan wrote:
> nit: DCHECK should be enough. NOTIFICATION_PROFILE_ADDED should only be fired
> once for a profile.

thanks, makes sense!

i realized that i probably have a problem here due to ExtensionRegistry objects
being shared between regular and incognito profiles:

  - i see _ADDED for regular profiles but not incognito ones (luckily, or else
this check would be hit)
  - i see _DESTROYED for both regular and incognito profiles
  - there's a _CREATED notification that's presumably sent for both

so if an incognito window is opened and then closed, i think i'll incorrectly
stop observing the regular profile's registry when i see _DESTROYED for the
incognito profile.

one workaround might be not unregistering when _DESTROYED is received for an
incognito profile, but i'm wondering if there's a cleaner way to handle this.
any thoughts?

[xiyuan, 2016-12-21 18:19:41]

https://codereview.chromium.org/2587913006/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/note_taking_helper.cc (right):

https://codereview.chromium.org/2587913006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/note_taking_helper.cc:363:
CHECK(!extension_registry_observer_.IsObserving(registry));
On 2016/12/21 18:08:51, Daniel Erat wrote:
> On 2016/12/21 17:11:17, xiyuan wrote:
> > nit: DCHECK should be enough. NOTIFICATION_PROFILE_ADDED should only be
fired
> > once for a profile.
> 
> thanks, makes sense!
> 
> i realized that i probably have a problem here due to ExtensionRegistry
objects
> being shared between regular and incognito profiles:
> 
>   - i see _ADDED for regular profiles but not incognito ones (luckily, or else
> this check would be hit)
>   - i see _DESTROYED for both regular and incognito profiles
>   - there's a _CREATED notification that's presumably sent for both
> 
> so if an incognito window is opened and then closed, i think i'll incorrectly
> stop observing the regular profile's registry when i see _DESTROYED for the
> incognito profile.
> 
> one workaround might be not unregistering when _DESTROYED is received for an
> incognito profile, but i'm wondering if there's a cleaner way to handle this.
> any thoughts?

ExtensionRegistryObserver has an "OnShutdown" method. Maybe we clean up there
instead of observing _DESTROYED?

[Daniel Erat, 2016-12-21 18:41:27]

https://codereview.chromium.org/2587913006/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/note_taking_helper.cc (right):

https://codereview.chromium.org/2587913006/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/note_taking_helper.cc:363:
CHECK(!extension_registry_observer_.IsObserving(registry));
On 2016/12/21 18:19:41, xiyuan wrote:
> On 2016/12/21 18:08:51, Daniel Erat wrote:
> > On 2016/12/21 17:11:17, xiyuan wrote:
> > > nit: DCHECK should be enough. NOTIFICATION_PROFILE_ADDED should only be
> fired
> > > once for a profile.
> > 
> > thanks, makes sense!
> > 
> > i realized that i probably have a problem here due to ExtensionRegistry
> objects
> > being shared between regular and incognito profiles:
> > 
> >   - i see _ADDED for regular profiles but not incognito ones (luckily, or
else
> > this check would be hit)
> >   - i see _DESTROYED for both regular and incognito profiles
> >   - there's a _CREATED notification that's presumably sent for both
> > 
> > so if an incognito window is opened and then closed, i think i'll
incorrectly
> > stop observing the regular profile's registry when i see _DESTROYED for the
> > incognito profile.
> > 
> > one workaround might be not unregistering when _DESTROYED is received for an
> > incognito profile, but i'm wondering if there's a cleaner way to handle
this.
> > any thoughts?
> 
> ExtensionRegistryObserver has an "OnShutdown" method. Maybe we clean up there
> instead of observing _DESTROYED?

thanks for pointing it out; that's much cleaner.

[Daniel Erat, 2016-12-21 18:41:33]

The CQ bit was checked by derat@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-21 18:41:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[xiyuan, 2016-12-21 18:42:51]

lgtm++

[Daniel Erat, 2016-12-21 18:45:24]

The CQ bit was unchecked by derat@chromium.org

[Daniel Erat, 2016-12-21 18:45:27]

The CQ bit was checked by derat@chromium.org

[commit-bot: I haz the power, 2016-12-21 18:45:41]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-21 19:18:29]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1482345927635910,
"parent_rev": "212cdffe7365dd31e74249f6b90df05f17b503e2", "commit_rev":
"206370b68b48ddb86c2dad7f26b95ff089d7271a"}

[commit-bot: I haz the power, 2016-12-21 19:18:57]

Description was changed from

==========
chromeos: Notify about Chrome note-taking apps.

Make NoteTakingHelper notify its observers when Chrome
note-taking apps are loaded or unloaded.

BUG=633596
==========

to

==========
chromeos: Notify about Chrome note-taking apps.

Make NoteTakingHelper notify its observers when Chrome
note-taking apps are loaded or unloaded.

BUG=633596

Review-Url: https://codereview.chromium.org/2587913006
==========

[commit-bot: I haz the power, 2016-12-21 19:18:58]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-12-21 19:22:58]

Description was changed from

==========
chromeos: Notify about Chrome note-taking apps.

Make NoteTakingHelper notify its observers when Chrome
note-taking apps are loaded or unloaded.

BUG=633596

Review-Url: https://codereview.chromium.org/2587913006
==========

to

==========
chromeos: Notify about Chrome note-taking apps.

Make NoteTakingHelper notify its observers when Chrome
note-taking apps are loaded or unloaded.

BUG=633596

Committed: https://crrev.com/e97a722caafc2d3a8b807ee115bfb307f7d2cfd9
Cr-Commit-Position: refs/heads/master@{#440174}
==========

[commit-bot: I haz the power, 2016-12-21 19:22:59]

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