Re: [webkit-dev] [Styling] () for a lambda without arguments (Was Space between [] and () in C++ lambdas)

[Ryosuke Niwa]

Seems like there is a consensus here. Here's a patch to codify it in our
code style guidelines: https://bugs.webkit.org/show_bug.cgi?id=204021

On Mon, Nov 4, 2019 at 8:06 AM Alex Christensen 
wrote:

> When the lambda is mutable or has a trailing return type, the () is
> currently required by the C++ grammar, so we can’t say to always omit ().
> We could say to always have it, to only have it when necessary, or have no
> code style guideline.  I think we should always have spaces before and
> after, though.
>
> On Nov 3, 2019, at 3:27 AM, Ryosuke Niwa  wrote:
>
>
>
> On Sat, Nov 2, 2019 at 8:25 PM Ryosuke Niwa  wrote:
>
>>
>> On Sat, Nov 2, 2019 at 7:54 PM Chris Dumez  wrote:
>>
>>>
>>>
>>> On Nov 2, 2019, at 7:38 PM, Ryosuke Niwa  wrote:
>>>
>>> 
>>>
>>> On Sat, Nov 2, 2019 at 1:23 AM Antti Koivisto  wrote:
>>>

 On Sat, Nov 2, 2019 at 1:38 AM Ryosuke Niwa  wrote:

> On Fri, Nov 1, 2019 at 11:53 AM Michael Catanzaro <
> mcatanz...@gnome.org> wrote:
>
>> On Fri, Nov 1, 2019 at 11:19 am, Ryosuke Niwa 
>> wrote:
>> > Namely, some people write a lambda as:
>> > auto x = [] () { }
>> >
>> > with a space between [] and () while others would write it as:
>> >
>> > auto x = []() { }
>>
>> 🔧: I omit the () when there are no parameters, as in these examples.
>>
>
> I guess that's another thing we should decide. Should we, or should we
> not have () when there are no arguments.
>

 I think this is easily settled by voting via exiting practice. We have
 1287 instances of [&] { and 107 instances of [&]() { and &] () { across the
 whole WebKit.

>>>
>>> That’s good to know. Why don’t we go with the status quo then.
>>>
>>> In this case, we do put a space between ] or ) and {, right?
>>>
>>>
>>> How is this the conclusion from Antti’s comment?
>>>
>>> Based on the discussion so far, it thought no space had a slight lead.
>>>
>>
>> I think you’re conflating this discussion with the other email thread
>> about a space between [] and ().
>>
>> Here, I’m talking about placing a space after [] before { as in:
>> [] { }
>>
>> As opposed to:
>> []{ }
>>
>> We never use the latter style whether it’s other control flow statements
>> like if, while, or for, or for function definitions.
>>
>> - R. Niwa
>>
>> --
>> - R. Niwa
>>
> --
> - R. Niwa
> ___
> 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] [Styling] Space between [] and () in C++ lambdas

[Ryosuke Niwa]

There has been no more votes either way so no space wins. Here's a patch to
codify it in our code style guidelines:
https://bugs.webkit.org/show_bug.cgi?id=204021

- R. Niwa

On Sat, Nov 2, 2019 at 8:26 PM Ryosuke Niwa  wrote:

> On Sat, Nov 2, 2019 at 10:16 AM Caitlin Potter  wrote:
>
>> Not that anybody asked me, but I also prefer to not include a space
>> between captures and parameter, for similar reasons.
>>
>> If I’m not mistaken, v8/chromium tends to omit the space as well. If
>> that’s still true and WebKit adopted that style, context switching between
>> both codebases would be marginally easier for me.
>>
>> On Nov 2, 2019, at 4:19 AM, Antti Koivisto  wrote:
>>
>> 
>>
>> On Fri, Nov 1, 2019 at 10:50 PM Yusuke Suzuki  wrote:
>>
>>>
>>> > On Nov 1, 2019, at 11:53, Michael Catanzaro 
>>> wrote:
>>> >
>>> > On Fri, Nov 1, 2019 at 11:19 am, Ryosuke Niwa 
>>> wrote:
>>> >> Namely, some people write a lambda as:
>>> >> auto x = [] () { }
>>> >> with a space between [] and () while others would write it as:
>>> >> auto x = []() { }
>>> >
>>> > 🔧: I omit the () when there are no parameters, as in these examples.
>>> >
>>> > No preference on spacing.
>>>
>>> I like having a space here, because this rule is simpler to me.
>>> If we always have a space between them, this is clear that the above
>>> case is written in `[] { }` instead of `[]{ }`.
>>>
>>
>> I prefer not having the redundant space in [](). It also makes logical
>> sense to me to keep the lambda signature together. I started using lambdas
>> with space there, dropped it later, and suffered no adverse consequences.
>>
>> As for existing practice, WebCore favors spaceless ]( about 2:1 but
>> across the entire WebKit it is closer to 1:1.
>>
>> We always put space before { } block, I don't think that is really in
>> question here, or creating any inconsistencies.
>>
>>
>>antti
>>
>>
>>>
>>> -Yusuke
>>>
>>> >
>>> >
>>> > ___
>>> > 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
>>
> --
> - R. Niwa
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

[webkit-dev] Supporting for finding ref tests

[Simon Fraser]

I'd like to land a patch to support finding test references via :
https://bugs.webkit.org/show_bug.cgi?id=203784 


There has been some discussion about this in the past:
https://lists.webkit.org/pipermail/webkit-dev/2011-November/018470.html 


But I think the benefits outweigh the drawbacks. As that mail states:
> *Link element approach*
> Pros:
> 
>- Can reuse same ref. file for multiple tests
Still true.
>- Can have multiple ref. files for single test
True but no something that we support, and I haven't see any WPT use this (our 
importer throws an error if it sees this)
>- Information is self-contained in the test file
Still true
>- We may get away with test suite build step
It certainly simplifies WPT test import.
Currently importing some CSS suites (e.g. css-backgrounds) results in broken 
-expected.html files because copying them breaks references to sub resources.
> (It turns out that we can't convert W3C ref tests to use WebKit conventions
> due to the first two points.)
We're doing this much more now, and the "multiple references" point is moot, so 
I think we can import WPT tests mostly as-is.
> 
> Cons:
> 
>- Requires us modifying each port's DRT to support this format
No, it just requires webkitpy hacking which I've done in the patch.
>- Adding link elements itself may affect tests (all W3C tests are
>required to have link elements at the moment)
I haven't seen this be an issue.
>- Hard to understand relationship between files. e.g. if we want to
>figure out which tests use ref.html, we must look at all test files
This is true, but I don't really see it being a problem in practice. What I 
have seen is us importing CSS 2.1 tests that have foo.html and foo-ref.html, 
and treating foo-ref.html as a test so generating foo-expected.txt and 
foo-ref-expected.txt. That seems worse.
>- Other browser vendors (Firefox and Opera) prefer manifest file approach
This is no longer true. "reftest.list" files are deprecated 
(webkit.org/b/203783, https://github.com/web-platform-tests/wpt/issues/20060 
).
So now that WPT is heavily invested in  I think we should follow 
suite. It will simplify WPT import, and reduced the number of cloned 
-expected.html files significantly.
Simon

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

