origin_trial_os and tests (and, it working in general)

[Matt Giuca]

Hi,

I am setting up the InstalledApp feature as an origin trial like this in RuntimeEnabledFeatures.json5:

    {

      name: "InstalledApp",

      origin_trial_feature_name: "InstalledApp",

      // This makes our tests break on non-Android. DO NOT SUBMIT.

      origin_trial_os: ["android"],

      status: "experimental",

    },

Having a few troubles with this on non-Android (Linux):

1. In production, it seems to completely ignore origin_trial_os. Using the test trial token, I can still use the origin trial feature on Linux if I have a valid token. This works on both the static <meta> tag in the HTML file, as well as inserting a <meta> tag dynamically.
2. In the layout test, it ignores the origin_trial_os but only when inserted dynamically. Running the test on Linux, the trial feature isn't available in the static <meta> tag test, but it does succeed when inserted dynamically. That's confusing.

What's really confusing is that Web Share (the only other origin trial currently using origin_trial_os, also my feature) seems to be properly restricted to Android in production. (It does not currently have layout tests for origin trial.) Any explanation why Web Share is being restricted correctly while InstalledApp is ignoring origin_trial_os?

Lastly, *how* can I do layout tests correctly when using origin_trial_os? Is there a way to write the layout test so that it expects a feature on Android but not on other platforms?

(Since my feature works properly on non-Android by simply returning an empty list, I think I might just go ahead and not use origin_trial_os, essentially rolling it out on all platforms but with an empty implementation on non-Android platforms, which is how it will be eventually anyway.)

[Matt Giuca]

The CL to enable the origin trial: https://codereview.chromium.org/2785913003/

In the end, I removed the origin_trial_os = android (I think this is best anyway). But if you want to try out what happens with that flag turned on, see that CL.

[Jason Chase]

I agree it's probably best to avoid using origin_trial_os, at least for now.

I actually didn't know that origin_trial_os was added until recently. Looking at the CL that added it, I don't think anyone on the OT team weighed in on the implementation. I think that's sub-optimal, so I updated our watch lists to try to prevent that in the future. There's a few moving parts to enabling and exposing trial features, and unfortunately the code isn't all together, which I think contributed to some gaps in the overall implementation for origin_trial_os.

More comments inline.

Thanks,

Jason

On Fri, Mar 31, 2017 at 1:34 AM, Matt Giuca <mgi...@chromium.org> wrote:

The CL to enable the origin trial: https://codereview.chromium.org/2785913003/

In the end, I removed the origin_trial_os = android (I think this is best anyway). But if you want to try out what happens with that flag turned on, see that CL.

On Fri, 31 Mar 2017 at 15:47 Matt Giuca <mgi...@chromium.org> wrote:

Hi,

I am setting up the InstalledApp feature as an origin trial like this in RuntimeEnabledFeatures.json5:

    {

      name: "InstalledApp",

      origin_trial_feature_name: "InstalledApp",

      // This makes our tests break on non-Android. DO NOT SUBMIT.

      origin_trial_os: ["android"],

      status: "experimental",

    },

Having a few troubles with this on non-Android (Linux):

1. In production, it seems to completely ignore origin_trial_os. Using the test trial token, I can still use the origin trial feature on Linux if I have a valid token. This works on both the static <meta> tag in the HTML file, as well as inserting a <meta> tag dynamically.
2. In the layout test, it ignores the origin_trial_os but only when inserted dynamically. Running the test on Linux, the trial feature isn't available in the static <meta> tag test, but it does succeed when inserted dynamically. That's confusing.

Looking across all the code, I think this is due to the fact OS-filtering logic is only in the generated helper functions to check for enabled trials (in OriginTrials::*). The concept of OS-filtering isn't considered in the core origin trials code (i.e. OriginTrialContext.cpp).  Thus, if there is code that doesn't use the generated helper functions, like the bindings logic (in ConditionalFeatures*), the OS-filtering is applied inconsistently or not at all.

