InstantExtended: Send search URLs to renderers.

InstantExtended: Send search URLs to renderers.                                 
                                                                                
Previously, navigations initiated by an instant renderer were bounced to
the browser to be rebucketed into an instant or non-instant renderer.
But navigations from non-instant renderers to instant URLs (i.e.
privileged search page URLs) were not rebucketed, because the renderer
had no knowledge of which URLs were instant URLs, and it would be too
expensive to bounce ALL URLs. As a result, non-instant pages like
google.com/setprefs could not redirect to instant pages like
google.com/search.
                                                                        
This change has InstantService tell renderers about URLs associated with
the default search engine, and uses this knowledge to bounce
renderer-initiated navigations to likely instant URLs from non-instant
processes back into the browser for rebucketing. So now link clicks from
non-instant pages to instant pages will put the destination pages into
an instant process. Unfortunately, though, redirects are still broken.
For example,
                                                                                
0. User clicks "Set preferences" button on an instant page.
1. Instant renderer bounces google.com/setprefs to the browser.
2. Browser says google.com/setprefs is not an instant URL,
   and assigns it to a non-instant renderer.
3. After rebucketing to the non-instant renderer, google.com/setprefs
   is marked as a browser initiated request(!)
4. /setprefs redirects to /search, which _is_ an instant URL, but
   because /setprefs is marked as browser initiated the non-instant
   renderer does not get a chance to bounce it back to the browser.
                                                                                
I tried working around this by giving redirects a chance to bounce back
to the browser in step #4, but this broke the signin process model in a
scary way that I don't fully understand. It seems like the right fix
here is going to need to EITHER follow the redirect chain in step #2
when determining if an URL is an instant URL, OR somehow flag the
bounced redirect request for further inspection in step #3.

This change also includes a small side benefit. Since renderers now know
about instant URLs, we can suppress a flash of a baked-in error page in
the event of an error while loading the InstantExtended new tab page.

TEST=manual,unit,browsertest
BUG=271088, 223754
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=226103

[Jered, 2013-09-12 22:34:39]

samarth -> search
creis -> content
palmer,jschuh -> ipcs, security
jhawkins -> chrome/renderer and webui

[Charlie Reis, 2013-09-13 22:33:17]

I know there was a lot of discussion about this behavior on
http://crbug.com/223754, so we should really have a test to verify it if we're
adding it.

