Remove Ordinals Setters and Getters from ExtensionService

Remove Ordinals Setters and Getters from ExtensionService

Move the page and app launch setters and getters out of ExtensionService, moving their functionality into ExtensionSorting with the rest of the ordinal logic.


BUG=109769
TEST=ExtensionService unit tests pass and TwoClientAppsSync sync_integreation tests pass.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127953

[Aaron Boodman, 2012-03-14 18:54:27]

LGTM

http://codereview.chromium.org/9706017/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_sorting.cc (right):

http://codereview.chromium.org/9706017/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_sorting.cc:290: DCHECK(!ext ||
ext->is_app());
The extension system prefers CHECK to DCHECK.

http://codereview.chromium.org/9706017/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_sorting.cc:292: if (ext)
Move the DCHECK inside this branch and make it just DCHECK(ext->is_app()) ?

http://codereview.chromium.org/9706017/diff/1/chrome/browser/extensions/exten...
File chrome/browser/extensions/extension_sorting.h (right):

http://codereview.chromium.org/9706017/diff/1/chrome/browser/extensions/exten...
chrome/browser/extensions/extension_sorting.h:142: ExtensionService*
extension_service_;  // Weak.
Instead of having a direct dependency on ExtensionService, consider making it a
dependency on ExtensionServiceInterface. This might allow you to move the unit
tests of ExtensionSorting out into their own file, which would be nice since
extension_service_unittest is so large.

[csharp, 2012-03-15 17:21:19]

https://chromiumcodereview.appspot.com/9706017/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_sorting.cc (right):

https://chromiumcodereview.appspot.com/9706017/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_sorting.cc:290: DCHECK(!ext ||
ext->is_app());
On 2012/03/14 18:54:28, Aaron Boodman wrote:
> The extension system prefers CHECK to DCHECK.

Done.

https://chromiumcodereview.appspot.com/9706017/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_sorting.cc:292: if (ext)
On 2012/03/14 18:54:28, Aaron Boodman wrote:
> Move the DCHECK inside this branch and make it just DCHECK(ext->is_app()) ?

Done.

https://chromiumcodereview.appspot.com/9706017/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/extension_sorting.h (right):

https://chromiumcodereview.appspot.com/9706017/diff/1/chrome/browser/extensio...
chrome/browser/extensions/extension_sorting.h:142: ExtensionService*
extension_service_;  // Weak.
On 2012/03/14 18:54:28, Aaron Boodman wrote:
> Instead of having a direct dependency on ExtensionService, consider making it
a
> dependency on ExtensionServiceInterface. This might allow you to move the unit
> tests of ExtensionSorting out into their own file, which would be nice since
> extension_service_unittest is so large.

Ok, I've changed the dependency. I looked at the tests related to the ordinals
that are in extension_service_unittest and they are testing ExtensionService
functionality, so I don't think they should be moved.

[akalin, 2012-03-15 19:29:38]

LGTM

http://codereview.chromium.org/9706017/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_sorting.cc (right):

http://codereview.chromium.org/9706017/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_sorting.cc:385: DCHECK(!ext ||
ext->is_app());
CHECK also?  Make this consistent with the analogous block above?

May also want to decomp it into a helper function

[csharp, 2012-03-15 20:19:45]

https://chromiumcodereview.appspot.com/9706017/diff/4001/chrome/browser/exten...
File chrome/browser/extensions/extension_sorting.cc (right):

https://chromiumcodereview.appspot.com/9706017/diff/4001/chrome/browser/exten...
chrome/browser/extensions/extension_sorting.cc:385: DCHECK(!ext ||
ext->is_app());
On 2012/03/15 19:29:39, akalin wrote:
> CHECK also?  Make this consistent with the analogous block above?
> 
> May also want to decomp it into a helper function

Done.

[commit-bot: I haz the power, 2012-03-15 20:30:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/9706017/5002

[commit-bot: I haz the power, 2012-03-16 01:46:30]

Commit queue had an internal error.
Something went really wrong, probably a crash, a hickup or
simply the monkeys went out for diner.
The relevant dude was pinged already.

[commit-bot: I haz the power, 2012-03-16 01:47:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/9706017/5002

[commit-bot: I haz the power, 2012-03-16 04:24:35]

Try job failure for 9706017-5002 (retry) on mac_rel for steps "browser_tests,
unit_tests".
It's a second try, previously, steps "browser_tests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[akalin, 2012-03-21 00:14:29]

On 2012/03/16 04:24:35, I haz the power (commit-bot) wrote:
> Try job failure for 9706017-5002 (retry) on mac_rel for steps "browser_tests,
> unit_tests".
> It's a second try, previously, steps "browser_tests, unit_tests" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

Is this going to be checked in?

[commit-bot: I haz the power, 2012-03-21 12:57:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/9706017/5002

[csharp, 2012-03-21 12:58:15]

On 2012/03/21 00:14:29, akalin wrote:
> On 2012/03/16 04:24:35, I haz the power (commit-bot) wrote:
> > Try job failure for 9706017-5002 (retry) on mac_rel for steps
"browser_tests,
> > unit_tests".
> > It's a second try, previously, steps "browser_tests, unit_tests" failed.
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
> 
> Is this going to be checked in?

Yup, I'd seen a weird error on one of the mac try bots (looked related to this
change) and it's taken a while to get a try bot that wasn't having trouble on
the compile step (for other reasons, had to clobber the machine to get it work).

[commit-bot: I haz the power, 2012-03-21 14:15:49]

Change committed as 127953