Revert "Reland of Default enable main frame before activation and remove finch experiment. (patchse…

Revert "Reland of Default enable main frame before activation and remove finch experiment. (patchset #1 id:1 of https://codereview.chromium.org/2170233002/ )"

This reverts commit 8d9a5f91b19c30f88e0a9812abf2e936a38dbd0b.
Enabling main-frame-before-activation causes flickering on ChromeOS when
browsing to: https://ride.uber.com/en_US/

Note, the testing/variations/fieldtrial_testing_config_* have now all been
consolidated into a single testing/variations/fieldtrial_testing_config.

BUG=chromium:664007
TEST=https://ride.uber.com/en_US/ background video does not flicker.

R=piman,marcheu,sunnyps,rkaplow,holte

[sunnyps, 2016-11-10 18:58:41]

not lgtm

This optimization was landed a few months ago in M54. I don't know if it's safe
to revert at this point.

[Daniel Kurtz, 2016-11-13 01:36:36]

On 2016/11/10 18:58:41, sunnyps wrote:
> not lgtm
> 
> This optimization was landed a few months ago in M54. I don't know if it's
safe
> to revert at this point.

What do you mean by "safe"?

[piman, 2016-11-14 18:39:13]

On Sat, Nov 12, 2016 at 5:36 PM, <djkurtz@chromium.org> wrote:

> Reviewers: holte, marcheu, piman, rkaplow, sunnyps
> CL: https://codereview.chromium.org/2488193002/
>
> Message:
> On 2016/11/10 18:58:41, sunnyps wrote:
> > not lgtm
> >
> > This optimization was landed a few months ago in M54. I don't know if
> it's
> safe
> > to revert at this point.
>
> What do you mean by "safe"?
>

This change landed >3 months ago. A lot of code was written since then that
assumes this change. It's not ok to revert on trunk.


