Mirror app list hierarchy data in profile prefs.

Mirror app list hierarchy data in profile prefs.

This CL adds an app_list.model profile pref which is a write-only mirror of the
synced app list hierarchy data. This will be used to populate the app list
model before the sync system is initialized, preventing large ordering changes
in the app list that happens a couple of seconds after it is cold started.

BUG=372264
Committed: https://crrev.com/5ec7ec7d9b202464b7f8ba99613a6c47984b1c01
Cr-Commit-Position: refs/heads/master@{#297984}

[calamity, 2014-09-26 04:22:16]

Patchset #1 (id:1) has been deleted

[calamity, 2014-09-26 04:22:24]

Patchset #1 (id:20001) has been deleted

[calamity, 2014-09-26 04:41:01]

calamity@chromium.org changed reviewers:
+ mgiuca@chromium.org

[stevenjb, 2014-09-26 16:22:15]

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

[stevenjb, 2014-09-26 16:22:16]

We should have a unit test for AppListPrefs.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_prefs.h (right):

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.h:49: ITEM_TYPE_END = FOLDER_ITEM,
nit: I think it would be simpler to explicitly assign values to these enums, and
handle them explicitly rather than using any loops, especially since we don't
expect there to be very many.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.h:51: explicit AppListInfo(const
base::DictionaryValue* item_dict);
nit: Multiple constructors like this can be confusing, I think this would be
better as a static CreateFromDictionary method (or even just a file local method
if it is only used here).

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.h:56: std::string folder_id;
All of the sync code uses 'parent_id', we should be consistent here.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.h:62: syncer::StringOrdinal position;
We currently do not use the page_ordinal field from AppListSpecifics in the UI,
but we do sync it so that if/when we do start using it everything should just
work. We should track it here also.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.h:77: std::map<std::string,
AppListInfo> GetAllAppListInfos() const;
This is passing a potentially very large map by value. Instead the method should
take a pointer to the map to fill in as input to avoid unnecessary copying.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:212: :
app_list_prefs_(app_list_prefs), model_(model) {
one line per arg

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:253: };
This appears to be entirely independent of AppListSyncableService, I think it
should be implemented in a separate file.

[Matt Giuca, 2014-09-30 02:49:47]

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/two_client_app_list_sync_test.cc
(right):

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_app_list_sync_test.cc:413:
app_list::AppListPrefs::Get(GetProfile(1))->GetAllAppListInfos().size());
Can you add some checks on the values returned by GetAllAppListInfos (not just
the size)? Also can you add a test for GetAppListInfo?

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_prefs.cc (right):

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.cc:42:
AppListPrefs::AppListInfo::AppListInfo(const base::DictionaryValue* item_dict) {
Add comments delineating the methods of different types, like:

// AppListInfo

and later

// AppListPrefs

(This will make more sense when you move AppListInfo to the top of the class.)

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.cc:51: item_type =
AppListInfo::ITEM_TYPE_INVALID;
Return early; no else.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.cc:74:
scoped_ptr<base::DictionaryValue> info_dict(new base::DictionaryValue());
I think this should be a method on AppListInfo:

scoped_ptr<base::DictionaryValue> AppListInfo::ToDict() const;

Then SetAppListInfo can just be a one-liner. If you're taking Steven's advice to
make the constructor into a static factory method (ie FromDict), then it nicely
mirrors that method.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.cc:79: info_dict->Set(kModelItemType,
new base::FundamentalValue(info.item_type));
Should this be static_cast too?

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_prefs.h (right):

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.h:40: struct AppListInfo {
This definition should appear before the methods.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.h:49: ITEM_TYPE_END = FOLDER_ITEM,
I agree. You currently only use ITEM_TYPE_BEGIN/ITEM_TYPE_END to check validity.
You can do that explicitly and future-proof-ly using a switch statement. Make a
function "bool IsItemTypeValid(ItemType type)" with a switch statement that
returns false for ITEM_TYPE_INVALID and true for each other type, and then if
someone adds a new item type, the compiler will complain and they will need to
add a new item to the switch.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.h:51: explicit AppListInfo(const
base::DictionaryValue* item_dict);
+1

(You could also add another constructor that takes a folder_id, name, position
and item_type, if that would be useful. For a struct, I think it's fine to have
both a default constructor and a constructor that populates the fields directly;
if you want a cleverer constructor, make it a static factory method.)

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.h:62: syncer::StringOrdinal position;
We're explicitly trying to avoid having page_ordinal modelled here. (And we were
wondering what it was doing in AppListSpecifics.) Pages aren't part of the app
list model and never have been -- they are merely an artifact of the view.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:212: :
app_list_prefs_(app_list_prefs), model_(model) {
I think this is fine, as long as they are all on one line (ultimately, do
whatever ClangFormat does).

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:253: };
It's simple and could be considered an implementation detail of
AppListSyncableService; I think it's fine to leave it here.

[calamity, 2014-09-30 05:35:46]

Patchset #2 (id:60001) has been deleted

[calamity, 2014-09-30 05:40:08]

I'm not sure what extra value unit tests would provide here. There's not really
any logic that needs to be tested and coverage and read/write functionality is
tested by the sync test.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/sync/test...
File chrome/browser/sync/test/integration/two_client_app_list_sync_test.cc
(right):

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/sync/test...
chrome/browser/sync/test/integration/two_client_app_list_sync_test.cc:413:
app_list::AppListPrefs::Get(GetProfile(1))->GetAllAppListInfos().size());
On 2014/09/30 02:49:46, Matt Giuca wrote:
> Can you add some checks on the values returned by GetAllAppListInfos (not just
> the size)? Also can you add a test for GetAppListInfo?

