Make ExtensionSettingsHandler only listen to notifications pertaining to its profile.

Make ExtensionSettingsHandler only listen to notifications pertaining to its profile.

BUG=98996
TEST=covered by existing tests

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

[Yoyo Zhou, 2011-10-07 21:11:00]

jstritar: primary reviewer
finnur: mostly FYI, but also if you have specific knowledge about these
notifications

[Finnur, 2011-10-10 10:34:53]

Thanks for doing this... One question to ponder...

http://codereview.chromium.org/8199022/diff/1/chrome/browser/ui/webui/options...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):

http://codereview.chromium.org/8199022/diff/1/chrome/browser/ui/webui/options...
chrome/browser/ui/webui/options/extension_settings_handler.cc:647: }
There's a lot of duplicate code here... I'm wondering... can't you collapse this
into:

Profile* profile = NULL;
bool compare_profiles = true;
switch (type) {
  case content::NOTIFICATION_RENDER_VIEW_HOST_CREATED:
    profile = Profile::FromBrowserContext(
        Source<RenderViewHost>(source)->site_instance()->
        browsing_instance()->browser_context());
    break;
  case content::NOTIFICATION_RENDER_VIEW_HOST_DELETED:
    profile = ...;
    break;
  case chrome::NOTIFICATION_BACKGROUND_CONTENTS_DELETED:
    profile = ...;
    deleting_rvh = ...;
    break;
  ... etc ...
  case chrome::NOTIFICATION_EXTENSION_LOADED:
  case chrome::NOTIFICATION_EXTENSION_PROCESS_CREATED:
  ... etc ...
    compare_profiles = false;
    break;
  default:
    NOTREACHED();
    break;
}

if (compare_profiles && !this_profile->IsSameProfile(profile))
  return;

MaybeUpdateAfterNotification();


What do you think? Think this will improve things or make things worse? (I'll
leave it up to you).

[Yoyo Zhou, 2011-10-10 18:41:36]

Hmm, I misread your comment and reimplemented it differently. =) I thought I'd
read something in the style guide about not falling through between cases, but I
don't see anything like that, so I now use some (carefully annotated) fall
throughs to not have as much redundant code. (Also, I found an easier way to get
the profile for BACKGROUND_CONTENTS_* notifications.)

[jstritar, 2011-10-10 20:05:02]

LGTM

http://codereview.chromium.org/8199022/diff/4003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):

http://codereview.chromium.org/8199022/diff/4003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/extension_settings_handler.cc:603:
browsing_instance()->browser_context());
nit: not sure if this is the right way to indent this. do you need another 4
spaces on the last line?

[Yoyo Zhou, 2011-10-10 20:30:37]

http://codereview.chromium.org/8199022/diff/4003/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):

http://codereview.chromium.org/8199022/diff/4003/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/extension_settings_handler.cc:603:
browsing_instance()->browser_context());
On 2011/10/10 20:05:03, jstritar wrote:
> nit: not sure if this is the right way to indent this. do you need another 4
> spaces on the last line?

I think this is right for line continuations that aren't continuations of the
first line. The C++ style guide doesn't actually say, but this is how the
apropos Emacs mode indents it.

[jstritar, 2011-10-10 20:37:10]

On 2011/10/10 20:30:37, Yoyo Zhou wrote:
>
http://codereview.chromium.org/8199022/diff/4003/chrome/browser/ui/webui/opti...
> File chrome/browser/ui/webui/options/extension_settings_handler.cc (right):
> 
>
http://codereview.chromium.org/8199022/diff/4003/chrome/browser/ui/webui/opti...
> chrome/browser/ui/webui/options/extension_settings_handler.cc:603:
> browsing_instance()->browser_context());
> On 2011/10/10 20:05:03, jstritar wrote:
> > nit: not sure if this is the right way to indent this. do you need another 4
> > spaces on the last line?
> 
> I think this is right for line continuations that aren't continuations of the
> first line. The C++ style guide doesn't actually say, but this is how the
> apropos Emacs mode indents it.

Nice, sgtm