|
|
Created:
5 years, 9 months ago by João Eiras Modified:
5 years, 9 months ago Reviewers:
Devlin, jochen (gone - plz use gerrit), Lei Zhang, Nico, Fady Samuel, James Hawkins, not at google - send to devlin CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChromeContentRendererClient 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}
Patch Set 1 #
Total comments: 4
Patch Set 2 : removing is_extension_process from extensions::Dispatcher #Patch Set 3 : missing ifdefs #
Created: 5 years, 9 months ago
Messages
Total messages: 24 (7 generated)
joaoe@opera.com changed reviewers: + jhawkins@chromium.org, jochen@chromium.org, thakis@chromium.org, thestig@chromium.org
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.
thestig@chromium.org changed reviewers: + kalman@chromium.org
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.
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.
> 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.
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.
joaoe@opera.com changed reviewers: + fsamuel@chromium.org, rdevlin.cronin@chromium.org
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.
Devlin and Fady, after reading the conversation, waht's your opinion on extensions::Dispatcher::is_extension_process ? Thanks.
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?
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.
lgtm
lgtm
The CQ bit was checked by joaoe@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/956313002/20001
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by joaoe@opera.com
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")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/956313002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/874b1b9796998d7d8bf4d27ed1369251bd868d13 Cr-Commit-Position: refs/heads/master@{#318938} |