Re: [webkit-dev] Supporting for finding ref tests

[Ryosuke Niwa]

On Fri, Nov 8, 2019 at 2:01 PM Simon Fraser  wrote:
>
> I'd like to land a patch to support finding test references via  rel="match/mismatch">:
> https://bugs.webkit.org/show_bug.cgi?id=203784
>
> There has been some discussion about this in the past:
> https://lists.webkit.org/pipermail/webkit-dev/2011-November/018470.html
>
> But I think the benefits outweigh the drawbacks. As that mail states:
>
> *Link element approach*
> Pros:
>
>- Can reuse same ref. file for multiple tests
>
> Still true.
>
>- Can have multiple ref. files for single test
>
> True but no something that we support, and I haven't see any WPT use this 
> (our importer throws an error if it sees this)
>
>- Information is self-contained in the test file
>
> Still true
>
>- We may get away with test suite build step
>
> It certainly simplifies WPT test import.
>
> Currently importing some CSS suites (e.g. css-backgrounds) results in broken 
> -expected.html files because copying them breaks references to sub resources.
>
> (It turns out that we can't convert W3C ref tests to use WebKit conventions
> due to the first two points.)
>
> We're doing this much more now, and the "multiple references" point is moot, 
> so I think we can import WPT tests mostly as-is.
>
> Cons:
>
>- Requires us modifying each port's DRT to support this format
>
> No, it just requires webkitpy hacking which I've done in the patch.

I'm not certain writing a bunch of regular expressions in webkitpy is
a reliable mechanism to find expected results. Another issue I found
back then was that it significantly slowed run-webkit-tests' startup
time because WPT has a workflow to find all tests & their expected
results upfront before any tests could run.
>
>- Adding link elements itself may affect tests (all W3C tests are
>required to have link elements at the moment)
>
> I haven't seen this be an issue.

Another issue is that if you were to modify a test which happens to be
also used as a reference or a mismatch result (worse) for some other
test, then you may not notice that without inspecting every other test
in existence.

