Similar as extension, DOM UI needs to be created in its own process....

Similar as extension, DOM UI needs to be created in its own process.

This should fix the bug, crbug.com/68548, where the process limit is not honored when process per tab is defined.  

BUG=6106003
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=70815

[Charlie Reis, 2011-01-06 18:27:58]

Thanks for fixing this.

I'm not sure what the process limit has to do with this, though.  This change
just consolidates DOM UI pages into the same process, regardless of the limit. 
Will the limit still be respected if you open normal URLs in multiple tabs?

http://codereview.chromium.org/6106003/diff/1/chrome/browser/browsing_instanc...
File chrome/browser/browsing_instance.cc (right):

http://codereview.chromium.org/6106003/diff/1/chrome/browser/browsing_instanc...
chrome/browser/browsing_instance.cc:37: if
(url.SchemeIs(chrome::kChromeUIScheme)) {
I think we may have had a bug before-- should this use
DOMUIFactory::UseDOMUIForURL as we do in
RenderViewHostManager::ShouldSwapProcessesForNavigation?  (I think we want to
consolidate all types of DOMUI pages.)

Also, please add a trailing parenthesis to the comment below, as long as we're
here.

http://codereview.chromium.org/6106003/diff/1/chrome/browser/browsing_instanc...
chrome/browser/browsing_instance.cc:49: // about it here.
This comment and the one below assume that the process-per-tab model didn't want
any tabs to be consolidated (since that was the original model).  That's no
longer the case-- we apparently want most of the special cases like extensions
and DOMUI to be consolidated whether we're in process-per-site-instance or
process-per-tab.

That means your change is correct, but please also move these comments up after
line 29 and update them accordingly.  We don't need the "if" on line 43 anymore.

http://codereview.chromium.org/6106003/diff/1/chrome/browser/renderer_host/te...
File chrome/browser/renderer_host/test/render_process_host_browsertest.cc
(right):

http://codereview.chromium.org/6106003/diff/1/chrome/browser/renderer_host/te...
chrome/browser/renderer_host/test/render_process_host_browsertest.cc:35:
RenderProcessHost::SetMaxRendererProcessCount(1);
I don't think you need this line.  Even if the max renderer process count is 20,
your change should make it so that opening two New Tab Pages share the same
process.

http://codereview.chromium.org/6106003/diff/1/chrome/browser/tab_contents/ren...
File chrome/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6106003/diff/1/chrome/browser/tab_contents/ren...
chrome/browser/tab_contents/render_view_host_manager.cc:302: // DOM UI page, it
needs to run in a different process. This matches the
Nit: "it needs to be grouped with other DOM UI pages."

http://codereview.chromium.org/6106003/diff/1/chrome/browser/tab_contents/ren...
chrome/browser/tab_contents/render_view_host_manager.cc:303: // logic when
transition between DOM UI and normal pages.
Nit: transitioning

http://codereview.chromium.org/6106003/diff/1/chrome/browser/tab_contents/ren...
chrome/browser/tab_contents/render_view_host_manager.cc:304: if
(new_entry->url().SchemeIs(chrome::kChromeUIScheme))
I think we want to use DOMUIFactory::UseDOMUIForURL here, as we do below on line
319.

[klobag.chromium, 2011-01-06 20:23:01]

I have uploaded the new changes to address the comments. Thanks for reviewing.

http://codereview.chromium.org/6106003/diff/1/chrome/browser/browsing_instanc...
File chrome/browser/browsing_instance.cc (right):

http://codereview.chromium.org/6106003/diff/1/chrome/browser/browsing_instanc...
chrome/browser/browsing_instance.cc:37: if
(url.SchemeIs(chrome::kChromeUIScheme)) {
On 2011/01/06 18:27:59, creis wrote:
> I think we may have had a bug before-- should this use
> DOMUIFactory::UseDOMUIForURL as we do in
> RenderViewHostManager::ShouldSwapProcessesForNavigation?  (I think we want to
> consolidate all types of DOMUI pages.)
> 
> Also, please add a trailing parenthesis to the comment below, as long as we're
> here.

Done.

http://codereview.chromium.org/6106003/diff/1/chrome/browser/browsing_instanc...
chrome/browser/browsing_instance.cc:49: // about it here.
On 2011/01/06 18:27:59, creis wrote:
> This comment and the one below assume that the process-per-tab model didn't
want
> any tabs to be consolidated (since that was the original model).  That's no
> longer the case-- we apparently want most of the special cases like extensions
> and DOMUI to be consolidated whether we're in process-per-site-instance or
> process-per-tab.
> 
> That means your change is correct, but please also move these comments up
after
> line 29 and update them accordingly.  We don't need the "if" on line 43
anymore.

Done.

http://codereview.chromium.org/6106003/diff/1/chrome/browser/renderer_host/te...
File chrome/browser/renderer_host/test/render_process_host_browsertest.cc
(right):

http://codereview.chromium.org/6106003/diff/1/chrome/browser/renderer_host/te...
chrome/browser/renderer_host/test/render_process_host_browsertest.cc:35:
RenderProcessHost::SetMaxRendererProcessCount(1);
I added some normal page cases.

http://codereview.chromium.org/6106003/diff/1/chrome/browser/tab_contents/ren...
File chrome/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6106003/diff/1/chrome/browser/tab_contents/ren...
chrome/browser/tab_contents/render_view_host_manager.cc:302: // DOM UI page, it
needs to run in a different process. This matches the
On 2011/01/06 18:27:59, creis wrote:
> Nit: "it needs to be grouped with other DOM UI pages."

Done.

http://codereview.chromium.org/6106003/diff/1/chrome/browser/tab_contents/ren...
chrome/browser/tab_contents/render_view_host_manager.cc:303: // logic when
transition between DOM UI and normal pages.
On 2011/01/06 18:27:59, creis wrote:
> Nit: transitioning

Done.

http://codereview.chromium.org/6106003/diff/1/chrome/browser/tab_contents/ren...
chrome/browser/tab_contents/render_view_host_manager.cc:304: if
(new_entry->url().SchemeIs(chrome::kChromeUIScheme))
On 2011/01/06 18:27:59, creis wrote:
> I think we want to use DOMUIFactory::UseDOMUIForURL here, as we do below on
line
> 319.

Done.

[Charlie Reis, 2011-01-06 21:00:20]

LGTM apart from the missing comment in browsing_instance.cc.

http://codereview.chromium.org/6106003/diff/1/chrome/browser/browsing_instanc...
File chrome/browser/browsing_instance.cc (right):

http://codereview.chromium.org/6106003/diff/1/chrome/browser/browsing_instanc...
chrome/browser/browsing_instance.cc:49: // about it here.
On 2011/01/06 20:23:02, klobag.chromium wrote:
> On 2011/01/06 18:27:59, creis wrote:
> > This comment and the one below assume that the process-per-tab model didn't
> want
> > any tabs to be consolidated (since that was the original model).  That's no
> > longer the case-- we apparently want most of the special cases like
extensions
> > and DOMUI to be consolidated whether we're in process-per-site-instance or
> > process-per-tab.
> > 
> > That means your change is correct, but please also move these comments up
> after
> > line 29 and update them accordingly.  We don't need the "if" on line 43
> anymore.
> 
> Done.

Actually, it looks like the comments are gone now.  I think we still want them
above the extension check.

[klobag, 2011-01-06 21:30:05]

Updated the comment. Please take another look.

On Thu, Jan 6, 2011 at 1:00 PM,  <creis@chromium.org> wrote:
> LGTM apart from the missing comment in browsing_instance.cc.
>
>
>
http://codereview.chromium.org/6106003/diff/1/chrome/browser/browsing_instanc...
> File chrome/browser/browsing_instance.cc (right):
>
>
http://codereview.chromium.org/6106003/diff/1/chrome/browser/browsing_instanc...
> chrome/browser/browsing_instance.cc:49: // about it here.
> On 2011/01/06 20:23:02, klobag.chromium wrote:
>>
>> On 2011/01/06 18:27:59, creis wrote:
>> > This comment and the one below assume that the process-per-tab model
>
> didn't
>>
>> want
>> > any tabs to be consolidated (since that was the original model).
>
> That's no
>>
>> > longer the case-- we apparently want most of the special cases like
>
> extensions
>>
>> > and DOMUI to be consolidated whether we're in
>
> process-per-site-instance or
>>
>> > process-per-tab.
>> >
>> > That means your change is correct, but please also move these
>
> comments up
>>
>> after
>> > line 29 and update them accordingly.  We don't need the "if" on line
>
> 43
>>
>> anymore.
>
>> Done.
>
> Actually, it looks like the comments are gone now.  I think we still
> want them above the extension check.
>
> http://codereview.chromium.org/6106003/
>



-- 
thanks, Grace

[Charlie Reis, 2011-01-06 21:36:59]

Thanks-- LGTM!

http://codereview.chromium.org/6106003/diff/13001/chrome/browser/browsing_ins...
File chrome/browser/browsing_instance.cc (right):

http://codereview.chromium.org/6106003/diff/13001/chrome/browser/browsing_ins...
chrome/browser/browsing_instance.cc:32: // We want to consolidate the particular
sites like extensions and DOMUI
Nit: "consolidate particular"

[klobag.chromium, 2011-01-06 23:21:26]

Add a NULL check to fix unit_tests. Please take another look.

[Charlie Reis, 2011-01-06 23:30:54]

Null check looks ok.

http://codereview.chromium.org/6106003/diff/23002/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/6106003/diff/23002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_host_manager.cc:306: //      return
true;
These shouldn't be commented out.

[klobag, 2011-01-06 23:33:05]

On Thu, Jan 6, 2011 at 3:30 PM,  <creis@chromium.org> wrote:
> Null check looks ok.
>
>
>
http://codereview.chromium.org/6106003/diff/23002/chrome/browser/tab_contents...
> File chrome/browser/tab_contents/render_view_host_manager.cc (right):
>
>
http://codereview.chromium.org/6106003/diff/23002/chrome/browser/tab_contents...
> chrome/browser/tab_contents/render_view_host_manager.cc:306: //
> return true;
> These shouldn't be commented out.
>

Oops. I was tracking which one caused the unit_tests failed. Thanks
for the catch. Sorry about the back and forth.

> http://codereview.chromium.org/6106003/
>



-- 
thanks, Grace

[Charlie Reis, 2011-01-06 23:38:12]

No worries.  LGTM.

[Charlie Reis, 2011-01-07 23:40:37]

http://codereview.chromium.org/6106003/diff/46001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_host_manager_unittest.cc (left):

http://codereview.chromium.org/6106003/diff/46001/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_host_manager_unittest.cc:267:
manager.DidNavigateMainFrame(host);
Let's keep this commit block as the last line of the test (below the DOMUI
related EXPECTs), just to prevent regressions in DidNavigateMainFrame in this
case.

http://codereview.chromium.org/6106003/diff/46001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_host_manager_unittest.cc (right):

http://codereview.chromium.org/6106003/diff/46001/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_host_manager_unittest.cc:264:
EXPECT_FALSE(manager.pending_dom_ui());
Let's put a comment above this line saying that the DOMUI is committed
immediately because the RenderViewHost has not been used yet.  It'll help others
understand why the change was needed.

[klobag.chromium, 2011-01-07 23:50:30]

Thanks. Added the comment and added back DidNavigateMainFrame()

[Charlie Reis, 2011-01-07 23:54:16]

Thanks-- LGTM.

[brettw, 2011-01-10 20:21:05]

LGTM2