Do conservative prerendering based on the LocalPredictor in Dev/Canary.

Do conservative prerendering based on the LocalPredictor in Dev/Canary.
Also disables SideEffectFreeWhitelist field trial.
BUG=239180
R=shishir@chromium.org, thakis@chromium.org

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

[Shishir, 2013-05-07 22:57:30]

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_field_trial.cc:236:
chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel();
Since this can have bad side effects there should be a way for users to turn
this off on the dev channel. Add a flag in about:flags?

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:56: struct
PrerenderLocalPredictor::LocalPredictorURLInfo {
Add comment about struct.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:65: struct
PrerenderLocalPredictor::LocalPredictorURLLookupInfo {
Add comments about struct.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:78: while (insert_pos > 0
&&
Maybe use a list so you can just insert in the right position.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:95: request,
indent 4 spaces.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:196: bool
StrCaseStr(string haystack, string needle) {
Maybe rename to StringContainsIgnoringCase?

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:435: if
(lookup_info->candidate_urls_.size() < 1) {
== 0

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:449:
base::Unretained(this),
unreatined? What if the predictor is deleted?

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:460:
!(info->candidate_urls_.size() >= 1) ||
This condition cannot happen, so you can add a dcheck.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:461:
!info->candidate_urls_[0].url_lookup_success) {
Cant you use the second candidate if the first's lookup fails?

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:471: for
(TabContentsIterator it; !it.done(); it.Next()) {
Need to document this correctly since this is not straight forward.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:668: break;
Why is this case different? Documentation?

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:681:
RecordEvent(EVENT_CONTINUE_PRERENDER_CHECK_NOT_LOGGED_IN);
Shouldn't this be checked early?

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tab_helper.cc:156: if
(IsLocalPredictorEnabled())
In the previous line you are marking the web_contents as not prerendering and
then trying to prerender it. Looks like the prerender manager will be
inconsistent. Also is ProvisionalChangeToMainFrameUrl the right place to do this
swap? We currently do it in chrome::Navigate.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tab_helper.cc:178: if
(IsLocalPredictorEnabled())
Why are we trying to swap at both start and commit of provisional load? Isn't
just one enough?

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
Test for local predictor prerender?

[tburkard, 2013-05-07 23:21:59]

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_field_trial.cc:236:
chrome::VersionInfo::Channel channel = chrome::VersionInfo::GetChannel();
I will do this separately (before committing), but wanted to send out these
comments to you first.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:56: struct
PrerenderLocalPredictor::LocalPredictorURLInfo {
On 2013/05/07 22:57:30, Shishir wrote:
> Add comment about struct.

Done.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:65: struct
PrerenderLocalPredictor::LocalPredictorURLLookupInfo {
On 2013/05/07 22:57:30, Shishir wrote:
> Add comments about struct.

Done.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:78: while (insert_pos > 0
&&
I'd prefer to keep it the way it is for now, since we are just talking about 5
items and this works and is already written. Also these thing will be likely to
be changed in the near future as we experiment further.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:95: request,
On 2013/05/07 22:57:30, Shishir wrote:
> indent 4 spaces.

Done.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:196: bool
StrCaseStr(string haystack, string needle) {
On 2013/05/07 22:57:30, Shishir wrote:
> Maybe rename to StringContainsIgnoringCase?

Done.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:435: if
(lookup_info->candidate_urls_.size() < 1) {
On 2013/05/07 22:57:30, Shishir wrote:
> == 0

Done.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:449:
base::Unretained(this),
The use of the history_db_consumer_ will prevent anything bad from happening in
this case.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:460:
!(info->candidate_urls_.size() >= 1) ||
On 2013/05/07 22:57:30, Shishir wrote:
> This condition cannot happen, so you can add a dcheck.

Done.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:461:
!info->candidate_urls_[0].url_lookup_success) {
On 2013/05/07 22:57:30, Shishir wrote:
> Cant you use the second candidate if the first's lookup fails?

Done.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:471: for
(TabContentsIterator it; !it.done(); it.Next()) {
On 2013/05/07 22:57:30, Shishir wrote:
> Need to document this correctly since this is not straight forward.

Done.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:668: break;
On 2013/05/07 22:57:30, Shishir wrote:
> Why is this case different? Documentation?

Done.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:681:
RecordEvent(EVENT_CONTINUE_PRERENDER_CHECK_NOT_LOGGED_IN);
On 2013/05/07 22:57:30, Shishir wrote:
> Shouldn't this be checked early?

Done.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tab_helper.cc:156: if
(IsLocalPredictorEnabled())
No, this only means that the data currently in the webcontents was not
prerendered. This is done because the URL changed. If we swap in, we will update
this to say that it was prerendered, based on the swap-in. So this is correct
(in particular, because of the order).

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tab_helper.cc:178: if
(IsLocalPredictorEnabled())
Some URL changes only show up in one or the other, but not in both, which is why
we need this twice.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
Local predictor is still all over the place and will still change a ton, so I'd
rather hold off on unit testing until its behavior has settled more.

[tburkard, 2013-05-08 00:19:21]

Added the about:flag disable mechanism.

This should address all the comments you had, please take a look.

Thanks.

[tburkard, 2013-05-08 00:25:26]

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:681:
RecordEvent(EVENT_CONTINUE_PRERENDER_CHECK_NOT_LOGGED_IN);
Sorry, didn't properly reply to this. No, the location of the check is just
right: we want to prerender root pages even for logged in users, and we don't
want to prerender URLs which contain "login/logout" for any users. Ditto https
and fragments. That's why these other checks come first, since they prescribe an
action independent of logged in state.

On 2013/05/07 22:57:30, Shishir wrote:
> Shouldn't this be checked early?

[tburkard, 2013-05-08 20:06:16]

thakis: could you please review chrome/common and chrome/app, addition of an
about flag and a switch?

Thanks :)

[Shishir, 2013-05-08 20:19:20]

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:78: while (insert_pos > 0
&&
On 2013/05/07 23:21:59, tburkard wrote:
> I'd prefer to keep it the way it is for now, since we are just talking about 5
> items and this works and is already written. Also these thing will be likely
to
> be changed in the near future as we experiment further.
> 

I still think this code is messy. Eg. it has the following check three times.
Add a todo if you will leave as is.

if (insert_pos < kNumPrerenderCandidates)

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:681:
RecordEvent(EVENT_CONTINUE_PRERENDER_CHECK_NOT_LOGGED_IN);
On 2013/05/08 00:25:27, tburkard wrote:
> Sorry, didn't properly reply to this. No, the location of the check is just
> right: we want to prerender root pages even for logged in users, and we don't
> want to prerender URLs which contain "login/logout" for any users. Ditto https
> and fragments. That's why these other checks come first, since they prescribe
an
> action independent of logged in state.
> 
> On 2013/05/07 22:57:30, Shishir wrote:
> > Shouldn't this be checked early?
> 

Since the sequence of checks is to a certain logic, can you write a comment in
this regard before the checks begin.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tab_helper.cc:178: if
(IsLocalPredictorEnabled())
On 2013/05/07 23:21:59, tburkard wrote:
> Some URL changes only show up in one or the other, but not in both, which is
why
> we need this twice.

Can you mention which ones show up on in only one or other? Isn't the commit of
a load a bit late to swap in prerendering. It possibly means that we have
recieved a response for the request.

https://codereview.chromium.org/15021007/diff/6015/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/15021007/diff/6015/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_field_trial.cc:306: #else
remove the else.

https://codereview.chromium.org/15021007/diff/6015/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/15021007/diff/6015/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_local_predictor.cc:465:
DCHECK(info->candidate_urls_.size() >= 1);
DCHECK_GE(info.., 1);

https://codereview.chromium.org/15021007/diff/6015/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_local_predictor.cc:480: // may have navigated
elsewhere. Hopefully, this doesn't happen to often,
s/to/too

https://codereview.chromium.org/15021007/diff/6015/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_local_predictor.h (right):

https://codereview.chromium.org/15021007/diff/6015/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_local_predictor.h:38: struct
LocalPredictorURLInfo;
Can you make these structs private? Looks like only this class needs them.

https://codereview.chromium.org/15021007/diff/6015/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_local_predictor.h:101: void
OnLookupURL(scoped_ptr<LocalPredictorURLLookupInfo> info);
make private.

[Nico, 2013-05-08 20:22:09]

From the CL description, this sounds big enough that it should have a BUG= line
with a tracking bug.

For features, the flag progression is usually:
1. Land feature
2. Land opt-in flag
3. Switch to opt-out flag
4. Delete flag

Since this is using an opt-out flag immediately, do you need the flag at all?

[tburkard, 2013-05-08 20:35:46]

Re: Shishir's comments.
Nico, I will respond to yours in a sec.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:78: while (insert_pos > 0
&&
On 2013/05/08 20:19:20, Shishir wrote:
> On 2013/05/07 23:21:59, tburkard wrote:
> > I'd prefer to keep it the way it is for now, since we are just talking about
5
> > items and this works and is already written. Also these thing will be likely
> to
> > be changed in the near future as we experiment further.
> > 
> 
> I still think this code is messy. Eg. it has the following check three times.
> Add a todo if you will leave as is.
> 
> if (insert_pos < kNumPrerenderCandidates) 

Done.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_local_predictor.cc:681:
RecordEvent(EVENT_CONTINUE_PRERENDER_CHECK_NOT_LOGGED_IN);
On 2013/05/08 20:19:20, Shishir wrote:
> On 2013/05/08 00:25:27, tburkard wrote:
> > Sorry, didn't properly reply to this. No, the location of the check is just
> > right: we want to prerender root pages even for logged in users, and we
don't
> > want to prerender URLs which contain "login/logout" for any users. Ditto
https
> > and fragments. That's why these other checks come first, since they
prescribe
> an
> > action independent of logged in state.
> > 
> > On 2013/05/07 22:57:30, Shishir wrote:
> > > Shouldn't this be checked early?
> > 
> 
> Since the sequence of checks is to a certain logic, can you write a comment in
> this regard before the checks begin.

Done.

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
File chrome/browser/prerender/prerender_tab_helper.cc (right):

https://codereview.chromium.org/15021007/diff/36009/chrome/browser/prerender/...
chrome/browser/prerender/prerender_tab_helper.cc:178: if
(IsLocalPredictorEnabled())
Removing these changes in this file altogether, since Chris is preparing a new
change to address this that is cleaner.

https://codereview.chromium.org/15021007/diff/6015/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_field_trial.cc (right):

https://codereview.chromium.org/15021007/diff/6015/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_field_trial.cc:306: #else
On 2013/05/08 20:19:20, Shishir wrote:
> remove the else.

Done.

https://codereview.chromium.org/15021007/diff/6015/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_local_predictor.cc (right):

https://codereview.chromium.org/15021007/diff/6015/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_local_predictor.cc:465:
DCHECK(info->candidate_urls_.size() >= 1);
On 2013/05/08 20:19:20, Shishir wrote:
> DCHECK_GE(info.., 1);

Done.

https://codereview.chromium.org/15021007/diff/6015/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_local_predictor.cc:480: // may have navigated
elsewhere. Hopefully, this doesn't happen to often,
On 2013/05/08 20:19:20, Shishir wrote:
> s/to/too

Done.

https://codereview.chromium.org/15021007/diff/6015/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_local_predictor.h (right):

https://codereview.chromium.org/15021007/diff/6015/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_local_predictor.h:38: struct
LocalPredictorURLInfo;
can't be made private, used by anonymous helper routines in .cc

https://codereview.chromium.org/15021007/diff/6015/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_local_predictor.h:101: void
OnLookupURL(scoped_ptr<LocalPredictorURLLookupInfo> info);
On 2013/05/08 20:19:20, Shishir wrote:
> make private.

Done.

[tburkard, 2013-05-08 20:40:11]

On 2013/05/08 20:22:09, Nico wrote:
> From the CL description, this sounds big enough that it should have a BUG=
line
> with a tracking bug.
Done.
 
> Since this is using an opt-out flag immediately, do you need the flag at all?
Yes, since we want to experiment with this in Dev/Canary, and we are very
confident that this will not impact users and does not need to be turned off.
This is more of an emergency mechanism that should individual users experience
issues and would like to be able to turn it off, they can do it using this flag.

[Nico, 2013-05-08 20:51:09]

On Wed, May 8, 2013 at 1:40 PM, <tburkard@chromium.org> wrote:

> On 2013/05/08 20:22:09, Nico wrote:
>
>>  From the CL description, this sounds big enough that it should have a
>> BUG=
>>
> line
>
>> with a tracking bug.
>>
> Done.
>
>
>  Since this is using an opt-out flag immediately, do you need the flag at
>> all?
>>
> Yes, since we want to experiment with this in Dev/Canary, and we are very
> confident that this will not impact users and does not need to be turned
> off.
> This is more of an emergency mechanism that should individual users
> experience
> issues and would like to be able to turn it off, they can do it using this
> flag.
>

Couldn't you just revert the CL then, or fix whatever bug people might see?
Dev / Canary is expected to be bumpy.

If you want to add the flag, what's your criterion for removing it again?


>
>
>
https://codereview.chromium.**org/15021007/<https://codereview.chromium.org/1...
>

[tburkard, 2013-05-08 20:53:15]

I don't feel strongly about adding the flag, shishir@ on this review made a
case for it. Shishir - could you back up your case for this to Nico, please
(Nico, also see Shishir's comment above). I think the flag does make sense
though.

We will experiment with this for a few months. At the conclusion of the
experimentation, we will either disable this entirely, or put it into
stable. In both cases, we'd remove the flag. In particular, once we get
more confident that this actually does not impact users, we will remove the
flag.

[Nico, 2013-05-08 20:57:32]

If you want to do a/b experiments, have you considered Finch?

If this is just for people to turn it off in case it's buggy, I think you
should just fix bugs as they arrive and not speculatively add a flag that
might not be necessary.

(I don't feel terribly strongly about this though; if you really want it,
keep it, as long as you remove it at some point. I'm just not a fan of
adding complexity that might not be necessary. If you think it's necessary
then lgtm.)


On Wed, May 8, 2013 at 1:53 PM, Timo Burkard <tburkard@chromium.org> wrote:

> I don't feel strongly about adding the flag, shishir@ on this review made
> a case for it. Shishir - could you back up your case for this to Nico,
> please (Nico, also see Shishir's comment above). I think the flag does make
> sense though.
>
> We will experiment with this for a few months. At the conclusion of the
> experimentation, we will either disable this entirely, or put it into
> stable. In both cases, we'd remove the flag. In particular, once we get
> more confident that this actually does not impact users, we will remove the
> flag.
>

[Shishir, 2013-05-08 20:58:26]

On 2013/05/08 20:53:15, tburkard wrote:
> I don't feel strongly about adding the flag, shishir@ on this review made a
> case for it. Shishir - could you back up your case for this to Nico, please
> (Nico, also see Shishir's comment above). I think the flag does make sense
> though.
> 
> We will experiment with this for a few months. At the conclusion of the
> experimentation, we will either disable this entirely, or put it into
> stable. In both cases, we'd remove the flag. In particular, once we get
> more confident that this actually does not impact users, we will remove the
> flag.

The reason to have the flag:

- Was to be able to turn of this feature if it has really averse side effects. 
E.g. it prerenders a page for a user that logs him out. Since this could only
happen to some particular users and may not even be fixable easily via code,
having a kill switch makes sense.

- There are other features in dev/canary that I would like to continue testing
and if this feature interferes with them, might be nice to be able to disable
it.

[tburkard, 2013-05-08 20:58:31]

I'm perfectly fine removing it, in fact, that's the way i had it before.
Shishir, please let us know if, given Nico's points, you think it's still
necessary, and let us know why, and we'll go with that.

Thanks.

[tburkard, 2013-05-08 20:59:22]

I think Shishir made two excellent points here, so based on that, I'd
prefer having the flag now as well.

Thanks.

[Nico, 2013-05-08 21:02:36]

On Wed, May 8, 2013 at 1:58 PM, <shishir@chromium.org> wrote:

> On 2013/05/08 20:53:15, tburkard wrote:
>
>> I don't feel strongly about adding the flag, shishir@ on this review
>> made a
>> case for it. Shishir - could you back up your case for this to Nico,
>> please
>> (Nico, also see Shishir's comment above). I think the flag does make sense
>> though.
>>
>
>  We will experiment with this for a few months. At the conclusion of the
>> experimentation, we will either disable this entirely, or put it into
>> stable. In both cases, we'd remove the flag. In particular, once we get
>> more confident that this actually does not impact users, we will remove
>> the
>> flag.
>>
>
> The reason to have the flag:
>
> - Was to be able to turn of this feature if it has really averse side
> effects.
> E.g. it prerenders a page for a user that logs him out. Since this could
> only
> happen to some particular users and may not even be fixable easily via
> code,
> having a kill switch makes sense.
>

Would you tell those uses "just disable feature X" though? Most users don't
know about flags and will never know that disabling the feature helps.
Worst case, random blog posts on the interwebs will say "random problem Y
went away after i disabled this, so you might want to do that too".

*shrug*


>
> - There are other features in dev/canary that I would like to continue
> testing
> and if this feature interferes with them, might be nice to be able to
> disable
> it.
>
>
https://codereview.chromium.**org/15021007/<https://codereview.chromium.org/1...
>

[tburkard, 2013-05-08 21:19:34]

I think shishir does make a good point re: testing though. But for testing,
just a flag would suffice. Shishir, what do you think?

[Shishir, 2013-05-08 21:21:23]

On 2013/05/08 21:19:34, tburkard wrote:
> I think shishir does make a good point re: testing though. But for testing,
> just a flag would suffice. Shishir, what do you think?

Would work.

[Nico, 2013-05-08 21:22:50]

As I said, lgtm for whatever you want to do as long as the flag goes away
at some well-defined point.


On Wed, May 8, 2013 at 2:21 PM, <shishir@chromium.org> wrote:

> On 2013/05/08 21:19:34, tburkard wrote:
>
>> I think shishir does make a good point re: testing though. But for
>> testing,
>> just a flag would suffice. Shishir, what do you think?
>>
>
> Would work.
>
>
https://codereview.chromium.**org/15021007/<https://codereview.chromium.org/1...
>

[Shishir, 2013-05-08 21:25:09]

Is the latest patch up to date? I dont see the prerender tab helper.

[tburkard, 2013-05-08 21:26:18]

Yes, it is up to date. I removed prerender_tab_helper (I wrote that in the
response to your concern, too), since Chris is doing a different fix for
this that's more long-term. He already has it ready:
https://codereview.chromium.org/15027009/

[tburkard, 2013-05-08 21:28:19]

Nico: Per shishir, removed the about flag, now there's only a switch. (see
new patchset)

[Shishir, 2013-05-08 22:49:26]

prerender lgtm.

[tburkard, 2013-05-08 23:05:50]

Committed patchset #12 manually as r199040 (presubmit successful).