ChromeContentRendererClient should not rely on Dispatcher::is_extension_process

ChromeContentRendererClient should not rely on Dispatcher::is_extension_process

The extensions::Dispatcher::is_extension_process flag is set for both the
standalone extensions renderer and single process mode. This caused
ChromeContentRendererClient to act differently in single process mode, like
returning true from ShouldFork for any navigation started by blink, which
would cause a new WebView to be created for each navigation.

BUG=462210
Committed: https://crrev.com/874b1b9796998d7d8bf4d27ed1369251bd868d13
Cr-Commit-Position: refs/heads/master@{#318938}

[João Eiras, 2015-02-26 14:09:30]

joaoe@opera.com changed reviewers:
+ jhawkins@chromium.org, jochen@chromium.org, thakis@chromium.org,
thestig@chromium.org

[João Eiras, 2015-02-26 14:11:25]

The behavior of checking only the extension_process flag causes, for instance,
ShouldFork to dispatch a navigation request to the UI everytime there is a
navigation triggered by JS instead of that navigation being handled directly by
blink. The other 2 fixes seemed to make sense, although I'd still like to know
if they're ok.

[Lei Zhang, 2015-02-26 22:17:58]

thestig@chromium.org changed reviewers:
+ kalman@chromium.org

[Lei Zhang, 2015-02-26 22:17:58]

I actually don't know how well extensions work at all in single process mode.
+kalman randomly from the extensions OWNERS file.

https://codereview.chromium.org/956313002/diff/1/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/956313002/diff/1/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:286: bool IsSingleProcess() {
Given you want to do this check in every place in the file that also calls
is_extension_process(), you probably want to add a method to
ChromeContentRendererClient to do both.

[not at google - send to devlin, 2015-02-26 22:25:03]

https://codereview.chromium.org/956313002/diff/1/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/956313002/diff/1/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:286: bool IsSingleProcess() {
On 2015/02/26 22:17:58, Lei Zhang wrote:
> Given you want to do this check in every place in the file that also calls
> is_extension_process(), you probably want to add a method to
> ChromeContentRendererClient to do both.

I don't know this code that well either, but note that in dispatcher.cc we
already lie about it being an extension process in single process mode:

https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...

and all it uses that for is deciding whether to monitor idle time.

It may be more appropriate to change dispatcher.cc rather than this file.

[João Eiras, 2015-02-27 01:30:40]

> I don't know this code that well either, but note that in dispatcher.cc we
> already lie about it being an extension process in single process mode:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...
> 
> and all it uses that for is deciding whether to monitor idle time.
> 
> It may be more appropriate to change dispatcher.cc rather than this file.

Changing it in what way ? My changes have the opposite effect of the code in
dispatcher.cc which sets the flag in case of single-process. But then I don't
thing changing the behavior of is_extension_process is a good idea, since that's
supposed to tell whether extensions are running in the current process or not.

[not at google - send to devlin, 2015-02-27 01:34:28]

On 2015/02/27 01:30:40, João Eiras wrote:
> > I don't know this code that well either, but note that in dispatcher.cc we
> > already lie about it being an extension process in single process mode:
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...
> > 
> > and all it uses that for is deciding whether to monitor idle time.
> > 
> > It may be more appropriate to change dispatcher.cc rather than this file.
> 
> Changing it in what way ? My changes have the opposite effect of the code in
> dispatcher.cc which sets the flag in case of single-process. But then I don't
> thing changing the behavior of is_extension_process is a good idea, since
that's
> supposed to tell whether extensions are running in the current process or not.

I'm not sure what "is extension process" is trying to do - you could interpret
it either way. I'm suggesting removing the single-process check from
is_extension_process and instead having a special "enable idle monitoring" flag
or something. To help with the naming you might call it
IsExtensionRenderProcess.

[João Eiras, 2015-02-27 13:59:58]

joaoe@opera.com changed reviewers:
+ fsamuel@chromium.org, rdevlin.cronin@chromium.org

[João Eiras, 2015-02-27 14:00:16]

Forget to tell. Without this fix any new navigation to a new url causes a new
ViewMsg_New message to be sent to the renderer, creating a new WebView and
throwing away the old one.

[João Eiras, 2015-02-27 14:01:21]

Devlin and Fady, after reading the conversation, waht's your opinion on
extensions::Dispatcher::is_extension_process ? Thanks.

[Devlin, 2015-02-27 16:54:32]

Each reviewer you add can boost the complexity of reviewing a change.  It's not
really common practice to add so many people, because it can lead to a "Too many
cooks in the kitchen" problem.

But, since I'm here...

https://codereview.chromium.org/956313002/diff/1/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/956313002/diff/1/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:286: bool IsSingleProcess() {
On 2015/02/26 22:25:03, kalman wrote:
> On 2015/02/26 22:17:58, Lei Zhang wrote:
> > Given you want to do this check in every place in the file that also calls
> > is_extension_process(), you probably want to add a method to
> > ChromeContentRendererClient to do both.
> 
> I don't know this code that well either, but note that in dispatcher.cc we
> already lie about it being an extension process in single process mode:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...
> 
> and all it uses that for is deciding whether to monitor idle time.
> 
> It may be more appropriate to change dispatcher.cc rather than this file.

Two things:
- I tend to agree with Ben - if "lying" in the dispatcher is causing us to have
to check IsSingleProcess at every point here, then the problem sounds like it's
in dispatcher, not here.
- This function looks wrong - shouldn't IsSingleProcess return true if the
command line *does* have the single process flag?

[João Eiras, 2015-03-02 17:02:49]

https://codereview.chromium.org/956313002/diff/1/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/956313002/diff/1/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:286: bool IsSingleProcess() {
On 2015/02/27 16:54:32, Devlin wrote:
> - I tend to agree with Ben - if "lying" in the dispatcher is causing us to
have
> to check IsSingleProcess at every point here, then the problem sounds like
it's
> in dispatcher, not here.

I changed the dispatcher code then. Thanks for your input.

> - This function looks wrong - shouldn't IsSingleProcess return true if the
> command line *does* have the single process flag?

Was wrong yes. But now the code is different.

[not at google - send to devlin, 2015-03-02 17:51:16]

lgtm

[jochen (gone - plz use gerrit), 2015-03-03 13:53:11]

lgtm

[João Eiras, 2015-03-03 15:25:09]

The CQ bit was checked by joaoe@opera.com

[commit-bot: I haz the power, 2015-03-03 15:26:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/956313002/20001

[commit-bot: I haz the power, 2015-03-03 16:01:35]

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

[commit-bot: I haz the power, 2015-03-03 16:01:37]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[João Eiras, 2015-03-03 17:45:33]

The CQ bit was checked by joaoe@opera.com

[João Eiras, 2015-03-03 17:45:34]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/956313002/#ps40001 (title:
"missing ifdefs")

[commit-bot: I haz the power, 2015-03-03 17:46:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/956313002/40001

[commit-bot: I haz the power, 2015-03-03 20:44:09]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-03 20:45:04]

Patchset 3 (id:??) landed as
https://crrev.com/874b1b9796998d7d8bf4d27ed1369251bd868d13
Cr-Commit-Position: refs/heads/master@{#318938}