Fix regression in showing bookmark star when an extension should override it

Fix regression in showing bookmark star when an extension should override it

https://codereview.chromium.org/505373002 introduced a regression by
implicitly changing an Update() call to an UpdatePageActions() call on
extensions being loaded/unloaded. This is mostly correct, but we lost the
check to make sure an extension didn't override the bookmark star. Correct this.
Also add a regression test.

BUG=410372
TBR=sky@chromium.org (minor TestBrowserWindow change)

Committed: https://crrev.com/5d9dfa036fe7ace92f35e34bfc2a156dbecbbb2a
Cr-Commit-Position: refs/heads/master@{#293737}

[Devlin, 2014-09-03 23:01:07]

Patchset #1 (id:1) has been deleted

[Devlin, 2014-09-03 23:01:50]

rdevlin.cronin@chromium.org changed reviewers:
+ avi@chromium.org, finnur@chromium.org, pkasting@chromium.org

[Peter Kasting, 2014-09-03 23:13:09]

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller.cc (right):

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:73: if (!browser)
Can this actually be NULL?

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:76: browser->window() ?
browser->window()->GetLocationBar() : NULL;
Likewise, can the window() or the location bar be NULL?

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/ui/locati...
File chrome/browser/ui/location_bar/location_bar_browsertest.cc (right):

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/ui/locati...
chrome/browser/ui/location_bar/location_bar_browsertest.cc:56: };
Nit: While here: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/ui/locati...
chrome/browser/ui/location_bar/location_bar_browsertest.cc:129: // Test that
installing an extension that overrides the bookmark star works.
Nit: works -> successfully hides the star

[Devlin, 2014-09-03 23:23:21]

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller.cc (right):

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:73: if (!browser)
On 2014/09/03 23:13:08, Peter Kasting wrote:
> Can this actually be NULL?

The comment above the method would imply so. [1] Unfortunately, it isn't clear
under which circumstances that could happen.  That said, a small random sampling
of the callers indicate that it is common practice to check the result before
using it.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:76: browser->window() ?
browser->window()->GetLocationBar() : NULL;
On 2014/09/03 23:13:08, Peter Kasting wrote:
> Likewise, can the window() or the location bar be NULL?

window() can be NULL if this is called before CreateBrowserWindow() completes
[1], and location bar can be NULL if the window doesn't have a toolbar (e.g., an
app window). [2]

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/ui/locati...
File chrome/browser/ui/location_bar/location_bar_browsertest.cc (right):

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/ui/locati...
chrome/browser/ui/location_bar/location_bar_browsertest.cc:56: };
On 2014/09/03 23:13:08, Peter Kasting wrote:
> Nit: While here: DISALLOW_COPY_AND_ASSIGN

Ah, good catch. Done.

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/ui/locati...
chrome/browser/ui/location_bar/location_bar_browsertest.cc:129: // Test that
installing an extension that overrides the bookmark star works.
On 2014/09/03 23:13:09, Peter Kasting wrote:
> Nit: works -> successfully hides the star

Done.

[Peter Kasting, 2014-09-03 23:41:04]

LGTM other than below

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller.cc (right):

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:73: if (!browser)
On 2014/09/03 23:23:20, Devlin wrote:
> On 2014/09/03 23:13:08, Peter Kasting wrote:
> > Can this actually be NULL?
> 
> The comment above the method would imply so. [1] Unfortunately, it isn't clear
> under which circumstances that could happen.  That said, a small random
sampling
> of the callers indicate that it is common practice to check the result before
> using it.

Ugh, I really hate copying NULL-checks because other people do them but it's not
clear if they're actually needed.

If this could happen, I'd expect it'd be sometime during startup or shutdown of
a WebContents.  It would be nice to know definitively if this is possible. 
Maybe Avi could comment on this, since if anyone would know this I'd think it'd
be him.

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:76: browser->window() ?
browser->window()->GetLocationBar() : NULL;
On 2014/09/03 23:23:20, Devlin wrote:
> On 2014/09/03 23:13:08, Peter Kasting wrote:
> > Likewise, can the window() or the location bar be NULL?
> 
> window() can be NULL if this is called before CreateBrowserWindow() completes
> [1], and location bar can be NULL if the window doesn't have a toolbar (e.g.,
an
> app window).

Cool, can you add brief comments noting that?

[Mike Wittman, 2014-09-03 23:57:44]

wittman@chromium.org changed reviewers:
+ wittman@chromium.org

[Mike Wittman, 2014-09-03 23:57:44]

https://codereview.chromium.org/540483002/diff/40001/chrome/browser/ui/locati...
File chrome/browser/ui/location_bar/location_bar_browsertest.cc (right):

https://codereview.chromium.org/540483002/diff/40001/chrome/browser/ui/locati...
chrome/browser/ui/location_bar/location_bar_browsertest.cc:146:
SetBoolean("remove_bookmark_shortcut", true));
nit: this property shouldn't be necessary

