[Cleanup] Move all browsertests to use ScopedFeatureList to modify features

[Cleanup] Move all browsertests to use ScopedFeatureList to modify features

BUG=727044

[chaopeng, 2017-05-27 20:08:06]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-27 20:08:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-27 20:21:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-27 20:21:46]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chaopeng, 2017-05-27 20:37:41]

Patchset #1 (id:1) has been deleted

[chaopeng, 2017-05-27 20:37:50]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-27 20:37:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-27 20:48:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-27 20:48:04]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[chaopeng, 2017-05-27 21:06:38]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-27 21:06:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chaopeng, 2017-05-27 21:06:45]

Patchset #1 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-05-27 22:38:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-27 22:38:43]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[chaopeng, 2017-06-06 20:42:25]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 20:42:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 20:45:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-06 20:45:58]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chaopeng, 2017-06-06 20:56:12]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 20:56:29]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 21:23:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-06 21:23:16]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[chaopeng, 2017-06-06 22:14:58]

Patchset #3 (id:80001) has been deleted

[chaopeng, 2017-06-06 22:15:08]

Patchset #2 (id:60001) has been deleted

[chaopeng, 2017-06-06 22:15:16]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 22:15:35]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 22:33:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-06 22:33:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chaopeng, 2017-06-06 22:41:03]

Patchset #2 (id:100001) has been deleted

[chaopeng, 2017-06-06 22:41:39]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 22:41:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-06 23:56:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-06 23:56:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chaopeng, 2017-06-12 18:00:38]

Description was changed from

==========
Change all browsertests to use ScopedFeatureList to modify features

BUG=727044
==========

to

==========
Change all browsertests to use ScopedFeatureList to modify features

BUG=727044
==========

[chaopeng, 2017-06-12 18:00:38]

chaopeng@chromium.org changed reviewers:
+ isherman@chromium.org

[chaopeng, 2017-06-12 18:04:17]

Description was changed from

==========
Change all browsertests to use ScopedFeatureList to modify features

BUG=727044
==========

to

==========
Move all browsertests to use ScopedFeatureList to modify features

BUG=727044
==========

[chaopeng, 2017-06-12 18:04:35]

Description was changed from

==========
Move all browsertests to use ScopedFeatureList to modify features

BUG=727044
==========

to

==========
[Cleanup] Move all browsertests to use ScopedFeatureList to modify features

BUG=727044
==========

[chaopeng, 2017-06-12 18:04:52]

Ilya, PTAL. Thank you.

[Ilya Sherman, 2017-06-12 21:50:29]

