ifdef extensions bits in ContentSettingsObserver.

ifdef extensions bits in ContentSettingsObserver.

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

[Devlin, 2014-08-12 15:34:59]

https://codereview.chromium.org/464693002/diff/20001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/464693002/diff/20001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:29: #include
"extensions/common/permissions/permissions_data.h"
Doesn't actually look like we use this at first glance.

https://codereview.chromium.org/464693002/diff/20001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:445: WebFrame* frame =
render_frame()->GetWebFrame();
This and the code in allowMutationEvents looks very similar, and it also looks
like we only use GetExtension(origin) in these two places.  What do you think of
refactoring so that GetExtension becomes HasExtensionAndIsPlatformApp()
(strawman name - feel free to use something better) and also does the work in
lines 434 - 438?  I'd like to try and keep these if defined blocks as
concentrated as possible...

[Lei Zhang, 2014-08-12 23:03:17]

https://codereview.chromium.org/464693002/diff/20001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/464693002/diff/20001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:29: #include
"extensions/common/permissions/permissions_data.h"
On 2014/08/12 15:34:59, Devlin wrote:
> Doesn't actually look like we use this at first glance.

deleted

https://codereview.chromium.org/464693002/diff/20001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:445: WebFrame* frame =
render_frame()->GetWebFrame();
On 2014/08/12 15:34:59, Devlin wrote:
> This and the code in allowMutationEvents looks very similar, and it also looks
> like we only use GetExtension(origin) in these two places.  What do you think
of
> refactoring so that GetExtension becomes HasExtensionAndIsPlatformApp()
> (strawman name - feel free to use something better) and also does the work in
> lines 434 - 438?  I'd like to try and keep these if defined blocks as
> concentrated as possible...

[fry_squint] These two functions look similar.[/fry_squint]

[Devlin, 2014-08-12 23:16:12]

https://codereview.chromium.org/464693002/diff/20001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/464693002/diff/20001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:445: WebFrame* frame =
render_frame()->GetWebFrame();
On 2014/08/12 23:03:17, Lei Zhang wrote:
> On 2014/08/12 15:34:59, Devlin wrote:
> > This and the code in allowMutationEvents looks very similar, and it also
looks
> > like we only use GetExtension(origin) in these two places.  What do you
think
> of
> > refactoring so that GetExtension becomes HasExtensionAndIsPlatformApp()
> > (strawman name - feel free to use something better) and also does the work
in
> > lines 434 - 438?  I'd like to try and keep these if defined blocks as
> > concentrated as possible...
> 
> [fry_squint] These two functions look similar.[/fry_squint]

:D

https://codereview.chromium.org/464693002/diff/40001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/464693002/diff/40001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:634: const extensions::Extension*
extension = GetExtension(origin);
Any reason to not save a couple ifdefs and just inline GetExtension() here?

const Extension* = NULL;
if (EqualsASCII(origin.protocol(), extensions::kExtensionScheme)) {
  const std::string extension_id = origin.host().utf8().data();
  extension = extension_dispatcher_->IsExtensionActive(extension_id) ?
      extension_dispatcher_->extensions()->GetByID(extension_id) : NULL;
}

[Lei Zhang, 2014-08-12 23:18:06]

https://codereview.chromium.org/464693002/diff/40001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/464693002/diff/40001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:634: const extensions::Extension*
extension = GetExtension(origin);
On 2014/08/12 23:16:12, Devlin wrote:
> Any reason to not save a couple ifdefs and just inline GetExtension() here?
> 
> const Extension* = NULL;
> if (EqualsASCII(origin.protocol(), extensions::kExtensionScheme)) {
>   const std::string extension_id = origin.host().utf8().data();
>   extension = extension_dispatcher_->IsExtensionActive(extension_id) ?
>       extension_dispatcher_->extensions()->GetByID(extension_id) : NULL;
> }

It's already there and it's very obvious what it does?

[Devlin, 2014-08-12 23:20:47]

lgtm

https://codereview.chromium.org/464693002/diff/40001/chrome/renderer/content_...
File chrome/renderer/content_settings_observer.cc (right):

https://codereview.chromium.org/464693002/diff/40001/chrome/renderer/content_...
chrome/renderer/content_settings_observer.cc:634: const extensions::Extension*
extension = GetExtension(origin);
On 2014/08/12 23:18:06, Lei Zhang wrote:
> On 2014/08/12 23:16:12, Devlin wrote:
> > Any reason to not save a couple ifdefs and just inline GetExtension() here?
> > 
> > const Extension* = NULL;
> > if (EqualsASCII(origin.protocol(), extensions::kExtensionScheme)) {
> >   const std::string extension_id = origin.host().utf8().data();
> >   extension = extension_dispatcher_->IsExtensionActive(extension_id) ?
> >       extension_dispatcher_->extensions()->GetByID(extension_id) : NULL;
> > }
> 
> It's already there and it's very obvious what it does?

Yeah.  I just think that ifdefs in general make code harder to read (and harder
to pull out functions that shouldn't be there), so the more we condense them the
better.  But... I don't feel strongly enough about it in this case. :)

[Lei Zhang, 2014-08-12 23:25:03]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-08-12 23:52:44]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/thestig@chromium.org/464693002/40001

[commit-bot: I haz the power, 2014-08-13 03:40:08]

Change committed as 289188