Refactor of ProfileInfoCache in c/b/notifications

Refactor of ProfileInfoCache in c/b/notifications

ProfileInfoCache is being refactored into ProfileAttributesStorage and ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See https://codereview.chromium.org/1599013002
for details.

Member variable index is removed from message_center::NotifierGroup
and its subclasses because it is apparently unused except in tests.

BUG=305720
Committed: https://crrev.com/99addc8596703ef749ff20c9bf98dc29a80e37ea
Cr-Commit-Position: refs/heads/master@{#374668}

[lwchkg, 2016-02-03 17:28:07]

Description was changed from

==========
Draft refactor of c/b/notifications.

BUG=305720
==========

to

==========
Draft refactor of c/b/notifications.

BUG=305720
==========

[lwchkg, 2016-02-03 17:28:07]

lwchkg@gmail.com changed reviewers:
+ anthonyvd@chromium.org, dewittj@chromium.org, peter@chromium.org

[lwchkg, 2016-02-03 17:32:53]

Description was changed from

==========
Draft refactor of c/b/notifications.

BUG=305720
==========

to

==========
123456789012345678901234567890123456789012345678901234567890123456789012
Draft refactor of ProfileInfoCache in c/b/notifications

We plan to change all instances of ProfileInfoCache into
ProfileAttributesStorage in the code base, because we want to remove the
use of indices in ProfileInfoCache.

The is a blocked WIP because message_center::NotifierGroup and it's
subclasses use the indices in their internal structures.

BUG=305720
==========

[lwchkg, 2016-02-03 17:32:59]

Description was changed from

==========
123456789012345678901234567890123456789012345678901234567890123456789012
Draft refactor of ProfileInfoCache in c/b/notifications

We plan to change all instances of ProfileInfoCache into
ProfileAttributesStorage in the code base, because we want to remove the
use of indices in ProfileInfoCache.

The is a blocked WIP because message_center::NotifierGroup and it's
subclasses use the indices in their internal structures.

BUG=305720
==========

to

==========
Draft refactor of ProfileInfoCache in c/b/notifications

We plan to change all instances of ProfileInfoCache into
ProfileAttributesStorage in the code base, because we want to remove the
use of indices in ProfileInfoCache.

The is a blocked WIP because message_center::NotifierGroup and it's
subclasses use the indices in their internal structures.

BUG=305720
==========

[lwchkg, 2016-02-03 17:40:12]

Dear all,

  This is still a WIP so there is no need to review yet. However, I need help
about NotifierGroup, so I'm sending the "review request" to you.

  Thanks for help in advance!

Regards,
WC Leung.

https://codereview.chromium.org/1657913003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/message_center_settings_controller.cc (right):

https://codereview.chromium.org/1657913003/diff/1/chrome/browser/notification...
chrome/browser/notifications/message_center_settings_controller.cc:522:
entry->GetPath()));
Do you know what is the use of the index in ProfileNotifierGroup?

[dewittj, 2016-02-03 17:43:59]

https://codereview.chromium.org/1657913003/diff/1/chrome/browser/notification...
File chrome/browser/notifications/message_center_settings_controller.cc (right):

https://codereview.chromium.org/1657913003/diff/1/chrome/browser/notification...
chrome/browser/notifications/message_center_settings_controller.cc:522:
entry->GetPath()));
On 2016/02/03 17:40:12, lwchkg wrote:
> Do you know what is the use of the index in ProfileNotifierGroup?

According to code search it isn't used at all.  I wonder if that was refactored
out.  Can you try just removing index from the NotifierGroup class and see what
happens?

[lwchkg, 2016-02-05 17:56:49]

Description was changed from

==========
Draft refactor of ProfileInfoCache in c/b/notifications

We plan to change all instances of ProfileInfoCache into
ProfileAttributesStorage in the code base, because we want to remove the
use of indices in ProfileInfoCache.

The is a blocked WIP because message_center::NotifierGroup and it's
subclasses use the indices in their internal structures.

BUG=305720
==========

to

==========
123456789012123456789012123456789012123456789012123456789012123456789012

Refactor of ProfileInfoCache in c/b/notifications

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical 
indices in the interface. See https://codereview.chromium.org/1599013002
for details.

Member variable index is removed from message_center::NotifierGroup
and its subclasses because it is apparently unused except in tests.

BUG=305720
==========

[lwchkg, 2016-02-05 17:56:56]

Description was changed from

==========
123456789012123456789012123456789012123456789012123456789012123456789012

Refactor of ProfileInfoCache in c/b/notifications

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical 
indices in the interface. See https://codereview.chromium.org/1599013002
for details.

Member variable index is removed from message_center::NotifierGroup
and its subclasses because it is apparently unused except in tests.

BUG=305720
==========

to

==========
Refactor of ProfileInfoCache in c/b/notifications

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical 
indices in the interface. See https://codereview.chromium.org/1599013002
for details.

Member variable index is removed from message_center::NotifierGroup
and its subclasses because it is apparently unused except in tests.

BUG=305720
==========

