Mark ChromeViewHostMsg_DetailedConsoleMessageAdded as extensions-only.

Mark ChromeViewHostMsg_DetailedConsoleMessageAdded as extensions-only.

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

[Devlin, 2014-06-06 19:03:00]

+yoz, who knows more about refactoring to be extensions-only.

While we do have a number of places where we check EXTENSIONS_ENABLED, it seems
that this might be taking it a bit far - if we did this for multiple features,
it would become very difficult to read which code is executing and which is not.
 I think it would be better to try and architect this in such a way that
extension-specific classes are responsible for sending the messages (such that
if we compile without extensions on, those classes won't exist and won't send
the message).  There are a lot of extension-related messages, and to ifdef all
of them out is going to get messy.

[Lei Zhang, 2014-06-06 19:06:38]

On 2014/06/06 19:03:00, Devlin wrote:
> +yoz, who knows more about refactoring to be extensions-only.
> 
> While we do have a number of places where we check EXTENSIONS_ENABLED, it
seems
> that this might be taking it a bit far - if we did this for multiple features,
> it would become very difficult to read which code is executing and which is
not.
>  I think it would be better to try and architect this in such a way that
> extension-specific classes are responsible for sending the messages (such that
> if we compile without extensions on, those classes won't exist and won't send
> the message).  There are a lot of extension-related messages, and to ifdef all
> of them out is going to get messy.

Of course the eventual goal is to not build most of c/b/extensions when
extensions are not enabled at compile time. But some of the code is a bit too
tangled still. Mainly TabHelper. ChromeExtensionWebContentsObserver is small and
I can try to exclude it altogether.

[Devlin, 2014-06-06 19:10:28]

On 2014/06/06 19:06:38, Lei Zhang wrote:
> On 2014/06/06 19:03:00, Devlin wrote:
> > +yoz, who knows more about refactoring to be extensions-only.
> > 
> > While we do have a number of places where we check EXTENSIONS_ENABLED, it
> seems
> > that this might be taking it a bit far - if we did this for multiple
features,
> > it would become very difficult to read which code is executing and which is
> not.
> >  I think it would be better to try and architect this in such a way that
> > extension-specific classes are responsible for sending the messages (such
that
> > if we compile without extensions on, those classes won't exist and won't
send
> > the message).  There are a lot of extension-related messages, and to ifdef
all
> > of them out is going to get messy.
> 
> Of course the eventual goal is to not build most of c/b/extensions when
> extensions are not enabled at compile time. But some of the code is a bit too
> tangled still. Mainly TabHelper. ChromeExtensionWebContentsObserver is small
and
> I can try to exclude it altogether.

It seems to me that it might also be worthwhile to, instead of ifdef'ing
ChromeRenderFrameObserver, make a ChromeExtensionsRenderFrameObserver. 
Thoughts?

[palmer, 2014-06-06 19:18:36]

IPC LGTM.

[Lei Zhang, 2014-06-06 20:49:19]

On 2014/06/06 19:10:28, Devlin wrote:
> On 2014/06/06 19:06:38, Lei Zhang wrote:
> > On 2014/06/06 19:03:00, Devlin wrote:
> > > +yoz, who knows more about refactoring to be extensions-only.
> > > 
> > > While we do have a number of places where we check EXTENSIONS_ENABLED, it
> > seems
> > > that this might be taking it a bit far - if we did this for multiple
> features,
> > > it would become very difficult to read which code is executing and which
is
> > not.
> > >  I think it would be better to try and architect this in such a way that
> > > extension-specific classes are responsible for sending the messages (such
> that
> > > if we compile without extensions on, those classes won't exist and won't
> send
> > > the message).  There are a lot of extension-related messages, and to ifdef
> all
> > > of them out is going to get messy.
> > 
> > Of course the eventual goal is to not build most of c/b/extensions when
> > extensions are not enabled at compile time. But some of the code is a bit
too
> > tangled still. Mainly TabHelper. ChromeExtensionWebContentsObserver is small
> and
> > I can try to exclude it altogether.
> 
> It seems to me that it might also be worthwhile to, instead of ifdef'ing
> ChromeRenderFrameObserver, make a ChromeExtensionsRenderFrameObserver. 
> Thoughts?

Patch set 3.

[Devlin, 2014-06-06 20:57:24]

