Moved theme related state from BrowserInstantController to InstantService.

Moved theme related state from BrowserInstantController to InstantService.

Created InstantServiceObserver to define an observer interface for InstantService. Registered InstantController as an InstantServiceObserver to get theme change events. We no longer send theme changed events on tab switch.

BUG=225760
TEST=Added an interactive ui test.

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

[kmadhusu, 2013-06-14 19:07:59]

samarth@: This CL will fix the bug stated in crbug.com/225760. Need to change
server side code to fix the bug stated in comment#2. PTAL.

Thanks.

[samarth, 2013-06-19 00:14:36]

Change works as expected. No major comments below.

Thanks!
Samarth

https://codereview.chromium.org/16413002/diff/84001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/16413002/diff/84001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:151: if (!theme_info_.IsValid())
Instead of adding a IsValid here, just keep a boolean like we did in
BrowserInstantController. (see my comments in instant_types.h: I don't really
want ThemeInfo to become too complicated)

https://codereview.chromium.org/16413002/diff/84001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:250: if (!theme_service) {
I slightly preferred the way it was written in BrowserInstantController: with
the if around the block of code below and a single call to FOR_EACH_OBSERVER at
the end. But up to you.

https://codereview.chromium.org/16413002/diff/84001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:312: default:
This is new. Why?

https://codereview.chromium.org/16413002/diff/84001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.h (right):

https://codereview.chromium.org/16413002/diff/84001/chrome/browser/search/ins...
chrome/browser/search/instant_service.h:74: // Invoked by |instant_| to update
theme information for NTP.
The "by |instant_|" part doesn't make sense here.

https://codereview.chromium.org/16413002/diff/84001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/16413002/diff/84001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:270: instant_.UpdateThemeInfo();
Is this really necessary? Shouldn't the page be getting the theme from other
sources (e.g. either as InstantNTP or InstantTab)?

I tried commenting it out and didn't notice any issues. But perhaps I'm missing
something.

https://codereview.chromium.org/16413002/diff/84001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/16413002/diff/84001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1092: if (!extended_enabled() ||
!search_mode_.is_ntp())
Don't check for search_mode_ here. We'd want to update ntp_ with an updated
theme, even if we're on some random page.

