RuntimeEnabledFeatures status

[Jochen Eisinger]

Hey,

I'd like to start a discussion about how we use the different status flags. My concern is that we add features with status=experimental even though they're not going to be shipped in the near future.

By doing this, we end up testing a different code path that what we actually ship which is bad.

I'd therefore propose that we add new features without a status by default, i.e., a layout test needs to enable the feature with window.internal.settings or similar.

If the feature is more or less done and we want to prepare for the intent to ship, it's probably the right point in time to upgrade to status=test which enables the feature for layout tests by default.

When we then want to collect data from our bleeding edge users, we can upgrade the feature to status=experimental which basically means, it's stable enough to expose users to it, and we will soon launch it, so we want to give early adopters a chance to play with it.

best

-jochen

[PhistucK]

Can you add a flag that enables status=test features as well?

☆PhistucK

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Jochen Eisinger]

Sorry if I was not clear.

status flags here means the entries in this file: https://code.google.com/p/chromium/codesearch#search/&sq=package:chromium&q=RuntimeEnabledFeatures.in

[PhistucK]

You were clear (for me, anyway), but my question still stands.

☆PhistucK

[Jochen Eisinger]

You mean like a command line flag? That would be unrelated to my discussion, and I'd ask you file a feature request for that on crbug.com.

[Philip Jägenstedt]

This makes a lot of sense to me, Jochen.

I've started work on a CL to drop status=experimental from the
RuntimeEnabledFeatures that I have some connection to:
https://codereview.chromium.org/900863003

Philip

[PhistucK]

It is related, because many features that are currently status=experimental will turn into status=test and will no longer be accessible using the experimental web platform features flag.

So if you are going through with this decision, it makes sense to add a new flag in order to test the interactions of all of the status=test (and status=experimental) features when all of the are enabled together.

If you are not going through with this decision, this new flag is unnecessary (as far as I see).

☆PhistucK

[Peter Beverloo]

In the following CL, Jeremy is introducing two new command line flags for enabling or disabling Blink runtime features:

    https://codereview.chromium.org/898483004/

You will be able to use them like: --enable-blink-features="AudioVideoTracks,MediaController"

Thanks,

Peter

[PhistucK]

And if I do not specify a feature, will it enable all of them (pretty please? :))?

☆PhistucK

[Peter Beverloo]

Nope. Again, please file a feature request for such additions :-).

Thanks,

Peter

[PhistucK]

Of course, I was waiting for your answer first.

crbug.com/455270

☆PhistucK

[Douglas Stockwell]

There were some objections to exposing such flags (--enable-blink-features) in the past. I had a similar change in 2013: https://codereview.chromium.org/18578007/

The main issue was the explosion of configuration spaces. In the past I believe we desired to run layout tests against both stable and test/experimental configuration modes. Given that it hasn't happened (beyond the small virtual/stable suite), presumably due to the feasibility of running every layout test several times, I'm supportive of this change and preferring the creation of virtual suites over premature use of status=test.

[Philip Jägenstedt]

On Wed, Feb 4, 2015 at 7:57 PM, 'Jochen Eisinger' via blink-dev
<blin...@chromium.org> wrote:
> I'd therefore propose that we add new features without a status by default,
> i.e., a layout test needs to enable the feature with
> window.internal.settings or similar.

So after getting a list of the failing tests I realized a hadn't
thought of step 2. Do you think we should have a virtual test suite
for every feature? Since it's runtime-enabled you can't enable it
inside the test itself, right?

Philip

[Yoav Weiss]

As far as I understand it, you can modify the state of runtime-enabled features from InternalSettings (Looks like that's what 

InternalSettings::setScrollTopLeftInteropEnabled is doing for example).

The problem here is that not all features start running after the page's JS has ran.

I now need to test and add runtime-enabled flags to Link header based dns-prefetch. I don't think I can do that if by default the test would be off unless the test's JS turned it on. Can I rely on InternalSettings/RuntimeEnabled being preserved between page reloads?

[Yoav Weiss]

On Thu, Feb 5, 2015 at 9:59 AM, Yoav Weiss <yo...@yoav.ws> wrote:

The problem here is that not all features start running after the page's JS has ran.

I now need to test and add runtime-enabled flags to Link header based dns-prefetch. I don't think I can do that if by default the test would be off unless the test's JS turned it on. Can I rely on InternalSettings/RuntimeEnabled being preserved between page reloads?

For my case, I can probably resolve it by using iframes (set the JS at the main document, then load the iframe afterwards and run the test inside it).