Thanks.

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/app_view_browsertest.cc (left):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/app_view_browsertest.cc:18: #include
"content/public/common/content_switches.h"
This change seems unrelated.  Could you please split it off to a separate CL?

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/extension_view/extension_view_browsertest.cc
(left):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/extension_view/extension_view_browsertest.cc:14:
#include "content/public/common/content_switches.h"
This change seems unrelated.  Could you please split it off to a separate CL?

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (left):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_browsertest.cc:3628:
base::test::ScopedFeatureList scoped_feature_list_;
This change seems unrelated.  Could you please split it off to a separate CL?

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/input_method_apitest_chromeos.cc
(right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/input_method_apitest_chromeos.cc:86:
scoped_feature_list_.InitAndEnableFeature(features::kOptInImeMenu);
Does this need to be done as part of SetUpCommandLine?  Could it happen during
SetUp() instead?  SetUpCommandLine() is a much more specialized method, and I'd
prefer to minimize the amount of code in it, for clarity.

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/media/e...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:296:
media::kExternalClearKeyForTesting);
As in the other file, could this be done outside of SetUpCommandLine()?

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:303: #endif  //
BUILDFLAG(ENABLE_PEPPER_CDMS)
Does anything go wrong if you omit this #if, and just always create the feature
list?

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/media/e...
File chrome/browser/media/encrypted_media_supported_types_browsertest.cc
(right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_supported_types_browsertest.cc:308:
media::kExternalClearKeyForTesting);
Could this be done outside of SetUpCommandLine()?

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_supported_types_browsertest.cc:351:
media::kExternalClearKeyForTesting);
Could this be done outside of SetUpCommandLine()?

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/net/pre...
File chrome/browser/net/predictor_browsertest.cc (right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:525:
scoped_feature_list_.InitAndEnableFeature(features::kPreconnectMore);
Could this be done outside of SetUpCommandLine()?  I'm going to stop calling
these out, but please fix this for all of the test files in this CL.

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/net/pre...
File chrome/browser/net/predictor_tab_helper.h (right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/net/pre...
chrome/browser/net/predictor_tab_helper.h:20: }  // namespace features
I don't think that it's appropriate for this header to add to the features
namespace.  Instead, please declare and define this feature in
//chrome/common/chrome_features.{h,cc}.

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:22:
}  // namespace features
Same, for this and other features that you'd like to have public visibility
within //chrome.

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc
(right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc:28:
scoped_feature_list_.InitAndEnableFeature(features::kAdsFeature);
Why are you doing this during SetUpCommandLine, rather than -- say -- during the
test constructor?

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/plugins...
File chrome/browser/plugins/flash_permission_browsertest.cc (right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/plugins...
chrome/browser/plugins/flash_permission_browsertest.cc:58: #if
BUILDFLAG(ENABLE_PLUGINS)
Why did you add this #if wrapper?

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/plugins...
chrome/browser/plugins/flash_permission_browsertest.cc:60: // permission.
Why did you drop the setting of the param?

https://codereview.chromium.org/2910843002/diff/160001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:1115: "//chrome/browser:browser",
Hmm, why did you add this dependency?  It's already included on line 1613.

https://codereview.chromium.org/2910843002/diff/160001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:1116: "//components/safe_browsing_db:v4_feature_list",
Would this be appropriate to move down into the desktop-only section, or is it
needed for Android as well?

https://codereview.chromium.org/2910843002/diff/160001/components/safe_browsi...
File components/safe_browsing_db/v4_feature_list.h (right):

https://codereview.chromium.org/2910843002/diff/160001/components/safe_browsi...
components/safe_browsing_db/v4_feature_list.h:14: }  // namespace features
Please define a separate features.h file for this feature (and a corresponding
impl file for the definition.)

[chaopeng, 2017-06-28 15:59:30]

Patchset #5 (id:180001) has been deleted

[chaopeng, 2017-06-28 19:08:30]

Patchset #7 (id:240001) has been deleted

[chaopeng, 2017-06-28 19:21:26]

Updated, PTAL. Thank you.

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/app_view_browsertest.cc (left):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/app_view_browsertest.cc:18: #include
"content/public/common/content_switches.h"
On 2017/06/12 21:50:29, Ilya Sherman wrote:
> This change seems unrelated.  Could you please split it off to a separate CL?

Removed.

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/extension_view/extension_view_browsertest.cc
(left):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/extension_view/extension_view_browsertest.cc:14:
#include "content/public/common/content_switches.h"
On 2017/06/12 21:50:29, Ilya Sherman wrote:
> This change seems unrelated.  Could you please split it off to a separate CL?

Removed.

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/apps/gu...
File chrome/browser/apps/guest_view/web_view_browsertest.cc (left):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/apps/gu...
chrome/browser/apps/guest_view/web_view_browsertest.cc:3628:
base::test::ScopedFeatureList scoped_feature_list_;
On 2017/06/12 21:50:29, Ilya Sherman wrote:
> This change seems unrelated.  Could you please split it off to a separate CL?

Removed

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/input_method_apitest_chromeos.cc
(right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/input_method_apitest_chromeos.cc:86:
scoped_feature_list_.InitAndEnableFeature(features::kOptInImeMenu);
On 2017/06/12 21:50:29, Ilya Sherman wrote:
> Does this need to be done as part of SetUpCommandLine?  Could it happen during
> SetUp() instead?  SetUpCommandLine() is a much more specialized method, and
I'd
> prefer to minimize the amount of code in it, for clarity.

Move all calls to SetUp

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/media/e...
File chrome/browser/media/encrypted_media_browsertest.cc (right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:296:
media::kExternalClearKeyForTesting);
On 2017/06/12 21:50:29, Ilya Sherman wrote:
> As in the other file, could this be done outside of SetUpCommandLine()?

Done.

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_browsertest.cc:303: #endif  //
BUILDFLAG(ENABLE_PEPPER_CDMS)
On 2017/06/12 21:50:29, Ilya Sherman wrote:
> Does anything go wrong if you omit this #if, and just always create the
feature
> list?

Done.

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/media/e...
File chrome/browser/media/encrypted_media_supported_types_browsertest.cc
(right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_supported_types_browsertest.cc:308:
media::kExternalClearKeyForTesting);
On 2017/06/12 21:50:29, Ilya Sherman wrote:
> Could this be done outside of SetUpCommandLine()?

Done.

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/media/e...
chrome/browser/media/encrypted_media_supported_types_browsertest.cc:351:
media::kExternalClearKeyForTesting);
On 2017/06/12 21:50:29, Ilya Sherman wrote:
> Could this be done outside of SetUpCommandLine()?

Done.

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/net/pre...
File chrome/browser/net/predictor_browsertest.cc (right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:525:
scoped_feature_list_.InitAndEnableFeature(features::kPreconnectMore);
On 2017/06/12 21:50:29, Ilya Sherman wrote:
> Could this be done outside of SetUpCommandLine()?  I'm going to stop calling
> these out, but please fix this for all of the test files in this CL.

Done.

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/net/pre...
File chrome/browser/net/predictor_tab_helper.h (right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/net/pre...
chrome/browser/net/predictor_tab_helper.h:20: }  // namespace features
On 2017/06/12 21:50:29, Ilya Sherman wrote:
> I don't think that it's appropriate for this header to add to the features
> namespace.  Instead, please declare and define this feature in
> //chrome/common/chrome_features.{h,cc}.

Done.

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h
(right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h:22:
}  // namespace features
On 2017/06/12 21:50:29, Ilya Sherman wrote:
> Same, for this and other features that you'd like to have public visibility
> within //chrome.

Done.

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc
(right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc:28:
scoped_feature_list_.InitAndEnableFeature(features::kAdsFeature);
On 2017/06/12 21:50:29, Ilya Sherman wrote:
> Why are you doing this during SetUpCommandLine, rather than -- say -- during
the
> test constructor?

Moved to SetUp.

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/plugins...
File chrome/browser/plugins/flash_permission_browsertest.cc (right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/plugins...
chrome/browser/plugins/flash_permission_browsertest.cc:58: #if
BUILDFLAG(ENABLE_PLUGINS)
On 2017/06/12 21:50:29, Ilya Sherman wrote:
> Why did you add this #if wrapper?

This feature only define when enable plugins.
https://cs.chromium.org/chromium/src/chrome/common/chrome_features.cc?rcl=239...

https://codereview.chromium.org/2910843002/diff/160001/chrome/browser/plugins...
chrome/browser/plugins/flash_permission_browsertest.cc:60: // permission.
On 2017/06/12 21:50:29, Ilya Sherman wrote:
> Why did you drop the setting of the param?

Done.

https://codereview.chromium.org/2910843002/diff/160001/chrome/test/BUILD.gn
File chrome/test/BUILD.gn (right):

https://codereview.chromium.org/2910843002/diff/160001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:1115: "//chrome/browser:browser",
On 2017/06/12 21:50:29, Ilya Sherman wrote:
> Hmm, why did you add this dependency?  It's already included on line 1613.

Done.

https://codereview.chromium.org/2910843002/diff/160001/chrome/test/BUILD.gn#n...
chrome/test/BUILD.gn:1116: "//components/safe_browsing_db:v4_feature_list",
On 2017/06/12 21:50:29, Ilya Sherman wrote:
> Would this be appropriate to move down into the desktop-only section, or is it
> needed for Android as well?

Android does not need this. Moved to desktop-only.

https://codereview.chromium.org/2910843002/diff/160001/components/safe_browsi...
File components/safe_browsing_db/v4_feature_list.h (right):

https://codereview.chromium.org/2910843002/diff/160001/components/safe_browsi...
components/safe_browsing_db/v4_feature_list.h:14: }  // namespace features
On 2017/06/12 21:50:29, Ilya Sherman wrote:
> Please define a separate features.h file for this feature (and a corresponding
> impl file for the definition.)

Done.

[Ilya Sherman, 2017-06-28 19:56:05]

Thanks!

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/input_method_apitest_chromeos.cc
(right):

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/input_method_apitest_chromeos.cc:84: }
nit: Please leave a blank line here.

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/metrics...
File chrome/browser/metrics/ukm_browsertest.cc (right):

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/metrics...
chrome/browser/metrics/ukm_browsertest.cc:61: }
nit: Please leave a blank line here.

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/net/pre...
File chrome/browser/net/predictor_browsertest.cc (right):

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:32: #include
"chrome/browser/net/predictor_tab_helper.h"
nit: Is this #include needed as part of this CL?

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/notific...
File chrome/browser/notifications/notification_interactive_uitest_support.h
(left):

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/notific...
chrome/browser/notifications/notification_interactive_uitest_support.h:33: //
InProcessBrowserTest overrides.
nit: Please preserve this line.

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc
(right):

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc:11:
#include
"chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h"
nit: Is this #include needed as part of this CL?

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/plugins...
File chrome/browser/plugins/flash_permission_browsertest.cc (right):

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/plugins...
chrome/browser/plugins/flash_permission_browsertest.cc:55: std::string());
Hmm, are you sure that this works?  I thought that the test basically runs as
part of SetUpOnMainThread().  Did you mean to override SetUp()?

