Re: [webkit-dev] Exciting JS features (class fields) in need of review :)

[Xan Lopez]

Hi Maciej,

the first patches had the flag indeed, so it should be easy to add it back
to the patch. Not sure what's the usual procedure, but I guess it makes
sense to enable it by default in the bug so that the bots keep testing the
code? Then we'll disable it before landing if that's our decision.

Cheers,
Xan

On Thu, Feb 14, 2019 at 8:47 AM Maciej Stachowiak  wrote:

>
> I left the boring review feedback that this work should be behind a
> feature flag. Mentioning it here because this may apply to other feature
> patches you have in progress. (I am not qualified to review the substance
> of what the patch is doing.)
>
> > On Feb 13, 2019, at 1:51 PM, ca...@igalia.com wrote:
> >
> > Hi WebKitters,
> >
> > My colleagues at Igalia have been working on a number of JS language
> features! We want WebKit to have implementations in order to provide
> feedback for TC39, and to help meet the requirements to have them merged
> into the specification proper.
> >
> > Unfortunately, JSC suggested reviewers have been occupied for the past 6
> months, unable to provide feedback and help us upstream the patches. I'm
> reaching out to see if there are other folks reading webkit-dev who might
> be able to check on the work, see how it looks, and help us get this stuff
> upstream. We've been doing internal reviews, but unfortunately don't yet
> have any JSC reviewers on our team.
> >
> > You can find the patch for instance class fields at
> https://bugs.webkit.org/show_bug.cgi?id=174212. Any kind of constructive
> feedback (from anyone) would be much appreciated :)
> >
> > ___
> > webkit-dev mailing list
> > webkit-dev@lists.webkit.org
> > https://lists.webkit.org/mailman/listinfo/webkit-dev
>
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Experimental features review

[Michael Catanzaro]

On Wed, Feb 13, 2019 at 9:16 PM, Simon Fraser  
wrote:
For these two, we now have them on by default because we think they 
are ready to ship. They still exist as experimental features so that 
people can turn them off for regression testing, but is the policy 
now to move them back to Debug features at this stage?


Well, I'm really not sure, other than that the feature is no longer 
supposed to be experimental once it's ready to be on by default.


I notice there is a new class of features called internal features:
https://trac.webkit.org/changeset/235921/webkit. Perhaps that would 
suffice for regression testing?


Michael

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Exciting JS features (class fields) in need of review :)

[Saam Barati]

Hi Caitlin and Xan,

> On Feb 13, 2019, at 1:51 PM, ca...@igalia.com wrote:
> 
> Hi WebKitters,
> 
> My colleagues at Igalia have been working on a number of JS language 
> features! We want WebKit to have implementations in order to provide feedback 
> for TC39, and to help meet the requirements to have them merged into the 
> specification proper.
> 
> Unfortunately, JSC suggested reviewers have been occupied for the past 6 
> months, unable to provide feedback and help us upstream the patches. I'm 
> reaching out to see if there are other folks reading webkit-dev who might be 
> able to check on the work, see how it looks, and help us get this stuff 
> upstream. We've been doing internal reviews, but unfortunately don't yet have 
> any JSC reviewers on our team.

This is unacceptable. We shouldn’t leave a patch without feedback for so long. 
I’ll review this before EOD.

- Saam
> 
> You can find the patch for instance class fields at 
> https://bugs.webkit.org/show_bug.cgi?id=174212. Any kind of constructive 
> feedback (from anyone) would be much appreciated :)
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Exciting JS features (class fields) in need of review :)

[Maciej Stachowiak]

> On Feb 14, 2019, at 1:37 AM, Xan Lopez  wrote:
> 
> Hi Maciej,
> 
> the first patches had the flag indeed, so it should be easy to add it back to 
> the patch. Not sure what's the usual procedure, but I guess it makes sense to 
> enable it by default in the bug so that the bots keep testing the code? Then 
> we'll disable it before landing if that's our decision.

