> On Feb 14, 2019, at 1:37 AM, Xan Lopez <x...@gnome.org> 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 <m...@apple.com 
> <mailto:m...@apple.com>> 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 <mailto: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 
> > <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 <mailto:webkit-dev@lists.webkit.org>
> > https://lists.webkit.org/mailman/listinfo/webkit-dev 
> > <https://lists.webkit.org/mailman/listinfo/webkit-dev>
> 
> _______________________________________________
> webkit-dev mailing list
> webkit-dev@lists.webkit.org <mailto:webkit-dev@lists.webkit.org>
> https://lists.webkit.org/mailman/listinfo/webkit-dev 
> <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

Reply via email to