https://codereview.chromium.org/2910843002/diff/260001/components/safe_browsi...
File components/safe_browsing_db/BUILD.gn (right):

https://codereview.chromium.org/2910843002/diff/260001/components/safe_browsi...
components/safe_browsing_db/BUILD.gn:216: "features.h",
Hmm, did you forget to "git add" these files?  Or am I simply overlooking them?

[chaopeng, 2017-06-30 19:22:43]

Patchset #8 (id:280001) has been deleted

[chaopeng, 2017-06-30 19:32:00]

Patchset #8 (id:300001) has been deleted

[chaopeng, 2017-06-30 19:32:11]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-30 19:32:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chaopeng, 2017-06-30 19:32:37]

Updated. PTAL. Thank you.

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/chromeo...
File chrome/browser/chromeos/extensions/input_method_apitest_chromeos.cc
(right):

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/chromeo...
chrome/browser/chromeos/extensions/input_method_apitest_chromeos.cc:84: }
On 2017/06/28 19:56:04, Ilya Sherman wrote:
> nit: Please leave a blank line here.

Done.

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/metrics...
File chrome/browser/metrics/ukm_browsertest.cc (right):

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/metrics...
chrome/browser/metrics/ukm_browsertest.cc:61: }
On 2017/06/28 19:56:05, Ilya Sherman wrote:
> nit: Please leave a blank line here.