Done.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_prefs.cc (right):

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.cc:42:
AppListPrefs::AppListInfo::AppListInfo(const base::DictionaryValue* item_dict) {
On 2014/09/30 02:49:46, Matt Giuca wrote:
> Add comments delineating the methods of different types, like:
> 
> // AppListInfo
> 
> and later
> 
> // AppListPrefs
> 
> (This will make more sense when you move AppListInfo to the top of the class.)

Done.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.cc:51: item_type =
AppListInfo::ITEM_TYPE_INVALID;
On 2014/09/30 02:49:47, Matt Giuca wrote:
> Return early; no else.

Done.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.cc:74:
scoped_ptr<base::DictionaryValue> info_dict(new base::DictionaryValue());
On 2014/09/30 02:49:46, Matt Giuca wrote:
> I think this should be a method on AppListInfo:
> 
> scoped_ptr<base::DictionaryValue> AppListInfo::ToDict() const;
> 
> Then SetAppListInfo can just be a one-liner. If you're taking Steven's advice
to
> make the constructor into a static factory method (ie FromDict), then it
nicely
> mirrors that method.

Done.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.cc:79: info_dict->Set(kModelItemType,
new base::FundamentalValue(info.item_type));
On 2014/09/30 02:49:46, Matt Giuca wrote:
> Should this be static_cast too?

Hmm. Doesn't seem to need to be.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_prefs.h (right):

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.h:40: struct AppListInfo {
On 2014/09/30 02:49:47, Matt Giuca wrote:
> This definition should appear before the methods.

Done.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.h:49: ITEM_TYPE_END = FOLDER_ITEM,
On 2014/09/30 02:49:47, Matt Giuca wrote:
> I agree. You currently only use ITEM_TYPE_BEGIN/ITEM_TYPE_END to check
validity.
> You can do that explicitly and future-proof-ly using a switch statement. Make
a
> function "bool IsItemTypeValid(ItemType type)" with a switch statement that
> returns false for ITEM_TYPE_INVALID and true for each other type, and then if
> someone adds a new item type, the compiler will complain and they will need to
> add a new item to the switch.

I'm using the begin and end values for validity checking.

Bleh. I tried to do this with an IsValidItemType(int) function but it loses the
future-proofness because a switch statement doesn't properly cover all cases of
the int. If I cast to an ItemType first, then the switch may encounter a value
that's invalid and the checks wouldn't really cover bad cases anyway.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.h:51: explicit AppListInfo(const
base::DictionaryValue* item_dict);
On 2014/09/26 16:22:16, stevenjb wrote:
> nit: Multiple constructors like this can be confusing, I think this would be
> better as a static CreateFromDictionary method (or even just a file local
method
> if it is only used here).
> 

Done.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.h:56: std::string folder_id;
On 2014/09/26 16:22:16, stevenjb wrote:
> All of the sync code uses 'parent_id', we should be consistent here.

Done.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.h:62: syncer::StringOrdinal position;
On 2014/09/26 16:22:16, stevenjb wrote:
> We currently do not use the page_ordinal field from AppListSpecifics in the
UI,
> but we do sync it so that if/when we do start using it everything should just
> work. We should track it here also.

The page ordinal field is completely unused at the moment. I have a CL to kill
that field as well which is at https://codereview.chromium.org/597873002/ .

It's used for the NTP page numbering but the app list doesn't have a concept of
model pages. If we ever want it, we can re-add it.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.h:77: std::map<std::string,
AppListInfo> GetAllAppListInfos() const;
On 2014/09/26 16:22:16, stevenjb wrote:
> This is passing a potentially very large map by value. Instead the method
should
> take a pointer to the map to fill in as input to avoid unnecessary copying.

Done.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:212: :
app_list_prefs_(app_list_prefs), model_(model) {
On 2014/09/30 02:49:47, Matt Giuca wrote:
> I think this is fine, as long as they are all on one line (ultimately, do
> whatever ClangFormat does).

Yeah, this is ClangFormat's doing.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:253: };
On 2014/09/30 02:49:47, Matt Giuca wrote:
> It's simple and could be considered an implementation detail of
> AppListSyncableService; I think it's fine to leave it here.

I also think of it as a utility class that allows the AppListSyncableService to
fulfill its responsibility to update the AppListPrefs. I could have shoved it
into the existing ModelObserver but I think the separation of responsibilities
of each observer is nicer.

[Matt Giuca, 2014-09-30 06:04:42]

lgtm (see question)

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_prefs.h (right):

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.h:49: ITEM_TYPE_END = FOLDER_ITEM,
On 2014/09/30 05:40:08, calamity wrote:
> On 2014/09/30 02:49:47, Matt Giuca wrote:
> > I agree. You currently only use ITEM_TYPE_BEGIN/ITEM_TYPE_END to check
> validity.
> > You can do that explicitly and future-proof-ly using a switch statement.
Make
> a
> > function "bool IsItemTypeValid(ItemType type)" with a switch statement that
> > returns false for ITEM_TYPE_INVALID and true for each other type, and then
if
> > someone adds a new item type, the compiler will complain and they will need
to
> > add a new item to the switch.
> 
> I'm using the begin and end values for validity checking.
> 
> Bleh. I tried to do this with an IsValidItemType(int) function but it loses
the
> future-proofness because a switch statement doesn't properly cover all cases
of
> the int. If I cast to an ItemType first, then the switch may encounter a value
> that's invalid and the checks wouldn't really cover bad cases anyway.

Acknowledged.

https://codereview.chromium.org/599343002/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_prefs.cc (right):

https://codereview.chromium.org/599343002/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.cc:24: const base::DictionaryValue*
item_dict) {
Any reason you didn't make these two guys methods of AppListInfo? It seems more
appropriate to put them there. (This one is a static; the other is not.)

[calamity, 2014-09-30 08:37:07]

calamity@chromium.org changed reviewers:
+ battre@chromium.org, zea@chromium.org

[calamity, 2014-09-30 08:37:08]

+battre for chrome/browser/browser_prefs.cc OWNERS
+zea for chrome/browser/sync OWNERS

https://codereview.chromium.org/599343002/diff/80001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_prefs.cc (right):

https://codereview.chromium.org/599343002/diff/80001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.cc:24: const base::DictionaryValue*
item_dict) {
On 2014/09/30 06:04:42, Matt Giuca wrote:
> Any reason you didn't make these two guys methods of AppListInfo? It seems
more
> appropriate to put them there. (This one is a static; the other is not.)

Done.

[battre, 2014-09-30 13:36:42]

LGTM with comment.

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_prefs.cc (right):

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_prefs.cc:16: const char kPrefModel[] =
"app_list.model";
I think this should go to chrome/common/pref_names.* - there are several items
prefixed with app_list already.

[stevenjb, 2014-09-30 18:16:24]

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_prefs.h (right):

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_prefs.h:62: syncer::StringOrdinal position;
On 2014/09/30 05:40:08, calamity wrote:
> On 2014/09/26 16:22:16, stevenjb wrote:
> > We currently do not use the page_ordinal field from AppListSpecifics in the
> UI,
> > but we do sync it so that if/when we do start using it everything should
just
> > work. We should track it here also.
> 
> The page ordinal field is completely unused at the moment. I have a CL to kill
> that field as well which is at https://codereview.chromium.org/597873002/ .
> 
> It's used for the NTP page numbering but the app list doesn't have a concept
of
> model pages. If we ever want it, we can re-add it.

We should probably have an "App Launcher" meeting to decide the fate of pages.

Originally, UX wanted users to be able to organize apps by page, similar to how
iPhones behave. The page_id was added to support that.

It is easier to ignore a sync field than it is to add one, so we put it in there
initially, but because of design complications have not yet implemented it.

If we are sure we are never going to implement this than we can go ahead and
eliminate it from the AppListSyncableService code, but if we think we might
implement it, the overhead of having it is pretty minimal imho.

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:212: :
app_list_prefs_(app_list_prefs), model_(model) {
On 2014/09/30 05:40:08, calamity wrote:
> On 2014/09/30 02:49:47, Matt Giuca wrote:
> > I think this is fine, as long as they are all on one line (ultimately, do
> > whatever ClangFormat does).
> 
> Yeah, this is ClangFormat's doing.

Hm, we should probably update the style guide then, but yes, we bow to
ClangFormat.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constructor_I...

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:253: };
On 2014/09/30 05:40:08, calamity wrote:
> On 2014/09/30 02:49:47, Matt Giuca wrote:
> > It's simple and could be considered an implementation detail of
> > AppListSyncableService; I think it's fine to leave it here.
> 
> I also think of it as a utility class that allows the AppListSyncableService
to
> fulfill its responsibility to update the AppListPrefs. I could have shoved it
> into the existing ModelObserver but I think the separation of responsibilities
> of each observer is nicer.

The reason I don't especially like it here is that it is not an implementation
detail of AppListSyncableService, there are no co-dependencies here, other than
that AppListSyncableService is a convenient place to own PrefUpdater (which I
think is fine). Having it here, especially as a member class, implies that there
are dependencies between the classes when if fact there are not. PrefUpdater
relies solely on AppListModel (as it should).

I think there might be some complexity when we implement conflict resolution
between AppListSyncableService and AppListPrefs. I think that having PrefUpdater
separated from AppListSyncableService might help lead to a cleaner / more
maintainable result.

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_prefs.cc (right):

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_prefs.cc:37: new
base::StringValue(position.ToInternalValue()));
nit: item_dict->SetString here and below

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_prefs.cc:40: item_dict->Set(kModelItemType,
new base::FundamentalValue(item_type));
nit: SetInteger

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_prefs.cc:49:
scoped_ptr<AppListPrefs::AppListInfo> info(new AppListPrefs::AppListInfo());
nit: AppListPrefs:: unnecessary inside this method

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_prefs.cc:112: std::map<std::string,
AppListInfo>* out) const {
nit: This would be more readable with a typedef for the map

[Nicolas Zea, 2014-09-30 21:29:34]

sync lgtm

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_app_list_sync_test.cc
(right):

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_app_list_sync_test.cc:71:
EXPECT_TRUE(info);
nit: assert_true

[calamity, 2014-10-01 08:40:11]

Patchset #4 (id:120001) has been deleted

[calamity, 2014-10-01 08:46:15]

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:253: };
On 2014/09/30 18:16:23, stevenjb wrote:
> On 2014/09/30 05:40:08, calamity wrote:
> > On 2014/09/30 02:49:47, Matt Giuca wrote:
> > > It's simple and could be considered an implementation detail of
> > > AppListSyncableService; I think it's fine to leave it here.
> > 
> > I also think of it as a utility class that allows the AppListSyncableService
> to
> > fulfill its responsibility to update the AppListPrefs. I could have shoved
it
> > into the existing ModelObserver but I think the separation of
responsibilities
> > of each observer is nicer.
> 
> The reason I don't especially like it here is that it is not an implementation
> detail of AppListSyncableService, there are no co-dependencies here, other
than
> that AppListSyncableService is a convenient place to own PrefUpdater (which I
> think is fine). Having it here, especially as a member class, implies that
there
> are dependencies between the classes when if fact there are not. PrefUpdater
> relies solely on AppListModel (as it should).
> 
> I think there might be some complexity when we implement conflict resolution
> between AppListSyncableService and AppListPrefs. I think that having
PrefUpdater
> separated from AppListSyncableService might help lead to a cleaner / more
> maintainable result.

Done.

FYI, I think our conflict resolution plan is to always have sync overwrite local
data. This means there is a brief window where local changes will be clobbered
but I think that's okay for now. If we really want to fix this, we can cache
local changes until the sync service is ready and then flush them.

Therefore the prefs should only ever write to the model when the
AppListSyncableService is first created, before MergeDataAndStartSyncing is
called.

When signing in for the first time without any prior sync data, the syncable
service will dump all the local prefs into sync.

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_app_list_sync_test.cc
(right):

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_app_list_sync_test.cc:71:
EXPECT_TRUE(info);
On 2014/09/30 21:29:34, Nicolas Zea wrote:
> nit: assert_true

Done.

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_prefs.cc (right):

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_prefs.cc:16: const char kPrefModel[] =
"app_list.model";
On 2014/09/30 13:36:42, battre wrote:
> I think this should go to chrome/common/pref_names.* - there are several items
> prefixed with app_list already.

Those are local state prefs. Since this is a profile pref and I only want it to
ever be modified through here, I'd rather keep it here only. This follows
ExtensionPrefs.

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_prefs.cc:37: new
base::StringValue(position.ToInternalValue()));
On 2014/09/30 18:16:24, stevenjb wrote:
> nit: item_dict->SetString here and below

