Exciting JS features (class fields) in need of review :)
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 :)
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, caitp@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
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 <mjs@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, caitp@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
On Feb 14, 2019, at 1:37 AM, Xan Lopez <xan@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 <mjs@apple.com <mailto:mjs@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, caitp@igalia.com <mailto:caitp@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>
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: <!-- webkit-test-runner [ jscOptions=--myOptions=true ] --> This way, the feature can be disabled by default but still receive testing. Mark
On Feb 14, 2019, at 9:27 AM, Maciej Stachowiak <mjs@apple.com> wrote:
On Feb 14, 2019, at 1:37 AM, Xan Lopez <xan@gnome.org <mailto:xan@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 <mjs@apple.com <mailto:mjs@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, caitp@igalia.com <mailto:caitp@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
Hi all, last February we got a thorough review of the class fields patch, and have been updating it pretty frequently since. We think it's ready to be reviewed again. Hopefully we'll manage to either land this first stage of the feature, or receive enough new feedback to keep working on it. The bug is: https://bugs.webkit.org/show_bug.cgi?id=174212 Cheers, Xan On Fri, Feb 15, 2019 at 12:21 PM Mark Lam <mark.lam@apple.com> wrote:
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:
<!-- webkit-test-runner [ jscOptions=--myOptions=true ] -->
This way, the feature can be disabled by default but still receive testing.
Mark
On Feb 14, 2019, at 9:27 AM, Maciej Stachowiak <mjs@apple.com> wrote:
On Feb 14, 2019, at 1:37 AM, Xan Lopez <xan@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 <mjs@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, caitp@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
Hi Caitlin and Xan,
On Feb 13, 2019, at 1:51 PM, caitp@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
participants (5)
-
caitp＠igalia.com
-
Maciej Stachowiak
-
Mark Lam
-
Saam Barati
-
Xan Lopez