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

chrome/browser/extensions/location_bar_controller.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/location_bar_controller.h"
 
 #include "chrome/browser/extensions/active_script_controller.h"
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/location_bar/location_bar.h"
+#include "chrome/common/extensions/manifest_handlers/ui_overrides_handler.h"
 #include "content/public/browser/web_contents.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/feature_switch.h"
 
 namespace extensions {
 
 LocationBarController::LocationBarController(
     content::WebContents* web_contents)
     : web_contents_(web_contents),
       browser_context_(web_contents->GetBrowserContext()),
@@ skipping 33 matching lines @@
 }
 
 void LocationBarController::OnExtensionLoaded(
     content::BrowserContext* browser_context,
     const Extension* extension) {
   if (action_manager_->GetPageAction(*extension) ||
       active_script_controller_->GetActionForExtension(extension)) {
     ExtensionActionAPI::Get(browser_context)->
         NotifyPageActionsChanged(web_contents_);
   }
+
+  // We might also need to update the location bar if the extension can remove
+  // the bookmark star.
+  if (UIOverrides::RemovesBookmarkButton(extension)) {
+    Browser* browser = chrome::FindBrowserWithWebContents(web_contents_);
+    if (!browser)

[Peter Kasting, 2014/09/03 23:13:08]

Can this actually be NULL?

[Devlin, 2014/09/03 23:23:20]

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/...

[Peter Kasting, 2014/09/03 23:41:03]

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.

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

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. :(

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

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.

+      return;
+    LocationBar* location_bar =
+        browser->window() ? browser->window()->GetLocationBar() : NULL;

[Peter Kasting, 2014/09/03 23:13:08]

Likewise, can the window() or the location bar be NULL?

[Devlin, 2014/09/03 23:23:20]

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/...

[Peter Kasting, 2014/09/03 23:41:03]

Cool, can you add brief comments noting that?

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

Done.

+    if (!location_bar)
+      return;
+    location_bar->UpdateBookmarkStarVisibility();
+  }
 }
 
 void LocationBarController::OnExtensionUnloaded(
     content::BrowserContext* browser_context,
     const Extension* extension,
     UnloadedExtensionInfo::Reason reason) {
   if (action_manager_->GetPageAction(*extension)) {
     ExtensionActionAPI::Get(browser_context)->
         NotifyPageActionsChanged(web_contents_);
   }
 }
 
 }  // namespace extensions