If it’s a runtime flag then it’s possible to have the tests run without having 
it enabled by default. This is one reason runtime flags are the preferred 
choice, the feature can be running tests all along while still getting further 
polish and refinement.

For a compile-time flag, your approach sounds reasonable to me (patch that 
includes a default-on flag, ideally with a note in the ChangeLog that it will 
be flipped before landing).

We almost never turn on features by default right in the patch where they first 
land, unless it is really small and simple, to reduce risk of anyone 
accidentally shipping it if they happen to cut a release branch soon after it 
lands (and before it has had enough bake time).

BTW I appreciate the contribution of such a substantial feature, I regret that 
you haven’t gotten more active review, and I am glad that Saam is now giving 
you another review pass.

Cheers,
Maciej

> 
> Cheers,
> Xan
> 
> On Thu, Feb 14, 2019 at 8:47 AM Maciej Stachowiak  > wrote:
> 
> I left the boring review feedback that this work should be behind a feature 
> flag. Mentioning it here because this may apply to other feature patches you 
> have in progress. (I am not qualified to review the substance of what the 
> patch is doing.)
> 
> > On Feb 13, 2019, at 1:51 PM, ca...@igalia.com  
> > wrote:
> > 
> > Hi WebKitters,
> > 
> > My colleagues at Igalia have been working on a number of JS language 
> > features! We want WebKit to have implementations in order to provide 
> > feedback for TC39, and to help meet the requirements to have them merged 
> > into the specification proper.
> > 
> > Unfortunately, JSC suggested reviewers have been occupied for the past 6 
> > months, unable to provide feedback and help us upstream the patches. I'm 
> > reaching out to see if there are other folks reading webkit-dev who might 
> > be able to check on the work, see how it looks, and help us get this stuff 
> > upstream. We've been doing internal reviews, but unfortunately don't yet 
> > have any JSC reviewers on our team.
> > 
> > You can find the patch for instance class fields at 
> > https://bugs.webkit.org/show_bug.cgi?id=174212 
> > . Any kind of constructive 
> > feedback (from anyone) would be much appreciated :)
> > 
> > ___
> > webkit-dev mailing list
> > webkit-dev@lists.webkit.org 
> > https://lists.webkit.org/mailman/listinfo/webkit-dev 
> > 
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org 
> https://lists.webkit.org/mailman/listinfo/webkit-dev 
> 

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Experimental features review

[Maciej Stachowiak]

> On Feb 14, 2019, at 9:01 AM, Michael Catanzaro  wrote:
> 
> On Wed, Feb 13, 2019 at 9:16 PM, Simon Fraser  wrote:
>> For these two, we now have them on by default because we think they are 
>> ready to ship. They still exist as experimental features so that people can 
>> turn them off for regression testing, but is the policy now to move them 
>> back to Debug features at this stage?
> 
> Well, I'm really not sure, other than that the feature is no longer supposed 
> to be experimental once it's ready to be on by default.
> 
> I notice there is a new class of features called internal features:
> https://trac.webkit.org/changeset/235921/webkit. Perhaps that would suffice 
> for regression testing?

I think this approach is needlessly confusing. For many features, there’s 
likely to be a period where the default flips, but it’s still useful for it to 
be switchable. Either for debugging, or because it hasn’t shipped in products 
yet and it is useful to compare. It would be sad if flags disappeared the 
moment the default flips, and likewise sad if they moved to a different menu as 
soon as the default flips.

(As an aside, I kind of hate experimental features being a menu like it is in 
Safari. Other browsers have more readable and persistent UI for this, like a 
special page or a settings pane. They also tend to have both default-on and 
default-off flags in the same place, so you don’t get lost on the day the 
default flips.)
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Experimental features review

[Michael Catanzaro]