It certainly complicates testing, but it's doable. 

[Mike West]

Perhaps we could add some logic to the test runner that associates flags with directory names? That is, if running a test in `/http/tests/flagged/enableDnsPrefetch`, we'd set the corresponding runtime flag?

-mike

--
Mike West <mk...@google.com>, @mikewest

Google Germany GmbH, Dienerstrasse 12, 80331 München, Germany, Registergericht und -nummer: Hamburg, HRB 86891, Sitz der Gesellschaft: Hamburg, Geschäftsführer: Graham Law, Christine Elizabeth Flores

(Sorry; I'm legally required to add this exciting detail to emails. Bleh.)

[Julien Chaffraix]

On Wed, Feb 4, 2015 at 11:57 PM, 'Jochen Eisinger' via blink-dev
<blin...@chromium.org> wrote:
> Hey,
>
> I'd like to start a discussion about how we use the different status flags.
> My concern is that we add features with status=experimental even though
> they're not going to be shipped in the near future.
>
> By doing this, we end up testing a different code path that what we actually
> ship which is bad.
>
> I'd therefore propose that we add new features without a status by default,
> i.e., a layout test needs to enable the feature with
> window.internal.settings or similar.

I am extremely doubtful about using internal.settings more for several
reasons. First this is not a code path we ship so we're back on the
topic of testing code we don't ship (we restart the browser if you
change feature flags, not for settings).

More fundamentally, we shouldn't support 2 ways of enabling a feature.
A lot of the internal.settings stick to tests past the point when the
feature flag is enabled. Not to mention the churn generated by the
removal of these after switching the flag, which is just grudge work
without a good justification.

> If the feature is more or less done and we want to prepare for the intent to
> ship, it's probably the right point in time to upgrade to status=test which
> enables the feature for layout tests by default.
>
> When we then want to collect data from our bleeding edge users, we can
> upgrade the feature to status=experimental which basically means, it's
> stable enough to expose users to it, and we will soon launch it, so we want
> to give early adopters a chance to play with it.

I don't think I understand what class of bugs you're trying to
prevent. Giving some good examples about what you're thinking about
would help get the context.

We usually catch *more* bugs by enabling a feature in our testing
harness than not having it on. Also we can always improve our stable
coverage through the virtual stable test suite if we have some class
of bugs we're concerned about.

Thanks,
Julien

[Yoav Weiss]

Just added such a feature flag along with a test to https://codereview.chromium.org/788923003/

I agree with Julien that having to turn on the flag as part of the test would require test code removal once the feature goes experimental, which adds a maintenance burden.

Also, in InternalSettings.h I found the following:

    // FIXME: The following are RuntimeEnabledFeatures and likely cannot be changed after process start. These setters should be removed or moved onto internals.runtimeFlags

For now these setters are working (at least after a page reload), but are they guaranteed to work?

[Jochen Eisinger]

The initial reason I wrote this mail was because ScrollLeftTopInterop made e.g. inbox.google.com almost impossible to use. When I then tried to disable the feature, a bunch of scroll-behavior tests started to crash :-/

[Yoav Weiss]

On Fri, Feb 6, 2015 at 10:58 AM, 'Jochen Eisinger' via blink-dev <blin...@chromium.org> wrote:

The initial reason I wrote this mail was because ScrollLeftTopInterop made e.g. inbox.google.com almost impossible to use. When I then tried to disable the feature, a bunch of scroll-behavior tests started to crash :-/

 

That's a good reason.

Maybe, like what Mike suggested, we can add a mechanism that turns on feature flags for entire test directories?

I don't know if having the flag as part of the path is the best way to go, but maybe a config file? 

That would solve the InternalSettings worries as well as the maintenance burden of writing complex tests and then rewriting them once the feature made it to experimental.

[PhistucK]