>
> Description:
> Revert "Reland of Default enable main frame before activation and remove
> finch
> experiment. (patchset #1 id:1 of https://codereview.chromium.
> org/2170233002/ )"
>
> This reverts commit 8d9a5f91b19c30f88e0a9812abf2e936a38dbd0b.
> Enabling main-frame-before-activation causes flickering on ChromeOS when
> browsing to: https://ride.uber.com/en_US/
>
> Note, the testing/variations/fieldtrial_testing_config_* have now all been
> consolidated into a single testing/variations/fieldtrial_testing_config.
>
> BUG=chromium:664007
> TEST=https://ride.uber.com/en_US/ background video does not flicker.
>
> R=piman,marcheu,sunnyps,rkaplow,holte
>
> Affected files (+26, -1 lines):
> M content/browser/gpu/compositor_util.cc
> M content/public/common/content_features.h
> M content/public/common/content_features.cc
> M testing/variations/fieldtrial_testing_config.json
>
>
> Index: content/browser/gpu/compositor_util.cc
> diff --git a/content/browser/gpu/compositor_util.cc b/content/browser/gpu/
> compositor_util.cc
> index f366994489d1a511851bbae343fc3d993370c639..
> 552d2bd874b784f678e5a7477d6d343e5eb59a40 100644
> --- a/content/browser/gpu/compositor_util.cc
> +++ b/content/browser/gpu/compositor_util.cc
> @@ -315,7 +315,7 @@ bool IsMainFrameBeforeActivationEnabled() {
> if (command_line.HasSwitch(cc::switches::kEnableMainFrameBeforeActivati
> on))
> return true;
>
> - return true;
> + return base::FeatureList::IsEnabled(features::
> kMainFrameBeforeActivation);
> }
>
> base::DictionaryValue* GetFeatureStatus() {
> Index: content/public/common/content_features.cc
> diff --git a/content/public/common/content_features.cc
> b/content/public/common/content_features.cc
> index 656b8fb78f25e535e8a6834d33a8675d84a6471b..
> 9eaf1f2afcdfeab172b9bbcec80785c02eb73a97 100644
> --- a/content/public/common/content_features.cc
> +++ b/content/public/common/content_features.cc
> @@ -95,6 +95,11 @@ const base::Feature kGuestViewCrossProcessFrames{
> // Enables lazily parsing css properties for performance.
> const base::Feature kLazyParseCSS{"LazyParseCSS",
> base::FEATURE_DISABLED_BY_DEFAULT};
> +// Can main thread be pipelined with activation. Always disabled for
> devices
> +// with fewer than 4 cores irrespective of this flag. Can also be
> overridden by
> +// --enable(disable)-main-frame-before-activation command line flag.
> +const base::Feature kMainFrameBeforeActivation{
> + "MainFrameBeforeActivation", base::FEATURE_DISABLED_BY_DEFAULT};
>
> // FeatureList definition for trials to enable the download button on
> // MediaDocument.
> Index: content/public/common/content_features.h
> diff --git a/content/public/common/content_features.h
> b/content/public/common/content_features.h
> index d7a233764e9bdf0bea9d65cb3f6def70b66eabf6..
> 3f76955bed57d7302d8696bee118ef65c6ed75d5 100644
> --- a/content/public/common/content_features.h
> +++ b/content/public/common/content_features.h
> @@ -33,6 +33,7 @@ CONTENT_EXPORT extern const base::Feature
> kGamepadExtensions;
> CONTENT_EXPORT extern const base::Feature kGenericSensor;
> CONTENT_EXPORT extern const base::Feature kGuestViewCrossProcessFrames;
> CONTENT_EXPORT extern const base::Feature kLazyParseCSS;
> +CONTENT_EXPORT extern const base::Feature kMainFrameBeforeActivation;
> CONTENT_EXPORT extern const base::Feature kMediaDocumentDownloadButton;
> CONTENT_EXPORT extern const base::Feature kMemoryCoordinator;
> CONTENT_EXPORT extern const base::Feature kNonValidatingReloadOnNormalRe
> load;
> Index: testing/variations/fieldtrial_testing_config.json
> diff --git a/testing/variations/fieldtrial_testing_config.json
> b/testing/variations/fieldtrial_testing_config.json
> index 31ee25560b6880e242c0fe0f0e4e9e26d06b5a29..
> fa22c694112a62f7b7d63401b7fc882aee70761a 100644
> --- a/testing/variations/fieldtrial_testing_config.json
> +++ b/testing/variations/fieldtrial_testing_config.json
> @@ -865,6 +865,25 @@
> ]
> }
> ],
> + "MainFrameBeforeActivation": [
> + {
> + "platforms": [
> + "android",
> + "chromeos",
> + "linux",
> + "mac",
> + "win"
> + ],
> + "experiments": [
> + {
> + "name": "Enabled",
> + "enable_features": [
> + "MainFrameBeforeActivation"
> + ]
> + }
> + ]
> + }
> + ],
> "MaterialDesignDownloads": [
> {
> "platforms": [
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[marcheu, 2016-11-14 19:28:21]

On 2016/11/14 18:39:13, piman wrote:
> On Sat, Nov 12, 2016 at 5:36 PM, <mailto:djkurtz@chromium.org> wrote:
> 
> > Reviewers: holte, marcheu, piman, rkaplow, sunnyps
> > CL: https://codereview.chromium.org/2488193002/
> >
> > Message:
> > On 2016/11/10 18:58:41, sunnyps wrote:
> > > not lgtm
> > >
> > > This optimization was landed a few months ago in M54. I don't know if
> > it's
> > safe
> > > to revert at this point.
> >
> > What do you mean by "safe"?
> >
> 
> This change landed >3 months ago. A lot of code was written since then that
> assumes this change. It's not ok to revert on trunk.

When there is no other fix, I think it's very fair to revert changes that cause
regressions. But maybe there is a fix I'm not aware of yet.


> 
> 
> >
> > Description:
> > Revert "Reland of Default enable main frame before activation and remove
> > finch
> > experiment. (patchset #1 id:1 of https://codereview.chromium.
> > org/2170233002/ )"
> >
> > This reverts commit 8d9a5f91b19c30f88e0a9812abf2e936a38dbd0b.
> > Enabling main-frame-before-activation causes flickering on ChromeOS when
> > browsing to: https://ride.uber.com/en_US/
> >
> > Note, the testing/variations/fieldtrial_testing_config_* have now all been
> > consolidated into a single testing/variations/fieldtrial_testing_config.
> >
> > BUG=chromium:664007
> > TEST=https://ride.uber.com/en_US/ background video does not flicker.
> >
> > R=piman,marcheu,sunnyps,rkaplow,holte
> >
> > Affected files (+26, -1 lines):
> > M content/browser/gpu/compositor_util.cc
> > M content/public/common/content_features.h
> > M content/public/common/content_features.cc
> > M testing/variations/fieldtrial_testing_config.json
> >
> >
> > Index: content/browser/gpu/compositor_util.cc
> > diff --git a/content/browser/gpu/compositor_util.cc b/content/browser/gpu/
> > compositor_util.cc
> > index f366994489d1a511851bbae343fc3d993370c639..
> > 552d2bd874b784f678e5a7477d6d343e5eb59a40 100644
> > --- a/content/browser/gpu/compositor_util.cc
> > +++ b/content/browser/gpu/compositor_util.cc
> > @@ -315,7 +315,7 @@ bool IsMainFrameBeforeActivationEnabled() {
> > if (command_line.HasSwitch(cc::switches::kEnableMainFrameBeforeActivati
> > on))
> > return true;
> >
> > - return true;
> > + return base::FeatureList::IsEnabled(features::
> > kMainFrameBeforeActivation);
> > }
> >
> > base::DictionaryValue* GetFeatureStatus() {
> > Index: content/public/common/content_features.cc
> > diff --git a/content/public/common/content_features.cc
> > b/content/public/common/content_features.cc
> > index 656b8fb78f25e535e8a6834d33a8675d84a6471b..
> > 9eaf1f2afcdfeab172b9bbcec80785c02eb73a97 100644
> > --- a/content/public/common/content_features.cc
> > +++ b/content/public/common/content_features.cc
> > @@ -95,6 +95,11 @@ const base::Feature kGuestViewCrossProcessFrames{
> > // Enables lazily parsing css properties for performance.
> > const base::Feature kLazyParseCSS{"LazyParseCSS",
> > base::FEATURE_DISABLED_BY_DEFAULT};
> > +// Can main thread be pipelined with activation. Always disabled for
> > devices
> > +// with fewer than 4 cores irrespective of this flag. Can also be
> > overridden by
> > +// --enable(disable)-main-frame-before-activation command line flag.
> > +const base::Feature kMainFrameBeforeActivation{
> > + "MainFrameBeforeActivation", base::FEATURE_DISABLED_BY_DEFAULT};
> >
> > // FeatureList definition for trials to enable the download button on
> > // MediaDocument.
> > Index: content/public/common/content_features.h
> > diff --git a/content/public/common/content_features.h
> > b/content/public/common/content_features.h
> > index d7a233764e9bdf0bea9d65cb3f6def70b66eabf6..
> > 3f76955bed57d7302d8696bee118ef65c6ed75d5 100644
> > --- a/content/public/common/content_features.h
> > +++ b/content/public/common/content_features.h
> > @@ -33,6 +33,7 @@ CONTENT_EXPORT extern const base::Feature
> > kGamepadExtensions;
> > CONTENT_EXPORT extern const base::Feature kGenericSensor;
> > CONTENT_EXPORT extern const base::Feature kGuestViewCrossProcessFrames;
> > CONTENT_EXPORT extern const base::Feature kLazyParseCSS;
> > +CONTENT_EXPORT extern const base::Feature kMainFrameBeforeActivation;
> > CONTENT_EXPORT extern const base::Feature kMediaDocumentDownloadButton;
> > CONTENT_EXPORT extern const base::Feature kMemoryCoordinator;
> > CONTENT_EXPORT extern const base::Feature kNonValidatingReloadOnNormalRe
> > load;
> > Index: testing/variations/fieldtrial_testing_config.json
> > diff --git a/testing/variations/fieldtrial_testing_config.json
> > b/testing/variations/fieldtrial_testing_config.json
> > index 31ee25560b6880e242c0fe0f0e4e9e26d06b5a29..
> > fa22c694112a62f7b7d63401b7fc882aee70761a 100644
> > --- a/testing/variations/fieldtrial_testing_config.json
> > +++ b/testing/variations/fieldtrial_testing_config.json
> > @@ -865,6 +865,25 @@
> > ]
> > }
> > ],
> > + "MainFrameBeforeActivation": [
> > + {
> > + "platforms": [
> > + "android",
> > + "chromeos",
> > + "linux",
> > + "mac",
> > + "win"
> > + ],
> > + "experiments": [
> > + {
> > + "name": "Enabled",
> > + "enable_features": [
> > + "MainFrameBeforeActivation"
> > + ]
> > + }
> > + ]
> > + }
> > + ],
> > "MaterialDesignDownloads": [
> > {
> > "platforms": [
> >
> >
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[Daniel Kurtz, 2016-11-30 08:31:15]

This has been finally by: https://codereview.chromium.org/2529533002

[Daniel Kurtz, 2016-11-30 08:31:26]

Description was changed from

==========
Revert "Reland of Default enable main frame before activation and remove finch
experiment. (patchset #1 id:1 of https://codereview.chromium.org/2170233002/ )"

This reverts commit 8d9a5f91b19c30f88e0a9812abf2e936a38dbd0b.
Enabling main-frame-before-activation causes flickering on ChromeOS when
browsing to: https://ride.uber.com/en_US/

Note, the testing/variations/fieldtrial_testing_config_* have now all been
consolidated into a single testing/variations/fieldtrial_testing_config.

BUG=chromium:664007
TEST=https://ride.uber.com/en_US/ background video does not flicker.

R=piman,marcheu,sunnyps,rkaplow,holte
==========

to

==========
Revert "Reland of Default enable main frame before activation and remove finch
experiment. (patchset #1 id:1 of https://codereview.chromium.org/2170233002/ )"

This reverts commit 8d9a5f91b19c30f88e0a9812abf2e936a38dbd0b.
Enabling main-frame-before-activation causes flickering on ChromeOS when
browsing to: https://ride.uber.com/en_US/

Note, the testing/variations/fieldtrial_testing_config_* have now all been
consolidated into a single testing/variations/fieldtrial_testing_config.

BUG=chromium:664007
TEST=https://ride.uber.com/en_US/ background video does not flicker.

R=piman,marcheu,sunnyps,rkaplow,holte
==========