Correct the flag: is_extension_upgrade, which denote whether the extension is being upgrading.

Correct the flag: is_extension_upgrade, which denote whether the extension is being upgrading.

In the function BrowserActionsContainer::BrowserActionAdded of chrome/browser/ui/views/browser_actions_container.cc, if the extension is marked as upgrading, it will stop enlarge the container if it was already at maximum size. The upgrading extension didn't remove the browser action, so it works correctly. However, when a extension is crashed (The browser action will be removed if it has) and reloaded (will  add the browser action icon back if it has), it shouldn't be marked as upgrading. The bug is introduced by http://src.chromium.org/viewvc/chrome?revision=196634&view=revision. 

BUG=261996
TEST=On Windows, reload a crashed extension shouldn't resize the browser action container of browser. For more detail, please refer to the bug.

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

[zhchbin, 2013-07-27 13:08:55]

Please review.

[asargent_no_longer_on_chrome, 2013-07-29 23:09:01]

Do you have the correct bug number in the BUG= field of the description here?
I'm a little surprised this code has anything to do with the problem described
in crbug.com/261996.

https://codereview.chromium.org/20909002/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/20909002/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:2007: }
I think this block would read a little better written as follows:


bool is_extension_upgrade = false;
const Extension* old = GetInstalledExtension(extension->id())) {
if (old) {
  DCHECK_NE(extension, old);
  is_extension_upgrade =
    (extension->version()->CompareTo(*(old->version())) > 0);
  // Other than for unpacked extensions, CrxInstaller should have guaranteed
  // that we aren't downgrading.
  if (!Manifest::IsUnpackedLocation(extension->location())) {
    CHECK(is_extension_upgrade);
  }
}

[zhchbin, 2013-07-30 01:46:41]

I have also updated the issue description for better understanding. Please take
a look.

https://codereview.chromium.org/20909002/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/20909002/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/extension_service.cc:2007: }
On 2013/07/29 23:09:01, Antony Sargent wrote:
> I think this block would read a little better written as follows:
> 
> 
> bool is_extension_upgrade = false;
> const Extension* old = GetInstalledExtension(extension->id())) {
> if (old) {
>   DCHECK_NE(extension, old);
>   is_extension_upgrade =
>     (extension->version()->CompareTo(*(old->version())) > 0);
>   // Other than for unpacked extensions, CrxInstaller should have guaranteed
>   // that we aren't downgrading.
>   if (!Manifest::IsUnpackedLocation(extension->location())) {
>     CHECK(is_extension_upgrade);
>   }
> }
> 

Note that here is *CHECK_GE*. And is_extension_upgrade should be true only when
the result of CompareTo greater than 0. The function AddExtension will be called
when reloading the extension, there are many situation need to involve it. For
example, extension upgrading, reload a crashed extension, reload an unpacked
app/extension, etc.

[zhchbin, 2013-07-30 01:53:56]

https://codereview.chromium.org/20909002/diff/11001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (left):

https://codereview.chromium.org/20909002/diff/11001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2001: DCHECK_NE(extension, old);
As my comment, this DCHECK_NE make no sense because the AddExtension will be
called in many code path. This line of code is copied without check before it's
committed. So I also remove it. https://chromiumcodereview.appspot.com/14490002

[zhchbin, 2013-07-31 14:05:42]

Ping for review.

[dhnishi (use Chromium), 2013-07-31 19:09:21]

One unintended side-effect of a crashed extension being reloaded when it is not
marked as being upgraded is that it can accidentally resize the browser actions
container.

For example, I have 3 browser actions. Two of them are showing, the third is in
the dropdown. The third one crashes and is reloaded. Because the browser bar is
full and we're not in the middle of upgrading, the browser bar is resized and
now all 3 are showing. Here is a link to the check where that happens:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

[zhchbin, 2013-08-01 00:49:51]

On 2013/07/31 19:09:21, Daniel Nishi wrote:
> One unintended side-effect of a crashed extension being reloaded when it is
not
> marked as being upgraded is that it can accidentally resize the browser
actions
> container.
> 
> For example, I have 3 browser actions. Two of them are showing, the third is
in
> the dropdown. The third one crashes and is reloaded. Because the browser bar
is
> full and we're not in the middle of upgrading, the browser bar is resized and
> now all 3 are showing. Here is a link to the check where that happens:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

Thanks very much for the exact explanation.

[asargent_no_longer_on_chrome, 2013-08-01 17:50:51]

lgtm, thanks for the patch and apologies for review latency

[dhnishi (use Chromium), 2013-08-01 17:55:50]

It looks like this patch removes the possibility of unpacked extensions being
reloaded being marked as being upgraded, even if they have had files changed. Is
having upgraded be a purely packed extension flag the intended functionality?

[asargent_no_longer_on_chrome, 2013-08-01 18:10:05]

oops, good catch DHNishi - I misread the code. The current patch does not lgtm -
please go back to the version I had suggested before, where you set the value of
is_extension_upgrade *outside* of the "if (<not unpacked>)" body.

[zhchbin, 2013-08-02 02:02:09]

On 2013/08/01 18:10:05, Antony Sargent wrote:
> oops, good catch DHNishi - I misread the code. The current patch does not lgtm
-
> please go back to the version I had suggested before, where you set the value
of
> is_extension_upgrade *outside* of the "if (<not unpacked>)" body.

PTAL

I didn't change the code just as your comment without any thinking. Though I
didn't think the "unpacked extensions being reloaded" can be marked as
"is_extension_upgrade = true", but in order to keep the change more minimal and
let you easy, *I have updated as @Daniel Nishi's suggestions and yours*. 

And AFAICS mark "is_extension_upgrade" as true when unpacked extensions being
reloaded (with version increased) will not break anything so far. Just it didn't
meet the meaing of the "is_extension_upgrade" literally, which I think it's used
for the extensions that can be updated.

Thanks for your time.

[asargent_no_longer_on_chrome, 2013-08-02 20:23:23]

lgtm

[dhnishi (use Chromium), 2013-08-02 20:32:35]

lgtm

[commit-bot: I haz the power, 2013-08-02 23:53:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhchbin@gmail.com/20909002/21001

[commit-bot: I haz the power, 2013-08-03 00:40:16]

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

[zhchbin, 2013-08-03 08:21:45]

On 2013/08/03 00:40:16, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_aura for step(s) unit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

Please review again.
Seems I have misled by the name "is_extension_upgrad", the
"CheckPermissionsIncrease" should check the installed extensions. Now it
shouldn't break the unit_tests.

[zhchbin, 2013-08-06 15:04:12]

On 2013/08/03 08:21:45, zhchbin wrote:
> On 2013/08/03 00:40:16, I haz the power (commit-bot) wrote:
> > Retried try job too often on linux_aura for step(s) unit_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
> 
> Please review again.
> Seems I have misled by the name "is_extension_upgrad", the
> "CheckPermissionsIncrease" should check the installed extensions. Now it
> shouldn't break the unit_tests.

Ping again. Sorry for the trouble.

[commit-bot: I haz the power, 2013-08-07 02:56:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhchbin@gmail.com/20909002/53001

[commit-bot: I haz the power, 2013-08-07 04:04:59]

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

[asargent_no_longer_on_chrome, 2013-08-07 22:14:21]

lgtm

[commit-bot: I haz the power, 2013-08-08 00:20:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zhchbin@gmail.com/20909002/53001

[commit-bot: I haz the power, 2013-08-08 17:23:22]

Change committed as 216407