https://codereview.chromium.org/321453003/diff/80001/chrome/common/render_mes...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/321453003/diff/80001/chrome/common/render_mes...
chrome/common/render_messages.h:710: // TODO(thestig) Eventually separate out
all the extensions message.
nit: s/message/messages.

https://codereview.chromium.org/321453003/diff/80001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/321453003/diff/80001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:398: new
ChromeRenderFrameObserver(render_frame);
Did you mean ChromeExtensionsRenderFrameObserver?

https://codereview.chromium.org/321453003/diff/80001/chrome/renderer/extensio...
File chrome/renderer/extensions/chrome_extensions_render_frame_observer.cc
(right):

https://codereview.chromium.org/321453003/diff/80001/chrome/renderer/extensio...
chrome/renderer/extensions/chrome_extensions_render_frame_observer.cc:12:
namespace {
please put all this in the extensions namespace.

[Lei Zhang, 2014-06-06 21:31:35]

https://codereview.chromium.org/321453003/diff/80001/chrome/common/render_mes...
File chrome/common/render_messages.h (right):

https://codereview.chromium.org/321453003/diff/80001/chrome/common/render_mes...
chrome/common/render_messages.h:710: // TODO(thestig) Eventually separate out
all the extensions message.
On 2014/06/06 20:57:23, Devlin wrote:
> nit: s/message/messages.

Done.

https://codereview.chromium.org/321453003/diff/80001/chrome/renderer/chrome_c...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/321453003/diff/80001/chrome/renderer/chrome_c...
chrome/renderer/chrome_content_renderer_client.cc:398: new
ChromeRenderFrameObserver(render_frame);
On 2014/06/06 20:57:23, Devlin wrote:
> Did you mean ChromeExtensionsRenderFrameObserver?

Yes. Copy paste error.

https://codereview.chromium.org/321453003/diff/80001/chrome/renderer/extensio...
File chrome/renderer/extensions/chrome_extensions_render_frame_observer.cc
(right):

https://codereview.chromium.org/321453003/diff/80001/chrome/renderer/extensio...
chrome/renderer/extensions/chrome_extensions_render_frame_observer.cc:12:
namespace {
On 2014/06/06 20:57:23, Devlin wrote:
> please put all this in the extensions namespace.

Done.

[Devlin, 2014-06-06 21:37:39]

Thanks for cleaning it up!  LGTM.

[Lei Zhang, 2014-06-06 21:39:12]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-06-06 21:41:00]

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

[Yoyo Zhou, 2014-06-06 21:44:28]

LGTM

https://codereview.chromium.org/321453003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/321453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Probably not for this patch, but in chrome/browser/ui/tab_helpers.cc is where
this gets included in the first place, and maybe could be excluded instead.

Looking over that file, which is in its current form due to
https://code.google.com/p/chromium/issues/detail?id=337600 , I think there are a
few other extensions tab helpers that are included on Android, perhaps
incorrectly.

[Lei Zhang, 2014-06-06 21:56:24]

https://codereview.chromium.org/321453003/diff/100001/chrome/browser/extensio...
File chrome/browser/extensions/tab_helper.cc (right):

https://codereview.chromium.org/321453003/diff/100001/chrome/browser/extensio...
chrome/browser/extensions/tab_helper.cc:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2014/06/06 21:44:28, Yoyo Zhou wrote:
> Probably not for this patch, but in chrome/browser/ui/tab_helpers.cc is where
> this gets included in the first place, and maybe could be excluded instead.
> 
> Looking over that file, which is in its current form due to
> https://code.google.com/p/chromium/issues/detail?id=337600 , I think there are
a
> few other extensions tab helpers that are included on Android, perhaps
> incorrectly.

I'll be happy to keep separating extensions bits until I fix bug 305852 / finish
auditing chrome IPC messages. If I encounter this file again, I'll take another
stab at removing it from non-extensions builds.

[commit-bot: I haz the power, 2014-06-07 01:31:48]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/14145)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/17262)

[commit-bot: I haz the power, 2014-06-07 02:00:55]

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

[commit-bot: I haz the power, 2014-06-07 02:00:56]

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

[Lei Zhang, 2014-06-07 02:11:37]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-06-07 02:13:36]

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

[commit-bot: I haz the power, 2014-06-07 08:19:53]

Change committed as 275641