Done.

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_prefs.cc:40: item_dict->Set(kModelItemType,
new base::FundamentalValue(item_type));
On 2014/09/30 18:16:23, stevenjb wrote:
> nit: SetInteger

Done.

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_prefs.cc:49:
scoped_ptr<AppListPrefs::AppListInfo> info(new AppListPrefs::AppListInfo());
On 2014/09/30 18:16:23, stevenjb wrote:
> nit: AppListPrefs:: unnecessary inside this method

Done.

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_prefs.cc:112: std::map<std::string,
AppListInfo>* out) const {
On 2014/09/30 18:16:23, stevenjb wrote:
> nit: This would be more readable with a typedef for the map

Done.

[battre, 2014-10-01 08:53:18]

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/ui/app_l...
File chrome/browser/ui/app_list/app_list_prefs.cc (right):

https://codereview.chromium.org/599343002/diff/100001/chrome/browser/ui/app_l...
chrome/browser/ui/app_list/app_list_prefs.cc:16: const char kPrefModel[] =
"app_list.model";
On 2014/10/01 08:46:15, calamity wrote:
> On 2014/09/30 13:36:42, battre wrote:
> > I think this should go to chrome/common/pref_names.* - there are several
items
> > prefixed with app_list already.
> 
> Those are local state prefs. Since this is a profile pref and I only want it
to
> ever be modified through here, I'd rather keep it here only. This follows
> ExtensionPrefs.

Acknowledged.

[stevenjb, 2014-10-01 16:29:33]

1. We still need resolution on 'page_ordinal', although I am OK with committing
this without that for now; at this point I don't see any potential problems with
adding it later.

2. I still think that a unit test for ModelPrefUpdater that confirms that
changes to AppListModel are reflected in the pref store would be useful. It
would certainly be an easier / faster way to identify problems if, for example,
changes were made to the model observer behavior for the sake of
AppListSyncableService but not reflected in ModelPrefUpdater. It would also
identify any problems with assumptions about the pref store behavior. (I'm not
anticipating any, but a unit test would ensure that the pref store behaves as
expected).

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
File chrome/browser/ui/app_list/app_list_syncable_service.cc (right):

https://codereview.chromium.org/599343002/diff/40001/chrome/browser/ui/app_li...
chrome/browser/ui/app_list/app_list_syncable_service.cc:253: };
On 2014/10/01 08:46:14, calamity wrote:
> On 2014/09/30 18:16:23, stevenjb wrote:
> > On 2014/09/30 05:40:08, calamity wrote:
> > > On 2014/09/30 02:49:47, Matt Giuca wrote:
> > > > It's simple and could be considered an implementation detail of
> > > > AppListSyncableService; I think it's fine to leave it here.
> > > 
> > > I also think of it as a utility class that allows the
AppListSyncableService
> > to
> > > fulfill its responsibility to update the AppListPrefs. I could have shoved
> it
> > > into the existing ModelObserver but I think the separation of
> responsibilities
> > > of each observer is nicer.
> > 
> > The reason I don't especially like it here is that it is not an
implementation
> > detail of AppListSyncableService, there are no co-dependencies here, other
> than
> > that AppListSyncableService is a convenient place to own PrefUpdater (which
I
> > think is fine). Having it here, especially as a member class, implies that
> there
> > are dependencies between the classes when if fact there are not. PrefUpdater
> > relies solely on AppListModel (as it should).
> > 
> > I think there might be some complexity when we implement conflict resolution
> > between AppListSyncableService and AppListPrefs. I think that having
> PrefUpdater
> > separated from AppListSyncableService might help lead to a cleaner / more
> > maintainable result.
> 
> Done.
> 
> FYI, I think our conflict resolution plan is to always have sync overwrite
local
> data. This means there is a brief window where local changes will be clobbered
> but I think that's okay for now. If we really want to fix this, we can cache
> local changes until the sync service is ready and then flush them.
> 
> Therefore the prefs should only ever write to the model when the
> AppListSyncableService is first created, before MergeDataAndStartSyncing is
> called.
> 
> When signing in for the first time without any prior sync data, the syncable
> service will dump all the local prefs into sync.