I've logged crbug.com/707232 to fix the implementation of origin_trial_os.

 

What's really confusing is that Web Share (the only other origin trial currently using origin_trial_os, also my feature) seems to be properly restricted to Android in production. (It does not currently have layout tests for origin trial.) Any explanation why Web Share is being restricted correctly while InstalledApp is ignoring origin_trial_os?

From code inspection, I'm not sure why you'd see different behaviour for WebShare vs InstalledApp. Could you give us instructions for how you're testing WebShare?

 

Lastly, *how* can I do layout tests correctly when using origin_trial_os? Is there a way to write the layout test so that it expects a feature on Android but not on other platforms?

I had started trying to write layout tests for Web Share (see https://codereview.chromium.org/2714733002/). I didn't get through testing all the scenarios, so I haven't tried to land them. As you pointed out, I didn't find a way to detect the os/platform in the layout test. As well, I'm hoping we'll need fewer such layout tests in the future, once Ian finishes changes to automatically generate the code to expose interfaces to JavaScript.

 

(Since my feature works properly on non-Android by simply returning an empty list, I think I might just go ahead and not use origin_trial_os, essentially rolling it out on all platforms but with an empty implementation on non-Android platforms, which is how it will be eventually anyway.)

--
You received this message because you are subscribed to the Google Groups "experimentation-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to experimentation-dev+unsub...@chromium.org.
To post to this group, send email to experimentation-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/experimentation-dev/CAHqYdcbOXscN5FgO_Pvuch%2BNUu-jFZh5%3Dt5evKFAAphuncvRfw%40mail.gmail.com.

[Matt Giuca]

On Sat, 1 Apr 2017 at 01:02 Jason Chase <cha...@chromium.org> wrote:

I agree it's probably best to avoid using origin_trial_os, at least for now.

I actually didn't know that origin_trial_os was added until recently. Looking at the CL that added it, I don't think anyone on the OT team weighed in on the implementation. I think that's sub-optimal, so I updated our watch lists to try to prevent that in the future. There's a few moving parts to enabling and exposing trial features, and unfortunately the code isn't all together, which I think contributed to some gaps in the overall implementation for origin_trial_os.

Ah :| I had forgotten that Sam added origin_trial_os specifically for Web Share. Adding him to this thread.

I see we didn't add any OT people to that CL, instead we got the Web Share reviewers. That was an oversight.

 

More comments inline.

Thanks,

Jason

On Fri, Mar 31, 2017 at 1:34 AM, Matt Giuca <mgi...@chromium.org> wrote:

The CL to enable the origin trial: https://codereview.chromium.org/2785913003/

In the end, I removed the origin_trial_os = android (I think this is best anyway). But if you want to try out what happens with that flag turned on, see that CL.

On Fri, 31 Mar 2017 at 15:47 Matt Giuca <mgi...@chromium.org> wrote:

Hi,

I am setting up the InstalledApp feature as an origin trial like this in RuntimeEnabledFeatures.json5:

    {

      name: "InstalledApp",

      origin_trial_feature_name: "InstalledApp",

      // This makes our tests break on non-Android. DO NOT SUBMIT.

      origin_trial_os: ["android"],

      status: "experimental",

    },

Having a few troubles with this on non-Android (Linux):

1. In production, it seems to completely ignore origin_trial_os. Using the test trial token, I can still use the origin trial feature on Linux if I have a valid token. This works on both the static <meta> tag in the HTML file, as well as inserting a <meta> tag dynamically.
2. In the layout test, it ignores the origin_trial_os but only when inserted dynamically. Running the test on Linux, the trial feature isn't available in the static <meta> tag test, but it does succeed when inserted dynamically. That's confusing.

Looking across all the code, I think this is due to the fact OS-filtering logic is only in the generated helper functions to check for enabled trials (in OriginTrials::*). The concept of OS-filtering isn't considered in the core origin trials code (i.e. OriginTrialContext.cpp).  Thus, if there is code that doesn't use the generated helper functions, like the bindings logic (in ConditionalFeatures*), the OS-filtering is applied inconsistently or not at all.

I've logged crbug.com/707232 to fix the implementation of origin_trial_os.

Since it's only used for Web Share, we could just remove it and make manual conditional logic in ConditionalFeaturesForModules. But Ian is going to be automating this code so I guess we *do* want some declarative OS-specific thing.

Is there a precedent for this? Are there any other OTs that launched only on Android (or another specific platform) first? I'm happy for getInstalledRelatedApps to exist on all platforms (but return an empty list) and I think that's fine for most APIs to behave that way. The difference with Web Share is that the existence of the API is the only way to feature-detect whether it's going to work or not, and we (even in full launch, after the OT) want it to be undefined on platforms that don't support it yet.

Maybe that means this should be an IDL property, not an origin trial property. Maybe the rule for origin trials should be "make it work the way you expect the final API to work, across all platforms". So OTs should not be restricted to a certain OS. Rather, you either restrict it to your OS in IDL, or make it available on all OSes but (maybe) give different behaviour in the implementation. Does that sound reasonable? In which case we should remove origin_trial_os.

 

 

What's really confusing is that Web Share (the only other origin trial currently using origin_trial_os, also my feature) seems to be properly restricted to Android in production. (It does not currently have layout tests for origin trial.) Any explanation why Web Share is being restricted correctly while InstalledApp is ignoring origin_trial_os?

From code inspection, I'm not sure why you'd see different behaviour for WebShare vs InstalledApp. Could you give us instructions for how you're testing WebShare?

Sure, the manual testing instructions. Just go to:

https://mgiuca.github.io/web-share/demos/share.html

And click "Share". That site has a valid OT token so you should see navigator.share as a valid function on Android, but undefined on Desktop.

 

 

Lastly, *how* can I do layout tests correctly when using origin_trial_os? Is there a way to write the layout test so that it expects a feature on Android but not on other platforms?

I had started trying to write layout tests for Web Share (see https://codereview.chromium.org/2714733002/). I didn't get through testing all the scenarios, so I haven't tried to land them. As you pointed out, I didn't find a way to detect the os/platform in the layout test. As well, I'm hoping we'll need fewer such layout tests in the future, once Ian finishes changes to automatically generate the code to expose interfaces to JavaScript.

Oh, sorry I didn't mean for you to have to write tests for my feature. Do you want me to take over that CL? (Having said that, Web Share is going out of origin trial in M58 so there may be no point having tests for it unless we do another origin trial.)

Matt

 

 

(Since my feature works properly on non-Android by simply returning an empty list, I think I might just go ahead and not use origin_trial_os, essentially rolling it out on all platforms but with an empty implementation on non-Android platforms, which is how it will be eventually anyway.)

--
You received this message because you are subscribed to the Google Groups "experimentation-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to experimentation...@chromium.org.
To post to this group, send email to experimen...@chromium.org.

[Jason Chase]

Let's continue the discussion about what to do with origin_trial_os on crbug.com/707232, since Matt helpfully started the conversation there.

As for the layout tests for Web Share, I would agree that we can probably just leave them out. If WebShare goes for another trial, or the automatic code generation from Ian isn't going to land soon, we can revisit.

Thanks,

Jason

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

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/experimentation-dev/CAHqYdcbOXscN5FgO_Pvuch%2BNUu-jFZh5%3Dt5evKFAAphuncvRfw%40mail.gmail.com.

--

You received this message because you are subscribed to the Google Groups "experimentation-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to experimentation-dev+unsub...@chromium.org.
To post to this group, send email to experimentation-dev@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/experimentation-dev/CAHqYdcZWQ-w_E2rbNqP8AckVmLSzAFMs0W8D4WkSzPi1WPF3rA%40mail.gmail.com.