Completely off topic - but have you filed a bug with the Inbox team regarding this? This means they are not being very friendly to other browsers, they are not adhering to the standards, or they use browser sniffing. :(

☆PhistucK

[Antonio Gomes]

@jochen: I support your patch, given that there seem to be no movement towards toggling scropLeftTopInterop on for stable channels at the moment, and it was deviating common scroll code from the path blink ships.

On Fri, Feb 6, 2015 at 12:00 PM, PhistucK <phis...@gmail.com> wrote:

Completely off topic - but have you filed a bug with the Inbox team regarding this? This means they are not being very friendly to other browsers, they are not adhering to the standards, or they use browser sniffing. :(

+1, please follow up with google partners to get this fixed on their end too. Their site is clearing doing browser detection instead of feature detection.

Cheers,

[Rick Byers]

On Sat, Feb 7, 2015 at 9:07 AM, Antonio Gomes <toni...@gmail.com> wrote:

@jochen: I support your patch, given that there seem to be no movement towards toggling scropLeftTopInterop on for stable channels at the moment, and it was deviating common scroll code from the path blink ships.

I think this is the key issue here.  scrollLeftTopInterop is different from most other RuntimeEnabledFeatures in that it toggles the behavior of major APIs in a way that's (I believe) non-trivial to feature detect for.  Supporting both modes is a burden on the engine and we really shouldn't leave the engine in this state for long.

That said, it would be shame to just give up on this issue.  Perhaps we should have a separate thread about how best to move it along.

[Morten Stenshorne]

Yoav Weiss <yo...@yoav.ws> writes:

> On Fri, Feb 6, 2015 at 10:58 AM, 'Jochen Eisinger' via blink-dev <blin...@chromium.org> wrote:
>
> The initial reason I wrote this mail was because ScrollLeftTopInterop made e.g. inbox.google.com almost impossible
> to use. When I then tried to disable the feature, a bunch of scroll-behavior tests started to crash :-/
>
>  
> That's a good reason.
>
> Maybe, like what Mike suggested, we can add a mechanism that turns on feature flags for entire test directories?
> I don't know if having the flag as part of the path is the best way to go, but maybe a config file? 

Seen PhysicalTestSuite in Tools/Scripts/webkitpy/layout_tests/port/base.py ?

--
---- Morten Stenshorne, developer, Opera Software ASA ----
------------------ http://www.opera.com/ -----------------

[Yoav Weiss]

On Sun, Feb 8, 2015 at 9:56 AM, Morten Stenshorne <mste...@opera.com> wrote:

Yoav Weiss <yo...@yoav.ws> writes:

> On Fri, Feb 6, 2015 at 10:58 AM, 'Jochen Eisinger' via blink-dev <blin...@chromium.org> wrote:
>
>     The initial reason I wrote this mail was because ScrollLeftTopInterop made e.g. inbox.google.com almost impossible
>     to use. When I then tried to disable the feature, a bunch of scroll-behavior tests started to crash :-/
>
>  
> That's a good reason.
>
> Maybe, like what Mike suggested, we can add a mechanism that turns on feature flags for entire test directories?
> I don't know if having the flag as part of the path is the best way to go, but maybe a config file? 

Seen PhysicalTestSuite in Tools/Scripts/webkitpy/layout_tests/port/base.py ?

Now I have :)

If I understand it correctly, it can be used to turn on a command line flag on a certain directory. Do we have a way to control specific feature flags from the command line?

Also, how would one use that? Override that function in the python code? Maybe we can hook it up with a config file.

[Yoav Weiss]

Also - would this mean that we'd need to add some sort of "intent to experiment" to our process?

[Dirk Pranke]

On Sun, Feb 8, 2015 at 2:37 AM, Yoav Weiss <yo...@yoav.ws> wrote:

On Sun, Feb 8, 2015 at 9:56 AM, Morten Stenshorne <mste...@opera.com> wrote:

Yoav Weiss <yo...@yoav.ws> writes:

> On Fri, Feb 6, 2015 at 10:58 AM, 'Jochen Eisinger' via blink-dev <blin...@chromium.org> wrote:
>
>     The initial reason I wrote this mail was because ScrollLeftTopInterop made e.g. inbox.google.com almost impossible
>     to use. When I then tried to disable the feature, a bunch of scroll-behavior tests started to crash :-/
>
>  
> That's a good reason.
>
> Maybe, like what Mike suggested, we can add a mechanism that turns on feature flags for entire test directories?
> I don't know if having the flag as part of the path is the best way to go, but maybe a config file? 

Seen PhysicalTestSuite in Tools/Scripts/webkitpy/layout_tests/port/base.py ?

Now I have :)

If I understand it correctly, it can be used to turn on a command line flag on a certain directory.

Yup, that's the entire point of that feature.

 

Do we have a way to control specific feature flags from the command line?

People usually create command line flags for each feature; I don't think we have a generic mechanism.

 

Also, how would one use that? Override that function in the python code? Maybe we can hook it up with a config file.

As the code stands today, you'd modify that function. It would be trivial to reuse the code for virtual_test_suites() in that file, though, and create a LayoutTests/PhysicalTestSuites file.

-- Dirk

[Morten Stenshorne]

Dirk Pranke <dpr...@chromium.org> writes:

> On Sun, Feb 8, 2015 at 2:37 AM, Yoav Weiss <yo...@yoav.ws> wrote:
>
> On Sun, Feb 8, 2015 at 9:56 AM, Morten Stenshorne <mste...@opera.com> wrote:
>
> Yoav Weiss <yo...@yoav.ws> writes:
>
> > On Fri, Feb 6, 2015 at 10:58 AM, 'Jochen Eisinger' via blink-dev <blin...@chromium.org> wrote:
> >
> >     The initial reason I wrote this mail was because ScrollLeftTopInterop made e.g. inbox.google.com almost impossible
> >     to use. When I then tried to disable the feature, a bunch of scroll-behavior tests started to crash :-/
> >
> >  
> > That's a good reason.
> >
> > Maybe, like what Mike suggested, we can add a mechanism that turns on feature flags for entire test directories?
> > I don't know if having the flag as part of the path is the best way to go, but maybe a config file? 
>
> Seen PhysicalTestSuite in Tools/Scripts/webkitpy/layout_tests/port/base.py ?
>
> Now I have :)
> If I understand it correctly, it can be used to turn on a command line flag on a certain directory.
>
> Yup, that's the entire point of that feature.
>  
>
> Do we have a way to control specific feature flags from the command line?
>
> People usually create command line flags for each feature; I don't
> think we have a generic mechanism.

Yes, we do (?). Although I haven't looked at it myself, in this very

thread, Peter Beverloo <pe...@chromium.org> wrote:

> In the following CL, Jeremy is introducing two new command line flags
> for enabling or disabling Blink runtime features:  
> https://codereview.chromium.org/898483004/

> You will be able to use them like: --enable-blink-features="AudioVideoTracks,MediaController"

[Dirk Pranke]

Heh. Right you are :).

-- Dirk 

[Yoav Weiss]

Oh, that's awesome!!

[Julien Chaffraix]

On Sat, Feb 7, 2015 at 9:56 PM, Rick Byers <rby...@chromium.org> wrote:
> On Sat, Feb 7, 2015 at 9:07 AM, Antonio Gomes <toni...@gmail.com> wrote:
>>
>>
>> @jochen: I support your patch, given that there seem to be no movement
>> towards toggling scropLeftTopInterop on for stable channels at the moment,
>> and it was deviating common scroll code from the path blink ships.
>
>
> I think this is the key issue here. scrollLeftTopInterop is different from
> most other RuntimeEnabledFeatures in that it toggles the behavior of major
> APIs in a way that's (I believe) non-trivial to feature detect for.
> Supporting both modes is a burden on the engine and we really shouldn't
> leave the engine in this state for long.
>
> That said, it would be shame to just give up on this issue. Perhaps we
> should have a separate thread about how best to move it along.

Rick is spot on. This flag was added because it changes some core API,
which is not usual for runtime features.

I was the one asking for this flag as we needed a way to toggle the
behavior if it broke something so I bear responsibility in this issue.
The whole idea failed because the test coverage was bad and having
some recommendation / guideline for testing would be useful.

The proposed runtime flag policy may have caught the issue but it's
unclear if it would have. Thus it seems better IMHO that we
concentrate on how to make sure flags for breaking change are
correctly tested.

Julien

[Rick Byers]

In case you missed the other thread, I'm willing to take point on driving the issue to enable us to remove this particular flag  (one way or another) over the next couple milestones.  Sorry I didn't do this sooner.

Julien

[Jochen Eisinger]

This was just one example - on the bug I filed, I demo'd the issue on the closure demo page. Virtually any site that queries the scroll position and works with Chrome relies on the non-standard behavior :-/

 

Cheers,

[Jochen Eisinger]

On Sat Feb 07 2015 at 11:56:43 AM Rick Byers <rby...@chromium.org> wrote:

On Sat, Feb 7, 2015 at 9:07 AM, Antonio Gomes <toni...@gmail.com> wrote:

@jochen: I support your patch, given that there seem to be no movement towards toggling scropLeftTopInterop on for stable channels at the moment, and it was deviating common scroll code from the path blink ships.

I think this is the key issue here.  scrollLeftTopInterop is different from most other RuntimeEnabledFeatures in that it toggles the behavior of major APIs in a way that's (I believe) non-trivial to feature detect for.  Supporting both modes is a burden on the engine and we really shouldn't leave the engine in this state for long.

Yeah, I guess that's what I'm saying. This, and that marking something as experimental should mean you will ship it soonish.

 

That said, it would be shame to just give up on this issue.  Perhaps we should have a separate thread about how best to move it along.

sg