https://codereview.chromium.org/16413002/diff/84001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_controller.cc:1200: void
InstantController::UpdateThemeInfo() {
If UpdateThemeInfo doesn't need to be called from BrowserInstantController, you
can make it private. Or even better, just make a helper to get InstantService
and inline the calls to UpdateThemeInfo

https://codereview.chromium.org/16413002/diff/84001/chrome/common/instant_typ...
File chrome/common/instant_types.cc (right):

https://codereview.chromium.org/16413002/diff/84001/chrome/common/instant_typ...
chrome/common/instant_types.cc:51: void ThemeBackgroundInfo::Initialize() {
I don't like having a Initialize method on a struct. This should just be the
default constructor.

[kmadhusu, 2013-06-19 02:27:23]

Addressed comments. PTAL.

Thanks.

https://chromiumcodereview.appspot.com/16413002/diff/84001/chrome/browser/sea...
File chrome/browser/search/instant_service.cc (right):

https://chromiumcodereview.appspot.com/16413002/diff/84001/chrome/browser/sea...
chrome/browser/search/instant_service.cc:151: if (!theme_info_.IsValid())
On 2013/06/19 00:14:36, samarth wrote:
> Instead of adding a IsValid here, just keep a boolean like we did in
> BrowserInstantController. (see my comments in instant_types.h: I don't really
> want ThemeInfo to become too complicated)

I want to invalidate ThemeBackgroundInfo in SearchTabHelper. Instead of
introducing an invalidate behavior in struct, I prefer to construct the object
with invalid values and use Initialize() to initialize the struct member
variables with valid values. I wanted to get rid of the boolean variable which
we used in BrowserInstantController.

If you don't prefer Initialize() and IsValid() on struct, Can I modify
InstantService and SearchTabHelper to store a scoped_ptr<ThemeBackgroundInfo>?

https://chromiumcodereview.appspot.com/16413002/diff/84001/chrome/browser/sea...
chrome/browser/search/instant_service.cc:250: if (!theme_service) {
On 2013/06/19 00:14:36, samarth wrote:
> I slightly preferred the way it was written in BrowserInstantController: with
> the if around the block of code below and a single call to FOR_EACH_OBSERVER
at
> the end. But up to you.

I would like to leave it as it is. I don't want to define the entire function
within an if-block.

https://chromiumcodereview.appspot.com/16413002/diff/84001/chrome/browser/sea...
chrome/browser/search/instant_service.cc:312: default:
On 2013/06/19 00:14:36, samarth wrote:
> This is new. Why?

Good catch. Removed.

https://chromiumcodereview.appspot.com/16413002/diff/84001/chrome/browser/sea...
File chrome/browser/search/instant_service.h (right):

https://chromiumcodereview.appspot.com/16413002/diff/84001/chrome/browser/sea...
chrome/browser/search/instant_service.h:74: // Invoked by |instant_| to update
theme information for NTP.
On 2013/06/19 00:14:36, samarth wrote:
> The "by |instant_|" part doesn't make sense here.

Fixed.

https://chromiumcodereview.appspot.com/16413002/diff/84001/chrome/browser/ui/...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://chromiumcodereview.appspot.com/16413002/diff/84001/chrome/browser/ui/...
chrome/browser/ui/browser_instant_controller.cc:270: instant_.UpdateThemeInfo();
On 2013/06/19 00:14:36, samarth wrote:
> Is this really necessary? Shouldn't the page be getting the theme from other
> sources (e.g. either as InstantNTP or InstantTab)?
> 
> I tried commenting it out and didn't notice any issues. But perhaps I'm
missing
> something.

You are right. It is not required. Removed.

https://chromiumcodereview.appspot.com/16413002/diff/84001/chrome/browser/ui/...
File chrome/browser/ui/search/instant_controller.cc (right):

https://chromiumcodereview.appspot.com/16413002/diff/84001/chrome/browser/ui/...
chrome/browser/ui/search/instant_controller.cc:1092: if (!extended_enabled() ||
!search_mode_.is_ntp())
On 2013/06/19 00:14:36, samarth wrote:
> Don't check for search_mode_ here. We'd want to update ntp_ with an updated
> theme, even if we're on some random page.

Done.

https://chromiumcodereview.appspot.com/16413002/diff/84001/chrome/browser/ui/...
chrome/browser/ui/search/instant_controller.cc:1200: void
InstantController::UpdateThemeInfo() {
On 2013/06/19 00:14:36, samarth wrote:
> If UpdateThemeInfo doesn't need to be called from BrowserInstantController,
you
> can make it private. Or even better, just make a helper to get InstantService
> and inline the calls to UpdateThemeInfo

Done.

https://chromiumcodereview.appspot.com/16413002/diff/84001/chrome/common/inst...
File chrome/common/instant_types.cc (right):

https://chromiumcodereview.appspot.com/16413002/diff/84001/chrome/common/inst...
chrome/common/instant_types.cc:51: void ThemeBackgroundInfo::Initialize() {
On 2013/06/19 00:14:36, samarth wrote:
> I don't like having a Initialize method on a struct. This should just be the
> default constructor.

SearchTabHelper tracks |last_known_theme_info_|. Whenever a navigation entry is
committed, I want to invalidate |last_known_theme_info_|. I don't want to
invalidate the member values in SearchTabHelper.

Instead of defining a behavior to invalidate the member variables, I thought it
would be better if the default constructor initializes the member variables with
invalid values and then the caller can initialize the object using Initialize()
function. As per Google C++ style guide, a struct can have Initialize() function
(Ref:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Struct...).


Do you prefer to have an Invalidate() on a struct?

[samarth, 2013-06-19 05:34:53]

https://codereview.chromium.org/16413002/diff/84001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/16413002/diff/84001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:151: if (!theme_info_.IsValid())
On 2013/06/19 02:27:24, kmadhusu wrote:
> On 2013/06/19 00:14:36, samarth wrote:
> > Instead of adding a IsValid here, just keep a boolean like we did in
> > BrowserInstantController. (see my comments in instant_types.h: I don't
really
> > want ThemeInfo to become too complicated)
> 
> I want to invalidate ThemeBackgroundInfo in SearchTabHelper. Instead of
> introducing an invalidate behavior in struct, I prefer to construct the object
> with invalid values and use Initialize() to initialize the struct member
> variables with valid values. I wanted to get rid of the boolean variable which
> we used in BrowserInstantController.
> 
> If you don't prefer Initialize() and IsValid() on struct, Can I modify
> InstantService and SearchTabHelper to store a scoped_ptr<ThemeBackgroundInfo>?

That makes sense but I'm still not a fan of having to call Initialize() each
time we create one of these.  I'd prefer using a scoped_ptr or a bool.

https://codereview.chromium.org/16413002/diff/114001/chrome/browser/search/in...
File chrome/browser/search/instant_service.h (right):

https://codereview.chromium.org/16413002/diff/114001/chrome/browser/search/in...
chrome/browser/search/instant_service.h:75: void UpdateThemeInfo();
TODO: calling this from InstantController really shouldn't be necessary.  We
should investigate if it is and why (but this can wait).

https://codereview.chromium.org/16413002/diff/114001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/16413002/diff/114001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.cc:1196: 
Stray newline

https://codereview.chromium.org/16413002/diff/114001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.cc:1898: InstantService*
InstantController::GetInstantService() const {
Eh, seeing this, it's arguable whether this function really adds anything.  Up
to you if you want to keep it.

https://codereview.chromium.org/16413002/diff/114001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_controller.h (right):

https://codereview.chromium.org/16413002/diff/114001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.h:199: // Overridden from
InstantServiceObserver:
This can be private I think.

https://codereview.chromium.org/16413002/diff/114001/chrome/renderer/searchbo...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/16413002/diff/114001/chrome/renderer/searchbo...
chrome/renderer/searchbox/searchbox_extension.cc:993: default:
Again, any reason to add this as part of this change?

[kmadhusu, 2013-06-19 17:43:29]

Addressed comments. PTAL.

Thanks.

https://codereview.chromium.org/16413002/diff/84001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/16413002/diff/84001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:151: if (!theme_info_.IsValid())
On 2013/06/19 05:34:54, samarth wrote:
> On 2013/06/19 02:27:24, kmadhusu wrote:
> > On 2013/06/19 00:14:36, samarth wrote:
> > > Instead of adding a IsValid here, just keep a boolean like we did in
> > > BrowserInstantController. (see my comments in instant_types.h: I don't
> really
> > > want ThemeInfo to become too complicated)
> > 
> > I want to invalidate ThemeBackgroundInfo in SearchTabHelper. Instead of
> > introducing an invalidate behavior in struct, I prefer to construct the
object
> > with invalid values and use Initialize() to initialize the struct member
> > variables with valid values. I wanted to get rid of the boolean variable
which
> > we used in BrowserInstantController.
> > 
> > If you don't prefer Initialize() and IsValid() on struct, Can I modify
> > InstantService and SearchTabHelper to store a
scoped_ptr<ThemeBackgroundInfo>?
> 
> That makes sense but I'm still not a fan of having to call Initialize() each
> time we create one of these.  I'd prefer using a scoped_ptr or a bool.

Done. Used scoped_ptr.

https://codereview.chromium.org/16413002/diff/114001/chrome/browser/search/in...
File chrome/browser/search/instant_service.h (right):

https://codereview.chromium.org/16413002/diff/114001/chrome/browser/search/in...
chrome/browser/search/instant_service.h:75: void UpdateThemeInfo();
On 2013/06/19 05:34:54, samarth wrote:
> TODO: calling this from InstantController really shouldn't be necessary.  We
> should investigate if it is and why (but this can wait).

Added TODO.

https://codereview.chromium.org/16413002/diff/114001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/16413002/diff/114001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.cc:1196: 
On 2013/06/19 05:34:54, samarth wrote:
> Stray newline

Removed.

https://codereview.chromium.org/16413002/diff/114001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.cc:1898: InstantService*
InstantController::GetInstantService() const {
On 2013/06/19 05:34:54, samarth wrote:
> Eh, seeing this, it's arguable whether this function really adds anything.  Up
> to you if you want to keep it.

Let's keep it.

https://codereview.chromium.org/16413002/diff/114001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_controller.h (right):

https://codereview.chromium.org/16413002/diff/114001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.h:199: // Overridden from
InstantServiceObserver:
On 2013/06/19 05:34:54, samarth wrote:
> This can be private I think.

Done.

https://codereview.chromium.org/16413002/diff/114001/chrome/renderer/searchbo...
File chrome/renderer/searchbox/searchbox_extension.cc (right):

https://codereview.chromium.org/16413002/diff/114001/chrome/renderer/searchbo...
chrome/renderer/searchbox/searchbox_extension.cc:993: default:
On 2013/06/19 05:34:54, samarth wrote:
> Again, any reason to add this as part of this change?

Removed. Reverted ThemeBackgroundInfo changes.

[samarth, 2013-06-19 18:30:02]

https://codereview.chromium.org/16413002/diff/127001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_tab_helper.h (right):

https://codereview.chromium.org/16413002/diff/127001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper.h:99: scoped_ptr<ThemeBackgroundInfo>
last_known_theme_info_;
As discussed offline, let's track this in the renderer. So we'll always send an
IPC from InstantController in ThemeInfoChanged(). The renderer (searchbox.cc)
should then make sure we don't send duplicate events.

If this works, we can make the same change for most visited.

[kmadhusu, 2013-06-19 21:31:01]

PTAL. Thanks.

https://codereview.chromium.org/16413002/diff/127001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_tab_helper.h (right):

https://codereview.chromium.org/16413002/diff/127001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper.h:99: scoped_ptr<ThemeBackgroundInfo>
last_known_theme_info_;
On 2013/06/19 18:30:02, samarth wrote:
> As discussed offline, let's track this in the renderer. So we'll always send
an
> IPC from InstantController in ThemeInfoChanged(). The renderer (searchbox.cc)
> should then make sure we don't send duplicate events.
> 
> If this works, we can make the same change for most visited.

Done.

[samarth, 2013-06-19 22:37:15]

lgtm

https://codereview.chromium.org/16413002/diff/152001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_controller.cc (left):

https://codereview.chromium.org/16413002/diff/152001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.cc:1087: void
InstantController::ThemeChanged(const ThemeBackgroundInfo& theme_info) {
Just undo this diff?

[kmadhusu, 2013-06-20 01:21:26]

https://codereview.chromium.org/16413002/diff/152001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_controller.cc (left):

https://codereview.chromium.org/16413002/diff/152001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.cc:1087: void
InstantController::ThemeChanged(const ThemeBackgroundInfo& theme_info) {
On 2013/06/19 22:37:15, samarth wrote:
> Just undo this diff?

Done. I was trying to match the function declaration and definition order. But,
this class definitions are already mixed. I can do this clean up in a separate
CL.

[commit-bot: I haz the power, 2013-06-20 17:23:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/16413002/119004

[commit-bot: I haz the power, 2013-06-20 19:05:20]

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

[commit-bot: I haz the power, 2013-06-20 19:07:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/16413002/119004

[commit-bot: I haz the power, 2013-06-20 21:31:17]

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

[commit-bot: I haz the power, 2013-06-20 21:39:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/16413002/119004

[commit-bot: I haz the power, 2013-06-20 23:15:08]

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

[kmadhusu, 2013-06-21 02:22:18]

BrowserActionApiTest.IncognitoBasic and
BrowserTest.DisableMenuItemsWhenIncognitoIsForced test were failing because the
incognito profile was destroyed before BrowserInstantController. Made changes to
"Browser" destructor to fix this ordering.

PTAL. 
Thanks.

[samarth, 2013-06-21 02:28:38]

+Peter for OWNERS review for c/b/ui/browser.cc

[Peter Kasting, 2013-06-21 02:33:29]

https://codereview.chromium.org/16413002/diff/185002/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/16413002/diff/185002/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:487: instant_controller_.reset();
Check with erg/rlp on whether there's a better way to indicate this dependency
within your class itself.  (Maybe we should make the instant controller a
profile-keyed service or something.)

[kmadhusu, 2013-06-21 02:49:39]

https://codereview.chromium.org/16413002/diff/185002/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/16413002/diff/185002/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:487: instant_controller_.reset();
rlp@: Is there any better way to indicate this dependency?

[rpetterson, 2013-06-21 06:19:05]

https://codereview.chromium.org/16413002/diff/185002/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/16413002/diff/185002/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:487: instant_controller_.reset();
On 2013/06/21 02:49:39, kmadhusu wrote:
> rlp@: Is there any better way to indicate this dependency?

Are BrowserInstantControllers per profile or per browser? If it's the former,
then yes, it should be a browser_context keyed service. If not, then this is
probably the appropriate place for this.

[kmadhusu, 2013-06-21 15:45:45]

rpetterson@: Thanks.

https://codereview.chromium.org/16413002/diff/185002/chrome/browser/ui/browse...
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/16413002/diff/185002/chrome/browser/ui/browse...
chrome/browser/ui/browser.cc:487: instant_controller_.reset();
On 2013/06/21 06:19:06, rpetterson wrote:
> On 2013/06/21 02:49:39, kmadhusu wrote:
> > rlp@: Is there any better way to indicate this dependency?
> 
> Are BrowserInstantControllers per profile or per browser? If it's the former,
> then yes, it should be a browser_context keyed service. If not, then this is
> probably the appropriate place for this.

BrowserInstantController is instantiated per browser and not per profile.

[kmadhusu, 2013-06-21 15:53:57]

During our offline conversation, pkasting@ mentioned that he is not the right
reviewer for the browser.cc changes.

sky@: Can you please do an OWNER's check for browser.cc changes? 

Thanks.

[sky, 2013-06-21 16:34:46]

browser.cc LGTM

[commit-bot: I haz the power, 2013-06-21 16:36:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/16413002/198001

[commit-bot: I haz the power, 2013-06-21 20:44:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/16413002/210001

[commit-bot: I haz the power, 2013-06-22 02:29:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/16413002/210001

[commit-bot: I haz the power, 2013-06-22 02:29:48]

Failed to apply patch for chrome/browser/ui/search/instant_controller.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/search/instant_controller.cc
  Hunk #2 succeeded at 1096 with fuzz 1 (offset 2 lines).
  Hunk #3 succeeded at 1137 (offset 2 lines).
  Hunk #4 succeeded at 1162 (offset 2 lines).
  Hunk #5 succeeded at 1195 (offset 2 lines).
  Hunk #6 succeeded at 1204 (offset 2 lines).
  Hunk #7 succeeded at 1212 (offset 2 lines).
  Hunk #8 succeeded at 1243 (offset 2 lines).
  Hunk #9 FAILED at 1686.
  Hunk #10 succeeded at 1927 (offset 2 lines).
  1 out of 10 hunks FAILED -- saving rejects to file
chrome/browser/ui/search/instant_controller.cc.rej

Patch:       chrome/browser/ui/search/instant_controller.cc
Index: chrome/browser/ui/search/instant_controller.cc
diff --git a/chrome/browser/ui/search/instant_controller.cc
b/chrome/browser/ui/search/instant_controller.cc
index
49e0bf46e62418de8f10a172c5ef19521e4e3355..5a88d2b8f516b0cbc738cb2e94c64afc8f82e6df
100644
--- a/chrome/browser/ui/search/instant_controller.cc
+++ b/chrome/browser/ui/search/instant_controller.cc
@@ -274,11 +274,17 @@
InstantController::InstantController(BrowserInstantController* browser,
   // InstantService sets up profile-level facilities such as the ThemeSource
for
   // the NTP.
   // However, in some tests, browser_ may be null.
-  if (browser_)
-    InstantServiceFactory::GetForProfile(browser_->profile());
+  if (browser_) {
+    InstantService* instant_service = GetInstantService();
+    instant_service->AddObserver(this);
+  }
 }
 
 InstantController::~InstantController() {
+  if (browser_) {
+    InstantService* instant_service = GetInstantService();
+    instant_service->RemoveObserver(this);
+  }
 }
 
 void InstantController::OnAutocompleteStart() {
@@ -1088,7 +1094,8 @@ void InstantController::SetInstantEnabled(bool
instant_enabled,
     instant_tab_->SetDisplayInstantResults(instant_enabled_);
 }
 
-void InstantController::ThemeChanged(const ThemeBackgroundInfo& theme_info) {
+void InstantController::ThemeInfoChanged(
+    const ThemeBackgroundInfo& theme_info) {
   if (!extended_enabled())
     return;
 
@@ -1128,8 +1135,7 @@ void InstantController::ReloadOverlayIfStale() {
 void InstantController::OverlayLoadCompletedMainFrame() {
   if (!overlay_ || overlay_->supports_instant())
     return;
-  InstantService* instant_service =
-      InstantServiceFactory::GetForProfile(browser_->profile());
+  InstantService* instant_service = GetInstantService();
   content::WebContents* contents = overlay_->contents();
   DCHECK(contents);
   if (instant_service->IsInstantProcess(
@@ -1154,8 +1160,7 @@ void InstantController::ClearDebugEvents() {
 }
 
 void InstantController::UpdateMostVisitedItems() {
-  InstantService* instant_service =
-      InstantServiceFactory::GetForProfile(profile());
+  InstantService* instant_service = GetInstantService();
   if (!instant_service)
     return;
 
@@ -1188,8 +1193,7 @@ void InstantController::UpdateMostVisitedItems() {
 
 void InstantController::DeleteMostVisitedItem(const GURL& url) {
   DCHECK(!url.is_empty());
-  InstantService* instant_service =
-      InstantServiceFactory::GetForProfile(profile());
+  InstantService* instant_service = GetInstantService();
   if (!instant_service)
     return;
 
@@ -1198,8 +1202,7 @@ void InstantController::DeleteMostVisitedItem(const GURL&
url) {
 
 void InstantController::UndoMostVisitedDeletion(const GURL& url) {
   DCHECK(!url.is_empty());
-  InstantService* instant_service =
-      InstantServiceFactory::GetForProfile(profile());
+  InstantService* instant_service = GetInstantService();
   if (!instant_service)
     return;
 
@@ -1207,8 +1210,7 @@ void InstantController::UndoMostVisitedDeletion(const
GURL& url) {
 }
 
 void InstantController::UndoAllMostVisitedDeletions() {
-  InstantService* instant_service =
-      InstantServiceFactory::GetForProfile(profile());
+  InstantService* instant_service = GetInstantService();
   if (!instant_service)
     return;
 
@@ -1239,7 +1241,9 @@ void InstantController::InstantPageRenderViewCreated(
     return;
 
   // Update theme info so that the page picks it up.
-  browser_->UpdateThemeInfo();
+  InstantService* instant_service = GetInstantService();
+  if (instant_service)
+    instant_service->UpdateThemeInfo();
 
   // Ensure the searchbox API has the correct initial state.
   if (IsContentsFrom(overlay(), contents)) {
@@ -1682,7 +1686,11 @@ void InstantController::ResetInstantTab() {
 
 void InstantController::UpdateInfoForInstantTab() {
   if (instant_tab_) {
-    browser_->UpdateThemeInfo();
+    // Update theme details.
+    InstantService* instant_service = GetInstantService();
+    if (instant_service)
+      instant_service->UpdateThemeInfo();
+
     instant_tab_->SetDisplayInstantResults(instant_enabled_);
     instant_tab_->SetOmniboxBounds(omnibox_bounds_);
     instant_tab_->InitializeFonts();
@@ -1921,3 +1929,7 @@ bool InstantController::InStartup() const {
   // startup time.
   return !browser_->GetActiveWebContents();
 }
+
+InstantService* InstantController::GetInstantService() const {
+  return InstantServiceFactory::GetForProfile(profile());
+}

[commit-bot: I haz the power, 2013-06-22 05:43:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kmadhusu@chromium.org/16413002/223001

[commit-bot: I haz the power, 2013-06-22 13:49:18]

Change committed as 208023