There are a couple of things that concern me off the top of my head. We probably
shouldn't discuss them here, but just to mention them:

1. A Windows user with sync disabled installs and organizes a bunch of apps.
They also have a bunch of apps on a Chromebook. When they enable sync on
Windows, do we clobber all of the local data? What about apps that were not
installed on the Chromebook?

2."When signing in for the first time without any prior sync data" isn't
entirely straightforward. We don't know whether or not we have any sync data
until MergeDataAndStartSyncing gets called. At that point, the initial data set
might have positions for some apps that are in AppListPrefs but not others.

*Hopefully* we will be able to resolve this in a straightforward manner that is
satisfactory, but if there is some complexity there, I would like to keep it out
of this already large and complicated class/file, so thanks.


I am also hopeful that a straightforward approach will only have minor edge
cases that we do not need to be concerned with, but if we do run into any
complexity I think it will be helpful to have these classes separated, so
thanks.

[Matt Giuca, 2014-10-02 00:17:00]

We just had a chat with Alex and he said we're not likely to need page data in
the near future, so to just get rid of it.

If we do decide to support explicit paging in the future, it's very easy to add
it back.

[stevenjb, 2014-10-02 15:43:27]

On 2014/10/02 00:17:00, Matt Giuca wrote:
> We just had a chat with Alex and he said we're not likely to need page data in
> the near future, so to just get rid of it.
> 
> If we do decide to support explicit paging in the future, it's very easy to
add
> it back.

sgtm

I would still like to see unit tests for this, but would be willing to let that
go, at least until we start writing to AppListModel from ModelPrefUpdater.

lgtm

[calamity, 2014-10-03 02:54:11]

The CQ bit was checked by calamity@chromium.org

[calamity, 2014-10-03 02:54:26]

I added a simple unit test.

[commit-bot: I haz the power, 2014-10-03 02:54:53]

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

[calamity, 2014-10-03 02:55:10]

The CQ bit was unchecked by calamity@chromium.org

[calamity, 2014-10-03 03:56:47]

The CQ bit was checked by calamity@chromium.org

[commit-bot: I haz the power, 2014-10-03 03:57:12]

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

[commit-bot: I haz the power, 2014-10-03 03:57:57]

Committed patchset #5 (id:160001) as b5f95001be30fad7a2de39da5ffd7d7bdf46dc6a

[commit-bot: I haz the power, 2014-10-03 03:58:35]

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

[stevenjb, 2014-10-03 16:14:41]

On 2014/10/03 02:54:26, calamity wrote:
> I added a simple unit test.

Thanks!