Done.

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/net/pre...
File chrome/browser/net/predictor_browsertest.cc (right):

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/net/pre...
chrome/browser/net/predictor_browsertest.cc:32: #include
"chrome/browser/net/predictor_tab_helper.h"
On 2017/06/28 19:56:05, Ilya Sherman wrote:
> nit: Is this #include needed as part of this CL?

Done.

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/notific...
File chrome/browser/notifications/notification_interactive_uitest_support.h
(left):

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/notific...
chrome/browser/notifications/notification_interactive_uitest_support.h:33: //
InProcessBrowserTest overrides.
On 2017/06/28 19:56:05, Ilya Sherman wrote:
> nit: Please preserve this line.

Done.

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc
(right):

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc:11:
#include
"chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer.h"
On 2017/06/28 19:56:05, Ilya Sherman wrote:
> nit: Is this #include needed as part of this CL?

Done.

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/plugins...
File chrome/browser/plugins/flash_permission_browsertest.cc (right):

https://codereview.chromium.org/2910843002/diff/260001/chrome/browser/plugins...
chrome/browser/plugins/flash_permission_browsertest.cc:55: std::string());
On 2017/06/28 19:56:05, Ilya Sherman wrote:
> Hmm, are you sure that this works?  I thought that the test basically runs as
> part of SetUpOnMainThread().  Did you mean to override SetUp()?

I have not test this one yet. But we can not do this in SetUp with field trials.
Same as https://codereview.chromium.org/2929713002/.

https://codereview.chromium.org/2910843002/diff/260001/components/safe_browsi...
File components/safe_browsing_db/BUILD.gn (right):

https://codereview.chromium.org/2910843002/diff/260001/components/safe_browsi...
components/safe_browsing_db/BUILD.gn:216: "features.h",
On 2017/06/28 19:56:05, Ilya Sherman wrote:
> Hmm, did you forget to "git add" these files?  Or am I simply overlooking
them?

I forgot these, added.

https://codereview.chromium.org/2910843002/diff/320001/chrome/browser/ui/sear...
File chrome/browser/ui/search/local_ntp_browsertest.cc (right):

https://codereview.chromium.org/2910843002/diff/320001/chrome/browser/ui/sear...
chrome/browser/ui/search/local_ntp_browsertest.cc:16: #include
"build/build_config.h"
I got warning point to add this header file.

https://codereview.chromium.org/2910843002/diff/320001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/2910843002/diff/320001/content/test/BUILD.gn#...
content/test/BUILD.gn:353: "//third_party/webrtc/rtc_base:rtc_base_approved",
I got warning point to format this file.

