Make ExtensionService use ExtensionSet.

Make ExtensionService use ExtensionSet.

Make ExtensionSet iterate like a set instead of a map.

BUG=104091
TEST=existing tests

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

[Aaron Boodman, 2011-12-01 08:27:12]

Sweet. LGTM

http://codereview.chromium.org/8733004/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (left):

http://codereview.chromium.org/8733004/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1682: if
(!IsExtensionEnabled(e->id())) {
Because all those extensions would be in disabled_extensions_ ?

http://codereview.chromium.org/8733004/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/8733004/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1883: // disabled, e.g. as in
ExtensionManagementTest.InstallRequiresConfirm.
heh

http://codereview.chromium.org/8733004/diff/2001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_set.cc (right):

http://codereview.chromium.org/8733004/diff/2001/chrome/common/extensions/ext...
chrome/common/extensions/extension_set.cc:20: bool ExtensionSet::empty() const {
I prefer is_empty() to avoid confusion with clear(). Though I can understand the
desire for consistency with STL.

http://codereview.chromium.org/8733004/diff/2001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_set.h (right):

http://codereview.chromium.org/8733004/diff/2001/chrome/common/extensions/ext...
chrome/common/extensions/extension_set.h:26: // it should iterate like a set
iterator).
Fair enough.

[Yoyo Zhou, 2011-12-05 19:34:09]

http://codereview.chromium.org/8733004/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_crash_recovery_browsertest.cc (right):

http://codereview.chromium.org/8733004/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_crash_recovery_browsertest.cc:356:
MAYBE_TwoExtensionsIgnoreFirst) {
I think this test is just wrong. It's meant to ignore extension 1 and reload 2,
but actually I'm pretty sure 1 gets reloaded.

http://codereview.chromium.org/8733004/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_service.cc (left):

http://codereview.chromium.org/8733004/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_service.cc:1682: if
(!IsExtensionEnabled(e->id())) {
On 2011/12/01 08:27:12, Aaron Boodman wrote:
> Because all those extensions would be in disabled_extensions_ ?

Yeah, all IsExtensionEnabled does is check that it's in extensions_ or
terminated_extensions_, so checking here is pointless.

Looks like I should revise the implementations of such functions since I'm
guessing we want to get rid of all the GetExtensionById* functions in favor of
ExtensionSet::GetByID.

http://codereview.chromium.org/8733004/diff/2001/chrome/common/extensions/ext...
File chrome/common/extensions/extension_set.cc (right):

http://codereview.chromium.org/8733004/diff/2001/chrome/common/extensions/ext...
chrome/common/extensions/extension_set.cc:20: bool ExtensionSet::empty() const {
On 2011/12/01 08:27:12, Aaron Boodman wrote:
> I prefer is_empty() to avoid confusion with clear(). Though I can understand
the
> desire for consistency with STL.

Done.

[Yoyo Zhou, 2011-12-05 19:35:54]

On Mon, Dec 5, 2011 at 11:34 AM,  <yoz@chromium.org> wrote:
>
>
http://codereview.chromium.org/8733004/diff/2001/chrome/browser/extensions/ex...
> File chrome/browser/extensions/extension_crash_recovery_browsertest.cc
> (right):
>
>
http://codereview.chromium.org/8733004/diff/2001/chrome/browser/extensions/ex...
> chrome/browser/extensions/extension_crash_recovery_browsertest.cc:356:
> MAYBE_TwoExtensionsIgnoreFirst) {
> I think this test is just wrong. It's meant to ignore extension 1 and
> reload 2, but actually I'm pretty sure 1 gets reloaded.

Sorry, ignore this comment -- I fixed this test last week.

[Yoyo Zhou, 2011-12-05 21:28:55]

Fred: can you please review for sync?

[akalin, 2011-12-05 22:26:58]

On 2011/12/05 21:28:55, Yoyo Zhou wrote:
> Fred: can you please review for sync?

sync stuff lgtm