Add chrome-search: access from Instant overlay

Add chrome-search: access from Instant overlay

Enables the Instant NTP page access to:
  chrome-search://theme/*,
  chrome-search://favicon/*, and
  chrome-search://thumb/*.

This CL has a number of aspects to make the installed theme
background image available to the Instant overlay:
   1. Utilizes the new chrome-search: scheme that is accessible only
      to the Instant overlay's render process.  This is enforced
      on the renderer side by WebKit::WebSecurityPolicy
      facilities.  This is enforced on the browser side by the
      InstantIOContext::ShouldServiceRequest and URLDataSource::ShouldServiceRequest facilities.
   2. Whitelists the chrome-search://theme origin to be made
      available to the specific Instant overlay's origin/url.
   3. Backend CSS that sets the appropriate background image
      that resides in the chrome-search://theme origin.
   4. A small refactoring of the Instant tests to support mix-in
      usage of InstantTestBase.

BUG=172408, 134937
TEST=InstantPolicyTest.SearchSchemePolicy
R=sreeram@chromium.org, creis@chromium.org


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

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

[dhollowa, 2013-01-28 17:18:42]

sreeram -> Instant
creis   -> browser and webkit policy changes

jschuh  -> OWNERS chrome/common/render_messages.h
joi     -> OWNERS content/public/
sky     -> OWNERS chrome/browser/... <misc>

[sreeram, 2013-01-29 16:04:34]

https://codereview.chromium.org/11896113/diff/11004/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11896113/diff/11004/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:214: GrantAccessToChromeSearch();
Note to Shishir: This also needs to be moved to RenderViewCreated(), in
https://codereview.chromium.org/11833043/.

https://codereview.chromium.org/11896113/diff/11004/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:431:
client_.SendGrantAccessFromOrigin(instant_url_);
Hmm... What happens if the URL redirects (say google.com -> google.ca)?

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

https://codereview.chromium.org/11896113/diff/11004/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:231: // chrome:,
chrome-devtools:, and chrome-internal: pages should not be
Update comment to include "chrome-search:".

https://codereview.chromium.org/11896113/diff/11004/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11896113/diff/11004/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:279:
ASCIIToUTF16(chrome::kChromeSearchScheme),
Consider making the above two objects just once and reusing them in all the
calls.

https://codereview.chromium.org/11896113/diff/11004/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:292: }
I forgot to ask in your earlier CL that introduced the dedicated Instant
process: How do we know that a random webpage won't get assigned to the Instant
process (if the user has hit the render process limit)?

Not that it matters here (since you are restricting by origin), but I was
wondering if that page would then be able to do any shenanigans with the
searchbox API.

[dhollowa, 2013-01-29 21:51:05]

https://codereview.chromium.org/11896113/diff/11004/chrome/browser/instant/in...
File chrome/browser/instant/instant_loader.cc (right):

https://codereview.chromium.org/11896113/diff/11004/chrome/browser/instant/in...
chrome/browser/instant/instant_loader.cc:431:
client_.SendGrantAccessFromOrigin(instant_url_);
On 2013/01/29 16:04:34, sreeram wrote:
> Hmm... What happens if the URL redirects (say http://google.com -> google.ca)?

Yes, redirects don't work yet.  As we discussed, Shishir will tackle solving the
redirect problem wholesale with crbug.com/172909.

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

https://codereview.chromium.org/11896113/diff/11004/chrome/renderer/chrome_co...
chrome/renderer/chrome_content_renderer_client.cc:231: // chrome:,
chrome-devtools:, and chrome-internal: pages should not be
On 2013/01/29 16:04:34, sreeram wrote:
> Update comment to include "chrome-search:".

Done.

https://codereview.chromium.org/11896113/diff/11004/chrome/renderer/searchbox...
File chrome/renderer/searchbox/searchbox.cc (right):

https://codereview.chromium.org/11896113/diff/11004/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:279:
ASCIIToUTF16(chrome::kChromeSearchScheme),
On 2013/01/29 16:04:34, sreeram wrote:
> Consider making the above two objects just once and reusing them in all the
> calls.

Done.

https://codereview.chromium.org/11896113/diff/11004/chrome/renderer/searchbox...
chrome/renderer/searchbox/searchbox.cc:292: }
On 2013/01/29 16:04:34, sreeram wrote:
> I forgot to ask in your earlier CL that introduced the dedicated Instant
> process: How do we know that a random webpage won't get assigned to the
Instant
> process (if the user has hit the render process limit)?
> 
> Not that it matters here (since you are restricting by origin), but I was
> wondering if that page would then be able to do any shenanigans with the
> searchbox API.

From my understanding the predicate
ChromeContentBrowserClient::ShouldUseProcessPerSite governs whether a given
process should be reserved for urls that conform to a particular "site" grouping
via their "effective" url.  This is what we did for the Instant process so
implicitly this process is only ever used for Instant urls, even in the case
where the --render-process-limit=3 say (i.e. the limit is exceeded).

I tested this manually with --render-process-limit=3 and my statements above
seem to hold true.

That said, I'm having trouble putting my finger on the exact logic that enforces
this.  I'll pose this question to Charlie for the definitive answer.

[Charlie Reis, 2013-01-29 22:03:36]

I'm in MTV and busy with meetings/talks at the moment, but I'll try to take a
look tomorrow.

[dhollowa, 2013-01-29 22:15:31]

On 2013/01/29 21:51:05, dhollowa wrote:
>
https://codereview.chromium.org/11896113/diff/11004/chrome/browser/instant/in...
> File chrome/browser/instant/instant_loader.cc (right):
> 
>
https://codereview.chromium.org/11896113/diff/11004/chrome/browser/instant/in...
> chrome/browser/instant/instant_loader.cc:431:
> client_.SendGrantAccessFromOrigin(instant_url_);
> On 2013/01/29 16:04:34, sreeram wrote:
> > Hmm... What happens if the URL redirects (say http://google.com ->
google.ca)?
> 
> Yes, redirects don't work yet.  As we discussed, Shishir will tackle solving
the
> redirect problem wholesale with crbug.com/172909.
> 
>
https://codereview.chromium.org/11896113/diff/11004/chrome/renderer/chrome_co...
> File chrome/renderer/chrome_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/11896113/diff/11004/chrome/renderer/chrome_co...
> chrome/renderer/chrome_content_renderer_client.cc:231: // chrome:,
> chrome-devtools:, and chrome-internal: pages should not be
> On 2013/01/29 16:04:34, sreeram wrote:
> > Update comment to include "chrome-search:".
> 
> Done.
> 
>
https://codereview.chromium.org/11896113/diff/11004/chrome/renderer/searchbox...
> File chrome/renderer/searchbox/searchbox.cc (right):
> 
>
https://codereview.chromium.org/11896113/diff/11004/chrome/renderer/searchbox...
> chrome/renderer/searchbox/searchbox.cc:279:
> ASCIIToUTF16(chrome::kChromeSearchScheme),
> On 2013/01/29 16:04:34, sreeram wrote:
> > Consider making the above two objects just once and reusing them in all the
> > calls.
> 
> Done.
> 
>
https://codereview.chromium.org/11896113/diff/11004/chrome/renderer/searchbox...
> chrome/renderer/searchbox/searchbox.cc:292: }
> On 2013/01/29 16:04:34, sreeram wrote:
> > I forgot to ask in your earlier CL that introduced the dedicated Instant
> > process: How do we know that a random webpage won't get assigned to the
> Instant
> > process (if the user has hit the render process limit)?
> > 
> > Not that it matters here (since you are restricting by origin), but I was
> > wondering if that page would then be able to do any shenanigans with the
> > searchbox API.
> 
> From my understanding the predicate
> ChromeContentBrowserClient::ShouldUseProcessPerSite governs whether a given
> process should be reserved for urls that conform to a particular "site"
grouping
> via their "effective" url.  This is what we did for the Instant process so
> implicitly this process is only ever used for Instant urls, even in the case
> where the --render-process-limit=3 say (i.e. the limit is exceeded).
> 
> I tested this manually with --render-process-limit=3 and my statements above
> seem to hold true.
> 
> That said, I'm having trouble putting my finger on the exact logic that
enforces
> this.  I'll pose this question to Charlie for the definitive answer.

Ah, found it... SiteInstance::GetProcess() eventually calls
RPH::ShouldTryToUseExistingProcessHost() and then on to CCBC::IsSuitableHost()
which we've made to return false for the Instant process.

[sreeram, 2013-01-30 18:24:53]

https://codereview.chromium.org/11896113/diff/4009/chrome/browser/instant/ins...
File chrome/browser/instant/instant_browsertest.cc (right):

https://codereview.chromium.org/11896113/diff/4009/chrome/browser/instant/ins...
chrome/browser/instant/instant_browsertest.cc:63: // Instant overly.  This is a
subclass of |ExtensionBrowserTest| because it
overly -> overlay

https://codereview.chromium.org/11896113/diff/4009/chrome/browser/instant/ins...
chrome/browser/instant/instant_browsertest.cc:102: content::RenderViewHost*
render_view_host() {
No need for this accessor. content::ExecuteScript...() accepts WebContents, so
at the call sites, you can just directly do something like this:

LoadThemeImage(chrome::GetActiveWebContents(browser()), ...);
...
LoadThemeImage(instant()->GetPreviewContents(), ...);

https://codereview.chromium.org/11896113/diff/4009/chrome/browser/instant/ins...
File chrome/browser/instant/instant_test_utils.h (right):

https://codereview.chromium.org/11896113/diff/4009/chrome/browser/instant/ins...
chrome/browser/instant/instant_test_utils.h:19: #include
"chrome/test/base/in_process_browser_test.h"
Remove these includes:
in_process_browser_test.h
instant_controller.h
omnibox_view.h

Add these includes:
basictypes.h
compiler_specific.h
gurl.h

And add these forward declarations:
content::WebContents
OmniboxView
InstantController
InstantModel

https://codereview.chromium.org/11896113/diff/4009/chrome/browser/instant/ins...
chrome/browser/instant/instant_test_utils.h:60: const GURL& instant_url() {
return instant_url_; }
instant_url() const {

[dhollowa, 2013-01-30 18:56:41]

https://codereview.chromium.org/11896113/diff/4009/chrome/browser/instant/ins...
File chrome/browser/instant/instant_browsertest.cc (right):

https://codereview.chromium.org/11896113/diff/4009/chrome/browser/instant/ins...
chrome/browser/instant/instant_browsertest.cc:63: // Instant overly.  This is a
subclass of |ExtensionBrowserTest| because it
On 2013/01/30 18:24:53, sreeram wrote:
> overly -> overlay

Done.

https://codereview.chromium.org/11896113/diff/4009/chrome/browser/instant/ins...
chrome/browser/instant/instant_browsertest.cc:102: content::RenderViewHost*
render_view_host() {
On 2013/01/30 18:24:53, sreeram wrote:
> No need for this accessor. content::ExecuteScript...() accepts WebContents, so
> at the call sites, you can just directly do something like this:
> 
> LoadThemeImage(chrome::GetActiveWebContents(browser()), ...);
> ...
> LoadThemeImage(instant()->GetPreviewContents(), ...);

Done.

https://codereview.chromium.org/11896113/diff/4009/chrome/browser/instant/ins...
File chrome/browser/instant/instant_test_utils.h (right):

https://codereview.chromium.org/11896113/diff/4009/chrome/browser/instant/ins...
chrome/browser/instant/instant_test_utils.h:19: #include
"chrome/test/base/in_process_browser_test.h"
On 2013/01/30 18:24:53, sreeram wrote:
> Remove these includes:
> in_process_browser_test.h
> instant_controller.h
> omnibox_view.h
> 
> Add these includes:
> basictypes.h
> compiler_specific.h
> gurl.h
> 
> And add these forward declarations:
> content::WebContents
> OmniboxView
> InstantController
> InstantModel

Done.

https://codereview.chromium.org/11896113/diff/4009/chrome/browser/instant/ins...
chrome/browser/instant/instant_test_utils.h:60: const GURL& instant_url() {
return instant_url_; }
On 2013/01/30 18:24:53, sreeram wrote:
> instant_url() const {

Done.

[sreeram, 2013-01-30 19:09:46]

lgtm

https://codereview.chromium.org/11896113/diff/11005/chrome/browser/instant/in...
File chrome/browser/instant/instant_test_utils.h (right):

https://codereview.chromium.org/11896113/diff/11005/chrome/browser/instant/in...
chrome/browser/instant/instant_test_utils.h:13: #include "base/run_loop.h"
Please sort.

https://codereview.chromium.org/11896113/diff/11005/chrome/browser/instant/in...
chrome/browser/instant/instant_test_utils.h:14: #include
"chrome/browser/instant/instant_controller.h"
No need for this include.

https://codereview.chromium.org/11896113/diff/11005/chrome/browser/instant/in...
chrome/browser/instant/instant_test_utils.h:24: class InstantModel;
Please sort.

[dhollowa, 2013-01-30 19:31:21]

https://codereview.chromium.org/11896113/diff/11005/chrome/browser/instant/in...
File chrome/browser/instant/instant_test_utils.h (right):

https://codereview.chromium.org/11896113/diff/11005/chrome/browser/instant/in...
chrome/browser/instant/instant_test_utils.h:13: #include "base/run_loop.h"
On 2013/01/30 19:09:46, sreeram wrote:
> Please sort.

Done.  ...embarassing...

https://codereview.chromium.org/11896113/diff/11005/chrome/browser/instant/in...
chrome/browser/instant/instant_test_utils.h:14: #include
"chrome/browser/instant/instant_controller.h"
On 2013/01/30 19:09:46, sreeram wrote:
> No need for this include.

Done.

https://codereview.chromium.org/11896113/diff/11005/chrome/browser/instant/in...
chrome/browser/instant/instant_test_utils.h:24: class InstantModel;
On 2013/01/30 19:09:46, sreeram wrote:
> Please sort.

Done.

[dhollowa, 2013-01-30 19:43:27]

jschuh  -> OWNERS chrome/common/render_messages.h
joi     -> OWNERS content/public/
sky     -> OWNERS chrome/browser/... <misc>

[jschuh, 2013-01-30 21:07:12]

I'll probably just end up rubberstamping Charlie's review after he okays it,
because he has a far better understanding of the security impact here than I do.

[sky, 2013-01-30 23:05:57]

On Wed, Jan 30, 2013 at 11:43 AM,  <dhollowa@chromium.org> wrote:
> jschuh  -> OWNERS chrome/common/render_messages.h
> joi     -> OWNERS content/public/
> sky     -> OWNERS chrome/browser/... <misc>

It looks like there are only instant files in chrome/browser, which
Sreeram approved. Are there other files you need me to review?

  -Scott

[dhollowa, 2013-01-30 23:41:03]

On 2013/01/30 23:05:57, sky wrote:
> On Wed, Jan 30, 2013 at 11:43 AM,  <mailto:dhollowa@chromium.org> wrote:
> > jschuh  -> OWNERS chrome/common/render_messages.h
> > joi     -> OWNERS content/public/
> > sky     -> OWNERS chrome/browser/... <misc>
> 
> It looks like there are only instant files in chrome/browser, which
> Sreeram approved. Are there other files you need me to review?
> 
>   -Scott

Oops, no.  Sorry.  The presubmit check tricked me with your name.

Instead:

estade@chromium.org -> OWNERS content/browser/webui/...

[sreeram, 2013-01-30 23:57:08]

https://codereview.chromium.org/11896113/diff/8006/chrome/browser/instant/ins...
File chrome/browser/instant/instant_browsertest.cc (right):

https://codereview.chromium.org/11896113/diff/8006/chrome/browser/instant/ins...
chrome/browser/instant/instant_browsertest.cc:1094:
chrome::GetActiveWebContents(browser())->GetRenderViewHost();
Heads up: @avi removed this in http://crrev.com/179466 yesterday.

[dhollowa, 2013-01-31 00:51:56]

https://codereview.chromium.org/11896113/diff/8006/chrome/browser/instant/ins...
File chrome/browser/instant/instant_browsertest.cc (right):

https://codereview.chromium.org/11896113/diff/8006/chrome/browser/instant/ins...
chrome/browser/instant/instant_browsertest.cc:1094:
chrome::GetActiveWebContents(browser())->GetRenderViewHost();
On 2013/01/30 23:57:08, sreeram wrote:
> Heads up: @avi removed this in http://crrev.com/179466 yesterday.

Done.  Thanks!  Rebased.

[Jói, 2013-01-31 11:37:51]

+jam

It feels a bit wrong to have content know about Instant in this way.  I guess
there is precedent with the chrome:// and devtools specific URI schemes, but I'm
wondering if thought has been given to accomplishing this in a more generic way.
 Imagine if the embedder (Chrome) wants to add two or three more such schemas in
the next several months, would we really want to update content each time?  Is
there anything in how these URIs are handled that's different from how e.g.
chrome:// and devtools are handled?

Cheers,
Jói

[dhollowa, 2013-01-31 16:31:29]

On 2013/01/31 11:37:51, Jói wrote:
> +jam
> 
> It feels a bit wrong to have content know about Instant in this way.  I guess
> there is precedent with the chrome:// and devtools specific URI schemes, but
I'm
> wondering if thought has been given to accomplishing this in a more generic
way.
>  Imagine if the embedder (Chrome) wants to add two or three more such schemas
in
> the next several months, would we really want to update content each time?  Is
> there anything in how these URIs are handled that's different from how e.g.
> chrome:// and devtools are handled?
> 
> Cheers,
> Jói

These are good points.  However, I'd argue that this isn't so much about having
content know about Instant, per say.  Rather, this chrome-search: scheme is
about exposing select bits of local data to the open web.  This is a fairly
general concept.  Chrome is choosing to lock access down to only the Instant
feature currently, but that is the embedder-specific choice.

In this sense, maybe the choice of the scheme name is a bit off, and that
something more like "chrome-ext:" is a better choice.

[Jói, 2013-01-31 17:06:28]

Thanks David. With the different naming and your explanation it sounds
more reasonable.

I think we should get John's feedback as well before you go ahead.

Cheers,
Jói


On Thu, Jan 31, 2013 at 4:31 PM,  <dhollowa@chromium.org> wrote:
> On 2013/01/31 11:37:51, Jói wrote:
>>
>> +jam
>
>
>> It feels a bit wrong to have content know about Instant in this way.  I
>> guess
>> there is precedent with the chrome:// and devtools specific URI schemes,
>> but
>
> I'm
>>
>> wondering if thought has been given to accomplishing this in a more
>> generic
>
> way.
>>
>>   Imagine if the embedder (Chrome) wants to add two or three more such
>> schemas
>
> in
>>
>> the next several months, would we really want to update content each time?
>> Is
>> there anything in how these URIs are handled that's different from how
>> e.g.
>> chrome:// and devtools are handled?
>
>
>> Cheers,
>> Jói
>
>
> These are good points.  However, I'd argue that this isn't so much about
> having
> content know about Instant, per say.  Rather, this chrome-search: scheme is
> about exposing select bits of local data to the open web.  This is a fairly
> general concept.  Chrome is choosing to lock access down to only the Instant
> feature currently, but that is the embedder-specific choice.
>
> In this sense, maybe the choice of the scheme name is a bit off, and that
> something more like "chrome-ext:" is a better choice.
>
> https://codereview.chromium.org/11896113/

[jam, 2013-01-31 17:46:52]

I agree that this stuff doesn't belong in content. renaming chrome-search to
chrome-foo would just be masking the layering violation.

can't you do this without changes to content? i encourage you to find a way. i
don't have a block of time to look deeply into this to suggest one off the top
of my head.

note that chrome:// is in content to only service webuis that content layer know
about, i.e. devtools, tracing (soon), gpu, webrtc-internals, media-internals
etc. but content shouldn't know anything about webuis for chrome features.

On 2013/01/31 16:31:29, dhollowa wrote:
> On 2013/01/31 11:37:51, Jói wrote:
> > +jam
> > 
> > It feels a bit wrong to have content know about Instant in this way.  I
guess
> > there is precedent with the chrome:// and devtools specific URI schemes, but
> I'm
> > wondering if thought has been given to accomplishing this in a more generic
> way.
> >  Imagine if the embedder (Chrome) wants to add two or three more such
schemas
> in
> > the next several months, would we really want to update content each time? 
Is
> > there anything in how these URIs are handled that's different from how e.g.
> > chrome:// and devtools are handled?
> > 
> > Cheers,
> > Jói
> 
> These are good points.  However, I'd argue that this isn't so much about
having
> content know about Instant, per say.  Rather, this chrome-search: scheme is
> about exposing select bits of local data to the open web.  This is a fairly
> general concept.  Chrome is choosing to lock access down to only the Instant
> feature currently, but that is the embedder-specific choice.
> 
> In this sense, maybe the choice of the scheme name is a bit off, and that
> something more like "chrome-ext:" is a better choice.

[dhollowa, 2013-01-31 18:50:47]

Would you be ok with me landing the chrome-search: addition, as it stands, in
the short-term and take it as an follow-up task to put in place the content
separation?  This feels like a sizeable chunk of work to factor
child_process_security_policy*, bindings, etc.  And this CL is gating a bunch of
other work that is time-critical.

On 2013/01/31 17:46:52, jam wrote:
> I agree that this stuff doesn't belong in content. renaming chrome-search to
> chrome-foo would just be masking the layering violation.
> 
> can't you do this without changes to content? i encourage you to find a way. i
> don't have a block of time to look deeply into this to suggest one off the top
> of my head.
> 
> note that chrome:// is in content to only service webuis that content layer
know
> about, i.e. devtools, tracing (soon), gpu, webrtc-internals, media-internals
> etc. but content shouldn't know anything about webuis for chrome features.
> 
> On 2013/01/31 16:31:29, dhollowa wrote:
> > On 2013/01/31 11:37:51, Jói wrote:
> > > +jam
> > > 
> > > It feels a bit wrong to have content know about Instant in this way.  I
> guess
> > > there is precedent with the chrome:// and devtools specific URI schemes,
but
> > I'm
> > > wondering if thought has been given to accomplishing this in a more
generic
> > way.
> > >  Imagine if the embedder (Chrome) wants to add two or three more such
> schemas
> > in
> > > the next several months, would we really want to update content each time?

> Is
> > > there anything in how these URIs are handled that's different from how
e.g.
> > > chrome:// and devtools are handled?
> > > 
> > > Cheers,
> > > Jói
> > 
> > These are good points.  However, I'd argue that this isn't so much about
> having
> > content know about Instant, per say.  Rather, this chrome-search: scheme is
> > about exposing select bits of local data to the open web.  This is a fairly
> > general concept.  Chrome is choosing to lock access down to only the Instant
> > feature currently, but that is the embedder-specific choice.
> > 
> > In this sense, maybe the choice of the scheme name is a bit off, and that
> > something more like "chrome-ext:" is a better choice.

[jam, 2013-01-31 19:02:12]

On 2013/01/31 18:50:47, dhollowa wrote:
> Would you be ok with me landing the chrome-search: addition, as it stands, in
> the short-term and take it as an follow-up task to put in place the content
> separation?  This feels like a sizeable chunk of work to factor
> child_process_security_policy*, bindings, etc.  And this CL is gating a bunch
of
> other work that is time-critical.


no, I'd really prefer not. future cleanup is often low priority compared to
other tasks and gets bumped. also, after all the work that was done to properly
separate content from chrome, any steps backwards are something we always try to
avoid.

that's not to say that doing this without involving content should be a sizeable
amount of work. i.e. it's not clear to me that childprocessecuritypolicy needs
to change. it's hard for me to suggest a concrete way of doing this without
finding a chunk of time to look into this, so that's why i askyou if you can
investigate other ways of doing this. if nothing jumps up, we can chat tomorrow
in-person and go into the details of this (i'm wfsf today)

[dhollowa, 2013-01-31 19:53:08]

On 2013/01/31 19:02:12, jam wrote:
> 
> no, I'd really prefer not. future cleanup is often low priority compared to
> other tasks and gets bumped. also, after all the work that was done to
properly
> separate content from chrome, any steps backwards are something we always try
to
> avoid.
> 
> that's not to say that doing this without involving content should be a
sizeable
> amount of work. i.e. it's not clear to me that childprocessecuritypolicy needs
> to change. it's hard for me to suggest a concrete way of doing this without
> finding a chunk of time to look into this, so that's why i askyou if you can
> investigate other ways of doing this. if nothing jumps up, we can chat
tomorrow
> in-person and go into the details of this (i'm wfsf today)

Maybe we should back up a bit...  This chrome-search: scheme is really  just an
alias for chrome:, but restricted to serve specific local assets to the open
web.  In a sense, this is a mechanism to turn the browser into a mini-server
who's assets accessible from remote pages.  This seems like a general mechanism,
and really just a refinement of existing chrome: concepts that you say are
appropriate for inclusion in "content".  Does that make more sense?

[jam, 2013-01-31 20:03:47]

On Thu, Jan 31, 2013 at 11:53 AM, <dhollowa@chromium.org> wrote:

> On 2013/01/31 19:02:12, jam wrote:
>
>  no, I'd really prefer not. future cleanup is often low priority compared
>> to
>> other tasks and gets bumped. also, after all the work that was done to
>>
> properly
>
>> separate content from chrome, any steps backwards are something we always
>> try
>>
> to
>
>> avoid.
>>
>
>  that's not to say that doing this without involving content should be a
>>
> sizeable
>
>> amount of work. i.e. it's not clear to me that childprocessecuritypolicy
>> needs
>> to change. it's hard for me to suggest a concrete way of doing this
>> without
>> finding a chunk of time to look into this, so that's why i askyou if you
>> can
>> investigate other ways of doing this. if nothing jumps up, we can chat
>>
> tomorrow
>
>> in-person and go into the details of this (i'm wfsf today)
>>
>
> Maybe we should back up a bit...  This chrome-search: scheme is really
>  just an
> alias for chrome:, but restricted to serve specific local assets to the
> open
> web.


open web = open web platform?

Is this exposing webui pages to google search? If so, this is a feature
just for google search; this isn't in any spec afaik and not implemented by
any other browser. This is usually the criteria for how we decide what's in
content vs chrome.


>  In a sense, this is a mechanism to turn the browser into a mini-server
> who's assets accessible from remote pages.  This seems like a general
> mechanism,
> and really just a refinement of existing chrome: concepts that you say are
> appropriate for inclusion in "content".


webui's (i.e. the existing chrome://) aren't used for injecting stuff into
web pages, they're used to render webui's. Note that content only knows
about webui schemes which are used for its own functioning, usually for
developer pages like devtools, gpu etc.


>  Does that make more sense?
>
>
https://codereview.chromium.**org/11896113/<https://codereview.chromium.org/1...
>

[jam, 2013-01-31 20:04:45]

On Thu, Jan 31, 2013 at 12:03 PM, John Abd-El-Malek <jam@chromium.org>wrote:

>
>
> On Thu, Jan 31, 2013 at 11:53 AM, <dhollowa@chromium.org> wrote:
>
>> On 2013/01/31 19:02:12, jam wrote:
>>
>>  no, I'd really prefer not. future cleanup is often low priority compared
>>> to
>>> other tasks and gets bumped. also, after all the work that was done to
>>>
>> properly
>>
>>> separate content from chrome, any steps backwards are something we
>>> always try
>>>
>> to
>>
>>> avoid.
>>>
>>
>>  that's not to say that doing this without involving content should be a
>>>
>> sizeable
>>
>>> amount of work. i.e. it's not clear to me that childprocessecuritypolicy
>>> needs
>>> to change. it's hard for me to suggest a concrete way of doing this
>>> without
>>> finding a chunk of time to look into this, so that's why i askyou if you
>>> can
>>> investigate other ways of doing this. if nothing jumps up, we can chat
>>>
>> tomorrow
>>
>>> in-person and go into the details of this (i'm wfsf today)
>>>
>>
>> Maybe we should back up a bit...  This chrome-search: scheme is really
>>  just an
>> alias for chrome:, but restricted to serve specific local assets to the
>> open
>> web.
>
>
> open web = open web platform?
>
> Is this exposing webui pages to google search? If so, this is a feature
> just for google search; this isn't in any spec afaik and not implemented by
> any other browser. This is usually the criteria for how we decide what's in
> content vs chrome.
>

also, note that by design all the instant code is in chrome and not in
content.


>
>
>>  In a sense, this is a mechanism to turn the browser into a mini-server
>> who's assets accessible from remote pages.  This seems like a general
>> mechanism,
>> and really just a refinement of existing chrome: concepts that you say are
>> appropriate for inclusion in "content".
>
>
> webui's (i.e. the existing chrome://) aren't used for injecting stuff into
> web pages, they're used to render webui's. Note that content only knows
> about webui schemes which are used for its own functioning, usually for
> developer pages like devtools, gpu etc.
>
>
>>  Does that make more sense?
>>
>>
https://codereview.chromium.**org/11896113/<https://codereview.chromium.org/1...
>>
>
>

[dhollowa, 2013-01-31 20:44:06]

On 2013/01/31 20:04:45, jam wrote:
> On Thu, Jan 31, 2013 at 12:03 PM, John Abd-El-Malek <jam@chromium.org>wrote:
> 
> >
> >
> > On Thu, Jan 31, 2013 at 11:53 AM, <mailto:dhollowa@chromium.org> wrote:
> >
> >> On 2013/01/31 19:02:12, jam wrote:
> >>
> >>  no, I'd really prefer not. future cleanup is often low priority compared
> >>> to
> >>> other tasks and gets bumped. also, after all the work that was done to
> >>>
> >> properly
> >>
> >>> separate content from chrome, any steps backwards are something we
> >>> always try
> >>>
> >> to
> >>
> >>> avoid.
> >>>
> >>
> >>  that's not to say that doing this without involving content should be a
> >>>
> >> sizeable
> >>
> >>> amount of work. i.e. it's not clear to me that childprocessecuritypolicy
> >>> needs
> >>> to change. it's hard for me to suggest a concrete way of doing this
> >>> without
> >>> finding a chunk of time to look into this, so that's why i askyou if you
> >>> can
> >>> investigate other ways of doing this. if nothing jumps up, we can chat
> >>>
> >> tomorrow
> >>
> >>> in-person and go into the details of this (i'm wfsf today)
> >>>
> >>
> >> Maybe we should back up a bit...  This chrome-search: scheme is really
> >>  just an
> >> alias for chrome:, but restricted to serve specific local assets to the
> >> open
> >> web.
> >
> >
> > open web = open web platform?
> >

No, sorry, by "open web" I meant specific https: pages.

> > Is this exposing webui pages to google search? If so, this is a feature
> > just for google search; this isn't in any spec afaik and not implemented by
> > any other browser. This is usually the criteria for how we decide what's in
> > content vs chrome.

Not quite true.  Firstly we're not exposing webui pages, but rather specific
assets such as theme background images, favicons, and most-visited thumbnails. 
Secondly, this is not google-search specific but is in support of any
search-provider that implements the (open) searchbox api:
http://dev.chromium.org/searchbox and tangentially the public OpenSearch API
http://www.opensearch.org/Home.

It is true that no other browser implements this spec, but we're hoping to
change that by demonstrating its awesomeness and drive future adoption in other
browsers.  Also, we love to see other search-providers hook in too.

> >
> 
> also, note that by design all the instant code is in chrome and not in
> content.
> 

Agreed.  No contention there.  But Chrome's Instant feature is backing the
searchbox api which is general to other search-providers as mentioned.

> 
> >
> >
> >>  In a sense, this is a mechanism to turn the browser into a mini-server
> >> who's assets accessible from remote pages.  This seems like a general
> >> mechanism,
> >> and really just a refinement of existing chrome: concepts that you say are
> >> appropriate for inclusion in "content".
> >
> >
> > webui's (i.e. the existing chrome://) aren't used for injecting stuff into
> > web pages, they're used to render webui's. Note that content only knows
> > about webui schemes which are used for its own functioning, usually for
> > developer pages like devtools, gpu etc.
> >

The 1993 project is attempting to externalize some of this webui (NTP pages
specifically) in an search-provider neutral way.  To this end we need to make
available certain local assets (theme images, favicons, most-visited
thumbnails).  It is not tenable to serve these local assets, which can be quite
large, over the browser/renderer IPC channels.

[dhollowa, 2013-02-01 00:52:44]

John, I took a look at how I might move my chrome-search: solution on the Chrome
side.  And wanted to run a potential solution by you:

  1.  Move the protocol handler setup to
chrome/browser/profiles/profile_io_data.cc in
ProfileIOData::SetUpJobFactoryDefaults(), this is where chrome-extensions:
scheme is setup also.

  2.  Create my own ProtocolHandler and factory method:
CreateChromeSearchProtocolHandler() for instance.  This would largely wrap
URLDataManagerBackend stuff (hand-wave, hand-wave).  This may require exposing
some of the private-content implementation to a public place, so I can leverage
it.

 3. Instead of modifying the BINDINGS _POLICY_... stuff on the content side, I
would enforce my own browser-side restrictions in my ChromeSearchProtocolHandler
class.

I think I might be able to make that work.  It would mirror more closely what we
do with Extensions and may in the end give me more flexibility in enforcing my
own (evolving) policy controls.

Thoughts?

[jam, 2013-02-01 03:15:36]

On 2013/02/01 00:52:44, dhollowa wrote:
> John, I took a look at how I might move my chrome-search: solution on the
Chrome
> side.  And wanted to run a potential solution by you:
> 
>   1.  Move the protocol handler setup to
> chrome/browser/profiles/profile_io_data.cc in
> ProfileIOData::SetUpJobFactoryDefaults(), this is where chrome-extensions:
> scheme is setup also.

That sounds like a good idea.

> 
>   2.  Create my own ProtocolHandler and factory method:
> CreateChromeSearchProtocolHandler() for instance.  

ditto

> This would largely wrap
> URLDataManagerBackend stuff (hand-wave, hand-wave).  This may require exposing
> some of the private-content implementation to a public place, so I can
leverage
> it.

URLDataManagerBackend is meant to be hidden. since chrome-extensions gets away
without this in ExtensionProtocolHandler, can something similar be done?

> 
>  3. Instead of modifying the BINDINGS _POLICY_... stuff on the content side, I
> would enforce my own browser-side restrictions in my
ChromeSearchProtocolHandler
> class.
> 
> I think I might be able to make that work.  It would mirror more closely what
we
> do with Extensions and may in the end give me more flexibility in enforcing my
> own (evolving) policy controls.
> 
> Thoughts?

thanks for looking, I wasn't aware of how extensions did it. yes, if they found
a way to do it outside of content, doing something similar (and sharing code if
it makes sense) sgtm

[dhollowa, 2013-02-06 01:01:49]

On 2013/02/01 03:15:36, jam wrote:
> On 2013/02/01 00:52:44, dhollowa wrote:
> > John, I took a look at how I might move my chrome-search: solution on the
> Chrome
> > side.  And wanted to run a potential solution by you:
> > 
> ...

Hey John, here's a very preliminary cut on moving the chrome-search: protocol
out to its own "ChromeSearchProtocolHandler" and registering it on the
browser-side.

Please see c/b/instant/chrome_search_protocol* and how this hangs to gether. 
The chrome_search_protocol_unittest.cc shows how it is used.

Pros:
 - preserves the content/browser layering
 - give us more control on how we enforce browser-side (as opposed to
renderer-side) policy
 - reuses the existing URLDataSource subclasses such as ThemeSource,
FaviconSource, ThumbnailSource,
 - can be made to be simpler than the full URLDataManager* set of classes
 - keeps chrome-search: scheme which is necessary, we believe, from a sec point
of view

Cons:
 - code duplication/reimplementation w.r.t. URLDataManager*


Caveat: The CL as-posted is incomplete in a number of ways.  The browser-side
policy enforcement would need to be added to
instant_service/chrome_search_protocol*, and the theme/favicon/thumb backends
need to actually be registered.  That and a pile of "edge" case and thread
management that would need to be done properly.  Just wanted to get this out in
preliminary form so folks could comment on the approach.

Thanks.

[dhollowa, 2013-02-15 01:34:45]

Charlie, John, Sreeram.  I've uploaded changes that move the chrome-search:
scheme serving and policy check logic out of content and into
c/b/instant/chrome_search_protocol*.

This is a simplified version of URLDataManager* stuff.  It allows Instant to
enforce policy on the browser side in-sync with the WebKit enablements and
cross-origin checks on the renderer side.

PTAL.  I think this is working pretty nicely now.  Thanks.

[jam, 2013-02-15 01:49:56]

On 2013/02/15 01:34:45, dhollowa wrote:
> Charlie, John, Sreeram.  I've uploaded changes that move the chrome-search:
> scheme serving and policy check logic out of content and into
> c/b/instant/chrome_search_protocol*.
> 
> This is a simplified version of URLDataManager* stuff.  It allows Instant to
> enforce policy on the browser side in-sync with the WebKit enablements and
> cross-origin checks on the renderer side.
> 
> PTAL.  I think this is working pretty nicely now.  Thanks.

hmm. from your previous patch I thought there'd be ~100 lines of duplicated
code. now it looks like 250, and with more tricky logic. i'm a bit anxious
looking at this.

i will look at this more and give it some more thought.

[dhollowa, 2013-02-15 19:55:50]

On 2013/02/15 01:49:56, jam wrote:
> On 2013/02/15 01:34:45, dhollowa wrote:
> > Charlie, John, Sreeram.  I've uploaded changes that move the chrome-search:
> > scheme serving and policy check logic out of content and into
> > c/b/instant/chrome_search_protocol*.
> > 
> > This is a simplified version of URLDataManager* stuff.  It allows Instant to
> > enforce policy on the browser side in-sync with the WebKit enablements and
> > cross-origin checks on the renderer side.
> > 
> > PTAL.  I think this is working pretty nicely now.  Thanks.
> 
> hmm. from your previous patch I thought there'd be ~100 lines of duplicated
> code. now it looks like 250, and with more tricky logic. i'm a bit anxious
> looking at this.
> 
> i will look at this more and give it some more thought.

A bunch of the additional logic is in service of policy enforcement for the
Instant process(es).  This would need to be implemented independent of the
URLDataSource support.  So I think the code duplication is less than it might
appear at first.

I looked at possibly simplifying this by changing
ChromeContentRendererClient::WillSendRequest() but that had its own
complexities.  Encapsulating everything in ChromeSearchProtocolBackend seems
cleaner to me.

[jam, 2013-02-15 21:00:16]

On 2013/02/15 19:55:50, dhollowa wrote:
> On 2013/02/15 01:49:56, jam wrote:
> > On 2013/02/15 01:34:45, dhollowa wrote:
> > > Charlie, John, Sreeram.  I've uploaded changes that move the
chrome-search:
> > > scheme serving and policy check logic out of content and into
> > > c/b/instant/chrome_search_protocol*.
> > > 
> > > This is a simplified version of URLDataManager* stuff.  It allows Instant
to
> > > enforce policy on the browser side in-sync with the WebKit enablements and
> > > cross-origin checks on the renderer side.
> > > 
> > > PTAL.  I think this is working pretty nicely now.  Thanks.
> > 
> > hmm. from your previous patch I thought there'd be ~100 lines of duplicated
> > code. now it looks like 250, and with more tricky logic. i'm a bit anxious
> > looking at this.
> > 
> > i will look at this more and give it some more thought.
> 
> A bunch of the additional logic is in service of policy enforcement for the
> Instant process(es).  This would need to be implemented independent of the
> URLDataSource support.  So I think the code duplication is less than it might
> appear at first.
> 
> I looked at possibly simplifying this by changing
> ChromeContentRendererClient::WillSendRequest() but that had its own
> complexities.  Encapsulating everything in ChromeSearchProtocolBackend seems
> cleaner to me.

i'm ignoring the instant specific code, but lines 27-307, 331-381 seem like
duplicated code right? and all of chrome_search_protocol.cc of course, although
that's not too big relatively.

[dhollowa, 2013-02-15 21:33:14]

On 2013/02/15 21:00:16, jam wrote:
> On 2013/02/15 19:55:50, dhollowa wrote:
> > On 2013/02/15 01:49:56, jam wrote:
> > > On 2013/02/15 01:34:45, dhollowa wrote:
> > > > Charlie, John, Sreeram.  I've uploaded changes that move the
> chrome-search:
> > > > scheme serving and policy check logic out of content and into
> > > > c/b/instant/chrome_search_protocol*.
> > > > 
> > > > This is a simplified version of URLDataManager* stuff.  It allows
Instant
> to
> > > > enforce policy on the browser side in-sync with the WebKit enablements
and
> > > > cross-origin checks on the renderer side.
> > > > 
> > > > PTAL.  I think this is working pretty nicely now.  Thanks.
> > > 
> > > hmm. from your previous patch I thought there'd be ~100 lines of
duplicated
> > > code. now it looks like 250, and with more tricky logic. i'm a bit anxious
> > > looking at this.
> > > 
> > > i will look at this more and give it some more thought.
> > 
> > A bunch of the additional logic is in service of policy enforcement for the
> > Instant process(es).  This would need to be implemented independent of the
> > URLDataSource support.  So I think the code duplication is less than it
might
> > appear at first.
> > 
> > I looked at possibly simplifying this by changing
> > ChromeContentRendererClient::WillSendRequest() but that had its own
> > complexities.  Encapsulating everything in ChromeSearchProtocolBackend seems
> > cleaner to me.
> 
> i'm ignoring the instant specific code, but lines 27-307, 331-381 seem like
> duplicated code right? and all of chrome_search_protocol.cc of course,
although
> that's not too big relatively.

Yes, that's correct.

[dhollowa, 2013-02-19 21:56:16]

On 2013/02/15 21:33:14, dhollowa wrote:
> On 2013/02/15 21:00:16, jam wrote:
> > On 2013/02/15 19:55:50, dhollowa wrote:
> > > On 2013/02/15 01:49:56, jam wrote:
> > > > On 2013/02/15 01:34:45, dhollowa wrote:
> > > > > Charlie, John, Sreeram.  I've uploaded changes that move the
> > chrome-search:
> > > > > scheme serving and policy check logic out of content and into
> > > > > c/b/instant/chrome_search_protocol*.
> > > > > 
> > > > > This is a simplified version of URLDataManager* stuff.  It allows
> Instant
> > to
> > > > > enforce policy on the browser side in-sync with the WebKit enablements
> and
> > > > > cross-origin checks on the renderer side.
> > > > > 
> > > > > PTAL.  I think this is working pretty nicely now.  Thanks.
> > > > 
> > > > hmm. from your previous patch I thought there'd be ~100 lines of
> duplicated
> > > > code. now it looks like 250, and with more tricky logic. i'm a bit
anxious
> > > > looking at this.
> > > > 
> > > > i will look at this more and give it some more thought.
> > > 
> > > A bunch of the additional logic is in service of policy enforcement for
the
> > > Instant process(es).  This would need to be implemented independent of the
> > > URLDataSource support.  So I think the code duplication is less than it
> might
> > > appear at first.
> > > 
> > > I looked at possibly simplifying this by changing
> > > ChromeContentRendererClient::WillSendRequest() but that had its own
> > > complexities.  Encapsulating everything in ChromeSearchProtocolBackend
seems
> > > cleaner to me.
> > 
> > i'm ignoring the instant specific code, but lines 27-307, 331-381 seem like
> > duplicated code right? and all of chrome_search_protocol.cc of course,
> although
> > that's not too big relatively.
> 
> Yes, that's correct.

Summarizing offline discussion about this:

Due to discomfort with the parallel implementation of URLDataManagerBackend John
would like to pursue an approach that allows the embedder to:
 (1) Register additional WebUI schemes, and
 (2) Extend the URLDataSource interface to allow for custom security checks

I'll pursue that approach and retire the chrome_search_protocol* stuff.

[dhollowa, 2013-02-26 04:27:59]

jam     -> This now implements the content changes we discussed.

creis   -> The new instant_io_context.h/cc implements access control on the
browser side.  The renderer side is given access via
SearchBox::OnGrantAccessFromOrigin and its callers.

sreeram -> Some changes to c/b/instant/* since you last looked.

Thanks!

[jam, 2013-02-27 01:11:13]

Thanks David, this looks much nicer to me. I skipped the instant files. The rest
looks like it's mostly just changing the plumbing that pass the protocol
handlers. Great to see that the impact to content is minimal.

My only comment is regarding generalizing how the protocol handlers are passed.
I think that would simplify things further.

https://codereview.chromium.org/11896113/diff/44001/content/browser/webui/url...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/11896113/diff/44001/content/browser/webui/url...
content/browser/webui/url_data_manager_backend.cc:26: #include
"content/public/common/content_client.h"
nit: by convention we don't include content_client when including
content_*_client (that includes it)

https://codereview.chromium.org/11896113/diff/44001/content/browser/webui/url...
content/browser/webui/url_data_manager_backend.cc:52: #if !defined(NDEBUG)
the release builds on trybots are release-with-dchec, so i don't think this will
compile

https://codereview.chromium.org/11896113/diff/44001/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11896113/diff/44001/content/public/browser/co...
content/public/browser/content_browser_client.h:86: typedef std::map<
nit: please describe what the key is. also, by convention for the content api we
put typedefs beside the method that uses them

https://codereview.chromium.org/11896113/diff/44001/content/public/browser/co...
content/public/browser/content_browser_client.h:148: // act as aliases to the
chrome: scheme.
nit: the second sentance in the comment makes it seem that any scheme returned
here will serve webui pages. can you make it clear that anything here _may_
serve webui pages, but only if the URLDataSource implementations allow this by
overriding URLDataSource::ShouldServiceRequest

https://codereview.chromium.org/11896113/diff/44001/content/public/browser/co...
content/public/browser/content_browser_client.h:164: AdditionalWebUIHandlers*
additional_webui_handlers,
actually, why not make this method and the one below just take a map for all the
arguments (i.e. blob/file/developer)

[Charlie Reis, 2013-02-27 22:50:22]

Just a high level question or two at the moment, since I'm not sure I understand
what's happening here yet.

Why does this new scheme need to be considered WebUI?  I'm concerned about
things like leaking the ability to navigate to other WebUI pages or the ability
to use general WebUI bindings within the Instant process.  When web content gets
access to those, we've ended up with sandbox escapes in the past.  My impression
was that this just needed to be a regular scheme that was only made available to
the Instant process-- is there a reason it needs to be WebUI?

https://codereview.chromium.org/11896113/diff/44001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11896113/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:1078: profile_, new
FaviconSource(profile_, FaviconSource::FAVICON));
Why are these changes in this CL?  It's already a massive CL, and these seem
like unrelated cleanup.

https://codereview.chromium.org/11896113/diff/44001/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.h (right):

https://codereview.chromium.org/11896113/diff/44001/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:142: // origin access rights for select
origins.
nit: drop the first "origin", since it's more of a scheme check and ends up
being confusing with the other uses of "origin".

https://codereview.chromium.org/11896113/diff/44001/chrome/common/url_constan...
File chrome/common/url_constants.h (right):

https://codereview.chromium.org/11896113/diff/44001/chrome/common/url_constan...
chrome/common/url_constants.h:413: extern const char kChromeSearchScheme[];
I've been confused about this.  Why are we calling it chrome-search?  It doesn't
seem like it has any search-specific data.  Isn't it user-specific data from the
NTP, like most visited and history thumbnails?

We happen to be embedding the data in a page that has a search box on it, but
that seems orthogonal to what the scheme is named.  Is something like
chrome-ntp: or even chrome-instant: more accurate?

[jam, 2013-02-27 22:59:54]

On 2013/02/27 22:50:22, creis wrote:
> Just a high level question or two at the moment, since I'm not sure I
understand
> what's happening here yet.
> 
> Why does this new scheme need to be considered WebUI?  I'm concerned about
> things like leaking the ability to navigate to other WebUI pages or the
ability
> to use general WebUI bindings within the Instant process.  When web content
gets
> access to those, we've ended up with sandbox escapes in the past.  My
impression
> was that this just needed to be a regular scheme that was only made available
to
> the Instant process-- is there a reason it needs to be WebUI?

Here's how I understand it (Dave: feel free to correct me).

WebUI related classes are being reused since this change wants to reuse the
thumbnail, favicon and theme sources. However webui-bindings won't be "given" to
the process since afaik that's only used with chorme:// urls.

[Charlie Reis, 2013-02-27 23:02:43]

On 2013/02/27 22:59:54, jam wrote:
> On 2013/02/27 22:50:22, creis wrote:
> > Just a high level question or two at the moment, since I'm not sure I
> understand
> > what's happening here yet.
> > 
> > Why does this new scheme need to be considered WebUI?  I'm concerned about
> > things like leaking the ability to navigate to other WebUI pages or the
> ability
> > to use general WebUI bindings within the Instant process.  When web content
> gets
> > access to those, we've ended up with sandbox escapes in the past.  My
> impression
> > was that this just needed to be a regular scheme that was only made
available
> to
> > the Instant process-- is there a reason it needs to be WebUI?
> 
> Here's how I understand it (Dave: feel free to correct me).
> 
> WebUI related classes are being reused since this change wants to reuse the
> thumbnail, favicon and theme sources. However webui-bindings won't be "given"
to
> the process since afaik that's only used with chorme:// urls.

Thanks, John.  If that's true, we need to be very explicit about this, so that
someone doesn't later "clean up" the use of WebUI schemes by granting WebUI
bindings to this process.  We should both document that this doesn't get WebUI
bindings and test for it.

[dhollowa, 2013-02-28 00:00:37]

https://codereview.chromium.org/11896113/diff/44001/content/browser/webui/url...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/11896113/diff/44001/content/browser/webui/url...
content/browser/webui/url_data_manager_backend.cc:26: #include
"content/public/common/content_client.h"
On 2013/02/27 01:11:13, jam wrote:
> nit: by convention we don't include content_client when including
> content_*_client (that includes it)

Done.

https://codereview.chromium.org/11896113/diff/44001/content/browser/webui/url...
content/browser/webui/url_data_manager_backend.cc:52: #if !defined(NDEBUG)
On 2013/02/27 01:11:13, jam wrote:
> the release builds on trybots are release-with-dchec, so i don't think this
will
> compile

Done.

https://codereview.chromium.org/11896113/diff/44001/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11896113/diff/44001/content/public/browser/co...
content/public/browser/content_browser_client.h:86: typedef std::map<
On 2013/02/27 01:11:13, jam wrote:
> nit: please describe what the key is. also, by convention for the content api
we
> put typedefs beside the method that uses them

I added doc for the key.  But I left the decl up here because having
|content::ContentBrowserClient::ProtocolHandlerMap* protocol_handlers| in all
the method declarations/definitions was excessively verbose. 
|content::ProtocolHandlerMap* protocol_handlers| seemed more appropriate since
it is used outside of this class.  If you disagree I'll change it, but I don't
think you'll like the result.

https://codereview.chromium.org/11896113/diff/44001/content/public/browser/co...
content/public/browser/content_browser_client.h:148: // act as aliases to the
chrome: scheme.
On 2013/02/27 01:11:13, jam wrote:
> nit: the second sentance in the comment makes it seem that any scheme returned
> here will serve webui pages. can you make it clear that anything here _may_
> serve webui pages, but only if the URLDataSource implementations allow this by
> overriding URLDataSource::ShouldServiceRequest

Done.

https://codereview.chromium.org/11896113/diff/44001/content/public/browser/co...
content/public/browser/content_browser_client.h:164: AdditionalWebUIHandlers*
additional_webui_handlers,
On 2013/02/27 01:11:13, jam wrote:
> actually, why not make this method and the one below just take a map for all
the
> arguments (i.e. blob/file/developer)

Done.  It made sense to do all the arguments except |developer_protocol_handler|
because it is not used as a protocol but rather as an interceptor (so no
scheme).

There were some variants to account for, for example:
AwURLRequestContextGetter::GetURLRequestContext swaps in different file: and
data: handlers and ignores the ones passed in.  Not a problem, but something to
note.

[dhollowa, 2013-02-28 00:15:22]

On 2013/02/27 22:59:54, jam wrote:
> On 2013/02/27 22:50:22, creis wrote:
>[...]
>Thanks, John.  If that's true, we need to be very explicit
>about this, so that
>someone doesn't later "clean up" the use of WebUI schemes 
>by granting WebUI
>bindings to this process.  We should both document that 
>this doesn't get WebUI
>bindings and test for it.

Yes, John is right.  By "explicit" I was under the impression that partitioning
to the different "chrome-search:" scheme was explicit enough, since no bindings
would be in effect this way.

The new URLDataSource::ShouldServiceRequest is orthogonal to bindings, but gives
me the process_id that I need to enforce browser-side access control.

https://codereview.chromium.org/11896113/diff/44001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/11896113/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:1078: profile_, new
FaviconSource(profile_, FaviconSource::FAVICON));
On 2013/02/27 22:50:22, creis wrote:
> Why are these changes in this CL?  It's already a massive CL, and these seem
> like unrelated cleanup.

Done.  I'd made more substantive changes in previous iteration and thought I'd
keep the cleanup.  Reverted now.

https://codereview.chromium.org/11896113/diff/44001/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.h (right):

https://codereview.chromium.org/11896113/diff/44001/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:142: // origin access rights for select
origins.
On 2013/02/27 22:50:22, creis wrote:
> nit: drop the first "origin", since it's more of a scheme check and ends up
> being confusing with the other uses of "origin".

Done.

https://codereview.chromium.org/11896113/diff/44001/chrome/common/url_constan...
File chrome/common/url_constants.h (right):

https://codereview.chromium.org/11896113/diff/44001/chrome/common/url_constan...
chrome/common/url_constants.h:413: extern const char kChromeSearchScheme[];
On 2013/02/27 22:50:22, creis wrote:
> I've been confused about this.  Why are we calling it chrome-search?  It
doesn't
> seem like it has any search-specific data.  Isn't it user-specific data from
the
> NTP, like most visited and history thumbnails?
> 
> We happen to be embedding the data in a page that has a search box on it, but
> that seems orthogonal to what the scheme is named.  Is something like
> chrome-ntp: or even chrome-instant: more accurate?

No we'll be using chrome-search: assets for the in-page omnibox drop-down
suggestions as well.  It is not just NTP related.  And we have plans to rename
Instant->Search in general.  "Instant" is no longer accurate.

[jam, 2013-02-28 02:21:20]

https://codereview.chromium.org/11896113/diff/44001/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11896113/diff/44001/content/public/browser/co...
content/public/browser/content_browser_client.h:86: typedef std::map<
On 2013/02/28 00:00:37, dhollowa wrote:
> On 2013/02/27 01:11:13, jam wrote:
> > nit: please describe what the key is. also, by convention for the content
api
> we
> > put typedefs beside the method that uses them
> 
> I added doc for the key.  But I left the decl up here because having
> |content::ContentBrowserClient::ProtocolHandlerMap* protocol_handlers| in all
> the method declarations/definitions was excessively verbose. 
> |content::ProtocolHandlerMap* protocol_handlers| seemed more appropriate since
> it is used outside of this class.  If you disagree I'll change it, but I don't
> think you'll like the result.

sounds fine for now, especially since this is temporary until ajwong finishes
his refactoring

https://codereview.chromium.org/11896113/diff/44001/content/public/browser/co...
content/public/browser/content_browser_client.h:164: AdditionalWebUIHandlers*
additional_webui_handlers,
On 2013/02/28 00:00:37, dhollowa wrote:
> On 2013/02/27 01:11:13, jam wrote:
> > actually, why not make this method and the one below just take a map for all
> the
> > arguments (i.e. blob/file/developer)
> 
> Done.  It made sense to do all the arguments except
|developer_protocol_handler|
> because it is not used as a protocol but rather as an interceptor (so no
> scheme).

ah, that's my fault (I added the protocol interceptor when I moved that code to
content before the other webui was moved).

it's not needed anymore though now that the chrome:// protocol handler is in
content

I created a change to do this now, since this is my fault, at
https://codereview.chromium.org/12386009/

it would be great that since you're touching all this that it would be removed
at the same time (i.e. if you can merge the change above into your change, or
you or i can land it first and then you can sync with it). up to you, and if
it's too much pain, I can do this after.

> 
> There were some variants to account for, for example:
> AwURLRequestContextGetter::GetURLRequestContext swaps in different file: and
> data: handlers and ignores the ones passed in.  Not a problem, but something
to
> note.

https://codereview.chromium.org/11896113/diff/67004/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11896113/diff/67004/content/public/browser/co...
content/public/browser/content_browser_client.h:160: static void
InstallProtocolHandlers(
nit: we don't really put static methods in this interface. since this method is
pretty simple, and the code that calls it will eventually be collapsed into one
method inside content, can you just duplicate the for loop everywhere? also i
dont think you need the ->clear() call on the map?

[dhollowa, 2013-02-28 18:21:11]

https://codereview.chromium.org/11896113/diff/44001/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11896113/diff/44001/content/public/browser/co...
content/public/browser/content_browser_client.h:164: AdditionalWebUIHandlers*
additional_webui_handlers,
On 2013/02/28 02:21:21, jam wrote:
> On 2013/02/28 00:00:37, dhollowa wrote:
> > On 2013/02/27 01:11:13, jam wrote:
> > > actually, why not make this method and the one below just take a map for
all
> > the
> > > arguments (i.e. blob/file/developer)
> > 
> > Done.  It made sense to do all the arguments except
> |developer_protocol_handler|
> > because it is not used as a protocol but rather as an interceptor (so no
> > scheme).
> 
> ah, that's my fault (I added the protocol interceptor when I moved that code
to
> content before the other webui was moved).
> 
> it's not needed anymore though now that the chrome:// protocol handler is in
> content
> 
> I created a change to do this now, since this is my fault, at
> https://codereview.chromium.org/12386009/
> 
> it would be great that since you're touching all this that it would be removed
> at the same time (i.e. if you can merge the change above into your change, or
> you or i can land it first and then you can sync with it). up to you, and if
> it's too much pain, I can do this after.
> 
> > 
> > There were some variants to account for, for example:
> > AwURLRequestContextGetter::GetURLRequestContext swaps in different file: and
> > data: handlers and ignores the ones passed in.  Not a problem, but something
> to
> > note.
> 

Done.  I removed the |developer_protocol_handler| as per your patch.

https://codereview.chromium.org/11896113/diff/67004/content/public/browser/co...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/11896113/diff/67004/content/public/browser/co...
content/public/browser/content_browser_client.h:160: static void
InstallProtocolHandlers(
On 2013/02/28 02:21:21, jam wrote:
> nit: we don't really put static methods in this interface. since this method
is
> pretty simple, and the code that calls it will eventually be collapsed into
one
> method inside content, can you just duplicate the for loop everywhere? also i
> dont think you need the ->clear() call on the map?

Done.  I moved this closer to the call sites.  One in shell, and one in
ProfileIOData.  I left the |clear()| call in, more as a defensive measure.

[Charlie Reis, 2013-02-28 18:47:12]

On 2013/02/28 00:15:22, dhollowa wrote:
> On 2013/02/27 22:59:54, jam wrote:
> > On 2013/02/27 22:50:22, creis wrote:
> >[...]
> >Thanks, John.  If that's true, we need to be very explicit
> >about this, so that
> >someone doesn't later "clean up" the use of WebUI schemes 
> >by granting WebUI
> >bindings to this process.  We should both document that 
> >this doesn't get WebUI
> >bindings and test for it.
> 
> Yes, John is right.  By "explicit" I was under the impression that
partitioning
> to the different "chrome-search:" scheme was explicit enough, since no
bindings
> would be in effect this way.

Ok, but that's not what I meant by being explicit about it.  There should be
documentation on the scheme in the CL, and there should be a test verifying that
the instant process does not have WebUI bindings.  You can check this in a
browser test using either RenderViewHost::GetEnabledBindings() or
ChildProcessSecurityImpl::HasWebUIBindings(child_id).  It's very important this
doesn't get inadvertently changed down the road.

[jschuh, 2013-02-28 19:48:34]

On 2013/02/28 18:47:12, creis wrote:
> Ok, but that's not what I meant by being explicit about it.  There should be
> documentation on the scheme in the CL, and there should be a test verifying
that
> the instant process does not have WebUI bindings.  You can check this in a
> browser test using either RenderViewHost::GetEnabledBindings() or
> ChildProcessSecurityImpl::HasWebUIBindings(child_id).  It's very important
this
> doesn't get inadvertently changed down the road.

To reiterate Charlie's point, we've had sandbox escapes (used in full chain
exploits) introduced under nearly the same circumstances in the past. So, it's
something we're particularly wary of.

[dhollowa, 2013-02-28 19:50:06]

On 2013/02/28 19:48:34, Justin Schuh wrote:
> On 2013/02/28 18:47:12, creis wrote:
> > Ok, but that's not what I meant by being explicit about it.  There should be
> > documentation on the scheme in the CL, and there should be a test verifying
> that
> > the instant process does not have WebUI bindings.  You can check this in a
> > browser test using either RenderViewHost::GetEnabledBindings() or
> > ChildProcessSecurityImpl::HasWebUIBindings(child_id).  It's very important
> this
> > doesn't get inadvertently changed down the road.
> 
> To reiterate Charlie's point, we've had sandbox escapes (used in full chain
> exploits) introduced under nearly the same circumstances in the past. So, it's
> something we're particularly wary of.

Understood.  Working on a test now.

[jam, 2013-02-28 21:41:25]

lgtm, thank you for going through all the different paths until we reached this.

(i didn't review files inside instant or searchbox)

[jam, 2013-02-28 21:43:33]

https://codereview.chromium.org/11896113/diff/76007/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

https://codereview.chromium.org/11896113/diff/76007/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_io_data.cc:28: #include
"content/public/browser/content_browser_client.h"
nit: not needed

[dhollowa, 2013-02-28 21:50:45]

On 2013/02/28 19:50:06, dhollowa wrote:
> On 2013/02/28 19:48:34, Justin Schuh wrote:
> > On 2013/02/28 18:47:12, creis wrote:
> > > Ok, but that's not what I meant by being explicit about it.  There should
be
> > > documentation on the scheme in the CL, and there should be a test
verifying
> > that
> > > the instant process does not have WebUI bindings.  You can check this in a
> > > browser test using either RenderViewHost::GetEnabledBindings() or
> > > ChildProcessSecurityImpl::HasWebUIBindings(child_id).  It's very important
> > this
> > > doesn't get inadvertently changed down the road.
> > 
> > To reiterate Charlie's point, we've had sandbox escapes (used in full chain
> > exploits) introduced under nearly the same circumstances in the past. So,
it's
> > something we're particularly wary of.
> 
> Understood.  Working on a test now.

Charlie, please see InstantExtendedTest.NoWebUIBindings* tests.  Wasn't sure if
testing transitions in/out of chrome: urls to/from Instant pages was worthwhile,
so left them out.  This CL has no chrome-search: page directly, just resources
now, so I'd need to add additional tests once we have chrome-search: pages in
place.  Thoughts?

Also, added doc in chrome/common/url_constants.h.

[dhollowa, 2013-02-28 21:55:24]

Thanks for the crash course in content layering John.  Appreciate the
hand-holding.  :-)

https://codereview.chromium.org/11896113/diff/76007/chrome/browser/profiles/o...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

https://codereview.chromium.org/11896113/diff/76007/chrome/browser/profiles/o...
chrome/browser/profiles/off_the_record_profile_io_data.cc:28: #include
"content/public/browser/content_browser_client.h"
On 2013/02/28 21:43:33, jam wrote:
> nit: not needed

Done.

[dhollowa, 2013-02-28 23:15:52]

+benm OWNERS android_webview/

[benm (inactive), 2013-03-01 11:23:23]

android_webview lgtm

[Charlie Reis, 2013-03-02 04:41:09]

Can you update the CL description?  It still talks about some stale things like
using ChildProcessSecurityPolicy.

I think I've mostly wrapped my head around it, and we're probably getting close.
 A few requests and questions below.

https://codereview.chromium.org/11896113/diff/75137/android_webview/browser/n...
File android_webview/browser/net/aw_url_request_context_getter.cc (left):

https://codereview.chromium.org/11896113/diff/75137/android_webview/browser/n...
android_webview/browser/net/aw_url_request_context_getter.cc:129:
developer_protocol_handler_.Pass()));
Was this intentionally removed?  The others seem to have equivalents above, so I
just want to be sure we didn't forget about one.

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:187: chrome::kChromeSearchScheme);
This means FilterURL won't complain if it sees chrome-search: requests from
normal renderer processes, I believe.  Is there somewhere else that we ensure
that only the Instant process can make those requests?

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:768:
additional_schemes.push_back(chrome::kChromeSearchScheme);
Can you add a comment here saying this is treated as a WebUI scheme but it does
not get WebUI bindings?  Sorry to be paranoid about it.

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:685:
IN_PROC_BROWSER_TEST_F(InstantExtendedTest, DISABLED_MostVisited) {
Why is this disabled?  Can you add a comment with a bug number for re-enabling
it?

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.cc (right):

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/instant/in...
chrome/browser/instant/instant_page.cc:60: Send(new
ChromeViewMsg_SearchBoxGrantAccessFromOrigin(
The comment mentioned that this function added RVH bindings, but it doesn't
appear to.

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.h (right):

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:144: void GrantAccessFromOrigin(const
GURL& origin_url);
nit: Can you make this name a bit clearer, by calling it
GrantChromeSearchAccessFromOrigin?  We have a lot of GrantAccess things
scattered around, and this will help disambiguate.

[dhollowa, 2013-03-04 18:39:34]

https://codereview.chromium.org/11896113/diff/75137/android_webview/browser/n...
File android_webview/browser/net/aw_url_request_context_getter.cc (left):

https://codereview.chromium.org/11896113/diff/75137/android_webview/browser/n...
android_webview/browser/net/aw_url_request_context_getter.cc:129:
developer_protocol_handler_.Pass()));
On 2013/03/02 04:41:09, creis wrote:
> Was this intentionally removed?  The others seem to have equivalents above, so
I
> just want to be sure we didn't forget about one.

Yes, this was intentional.  Jam asked that I remove it as it is now obsolete.

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/browser_pr...
File chrome/browser/browser_process_impl.cc (right):

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/browser_pr...
chrome/browser/browser_process_impl.cc:187: chrome::kChromeSearchScheme);
On 2013/03/02 04:41:09, creis wrote:
> This means FilterURL won't complain if it sees chrome-search: requests from
> normal renderer processes, I believe.  Is there somewhere else that we ensure
> that only the Instant process can make those requests?

The cross-origin checks block this normally on the renderer side, and the
URLDataSource::ShouldServiceRequest will block these on the browser-side
normally.

For the Instant case, I use
WebKit::WebSecurityPolicy::addOriginAccessWhitelistEntry on the renderer side,
and permit specific data-source access via URLDataSource::ShouldAccessRequest on
the browser side.

Since those checks are in place, I needed to disable the
ChildProcessSecurityPolicy stuff here.

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/chrome_con...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/chrome_con...
chrome/browser/chrome_content_browser_client.cc:768:
additional_schemes.push_back(chrome::kChromeSearchScheme);
On 2013/03/02 04:41:09, creis wrote:
> Can you add a comment here saying this is treated as a WebUI scheme but it
does
> not get WebUI bindings?  Sorry to be paranoid about it.

Done.

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/instant/in...
File chrome/browser/instant/instant_extended_browsertest.cc (right):

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/instant/in...
chrome/browser/instant/instant_extended_browsertest.cc:685:
IN_PROC_BROWSER_TEST_F(InstantExtendedTest, DISABLED_MostVisited) {
On 2013/03/02 04:41:09, creis wrote:
> Why is this disabled?  Can you add a comment with a bug number for re-enabling
> it?

I'm noticing flakes during my try-runs.  This is a larger problem that affects
multiple Instant tests however.  So I'd like to take it as a separate task to
sort this out.  Sreeram has a refactor in-flight that may address this too.

Added TODO and bug for now.

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.cc (right):

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/instant/in...
chrome/browser/instant/instant_page.cc:60: Send(new
ChromeViewMsg_SearchBoxGrantAccessFromOrigin(
On 2013/03/02 04:41:09, creis wrote:
> The comment mentioned that this function added RVH bindings, but it doesn't
> appear to.

Thanks, sorry, stale doc.  Fixed now.  The renderer side is done with the WebKit
origin stuff, the browser-side is done with the
URLDataManager::ShouldServiceRequest stuff.

This call here only sets up the renderer side.

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/instant/in...
File chrome/browser/instant/instant_page.h (right):

https://codereview.chromium.org/11896113/diff/75137/chrome/browser/instant/in...
chrome/browser/instant/instant_page.h:144: void GrantAccessFromOrigin(const
GURL& origin_url);
On 2013/03/02 04:41:09, creis wrote:
> nit: Can you make this name a bit clearer, by calling it
> GrantChromeSearchAccessFromOrigin?  We have a lot of GrantAccess things
> scattered around, and this will help disambiguate.

Done.

[sreeram, 2013-03-04 19:14:14]

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
File chrome/browser/instant/instant_io_context.cc (right):

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
chrome/browser/instant/instant_io_context.cc:16: const char
kInstantIOContextKeyName[] = "instant_io_context";
In other UserData cases I've seen, we just use the pointer of a module-static
variable as the key (see instant_overlay.cc for example), without bothering to
assign any value to the variable.

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
chrome/browser/instant/instant_io_context.cc:18: // Retrieves the instant data
from the |context|'s named user-data.
instant -> Instant

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
chrome/browser/instant/instant_io_context.cc:24: }
No curlies for single line 'if' statements.

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
chrome/browser/instant/instant_io_context.cc:93: }
Why bother forwarding the calls to these internal methods? The static methods
can access |process_ids_| directly, so why not just something like this:

// static
void InstantIOContents::AddInstantProcessOnIO(...) {
  DCHECK(...);
  GetDataForResourceContext(context)->process_ids_.insert(...);
}

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
File chrome/browser/instant/instant_io_context.h (right):

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
chrome/browser/instant/instant_io_context.h:30: // processes.  Used to keep
process ids in sync with InstantService.
ids -> IDs

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
chrome/browser/instant/instant_io_context.h:49: // The process ids associated
with Instant processes.  Mirror of the process
ids -> IDs

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
chrome/browser/instant/instant_io_context.h:50: // ids in InstantService. 
Duplicated here for synchronous access on the IO
ids -> IDs

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
File chrome/browser/instant/instant_service_factory.cc (right):

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
chrome/browser/instant/instant_service_factory.cc:36: }
The default is already false
(chrome/browser/profiles/profile_keyed_base_factory.cc), so why override this
method?

[Charlie Reis, 2013-03-04 19:43:27]

https://codereview.chromium.org/11896113/diff/75142/content/public/browser/ur...
File content/public/browser/url_data_source.h (right):

https://codereview.chromium.org/11896113/diff/75142/content/public/browser/ur...
content/public/browser/url_data_source.h:105: // implement fancier access
control.
This seems like an overly permissive default.  Most renderers are not allowed to
request chrome: URLs.

Could this default to something like the normal web-safe schemes, and then WebUI
processes can allow chrome: if needed?  (I admit I'm unclear on how this gets
used, since it doesn't seem to allow http and similar schemes in any of your
examples.  Maybe the name and/or comment could help clarify?)

[dhollowa, 2013-03-04 19:53:50]

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
File chrome/browser/instant/instant_io_context.cc (right):

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
chrome/browser/instant/instant_io_context.cc:16: const char
kInstantIOContextKeyName[] = "instant_io_context";
On 2013/03/04 19:14:14, sreeram wrote:
> In other UserData cases I've seen, we just use the pointer of a module-static
> variable as the key (see instant_overlay.cc for example), without bothering to
> assign any value to the variable.

Six of one...  a casual grep for "context->SetUserData" yields pretty consistent
use of the key-name approach that I've used here.

I'd prefer to keep as-is.

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
chrome/browser/instant/instant_io_context.cc:18: // Retrieves the instant data
from the |context|'s named user-data.
On 2013/03/04 19:14:14, sreeram wrote:
> instant -> Instant

Done.

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
chrome/browser/instant/instant_io_context.cc:24: }
On 2013/03/04 19:14:14, sreeram wrote:
> No curlies for single line 'if' statements.

Done.

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
chrome/browser/instant/instant_io_context.cc:93: }
On 2013/03/04 19:14:14, sreeram wrote:
> Why bother forwarding the calls to these internal methods? The static methods
> can access |process_ids_| directly, so why not just something like this:
> 
> // static
> void InstantIOContents::AddInstantProcessOnIO(...) {
>   DCHECK(...);
>   GetDataForResourceContext(context)->process_ids_.insert(...);
> }

Ah, yes.  Good point.  Done.

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
File chrome/browser/instant/instant_io_context.h (right):

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
chrome/browser/instant/instant_io_context.h:30: // processes.  Used to keep
process ids in sync with InstantService.
On 2013/03/04 19:14:14, sreeram wrote:
> ids -> IDs

Done.

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
chrome/browser/instant/instant_io_context.h:49: // The process ids associated
with Instant processes.  Mirror of the process
On 2013/03/04 19:14:14, sreeram wrote:
> ids -> IDs

Done.

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
chrome/browser/instant/instant_io_context.h:50: // ids in InstantService. 
Duplicated here for synchronous access on the IO
On 2013/03/04 19:14:14, sreeram wrote:
> ids -> IDs

Done.

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
File chrome/browser/instant/instant_service_factory.cc (right):

https://codereview.chromium.org/11896113/diff/75142/chrome/browser/instant/in...
chrome/browser/instant/instant_service_factory.cc:36: }
On 2013/03/04 19:14:14, sreeram wrote:
> The default is already false
> (chrome/browser/profiles/profile_keyed_base_factory.cc), so why override this
> method?

Gosh, I could have sworn this was default true when I wrote the tests.  Hmm. 
Removed now.

[dhollowa, 2013-03-04 20:06:19]

https://codereview.chromium.org/11896113/diff/75142/content/public/browser/ur...
File content/public/browser/url_data_source.h (right):

https://codereview.chromium.org/11896113/diff/75142/content/public/browser/ur...
content/public/browser/url_data_source.h:105: // implement fancier access
control.
On 2013/03/04 19:43:27, creis wrote:
> This seems like an overly permissive default.  Most renderers are not allowed
to
> request chrome: URLs.
> 
> Could this default to something like the normal web-safe schemes, and then
WebUI
> processes can allow chrome: if needed?  (I admit I'm unclear on how this gets
> used, since it doesn't seem to allow http and similar schemes in any of your
> examples.  Maybe the name and/or comment could help clarify?)

Yes, I think there may be confusion.  This method is only used *after* the
normal WebUI bindings checks have been performed, so this can only strengthen
existing checks not weaken them.

That is, with the exception of chrome-search: where we bypass the bindings stuff
and then ThemeSource::ShouldServiceRequest, FaviconSource::ShouldServiceRequest,
etc. open up access to only chrome-search: with the proper process-id checks.

Please see URLDataManagerBackend::StartRequest and
url_data_manager_backend.cc:56.  That may make it clearer.  This code is running
during the service of the requests, only after all existing renderer and
browser/bindings checks have already had a chance to deny.

[Charlie Reis, 2013-03-04 20:21:14]

https://codereview.chromium.org/11896113/diff/75142/content/public/browser/ur...
File content/public/browser/url_data_source.h (right):

https://codereview.chromium.org/11896113/diff/75142/content/public/browser/ur...
content/public/browser/url_data_source.h:105: // implement fancier access
control.
On 2013/03/04 20:06:19, dhollowa wrote:
> On 2013/03/04 19:43:27, creis wrote:
> > This seems like an overly permissive default.  Most renderers are not
allowed
> to
> > request chrome: URLs.
> > 
> > Could this default to something like the normal web-safe schemes, and then
> WebUI
> > processes can allow chrome: if needed?  (I admit I'm unclear on how this
gets
> > used, since it doesn't seem to allow http and similar schemes in any of your
> > examples.  Maybe the name and/or comment could help clarify?)
> 
> Yes, I think there may be confusion.  This method is only used *after* the
> normal WebUI bindings checks have been performed, so this can only strengthen
> existing checks not weaken them.
> 
> That is, with the exception of chrome-search: where we bypass the bindings
stuff
> and then ThemeSource::ShouldServiceRequest,
FaviconSource::ShouldServiceRequest,
> etc. open up access to only chrome-search: with the proper process-id checks.
> 
> Please see URLDataManagerBackend::StartRequest and
> url_data_manager_backend.cc:56.  That may make it clearer.  This code is
running
> during the service of the requests, only after all existing renderer and
> browser/bindings checks have already had a chance to deny.
> 

Ok.  The confusing thing here is that this class doesn't say anything about
being specific to WebUI, even though (in retrospect) it appears to be about
WebUI stuff.  Certainly the URLDataManagerBackend is in the webui folder, but
none of the class names reflect that, unfortunately.

Can you update either this comment or the class-level comment to say how it's
specific to WebUI?  I think that will avoid confusion in the future.

[sreeram, 2013-03-04 20:30:05]

instant and searchbox lgtm

[dhollowa, 2013-03-04 21:02:06]

https://codereview.chromium.org/11896113/diff/75142/content/public/browser/ur...
File content/public/browser/url_data_source.h (right):

https://codereview.chromium.org/11896113/diff/75142/content/public/browser/ur...
content/public/browser/url_data_source.h:105: // implement fancier access
control.
On 2013/03/04 20:21:14, creis wrote:
> On 2013/03/04 20:06:19, dhollowa wrote:
> > On 2013/03/04 19:43:27, creis wrote:
> > > This seems like an overly permissive default.  Most renderers are not
> allowed
> > to
> > > request chrome: URLs.
> > > 
> > > Could this default to something like the normal web-safe schemes, and then
> > WebUI
> > > processes can allow chrome: if needed?  (I admit I'm unclear on how this
> gets
> > > used, since it doesn't seem to allow http and similar schemes in any of
your
> > > examples.  Maybe the name and/or comment could help clarify?)
> > 
> > Yes, I think there may be confusion.  This method is only used *after* the
> > normal WebUI bindings checks have been performed, so this can only
strengthen
> > existing checks not weaken them.
> > 
> > That is, with the exception of chrome-search: where we bypass the bindings
> stuff
> > and then ThemeSource::ShouldServiceRequest,
> FaviconSource::ShouldServiceRequest,
> > etc. open up access to only chrome-search: with the proper process-id
checks.
> > 
> > Please see URLDataManagerBackend::StartRequest and
> > url_data_manager_backend.cc:56.  That may make it clearer.  This code is
> running
> > during the service of the requests, only after all existing renderer and
> > browser/bindings checks have already had a chance to deny.
> > 
> 
> Ok.  The confusing thing here is that this class doesn't say anything about
> being specific to WebUI, even though (in retrospect) it appears to be about
> WebUI stuff.  Certainly the URLDataManagerBackend is in the webui folder, but
> none of the class names reflect that, unfortunately.
> 
> Can you update either this comment or the class-level comment to say how it's
> specific to WebUI?  I think that will avoid confusion in the future.

Done.  Yes, I fully agree.  WebUIDataSource would be a much better name for this
class...  I've expanded the comments for the class and this method to clarify.

[Charlie Reis, 2013-03-04 21:47:09]

Thanks, I think that takes care of all my concerns.  LGTM.

[jschuh, 2013-03-04 21:55:41]

Rubberstamp based on @cries' lgtm.

[commit-bot: I haz the power, 2013-03-04 22:28:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dhollowa@chromium.org/11896113/78023

[commit-bot: I haz the power, 2013-03-05 00:30:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dhollowa@chromium.org/11896113/75219

[commit-bot: I haz the power, 2013-03-05 01:45:41]

Retried try job too often on linux_chromeos for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-05 19:11:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dhollowa@chromium.org/11896113/103001

[commit-bot: I haz the power, 2013-03-05 19:20:55]

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=linux_chro...

[commit-bot: I haz the power, 2013-03-05 22:01:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dhollowa@chromium.org/11896113/107001

[commit-bot: I haz the power, 2013-03-05 23:14:38]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-03-06 01:11:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dhollowa@chromium.org/11896113/134001

[commit-bot: I haz the power, 2013-03-06 04:02:19]

Change committed as 186347

[dhollowa, 2013-03-07 01:26:35]

Sreeram: I made changes to c/b/instant/instant_io_context.* & instant_service.*
with respect to lifetime of the InstantIOContext.  There was a glitch during the
shutdown sequence that caused use-after-free of the InstantIOContext on the IO
thread.

Have run this through Valgrind try-bots now with positive results.

PTAL.  Thanks.

[sreeram, 2013-03-07 02:17:21]

lgtm

https://codereview.chromium.org/11896113/diff/144001/chrome/browser/instant/i...
File chrome/browser/instant/instant_service.cc (right):

https://codereview.chromium.org/11896113/diff/144001/chrome/browser/instant/i...
chrome/browser/instant/instant_service.cc:15: #include
"content/public/browser/resource_context.h"
Maybe no need for this include.

https://codereview.chromium.org/11896113/diff/144001/chrome/browser/instant/i...
File chrome/browser/instant/instant_service.h (right):

https://codereview.chromium.org/11896113/diff/144001/chrome/browser/instant/i...
chrome/browser/instant/instant_service.h:13: #include
"chrome/browser/instant/instant_io_context.h"
You can probably just forward declare InstantIOContext instead of including the
.h.

[dhollowa, 2013-03-07 02:22:30]

https://codereview.chromium.org/11896113/diff/144001/chrome/browser/instant/i...
File chrome/browser/instant/instant_service.cc (right):

https://codereview.chromium.org/11896113/diff/144001/chrome/browser/instant/i...
chrome/browser/instant/instant_service.cc:15: #include
"content/public/browser/resource_context.h"
On 2013/03/07 02:17:21, sreeram wrote:
> Maybe no need for this include.

Done.

https://codereview.chromium.org/11896113/diff/144001/chrome/browser/instant/i...
File chrome/browser/instant/instant_service.h (right):

https://codereview.chromium.org/11896113/diff/144001/chrome/browser/instant/i...
chrome/browser/instant/instant_service.h:13: #include
"chrome/browser/instant/instant_io_context.h"
On 2013/03/07 02:17:21, sreeram wrote:
> You can probably just forward declare InstantIOContext instead of including
the
> .h.

Done.

[commit-bot: I haz the power, 2013-03-07 02:23:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dhollowa@chromium.org/11896113/162012

[commit-bot: I haz the power, 2013-03-07 04:15:50]

Retried try job too often on win_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-03-07 05:48:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dhollowa@chromium.org/11896113/162012

[commit-bot: I haz the power, 2013-03-07 05:48:25]

Failed to apply patch for
chrome/browser/instant/instant_extended_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/instant/instant_extended_browsertest.cc
  Hunk #8 FAILED at 243.
  Hunk #9 succeeded at 275 (offset -1 lines).
  Hunk #10 succeeded at 311 (offset -1 lines).
  Hunk #11 succeeded at 339 (offset -1 lines).
  Hunk #12 succeeded at 377 (offset -1 lines).
  Hunk #13 succeeded at 409 (offset -1 lines).
  Hunk #14 succeeded at 420 (offset -1 lines).
  Hunk #15 succeeded at 442 (offset -1 lines).
  Hunk #16 succeeded at 464 (offset -1 lines).
  Hunk #17 succeeded at 485 (offset -1 lines).
  Hunk #18 succeeded at 499 (offset -1 lines).
  Hunk #19 succeeded at 527 (offset -1 lines).
  Hunk #20 succeeded at 560 (offset -1 lines).
  Hunk #21 succeeded at 597 (offset -1 lines).
  Hunk #22 succeeded at 642 (offset -1 lines).
  Hunk #23 succeeded at 695 (offset -1 lines).
  Hunk #24 succeeded at 786 (offset -1 lines).
  Hunk #25 succeeded at 909 (offset -1 lines).
  Hunk #26 succeeded at 946 (offset -1 lines).
  Hunk #27 succeeded at 976 (offset -1 lines).
  1 out of 27 hunks FAILED -- saving rejects to file
chrome/browser/instant/instant_extended_browsertest.cc.rej

Patch:       chrome/browser/instant/instant_extended_browsertest.cc
Index: chrome/browser/instant/instant_extended_browsertest.cc
diff --git a/chrome/browser/instant/instant_extended_browsertest.cc
b/chrome/browser/instant/instant_extended_browsertest.cc
index
e265799f7b40c944cb964f3629b3ab54a98e16b2..7bed26480235d8c565fadd6eb024d4321bf9ec70
100644
--- a/chrome/browser/instant/instant_extended_browsertest.cc
+++ b/chrome/browser/instant/instant_extended_browsertest.cc
@@ -5,7 +5,11 @@
 #include <sstream>
 
 #include "base/prefs/pref_service.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
+#include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/favicon/favicon_tab_helper.h"
+#include "chrome/browser/history/history_types.h"
+#include "chrome/browser/history/top_sites.h"
 #include "chrome/browser/instant/instant_commit_type.h"
 #include "chrome/browser/instant/instant_ntp.h"
 #include "chrome/browser/instant/instant_overlay.h"
@@ -14,21 +18,45 @@
 #include "chrome/browser/instant/instant_tab.h"
 #include "chrome/browser/instant/instant_test_utils.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/themes/theme_service.h"
+#include "chrome/browser/themes/theme_service_factory.h"
+#include "chrome/browser/ui/omnibox/omnibox_view.h"
 #include "chrome/browser/ui/search/search.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/webui/theme_source.h"
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/pref_names.h"
+#include "chrome/common/thumbnail_score.h"
 #include "chrome/common/url_constants.h"
+#include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/interactive_test_utils.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/site_instance.h"
+#include "content/public/browser/url_data_source.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_view.h"
+#include "content/public/common/bindings_policy.h"
 #include "content/public/test/browser_test_utils.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 
-class InstantExtendedTest : public InstantTestBase {
+namespace {
+
+// Creates a bitmap of the specified color. Caller takes ownership.
+gfx::Image CreateBitmap(SkColor color) {
+  SkBitmap thumbnail;
+  thumbnail.setConfig(SkBitmap::kARGB_8888_Config, 4, 4);
+  thumbnail.allocPixels();
+  thumbnail.eraseColor(color);
+  return gfx::Image::CreateFrom1xBitmap(thumbnail);  // adds ref.
+}
+
+}  // namespace
+
+class InstantExtendedTest : public InProcessBrowserTest,
+                            public InstantTestBase {
  public:
   InstantExtendedTest()
       : on_most_visited_change_calls_(0),
@@ -38,21 +66,10 @@ class InstantExtendedTest : public InstantTestBase {
  protected:
   virtual void SetUpInProcessBrowserTestFixture() OVERRIDE {
     chrome::search::EnableInstantExtendedAPIForTesting();
-    ASSERT_TRUE(https_test_server_.Start());
-    instant_url_ = https_test_server_.
-        GetURL("files/instant_extended.html?strk=1&");
-  }
-
-  void FocusOmniboxAndWaitForInstantSupport() {
-    content::WindowedNotificationObserver ntp_observer(
-        chrome::NOTIFICATION_INSTANT_NTP_SUPPORT_DETERMINED,
-        content::NotificationService::AllSources());
-    content::WindowedNotificationObserver overlay_observer(
-        chrome::NOTIFICATION_INSTANT_OVERLAY_SUPPORT_DETERMINED,
-        content::NotificationService::AllSources());
-    FocusOmnibox();
-    ntp_observer.Wait();
-    overlay_observer.Wait();
+    ASSERT_TRUE(https_test_server().Start());
+    GURL instant_url = https_test_server().GetURL(
+        "files/instant_extended.html?strk=1&");
+    InstantTestBase::Init(instant_url);
   }
 
   std::string GetOmniboxText() {
@@ -91,14 +108,51 @@ class InstantExtendedTest : public InstantTestBase {
   int first_most_visited_item_id_;
 };
 
+// Test class used to verify chrome-search: scheme and access policy from the
+// Instant overlay.  This is a subclass of |ExtensionBrowserTest| because it
+// loads a theme that provides a background image.
+class InstantPolicyTest : public ExtensionBrowserTest, public InstantTestBase {
+ public:
+  InstantPolicyTest() {}
+
+ protected:
+  virtual void SetUpInProcessBrowserTestFixture() OVERRIDE {
+    chrome::search::EnableInstantExtendedAPIForTesting();
+    ASSERT_TRUE(https_test_server().Start());
+    GURL instant_url = https_test_server().GetURL(
+        "files/instant_extended.html?strk=1&");
+    InstantTestBase::Init(instant_url);
+  }
+
+  void InstallThemeSource() {
+    ThemeSource* theme = new ThemeSource(profile());
+    content::URLDataSource::Add(profile(), theme);
+  }
+
+  void InstallThemeAndVerify(const std::string& theme_dir,
+                             const std::string& theme_name) {
+    const base::FilePath theme_path = test_data_dir_.AppendASCII(theme_dir);
+    ASSERT_TRUE(InstallExtensionWithUIAutoConfirm(
+        theme_path, 1, ExtensionBrowserTest::browser()));
+    const extensions::Extension* theme =
+        ThemeServiceFactory::GetThemeForProfile(
+            ExtensionBrowserTest::browser()->profile());
+    ASSERT_NE(static_cast<extensions::Extension*>(NULL), theme);
+    ASSERT_EQ(theme->name(), theme_name);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(InstantPolicyTest);
+};
+
 IN_PROC_BROWSER_TEST_F(InstantExtendedTest, ExtendedModeIsOn) {
-  ASSERT_NO_FATAL_FAILURE(SetupInstant());
+  ASSERT_NO_FATAL_FAILURE(SetupInstant(browser()));
   EXPECT_TRUE(instant()->extended_enabled_);
 }
 
 // Test that Instant is preloaded when the omnibox is focused.
 IN_PROC_BROWSER_TEST_F(InstantExtendedTest, OmniboxFocusLoadsInstant) {
-  ASSERT_NO_FATAL_FAILURE(SetupInstant());
+  ASSERT_NO_FATAL_FAILURE(SetupInstant(browser()));
 
   // Explicitly unfocus the omnibox.
   EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
@@ -112,7 +166,7 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
OmniboxFocusLoadsInstant) {
   EXPECT_FALSE(instant()->GetOverlayContents());
 
   // Refocus the omnibox. The InstantController should've preloaded Instant.
-  FocusOmniboxAndWaitForInstantSupport();
+  FocusOmniboxAndWaitForInstantExtendedSupport();
 
   EXPECT_FALSE(ui_test_utils::IsViewFocused(browser(), VIEW_ID_TAB_CONTAINER));
   EXPECT_TRUE(omnibox()->model()->has_focus());
@@ -140,10 +194,10 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
OmniboxFocusLoadsInstant) {
 }
 
 IN_PROC_BROWSER_TEST_F(InstantExtendedTest, InputShowsOverlay) {
-  ASSERT_NO_FATAL_FAILURE(SetupInstant());
+  ASSERT_NO_FATAL_FAILURE(SetupInstant(browser()));
 
   // Focus omnibox and confirm overlay isn't shown.
-  FocusOmniboxAndWaitForInstantSupport();
+  FocusOmniboxAndWaitForInstantExtendedSupport();
   content::WebContents* overlay = instant()->GetOverlayContents();
   EXPECT_TRUE(overlay);
   EXPECT_FALSE(instant()->IsOverlayingSearchResults());
@@ -158,8 +212,8 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
InputShowsOverlay) {
 // Test that middle clicking on a suggestion opens the result in a new tab.
 IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
                        MiddleClickOnSuggestionOpensInNewTab) {
-  ASSERT_NO_FATAL_FAILURE(SetupInstant());
-  FocusOmniboxAndWaitForInstantSupport();
+  ASSERT_NO_FATAL_FAILURE(SetupInstant(browser()));
+  FocusOmniboxAndWaitForInstantExtendedSupport();
   EXPECT_TRUE(ui_test_utils::BringBrowserWindowToFront(browser()));
 
   EXPECT_EQ(1, browser()->tab_strip_model()->count());
@@ -189,17 +243,16 @@ IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
   // Check that the new tab URL is as expected.
   content::WebContents* new_tab_contents =
       browser()->tab_strip_model()->GetWebContentsAt(1);
-  EXPECT_EQ(new_tab_contents->GetURL().spec(), instant_url_.spec()+"q=santa");
+  EXPECT_EQ(new_tab_contents->GetURL().spec(), instant_url().spec()+"q=santa");
 
   // Check that there are now two tabs.
   EXPECT_EQ(2, browser()->tab_strip_model()->count());
 }
 
 // Test that omnibox text is correctly set when overlay is committed with
Enter.
-IN_PROC_BROWSER_TEST_F(InstantExtendedTest,
-                       OmniboxTextUponEnterCommit) {
-  ASSERT_NO_FATAL_FAILURE(SetupInstant());
-  FocusOmniboxAndWaitForInstantSupport();
+IN_PROC_BROWSER_TEST_F(InstantExtendedTest, OmniboxTextUponEnterCommit) {
+  ASSERT_NO_FATAL_FAILURE(SetupInstant(browser()));
+  FocusOmniboxAndWaitForInstantExtendedSupport();
 
   // The page will autocomplete once we set the omnibox value.
   EXPECT_TR…
(message too large)

[commit-bot: I haz the power, 2013-03-07 07:41:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dhollowa@chromium.org/11896113/186001

[commit-bot: I haz the power, 2013-03-07 12:30:09]

Change committed as 186680