>- Hard to understand relationship between files. e.g. if we want to
>figure out which tests use ref.html, we must look at all test files
>
> This is true, but I don't really see it being a problem in practice.

This definitely is an issue. It's possible WPT has improved things but
we've definitely had an experience where tests were used as reference
for other tests, etc... and having to think about this issue every
time I touch test drove me nuts.

> What I have seen is us importing CSS 2.1 tests that have foo.html and 
> foo-ref.html, and treating foo-ref.html as a test so generating 
> foo-expected.txt and foo-ref-expected.txt. That seems worse.

Seems like we can treat "-ref" as a special suffix like we already do
with support directory and resources directory.

> So now that WPT is heavily invested in  I think we should follow 
> suite. It will simplify WPT import, and reduced the number of cloned 
> -expected.html files significantly.

I really don't want to deal with tests being used as references for
other tests. I'm okay with this approach if we forbid that.

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

Re: [webkit-dev] Supporting for finding ref tests

[Robert Ma]

WPT has recently passed an RFC

trying to simplify the reftest graph (although it has not been implemented
yet), which eliminates a lot of the complexities and concerns.

On Fri, Nov 8, 2019 at 5:07 PM Ryosuke Niwa  wrote:

> On Fri, Nov 8, 2019 at 2:01 PM Simon Fraser 
> wrote:
> >
> > I'd like to land a patch to support finding test references via  rel="match/mismatch">:
> > https://bugs.webkit.org/show_bug.cgi?id=203784
> >
> > There has been some discussion about this in the past:
> > https://lists.webkit.org/pipermail/webkit-dev/2011-November/018470.html
> >
> > But I think the benefits outweigh the drawbacks. As that mail states:
> >
> > *Link element approach*
> > Pros:
> >
> >- Can reuse same ref. file for multiple tests
> >
> > Still true.
> >
> >- Can have multiple ref. files for single test
> >
> > True but no something that we support, and I haven't see any WPT use
> this (our importer throws an error if it sees this)
> >
> >- Information is self-contained in the test file
> >
> > Still true
> >
> >- We may get away with test suite build step
> >
> > It certainly simplifies WPT test import.
> >
> > Currently importing some CSS suites (e.g. css-backgrounds) results in
> broken -expected.html files because copying them breaks references to sub
> resources.
> >
> > (It turns out that we can't convert W3C ref tests to use WebKit
> conventions
> > due to the first two points.)
> >
> > We're doing this much more now, and the "multiple references" point is
> moot, so I think we can import WPT tests mostly as-is.
> >
> > Cons:
> >
> >- Requires us modifying each port's DRT to support this format
> >
> > No, it just requires webkitpy hacking which I've done in the patch.
>
> I'm not certain writing a bunch of regular expressions in webkitpy is
> a reliable mechanism to find expected results. Another issue I found
> back then was that it significantly slowed run-webkit-tests' startup
> time because WPT has a workflow to find all tests & their expected
> results upfront before any tests could run.
> >
> >- Adding link elements itself may affect tests (all W3C tests are
> >required to have link elements at the moment)
> >
> > I haven't seen this be an issue.
>
> Another issue is that if you were to modify a test which happens to be
> also used as a reference or a mismatch result (worse) for some other
> test, then you may not notice that without inspecting every other test
> in existence.
>
> >- Hard to understand relationship between files. e.g. if we want to
> >figure out which tests use ref.html, we must look at all test files
> >
> > This is true, but I don't really see it being a problem in practice.
>
> This definitely is an issue. It's possible WPT has improved things but
> we've definitely had an experience where tests were used as reference
> for other tests, etc... and having to think about this issue every
> time I touch test drove me nuts.
>
> > What I have seen is us importing CSS 2.1 tests that have foo.html and
> foo-ref.html, and treating foo-ref.html as a test so generating
> foo-expected.txt and foo-ref-expected.txt. That seems worse.
>
> Seems like we can treat "-ref" as a special suffix like we already do
> with support directory and resources directory.
>
> > So now that WPT is heavily invested in  I think we should
> follow suite. It will simplify WPT import, and reduced the number of cloned
> -expected.html files significantly.
>
> I really don't want to deal with tests being used as references for
> other tests. I'm okay with this approach if we forbid that.
>
> - R. Niwa
> ___
> 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] Supporting for finding ref tests

[Simon Fraser]

