Issue 10825179: Disable the prefetching trial during the CloudPrintPolicyFlag test.

Issue 10825179: Disable the prefetching trial during the CloudPrintPolicyFlag test. (Closed)

Created:
8 years, 4 months ago by Joao da Silva

Modified:
8 years, 4 months ago

Reviewers:
dominich, cbentzel, Scott Byer

CC:
chromium-reviews, tburkard+watch_chromium.org, gavinp+prer_chromium.org, cbentzel+watch_chromium.org, dominich+watch_chromium.org, mmenke

Base URL:
svn://svn.chromium.org/chrome/trunk/src

Visibility:
Public.

More Reviews

Description

Disable the prefetching trial during the CloudPrintPolicyFlag test. TBR=scottbyer@chromium.org,dominich@chromium.org BUG=140466 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=149841

Patch Set 1 #

Created: 8 years, 4 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+4 lines, -2 lines)			Patch
	M	chrome/browser/prerender/prerender_field_trial.cc	View		1 chunk	+1 line, -2 lines	0 comments	Download
	M	chrome/browser/printing/cloud_print/test/cloud_print_policy_browsertest.cc	View		1 chunk	+3 lines, -0 lines	0 comments	Download

Messages

Total messages: 4 (0 generated)

Expand Messages | Collapse Messages

cbentzel

Did you ever figure out why there was bad interaction? The bug doesn't seem to ...

8 years, 4 months ago (2012-08-03 14:09:25 UTC) #2

Did you ever figure out why there was bad interaction? The bug doesn't seem
to indicate why.

On Fri, Aug 3, 2012 at 8:43 AM, <joaodasilva@chromium.org> wrote:

> Reviewers: Scott Byer, dominich,
>
> Description:
> Disable the prefetching trial during the CloudPrintPolicyFlag test.
>
> TBR=scottbyer@chromium.org,dom**inich@chromium.org <dominich@chromium.org>
> BUG=140466
>
> Committed: https://src.chromium.org/**viewvc/chrome?view=rev&**
>
revision=149841<https://src.chromium.org/viewvc/chrome?view=rev&revision=149841>
>
> Please review this at
https://chromiumcodereview.**appspot.com/10825179/<https://chromiumcodereview...
>
> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>
> Affected files:
>   M chrome/browser/prerender/**prerender_field_trial.cc
>   M chrome/browser/printing/cloud_**print/test/cloud_print_policy_**
> browsertest.cc
>
>
> Index: chrome/browser/prerender/**prerender_field_trial.cc
> diff --git a/chrome/browser/prerender/**prerender_field_trial.cc
> b/chrome/browser/prerender/**prerender_field_trial.cc
> index 388ff263522bf5e5c5ca9768acb7f4**4871554f1b..**
> e47c412b9fa8487351c2f0160e8c30**0fa53e90e5 100644
> --- a/chrome/browser/prerender/**prerender_field_trial.cc
> +++ b/chrome/browser/prerender/**prerender_field_trial.cc
> @@ -285,8 +285,7 @@ bool
IsSpeculativeResourcePrefetchi**ngLearningEnabled(Profile*
> profile) {
>          CommandLine::**ForCurrentProcess()->**GetSwitchValueASCII(
>              switches::**kSpeculativeResourcePrefetchin**g);
>
> -    if (switch_value == switches::**kSpeculativeResourcePrefetchin**
> gLearning)
> -      return true;
> +    return switch_value == switches::**kSpeculativeResourcePrefetchin**
> gLearning;
>    }
>
>    const int group = FieldTrialList::FindValue(
> Index: chrome/browser/printing/cloud_**print/test/cloud_print_policy_**
> browsertest.cc
> diff --git
a/chrome/browser/printing/**cloud_print/test/cloud_print_**policy_browsertest.cc
> b/chrome/browser/printing/**cloud_print/test/cloud_print_**
> policy_browsertest.cc
> index 8ce63b49b461e81cae53fa917fb4e0**8917f7e684..**
> c7151794823e760ebc9a774eb7cd6d**c6f910822f 100644
> --- a/chrome/browser/printing/**cloud_print/test/cloud_print_**
> policy_browsertest.cc
> +++ b/chrome/browser/printing/**cloud_print/test/cloud_print_**
> policy_browsertest.cc
> @@ -58,6 +58,9 @@ IN_PROC_BROWSER_TEST_F(**CloudPrintPolicyTest,
> NormalPassedFlag) {
>  IN_PROC_BROWSER_TEST_F(**CloudPrintPolicyTest, CloudPrintPolicyFlag) {
>    CommandLine new_command_line(**GetCommandLineForRelaunch());
>    new_command_line.AppendSwitch(**switches::**
> kCheckCloudPrintConnectorPolic**y);
> +  new_command_line.**AppendSwitchASCII(
> +      switches::**kSpeculativeResourcePrefetchin**g,
> +      switches::**kSpeculativeResourcePrefetchin**gDisabled);
>
>    base::ProcessHandle handle;
>    bool launched =
>
>
>

Joao da Silva

On Fri, Aug 3, 2012 at 4:09 PM, Chris Bentzel <cbentzel@chromium.org> wrote: > Did you ...

8 years, 4 months ago (2012-08-03 14:16:59 UTC) #3

On Fri, Aug 3, 2012 at 4:09 PM, Chris Bentzel <cbentzel@chromium.org> wrote:

> Did you ever figure out why there was bad interaction? The bug doesn't
> seem to indicate why.


I've just posted the stack trace on the bug. I tried to but didn't find the
root cause, so I just patched the test to make the tree green without
completely disabling the test.


>
>
> On Fri, Aug 3, 2012 at 8:43 AM, <joaodasilva@chromium.org> wrote:
>
>> Reviewers: Scott Byer, dominich,
>>
>> Description:
>> Disable the prefetching trial during the CloudPrintPolicyFlag test.
>>
>> TBR=scottbyer@chromium.org,dom**inich@chromium.org<dominich@chromium.org>
>> BUG=140466
>>
>> Committed: https://src.chromium.org/**viewvc/chrome?view=rev&**
>>
revision=149841<https://src.chromium.org/viewvc/chrome?view=rev&revision=149841>
>>
>> Please review this at
https://chromiumcodereview.**appspot.com/10825179/<https://chromiumcodereview...
>>
>> SVN Base:
svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src>
>>
>> Affected files:
>>   M chrome/browser/prerender/**prerender_field_trial.cc
>>   M chrome/browser/printing/cloud_**print/test/cloud_print_policy_**
>> browsertest.cc
>>
>>
>> Index: chrome/browser/prerender/**prerender_field_trial.cc
>> diff --git a/chrome/browser/prerender/**prerender_field_trial.cc
>> b/chrome/browser/prerender/**prerender_field_trial.cc
>> index 388ff263522bf5e5c5ca9768acb7f4**4871554f1b..**
>> e47c412b9fa8487351c2f0160e8c30**0fa53e90e5 100644
>> --- a/chrome/browser/prerender/**prerender_field_trial.cc
>> +++ b/chrome/browser/prerender/**prerender_field_trial.cc
>> @@ -285,8 +285,7 @@ bool
IsSpeculativeResourcePrefetchi**ngLearningEnabled(Profile*
>> profile) {
>>          CommandLine::**ForCurrentProcess()->**GetSwitchValueASCII(
>>              switches::**kSpeculativeResourcePrefetchin**g);
>>
>> -    if (switch_value == switches::**kSpeculativeResourcePrefetchin**
>> gLearning)
>> -      return true;
>> +    return switch_value == switches::**kSpeculativeResourcePrefetchin**
>> gLearning;
>>    }
>>
>>    const int group = FieldTrialList::FindValue(
>> Index: chrome/browser/printing/cloud_**print/test/cloud_print_policy_**
>> browsertest.cc
>> diff --git
a/chrome/browser/printing/**cloud_print/test/cloud_print_**policy_browsertest.cc
>> b/chrome/browser/printing/**cloud_print/test/cloud_print_**
>> policy_browsertest.cc
>> index 8ce63b49b461e81cae53fa917fb4e0**8917f7e684..**
>> c7151794823e760ebc9a774eb7cd6d**c6f910822f 100644
>> --- a/chrome/browser/printing/**cloud_print/test/cloud_print_**
>> policy_browsertest.cc
>> +++ b/chrome/browser/printing/**cloud_print/test/cloud_print_**
>> policy_browsertest.cc
>> @@ -58,6 +58,9 @@ IN_PROC_BROWSER_TEST_F(**CloudPrintPolicyTest,
>> NormalPassedFlag) {
>>  IN_PROC_BROWSER_TEST_F(**CloudPrintPolicyTest, CloudPrintPolicyFlag) {
>>    CommandLine new_command_line(**GetCommandLineForRelaunch());
>>    new_command_line.AppendSwitch(**switches::**
>> kCheckCloudPrintConnectorPolic**y);
>> +  new_command_line.**AppendSwitchASCII(
>> +      switches::**kSpeculativeResourcePrefetchin**g,
>> +      switches::**kSpeculativeResourcePrefetchin**gDisabled);
>>
>>    base::ProcessHandle handle;
>>    bool launched =
>>
>>
>>
>

Scott Byer

8 years, 4 months ago (2012-08-03 16:56:02 UTC) #4

LGTM as far as the change goes, but as I mentioned in the bug, this probably
isn't sufficient; any flag that causes the browser to try and silent_launch
should disable the predictor (and possibly other things as well). I think the
test before this change caught something valid :-).

Expand Messages | Collapse Messages