app_sorting() calls should go via ExtensionSystem intsead of prefs.

app_sorting() in extension_prefs.h should be private and all access should go
via ExtensionSystem to access app_sorting().

BUG=550759
Committed: https://crrev.com/4ba69e641911cddbee70c18b8b5c740f9b86ca7d
Cr-Commit-Position: refs/heads/master@{#360014}

[Deepak, 2015-11-03 04:44:38]

Description was changed from

==========
test private

BUG=
==========

to

==========
app_sorting() in extension_prefs.h should be private and all access should go
via ExtensionSystem to access app_sorting().

BUG=550759
==========

[Deepak, 2015-11-03 04:45:28]

deepak.m1@samsung.com changed reviewers:
+ benwells@chromium.org

[Deepak, 2015-11-03 04:45:28]

PTAL
Thanks.

[benwells, 2015-11-03 21:09:23]

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

[benwells, 2015-11-03 21:09:48]

On 2015/11/03 04:45:28, Delhi_Deepak wrote:
> PTAL
> Thanks.

I've started a CQ dry run. In future if you can run try jobs, please do so
before sending for review.

[commit-bot: I haz the power, 2015-11-03 21:09:53]

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

[commit-bot: I haz the power, 2015-11-03 21:47:10]

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

[commit-bot: I haz the power, 2015-11-03 21:47:10]

Dry run: This issue passed the CQ dry run.

[Deepak, 2015-11-04 03:57:30]

On 2015/11/03 21:47:10, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

@benwell
yes, I have permissions to run try jobs.
I will run try jobs before sending my next review requests.
Thanks

[benwells, 2015-11-06 01:04:38]

https://codereview.chromium.org/1419823011/diff/40001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_app_list_helper.cc (right):

https://codereview.chromium.org/1419823011/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_app_list_helper.cc:15: #include
"extensions/browser/extension_prefs.h"
nit: Is this include still needed?

https://codereview.chromium.org/1419823011/diff/40001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_apps_sync_test.cc (right):

https://codereview.chromium.org/1419823011/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_apps_sync_test.cc:22: #include
"extensions/browser/extension_prefs.h"
Nit: is this include still needed?

https://codereview.chromium.org/1419823011/diff/40001/extensions/browser/exte...
File extensions/browser/extension_prefs.h (right):

https://codereview.chromium.org/1419823011/diff/40001/extensions/browser/exte...
extensions/browser/extension_prefs.h:683: AppSorting* app_sorting() const;
Nit: should the .cc be reordered to match?

[Deepak, 2015-11-06 06:13:48]

Thanks for review.
Changes done as suggested.
PTAL

https://codereview.chromium.org/1419823011/diff/40001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_app_list_helper.cc (right):

https://codereview.chromium.org/1419823011/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_app_list_helper.cc:15: #include
"extensions/browser/extension_prefs.h"
On 2015/11/06 01:04:37, benwells wrote:
> nit: Is this include still needed?

Done.

https://codereview.chromium.org/1419823011/diff/40001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_apps_sync_test.cc (right):

https://codereview.chromium.org/1419823011/diff/40001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_apps_sync_test.cc:22: #include
"extensions/browser/extension_prefs.h"
On 2015/11/06 01:04:38, benwells wrote:
> Nit: is this include still needed?

Done.

https://codereview.chromium.org/1419823011/diff/40001/extensions/browser/exte...
File extensions/browser/extension_prefs.h (right):

https://codereview.chromium.org/1419823011/diff/40001/extensions/browser/exte...
extensions/browser/extension_prefs.h:683: AppSorting* app_sorting() const;
On 2015/11/06 01:04:38, benwells wrote:
> Nit: should the .cc be reordered to match?

Done.

[benwells, 2015-11-08 23:53:37]

lgtm.

Please keep rebase and your changes in separate patches to make it easier for
reviewers to see what you've done, vs. what other people have done.

[Deepak, 2015-11-09 05:36:34]

deepak.m1@samsung.com changed reviewers:
+ dbeam@chromium.org, pvalenzuela@chromium.org

[Deepak, 2015-11-09 05:36:35]

@Thanks benwells for review.

Adding 
@pvalenzuela for
chrome/browser/sync/test/integration/

@dbeam for
chrome/browser/ui/webui/ntp/

PTAL

[pval...(no longer on Chromium), 2015-11-10 15:22:10]

lgtm for chrome/browser/sync/test/integration/

[Deepak, 2015-11-12 05:39:39]

@Dan Beam
chrome/browser/ui/webui/ntp/
PTAL
Thanks

[Dan Beam, 2015-11-12 22:37:21]

https://codereview.chromium.org/1419823011/diff/60001/extensions/browser/exte...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/1419823011/diff/60001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:380: AppSorting*
ExtensionPrefs::app_sorting() const {
this should be positioned under ExtensionsPrefs constructor to match the .h
order

https://codereview.chromium.org/1419823011/diff/60001/extensions/browser/exte...
File extensions/browser/extension_prefs.h (right):

https://codereview.chromium.org/1419823011/diff/60001/extensions/browser/exte...
extensions/browser/extension_prefs.h:569: AppSorting* app_sorting() const;
this probably shouldn't be const if AppSorting* is mutable

[benwells, 2015-11-12 23:33:49]

https://codereview.chromium.org/1419823011/diff/60001/extensions/browser/exte...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/1419823011/diff/60001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:380: AppSorting*
ExtensionPrefs::app_sorting() const {
On 2015/11/12 22:37:21, Dan Beam wrote:
> this should be positioned under ExtensionsPrefs constructor to match the .h
> order

Unfortunately the file appears to be generally out of order. This function is
now correctly above ExtensionPrefs::MakePathsRelative but not correctly under
the constructor, because they're out of order already.

I'm not sure if its worth putting up another patch to correct the file
completely or not.

https://codereview.chromium.org/1419823011/diff/60001/extensions/browser/exte...
File extensions/browser/extension_prefs.h (right):

https://codereview.chromium.org/1419823011/diff/60001/extensions/browser/exte...
extensions/browser/extension_prefs.h:569: AppSorting* app_sorting() const;
On 2015/11/12 22:37:21, Dan Beam wrote:
> this probably shouldn't be const if AppSorting* is mutable

I think it is debatable and rests on whether the ExtensionSystem conceptually
owns the AppSorting object. Whichever way its done, it should match
pref_service() above.

Given this is a refactoring (and reducing the visibility of the method from
public to private) I think it is acceptable to leave it as is.

[Dan Beam, 2015-11-17 00:28:48]

lgtm

https://codereview.chromium.org/1419823011/diff/60001/extensions/browser/exte...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/1419823011/diff/60001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:380: AppSorting*
ExtensionPrefs::app_sorting() const {
On 2015/11/12 23:33:48, benwells wrote:
> On 2015/11/12 22:37:21, Dan Beam wrote:
> > this should be positioned under ExtensionsPrefs constructor to match the .h
> > order
> 
> Unfortunately the file appears to be generally out of order. This function is
> now correctly above ExtensionPrefs::MakePathsRelative but not correctly under
> the constructor, because they're out of order already.
> 
> I'm not sure if its worth putting up another patch to correct the file
> completely or not.

then revert moving it to begin with

https://codereview.chromium.org/1419823011/diff/60001/extensions/browser/exte...
File extensions/browser/extension_prefs.h (right):

https://codereview.chromium.org/1419823011/diff/60001/extensions/browser/exte...
extensions/browser/extension_prefs.h:569: AppSorting* app_sorting() const;
On 2015/11/12 23:33:49, benwells wrote:
> On 2015/11/12 22:37:21, Dan Beam wrote:
> > this probably shouldn't be const if AppSorting* is mutable
> 
> I think it is debatable and rests on whether the ExtensionSystem conceptually
> owns the AppSorting object. Whichever way its done, it should match
> pref_service() above.
> 
> Given this is a refactoring (and reducing the visibility of the method from
> public to private) I think it is acceptable to leave it as is.

ok

[Deepak, 2015-11-17 05:00:45]

@Dan Beam
Thanks for Review.
Comments addressed.

https://codereview.chromium.org/1419823011/diff/60001/extensions/browser/exte...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/1419823011/diff/60001/extensions/browser/exte...
extensions/browser/extension_prefs.cc:380: AppSorting*
ExtensionPrefs::app_sorting() const {
On 2015/11/17 00:28:48, Dan Beam wrote:
> On 2015/11/12 23:33:48, benwells wrote:
> > On 2015/11/12 22:37:21, Dan Beam wrote:
> > > this should be positioned under ExtensionsPrefs constructor to match the
.h
> > > order
> > 
> > Unfortunately the file appears to be generally out of order. This function
is
> > now correctly above ExtensionPrefs::MakePathsRelative but not correctly
under
> > the constructor, because they're out of order already.
> > 
> > I'm not sure if its worth putting up another patch to correct the file
> > completely or not.
> 
> then revert moving it to begin with

Done.

[Deepak, 2015-11-17 05:00:56]

The CQ bit was checked by deepak.m1@samsung.com

[Deepak, 2015-11-17 05:00:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org, dbeam@chromium.org, pvalenzuela@chromium.org
Link to the patchset: https://codereview.chromium.org/1419823011/#ps80001
(title: "Changes as per review comments.")

[commit-bot: I haz the power, 2015-11-17 05:02:00]

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

[commit-bot: I haz the power, 2015-11-17 05:42:20]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-17 05:43:33]

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