> On Nov 8, 2019, at 2:07 PM, Ryosuke Niwa  wrote:
> 
> On Fri, Nov 8, 2019 at 2:01 PM Simon Fraser  wrote:
>> 
>> I'd like to land a patch to support finding test references via > rel="match/mismatch">:
>> https://bugs.webkit.org/show_bug.cgi?id=203784
>> 
>> There has been some discussion about this in the past:
>> https://lists.webkit.org/pipermail/webkit-dev/2011-November/018470.html
>> 
>> But I think the benefits outweigh the drawbacks. As that mail states:
>> 
>> *Link element approach*
>> Pros:
>> 
>>   - Can reuse same ref. file for multiple tests
>> 
>> Still true.
>> 
>>   - Can have multiple ref. files for single test
>> 
>> True but no something that we support, and I haven't see any WPT use this 
>> (our importer throws an error if it sees this)
>> 
>>   - Information is self-contained in the test file
>> 
>> Still true
>> 
>>   - We may get away with test suite build step
>> 
>> It certainly simplifies WPT test import.
>> 
>> Currently importing some CSS suites (e.g. css-backgrounds) results in broken 
>> -expected.html files because copying them breaks references to sub resources.
>> 
>> (It turns out that we can't convert W3C ref tests to use WebKit conventions
>> due to the first two points.)
>> 
>> We're doing this much more now, and the "multiple references" point is moot, 
>> so I think we can import WPT tests mostly as-is.
>> 
>> Cons:
>> 
>>   - Requires us modifying each port's DRT to support this format
>> 
>> No, it just requires webkitpy hacking which I've done in the patch.
> 
> I'm not certain writing a bunch of regular expressions in webkitpy is
> a reliable mechanism to find expected results. Another issue I found
> back then was that it significantly slowed run-webkit-tests' startup
> time because WPT has a workflow to find all tests & their expected
> results upfront before any tests could run.

The patch uses html5lib (via BeautifulSoup), which is exactly what WPT, and our 
importer use to find the ref tests.

We don't find references up-front; only when running each test. This patch does 
add some overhead for parsing each test file,
which I measured to be about 1-2 sec on a directory which took 30s to run. I 
think this slight slowdown is worthwhile (we could
probably eliminate it with some webkitpy optimizations).

>> 
>>   - Adding link elements itself may affect tests (all W3C tests are
>>   required to have link elements at the moment)
>> 
>> I haven't seen this be an issue.
> 
> Another issue is that if you were to modify a test which happens to be
> also used as a reference or a mismatch result (worse) for some other
> test, then you may not notice that without inspecting every other test
> in existence.

EWS will tell you. Also, generally a file won't act as both a test and a 
reference; I don't see us deviating from
our current "-expected.html" naming conventions. BTW, you don't *have* to add a 
 tag; we'll still fall
back to the current search behavior. If you have both, then webkitpy will warn.

> 
>>   - Hard to understand relationship between files. e.g. if we want to
>>   figure out which tests use ref.html, we must look at all test files
>> 
>> This is true, but I don't really see it being a problem in practice.
> 
> This definitely is an issue. It's possible WPT has improved things but
> we've definitely had an experience where tests were used as reference
> for other tests, etc... and having to think about this issue every
> time I touch test drove me nuts.

Do you have any examples? If this does happen in WPT, we should discourage it 
(and can fix it via PRs).

Generally references live in a resources/ or references/ subdirectory, or have 
"-ref" in the name.

> 
>> What I have seen is us importing CSS 2.1 tests that have foo.html and 
>> foo-ref.html, and treating foo-ref.html as a test so generating 
>> foo-expected.txt and foo-ref-expected.txt. That seems worse.
> 
> Seems like we can treat "-ref" as a special suffix like we already do
> with support directory and resources directory.

All that works now.

> 
>> So now that WPT is heavily invested in  I think we should follow 
>> suite. It will simplify WPT import, and reduced the number of cloned 
>> -expected.html files significantly.
> 
> I really don't want to deal with tests being used as references for
> other tests. I'm okay with this approach if we forbid that.

I'm OK with that (enforced by code review unless we see the need for tooling).

Simon

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

Re: [webkit-dev] Supporting for finding ref tests

[Simon Fraser]

> On Nov 8, 2019, at 2:15 PM, Robert Ma  wrote:
> 
> WPT has recently passed an RFC 
> 
>  trying to simplify the reftest graph (although it has not been implemented 
> yet), which eliminates a lot of the complexities and concerns.

That sounds like a very sensible proposal.

It will take some work in webkitpy to support multiple , but since 
we don't support that when importing now anyway, I don't see it as a blocker to 
adding  support.

I filed webkit.org/b/204022  to implement support 
for multiple .

Simon

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

Re: [webkit-dev] Supporting for finding ref tests

[Ryosuke Niwa]