[lwchkg, 2016-02-05 18:02:34]

Patchset #2 (id:20001) has been deleted

[lwchkg, 2016-02-05 18:03:43]

> According to code search it isn't used at all.  I wonder if that was
refactored
> out.  Can you try just removing index from the NotifierGroup class and see
what
> happens?

Thanks. I've just uploaded another patch. PTAL and can you please start CQ dry
run for me?

[dewittj, 2016-02-05 18:05:29]

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

[commit-bot: I haz the power, 2016-02-05 18:05:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1657913003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1657913003/40001

[commit-bot: I haz the power, 2016-02-05 18:21:25]

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

[commit-bot: I haz the power, 2016-02-05 18:21:26]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)

[lwchkg, 2016-02-07 16:36:24]

Uploaded another patch. Please help me to start a CQ dry run again.

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller.cc (right):

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller.cc:507:
storage_.GetAllProfilesAttributes();
Is it needed to sort the entries in a particular order? I guess it's a no here.

[dewittj, 2016-02-08 17:37:30]

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

[commit-bot: I haz the power, 2016-02-08 17:38:12]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1657913003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1657913003/60001

[dewittj, 2016-02-08 18:07:10]

lgtm

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller.cc (right):

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller.cc:507:
storage_.GetAllProfilesAttributes();
On 2016/02/07 16:36:23, lwchkg wrote:
> Is it needed to sort the entries in a particular order? I guess it's a no
here.

At this point, I believe the number of profiles will always be 1 since this code
is only used in ChromeOS now.  |index| used to be used for sorting purposes.  So
long as GetAllPRofilesAttributes returns a reasonably stable sort order it
should be fine.

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller.h (right):

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller.h:138:
ProfileAttributesStorage& storage_;
nit: profile_attributes_storage_

https://codereview.chromium.org/1657913003/diff/60001/ui/message_center/notif...
File ui/message_center/notifier_settings.h (right):

https://codereview.chromium.org/1657913003/diff/60001/ui/message_center/notif...
ui/message_center/notifier_settings.h:11: #include <vector>
nit: I don't think this is used in this header file?

[lwchkg, 2016-02-08 19:06:43]

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller.cc (right):

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller.cc:507:
storage_.GetAllProfilesAttributes();
On 2016/02/08 18:07:10, dewittj wrote:
> On 2016/02/07 16:36:23, lwchkg wrote:
> > Is it needed to sort the entries in a particular order? I guess it's a no
> here.
> 
> At this point, I believe the number of profiles will always be 1 since this
code
> is only used in ChromeOS now.  |index| used to be used for sorting purposes. 
So
> long as GetAllPRofilesAttributes returns a reasonably stable sort order it
> should be fine.

Confused. Does the "1" count the profiles that are not yet open?
Anyway, I want to know what is meant exactly by "reasonably stable sort order".
We don't provide a guarantee that entries GetAllProfileAttributes is in any
order (sorting is in another CL, still being reviewed), so if you rely on any
sort order, then the code here will not work properly. I do suspect that
"reasonably stable sort order" means it's okay not to sort the entries.

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller.h (right):

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller.h:138:
ProfileAttributesStorage& storage_;
On 2016/02/08 18:07:10, dewittj wrote:
> nit: profile_attributes_storage_

Acknowledged.

https://codereview.chromium.org/1657913003/diff/60001/ui/message_center/notif...
File ui/message_center/notifier_settings.h (right):

https://codereview.chromium.org/1657913003/diff/60001/ui/message_center/notif...
ui/message_center/notifier_settings.h:11: #include <vector>
On 2016/02/08 18:07:10, dewittj wrote:
> nit: I don't think this is used in this header file?

See line 180 (marked in comment.)

https://codereview.chromium.org/1657913003/diff/60001/ui/message_center/notif...
ui/message_center/notifier_settings.h:180: virtual void
GetNotifierList(std::vector<Notifier*>* notifiers) = 0;
<vector> corresponds to here.

[commit-bot: I haz the power, 2016-02-08 19:09:27]

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

[commit-bot: I haz the power, 2016-02-08 19:09:27]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dewittj, 2016-02-08 19:11:45]

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller.cc (right):

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller.cc:507:
storage_.GetAllProfilesAttributes();
This code is used to show a UI so users can choose a profile to configure.  The
reason I say "reasonably stable" is that we don't really want the order changing
every time the user interacts with the config page, if we can avoid it.

In ChromeOS there is only one active user profile, so there will only really
ever be one shown - this is why there the sort order is moot anyway.

https://codereview.chromium.org/1657913003/diff/60001/ui/message_center/notif...
File ui/message_center/notifier_settings.h (right):

https://codereview.chromium.org/1657913003/diff/60001/ui/message_center/notif...
ui/message_center/notifier_settings.h:11: #include <vector>
On 2016/02/08 19:06:43, lwchkg wrote:
> On 2016/02/08 18:07:10, dewittj wrote:
> > nit: I don't think this is used in this header file?
> 
> See line 180 (marked in comment.)

right.  Thanks!