No strong opinion from me here. I'm not familiar with how Safari's UI 
exposes some features but not others. In the GTK/WPE API, the features 
are not enumerable and only a few selected features are exposed at all, 
so that's not an issue for us.


I do think we have a semantic issue, though, if some features 
considered "experimental" are enabled by default. Not sure what the 
solution is. (And of course, if we change our experimental features 
policy, we'll want to ensure the comment in the WebPreferences.yaml 
file is updated.)


Michael

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Exciting JS features (class fields) in need of review :)

[Mark Lam]

Hi Xan,

FYI, if you’re writing JSC stress tests, you can add test specific options by 
putting the following at the top of the test file:

//@ requireOptions(“--myOption=true”, “--myOtherOption=1234”)

For LayoutTests, you can add the following to the top line of the test html 
file:



This way, the feature can be disabled by default but still receive testing.

Mark

> On Feb 14, 2019, at 9:27 AM, Maciej Stachowiak  wrote:
> 
> 
> 
>> On Feb 14, 2019, at 1:37 AM, Xan Lopez > > wrote:
>> 
>> Hi Maciej,
>> 
>> the first patches had the flag indeed, so it should be easy to add it back 
>> to the patch. Not sure what's the usual procedure, but I guess it makes 
>> sense to enable it by default in the bug so that the bots keep testing the 
>> code? Then we'll disable it before landing if that's our decision.
> 
> If it’s a runtime flag then it’s possible to have the tests run without 
> having it enabled by default. This is one reason runtime flags are the 
> preferred choice, the feature can be running tests all along while still 
> getting further polish and refinement.
> 
> For a compile-time flag, your approach sounds reasonable to me (patch that 
> includes a default-on flag, ideally with a note in the ChangeLog that it will 
> be flipped before landing).
> 
> We almost never turn on features by default right in the patch where they 
> first land, unless it is really small and simple, to reduce risk of anyone 
> accidentally shipping it if they happen to cut a release branch soon after it 
> lands (and before it has had enough bake time).
> 
> BTW I appreciate the contribution of such a substantial feature, I regret 
> that you haven’t gotten more active review, and I am glad that Saam is now 
> giving you another review pass.
> 
> Cheers,
> Maciej
> 
>> 
>> Cheers,
>> Xan
>> 
>> On Thu, Feb 14, 2019 at 8:47 AM Maciej Stachowiak > > wrote:
>> 
>> I left the boring review feedback that this work should be behind a feature 
>> flag. Mentioning it here because this may apply to other feature patches you 
>> have in progress. (I am not qualified to review the substance of what the 
>> patch is doing.)
>> 
>> > On Feb 13, 2019, at 1:51 PM, ca...@igalia.com  
>> > wrote:
>> > 
>> > Hi WebKitters,
>> > 
>> > My colleagues at Igalia have been working on a number of JS language 
>> > features! We want WebKit to have implementations in order to provide 
>> > feedback for TC39, and to help meet the requirements to have them merged 
>> > into the specification proper.
>> > 
>> > Unfortunately, JSC suggested reviewers have been occupied for the past 6 
>> > months, unable to provide feedback and help us upstream the patches. I'm 
>> > reaching out to see if there are other folks reading webkit-dev who might 
>> > be able to check on the work, see how it looks, and help us get this stuff 
>> > upstream. We've been doing internal reviews, but unfortunately don't yet 
>> > have any JSC reviewers on our team.
>> > 
>> > You can find the patch for instance class fields at 
>> > https://bugs.webkit.org/show_bug.cgi?id=174212 
>> > . Any kind of constructive 
>> > feedback (from anyone) would be much appreciated :)
>> > 
>> > ___
>> > webkit-dev mailing list
>> > webkit-dev@lists.webkit.org 
>> > https://lists.webkit.org/mailman/listinfo/webkit-dev 
>> > 
>> 
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org 
>> https://lists.webkit.org/mailman/listinfo/webkit-dev 
>> 
> 
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev