InstantExtended: hook up InstantTab in incognito.

InstantExtended: hook up InstantTab in incognito.

Hook up InstantTab in incognito, but don't send any data except submit events.

BUG=181427
TESTED=manually per report, also modes still work in incognito
R=jered@chromium.org

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

[samarth, 2013-06-17 19:06:58]

PTAL

[Jered, 2013-06-17 20:55:01]

lgtm

Seems to work. I tested that changing my search engine takes effect immediately,
and I can refine and stay in Images mode.

https://codereview.chromium.org/17303003/diff/1/chrome/browser/search/search_...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/17303003/diff/1/chrome/browser/search/search_...
chrome/browser/search/search_unittest.cc:421: {"chrome-search://foo",           
       false, "Chrome-search scheme"},
Extra space.

[samarth, 2013-06-17 21:44:06]

Thanks.

https://codereview.chromium.org/17303003/diff/1/chrome/browser/search/search_...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/17303003/diff/1/chrome/browser/search/search_...
chrome/browser/search/search_unittest.cc:421: {"chrome-search://foo",           
       false, "Chrome-search scheme"},
On 2013/06/17 20:55:01, Jered wrote:
> Extra space.

Done.

[commit-bot: I haz the power, 2013-06-17 22:18:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samarth@chromium.org/17303003/5001

[commit-bot: I haz the power, 2013-06-17 22:29:17]

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

[samarth, 2013-06-20 15:56:48]

FYI, I landed the revert separately, so this change now only makes it so search
modes work in incognito.

[samarth, 2013-06-20 16:49:25]

I realized my initial change was not correct because we would still be sending
things like most visited to the incognito InstantTab. To make it safer, I now
explicitly don't send anything but the bare minimum for InstantPages used in
incognito mode.

PTAL (I probably also need to update some unit tests. Will do that shortly.)

Thanks,
Samarth

[Jered, 2013-06-20 17:15:49]

On 2013/06/20 16:49:25, samarth wrote:
> I realized my initial change was not correct because we would still be sending
> things like most visited to the incognito InstantTab. To make it safer, I now
> explicitly don't send anything but the bare minimum for InstantPages used in
> incognito mode.
> 
> PTAL (I probably also need to update some unit tests. Will do that shortly.)
> 
> Thanks,
> Samarth

Wow, that's a little disturbing. This makes me more nervous about using
InstantTab in incognito, since it seems like it would be easy to add
something new and then forget to turn it off for incognito. I'm not sure
the original bug is worth the risk. I'll take a more careful look here.

[samarth1, 2013-06-20 17:49:30]

I had the same concern which is why I decided to put the checks in
InstantPage which is where all new data would have to flow through.  That
should make it easy to audit any new additions.

Note that today we are preloading a google.com page while you're in
incognito (InstantNTP) and sending it various bits of information re theme,
etc.  So this is strictly better I think.

Thanks,
Samarth


On Thu, Jun 20, 2013 at 10:15 AM, <jered@chromium.org> wrote:

> On 2013/06/20 16:49:25, samarth wrote:
>
>> I realized my initial change was not correct because we would still be
>> sending
>> things like most visited to the incognito InstantTab. To make it safer, I
>> now
>> explicitly don't send anything but the bare minimum for InstantPages used
>> in
>> incognito mode.
>>
>
>  PTAL (I probably also need to update some unit tests. Will do that
>> shortly.)
>>
>
>  Thanks,
>> Samarth
>>
>
> Wow, that's a little disturbing. This makes me more nervous about using
> InstantTab in incognito, since it seems like it would be easy to add
> something new and then forget to turn it off for incognito. I'm not sure
> the original bug is worth the risk. I'll take a more careful look here.
>
>
https://codereview.chromium.**org/17303003/<https://codereview.chromium.org/1...
>

[samarth1, 2013-06-20 17:51:12]

On Thu, Jun 20, 2013 at 10:48 AM, Samarth Keshava <samarth@google.com>wrote:

> Note that today we are preloading a google.com page while you're in
> incognito (InstantNTP) and sending it various bits of information re theme,
> etc.  So this is strictly better I think.
>

Actually that's not true since local_only_ is set for incognito profiles.
 In any case, I do like explicitly stopping any flow of sensitive
information in InstantPage when you're in incognito.


>
> Thanks,
> Samarth
>
>
> On Thu, Jun 20, 2013 at 10:15 AM, <jered@chromium.org> wrote:
>
>> On 2013/06/20 16:49:25, samarth wrote:
>>
>>> I realized my initial change was not correct because we would still be
>>> sending
>>> things like most visited to the incognito InstantTab. To make it safer,
>>> I now
>>> explicitly don't send anything but the bare minimum for InstantPages
>>> used in
>>> incognito mode.
>>>
>>
>>  PTAL (I probably also need to update some unit tests. Will do that
>>> shortly.)
>>>
>>
>>  Thanks,
>>> Samarth
>>>
>>
>> Wow, that's a little disturbing. This makes me more nervous about using
>> InstantTab in incognito, since it seems like it would be easy to add
>> something new and then forget to turn it off for incognito. I'm not sure
>> the original bug is worth the risk. I'll take a more careful look here.
>>
>>
https://codereview.chromium.**org/17303003/<https://codereview.chromium.org/1...
>>
>
>

[Jered, 2013-06-20 18:55:34]

Per offline discussion, instead of all these if (!incognito) checks, how about
we try to split the implementation of message routing into a Router which is
specialized for incognito?

https://codereview.chromium.org/17303003/diff/17001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_ntp.h (right):

https://codereview.chromium.org/17303003/diff/17001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_ntp.h:25: bool off_the_record);
"is_incognito" would be clearer.

https://codereview.chromium.org/17303003/diff/17001/chrome/browser/ui/search/...
File chrome/browser/ui/search/instant_tab.h (right):

https://codereview.chromium.org/17303003/diff/17001/chrome/browser/ui/search/...
chrome/browser/ui/search/instant_tab.h:16: explicit
InstantTab(InstantPage::Delegate* delegate,
Remove explicit.

[samarth, 2013-06-21 14:06:37]

PTAL
On Jun 20, 2013 11:55 AM, <jered@chromium.org> wrote:

> Per offline discussion, instead of all these if (!incognito) checks, how
> about
> we try to split the implementation of message routing into a Router which
> is
> specialized for incognito?
>
>
> https://codereview.chromium.**org/17303003/diff/17001/**
>
chrome/browser/ui/search/**instant_ntp.h<https://codereview.chromium.org/17303003/diff/17001/chrome/browser/ui/search/instant_ntp.h>
> File chrome/browser/ui/search/**instant_ntp.h (right):
>
> https://codereview.chromium.**org/17303003/diff/17001/**
>
chrome/browser/ui/search/**instant_ntp.h#newcode25<https://codereview.chromium.org/17303003/diff/17001/chrome/browser/ui/search/instant_ntp.h#newcode25>
> chrome/browser/ui/search/**instant_ntp.h:25: bool off_the_record);
> "is_incognito" would be clearer.
>
> https://codereview.chromium.**org/17303003/diff/17001/**
>
chrome/browser/ui/search/**instant_tab.h<https://codereview.chromium.org/17303003/diff/17001/chrome/browser/ui/search/instant_tab.h>
> File chrome/browser/ui/search/**instant_tab.h (right):
>
> https://codereview.chromium.**org/17303003/diff/17001/**
>
chrome/browser/ui/search/**instant_tab.h#newcode16<https://codereview.chromium.org/17303003/diff/17001/chrome/browser/ui/search/instant_tab.h#newcode16>
> chrome/browser/ui/search/**instant_tab.h:16: explicit
> InstantTab(InstantPage::**Delegate* delegate,
> Remove explicit.
>
>
https://codereview.chromium.**org/17303003/<https://codereview.chromium.org/1...
>

[samarth, 2013-06-21 14:08:56]

FYI I didn't bother with extracting out the message listener into a helper
since we're not listening to any of them in incognito anyway. Plus it would
add a lot of complexity to refactor that out.

Thanks,
Samarth
On Jun 21, 2013 7:06 AM, "Samarth Keshava" <samarth@chromium.org> wrote:

> PTAL
> On Jun 20, 2013 11:55 AM, <jered@chromium.org> wrote:
>
>> Per offline discussion, instead of all these if (!incognito) checks, how
>> about
>> we try to split the implementation of message routing into a Router which
>> is
>> specialized for incognito?
>>
>>
>> https://codereview.chromium.**org/17303003/diff/17001/**
>>
chrome/browser/ui/search/**instant_ntp.h<https://codereview.chromium.org/17303003/diff/17001/chrome/browser/ui/search/instant_ntp.h>
>> File chrome/browser/ui/search/**instant_ntp.h (right):
>>
>> https://codereview.chromium.**org/17303003/diff/17001/**
>>
chrome/browser/ui/search/**instant_ntp.h#newcode25<https://codereview.chromium.org/17303003/diff/17001/chrome/browser/ui/search/instant_ntp.h#newcode25>
>> chrome/browser/ui/search/**instant_ntp.h:25: bool off_the_record);
>> "is_incognito" would be clearer.
>>
>> https://codereview.chromium.**org/17303003/diff/17001/**
>>
chrome/browser/ui/search/**instant_tab.h<https://codereview.chromium.org/17303003/diff/17001/chrome/browser/ui/search/instant_tab.h>
>> File chrome/browser/ui/search/**instant_tab.h (right):
>>
>> https://codereview.chromium.**org/17303003/diff/17001/**
>>
chrome/browser/ui/search/**instant_tab.h#newcode16<https://codereview.chromium.org/17303003/diff/17001/chrome/browser/ui/search/instant_tab.h#newcode16>
>> chrome/browser/ui/search/**instant_tab.h:16: explicit
>> InstantTab(InstantPage::**Delegate* delegate,
>> Remove explicit.
>>
>>
https://codereview.chromium.**org/17303003/<https://codereview.chromium.org/1...
>>
>

[samarth, 2013-06-21 16:24:51]

Also added unit tests.

[Jered, 2013-06-21 17:58:29]

On 2013/06/21 16:24:51, samarth wrote:
> Also added unit tests.

instant_ipc_sender.{cc,h} are missing.

[samarth, 2013-06-21 18:44:23]

Oops, added them back.

[Jered, 2013-06-21 19:07:28]

On 2013/06/21 18:44:23, samarth wrote:
> Oops, added them back.

lgtm

This feels much safer. Add DISALLOW_COPY_AND_ASSIGN to the new classes.

[samarth, 2013-06-22 00:25:26]

Committed patchset #18 manually as r207969 (presubmit successful).