[lwchkg, 2016-02-08 19:22:00]

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller.cc (right):

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller.cc:507:
storage_.GetAllProfilesAttributes();
On 2016/02/08 19:11:45, dewittj wrote:
> This code is used to show a UI so users can choose a profile to configure. 
The
> reason I say "reasonably stable" is that we don't really want the order
changing
> every time the user interacts with the config page, if we can avoid it.
> 
> In ChromeOS there is only one active user profile, so there will only really
> ever be one shown - this is why there the sort order is moot anyway.

I see. How can I access that UI?

[dewittj, 2016-02-08 19:28:03]

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller.cc (right):

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller.cc:507:
storage_.GetAllProfilesAttributes();
On 2016/02/08 19:22:00, lwchkg wrote:
> On 2016/02/08 19:11:45, dewittj wrote:
> > This code is used to show a UI so users can choose a profile to configure. 
> The
> > reason I say "reasonably stable" is that we don't really want the order
> changing
> > every time the user interacts with the config page, if we can avoid it.
> > 
> > In ChromeOS there is only one active user profile, so there will only really
> > ever be one shown - this is why there the sort order is moot anyway.
> 
> I see. How can I access that UI?

1. Get a notification (could be from
https://tests.peter.sh/notification-generator/)
2. Click on the notification center icon in the shelf
3. Click the cog icon in the notification center

If there is more than one profile, a drop-down will be shown.  Again, though, on
ChromeOS since tehre is only one active user we will never show the drop down. 
The UI is inaccessible on other platforms.

[anthonyvd, 2016-02-08 21:48:35]

c/b/profiles/* lgtm

[lwchkg, 2016-02-10 14:57:24]

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
File chrome/browser/notifications/message_center_settings_controller.cc (right):

https://codereview.chromium.org/1657913003/diff/60001/chrome/browser/notifica...
chrome/browser/notifications/message_center_settings_controller.cc:507:
storage_.GetAllProfilesAttributes();
On 2016/02/08 19:28:03, dewittj wrote:
> On 2016/02/08 19:22:00, lwchkg wrote:
> > On 2016/02/08 19:11:45, dewittj wrote:
> > > This code is used to show a UI so users can choose a profile to configure.

> > The
> > > reason I say "reasonably stable" is that we don't really want the order
> > changing
> > > every time the user interacts with the config page, if we can avoid it.
> > > 
> > > In ChromeOS there is only one active user profile, so there will only
really
> > > ever be one shown - this is why there the sort order is moot anyway.
> > 
> > I see. How can I access that UI?
> 
> 1. Get a notification (could be from
> https://tests.peter.sh/notification-generator/)
> 2. Click on the notification center icon in the shelf
> 3. Click the cog icon in the notification center
> 
> If there is more than one profile, a drop-down will be shown.  Again, though,
on
> ChromeOS since tehre is only one active user we will never show the drop down.

> The UI is inaccessible on other platforms.

Thanks for explanation. I'll just land the CL.

[lwchkg, 2016-02-10 14:57:29]

The CQ bit was checked by lwchkg@gmail.com

[commit-bot: I haz the power, 2016-02-10 14:58:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1657913003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1657913003/60001

[commit-bot: I haz the power, 2016-02-10 16:15:06]

Description was changed from

==========
Refactor of ProfileInfoCache in c/b/notifications

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical 
indices in the interface. See https://codereview.chromium.org/1599013002
for details.

Member variable index is removed from message_center::NotifierGroup
and its subclasses because it is apparently unused except in tests.

BUG=305720
==========

to

==========
Refactor of ProfileInfoCache in c/b/notifications

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical 
indices in the interface. See https://codereview.chromium.org/1599013002
for details.

Member variable index is removed from message_center::NotifierGroup
and its subclasses because it is apparently unused except in tests.

BUG=305720
==========

[commit-bot: I haz the power, 2016-02-10 16:15:07]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-02-10 16:16:11]

Description was changed from

==========
Refactor of ProfileInfoCache in c/b/notifications

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical 
indices in the interface. See https://codereview.chromium.org/1599013002
for details.

Member variable index is removed from message_center::NotifierGroup
and its subclasses because it is apparently unused except in tests.

BUG=305720
==========

to

==========
Refactor of ProfileInfoCache in c/b/notifications

ProfileInfoCache is being refactored into ProfileAttributesStorage and
ProfileAttributesEntry, which use profile paths instead of numerical
indices in the interface. See https://codereview.chromium.org/1599013002
for details.

Member variable index is removed from message_center::NotifierGroup
and its subclasses because it is apparently unused except in tests.

BUG=305720

Committed: https://crrev.com/99addc8596703ef749ff20c9bf98dc29a80e37ea
Cr-Commit-Position: refs/heads/master@{#374668}
==========

[commit-bot: I haz the power, 2016-02-10 16:16:13]

Patchset 3 (id:??) landed as
https://crrev.com/99addc8596703ef749ff20c9bf98dc29a80e37ea
Cr-Commit-Position: refs/heads/master@{#374668}