ifdef extensions code in BrowserProcess, IOThread, and ChromeNetworkDelegate.

ifdef extensions code in BrowserProcess, IOThread, and ChromeNetworkDelegate.

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

[Lei Zhang, 2014-07-16 03:32:44]

There's a lot of #ifdefs. If you have better ideas, let me know.

[Ryan Sleevi, 2014-07-16 20:23:44]

... Gross :'(

Makes me wonder if we can't do something like base::ThreadChecker /
base::NonThreadSafe , with an empty base object.

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:197:
ExtensionRendererState::GetInstance()->Init();
What are the implications now with respect to startup? It's hard to know whether
the original ordering was intentional or not, especially given that these bits
(200-205) are touching extensions as well, and set the client before the
forwarder.

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:966: #endif
Prefer to stuff this in a member variable (wrapped in #ifdef) and then pass that
variable unconditionally


extensions::EventRouterForwarder* event_router_forwarder = NULL;
#if defined(ENABLE_EXTENSIONS)
event_router_forwarder = extension_event_router_forwarder_.get();
#endif
new IOThread ...


Of course, even this is unnecessary, as you could just call
extension_event_router_forwarder(), which already does the right #ifdef dance.

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/browser_...
chrome/browser/browser_process_impl.h:197:
scoped_ptr<extensions::ExtensionsBrowserClient> extensions_browser_client_;
Why doesn't this have to be #ifdef'd? It's in the extensions namespace.

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/browser_...
chrome/browser/browser_process_impl.h:216: #endif
I'm assuming the order of shutdown is particularly pertinent here with respect
to MediaFileSystemRegistry before RemoteDebugging before EventRouterForwarder?

Otherwise, would it make more sense to encapsulate this into a scoped_ptr<> of
some ExtensionsCrap struct (forward declared in the .h), where the .cc has the
ERF, MFSR, and PPDC/BPM objects hanging off?

The benefit here is to reduce the number of publicly-visible (read: in the
header) ifdefs, especially since they affect object size computations.

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:449: #if defined(ENABLE_EXTENSIONS)
Why do you #ifdef the member, but not the ctor param?

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:578: extension_event_router_forwarder_,
wrap this ala BrowserProcessImpl in a member accessor that does the #ifdef
dance?

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:350: #endif
Doesn't seem strictly necessary to #ifdef dance this out.

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate_unittest.cc (right):

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate_unittest.cc:120: #endif
Wrap this into a fixture helper?

[Lei Zhang, 2014-07-18 01:34:24]

I think having pointers to EventRouterForwarder that never get dereferenced when
extensions are disabled is the better way to separate the extensions code.
Having a dummy implementation means you will still have to depend on the
extensions targets, though it can potentially be a very small dummy target.

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:197:
ExtensionRendererState::GetInstance()->Init();
On 2014/07/16 20:23:43, Ryan Sleevi wrote:
> What are the implications now with respect to startup? It's hard to know
whether
> the original ordering was intentional or not, especially given that these bits
> (200-205) are touching extensions as well, and set the client before the
> forwarder.

I believe this to be ok, but I can loop in someone on the extensions-side to
verify.

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/browser_...
chrome/browser/browser_process_impl.cc:966: #endif
On 2014/07/16 20:23:43, Ryan Sleevi wrote:
> Prefer to stuff this in a member variable (wrapped in #ifdef) and then pass
that
> variable unconditionally
> 
> 
> extensions::EventRouterForwarder* event_router_forwarder = NULL;
> #if defined(ENABLE_EXTENSIONS)
> event_router_forwarder = extension_event_router_forwarder_.get();
> #endif
> new IOThread ...
> 
> 
> Of course, even this is unnecessary, as you could just call
> extension_event_router_forwarder(), which already does the right #ifdef dance.

Done.

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/browser_...
File chrome/browser/browser_process_impl.h (right):

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/browser_...
chrome/browser/browser_process_impl.h:197:
scoped_ptr<extensions::ExtensionsBrowserClient> extensions_browser_client_;
On 2014/07/16 20:23:43, Ryan Sleevi wrote:
> Why doesn't this have to be #ifdef'd? It's in the extensions namespace.

Too many pieces depend on the ExtensionsBrowserClient to exist. Thus I'm picking
off many of the other pieces in manageable CLs size-wise before going in and
removing the all the browser-side extensions code in one big swoop.

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/browser_...
chrome/browser/browser_process_impl.h:216: #endif
On 2014/07/16 20:23:43, Ryan Sleevi wrote:
> I'm assuming the order of shutdown is particularly pertinent here with respect
> to MediaFileSystemRegistry before RemoteDebugging before EventRouterForwarder?
> 
> Otherwise, would it make more sense to encapsulate this into a scoped_ptr<> of
> some ExtensionsCrap struct (forward declared in the .h), where the .cc has the
> ERF, MFSR, and PPDC/BPM objects hanging off?
> 
> The benefit here is to reduce the number of publicly-visible (read: in the
> header) ifdefs, especially since they affect object size computations.

The printing bits are separate from the extensions bits. I just happened to
notice they are ifdef'd out in the .cc file but not here. That leaves only 3
member variables that are extensions only. I'm kind of meh about making a struct
for so few. I did reorder them so they are all in 1 #ifdef.

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/io_threa...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:449: #if defined(ENABLE_EXTENSIONS)
On 2014/07/16 20:23:44, Ryan Sleevi wrote:
> Why do you #ifdef the member, but not the ctor param?

I considered #ifdefing the ctor, or adding a separate setter, but that makes all
the callers uglier.

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/io_threa...
chrome/browser/io_thread.cc:578: extension_event_router_forwarder_,
On 2014/07/16 20:23:44, Ryan Sleevi wrote:
> wrap this ala BrowserProcessImpl in a member accessor that does the #ifdef
> dance?

Done.

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate.cc:350: #endif
On 2014/07/16 20:23:44, Ryan Sleevi wrote:
> Doesn't seem strictly necessary to #ifdef dance this out.

All the throttle code here exists to support
--disable-extensions-http-throttling

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/net/chro...
File chrome/browser/net/chrome_network_delegate_unittest.cc (right):

https://codereview.chromium.org/387003008/diff/100001/chrome/browser/net/chro...
chrome/browser/net/chrome_network_delegate_unittest.cc:120: #endif
On 2014/07/16 20:23:44, Ryan Sleevi wrote:
> Wrap this into a fixture helper?

Done.

[Lei Zhang, 2014-07-18 01:35:55]

yoz: I don't believe the extensions init code ordering in
BrowserProcessImpl::BrowserProcessImpl() matters. Can you confirm?

[Yoyo Zhou, 2014-07-18 01:42:34]

On 2014/07/18 01:35:55, Lei Zhang wrote:
> yoz: I don't believe the extensions init code ordering in
> BrowserProcessImpl::BrowserProcessImpl() matters. Can you confirm?

I think that's right in this case.

[Ryan Sleevi, 2014-07-18 20:26:10]

On 2014/07/18 01:34:24, Lei Zhang wrote:
> I think having pointers to EventRouterForwarder that never get dereferenced
when
> extensions are disabled is the better way to separate the extensions code.
> Having a dummy implementation means you will still have to depend on the
> extensions targets, though it can potentially be a very small dummy target.


That also makes me happy, but that doesn't seem to be what this CL is doing,
right? Was that an indication you were going to be working more on this? I
wasn't sure.

[Lei Zhang, 2014-07-18 20:32:57]

On 2014/07/18 20:26:10, Ryan Sleevi wrote:
> On 2014/07/18 01:34:24, Lei Zhang wrote:
> > I think having pointers to EventRouterForwarder that never get dereferenced
> when
> > extensions are disabled is the better way to separate the extensions code.
> > Having a dummy implementation means you will still have to depend on the
> > extensions targets, though it can potentially be a very small dummy target.
> 
> 
> That also makes me happy, but that doesn't seem to be what this CL is doing,
> right? Was that an indication you were going to be working more on this? I
> wasn't sure.

The EventRouterForwarder pointer eventually goes to ChromeNetworkDelegate, which
then simply passes it off to ChromeExtensionsNetworkDelegate, where we have a
dummy implementation that ignores it when ENABLE_EXTENSIONS is not defined.

[Ryan Sleevi, 2014-07-18 21:01:35]

lgtm

[Lei Zhang, 2014-07-18 23:44:48]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-07-18 23:46:18]

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

[commit-bot: I haz the power, 2014-07-19 04:52:10]

Change committed as 284312