https://codereview.chromium.org/23455047/diff/4001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23455047/diff/4001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:3696: if
(GetContentClient()->renderer()->ShouldSuppressErrorPage(
Please coordinate with lazyboy@ on this, since he's doing almost the same thing
in https://chromiumcodereview.appspot.com/23856006/.

Either way, this should probably go earlier, closer to the ERR_ABORTED return
statement.

[Jered, 2013-09-13 22:48:24]

On 2013/09/13 22:33:17, creis wrote:
> I know there was a lot of discussion about this behavior on
> http://crbug.com/223754, so we should really have a test to verify it if we're
> adding it.

Agreed, my intent in sending this out without tests was to make
sure that this is sane for content, will add a bunch of course assuming
this approach is ok.

> 
>
https://codereview.chromium.org/23455047/diff/4001/content/renderer/render_vi...
> File content/renderer/render_view_impl.cc (right):
> 
>
https://codereview.chromium.org/23455047/diff/4001/content/renderer/render_vi...
> content/renderer/render_view_impl.cc:3696: if
> (GetContentClient()->renderer()->ShouldSuppressErrorPage(
> Please coordinate with lazyboy@ on this, since he's doing almost the same
thing
> in https://chromiumcodereview.appspot.com/23856006/.
> 
> Either way, this should probably go earlier, closer to the ERR_ABORTED return
> statement.

We still want to tell the viewhost about the error in the ntp case so
that it can initiate a redirect to the local page.

[Charlie Reis, 2013-09-13 22:58:09]

On 2013/09/13 22:48:24, Jered wrote:
> On 2013/09/13 22:33:17, creis wrote:
> > I know there was a lot of discussion about this behavior on
> > http://crbug.com/223754, so we should really have a test to verify it if
we're
> > adding it.
> 
> Agreed, my intent in sending this out without tests was to make
> sure that this is sane for content, will add a bunch of course assuming
> this approach is ok.

Great, thanks.

https://codereview.chromium.org/23455047/diff/4001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23455047/diff/4001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:3696: if
(GetContentClient()->renderer()->ShouldSuppressErrorPage(
> > Please coordinate with lazyboy@ on this, since he's doing almost the same
> thing
> > in https://chromiumcodereview.appspot.com/23856006/.
> > 
> > Either way, this should probably go earlier, closer to the ERR_ABORTED
return
> > statement.
> 
> We still want to tell the viewhost about the error in the ntp case so
> that it can initiate a redirect to the local page.

I'm not sure I understand your reply.  Which part of the code below line 3654 do
you need?

There's an existing comment about "messing up session history" on line 3667
which makes me wary about changing pending_navigation_params_ without calling
LoadNavigationErrorPage.

[Jered, 2013-09-16 22:30:46]

https://codereview.chromium.org/23455047/diff/4001/content/renderer/render_vi...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/23455047/diff/4001/content/renderer/render_vi...
content/renderer/render_view_impl.cc:3696: if
(GetContentClient()->renderer()->ShouldSuppressErrorPage(
On 2013/09/13 22:58:09, creis wrote:
> > > Please coordinate with lazyboy@ on this, since he's doing almost the same
> > thing
> > > in https://chromiumcodereview.appspot.com/23856006/.
> > > 
> > > Either way, this should probably go earlier, closer to the ERR_ABORTED
> return
> > > statement.
> > 
> > We still want to tell the viewhost about the error in the ntp case so
> > that it can initiate a redirect to the local page.
> 
> I'm not sure I understand your reply.  Which part of the code below line 3654
do
> you need?
> 
> There's an existing comment about "messing up session history" on line 3667
> which makes me wary about changing pending_navigation_params_ without calling
> LoadNavigationErrorPage.

Sorry, I confused myself, you're right. Moved the code up.

[samarth, 2013-09-17 20:50:59]

Overall this looks good to me. I just tried it out and it seems to work. I
remember you saying that it wasn't working for you; did I imagine that?

Thanks,
Samarth

https://codereview.chromium.org/23455047/diff/18001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/23455047/diff/18001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:260: rph->Send(new
ChromeViewMsg_SetSearchURLs(
You need to send these on template url updates as well, no?

[Jered, 2013-09-17 23:31:02]

PTAL, I added a lot of tests.
It was a little unclear how to test ChromeContentRendererClient, PS8 seemed like
the easiest way.

On 2013/09/17 20:50:59, samarth wrote:
> Overall this looks good to me. I just tried it out and it seems to work. I
> remember you saying that it wasn't working for you; did I imagine that?

This change works fine, but when I tried the steps in the bug (toggling
safesearch)
the srp redirected me to a non-espv page again. Maybe it doesn't do that after
some recent change?

> 
> Thanks,
> Samarth
> 
>
https://codereview.chromium.org/23455047/diff/18001/chrome/browser/search/ins...
> File chrome/browser/search/instant_service.cc (right):
> 
>
https://codereview.chromium.org/23455047/diff/18001/chrome/browser/search/ins...
> chrome/browser/search/instant_service.cc:260: rph->Send(new
> ChromeViewMsg_SetSearchURLs(
> You need to send these on template url updates as well, no?

[Charlie Reis, 2013-09-18 02:11:07]

Looks like lazyboy's https://chromiumcodereview.appspot.com/23856006/ is about
to land and conflict with your RenderViewImpl change.  I was hoping to avoid
having two different ways to suppress the error (renderer_preferences_ and a
ContentRendererClient method).  Perhaps you and lazyboy can coordinate to settle
on one approach that works for both situations?

Also, please have jam@ take a look at MockRenderThread, since I'm not sure I
understand it well enough to review it.

Apart from that, thanks for the thorough tests.

[lazyboy, 2013-09-18 08:00:17]

On 2013/09/18 02:11:07, creis wrote:
> Looks like lazyboy's https://chromiumcodereview.appspot.com/23856006/ is about
> to land and conflict with your RenderViewImpl change.  I was hoping to avoid
> having two different ways to suppress the error (renderer_preferences_ and a
> ContentRendererClient method).  Perhaps you and lazyboy can coordinate to
settle
> on one approach that works for both situations?
For my change related to BrowserPlugin, I needed to suppress the error looking
at error.reason and renderer_preferences_ as you mention. We do not have a
specific ContentRendererClient implementation for BrowserPlugin so we probably
can't move that suppression code entirely to ChromeContentRendererClient. We
probably should at least extract these suppression to a separate method:
RenderViewImpl::ShouldSuprressError(const WebURLError& error) {} and put both
suppressions there?
> 
> Also, please have jam@ take a look at MockRenderThread, since I'm not sure I
> understand it well enough to review it.
> 
> Apart from that, thanks for the thorough tests.

[jam, 2013-09-18 16:02:12]

content lgtm

[Charlie Reis, 2013-09-18 16:18:17]

On 2013/09/18 08:00:17, lazyboy wrote:
> On 2013/09/18 02:11:07, creis wrote:
> > Looks like lazyboy's https://chromiumcodereview.appspot.com/23856006/ is
about
> > to land and conflict with your RenderViewImpl change.  I was hoping to avoid
> > having two different ways to suppress the error (renderer_preferences_ and a
> > ContentRendererClient method).  Perhaps you and lazyboy can coordinate to
> settle
> > on one approach that works for both situations?
> For my change related to BrowserPlugin, I needed to suppress the error looking
> at error.reason and renderer_preferences_ as you mention. We do not have a
> specific ContentRendererClient implementation for BrowserPlugin so we probably
> can't move that suppression code entirely to ChromeContentRendererClient. We
> probably should at least extract these suppression to a separate method:
> RenderViewImpl::ShouldSuprressError(const WebURLError& error) {} and put both
> suppressions there?

Ah, I guess one caller is in content and the other is in chrome.  Ok, guess it
makes sense to have both.

[Jered, 2013-09-18 21:10:09]

PTAL jam/creis, it turns out that for ShouldFork() to work here, we need to
allow it on is_redirect as well as is_content_initiated.

Otherwise,
1. google.com/setprefs in an Instant render process bounces to browser.
2. Browser says google.com/setprefs is not an Instant url, assigns to
non-Instant renderer.
3. Now google.com/setprefs looks like a browser-initiated request, so does not
get a chance to bounce.
4. The redirect destination google.com/search is also marked browser-initiated.

With this PS, (4) will get another chance to fork because it is a redirect, even
though it's marked browser-initiated.

[Jered, 2013-09-18 21:34:52]

PTAL samarth, handled TemplateURL changes.

On 2013/09/17 20:50:59, samarth wrote:
> Overall this looks good to me. I just tried it out and it seems to work. I
> remember you saying that it wasn't working for you; did I imagine that?
> 
> Thanks,
> Samarth
> 
>
https://codereview.chromium.org/23455047/diff/18001/chrome/browser/search/ins...
> File chrome/browser/search/instant_service.cc (right):
> 
>
https://codereview.chromium.org/23455047/diff/18001/chrome/browser/search/ins...
> chrome/browser/search/instant_service.cc:260: rph->Send(new
> ChromeViewMsg_SetSearchURLs(
> You need to send these on template url updates as well, no?

[Jered, 2013-09-18 21:35:05]

https://codereview.chromium.org/23455047/diff/18001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/23455047/diff/18001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:260: rph->Send(new
ChromeViewMsg_SetSearchURLs(
On 2013/09/17 20:51:00, samarth wrote:
> You need to send these on template url updates as well, no?

Done.

[Charlie Reis, 2013-09-18 22:05:23]

On 2013/09/18 21:10:09, Jered wrote:
> PTAL jam/creis, it turns out that for ShouldFork() to work here, we need to
> allow it on is_redirect as well as is_content_initiated.
> 
> Otherwise,
> 1. google.com/setprefs in an Instant render process bounces to browser.
> 2. Browser says google.com/setprefs is not an Instant url, assigns to
> non-Instant renderer.
> 3. Now google.com/setprefs looks like a browser-initiated request, so does not
> get a chance to bounce.

Why is this marked as a browser-initiated request?  Can you give more details
about what you mean by that, and why it's happening?

> 4. The redirect destination google.com/search is also marked
browser-initiated.
> 
> With this PS, (4) will get another chance to fork because it is a redirect,
even
> though it's marked browser-initiated.

[Jered, 2013-09-18 22:59:08]

On 2013/09/18 22:05:23, creis wrote:
> On 2013/09/18 21:10:09, Jered wrote:
> > PTAL jam/creis, it turns out that for ShouldFork() to work here, we need to
> > allow it on is_redirect as well as is_content_initiated.
> > 
> > Otherwise,
> > 1. google.com/setprefs in an Instant render process bounces to browser.
> > 2. Browser says google.com/setprefs is not an Instant url, assigns to
> > non-Instant renderer.
> > 3. Now google.com/setprefs looks like a browser-initiated request, so does
not
> > get a chance to bounce.
> 
> Why is this marked as a browser-initiated request?  Can you give more details
> about what you mean by that, and why it's happening?

In render_view_impl.cc:3169, is_content_initiated is false in the
forked renderer after the /setprefs request bounces into it. That is
because we get back to RenderViewImpl::OnNavigate() and call
frame->loadRequest() normally. is_renderer_initiated is tracked up
to WebContentsImpl::NavigateToEntry(), but then is dropped in
the ViewMsg_Navigate_Params IPC as far as I can tell.

This seems reasonable because the forked renderer did not in fact
initiate the request. There's another case where we check
is_content_initiated || is_redirect above at render_view_impl.cc:3183,
so I assumed this had already been considered when that logic was
added.

> 
> > 4. The redirect destination google.com/search is also marked
> browser-initiated.
> > 
> > With this PS, (4) will get another chance to fork because it is a redirect,
> even
> > though it's marked browser-initiated.

[Charlie Reis, 2013-09-20 00:18:42]

On 2013/09/18 22:59:08, Jered wrote:
> On 2013/09/18 22:05:23, creis wrote:
> > On 2013/09/18 21:10:09, Jered wrote:
> > > PTAL jam/creis, it turns out that for ShouldFork() to work here, we need
to
> > > allow it on is_redirect as well as is_content_initiated.
> > > 
> > > Otherwise,
> > > 1. google.com/setprefs in an Instant render process bounces to browser.
> > > 2. Browser says google.com/setprefs is not an Instant url, assigns to
> > > non-Instant renderer.
> > > 3. Now google.com/setprefs looks like a browser-initiated request, so does
> not
> > > get a chance to bounce.
> > 
> > Why is this marked as a browser-initiated request?  Can you give more
details
> > about what you mean by that, and why it's happening?
> 
> In render_view_impl.cc:3169, is_content_initiated is false in the
> forked renderer after the /setprefs request bounces into it. That is
> because we get back to RenderViewImpl::OnNavigate() and call
> frame->loadRequest() normally. is_renderer_initiated is tracked up
> to WebContentsImpl::NavigateToEntry(), but then is dropped in
> the ViewMsg_Navigate_Params IPC as far as I can tell.
> 
> This seems reasonable because the forked renderer did not in fact
> initiate the request. There's another case where we check
> is_content_initiated || is_redirect above at render_view_impl.cc:3183,
> so I assumed this had already been considered when that logic was
> added.
> 

Ok.  content LGTM.

[samarth, 2013-09-20 16:43:39]

Looks good for search. Just a couple of nits and requests for tests.

Thanks,
Samarth

https://codereview.chromium.org/23455047/diff/40001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/23455047/diff/40001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:260: void
InstantService::SendSearchURLsToRenderer(content::RenderProcessHost* rph) {
Test please. Anuj's pending change adds a unit test framework for InstantService
so I'm OK if you want to wait for that to land and then add a test after.

https://codereview.chromium.org/23455047/diff/40001/chrome/browser/search/sea...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/23455047/diff/40001/chrome/browser/search/sea...
chrome/browser/search/search.cc:433: std::vector<GURL> GetSearchURLs(Profile*
profile) {
Test?

https://codereview.chromium.org/23455047/diff/40001/chrome/browser/search/sea...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/23455047/diff/40001/chrome/browser/search/sea...
chrome/browser/search/search.h:116: // Returns Instant URLs associated with the
default search engine for |profile|.
nit: can we not call them Instant URLs? I'd like to slowly rid of Instant where
it's not actually specifically talking about Instant.

https://codereview.chromium.org/23455047/diff/40001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/23455047/diff/40001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:305: // Send new search URLs to
the renderer.
I assume this will go in before https://codereview.chromium.org/20388003/. 
Please coordinate with Anuj to make sure this doesn't get lost in the merge. 
With his change, we should also send an IPC if the google base URL changes.

https://codereview.chromium.org/23455047/diff/40001/chrome/common/search_urls.cc
File chrome/common/search_urls.cc (right):

https://codereview.chromium.org/23455047/diff/40001/chrome/common/search_urls...
chrome/common/search_urls.cc:10: namespace search {
I think we should consider moving all of the utility stuff in search.h to be
under search.

FYI, see
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/namesp....
I didn't see a firm conclusion there, but I think for this case, this is fine.

https://codereview.chromium.org/23455047/diff/40001/chrome/common/search_urls...
chrome/common/search_urls.cc:22: bool MatchesOriginAndPath(const GURL& my_url,
const GURL& other_url) {
Unit test please.

[Jered, 2013-09-20 20:55:48]

palmer/jschuh, do either of you want to look at this patch? We went back and
forth on this several months ago.

https://codereview.chromium.org/23455047/diff/40001/chrome/browser/search/ins...
File chrome/browser/search/instant_service.cc (right):

https://codereview.chromium.org/23455047/diff/40001/chrome/browser/search/ins...
chrome/browser/search/instant_service.cc:260: void
InstantService::SendSearchURLsToRenderer(content::RenderProcessHost* rph) {
On 2013/09/20 16:43:40, samarth wrote:
> Test please. Anuj's pending change adds a unit test framework for
InstantService
> so I'm OK if you want to wait for that to land and then add a test after.

I added a simple test. I'll merge up to Anuj's change if that goes in first.

https://codereview.chromium.org/23455047/diff/40001/chrome/browser/search/sea...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/23455047/diff/40001/chrome/browser/search/sea...
chrome/browser/search/search.cc:433: std::vector<GURL> GetSearchURLs(Profile*
profile) {
On 2013/09/20 16:43:40, samarth wrote:
> Test?

Done.

https://codereview.chromium.org/23455047/diff/40001/chrome/browser/search/sea...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/23455047/diff/40001/chrome/browser/search/sea...
chrome/browser/search/search.h:116: // Returns Instant URLs associated with the
default search engine for |profile|.
On 2013/09/20 16:43:40, samarth wrote:
> nit: can we not call them Instant URLs? I'd like to slowly rid of Instant
where
> it's not actually specifically talking about Instant.

Done.

https://codereview.chromium.org/23455047/diff/40001/chrome/browser/ui/browser...
File chrome/browser/ui/browser_instant_controller.cc (right):

https://codereview.chromium.org/23455047/diff/40001/chrome/browser/ui/browser...
chrome/browser/ui/browser_instant_controller.cc:305: // Send new search URLs to
the renderer.
On 2013/09/20 16:43:40, samarth wrote:
> I assume this will go in before https://codereview.chromium.org/20388003/. 
> Please coordinate with Anuj to make sure this doesn't get lost in the merge. 
> With his change, we should also send an IPC if the google base URL changes.

Ack.

https://codereview.chromium.org/23455047/diff/40001/chrome/common/search_urls.cc
File chrome/common/search_urls.cc (right):

https://codereview.chromium.org/23455047/diff/40001/chrome/common/search_urls...
chrome/common/search_urls.cc:10: namespace search {
On 2013/09/20 16:43:40, samarth wrote:
> I think we should consider moving all of the utility stuff in search.h to be
> under search.
> 
> FYI, see
>
https://groups.google.com/a/chromium.org/forum/#%21searchin/chromium-dev/name....
> I didn't see a firm conclusion there, but I think for this case, this is fine.

I agree. I don't think the grab bag of functions exposed from search.h should go
into the global namespace, and "search" is a more meaningful grouping than
"chrome" for these functions. I prefer to take that on in a separate CL though.

https://codereview.chromium.org/23455047/diff/40001/chrome/common/search_urls...
chrome/common/search_urls.cc:22: bool MatchesOriginAndPath(const GURL& my_url,
const GURL& other_url) {
On 2013/09/20 16:43:40, samarth wrote:
> Unit test please.

Done.

[samarth, 2013-09-20 20:58:28]

lgtm

[Jered, 2013-09-20 20:58:38]

jhawkins PTAL at chrome/renderer miscellany and webui

[Jered, 2013-09-23 22:06:01]

jhawkins ping?
palmer+jschuh to CC.

[Jered, 2013-09-25 18:25:00]

+sky -> can you PTAL at some renderer* miscellany which has been OK'd for
content, and stamp for a trivial change in webui?
+tsepez for render_messages.h

[Tom Sepez, 2013-09-25 18:37:32]

Messages LGTM.

[sky, 2013-09-25 20:43:16]

https://codereview.chromium.org/23455047/diff/62001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/23455047/diff/62001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:236: if
(!extension_dispatcher_)
Why the ifs here and 244?

https://codereview.chromium.org/23455047/diff/62001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right):

https://codereview.chromium.org/23455047/diff/62001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client_browsertest.cc:17: class
InstantProcessNavigationTest : public ChromeRenderViewTest {
nit: use typedef

https://codereview.chromium.org/23455047/diff/62001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client_browsertest.cc:20:
TEST_F(InstantProcessNavigationTest, ForkForNavigationsFromInstantProcess) {
Add descriptions of what you're test is doing.

[Jered, 2013-09-25 22:23:37]

Thanks for the quick reviews

https://codereview.chromium.org/23455047/diff/62001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/23455047/diff/62001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:236: if
(!extension_dispatcher_)
On 2013/09/25 20:43:17, sky wrote:
> Why the ifs here and 244?

Added comments.

https://codereview.chromium.org/23455047/diff/62001/chrome/renderer/chrome_co...
File chrome/renderer/chrome_content_renderer_client_browsertest.cc (right):

https://codereview.chromium.org/23455047/diff/62001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client_browsertest.cc:17: class
InstantProcessNavigationTest : public ChromeRenderViewTest {
On 2013/09/25 20:43:17, sky wrote:
> nit: use typedef

Done.

https://codereview.chromium.org/23455047/diff/62001/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client_browsertest.cc:20:
TEST_F(InstantProcessNavigationTest, ForkForNavigationsFromInstantProcess) {
On 2013/09/25 20:43:17, sky wrote:
> Add descriptions of what you're test is doing.

Done.

[sky, 2013-09-25 22:34:09]

LGTM

[commit-bot: I haz the power, 2013-09-25 22:35:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jered@chromium.org/23455047/72001

[commit-bot: I haz the power, 2013-09-26 04:09:04]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[Jered, 2013-09-26 22:24:49]

On 2013/09/20 00:18:42, creis wrote:
> On 2013/09/18 22:59:08, Jered wrote:
> > On 2013/09/18 22:05:23, creis wrote:
> > > On 2013/09/18 21:10:09, Jered wrote:
> > > > PTAL jam/creis, it turns out that for ShouldFork() to work here, we need
> to
> > > > allow it on is_redirect as well as is_content_initiated.
> > > > 
> > > > Otherwise,
> > > > 1. google.com/setprefs in an Instant render process bounces to browser.
> > > > 2. Browser says google.com/setprefs is not an Instant url, assigns to
> > > > non-Instant renderer.
> > > > 3. Now google.com/setprefs looks like a browser-initiated request, so
does
> > not
> > > > get a chance to bounce.
> > > 
> > > Why is this marked as a browser-initiated request?  Can you give more
> details
> > > about what you mean by that, and why it's happening?
> > 
> > In render_view_impl.cc:3169, is_content_initiated is false in the
> > forked renderer after the /setprefs request bounces into it. That is
> > because we get back to RenderViewImpl::OnNavigate() and call
> > frame->loadRequest() normally. is_renderer_initiated is tracked up
> > to WebContentsImpl::NavigateToEntry(), but then is dropped in
> > the ViewMsg_Navigate_Params IPC as far as I can tell.
> > 
> > This seems reasonable because the forked renderer did not in fact
> > initiate the request. There's another case where we check
> > is_content_initiated || is_redirect above at render_view_impl.cc:3183,
> > so I assumed this had already been considered when that logic was
> > added.
> > 
> 
> Ok.  content LGTM.

creis, PTAL. I had to give up on adding the "is_redirect" check to
RenderViewImpl because it broke signin, which assumes
ContentRendererClient::ShouldFork()
will only ever be called on content-initiated requests (and otherwise,
grants redirects signin process status, which seems not ok).

I think this is the best I can do in this CL---it won't quite fix
crbug.com/223754, but it would do so if content-initiated requests,
with redirects, that got bounced in from another renderer, actually
still showed up as content-initiated. Plumbing that feels like a
separate step.

What do you think?

[Charlie Reis, 2013-09-27 16:42:58]

On 2013/09/26 22:24:49, Jered wrote:
> On 2013/09/20 00:18:42, creis wrote:
> > On 2013/09/18 22:59:08, Jered wrote:
> > > On 2013/09/18 22:05:23, creis wrote:
> > > > On 2013/09/18 21:10:09, Jered wrote:
> > > > > PTAL jam/creis, it turns out that for ShouldFork() to work here, we
need
> > to
> > > > > allow it on is_redirect as well as is_content_initiated.
> > > > > 
> > > > > Otherwise,
> > > > > 1. google.com/setprefs in an Instant render process bounces to
browser.
> > > > > 2. Browser says google.com/setprefs is not an Instant url, assigns to
> > > > > non-Instant renderer.
> > > > > 3. Now google.com/setprefs looks like a browser-initiated request, so
> does
> > > not
> > > > > get a chance to bounce.
> > > > 
> > > > Why is this marked as a browser-initiated request?  Can you give more
> > details
> > > > about what you mean by that, and why it's happening?
> > > 
> > > In render_view_impl.cc:3169, is_content_initiated is false in the
> > > forked renderer after the /setprefs request bounces into it. That is
> > > because we get back to RenderViewImpl::OnNavigate() and call
> > > frame->loadRequest() normally. is_renderer_initiated is tracked up
> > > to WebContentsImpl::NavigateToEntry(), but then is dropped in
> > > the ViewMsg_Navigate_Params IPC as far as I can tell.
> > > 
> > > This seems reasonable because the forked renderer did not in fact
> > > initiate the request. There's another case where we check
> > > is_content_initiated || is_redirect above at render_view_impl.cc:3183,
> > > so I assumed this had already been considered when that logic was
> > > added.
> > > 
> > 
> > Ok.  content LGTM.
> 
> creis, PTAL. I had to give up on adding the "is_redirect" check to
> RenderViewImpl because it broke signin, which assumes
> ContentRendererClient::ShouldFork()
> will only ever be called on content-initiated requests (and otherwise,
> grants redirects signin process status, which seems not ok).

[CC'ing rogerta@ and tim@]

Specifically, looks like it broke SigninBrowserTest.NotTrustedAfterRedirect. 
And yes, I seem to remember that the sign-in code was depending on not having
server redirects sent to the browser process.  Roger and Tim, is that technical
debt that needs to be paid down?

> I think this is the best I can do in this CL---it won't quite fix
> crbug.com/223754, but it would do so if content-initiated requests,
> with redirects, that got bounced in from another renderer, actually
> still showed up as content-initiated. Plumbing that feels like a
> separate step.
> 
> What do you think?

To be honest, I really don't understand your CL enough to know how this affects
it.  It looks like the CL description is potentially stale, and there's a lot
going on here.  I think I need a clearer explanation of what this CL is
currently trying to accomplish and what is left to be done in the future.

[Jered, 2013-09-27 22:16:22]

On 2013/09/27 16:42:58, creis wrote:
> On 2013/09/26 22:24:49, Jered wrote:
> > On 2013/09/20 00:18:42, creis wrote:
> > > On 2013/09/18 22:59:08, Jered wrote:
> > > > On 2013/09/18 22:05:23, creis wrote:
> > > > > On 2013/09/18 21:10:09, Jered wrote:
> > > > > > PTAL jam/creis, it turns out that for ShouldFork() to work here, we
> need
> > > to
> > > > > > allow it on is_redirect as well as is_content_initiated.
> > > > > > 
> > > > > > Otherwise,
> > > > > > 1. google.com/setprefs in an Instant render process bounces to
> browser.
> > > > > > 2. Browser says google.com/setprefs is not an Instant url, assigns
to
> > > > > > non-Instant renderer.
> > > > > > 3. Now google.com/setprefs looks like a browser-initiated request,
so
> > does
> > > > not
> > > > > > get a chance to bounce.
> > > > > 
> > > > > Why is this marked as a browser-initiated request?  Can you give more
> > > details
> > > > > about what you mean by that, and why it's happening?
> > > > 
> > > > In render_view_impl.cc:3169, is_content_initiated is false in the
> > > > forked renderer after the /setprefs request bounces into it. That is
> > > > because we get back to RenderViewImpl::OnNavigate() and call
> > > > frame->loadRequest() normally. is_renderer_initiated is tracked up
> > > > to WebContentsImpl::NavigateToEntry(), but then is dropped in
> > > > the ViewMsg_Navigate_Params IPC as far as I can tell.
> > > > 
> > > > This seems reasonable because the forked renderer did not in fact
> > > > initiate the request. There's another case where we check
> > > > is_content_initiated || is_redirect above at render_view_impl.cc:3183,
> > > > so I assumed this had already been considered when that logic was
> > > > added.
> > > > 
> > > 
> > > Ok.  content LGTM.
> > 
> > creis, PTAL. I had to give up on adding the "is_redirect" check to
> > RenderViewImpl because it broke signin, which assumes
> > ContentRendererClient::ShouldFork()
> > will only ever be called on content-initiated requests (and otherwise,
> > grants redirects signin process status, which seems not ok).
> 
> [CC'ing rogerta@ and tim@]
> 
> Specifically, looks like it broke SigninBrowserTest.NotTrustedAfterRedirect. 
> And yes, I seem to remember that the sign-in code was depending on not having
> server redirects sent to the browser process.  Roger and Tim, is that
technical
> debt that needs to be paid down?
> 
> > I think this is the best I can do in this CL---it won't quite fix
> > crbug.com/223754, but it would do so if content-initiated requests,
> > with redirects, that got bounced in from another renderer, actually
> > still showed up as content-initiated. Plumbing that feels like a
> > separate step.
> > 
> > What do you think?
> 
> To be honest, I really don't understand your CL enough to know how this
affects
> it.  It looks like the CL description is potentially stale, and there's a lot
> going on here.  I think I need a clearer explanation of what this CL is
> currently trying to accomplish and what is left to be done in the future.

Hey Charlie, I have updated the CL description to be much more detailed.
Does this make more sense?

[Charlie Reis, 2013-09-28 00:48:16]

On 2013/09/27 22:16:22, Jered wrote:
> Hey Charlie, I have updated the CL description to be much more detailed.
> Does this make more sense?

Thanks-- that's *much* clearer.  Side note: I wouldn't normally suggest such a
verbose CL description (compared to putting an explanation like that on the bug
report), but it seems worthwhile in this case.

This CL LGTM, and we can investigate the right answer for redirects separately. 
I'm guessing we'll want to revisit the sign-in restriction to see if we can lift
it, since that's going to have to go away anyway.

[Jered, 2013-09-30 17:04:56]

On 2013/09/28 00:48:16, creis wrote:
> On 2013/09/27 22:16:22, Jered wrote:
> > Hey Charlie, I have updated the CL description to be much more detailed.
> > Does this make more sense?
> 
> Thanks-- that's *much* clearer.  Side note: I wouldn't normally suggest such a
> verbose CL description (compared to putting an explanation like that on the
bug
> report), but it seems worthwhile in this case.
> 
> This CL LGTM, and we can investigate the right answer for redirects
separately. 
> I'm guessing we'll want to revisit the sign-in restriction to see if we can
lift
> it, since that's going to have to go away anyway.

Great, thanks.

The remaining test failures look bogus but I'll try CQ again
before dcommitting because this is a large CL.

[commit-bot: I haz the power, 2013-09-30 17:08:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jered@chromium.org/23455047/156001

[commit-bot: I haz the power, 2013-10-01 01:33:38]

Change committed as 226103