Create AppListSyncableService with BrowserContext

Create AppListSyncableService with BrowserContext

BUG=352805
R=erg@chromium.org, jennyz@chromium.org, xiyuan@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=261443

[stevenjb, 2014-03-28 22:20:54]

This will help with issue 352805, although it seems to take about 7-8 seconds
for the class to actually get initialized this way, so it is only a partial fix.

[stevenjb, 2014-03-31 18:36:22]

OK, it turns out that you need to construct a Factory in order for
ServiceIsCreatedWithBrowserContext() to be meaningful. Obvious in hindsight.

I also did some precise timing and I am seeing about 300ms between
AppListSyncableService being constructed and MergeDataAndStartSyncing() getting
called (at which point the correct position information for any cached apps will
be available).

I was unable to open the App List quickly enough to see any shuffling with three
different test accounts.

PTAL

[xiyuan, 2014-03-31 19:37:14]

lgtm

[jennyz, 2014-03-31 19:41:56]

lgtm

[stevenjb, 2014-03-31 19:55:04]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-03-31 19:57:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/217483004/60001

[commit-bot: I haz the power, 2014-03-31 20:25:59]

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

[commit-bot: I haz the power, 2014-03-31 20:25:59]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[stevenjb, 2014-03-31 20:33:04]

+erg@ for c/b/profiles owner

[Elliot Glaysher, 2014-03-31 21:41:11]

So, now that this is being force created, what is the additional startup latency
for people who aren't using the apps list?

[stevenjb, 2014-03-31 22:36:22]

On 2014/03/31 21:41:11, Elliot Glaysher wrote:
> So, now that this is being force created, what is the additional startup
latency
> for people who aren't using the apps list?
I wouldn't expect there to be - if the data has never been synced at all it
should be negligible. Even with a large number of synced apps the overhead
should be tiny compared to loading the apps themselves.

I ran half a dozen times on a Chrome OS device (Pixel) checking the results of
chrome://system/Profile[0] login-times, with and without the change, and saw no
measurable difference. Please let me know if there is a better metric you would
suggest.

[Elliot Glaysher, 2014-03-31 22:45:19]

On 2014/03/31 22:36:22, stevenjb wrote:
> On 2014/03/31 21:41:11, Elliot Glaysher wrote:
> > So, now that this is being force created, what is the additional startup
> latency
> > for people who aren't using the apps list?
> I wouldn't expect there to be - if the data has never been synced at all it
> should be negligible. Even with a large number of synced apps the overhead
> should be tiny compared to loading the apps themselves.
> 
> I ran half a dozen times on a Chrome OS device (Pixel) checking the results of
> chrome://system/Profile[0] login-times, with and without the change, and saw
no
> measurable difference. Please let me know if there is a better metric you
would
> suggest.

So, who consumes this data? If the view is consuming the data right out of the
AppListSyncableService, then this LGTM, I guess.

If there's another service that uses this data, that should probably be made to
DependsOn() AppListSyncableService in addition to this change.

[stevenjb, 2014-03-31 22:47:52]

The View consumes the data directly (or rather, indirectly through a Model
fed directly by AppListSyncableService).



On Mon, Mar 31, 2014 at 4:45 PM, <erg@chromium.org> wrote:

> On 2014/03/31 22:36:22, stevenjb wrote:
>
>> On 2014/03/31 21:41:11, Elliot Glaysher wrote:
>> > So, now that this is being force created, what is the additional startup
>> latency
>> > for people who aren't using the apps list?
>> I wouldn't expect there to be - if the data has never been synced at all
>> it
>> should be negligible. Even with a large number of synced apps the overhead
>> should be tiny compared to loading the apps themselves.
>>
>
>  I ran half a dozen times on a Chrome OS device (Pixel) checking the
>> results of
>> chrome://system/Profile[0] login-times, with and without the change, and
>> saw
>>
> no
>
>> measurable difference. Please let me know if there is a better metric you
>>
> would
>
>> suggest.
>>
>
> So, who consumes this data? If the view is consuming the data right out of
> the
> AppListSyncableService, then this LGTM, I guess.
>
> If there's another service that uses this data, that should probably be
> made to
> DependsOn() AppListSyncableService in addition to this change.
>
> https://codereview.chromium.org/217483004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[stevenjb, 2014-04-01 16:36:25]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-04-01 16:36:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/217483004/100001

[commit-bot: I haz the power, 2014-04-01 18:16:55]

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

[commit-bot: I haz the power, 2014-04-01 18:16:56]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[stevenjb, 2014-04-02 17:38:15]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-04-02 17:39:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/217483004/130001

[commit-bot: I haz the power, 2014-04-03 00:52:24]

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

[commit-bot: I haz the power, 2014-04-03 00:52:24]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[stevenjb, 2014-04-03 06:28:20]

The CQ bit was checked by stevenjb@chromium.org

[commit-bot: I haz the power, 2014-04-03 06:31:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/217483004/140001

[commit-bot: I haz the power, 2014-04-03 11:34:30]

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

[commit-bot: I haz the power, 2014-04-03 11:34:30]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[stevenjb, 2014-04-03 16:35:41]

Committed patchset #9 manually as r261443 (presubmit successful).