Extensions installed by policy overrun previously installed extensions.

Extensions installed by policy overrun previously installed extensions.

BUG=86519
TEST=Extensions tests, should all work as before. The test described in the bug should now work as expected.


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

[Joao da Silva, 2011-08-10 11:13:16]

Hi Sam,

I'm taking this over from Gábor's initial work at
http://codereview.chromium.org/7237028. This version works as described in the
bug comments (http://code.google.com/p/chromium/issues/detail?id=86519#c10).

WDYT of this fix? Thanks for reviewing!

[Sam Kerner (Chrome), 2011-08-10 17:21:09]

Thanks for working on this.

http://codereview.chromium.org/7605001/diff/1001/chrome/browser/extensions/pe...
File chrome/browser/extensions/pending_extension_manager.cc (right):

http://codereview.chromium.org/7605001/diff/1001/chrome/browser/extensions/pe...
chrome/browser/extensions/pending_extension_manager.cc:71: Extension::Location
location, bool overwrite) {
There are two reasons |overwrite| is useful:

1) The new location has a higher priority than the old one.

2) The extension is already installed, and you want to avoid the LOG(DFATAL).

I would prefer that PendingExtensionManager methods enforce the invariant that
the higher-ranked location wins.  Instead of adding the overwrite parameter,
lets solve #1 by using Extension::GetHigherPriorityLocation() to see if an
existing record should be overwritten.  #2 can be solved by LOG(DFATAL)ing only
when the new location does not outrank the old location.

[Joao da Silva, 2011-08-11 10:03:23]

I've made the changes proposed, please have another look. Thanks for reviewing!

http://codereview.chromium.org/7605001/diff/1001/chrome/browser/extensions/pe...
File chrome/browser/extensions/pending_extension_manager.cc (right):

http://codereview.chromium.org/7605001/diff/1001/chrome/browser/extensions/pe...
chrome/browser/extensions/pending_extension_manager.cc:71: Extension::Location
location, bool overwrite) {
On 2011/08/10 17:21:09, Sam Kerner (Chrome) wrote:
> There are two reasons |overwrite| is useful:
> 
> 1) The new location has a higher priority than the old one.
> 
> 2) The extension is already installed, and you want to avoid the LOG(DFATAL).
> 
> I would prefer that PendingExtensionManager methods enforce the invariant that
> the higher-ranked location wins.  Instead of adding the overwrite parameter,
> lets solve #1 by using Extension::GetHigherPriorityLocation() to see if an
> existing record should be overwritten.  #2 can be solved by LOG(DFATAL)ing
only
> when the new location does not outrank the old location.

Done.

[Sam Kerner (Chrome), 2011-08-11 14:04:09]

LGTM.

Thanks for cleaning this up.

[commit-bot: I haz the power, 2011-08-11 17:55:04]

Change committed as 96420