[chaopeng, 2017-06-30 19:32:46]

The CQ bit was unchecked by chaopeng@chromium.org

[Ilya Sherman, 2017-07-03 21:21:59]

Thanks!  Looks like there are quite a few test failures showing up on the bots. 
Please take a look at those.

Also, when possible, please separate out rebases from code changes, to ease code
review.  That is, please upload one patch set that is just a rebase, and a
separate patch set that is just code changes.  It's fine if the rebase is either
the first or the second patch set of the two.  But it's much harder to review
the changes when changes from a rebase are mixed in.  (I do realize that it can
be hard to remember, so occasionally patch sets will slip in that contain both. 
Just do try to keep it in mind :])

https://codereview.chromium.org/2910843002/diff/320001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc
(right):

https://codereview.chromium.org/2910843002/diff/320001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc:24:

nit: Please omit this blank line.

https://codereview.chromium.org/2910843002/diff/320001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc:28:
scoped_feature_list_.InitAndEnableFeature(features::kAdsFeature);
Hmm, if this runs before InProcessBrowserTest::SetUp(), is it important to do it
as part of SetUp(), or could it be part of the constructor?  The constructor is
always preferred when possible, as it's the simplest to reason about.

https://codereview.chromium.org/2910843002/diff/320001/chrome/browser/ui/sear...
File chrome/browser/ui/search/local_ntp_browsertest.cc (right):

https://codereview.chromium.org/2910843002/diff/320001/chrome/browser/ui/sear...
chrome/browser/ui/search/local_ntp_browsertest.cc:16: #include
"build/build_config.h"
On 2017/06/30 19:32:37, chaopeng wrote:
> I got warning point to add this header file.

That makes sense, since you're adding OS_ #if-stmts below.

[chaopeng, 2017-07-08 03:34:30]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-08 03:34:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-08 03:37:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-08 03:37:27]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[chaopeng, 2017-07-09 03:35:47]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-09 03:35:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-09 05:00:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-09 05:00:21]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chaopeng, 2017-07-10 13:51:04]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-10 13:51:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-10 13:54:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-10 13:54:51]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[chaopeng, 2017-07-10 14:04:51]

Patchset #11 (id:380001) has been deleted

[chaopeng, 2017-07-10 14:05:03]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-10 14:05:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-10 14:19:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-10 14:19:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chaopeng, 2017-07-10 19:29:08]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-10 19:29:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-10 20:16:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-10 20:16:48]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chaopeng, 2017-07-10 20:49:13]

Patchset #12 (id:420001) has been deleted

[chaopeng, 2017-07-10 20:49:24]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-10 20:49:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-10 22:01:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-10 22:01:17]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[chaopeng, 2017-07-10 23:05:29]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-10 23:05:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-11 01:13:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-11 01:13:46]

Dry run: This issue passed the CQ dry run.

[chaopeng, 2017-07-11 03:04:17]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-11 03:04:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-11 04:39:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-11 04:39:45]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[chaopeng, 2017-07-14 15:46:05]

Patchset #15 (id:500001) has been deleted

[chaopeng, 2017-07-14 15:46:47]

The CQ bit was checked by chaopeng@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-07-14 15:47:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-14 18:05:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-07-14 18:05:56]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[chaopeng, 2017-07-14 18:43:50]

Updated. Moved all calls to constructor. The failed try bots are from this CL
https://chromium-review.googlesource.com/c/565224 which conflict our proposal.

PTAL. Thank you.

https://codereview.chromium.org/2910843002/diff/320001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc
(right):

https://codereview.chromium.org/2910843002/diff/320001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc:24:

On 2017/07/03 21:21:59, Ilya Sherman wrote:
> nit: Please omit this blank line.

Done.

https://codereview.chromium.org/2910843002/diff/320001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/ads_page_load_metrics_observer_browsertest.cc:28:
scoped_feature_list_.InitAndEnableFeature(features::kAdsFeature);
On 2017/07/03 21:21:59, Ilya Sherman wrote:
> Hmm, if this runs before InProcessBrowserTest::SetUp(), is it important to do
it
> as part of SetUp(), or could it be part of the constructor?  The constructor
is
> always preferred when possible, as it's the simplest to reason about.

Move all calls to constructor.

[Ilya Sherman, 2017-07-14 23:46:59]

LGTM, thanks!