[Avi (use Gerrit), 2014-09-04 00:33:50]

The mac code LGTM, which is what I'm assuming you were asking.

[Avi (use Gerrit), 2014-09-04 00:47:08]

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller.cc (right):

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:73: if (!browser)
Of course chrome::FindBrowserWithWebContents can return NULL. WebContents are
all over the place, everywhere where you want to put web content in a rectangle.
If you ask it for the Browser for a WebContents that isn't a browser tab, you'll
get NULL.

And LocationBarController is owned by extensions::TabHelper, so the
web_contents_ is a browser tab.

But I think you're asking a deeper question: is it possible that a call to this
function can happen in a moment where the tab isn't actually inside any browser
window? And the answer is, I don't know. If you're changing this code, then you
know far better than me when this call can occur. The situation that comes to
mind is teardown, where the WebContents is being deleted, but I don't know if it
can happen at that point.

As for the other instances, I'm not sure if they're undeserved paranoia, or
legitimate responses to crashes. :(

[Finnur, 2014-09-04 10:07:55]

LGTM

[Devlin, 2014-09-04 23:32:07]

Patchset #3 (id:60001) has been deleted

[Devlin, 2014-09-04 23:48:47]

Patchset #3 (id:80001) has been deleted

[Devlin, 2014-09-05 20:15:16]

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/location_bar_controller.cc (right):

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:73: if (!browser)
On 2014/09/04 00:47:08, Avi wrote:
> Of course chrome::FindBrowserWithWebContents can return NULL. WebContents are
> all over the place, everywhere where you want to put web content in a
rectangle.
> If you ask it for the Browser for a WebContents that isn't a browser tab,
you'll
> get NULL.
> 
> And LocationBarController is owned by extensions::TabHelper, so the
> web_contents_ is a browser tab.
> 
> But I think you're asking a deeper question: is it possible that a call to
this
> function can happen in a moment where the tab isn't actually inside any
browser
> window? And the answer is, I don't know. If you're changing this code, then
you
> know far better than me when this call can occur. The situation that comes to
> mind is teardown, where the WebContents is being deleted, but I don't know if
it
> can happen at that point.
> 
> As for the other instances, I'm not sure if they're undeserved paranoia, or
> legitimate responses to crashes. :(

Since a LocationBarController is attached as part of the ExtensionTabHelper,
which, in turn, is attached to most web contents (at various points of their
creation), it seems that any assumption we make here, even if temporarily
correct, would be fragile.  I've added a comment noting that the browser could
legitimately be NULL but that, ideally, we'd be able to check it.

https://codereview.chromium.org/540483002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/location_bar_controller.cc:76: browser->window() ?
browser->window()->GetLocationBar() : NULL;
On 2014/09/03 23:41:03, Peter Kasting wrote:
> On 2014/09/03 23:23:20, Devlin wrote:
> > On 2014/09/03 23:13:08, Peter Kasting wrote:
> > > Likewise, can the window() or the location bar be NULL?
> > 
> > window() can be NULL if this is called before CreateBrowserWindow()
completes
> > [1], and location bar can be NULL if the window doesn't have a toolbar
(e.g.,
> an
> > app window).
> 
> Cool, can you add brief comments noting that?

Done.

https://codereview.chromium.org/540483002/diff/40001/chrome/browser/ui/locati...
File chrome/browser/ui/location_bar/location_bar_browsertest.cc (right):

https://codereview.chromium.org/540483002/diff/40001/chrome/browser/ui/locati...
chrome/browser/ui/location_bar/location_bar_browsertest.cc:146:
SetBoolean("remove_bookmark_shortcut", true));
On 2014/09/03 23:57:44, Mike Wittman wrote:
> nit: this property shouldn't be necessary

True enough. Removed.

[Devlin, 2014-09-05 20:16:03]

rdevlin.cronin@chromium.org changed reviewers:
+ sky@chromium.org

[Devlin, 2014-09-05 20:16:08]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-09-05 20:47:02]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/540483002/...

[commit-bot: I haz the power, 2014-09-05 21:36:20]

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

[commit-bot: I haz the power, 2014-09-05 21:36:21]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)

[Devlin, 2014-09-05 22:06:50]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-09-05 22:07:49]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/540483002/...

[commit-bot: I haz the power, 2014-09-06 04:10:26]

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

[commit-bot: I haz the power, 2014-09-06 04:10:27]

Exceeded time limit waiting for builds to trigger.

[Devlin, 2014-09-08 16:11:10]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-09-08 16:12:22]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/540483002/...

[commit-bot: I haz the power, 2014-09-08 16:14:18]

Committed patchset #4 (id:120001) as cec0875709fe4cf160c48556537443fb8bdb278b

[commit-bot: I haz the power, 2014-09-10 03:46:40]

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