On Fri, Nov 8, 2019 at 2:15 PM Simon Fraser  wrote:
>
> > On Nov 8, 2019, at 2:07 PM, Ryosuke Niwa  wrote:
> >
> > On Fri, Nov 8, 2019 at 2:01 PM Simon Fraser  wrote:
> >>
> >> I'd like to land a patch to support finding test references via  >> rel="match/mismatch">:
> >> https://bugs.webkit.org/show_bug.cgi?id=203784
> >>
> >> There has been some discussion about this in the past:
> >> https://lists.webkit.org/pipermail/webkit-dev/2011-November/018470.html
> >>
> >> But I think the benefits outweigh the drawbacks. As that mail states:
> >>
> >> *Link element approach*
> >> Pros:
> >>
> >>   - Can reuse same ref. file for multiple tests
> >>
> >> Still true.
> >>
> >>   - Can have multiple ref. files for single test
> >>
> >> True but no something that we support, and I haven't see any WPT use this 
> >> (our importer throws an error if it sees this)
> >>
> >>   - Information is self-contained in the test file
> >>
> >> Still true
> >>
> >>   - We may get away with test suite build step
> >>
> >> It certainly simplifies WPT test import.
> >>
> >> Currently importing some CSS suites (e.g. css-backgrounds) results in 
> >> broken -expected.html files because copying them breaks references to sub 
> >> resources.
> >>
> >> (It turns out that we can't convert W3C ref tests to use WebKit conventions
> >> due to the first two points.)
> >>
> >> We're doing this much more now, and the "multiple references" point is 
> >> moot, so I think we can import WPT tests mostly as-is.
> >>
> >> Cons:
> >>
> >>   - Requires us modifying each port's DRT to support this format
> >>
> >> No, it just requires webkitpy hacking which I've done in the patch.
> >
> > I'm not certain writing a bunch of regular expressions in webkitpy is
> > a reliable mechanism to find expected results. Another issue I found
> > back then was that it significantly slowed run-webkit-tests' startup
> > time because WPT has a workflow to find all tests & their expected
> > results upfront before any tests could run.
>
> The patch uses html5lib (via BeautifulSoup), which is exactly what WPT, and 
> our importer use to find the ref tests.
>
> We don't find references up-front; only when running each test. This patch 
> does add some overhead for parsing each test file,
> which I measured to be about 1-2 sec on a directory which took 30s to run. I 
> think this slight slowdown is worthwhile (we could
> probably eliminate it with some webkitpy optimizations).

Hm... that's ~3% overhead.

> >>   - Adding link elements itself may affect tests (all W3C tests are
> >>   required to have link elements at the moment)
> >>
> >> I haven't seen this be an issue.
> >
> > Another issue is that if you were to modify a test which happens to be
> > also used as a reference or a mismatch result (worse) for some other
> > test, then you may not notice that without inspecting every other test
> > in existence.
>
> EWS will tell you. Also, generally a file won't act as both a test and a 
> reference; I don't see us deviating from
> our current "-expected.html" naming conventions. BTW, you don't *have* to add 
> a  tag; we'll still fall
> back to the current search behavior. If you have both, then webkitpy will 
> warn.

I think we should enforce that in our tooling then.

> >>   - Hard to understand relationship between files. e.g. if we want to
> >>   figure out which tests use ref.html, we must look at all test files
> >>
> >> This is true, but I don't really see it being a problem in practice.
> >
> > This definitely is an issue. It's possible WPT has improved things but
> > we've definitely had an experience where tests were used as reference
> > for other tests, etc... and having to think about this issue every
> > time I touch test drove me nuts.
>
> Do you have any examples? If this does happen in WPT, we should discourage it 
> (and can fix it via PRs).

Oh yeah, it's discouraged but it still happens. If we're doing this in
WebKit, run-webkit-tests should outright refuse to run such tests.

> Generally references live in a resources/ or references/ subdirectory, or 
> have "-ref" in the name.

We need to enforce that in tool.

> >> What I have seen is us importing CSS 2.1 tests that have foo.html and 
> >> foo-ref.html, and treating foo-ref.html as a test so generating 
> >> foo-expected.txt and foo-ref-expected.txt. That seems worse.
> >
> > Seems like we can treat "-ref" as a special suffix like we already do
> > with support directory and resources directory.
>
> All that works now.

Cool. Now that I think about it, I may have added that LOL.

> >> So now that WPT is heavily invested in  I think we should 
> >> follow suite. It will simplify WPT import, and reduced the number of 
> >> cloned -expected.html files significantly.
> >
> > I really don't want to deal with tests being used as references for
> > other tests. I'm okay with this approach if we forbid that.
>
> I'm OK with that (enforced by code review unless we see the need for tooling).

I think we shoul