Re: [webkit-dev] maybe_unused vs UNUSED_PARAM

[Chris Dumez via webkit-dev]

Right, as long as it is part of the language and consistent across compilers / 
platforms, I don’t think we need to use macros.

> On Jan 24, 2024, at 11:59 PM, Anne van Kesteren  wrote:
> 
> I had a [[fallthrough]] patch, but internal C code got in the way:
> 
> - https://en.cppreference.com/w/c/language/attributes/fallthrough
> - https://bugs.webkit.org/show_bug.cgi?id=265789
> 
> Using them directly where we can seems nice for (new) readers of the code at 
> least. Not sure what a macro for [[fallthrough]] would buy us for instance.
> 
>> On Jan 25, 2024, at 12:28 AM, Ryosuke Niwa via webkit-dev 
>>  wrote:
>> 
>> If we’re adopting [[maybe_unused]], do we just write that directly in each 
>> function declaration / definition? Or do we define some a macro to do that 
>> anyway?
>> 
>> What bout other kinds of attributes like [[noreturn]], [[fallthrough]], and 
>> [[likely]]? Are we gonna start writing them directly in code, or are we 
>> gonna continue to use macros?
>> 
>> - R. NIwa
>> 
>>> On Jan 24, 2024, at 9:49 AM, Chris Dumez via webkit-dev 
>>>  wrote:
>>> 
>>> Hi,
>>> 
>>> Thanks for starting this discussion.
>>> 
>>> I personally think it would be nice for us to switch to [[maybe_unused]] 
>>> since it is now part of the language and it seems to fit our needs. 
>>> However, I do think we should be consistent and stop using UNUSED_PARAM() / 
>>> ASSERT_UNUSED() in new code entirely then.
>>> 
>>> So if we decide to switch, I think should add style checks to prevent using 
>>> UNUSED_PARAM() / ASSERT_UNUSED() and recommend using [[maybe_unused]] 
>>> instead. Eventually, we should try to phase out existing usage of these 
>>> macros so that we can remove them entirely.
>>> 
>>> Cheers,
>>> Chris.
>>> 
>>>> On Jan 24, 2024, at 9:34 AM, Alex Christensen via webkit-dev 
>>>>  wrote:
>>>> 
>>>> For many years we have used the UNUSED_PARAM macros, and we have almost 
>>>> 3000 of them.  C++17 introduced [[maybe_unused]] for this purpose, and a 
>>>> few uses of it are starting to pop up in WebKit.  Should we switch, should 
>>>> we transition, should we allow both, or should we just stick with 
>>>> UNUSED_PARAM?
>>>> ___
>>>> 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

Re: [webkit-dev] maybe_unused vs UNUSED_PARAM

[Chris Dumez via webkit-dev]

Hi,

Thanks for starting this discussion.

I personally think it would be nice for us to switch to [[maybe_unused]] since 
it is now part of the language and it seems to fit our needs. However, I do 
think we should be consistent and stop using UNUSED_PARAM() / ASSERT_UNUSED() 
in new code entirely then.

So if we decide to switch, I think should add style checks to prevent using 
UNUSED_PARAM() / ASSERT_UNUSED() and recommend using [[maybe_unused]] instead. 
Eventually, we should try to phase out existing usage of these macros so that 
we can remove them entirely.

Cheers,
Chris.

> On Jan 24, 2024, at 9:34 AM, Alex Christensen via webkit-dev 
>  wrote:
> 
> For many years we have used the UNUSED_PARAM macros, and we have almost 3000 
> of them.  C++17 introduced [[maybe_unused]] for this purpose, and a few uses 
> of it are starting to pop up in WebKit.  Should we switch, should we 
> transition, should we allow both, or should we just stick with UNUSED_PARAM?
> ___
> 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] Stricter type checking in downcast<>()

[Chris Dumez via webkit-dev]

Hi,

I have recently landed stricter type checking in downcast<>() [1]. It is 
stricter because the check is now happening in release / production builds on 
some platforms (ARM-based).
My objective is to enable to check in release on all platforms in the near 
future (still a small performance hit on remaining platforms at the moment).

Because of this, it is now recommended to use dynamicDowncast<>() instead of 
is<>() + downcast<>(), to avoid duplicating the type check.
dynamicDowncast<>() is also less error-prone and often results in more concise 
code.

If you have a case where performance matters and you’re confident a type check 
is not required, you may rely on uncheckedDowncast<>().
uncheckedDowncast<>() behaves the same way downcast<>() used to before my 
change (type check on debug builds only).

One such example I’ve seen in our codebase is when using a switch statement 
based on the type:
```
switch (node.nodeType()) {
case Node::DOCUMENT_TYPE_NODE:
  uncheckedDowncast(node)->foo();
```

Please let me know if you have any concerns / questions.

Cheers,
Chris Dumez.

[1] https://commits.webkit.org/272296@main

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

Re: [webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?

[Chris Dumez via webkit-dev]

> On Nov 28, 2023, at 4:06 PM, Jean-Yves Avenard  
> wrote:
> 
> Hi
> 
>> On 29 Nov 2023, at 9:44 am, Chris Dumez via webkit-dev 
>>  wrote:
>> 
>> FYI, our official documentation on WebKit.org <http://webkit.org/> says:
>> ```
>> Making unofficial reviews before you become a reviewer is encouraged. This 
>> is an excellent way to show your skills. Note that you should not put r+ nor 
>> r- on patches in such unofficial reviews.
>> ```
>> I guess this wan’t updated after the move to GitHub. For me, no r+ or r- on 
>> bugzilla translates to no approve / deny PRs on GitHub. So I simply wish 
>> we’d start enforcing this policy again.
>> 
>> Having the tools help us would be great but I don’t think it stops us from 
>> enforcing our own policies like we used to.
> 
> Personally, I’ve been requesting non-official reviewers to review my patches 
> because I know that their skill set is perfectly matched (and it will help 
> make them official reviewer)
> 
> Having them giving r+ explicitly is, I find, easier to spot than looking 
> through the often busy GitHub page to find the comments.

Even if someone approves, you’d still need to find the comments. r+ with 
comments is super common.

> 
> Could we relax the ability to give informal r+ review to people with commit 
> rights? 

They can just leave a comment saying the patch looks good. They don’t have to 
approve the PR. Approving the PR means nothing since it won’t let you merge 
your PR.
People are expected to wait for an official review to trigger the merge queue, 
we’re currently making it harder than it needs to be if we have an official 
review or not.

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

Re: [webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?

[Chris Dumez via webkit-dev]

FYI, our official documentation on WebKit.org <http://webkit.org/> says:
```
Making unofficial reviews before you become a reviewer is encouraged. This is 
an excellent way to show your skills. Note that you should not put r+ nor r- on 
patches in such unofficial reviews.
```
I guess this wan’t updated after the move to GitHub. For me, no r+ or r- on 
bugzilla translates to no approve / deny PRs on GitHub. So I simply wish we’d 
start enforcing this policy again.

Having the tools help us would be great but I don’t think it stops us from 
enforcing our own policies like we used to.

> On Nov 28, 2023, at 1:58 PM, Michael Catanzaro  wrote:
> 
> On Tue, Nov 28 2023 at 01:23:12 PM -0800, Chris Dumez via webkit-dev 
>  wrote:
>> I´m on board if it also cancels PR rejections from non-reviewers, not just 
>> approvals. I don´t see how approvals differ from rejections.
> 
> Sure. It doesn't really matter whether rejections are canceled or not, 
> because the important part of the rejection is the comments that were added, 
> not the rejection status itself. A rejection from a non-reviewer is not 
> effective anyway, so it's fine to have a bot clarify that.
> 
> Michael
> 
> 

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

Re: [webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?

[Chris Dumez via webkit-dev]

> On Nov 28, 2023, at 11:51 AM, Michael Catanzaro  wrote:
> 
> On Tue, Nov 28 2023 at 10:27:41 AM -0800, Chris Dumez via webkit-dev 
>  wrote:
>> The reason I would like us to make enforce this rule is that I find it 
>> confusing. We have a lot of new comers in the project and I do not always 
>> know if a person is a reviewer or not yet. I imagine it may be even more 
>> confusing for non-Apple people.
>> I have in some cases not reviewed patches because I had seen the "green 
>> check” and thought the PR already had been approved.
> 
> +1, if there is a green checkmark, then I assume that person is a reviewer. 
> It's really confusing when non-reviewers add the green checkmark.

FYI, my understanding is that the person gets a *green* checkmark when the 
person is present in contributors.json (common case), even if not marked as a 
reviewer in that file.
They get a *grey* checkmark when they’re not present in contributors.json (not 
common). Also, the difference between green and grey for this tiny checkmark is 
super subtle.

That said, most of the instances where I saw it happened was with a green 
checkmark.

Either way, I think non-reviewers should not approve/reject PR, just add 
comments to avoid confusion.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

[webkit-dev] Approving / Rejects PRs on GitHub when not a reviewer?

[Chris Dumez via webkit-dev]

Hi,

Back in the Bugzilla days, only reviewers were allowed to r+ or r- patches. 
Non-reviewers were - of course - encouraged to do informal reviews but they 
would do so by leaving comments. They would never r+ / r-.

Since we’ve moved to Github, it seems we have become a lot more lax about this 
and I have seen non-reviewers approve and reject PRs, not just leaving comments.
My understanding is that there is no way to prevent this with Github but could 
we at least make it a policy that non-reviewers should not approve / reject PRs 
and only leave comments instead?

The reason I would like us to make enforce this rule is that I find it 
confusing. We have a lot of new comers in the project and I do not always know 
if a person is a reviewer or not yet. I imagine it may be even more confusing 
for non-Apple people.

I have in some cases not reviewed patches because I had seen the "green check” 
and thought the PR already had been approved.
I have also seen cases of PRs rejected, asking the author to do more work, that 
I didn’t feel was necessary.
There is no easy way from the GitHub UI to tell if the person who 
approved/rejected your PR is actually a reviewer, as far as I know.

What do you think?

Thanks,
Chris Dumez.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Removal of trailing whitespace

[Chris Dumez via webkit-dev]

> On Apr 12, 2023, at 10:23 AM, Ryosuke Niwa via webkit-dev 
>  wrote:
> 
> Yeah, enforcing that new or otherwise modified lines don’t have trailing 
> whitespaces would be good.

Yes, I wouldn’t mind that either.

However, https://commits.webkit.org/262879@main has just landed and if you look 
at the changes to Document.cpp, it is mostly spacing changes :(
It makes it harder to review or to identify meaningful changes in a patch after 
landing. It also pollutes git blame for no great reason.

> 
> - R. Niwa
> 
>> On Apr 12, 2023, at 10:20 AM, Yusuke Suzuki  wrote:
>> 
>> I agree that we should not do it because it pollutes change history of 
>> files, git-blame results, and review-diff in PR.
>> But at the same time, I think there is no reason to add a new trailing 
>> whitespace via a new commit.
>> It is nice if we can enforce this rule only for newly added code (via 
>> style-checker) not to add new trailing spaces.
>> 
>> -Yusuke
>> 
>>> On Apr 12, 2023, at 10:08 AM, Ryosuke Niwa via webkit-dev 
>>>  wrote:
>>> 
>>> WebKi proejctt’s long term policy has been to not do this:
>>> https://lists.webkit.org/pipermail/webkit-dev/2009-August/009665.html
>>> 
>>> I don’t think we should change that.
>>> 
>>> - R. Niwa
>>> 
>>>> On Apr 12, 2023, at 9:17 AM, Chris Dumez via webkit-dev 
>>>>  wrote:
>>>> 
>>>> I am against this because it adds a lot of noise to patches I am trying to 
>>>> review.
>>>> I have seen PRs where white space changes account for more than half the 
>>>> patch I am trying to review.
>>>> 
>>>> Dropping trailing spaces on the lines you’re modifying is OK but in the 
>>>> whole file is too noisy IMO.
>>>> 
>>>> Chris.
>>>> 
>>>>> On Apr 12, 2023, at 1:22 AM, Anne van Kesteren via webkit-dev 
>>>>>  wrote:
>>>>> 
>>>>> To reduce the overhead of switching between projects with different
>>>>> whitespace requirements, I would like to suggest we start being
>>>>> lenient when trailing whitespace is removed. In particular when a file
>>>>> is being changed to fix a bug.
>>>>> 
>>>>> I could see going even further and enforcing this via the style
>>>>> checker, if there is appetite for that.
>>>>> 
>>>>> Thanks for considering!
>>>>> ___
>>>>> 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

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

Re: [webkit-dev] Removal of trailing whitespace

[Chris Dumez via webkit-dev]

I am against this because it adds a lot of noise to patches I am trying to 
review.
I have seen PRs where white space changes account for more than half the patch 
I am trying to review.

Dropping trailing spaces on the lines you’re modifying is OK but in the whole 
file is too noisy IMO.

Chris.

> On Apr 12, 2023, at 1:22 AM, Anne van Kesteren via webkit-dev 
>  wrote:
> 
> To reduce the overhead of switching between projects with different
> whitespace requirements, I would like to suggest we start being
> lenient when trailing whitespace is removed. In particular when a file
> is being changed to fix a bug.
> 
> I could see going even further and enforcing this via the style
> checker, if there is appetite for that.
> 
> Thanks for considering!
> ___
> 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] Unsigned to avoid negative values

[Chris Dumez via webkit-dev]

Hi,

What’s the benefit? I don’t think we should be changing our long-time coding 
practices unless there are clear benefits from doing so.
From your email, it is not yet clear to me what those benefits would be.

Chris.

> On Jan 24, 2023, at 6:58 AM, Michael Catanzaro via webkit-dev 
>  wrote:
> 
> On Tue, Jan 24 2023 at 02:00:11 AM -0800, Myles Maxfield via webkit-dev 
>  wrote:
>> What do you think?
> 
> This has been a best practice for a long time now. It's a good rule to reduce 
> bugs if adopted consistently, but I also fear that if we were to try to adapt 
> existing WebKit code to follow these guidelines, we might accidentally 
> introduce a lot of new bugs, especially regarding container types like 
> Vector. So I don't know what to think!
> 
> 
> ___
> 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] Style guide: enforce `while (true)` over `for (; ; )`

[Chris Dumez via webkit-dev]

Sounds good to me.Chris DumezOn Oct 5, 2022, at 9:04 PM, Kirsling, Ross via webkit-dev  wrote:




I've always kind of liked that `for (;;)` doesn't involve an explicit constant, but I too like consistency even more. :)

From: Ryosuke Niwa via webkit-dev 
Sent: Thursday, October 6, 2022 1:01:01 PM
To: Yusuke Suzuki ; Tim Nguyen ; WebKit Development 
Subject: Re: [webkit-dev] Style guide: enforce `while (true)` over `for (; ; )`
 

I do prefer
for (;;) because of less typing but if the existing code mostly uses
while (true) then we should go with it.


On Oct 5, 2022, at 8:58 PM, Yusuke Suzuki via webkit-dev  wrote:


+1

-Yusuke

On Oct 5, 2022, at 5:07 PM, Tim Nguyen via webkit-dev  wrote:

Hi everyone,

The WebKit codebase has an inconsistent mix of `while (true)` and `for (;;)`. Given 2/3 of the usages are `while (true)` and only 1/3 is `for (;;)` from code search, I would suggest enforcing `while (true)`. I also think it is generally more explicit and readable
 than `for (;;)`. If everyone agrees, I’ll enforce this via webkit-style, so we can end up in a consistent place.

What does everyone think?

Cheers,
Tim
___
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 listwebkit-dev@lists.webkit.orghttps://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] Proposal: Mandatory Commit and Merge Queue

[Chris Dumez via webkit-dev]

> On Jun 2, 2022, at 6:29 PM, Michael Catanzaro  wrote:
> 
> 
> 
> On Thu, Jun 2 2022 at 04:06:42 PM -0700, Chris Dumez via webkit-dev 
>  wrote:
>> I am concerned by this proposal given how slow EWS and the merge queue are 
>> these days.
> 
> Hopefully Jonathan's three proposed blockers will address this:
> 
>> - run-webkit-tests consulting results.webkit.org to avoid retrying known 
>> flakey or failing tests
>> - Another MergeQueue bot
>> - Xcode workspace builds to speed up incremental builds

It’s all speculative until we get actual merge queue times with those changes 
implemented. Implementing.a new restrictive policy based on speculative data 
sounds risky to me. Let’s not “hope”, let’s gather data.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Proposal: Mandatory Commit and Merge Queue

[Chris Dumez via webkit-dev]

I am concerned by this proposal given how slow EWS and the merge queue are 
these days. 

I think the issue is likely related to failures on the bots and the retries 
needed to determine if those failures are new.

We’ve never had this restriction before. Seems we are becoming overly strict 
with the move to GitHub and development speed will suffer as a result.

Chris Dumez

> On Jun 2, 2022, at 3:41 PM, Geoffrey Garen via webkit-dev 
>  wrote:
> 
> 
>> As we move to GitHub, I would like to propose we strengthen our protections 
>> on `main` by making MergeQueue and CommitQueue mandatory. 
> 
> 
> What is the goal of this proposal?
> 
> What problem are you trying to solve, and with what level of urgency?
> 
> Thanks,
> Geoff
> 
> 
>> On Jun 2, 2022, at 2:35 PM, Jonathan Bedard via webkit-dev 
>>  wrote:
>> 
>> As we move to GitHub, I would like to propose we strengthen our protections 
>> on `main` by making MergeQueue and CommitQueue mandatory. This would mean 
>> that with a few exceptions, all changes would need to be built and run 
>> layout tests before they are landed. To spell out what the exceptions I had 
>> in mind are:
>> 
>> - Revert commits, identified by a commit message that starts with 
>> “Unreviewed, revering…” would be exempt
>> - Changes which only modify files that do not effect building or testing 
>> WebKit would be exempt. These files specifically are:
>>  .github/
>>  JSTests/
>>  ManualTests/
>>  metadata/
>>  PerformanceTests/
>>  Tools/
>>   CISuport/
>>   EWSTools/
>>   WebKitBot
>> Websites/
>> - Emergency build and infrastructure fixes, identified by a commit message 
>> that starts with “Emergency build fix” or “Emergency infrastructure fix” 
>> would be exempt
>> - A reviewer who is not the commit author can overwrite this protection by 
>> adding `unsafe-merge-queue` instead of the commit author
>> - Changes which passed an EWS layout test queue within the last 7 days would 
>> skip the layout test check
>> 
>> These exceptions are designed to provide contributors for a way to by-pass 
>> potentially slow checks if extraordinary situations, or in ones where CI has 
>> already validated the change. I think we should keep the ability for any 
>> committer to deploy an emergency fix, because our project has many 
>> contributors in different timezones and with different holiday schedules.
>> 
>> We know that this policy change would potentially slow down development, so 
>> I think these 3 improvements block making MergeQueue and CommitQueue 
>> mandatory:
>> 
>> - run-webkit-tests consulting results.webkit.org to avoid retrying known 
>> flakey or failing tests
>> - Another MergeQueue bot
>> - Xcode workspace builds to speed up incremental builds
>> 
>> Jonathan Bedard
>> WebKit Continuous Integration
>> 
>> ___
>> 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] Add CODEOWNERS to WebKit

[Chris Dumez via webkit-dev]

I’m kind of ambivalent/neutral about this. One question though:

When adopting CODEOWNERS, will our existing watchlists get ported, or will each 
contributor have to modify CODEOWNERS themselves to match whatever was in the 
watchlists for them?

Thanks,
Chris.

> On Jun 2, 2022, at 1:12 PM, Jonathan Bedard via webkit-dev 
>  wrote:
> 
> Hey folks,
> 
> Yusuke is interested in adding the CODEOWNERS file to the WebKit project (see 
> https://github.com/WebKit/WebKit/pull/1137). There have been some objections 
> to this, the two ones I’m aware of:
> - WebKit doesn’t assign “ownership” to pieces of code, so the name is 
> unfortunate
> - The last match “wins”, so if you subscribed to a generic directory early in 
> the file, more specific matches would override that subscription
> 
> Despite these downsides, I think adding the CODEOWNERS file is good for the 
> project for a few reasons:
> - It’s a standard natively supported by GitHub, BitBucket and GitLab and will 
> be familiar to developers
> - File format is more simple than existing watchlist
> - Support for groups and individuals
> - No need for WebKit to host a service (other implementations of auto-CCing 
> would require this)
> 
> I intend to work with Yusuke to land 
> https://github.com/WebKit/WebKit/pull/1137 and start adopting CODEOWNERS on 
> Monday absent objections that folks think overshadow the benefits of the 
> CODEOWNERS file.
> 
> Jonathan
> ___
> 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] Proposal: Immediate Deprecation of ChangeLogs

[Chris Dumez via webkit-dev]

> On May 11, 2022, at 11:56 AM, Jonathan Bedard via webkit-dev 
>  wrote:
> 
> Trying to embed previous replies is going to get messy, will be referencing 
> those replies but not embedding them.
> 
> Unsafe-Merge-Queue should be very fast, I haven’t seen anything take longer 
> than 10 minutes from label application to landing or rejection. The average 
> case is 3-4 minutes. We’re aware of what the architectural problem with 
> Commit-Queue is that slows down the fast path, Unsafe-Merge-Queue has fixed 
> that. The solution we used isn’t transferable to Commit-Queue.

I personally don’t think that delaying a critical build fix / gardening by 10 
minutes is OK. It’s called “unsafe-merge-queue”, why does it take this long to 
commit? Why isn't it as fast as "generate a unique identifier and git push”?
Sure, I have no idea how the unsafe-merge-queue does but it is hard for me to 
imagine it should take more than 1 minute given what it needs to do and given 
that it should need to do extremely little testing given that it is “unsafe”.

4 to 10 minutes is nowhere near as fast as `git svn dcommit` and is therefore a 
pretty large regression.
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Proposal: Immediate Deprecation of ChangeLogs

[Chris Dumez via webkit-dev]

> On May 11, 2022, at 12:13 AM, Ryosuke Niwa via webkit-dev 
>  wrote:
> 
> On Tue, May 10, 2022 at 9:27 PM Ryosuke Niwa  wrote:
>> 
>> 
>> On Tue, May 10, 2022 at 20:36 Chris Dumez  wrote:
>>> 
>>> [Not sure why Apple Mail sent Ryosuke’s replies to the Junk folder but I 
>>> finally noticed.]
>> 
>> 
>> It's something to do with @webkit.org not being able to send a proper sender 
>> ID due to it being a forwarding address.
>> 
>>> On May 10, 2022, at 3:04 PM, Ryosuke Niwa via webkit-dev 
>>>  wrote:
>>> 
>>> On Tue, May 10, 2022 at 3:01 PM Jonathan Bedard via webkit-dev
>>>  wrote:
>>> 
>>> 
>>> On May 10, 2022, at 2:46 PM, Geoffrey Garen  wrote:
>>> 
>>> Do I undertand correctly that the proposal here is
>>> 
>>> (a) Immediately Deprecate ChangeLogs
>>> 
>>> 
>>> Yes
>>> 
>>> (b) Immediately end support for posting patches from Subversion 
>>> checkouts?
>>> 
>>> 
>>> We would be immediately ending support for _landing_ patches posted from a 
>>> Subversion checkout. EWS would continue to accept and test patches posted 
>>> from Subversion checkouts.
>>> 
>>> 
>>> Just this week, I landed 2~3 patches using a pure Subversion checkout.
>>> It's actually my primary method of landing patches in WebKit right
>>> now.
>>> 
>>> 
>>> Do you feel like 1 week is not enough time for you to do a git checkout and 
>>> familiarize yourself enough with GIT to upload patches? Is that the issue? 
>>> If so, how long do you feel would be reasonable?
>> 
>> 
>> I already have GitHub clones. The issue I have is with committing directly. 
>> I need to be able to commit directly as is since commit queue even fast one 
>> is simply way too slow.

I agree that the fast-cq is a lie and is way slower than committing manually 
(and usually not by a little). I haven’t tried the unsafe-merge-queue in GitHub 
yet but I hope it is much faster than fast-cq was. If unsafe-merge-queue is not 
nearly as fast as a manual commit then I think it needs to be fixed.
Given that the plan of record is that nobody has direct commit access to the 
GIT repo and the only way to land a build fix or test gardening will be via the 
merge queue, I think it is critical.

I’ll note though that IMO, there are some very specific cases for when 
extremely fast commit is required. Namely, build fixes and tests gardening, to 
get the tree / bots in a sane state as soon as possible and keep things running 
smoothly. For other kind of changes, I personally think they should go through 
rigorous EWS testing and merge queue.
That said, the speed of the commit queue and EWS has been going downhill and 
this is less and less practical IMO. The main reason for this seems to be that 
some tests are most of the time either plain failing or flaky on EWS, making 
processing much slower because the bot has to retry each patch. This can 
hopefully be addressed with much stricter / aggressive gardening / sheriffing.

>> 
>>> If you’re not ready to adopt the GitHub workflow for a reason or another, 
>>> git-svn / bugzilla patches is still a thing and will still work for now. 
>>> Only committing from pure SVN repositories would go away in a week.
>> 
>> 
>> Well, that's precisely my use case. I don't even write a patch in a pure 
>> Subversion checkout anymore these days.
>> 

Ok, seems like you are using GitHub checkouts for writing the patch and then 
separate Subversion checkout to commit the patch. I find it a bit surprising 
given that GitHub checkouts can still commit to SVN directly via git-svn 
(`git-webkit setup-git-svn` to set up as per 
https://github.com/WebKit/WebKit/wiki/Contributing#checking-out-WebKit).
Jonathan only mentioned that commits from pure SVN checkouts would be broken. 
He didn’t say anything about commits from git-svn checkouts so I assume those 
would still work (and should be more convenient for you?). @Jonathan, please 
correct me if I’m wrong.

>> - 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] Proposal: Immediate Deprecation of ChangeLogs

[Chris Dumez via webkit-dev]

[Not sure why Apple Mail sent Ryosuke’s replies to the Junk folder but I 
finally noticed.]

> On May 10, 2022, at 3:04 PM, Ryosuke Niwa via webkit-dev 
>  wrote:
> 
> On Tue, May 10, 2022 at 3:01 PM Jonathan Bedard via webkit-dev
>  wrote:
>> 
>>> On May 10, 2022, at 2:46 PM, Geoffrey Garen  wrote:
>>> 
>>> Do I undertand correctly that the proposal here is
>>> 
>>>  (a) Immediately Deprecate ChangeLogs
>> 
>> Yes
>> 
>>>  (b) Immediately end support for posting patches from Subversion 
>>> checkouts?
>> 
>> We would be immediately ending support for _landing_ patches posted from a 
>> Subversion checkout. EWS would continue to accept and test patches posted 
>> from Subversion checkouts.
> 
> Just this week, I landed 2~3 patches using a pure Subversion checkout.
> It's actually my primary method of landing patches in WebKit right
> now.

Do you feel like 1 week is not enough time for you to do a git checkout and 
familiarize yourself enough with GIT to upload patches? Is that the issue? If 
so, how long do you feel would be reasonable?

If you’re not ready to adopt the GitHub workflow for a reason or another, 
git-svn / bugzilla patches is still a thing and will still work for now. Only 
committing from pure SVN repositories would go away in a week.

> 
> - 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] Proposal: Immediate Deprecation of ChangeLogs

[Chris Dumez via webkit-dev]

Hi,

I think this is a step in the right direction.

I hope concerns from other contributors about change log reviews can be 
addressed in the near future. However, I don’t think it should prevent us from 
moving away from ChangeLog files, given that commenting on commit logs is still 
possible in GitHub, although not conveniently.

Thanks,

> On May 10, 2022, at 1:32 PM, Jonathan Bedard via webkit-dev 
>  wrote:
> 
> A few weeks ago, I started a discussion about deprecating ChangeLogs. In that 
> time, we’ve had more folks using the pull-request workflow and more folks 
> using newer versions of `git` which break automatic ChangeLog rebasing. I 
> propose that on Monday, May 16th, we implement the following policy changes 
> for the WebKit project:
> 
> - Commits no longer require ChangeLogs, they instead require commit messages
> - Commit messages are in the format of `prepare-ChangeLog --no-write`
> 
> Pull-request workflows based on `git-webkit` already support this workflow 
> well, and `git-webkit setup` creates a `prepare-commit-msg` hook that will 
> appropriately format commit messages. In addition, `git format-patch` allows 
> us to create a patch which contains a commit message. This means that 
> contributors still using patch workflows from a git or git-svn checkout will 
> be able to upload compliant patches to bugzilla.
> 
> This will, however, break contributors using pure-Subversion checkouts. This 
> is something that’s going to be happening in the very near future as we 
> deprecate Subversion entirely, so I think this is an acceptable cost in 
> exchange for fully supporting native git workflows.
> 
> The last thing I’d like to note is that a full git-native commit message 
> policy now is something we can modify in the future if we find that reviewing 
> commit messages with “Quote reply” comments is not sufficient, but resolving 
> project disagreements on how or if to address deficiencies in GitHub commit 
> message review don’t seem to be headed towards a resolution quickly.
> 
> Jonathan
> WebKit Continuous Integration
> 
> ___
> 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] ChangeLog Deprecation Plans

[Chris Dumez via webkit-dev]

I spent some time discussing this offline with Yusuke to better understand his 
proposal.

It sounds like Yusuke’s proposal is:
1. Have a separate COMMIT_MESSAGE file as part of the PR
2. When the merge queue commits the PR, it uses that COMMIT_MESSAGE content as 
commit log message and then drops the file from the commit.

I understand that this approach is a bit more flexible for people who like to 
make multiple commits locally for the same PR. I personally always have a 
single commit per PR that I keep amending but I think it is good to be flexible.
It also makes reviewing the changelog message a bit easier on GitHub.

I support Yusuke’s proposal. It is a bit more flexible and it still means that 
separate full/history ChangeLog files would be a thing of the past.

--
 Chris Dumez




> On Apr 22, 2022, at 2:10 PM, Chris Dumez via webkit-dev 
>  wrote:
> 
> I am strongly in favor of dropping the ChangeLog files and relying on the GIT 
> commit message instead. Not having to update ChangeLog files was actually a 
> big motivator for me to switch to GitHub, as I thought until now we didn’t 
> have to update ChangeLog files when switching Github PRs.
> 
> With the provided GIT commit hook, the changelog format is identical to what 
> we had in the ChangeLog files anyway. I don’t understand (yet) the need for 
> having the same message in two separate location.
> 
> Git commit logs don’t roll over (like ChangeLog files do), they are 
> searchable, they have the same format (you CAN add inline comments on a 
> per-file basis for e.g.). They are also easily modifiable from the GitHub 
> interface.
> 
> I will also note that ChangeLog files are a frequent source of conflicts when 
> using GIT. Yes, we do have a resolve-ChangeLogs script that’s supposed to 
> help with that. However, please note that this script hasn’t work reliably 
> for quite a while (at least for many of us at Apple with very recent SDKs).
> ChangeLog entries no longer show on top when rebasing, meaning I have to move 
> them back on top manually. Worse, if I fail to notice and use `webkit-patch 
> upload`, it will upload to the wrong bugzilla bug.
> 
> If people really still want to maintain separate ChangeLog files, I am hoping 
> this can be generated from my commit messages and done automatically for me 
> upon committing. I really don’t want that as part of my patch/PR.
> 
> --
>  Chris Dumez
> 
> 
> 
> 
>> On Apr 19, 2022, at 11:00 AM, Geoffrey Garen via webkit-dev 
>> mailto:webkit-dev@lists.webkit.org>> wrote:
>> 
>>>> Commit message is tied to a commit, so editing commit without breaking 
>>>> commit-message is hard (how to revert one change from one commit without 
>>>> rewriting commit message? It requires some git hack). Separate independent 
>>>> COMMIT_MESSAGE file can solve this problem and makes local development 
>>>> easy.
>>> 
>>> Developers who are used to git -- which nowadays is pretty much everyone 
>>> except WebKit devs -- are also used to rewriting commit messages.
>> 
>> I think it’s important for WebKit's git transition to take consideration of 
>> WebKit developers and WebKit workflows. I hope we can agree on this premise 
>> as we discuss various options for commit messages.
>> 
>> Thanks,
>> Geoff
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org <mailto: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] ChangeLog Deprecation Plans

[Chris Dumez via webkit-dev]

I am strongly in favor of dropping the ChangeLog files and relying on the GIT 
commit message instead. Not having to update ChangeLog files was actually a big 
motivator for me to switch to GitHub, as I thought until now we didn’t have to 
update ChangeLog files when switching Github PRs.

With the provided GIT commit hook, the changelog format is identical to what we 
had in the ChangeLog files anyway. I don’t understand (yet) the need for having 
the same message in two separate location.

Git commit logs don’t roll over (like ChangeLog files do), they are searchable, 
they have the same format (you CAN add inline comments on a per-file basis for 
e.g.). They are also easily modifiable from the GitHub interface.

I will also note that ChangeLog files are a frequent source of conflicts when 
using GIT. Yes, we do have a resolve-ChangeLogs script that’s supposed to help 
with that. However, please note that this script hasn’t work reliably for quite 
a while (at least for many of us at Apple with very recent SDKs).
ChangeLog entries no longer show on top when rebasing, meaning I have to move 
them back on top manually. Worse, if I fail to notice and use `webkit-patch 
upload`, it will upload to the wrong bugzilla bug.

If people really still want to maintain separate ChangeLog files, I am hoping 
this can be generated from my commit messages and done automatically for me 
upon committing. I really don’t want that as part of my patch/PR.

--
 Chris Dumez




> On Apr 19, 2022, at 11:00 AM, Geoffrey Garen via webkit-dev 
>  wrote:
> 
>>> Commit message is tied to a commit, so editing commit without breaking 
>>> commit-message is hard (how to revert one change from one commit without 
>>> rewriting commit message? It requires some git hack). Separate independent 
>>> COMMIT_MESSAGE file can solve this problem and makes local development easy.
>> 
>> Developers who are used to git -- which nowadays is pretty much everyone 
>> except WebKit devs -- are also used to rewriting commit messages.
> 
> I think it’s important for WebKit's git transition to take consideration of 
> WebKit developers and WebKit workflows. I hope we can agree on this premise 
> as we discuss various options for commit messages.
> 
> Thanks,
> Geoff
> ___
> 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] How to make changes to website?

[Chris Dumez via webkit-dev]

Jon Davis is probably the right contact for Webkit.org <http://webkit.org/>.

--
 Chris Dumez




> On Feb 16, 2022, at 2:01 PM, Michael Catanzaro via webkit-dev 
>  wrote:
> 
> Hi,
> 
> I want to make a modification to:
> 
> https://webkit.org/contributing-code/
> 
> Suggested here: https://bugs.webkit.org/show_bug.cgi?id=232935#c6
> 
> It looks like this page is not part of WebKit/Websites/webkit.org. Anyone 
> know who can edit it?
> 
> Michael
> 
> 
> ___
> 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] How to set up Intellisense-ish code completion/suggestions for editing WebKit sources on macOS?

[Chris Dumez via webkit-dev]

The code completion is excellent with Qt Creator.

You can import the WebKit project like so:
New Project > Import existing project > Choose the WebKit folder > click import 
button

Once imported, no need to fiddle with any settings, it just works (after an 
initial project scan that may take a couple of minutes).

--
 Chris Dumez




> On Nov 11, 2021, at 7:49 AM, Patrick Griffis via webkit-dev 
>  wrote:
> 
> On 2021-11-11 02:48, Michael[tm] Smith via webkit-dev wrote:
>> Can anyone recommend a combination of text-editor/IDE, plugins/tooling
>> (e.g., language server), and settings/config that’ll enable me to have
>> usable code-(auto)completion/suggestions (like Intellisense, etc.) when
>> editing WebKit sources in a macOS environment?
>> 
>> Anyway, the last thing I’ve been trying is Visual Studio Code. In that I
>> tried with both the clangd extension and the ccls extension, but ended up
>> having basically the same problems I have with the vim integrations.
>> 
>> So I switched back to trying the Microsoft-provided C/C++ Intellisense
>> extension (cpptools), and found that seems to work better than the clangd
>> or ccls extensions — at least so far as it seems to be able to at last
>> partially resolve the header include references. But then it too seems to
>> stumble on not being able to find some headers it needs.
>> 
>> For example, I think I’ve been able to make it figure out #include 
>> "config.h" —
>> but then the next problem I hit is stuff like this:
>> 
>>  cannot open source file "JavaScriptCore/JSExportMacros.h"
>> (dependency of "config.h")
>> 
>> ...And then anyway, again, ultimately, no completion suggests when I put a 
>> dot
>> after a particular object name I want to get the function and data-member
>> suggestions for. (It seems to work for some objects, but not others.)
>> 
>> So I’m hoping others here might have something working successfully in
>> their environments that gives them proper completion suggestions on member-
>> function names and data-member names.
>> 
>>  –Mike
> 
> This may be of limited use to you because I am on Linux but may be
> useful information in general. It works here using Visual Studio Code
> with the Cmake Tools and ccls extensions installed.
> 
> Header detection works, references work (including basic refactoring),
> intellisense works (not super fast but functional), debugging with GDB
> works (connecting to gdbserver with Microsoft's C++ extension for that).
> 
> I had the exact same header issues as you with only Microsofts C++
> extension. I have done no configuration to ccls.
> ___
> 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] Removal of makeRef() / makeRefPtr()

[Chris Dumez via webkit-dev]

Hi,

With WebKit adopting C++17 a while back, there are no longer any benefits to 
using makeRef() / makeRefPtr() as far as I can tell.

Code that was written like so:
auto protectedThis = makeRef(*this);
auto protectedPage = makeRefPtr(page);
auto result = makeRef(foo->bar());
auto lambda = [protectedThis = makeRef(*this)] { };
m_node = makeRef(node); // m_node being a Ref
m_node = makeRefPtr(node); // m_node being a RefPtr

Can now be written in a more concise way:
Ref protectedThis { *this };
RefPtr protectedPage { page };
Ref result = foo->bar();
auto lambda = [protectedThis = Ref { *this }] { };
m_node = node;
`m_node = node;` or `m_node =  // depending if node is a reference or 
pointer.

I am currently in the process on transitioning our code from one style to the 
other and removing makeRef() / makeRefPtr() altogether.
I am sending this email so people are not confused when they try to use 
makeRef() / makeRefPtr() in new code.

--
 Chris Dumez




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

Re: [webkit-dev] Request for position: Cryptographically secure random UUIDs

[Chris Dumez via webkit-dev]

I forgot to follow-up on this email, sorry.

Support for this landed via https://bugs.webkit.org/show_bug.cgi?id=229240 
<https://bugs.webkit.org/show_bug.cgi?id=229240>.

--
 Chris Dumez




> On Apr 10, 2021, at 7:00 PM, Benjamin Coe via webkit-dev 
>  wrote:
> 
> Hello WebKit folks,
> 
> This is a request for WebKit's position on introducing the JavaScript method 
> randomUUID(), for generating RFC 4122 version 4 identifiers, to the crypto 
> interface.
> 
> - Explainer: https://github.com/WICG/uuid/blob/gh-pages/explainer.md 
> <https://github.com/WICG/uuid/blob/gh-pages/explainer.md>
> - Specification: https://wicg.github.io/uuid/ <https://wicg.github.io/uuid/>
> - ChromeStatus: https://www.chromestatus.com/feature/5689159362543616 
> <https://www.chromestatus.com/feature/5689159362543616>
> 
> Look forward to feedback,
> 
> -- Ben.
> 
> ___
> 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] Request for position on self.reportError()

[Chris Dumez via webkit-dev]

It doesn’t seem like a controversial feature to me and it is indeed a fairly 
small amount of work to support.

I am working on it via Bug 228316 
<https://bugs.webkit.org/show_bug.cgi?id=228316>.

--
 Chris Dumez




> On Aug 27, 2021, at 1:58 PM, Domenic Denicola via webkit-dev 
>  wrote:
> 
> Hi webkit-dev,
> 
> We recently added a small utility function, self.reportError(), to the HTML 
> Standard [1]. It is pretty simple and just lets developers appropriately send 
> errors to the "error" event handler and the console, like what happens when 
> the browser reports uncaught exceptions.
> 
> This is already implemented in Firefox and we're looking to ship it in Chrome 
> soon. Would you all be interested this feature as well? It should be pretty 
> simple to implement; it was for us at least. [2]
> 
> Thanks,
> -Domenic
> 
> [1]: https://github.com/whatwg/html/pull/1196
> 
> [2]: https://chromium-review.googlesource.com/c/chromium/src/+/3125854
> ___
> 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] Moving from WTF::Optional to std::optional

[Chris Dumez via webkit-dev]

Hi,

Another thing Darin didn’t mention but I think people should be careful about:
The move constructor for std::optional does not clear the is-set flag (while 
the one for WTF::Optional did).

As a result, you will be having a very bad time if you do a use-after-move of a 
std::optional. Please make sure to use std::exchange() instead of WTFMove() if 
you want to leave to std::optional in a clean state for reuse later.

Chris Dumez

> On Jun 1, 2021, at 8:54 PM, Darin Adler via webkit-dev 
>  wrote:
> 
> Hi folks.
> 
> We’re getting rid of the WTF::Optional class template, because, hooray, 
> std::optional is supported quite well by all the C++17 compilers we use, and 
> we don’t have to keep using our own special version. Generally we don’t want 
> to reimplement the C++ standard library when there is not a significant 
> benefit, and this is one of those times.
> 
> Here are a few considerations:
> 
> 1) Since https://commits.webkit.org/238290@main, if you use Optional<> by 
> mistake instead of std::optional<>, your code won’t compile. (Unless you are 
> writing code for ANGLE, which has its own separate Optional<>.)
> 
> 2) If you want to use std::optional, include the C++ standard header, 
> , or something that includes it. In a lot of cases, adding an 
> include will not be required since it’s included by widely-used headers like 
> WTFString.h and Vector.h, so if you include one of those are covered. Another 
> way to think about this is that if your base class already uses 
> std::optional, then you don’t need to include it.
> 
> 3) Once the patch in https://bugs.webkit.org/show_bug.cgi?id=226437 lands, 
> includes of  won’t forward declare optional, and includes of 
>  won’t do anything at all.
> 
> — Darin
> ___
> 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] Clang Thread Safety Analysis

[Chris Dumez via webkit-dev]

Hi,

Just a quick follow-up to let you know that both CheckedLock/CheckedCondition 
and UncheckedLock/UncheckedCondition have been removed from the tree.
The whole codebase has been ported to Lock/Condition, which have the thread 
safety analysis annotations.

--
 Chris Dumez




> On May 23, 2021, at 10:40 PM, Chris Dumez  wrote:
> 
> Clang Thread Safety Analysis
> WTF::Lock now supports Clang Thread Safety Analysis 
> <https://clang.llvm.org/docs/ThreadSafetyAnalysis.html>. It is a C++ language 
> extension which warns about potential race conditions in code, at compile 
> time. It is the same great Lock, but with some extra help from clang to make 
> our multi-threaded code safer by giving out compilation errors when thread 
> unsafe code is detected.
> 
> Annotations
> Just by using WTF::Lock, Clang will already be able to do some basic 
> validation. However, to take full advantage of it, there are a few 
> annotations you should know about. Note that you'll see those annotations 
> throughout the code base already as a few of us have been busy adopting them. 
> All these annotations are declared in wtf/ThreadSafetyAnalysis.h 
> <http://trac.webkit.org/browser/webkit/trunk/Source/WTF/wtf/ThreadSafetyAnalysis.h>.
> 
> WTF_GUARDED_BY_LOCK()
> 
> WTF_GUARDED_BY_LOCK is an attribute on data members, which declares that the 
> data member is protected by the given lock. Read operations on the data 
> require shared access, while write operations require exclusive access.
> 
> WTF_POINTEE_GUARDED_BY_LOCK is similar, but is intended for use on pointers 
> and smart pointers. There is no constraint on the data member itself, but the 
> data that it points to is protected by the given lock.
> 
> class Foo {
> Vector m_strings WTF_GUARDED_BY_LOCK(m_stringsLock);
> Lock m_stringsLock;
> };
> You will get a compiler error if you try to access or modify m_strings 
> without grabbing the m_stringsLock lock first.
> 
> WTF_REQUIRES_LOCK()
> 
> WTF_REQUIRES_LOCK is an attribute on functions or methods, which declares 
> that the calling thread must have exclusive access to the given locks. More 
> than one lock may be specified. The locks must be held on entry to the 
> function, and must still be held on exit.
> 
> class Foo {
> void addString(String&&) WTF_REQUIRES_LOCK(m_stringsLock);
> 
> Vector m_strings WTF_GUARDED_BY_LOCK(m_stringsLock);
> Lock m_stringsLock;
> };
> You will get a compiler error if you try to call addString() without grabbing 
> the m_stringsLock lock first. This also allows to compiler to know that the 
> addString() implementation can modify the m_strings data member without 
> grabbing the lock first.
> 
> Another good use case:
> 
> static Lock globalCacheLock;
> static HashMap& globalCache() 
> WTF_REQUIRES_LOCK(globalCacheLock)
> {
> static NeverDestroyed> globalCache;
> return globalCache;
> }
> This will force all call sites to grab the globalCacheLock lock before 
> calling globalCache().
> 
> Previously, we may have passed a LockHolder& as parameter to try and convey 
> that. We have been updating code to stop passing such parameters though as it 
> is no longer useful.
> 
> WTF_ACQUIRES_LOCK() / WTF_RELEASES_LOCK()
> 
> WTF_ACQUIRES_LOCK is an attribute on functions or methods declaring that the 
> function acquires a lock, but does not release it. The given lock must not be 
> held on entry, and will be held on exit.
> 
> WTF_RELEASES_LOCK declares that the function releases the given lock. The 
> lock must be held on entry, and will no longer be held on exit.
> 
> class Foo {
> public:
> void suspend() WTF_ACQUIRES_LOCK(m_suspensionLock)
> {
> m_suspensionLock.lock();
> m_isSuspended = true;
> }
> void resume() WTF_RELEASE_LOCK(m_suspensionLock)
> {
> m_isSuspended = false;
> m_suspensionLock.unlock();
> }
> 
> private:
> Lock m_suspensionLock;
> bool m_isSuspended;
> };
> Without these annotations, you'd get a compiler error stating that you failed 
> to unlock m_suspensionLock before returning in suspend() and that you 
> unlocked m_suspensionLock in resume() even though it was not previously 
> locked.
> 
> WTF_RETURNS_LOCK()
> 
> WTF_RETURNS_LOCK is an attribute on functions or methods, which declares that 
> the function returns a reference to the given lock.
> 
> class Foo {
> public:
> Lock& lock() WTF_RETURNS_LOCK(m_lock) { return m_lock; }
> private:
> Lock m_lock;
> };
> WTF_EXCLUDES_LOCK()
> 
> WTF_EXCLUDES_LOCK is an attribute on functions or methods, which declares 
> that the caller

[webkit-dev] Clang Thread Safety Analysis

[Chris Dumez via webkit-dev]

ify why it is OK as is.
We will likely try and make the Locker more flexible to support more use 
cases. We are a bit limited by what clang supports but I believe there is room 
of improvement.
Credits
Credits to Kimmo for introducing the checked Lock to WebKit and helping with 
the adoption!
Big thanks to everyone who helped with reviews and feedback as well.

--
 Chris Dumez




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

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

[Chris Dumez]

> 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  
 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 guess this is also consistent with the way people write objective C blocks: 
> https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/WorkingwithBlocks/WorkingwithBlocks.html
> 
> For JavaScript, this rule probably doesn’t apply because arrow function and 
> regular anonymous function both require ().
> 
> - 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

[Chris Dumez]

I mildly prefer without the space :)

--
 Chris Dumez




> On Nov 1, 2019, at 11:19 AM, Ryosuke Niwa  wrote:
> 
> Hi all,
> 
> There seems to be inconsistency in our coding style with regards to spacing 
> in lambdas.
> 
> Namely, some people write a lambda as:
> auto x = [] () { }
> 
> with a space between [] and () while others would write it as:
> 
> auto x = []() { }
> 
> without a space between the two. I'd like to require either style in our 
> guideline so that things are consistent in our codebase. Which one should we 
> use?
> 
> FWIW, I mildly prefer having a space between [] and ().
> 
> - 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] 2019 WebKit Contributors Meeting - Registration is Open

[Chris Dumez]

Did not work for me either, got a page with text “None” shortly after logging 
in.

--
 Chris Dumez




> On Sep 18, 2019, at 2:12 PM,  
>  wrote:
> 
> Seems like the login form isn't working—I see:
> 
>   MultipleObjectsReturned at /auth/wlogin/
>   get() returned more than one MyUser -- it returned 2!
> 
> Perhaps I just need to be more patient? :)
> 
> Thanks,
> Ross
> 
> On 9/18/19, 1:49 PM, "webkit-dev on behalf of Jonathan Davis" 
>  wrote:
> 
>Hello WebKit Contributors,
> 
>You are invited to attend the annual WebKit Contributors Meeting to be 
> held at the Hilton Santa Clara hotel on Friday, November 1st, 2019 from 8 AM 
> to 6 PM Pacific.
> 
>Breakfast and sign-in will begin at 8 AM. Presentations will run between 9 
> AM to 6 PM. Lunch will be provided with all day coffee, tea and a 
> mid-afternoon snack break. A reception is planned at the venue after the 
> meeting with dinner and drinks from 6 PM to 9 PM.
> 
>To attend you must be an active WebKit contributor. The meeting will be 
> free of charge, and registration is open. Register at 
> https://webkit.org/meeting/ 
> 
>The event format will include a mix of prepared talks around 40 minutes 
> long with 10-15 minutes of questions, and spontaneous lightning talks about 
> 5-10 minutes long. If you have a topic idea that you want to present, please 
> email me directly.
> 
>We look forward to seeing you there!
> 
>Thank you,
>Jon Davis
>___
>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] IPC implementation help in haiku's webkit port

[Chris Dumez]

Alex is likely right that you are ending up with some kind of IPC deadlock 
since the IPC is synchronous.

But also,

> On May 27, 2019, at 11:49 PM, Rajagopalan Gangadharan  
> wrote:
> 
> 
> Please check the source references for more info:
> NetworkProcessProxy: [ 
> https://github.com/RAJAGOPALAN-GANGADHARAN/webkit/blob/6bf81d79669be06b4efd9d8ced4139cbe07216b2/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp#L150
>  
> ]

You’re passing 0 instead of the webPID with the IPC here.___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Crash When Running run-safari

[Chris Dumez]

Bug 196777 <https://bugs.webkit.org/show_bug.cgi?id=196777> will fix this.

--
 Chris Dumez




> On Apr 10, 2019, at 10:54 AM, Chris Dumez  wrote:
> 
> Yes, this commit broke it: http://trac.webkit.org/r243320 
> <http://trac.webkit.org/r243320>
> 
> --
>  Chris Dumez
> 
> 
> 
> 
>> On Apr 10, 2019, at 10:47 AM, Assaf Inbal > <mailto:shmuel...@gmail.com>> wrote:
>> 
>> Thanks for the update!
>> Since I don’t really need the bleeding-edge, can you tell me which 
>> tag/commit of WebKit I can use which should be compatible?
>> 
>> Thanks in advance!
>> 
>> On 10 Apr 2019, at 20:36, Chris Dumez > <mailto:cdu...@apple.com>> wrote:
>> 
>>> Oh, I am responsible for this. This is due to an incompatibility between 
>>> the very recent WebKit you built and the older system’s Safari.
>>> I will look into fixing this shortly, thanks for letting me know.
>>> 
>>> --
>>>  Chris Dumez
>>> 
>>> 
>>> 
>>> 
>>>> On Apr 10, 2019, at 4:44 AM, Assaf Inbal >>> <mailto:shmuel...@gmail.com>> wrote:
>>>> 
>>>> Hey,
>>>> 
>>>> I’m trying to build and run (top-of-branch) WebKit to debug some weird 
>>>> behaviour and am running into some issues.
>>>> I compiled everything on macOS with `./Tools/Scripts/build-webkit --debug` 
>>>> which seemed to work just find but when I go ahead and run Safari via 
>>>> `./Tools/Scripts/run-safari --debug` I get an exception and the program 
>>>> exits:
>>>> 2019-04-10 14:34:38.256 SafariForWebKitDevelopment[644:9602] *** 
>>>> Terminating app due to uncaught exception 'NSInvalidArgumentException', 
>>>> reason: 'Related web view  has data store 
>>>>  but configuration specifies a 
>>>> different data store '
>>>> Not sure what this means exactly and couldn’t find anything similar online.
>>>> 
>>>> Any help would be appreciated.
>>>> 
>>>> Thanks!
>>>> 
>>>> ___
>>>> 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

Re: [webkit-dev] Crash When Running run-safari

[Chris Dumez]

Yes, this commit broke it: http://trac.webkit.org/r243320 
<http://trac.webkit.org/r243320>

--
 Chris Dumez




> On Apr 10, 2019, at 10:47 AM, Assaf Inbal  wrote:
> 
> Thanks for the update!
> Since I don’t really need the bleeding-edge, can you tell me which tag/commit 
> of WebKit I can use which should be compatible?
> 
> Thanks in advance!
> 
> On 10 Apr 2019, at 20:36, Chris Dumez  <mailto:cdu...@apple.com>> wrote:
> 
>> Oh, I am responsible for this. This is due to an incompatibility between the 
>> very recent WebKit you built and the older system’s Safari.
>> I will look into fixing this shortly, thanks for letting me know.
>> 
>> --
>>  Chris Dumez
>> 
>> 
>> 
>> 
>>> On Apr 10, 2019, at 4:44 AM, Assaf Inbal >> <mailto:shmuel...@gmail.com>> wrote:
>>> 
>>> Hey,
>>> 
>>> I’m trying to build and run (top-of-branch) WebKit to debug some weird 
>>> behaviour and am running into some issues.
>>> I compiled everything on macOS with `./Tools/Scripts/build-webkit --debug` 
>>> which seemed to work just find but when I go ahead and run Safari via 
>>> `./Tools/Scripts/run-safari --debug` I get an exception and the program 
>>> exits:
>>> 2019-04-10 14:34:38.256 SafariForWebKitDevelopment[644:9602] *** 
>>> Terminating app due to uncaught exception 'NSInvalidArgumentException', 
>>> reason: 'Related web view  has data store 
>>>  but configuration specifies a 
>>> different data store '
>>> Not sure what this means exactly and couldn’t find anything similar online.
>>> 
>>> Any help would be appreciated.
>>> 
>>> Thanks!
>>> 
>>> ___
>>> 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

Re: [webkit-dev] Crash When Running run-safari

[Chris Dumez]

Oh, I am responsible for this. This is due to an incompatibility between the 
very recent WebKit you built and the older system’s Safari.
I will look into fixing this shortly, thanks for letting me know.

--
 Chris Dumez




> On Apr 10, 2019, at 4:44 AM, Assaf Inbal  wrote:
> 
> Hey,
> 
> I’m trying to build and run (top-of-branch) WebKit to debug some weird 
> behaviour and am running into some issues.
> I compiled everything on macOS with `./Tools/Scripts/build-webkit --debug` 
> which seemed to work just find but when I go ahead and run Safari via 
> `./Tools/Scripts/run-safari --debug` I get an exception and the program exits:
> 2019-04-10 14:34:38.256 SafariForWebKitDevelopment[644:9602] *** Terminating 
> app due to uncaught exception 'NSInvalidArgumentException', reason: 'Related 
> web view  has data store  0x7fbc872856e0> but configuration specifies a different data store 
> '
> Not sure what this means exactly and couldn’t find anything similar online.
> 
> Any help would be appreciated.
> 
> Thanks!
> 
> ___
> 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] Code Style: Opinion on returning void

[Chris Dumez]

> On Feb 20, 2019, at 1:14 PM, Maciej Stachowiak  wrote:
> 
> 
> It seems like `return foo();` where foo() is a void function can always be 
> replaced with `foo(); return;` for greater clarity at the cost of one extra 
> line break. For people who prefer the one-line style, can you say why you 
> don’t like the other way?

We do not allow more than one statement per line so it would be:
foo();
return;

Also, since we favor early returns in WebKit, things like:
If (!nok)
return completionHandler(Failed);

Would become:
If (!nok) {
completionHandler(Failed);
return;
}

It is not a big deal but I personally prefer the most concise version. 
Especially, it is not uncommon to have multiple early returns.
I think more concise is better and I personally do not see a readability issue 
here. It does not really matter what the completion handler is returning.

> 
>  - Maciej
> 
>> On Feb 20, 2019, at 10:33 AM, Simon Fraser > <mailto:simon.fra...@apple.com>> wrote:
>> 
>> I find it mind bending. It makes me wonder if the author made a coding error.
>> 
>> Simon
>> 
>>> On Feb 20, 2019, at 7:48 AM, Daniel Bates >> <mailto:dba...@webkit.org>> wrote:
>>> 
>>> Thanks for the opinion!
>>> 
>>> Dan
>>> 
>>> On Feb 20, 2019, at 7:26 AM, Saam Barati >> <mailto:sbar...@apple.com>> wrote:
>>> 
>>>> I prefer it as well.
>>>> 
>>>> - Saam
>>>> 
>>>> On Feb 20, 2019, at 6:58 AM, Chris Dumez >>> <mailto:cdu...@apple.com>> wrote:
>>>> 
>>>>> I also prefer allowed returning void. 
>>>>> 
>>>>> Chris Dumez
>>>>> 
>>>>> On Feb 19, 2019, at 10:35 PM, Daniel Bates >>>> <mailto:dba...@webkit.org>> wrote:
>>>>> 
>>>>>> 
>>>>>> 
>>>>>> On Feb 19, 2019, at 9:42 PM, Ryosuke Niwa >>>>> <mailto:rn...@webkit.org>> wrote:
>>>>>> 
>>>>>>> On Tue, Feb 19, 2019 at 8:59 PM Daniel Bates >>>>>> <mailto:dba...@webkit.org>> wrote:
>>>>>>> > On Feb 7, 2019, at 12:47 PM, Daniel Bates >>>>>> > <mailto:dba...@webkit.org>> wrote:
>>>>>>> >
>>>>>>> > Hi all,
>>>>>>> >
>>>>>>> > Something bothers me about code like:
>>>>>>> >
>>>>>>> > void f();
>>>>>>> > void g()
>>>>>>> > {
>>>>>>> > if (...)
>>>>>>> > return f();
>>>>>>> > return f();
>>>>>>> > }
>>>>>>> >
>>>>>>> > I prefer:
>>>>>>> >
>>>>>>> > void g()
>>>>>>> > {
>>>>>>> > if (...) {
>>>>>>> > f();
>>>>>>> > return
>>>>>>> > }
>>>>>>> > f();
>>>>>>> > }
>>>>>>> >
>>>>>>> Based on the responses it seems there is sufficient leaning to codify
>>>>>>> the latter style.
>>>>>>> 
>>>>>>> I don't think there is a sufficient consensus as far as I can tell. 
>>>>>>> Geoff
>>>>>> 
>>>>>> I didn't get this from Geoff's remark. Geoff wrote:
>>>>>> 
>>>>>> ***“return f()” when f returns void is a bit mind bending.***
>>>>>> Don't want to put words in Geoff's mouth. So, Geoff can you please 
>>>>>> confirm: for the former style, for the latter style, no strong opinion.
>>>>>> 
>>>>>>> and Alex both expressed preferences for being able to return void,
>>>>>> 
>>>>>> I got this from Alex's message
>>>>>> 
>>>>>>> and Saam pointed out that there is a lot of existing code which does 
>>>>>>> this.
>>>>>> 
>>>>>> I did not get this. He wrote emphasis mine:
>>>>>> 
>>>>>> I've definitely done this in JSC. ***I don't think it's super common***, 
>>>>>> but I've also seen code in JSC not written by me that also does this.
>>>>>> 
>>>>>>> Zalan also said he does th

Re: [webkit-dev] Code Style: Opinion on returning void

[Chris Dumez]

> On Feb 20, 2019, at 7:48 AM, Daniel Bates  wrote:
> 
> Okay, you’ve changed your mind from your earlier email of not having a strong 
> opinion. Would have been good to know from the get-go. Better late than never 
> knowing :/

I did not change my mind. I said I was using this pattern in my code. So did 
other people. If we use it in our code, it is because we prefer it.
What I meant to say is that if a majority of people felt strongly that we 
should not allow this pattern, then I would not stand in the way.

I don’t think this mail thread showed that people strongly feel that we should 
not allow this pattern. Therefore, I was also surprised by your email saying 
that we’d reached a consensus.

> 
> Dan
> 
> On Feb 20, 2019, at 6:58 AM, Chris Dumez  <mailto:cdu...@apple.com>> wrote:
> 
>> I also prefer allowed returning void. 
>> 
>> Chris Dumez
>> 
>> On Feb 19, 2019, at 10:35 PM, Daniel Bates > <mailto:dba...@webkit.org>> wrote:
>> 
>>> 
>>> 
>>> On Feb 19, 2019, at 9:42 PM, Ryosuke Niwa >> <mailto:rn...@webkit.org>> wrote:
>>> 
>>>> On Tue, Feb 19, 2019 at 8:59 PM Daniel Bates >>> <mailto:dba...@webkit.org>> wrote:
>>>> > On Feb 7, 2019, at 12:47 PM, Daniel Bates >>> > <mailto:dba...@webkit.org>> wrote:
>>>> >
>>>> > Hi all,
>>>> >
>>>> > Something bothers me about code like:
>>>> >
>>>> > void f();
>>>> > void g()
>>>> > {
>>>> > if (...)
>>>> > return f();
>>>> > return f();
>>>> > }
>>>> >
>>>> > I prefer:
>>>> >
>>>> > void g()
>>>> > {
>>>> > if (...) {
>>>> > f();
>>>> > return
>>>> > }
>>>> > f();
>>>> > }
>>>> >
>>>> Based on the responses it seems there is sufficient leaning to codify
>>>> the latter style.
>>>> 
>>>> I don't think there is a sufficient consensus as far as I can tell. Geoff
>>> 
>>> I didn't get this from Geoff's remark. Geoff wrote:
>>> 
>>> ***“return f()” when f returns void is a bit mind bending.***
>>> Don't want to put words in Geoff's mouth. So, Geoff can you please confirm: 
>>> for the former style, for the latter style, no strong opinion.
>>> 
>>>> and Alex both expressed preferences for being able to return void,
>>> 
>>> I got this from Alex's message
>>> 
>>>> and Saam pointed out that there is a lot of existing code which does this.
>>> 
>>> I did not get this. He wrote emphasis mine:
>>> 
>>> I've definitely done this in JSC. ***I don't think it's super common***, 
>>> but I've also seen code in JSC not written by me that also does this.
>>> 
>>>> Zalan also said he does this in his layout code.
>>> 
>>> I did not get this, quoting, emphasis mine:
>>> 
>>> I use this idiom too in the layout code. I guess I just prefer a more
>>> compact code.
>>> ***(I don't feel too strongly about it though)***
>>> 
>>> By the way, you even acknowledged that "WebKit ... tend[s] to have a 
>>> separate return.". So, I inferred you were okay with it. But from this 
>>> email I am no longer sure what your position is. Please state it clearly.
>>> 
>>>> - R. Niwa
>>>> 
>>> ___
>>> 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

Re: [webkit-dev] Code Style: Opinion on returning void

[Chris Dumez]

I also prefer allowed returning void. 

Chris Dumez

> On Feb 19, 2019, at 10:35 PM, Daniel Bates  wrote:
> 
> 
> 
>> On Feb 19, 2019, at 9:42 PM, Ryosuke Niwa  wrote:
>> 
>>> On Tue, Feb 19, 2019 at 8:59 PM Daniel Bates  wrote:
>> 
>>> > On Feb 7, 2019, at 12:47 PM, Daniel Bates  wrote:
>>> >
>>> > Hi all,
>>> >
>>> > Something bothers me about code like:
>>> >
>>> > void f();
>>> > void g()
>>> > {
>>> > if (...)
>>> > return f();
>>> > return f();
>>> > }
>>> >
>>> > I prefer:
>>> >
>>> > void g()
>>> > {
>>> > if (...) {
>>> > f();
>>> > return
>>> > }
>>> > f();
>>> > }
>>> >
>>> Based on the responses it seems there is sufficient leaning to codify
>>> the latter style.
>> 
>> I don't think there is a sufficient consensus as far as I can tell. Geoff
> 
> I didn't get this from Geoff's remark. Geoff wrote:
> 
> ***“return f()” when f returns void is a bit mind bending.***
> Don't want to put words in Geoff's mouth. So, Geoff can you please confirm: 
> for the former style, for the latter style, no strong opinion.
> 
>> and Alex both expressed preferences for being able to return void,
> 
> I got this from Alex's message
> 
>> and Saam pointed out that there is a lot of existing code which does this.
> 
> I did not get this. He wrote emphasis mine:
> 
> I've definitely done this in JSC. ***I don't think it's super common***, but 
> I've also seen code in JSC not written by me that also does this.
> 
>> Zalan also said he does this in his layout code.
> 
> I did not get this, quoting, emphasis mine:
> 
> I use this idiom too in the layout code. I guess I just prefer a more
> compact code.
> ***(I don't feel too strongly about it though)***
> 
> By the way, you even acknowledged that "WebKit ... tend[s] to have a separate 
> return.". So, I inferred you were okay with it. But from this email I am no 
> longer sure what your position is. Please state it clearly.
> 
>> - 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] Code Style: Opinion on returning void

[Chris Dumez]

Same here, I used it in PSON code with completion handlers. I liked the more 
concise code but I also do not feel strongly about it.

The extra return line often would have meant adding curly brackets for if 
statements leading to early returns.

Chris Dumez

> On Feb 7, 2019, at 8:23 PM, Zalan Bujtas  wrote:
> 
> I use this idiom too in the layout code. I guess I just prefer a more compact 
> code.
> (I don't feel too strongly about it though)
> 
> Alan.
> 
> 
>> On Thu, Feb 7, 2019 at 7:31 PM Alex Christensen  
>> wrote:
>> If you search for “return completionHandler” in WebKit you will find over a 
>> hundred instances.  Most if not all of them return void.  It means call the 
>> completion handler and return.  I probably wrote most of them and obviously 
>> think it’s a fabulous idiom.
>> 
>> > On Feb 7, 2019, at 2:32 PM, Geoffrey Garen  wrote:
>> > 
>> > FWIW, I’ve always felt conflicted about this case.
>> > 
>> > I very much prefer early return to if/else chains.
>> > 
>> > However, “return f()” when f returns void is a bit mind bending.
>> > 
>> > So, I can’t use my preferred style in functions that return void. Boo. 
>> > 
>> > Geoff
>> > 
>> >> On Feb 7, 2019, at 12:47 PM, Daniel Bates  wrote:
>> >> 
>> >> Hi all,
>> >> 
>> >> Something bothers me about code like:
>> >> 
>> >> void f();
>> >> void g()
>> >> {
>> >>if (...)
>> >>return f();
>> >>return f();
>> >> }
>> >> 
>> >> I prefer:
>> >> 
>> >> void g()
>> >> {
>> >>if (...) {
>> >>f();
>> >>return
>> >>}
>> >>f();
>> >> }
>> >> 
>> >> Does it bother you? For the former? For the latter? Update our style 
>> >> guide?
>> >> 
>> >> Opinions, please.
>> >> 
>> >> Dan
>> >> ___
>> >> 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
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Rename AtomicString to AtomString

[Chris Dumez]

Fine by me.

--
 Chris Dumez




> On Jan 30, 2019, at 9:35 AM, Antti Koivisto  wrote:
> 
> Sounds good to me.
> 
> 
>   antti
> 
> On Wed, Jan 30, 2019 at 6:33 PM Darin Adler  <mailto:da...@apple.com>> wrote:
> So, did we reach consensus that we should rename AtomicString to AtomString?
> 
> — Darin
> ___
> 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

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

Re: [webkit-dev] the name "AtomicString"

[Chris Dumez]

> On Dec 19, 2018, at 9:41 PM, Chris Dumez  wrote:
> 
>> 
>> On Dec 19, 2018, at 9:17 PM, Maciej Stachowiak > <mailto:m...@apple.com>> wrote:
>> 
>> 
>> 
>>> On Dec 19, 2018, at 8:06 PM, Ryosuke Niwa >> <mailto:rn...@webkit.org>> wrote:
>>> 
>>> On Wed, Dec 19, 2018 at 1:13 PM Simon Fraser >> <mailto:simon.fra...@apple.com>> wrote:
>>> > On Dec 19, 2018, at 12:33 PM, Michael Catanzaro >> > <mailto:mcatanz...@igalia.com>> wrote:
>>> > 
>>> > On Tue, Dec 18, 2018 at 9:31 PM, Darin Adler >> > <mailto:da...@apple.com>> wrote:
>>> >> I’ve gotten used to the name AtomicString over the years, but I wouldn’t 
>>> >> strongly object to changing it if other programmers are often confused 
>>> >> by it’s similarity to the term “atomic operations”.
>>> > 
>>> > Well there were two other developers in the thread Ryosuke linked to who 
>>> > made the exact same mistake as me, so I do think the current name is 
>>> > problematic. A change wouldn't need to be drastic, though. I think 
>>> > suggestions from the old thread like "StringAtom" or "AtomString" would 
>>> > be unproblematic. The problem is the specific word "atomic" carries an 
>>> > expectation that the object be safe to access concurrently across threads 
>>> > without locks; I think that expectation doesn't exist if not for the "ic" 
>>> > at the end.
>>> > 
>>> > FWIW I've only ever heard the "interned string" terminology prior to now.
>>> 
>>> SingletonString?
>>> UniquedString?
>>> 
>>> I do like UniquedString. That conveys what AtomicString really is. 
>>> SingletonString isn't so great since AtomicString table is still per thread.
>> 
>> So hard to pronounce though! Why not UniqueString? It’s not quite as 
>> explicit but close enough. 
> 
> Wouldn’t it be confusing to use UniqueString type for a string that is 
> *common* in order to save memory?
> 
> Personally, I like the AtomString proposal as it is close to the naming we 
> are used to and addresses the issue raised (atomic has a different meaning 
> with threading).

Or I also like “AtomizedString”.

> Also, I had never heard of interned strings before.
> ___
> 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

Re: [webkit-dev] the name "AtomicString"

[Chris Dumez]

> On Dec 19, 2018, at 9:17 PM, Maciej Stachowiak  wrote:
> 
> 
> 
>> On Dec 19, 2018, at 8:06 PM, Ryosuke Niwa > > wrote:
>> 
>> On Wed, Dec 19, 2018 at 1:13 PM Simon Fraser > > wrote:
>> > On Dec 19, 2018, at 12:33 PM, Michael Catanzaro > > > wrote:
>> > 
>> > On Tue, Dec 18, 2018 at 9:31 PM, Darin Adler > > > wrote:
>> >> I’ve gotten used to the name AtomicString over the years, but I wouldn’t 
>> >> strongly object to changing it if other programmers are often confused by 
>> >> it’s similarity to the term “atomic operations”.
>> > 
>> > Well there were two other developers in the thread Ryosuke linked to who 
>> > made the exact same mistake as me, so I do think the current name is 
>> > problematic. A change wouldn't need to be drastic, though. I think 
>> > suggestions from the old thread like "StringAtom" or "AtomString" would be 
>> > unproblematic. The problem is the specific word "atomic" carries an 
>> > expectation that the object be safe to access concurrently across threads 
>> > without locks; I think that expectation doesn't exist if not for the "ic" 
>> > at the end.
>> > 
>> > FWIW I've only ever heard the "interned string" terminology prior to now.
>> 
>> SingletonString?
>> UniquedString?
>> 
>> I do like UniquedString. That conveys what AtomicString really is. 
>> SingletonString isn't so great since AtomicString table is still per thread.
> 
> So hard to pronounce though! Why not UniqueString? It’s not quite as explicit 
> but close enough. 

Wouldn’t it be confusing to use UniqueString type for a string that is *common* 
in order to save memory?

Personally, I like the AtomString proposal as it is close to the naming we are 
used to and addresses the issue raised (atomic has a different meaning with 
threading).
Also, I had never heard of interned strings before.___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Watch out for std::optional's move constructor

[Chris Dumez]

> On Dec 17, 2018, at 3:55 PM, Alex Christensen  wrote:
> 
> Let me give a concrete example on why, even with our nice-to-use WTF types, 
> the state of a C++ object is undefined after being moved from:
> 
> #include 
> #include 
> #include 
> 
> class Test : public RefCounted { };
> 
> void useParameter(RefPtr&& param)
> {
>   RefPtr usedParam = WTFMove(param);
> }
> 
> void dontUseParameter(RefPtr&&) { }
> 
> int main() {
>   RefPtr a = adoptRef(new Test);
>   RefPtr b = adoptRef(new Test);
>   std::cout << "a null? " << !a << std::endl;
>   std::cout << "b null? " << !b << std::endl;
>   useParameter(WTFMove(a));
>   dontUseParameter(WTFMove(b));
>   std::cout << "a null? " << !a << std::endl;
>   std::cout << "b null? " << !b << std::endl;
>   return 0;
> }
> 
> // clang++ test.cpp -I Source/WTF -L WebKitBuild/Debug -l WTF -framework 
> Foundation -L /usr/lib -l icucore --std=c++17 && ./a.out  
>  
> // a null? 0  
>   
> // b null? 0  
>   
> // a null? 1  
>   
> // b null? 0  
>  
> 
> 
> As you can see, the internals of callee dontUseParameter (which could be in a 
> different translation unit) affects the state of the local variable b in this 
> function.  This is one of the reasons why the state of a moved-from variable 
> is intentionally undefined, and we can’t fix that by using our own 
> std::optional replacement.  If we care about the state of a moved-from 
> object, that is what std::exchange is for.  I think we should do something to 
> track and prevent the use of moved-from values instead of introducing our own 
> std::optional replacement.

Yes, this is a good example of case where std::exchange should be used or where 
the parameter should be a RefPtr and not a RefPtr&&. But as I 
mentioned earlier, the issue here is that the caller of WTFMove() is not 
actually calling any move constructor (it is merely doing a cast) and therefore 
cannot rely on the effects or a move constructor.
I do not think that this example is a good reason why we would not want 
optional to have a more predictable move constructor.


> 
>> On Dec 17, 2018, at 2:47 PM, Ryosuke Niwa > <mailto:rn...@webkit.org>> wrote:
>> 
>> Yeah, it seems like making std::optional more in line with our own 
>> convention provides more merits than downsides here. People are using 
>> WTFMove as if it's some sort of a swap operation in our codebase, and as 
>> Maciej pointed out, having rules where people have to think carefully as to 
>> when & when not to use WTFMove seems more troublesome than the proposed fix, 
>> which would mean this work for optional.
>> 
>> - R. Niwa
>> 
>> On Mon, Dec 17, 2018 at 2:24 PM Geoffrey Garen > <mailto:gga...@apple.com>> wrote:
>> I don’t understand the claim about “undefined behavior” here. As Maciej 
>> pointed out, these are our libraries. We are free to define their behaviors.
>> 
>> In general, “undefined behavior” is an unwanted feature of programming 
>> languages and libraries, which we accept begrudgingly simply because there 
>> are practical limits to what we can define. This acceptance is not a mandate 
>> to carry forward undefined-ness as a badge of honor. In any case where it 
>> would be practical to define a behavior, that defined behavior would be 
>> preferable to undefined behavior.
>> 
>> I agree that the behavior of move constructors in the standard library is 
>> undefined. The proposal here, as I understand it, is to (a) define the 
>> behaviors move constructors in WebKit and (b) avoid std::optional and use an 
>> optional class with well-defined behavior instead.
>> 
>> Because I do not ❤️ security updates, I do ❤️ defined behavior, and so I ❤️ 
>> this proposal.
>> 
>> Geoff
>> 
>>> On Dec 17, 2018, at 12:50 PM, Alex Christensen >> <mailto:achristen...@apple.com>> wrote:
>>> 
>>> This one and the many others like it ar

Re: [webkit-dev] Watch out for std::optional's move constructor

[Chris Dumez]

> On Dec 17, 2018, at 11:10 AM, Chris Dumez  wrote:
> 
> 
> 
>> On Dec 17, 2018, at 10:27 AM, Alex Christensen > <mailto:achristen...@apple.com>> wrote:
>> 
>>>>>> On Dec 14, 2018, at 1:37 PM, Chris Dumez >>>>> <mailto:cdu...@apple.com>> wrote:
>>>>> 
>>>>>> 
>>>>>> As far as I know, our convention in WebKit so far for our types has been 
>>>>>> that types getting moved-out are left in a valid “empty” state.
>> This is not necessarily true.  When we move out of an object to pass into a 
>> function parameter, for example, the state of the moved-from object depends 
>> on the behavior of the callee.  If the callee function uses the object, we 
>> often have behavior that leaves the object in an “empty” state of some kind, 
>> but we are definitely relying on fragile undefined behavior when we do so 
>> because changing the callee to not use the parameter changes the state of 
>> the caller.  We should never assume that WTFMove or std::move leaves the 
>> object in an empty state.  That is always a bug that needs to be replaced by 
>> std::exchange.
> 
> Feel like we’re taking about different things. I am talking about move 
> constructors (and assignment operators), which have a well defined behavior 
> in WebKit. And it seems you are talking about WTFMove(), which despite the 
> name does not “move” anything, it is merely a cast.
> In the case you’re talking about the caller does NOT call the move 
> constructor, it merely does a cast so I do not think your comment invalidates 
> my statement. Note that in my patch, I was nearly WTFMove()ing the data 
> member and assigning it to a local variable right away, calling the move 
> constructor.

Also note that may of us already rely on our move constructors’ behavior, just 
search for WTFMove(m_responseCompletionHandler) in:
https://trac.webkit.org/changeset/236463/webkit___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Watch out for std::optional's move constructor

[Chris Dumez]

> On Dec 17, 2018, at 10:27 AM, Alex Christensen  wrote:
> 
>>>>> On Dec 14, 2018, at 1:37 PM, Chris Dumez >>>> <mailto:cdu...@apple.com>> wrote:
>>>> 
>>>>> 
>>>>> As far as I know, our convention in WebKit so far for our types has been 
>>>>> that types getting moved-out are left in a valid “empty” state.
> This is not necessarily true.  When we move out of an object to pass into a 
> function parameter, for example, the state of the moved-from object depends 
> on the behavior of the callee.  If the callee function uses the object, we 
> often have behavior that leaves the object in an “empty” state of some kind, 
> but we are definitely relying on fragile undefined behavior when we do so 
> because changing the callee to not use the parameter changes the state of the 
> caller.  We should never assume that WTFMove or std::move leaves the object 
> in an empty state.  That is always a bug that needs to be replaced by 
> std::exchange.

Feel like we’re taking about different things. I am talking about move 
constructors (and assignment operators), which have a well defined behavior in 
WebKit. And it seems you are talking about WTFMove(), which despite the name 
does not “move” anything, it is merely a cast.
In the case you’re talking about the caller does NOT call the move constructor, 
it merely does a cast so I do not think your comment invalidates my statement. 
Note that in my patch, I was nearly WTFMove()ing the data member and assigning 
it to a local variable right away, calling the move constructor.


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

Re: [webkit-dev] Watch out for std::optional's move constructor

[Chris Dumez]

> On Dec 16, 2018, at 7:43 PM, Fujii Hironori  wrote:
> 
> I don't like the proposal because it encourages misuse of move.
> We can use move only for values about to be destroyed.

Just for reference, there are close to 400 matches for "WTFMove(m_” in our code 
base. People do seem to rely on the state of objects after being moved out.
I totally agree that the state of the object being moved out is not defined by 
the C++ standard. However, so far, in WebKit, we’ve been careful with our 
move-constructors.

I think that if we do not update std::optional’s move constructor, then I worry 
we’ll keep having to fix bugs in the future due to its misuse. Although, maybe 
this mail thread will help.

That being said, I agree with your and Daniel and we should use std::exchange 
more. I think all the "WTFMove(m_” lines in our code bases should probably be 
replaced with std::exchange.


> 
> I like Dan's suggestion. We should use std::exchange or std::optional::swap 
> for the cases.
> Or, what about adding a new method WTF::Optional::release() for the case?
> 
> ___
> 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] Watch out for std::optional's move constructor

[Chris Dumez]

So to be clear, it is often not truly about using the value after it is moved. 
It is about expecting that the variable / member has been nulled out after 
moving it.
If I WTFMove() out a data member m_dataMember, I expect later on `if 
(m_dataMember)` to be false.

--
 Chris Dumez




> On Dec 14, 2018, at 3:45 PM, Chris Dumez  wrote:
> 
> 
>> On Dec 14, 2018, at 3:39 PM, Fujii Hironori > <mailto:fujii.hiron...@gmail.com>> wrote:
>> 
>> 
>> On Sat, Dec 15, 2018 at 6:38 AM Chris Dumez > <mailto:cdu...@apple.com>> wrote:
>> 
>> I have now been caught twice by std::optional’s move constructor. 
>> 
>>  I don't understand how this could be useful? Do you want to use the value 
>> after it is moved? I'd like to see these your code. Could you show me these 
>> two patches?
> 
> This is the latest one: https://trac.webkit.org/changeset/239228/webkit 
> <https://trac.webkit.org/changeset/239228/webkit>
> 
> 
> ___
> 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] Watch out for std::optional's move constructor

[Chris Dumez]

> On Dec 14, 2018, at 3:39 PM, Fujii Hironori  wrote:
> 
> 
> On Sat, Dec 15, 2018 at 6:38 AM Chris Dumez  <mailto:cdu...@apple.com>> wrote:
> 
> I have now been caught twice by std::optional’s move constructor. 
> 
>  I don't understand how this could be useful? Do you want to use the value 
> after it is moved? I'd like to see these your code. Could you show me these 
> two patches?

This is the latest one: https://trac.webkit.org/changeset/239228/webkit


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

Re: [webkit-dev] Watch out for std::optional's move constructor

[Chris Dumez]

> On Dec 14, 2018, at 1:56 PM, Saam Barati  wrote:
> 
> 
> 
>> On Dec 14, 2018, at 1:54 PM, Saam Barati > <mailto:sbar...@apple.com>> wrote:
>> 
>> 
>> 
>>> On Dec 14, 2018, at 1:37 PM, Chris Dumez >> <mailto:cdu...@apple.com>> wrote:
>>> 
>>> Hi,
>>> 
>>> I have now been caught twice by std::optional’s move constructor. It turns 
>>> out that it leaves the std::optional being moved-out *engaged*, it merely 
>>> moves its value.
>>> 
>>> For example, testOptional.cpp:
>>> #include 
>>> #include 
>>> 
>>> int main()
>>> {
>>> std::optional a = 1;
>>> std::optional b = std::move(a);
>>> std::cout << "a is engaged? " << !!a << std::endl;
>>> std::cout << "b is engaged? " << !!b << std::endl;
>>> return 0;
>>> }
>>> 
>>> $ clang++ testOptional.cpp -o testOptional -std=c++17
>>> $ ./testOptional
>>> a is engaged? 1
>>> b is engaged? 1
>>> 
>>> I would have expected:
>>> a is engaged? 0
>>> b is engaged? 1
>> I would have expected this too.
>> 
>>> 
>>> This impacts the standard std::optional implementation on my machine as 
>>> well as the local copy in WebKit’s wtf/Optional.h.
>>> 
>>> As far as I know, our convention in WebKit so far for our types has been 
>>> that types getting moved-out are left in a valid “empty” state.
>> I believe it's UB to use an object after it has been moved.
> I'm wrong here.
> Apparently objects are left in a "valid but unspecified state".
> 
> https://stackoverflow.com/questions/32346143/undefined-behavior-with-stdmove 
> <https://stackoverflow.com/questions/32346143/undefined-behavior-with-stdmove>
I believe in WebKit, however, we’ve made sure our types are left in a valid 
“empty” state, thus my proposal for our own optional type that would be less 
error-prone / more convenient to use.

> 
> - Saam
>> 
>> - Saam
>> 
>>> As such, I find that std::optional’s move constructor behavior is 
>>> error-prone.
>>> 
>>> I’d like to know how do other feel about this behavior? If enough people 
>>> agree this is error-prone, would we consider having our
>>> own optional type in WTF which resets the engaged flag (and never allow the 
>>> std::optional)?
>>> 
>>> Thanks,
>>> --
>>>  Chris Dumez
>>> 
>>> 
>>> 
>>> 
>>> ___
>>> 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

[webkit-dev] Watch out for std::optional's move constructor

[Chris Dumez]

Hi,

I have now been caught twice by std::optional’s move constructor. It turns out 
that it leaves the std::optional being moved-out *engaged*, it merely moves its 
value.

For example, testOptional.cpp:
#include 
#include 

int main()
{
std::optional a = 1;
std::optional b = std::move(a);
std::cout << "a is engaged? " << !!a << std::endl;
std::cout << "b is engaged? " << !!b << std::endl;
return 0;
}

$ clang++ testOptional.cpp -o testOptional -std=c++17
$ ./testOptional
a is engaged? 1
b is engaged? 1

I would have expected:
a is engaged? 0
b is engaged? 1

This impacts the standard std::optional implementation on my machine as well as 
the local copy in WebKit’s wtf/Optional.h.

As far as I know, our convention in WebKit so far for our types has been that 
types getting moved-out are left in a valid “empty” state.
As such, I find that std::optional’s move constructor behavior is error-prone.

I’d like to know how do other feel about this behavior? If enough people agree 
this is error-prone, would we consider having our
own optional type in WTF which resets the engaged flag (and never allow the 
std::optional)?

Thanks,
--
 Chris Dumez




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

Re: [webkit-dev] PSA: String shouldn't be a member of a ThreadSafeRefCounted class

[Chris Dumez]

Actually, it is my understanding that isolatedCopy() does the right thing here. 
If you look at the implementation:

String String::isolatedCopy() const &
{
// FIXME: Should this function, and the many others like it, be inlined?
return m_impl ? m_impl->isolatedCopy() : String { };
}

String String::isolatedCopy() &&
{
if (isSafeToSendToAnotherThread()) {
// Since we know that our string is a temporary that will be destroyed
// we can just steal the m_impl from it, thus avoiding a copy.
return { WTFMove(*this) };
}

return m_impl ? m_impl->isolatedCopy() : String { };
}

The isolatedCopy() optimization (i.e. isSafeToSendToAnotherThread) can only be 
used if the String is a temporary. If you example, m_name is not a temporary 
and will be deep copied.

--
 Chris Dumez




> On Feb 23, 2018, at 11:45 PM, Ryosuke Niwa <rn...@webkit.org> wrote:
> 
> Hi all,
> 
> This is a remainder that our String class is NOT thread safe, and should NOT 
> be used inside an object shared across multiple threads. In particular, it's 
> not necessarily safe to have it as a member of ThreadSafeRefCounted class, 
> which can be accessed from multiple threads.
> 
> Let's consider the following example.
> 
> class A : public ThreadSafeRefCounted {
> public:
> A(const String& name)
> : m_name(name)
> { }
> String name() { return m_name.isolatedCopy(); }
> 
> private:
> String m_name;
> }
> 
> This code is NOT thread safe depending on how name() is used.
> 
> For example, if it's ever inserted or looked up in a hash table as the key, 
> or if it's ever converted into an AtomicString, then it would lead to memory 
> corruption. This is because String::hash() would mutate m_hashAndFlags member 
> variable without any lock, and isolatedCopy() doesn't make a copy if there is 
> exactly one reference to a given StringImpl (String is basically just a 
> RefPtr of StringImpl).
> 
> - 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] Upstreaming from LayoutTests to web-platform-tests, coordinating Blink+WebKit

[Chris Dumez]

> On Nov 17, 2017, at 9:18 AM, youenn fablet  wrote:
> 
> Chris recently noticed that some heavily used files (testharness*) were 
> cacheable through Apache but not WPT.

Wasn’t me. Alexey reported this via Description 
.___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] buildbot / ios-sim failing on serviceworket tests?

[Chris Dumez]

These are likely flaky tests, unrelated to your change.

--
 Chris Dumez




> On Nov 6, 2017, at 5:38 AM, Colin Bendell | +1.613.914.3387 
> <co...@bendell.ca> wrote:
> 
> Can someone provide guidance on how I should interpret serviceworker
> failures on the ios-sim queue? The same buildbot error appears in
> other patches:
> 
> https://webkit-queues.webkit.org/results/5118974 (bug 179231)
> https://webkit-queues.webkit.org/results/5121120 (bug 179285)
> 
> thoughts?
> 
> /colin
> ___
> 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] Get rid of RefPtr, replace with std::optional?

[Chris Dumez]

I think std::optional<Ref> looks ugly. Also, unlike RefPtr<>, I do not 
think it is copyable. It is pretty neat to be able to capture a RefPtr<> by 
value in a lambda.
Also, how do you convert it to a raw pointer? myOptionalRef.value_or(nullptr) 
would not work. Not sure there would be a nice way to do so.

Finally, the storage space argument from Maciej is a good one.
 
--
 Chris Dumez




> On Sep 1, 2017, at 9:46 AM, Maciej Stachowiak <m...@apple.com> wrote:
> 
> 
> 
>> On Sep 1, 2017, at 9:30 AM, Brady Eidson <beid...@apple.com> wrote:
>> 
>> I recently worked on a patch where - because of the organic refactoring of 
>> the patch over its development - I ended up with a std::optional 
>> instead of a RefPtr.
>> 
>> A followup review after it had already landed pointed this out, and it got 
>> me to thinking:
>> 
>> Does RefPtr do anything for us today that std::optional doesn’t?
> 
> The obvious things would be: uses less storage space, has a shorter name.
> 
>> 
>> I kind of like the idea of replacing RefPtr with std::optional. It 
>> makes it explicitly clear what object is actually holding the reference, and 
>> completely removes some of the confusion of “when should I use Ref vs 
>> RefPtr?"
>> 
>> Thoughts?
>> 
>> Thanks,
>> ~Brady
>> ___
>> 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] Growing tired of long build times? Check out this awesome new way to speed up your build... soon (HINT: It's not buying a new computer)

[Chris Dumez]

> On Aug 29, 2017, at 9:24 AM, Keith Miller <keith_mil...@apple.com> wrote:
> 
>> * We could not use any namespaces and just fix name clashes as they arise, 
>> but only if we have some more predictable way to determine which files will 
>> be built together regardless of port or build configuration. Say files in 
>> small directories all get built together, while files in large directories 
>> get built together if their filenames start with the same letter. That might 
>> be too confusing, but a scheme like that would make it possible for us to 
>> ensure we don't have name clashes regardless of port configuration, while 
>> avoiding the need to add namespaces all over the place.
> 
> I had considered something like this. Ultimately, I decided the FILENAME 
> solution was better because it lets us use descriptive names without having 
> to unique the name. When new cpp files are added to the project, if we don’t 
> use a namespace solution new errors in totally unrelated files may appear due 
> to reordering of the unified source lists. For new developers, errors 
> exclusively in .cpp files they didn’t touch would be particularly frustrating 
> and confusing. e.g. They add Foo.cpp which causes Bar.cpp and Baz.cpp to get 
> bundled together causing a redefinition error in Bar/Baz.cpp. While, new 
> developers might also be annoyed with the FILENAME solution, it’s a pretty 
> simple rule to follow and it doesn’t lead to the same surprises. 


Keep in mind that if you do this, you may not get a build error and still run 
into trouble. If several static functions end up having the same name but 
different parameters, your code may build but may end up crashing or with 
different behavior because we end up calling a different overload. This would 
seem very risky for a big project such as WebKit.

--
 Chris Dumez

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

Re: [webkit-dev] Growing tired of long build times? Check out this awesome new way to speed up your build... soon (HINT: It's not buying a new computer)

[Chris Dumez]

I worry about adopting unity build because while it makes clean builds faster, 
it also slows down incremental builds. As a developer, I rarely do clean 
builds, I mostly do incremental builds so this would likely make my experience 
worse?
Unity builds also require a lot of code churn to resolve naming conflicts and 
the resulting code does not look as nice.

Finally, the statement that the slow down of incremental build is acceptable 
because we spend most of our time resolving dependencies seems to assume we 
keep using make. Using Ninja would speed up incremental builds a lot by not
re-resolving dependencies so much.

--
 Chris Dumez




> On Aug 29, 2017, at 9:05 AM, Darin Adler <da...@apple.com> wrote:
> 
>> On Aug 28, 2017, at 8:34 PM, Ryosuke Niwa <rn...@webkit.org 
>> <mailto:rn...@webkit.org>> wrote:
>> 
>>> Wherever possible, we are going to want to convert file-static functions
>>> into private C++ class member functions.
>> 
>> I don't think we should make this change. It would mean that whenever
>> the function signature changes, we'd have to modify the header file,
>> which in turn triggers rebuild of every cpp file which includes that
>> header file.
> 
> 
> That was my first thought as well. In fact, I typically ask people to do the 
> opposite: Whenever a function does not require access to private members, 
> convert from a member function that has to be in the header file to a 
> function that is private to the source file.
> 
> More recently I have started doing something different for the many functions 
> that only have to be used on one place; I use lambdas to create nested 
> functions. This has a couple nice properties: The lambda shares access to 
> private members and anything else that the surrounding function has access 
> to. We have the option of capturing rather than passing arguments, which can 
> be clearer in some cases. It’s not quite as good as a separate function for 
> visual separation; the lambda is often mashed together with the rest of the 
> surrounding function.
> 
> — Darin
> ___
> 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] Unified source builds: A new rule for static variables

[Chris Dumez]

I indeed think this will require “using” statements or explicit namespace at 
call sites.

I don’t think anonymous namespaces are suitable for resolving naming conflicts 
due to unity builds since the functions and up in the same compilation unit.

--
 Chris Dumez




> On Aug 29, 2017, at 9:00 AM, Darin Adler <da...@apple.com> wrote:
> 
> How does this work? Without a “using” how does it know to search this 
> namespace? Is this superior to using anonymous namespaces instead of “static”?
> 
> — Darin
> ___
> 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] How we enable template functions

[Chris Dumez]

I personally prefer std::enable_if<>. For e.g.

template::value>
Class Foo { }

I don’t like that something inside the body of a class / function would cause a 
template to be enabled or not.

--
 Chris Dumez




> On Aug 22, 2017, at 8:34 PM, Keith Miller <keith_mil...@apple.com> wrote:
> 
> Hello fellow WebKittens,
> 
> I’ve noticed over time that we don’t have standard way that we enable 
> versions of template functions/classes (flasses?). For the most part it seems 
> that people use std::enable_if, although, it seems like it is attached to 
> every possible place in the function/class.
> 
> I propose that we choose a standard way to conditionally enable a template.
> 
> There are a ton of options; my personal favorite is to add the following 
> macro:
> 
> #define ENABLE_TEMPLATE_IF(condition) static_assert(condition, “template 
> disabled”)
> 
> Then have every function do:
> 
> template
> void foo(…)
> {
>ENABLE_TEMPLATE_IF(std::is_same<T, int>::value);
>…
> }
> 
> And classes:
> 
> template
> class Foo {
>ENABLE_TEMPLATE_IF(std::is_same<T, int>::value);
> };
> 
> I like this proposal because it doesn’t obstruct the signature/declaration of 
> the function/class but it’s still obvious when the class is enabled. 
> Obviously, I think we should require that this macro is the first line of the 
> function or class for visibility. Does anyone else have thoughts or ideas?
> 
> Cheers,
> Keith
> 
> P.S. in case you are wondering why this macro works (ugh C++), it’s because 
> if there is any compile time error in a template it cannot be selected as the 
> final candidate. In my examples, if you provided a type other than int 
> foo/Foo could not be selected because the static_assert condition would be 
> false, which is a compile error.
> ___
> 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] Does someone know how to fix WTF::Function on Windows

[Chris Dumez]

I seem to remember I fixed it last time by calling the WTF::Function 
constructor explicitly.

Chris Dumez

> On Jul 15, 2017, at 9:14 AM, Yusuke SUZUKI <utatane@gmail.com> wrote:
> 
> I'm not 100% confident, but can you try it `` instead?
> 
> Regards,
> Yusuke Suzuki
> 
>> On Sun, Jul 16, 2017 at 1:13 AM, Darin Adler <da...@apple.com> wrote:
>> Hi folks.
>> 
>> On Windows, WTF::Function doesn’t quite work right. Code that is something 
>> like this:
>> 
>> void addCallback(WTF::Function<void()>&&);
>> 
>> void testFunction()
>> {
>> // ...
>> }
>> 
>> void addTestFunction()
>> {
>> addCallback(testFunction);
>> }
>> 
>> Leads to errors like this:
>> 
>> error C2664: 'void addCallback(WTF::Function &&)': cannot 
>> convert argument 1 from 'void (__cdecl *)(void)' to 'WTF::Function> (void)> &&'
>> note: You cannot bind an lvalue to an rvalue reference
>> 
>> The problem might have something to do with cdecl vs. stdcall functions, but 
>> I am not sure that is the problem. It could be some other problem with how 
>> WTF::Function is written. Or it might even be a bug in the Visual Studio 
>> compiler. Since I don’t have a Windows machine myself, I was trying to use 
>> EWS to figure this out but that was slow. Then I tried using 
>> http://webcompiler.cloudapp.net but I could not reproduce any error there 
>> when I pasted in cut down code; it just compiled fine.
>> 
>> Is there someone who knows how to fix this?
>> 
>> Another way to put this is: We want to take off the explicit WTF::Function 
>> conversions in functions like canUseWithReason in SimpleLineLayout.cpp, 
>> Page::Page in Page.cpp, and Worker::Worker in Worker.cpp and have it still 
>> compile and work on Windows. Other platforms seem to compile fine without 
>> the explicit WTF::Function.
>> 
>> — Darin
>> ___
>> 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] Should we ever use std::function instead of WTF::Function?

[Chris Dumez]

> On Jun 13, 2017, at 12:51 PM, Filip Pizlo <fpi...@apple.com> wrote:
> 
>> 
>> On Jun 13, 2017, at 12:37 PM, Alex Christensen <achristen...@apple.com 
>> <mailto:achristen...@apple.com>> wrote:
>> 
>> Ok, maybe we can get rid of std::function, then!  I hadn’t used BlockPtr as 
>> much as Chris.  I’d be opposed to adding a copy constructor to WTF::Function 
>> because the non-copyability of WTF::Function is why we made it, and it has 
>> prevented many bugs.
> 
> I agree that the copy semantics of std::function are strange - each copy gets 
> its own view of the state of the closure.  This gets super weird when you 
> implement an algorithm that accidentally copies the function - the act of 
> copying invokes copy constructors on all of the lambda-lifted state, which is 
> probably not what you wanted.  So I’m not surprised that moving to 
> WTF::Function avoided bugs.  What I’m proposing also prevents many of the 
> same bugs because the lambda-lifted state never gets copied in my world.

I understand SharedTask avoids many of the same issues. My issue is that it 
adds an extra data member for refcounting that is very rarely needed in 
practice. Also, because this type is copyable, people can create refcounting 
churn by inadvertently copying.
Because most of the time, we do not want to share and WTF::Function is 
sufficient as it stands, I do not think it is worth making WTF::Function 
refcounted.

> 
> Do you think that code that uses ObjC blocks encounters the kind of bugs that 
> you saw WTF::Function preventing?  Or are the bugs that Function prevents 
> more specific to std::function?  I guess I’d never heard of a need to change 
> block semantics to avoid bugs, so I have a hunch that the bugs you guys 
> prevented were specific to the fact that std::function copies instead of 
> sharing.

To be cleared, we viewed this as “copies instead of truly moving”, not sharing. 
We never really needed sharing when WTF::Function was initially called 
WTF::NonCopyableFunction.
Yes, the bugs we were trying to avoid were related to using std::function and 
copying things implicitly, even if you WTFMove() it around. Because we started 
using WTF::Function instead of std::function in more places though, having 
BlockPtr::fromCallable() to be able to pass a WTF::Function to an ObjC function 
expecting a block became handy though. 

> 
>> 
>> I’ve also seen many cases where I have a WTF::Function that I want to make 
>> sure is called once and only once before destruction.  I wouldn’t mind 
>> adding a WTF::Callback subclass that just asserts that it has been called 
>> once.  That would’ve prevented some bugs, too, but not every use of 
>> WTF::Function has such a requirement.
>> 
>>> On Jun 13, 2017, at 12:31 PM, Chris Dumez <cdu...@apple.com 
>>> <mailto:cdu...@apple.com>> wrote:
>>> 
>>> We already have BlockPtr for passing a Function as a lambda block.
>>> 
>>> Chris Dumez
>>> 
>>> On Jun 13, 2017, at 12:29 PM, Alex Christensen <achristen...@apple.com 
>>> <mailto:achristen...@apple.com>> wrote:
>>> 
>>>> std::function, c++ lambda, and objc blocks are all interchangeable.  
>>>> WTF::Functions cannot be used as objc blocks because the latter must be 
>>>> copyable.  Until that changes or we stop using objc, we cannot completely 
>>>> eliminate std::function from WebKit.
>>>> ___
>>>> 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

Re: [webkit-dev] Should we ever use std::function instead of WTF::Function?

[Chris Dumez]

We already have BlockPtr for passing a Function as a lambda block.

Chris Dumez

> On Jun 13, 2017, at 12:29 PM, Alex Christensen <achristen...@apple.com> wrote:
> 
> std::function, c++ lambda, and objc blocks are all interchangeable.  
> WTF::Functions cannot be used as objc blocks because the latter must be 
> copyable.  Until that changes or we stop using objc, we cannot completely 
> eliminate std::function from WebKit.
> ___
> 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] Should we ever use std::function instead of WTF::Function?

[Chris Dumez]

In most cases in WebCore at least, we don’t actually want to share ownership of 
the lambda so we don’t need RefCounting / SharedTask. Because of this, I don’t 
think we should merge SharedTask into Function. I think that as it stands, 
WTF::Function is a suitable replacement for most uses in WebCore since we 
actually very rarely need copying (either it just builds or the code can be 
refactored very slightly to avoid the copying).

--
 Chris Dumez




> On Jun 13, 2017, at 9:34 AM, Filip Pizlo <fpi...@apple.com> wrote:
> 
> We should have a better story here.  Right now the story is too complicated.  
> We have:
> 
> - ScopedLambda or ScopedLambdaRef if you have a stack-allocated function that 
> outlives its user
> - SharedTask if you have a heap-allocated function that you want to share and 
> ref-count
> - WTF::Function if you have a heap-allocated function that you want to 
> transfer ownership (move yes, copy no)
> - std::function if you have a heap-alloated function that you want to pass by 
> copy
> 
> Only std::function and WTF::Function do the magic that lets you say:
> 
> std::function f = 
> 
> Also, std::function has the benefit that it does copying.  None of the others 
> do that.
> 
> ScopedLambda serves a specific purpose: it avoids allocation.  Probably we 
> want to keep that one even if we merge the others.
> 
> IMO SharedTask has the nicest semantics.  I don’t ever want the activation of 
> the function to be copied.  In my experience I always want sharing if more 
> than one reference to the function exists.  I think that what we really want 
> in most places is a WTF::Function that has sharing semantics like SharedTask. 
>  That would let us get rid of std::function and SharedTask.
> 
> In the current status quo, it’s not always correct to convert std::function 
> to the others because:
> 
> - Unlike ScopedLambda and SharedTask, std::function has the magic constructor 
> that allows you to just assign a lambda to it.
> - Unlike ScopedLambda, std::function is safe if the use is not scoped.
> - Unlike WTF::Function, std::function can be copied.
> 
> -Filip
> 
> 
>> On Jun 13, 2017, at 9:24 AM, Darin Adler <da...@apple.com> wrote:
>> 
>> I’ve noticed many patches switching us from std::function to WTF::Function 
>> recently, to fix problems with copying and thread safety.
>> 
>> Does std::function have any advantages over WTF::Function? Should we ever 
>> prefer std::function, or should we use WTF::Function everywhere in WebKit 
>> where we would otherwise use std::function?
>> 
>> — Darin
>> ___
>> 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] Should we ever use std::function instead of WTF::Function?

[Chris Dumez]

The only advantage of std::function that I know of is that it is copyable. 
Unless you really need to copy your lambda for a reason or another, I don’t see 
a good reason to ever use std::function instead of WTF::Function.

I have been slowly replacing std::function by WTF::Function in the code base 
but there are still a lot of uses of std::function.

--
 Chris Dumez




> On Jun 13, 2017, at 9:24 AM, Darin Adler <da...@apple.com> wrote:
> 
> I’ve noticed many patches switching us from std::function to WTF::Function 
> recently, to fix problems with copying and thread safety.
> 
> Does std::function have any advantages over WTF::Function? Should we ever 
> prefer std::function, or should we use WTF::Function everywhere in WebKit 
> where we would otherwise use std::function?
> 
> — Darin
> ___
> 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] "ReflectOnly" IDL equivalent

[Chris Dumez]

> On Jun 9, 2017, at 11:47 AM, Sam Weinig <wei...@apple.com> wrote:
> 
> 
> 
>> On Jun 2, 2017, at 11:32 AM, Ryosuke Niwa <rn...@webkit.org> wrote:
>> 
>> On Fri, Jun 2, 2017 at 9:18 AM, Chris Dumez <cdu...@apple.com> wrote:
>>> Hi,
>>> 
>>> No, I do not believe WebKit supports ReflectOnly and this is not standard
>>> IDL either.
>>> 
>>> The way to do it in WebKit would be to use a regular DOMString attribute, as
>>> in the specification and implement this logic in the c++ getter for this
>>> attribute. See HTMLElement::dir() for an example.
>>> 
>>> We could also consider adding support for something like ReflectOnly in our
>>> bindings generator considering that this seems to be used quite a bit in the
>>> HTML specification and it would decrease code complexity a little.
>>> I’d actually be in favor of that.
>> 
>> I'd suggest other names like "ReflectEnum" or even "Reflect"
>> where EnumType is the name of enum that defines the list of values.
>> 
>> "ReflectOnly" doesn't tell us on what "only" applies. If I didn't know
>> the context, it sounds like something that does less work than regular
>> "Reflect”.
> 
> 
> I don’t see a good reason to complicate the bindings until this becomes more 
> common place.  For now, I would just implement HTMLLinkElement::as() to 
> behave as you want and leave the IDL unannotated, and we can revisit it at a 
> later time.

As I said, this is already used in quite a few places in the HTML spec:
- https://html.spec.whatwg.org/#dom-dir <https://html.spec.whatwg.org/#dom-dir>
- https://html.spec.whatwg.org/#dom-link-as 
<https://html.spec.whatwg.org/#dom-link-as>
- https://html.spec.whatwg.org/#dom-link-referrerpolicy 
<https://html.spec.whatwg.org/#dom-link-referrerpolicy>
- https://html.spec.whatwg.org/#dom-link-updateviacache 
<https://html.spec.whatwg.org/#dom-link-updateviacache>
- https://html.spec.whatwg.org/#dom-a-referrerpolicy 
<https://html.spec.whatwg.org/#dom-a-referrerpolicy>
- https://html.spec.whatwg.org/#dom-img-referrerpolicy 
<https://html.spec.whatwg.org/#dom-img-referrerpolicy>
- https://html.spec.whatwg.org/#dom-iframe-referrerpolicy 
<https://html.spec.whatwg.org/#dom-iframe-referrerpolicy>
- https://html.spec.whatwg.org/#dom-track-kind 
<https://html.spec.whatwg.org/#dom-track-kind>
- https://html.spec.whatwg.org/#dom-media-preload 
<https://html.spec.whatwg.org/#dom-media-preload>
- https://html.spec.whatwg.org/#dom-area-referrerpolicy 
<https://html.spec.whatwg.org/#dom-area-referrerpolicy>
- https://html.spec.whatwg.org/#dom-th-scope 
<https://html.spec.whatwg.org/#dom-th-scope>
- https://html.spec.whatwg.org/#dom-form-autocomplete 
<https://html.spec.whatwg.org/#dom-form-autocomplete>
- https://html.spec.whatwg.org/#dom-input-type 
<https://html.spec.whatwg.org/#dom-input-type>
- https://html.spec.whatwg.org/#dom-input-inputmode 
<https://html.spec.whatwg.org/#dom-input-inputmode>
- https://html.spec.whatwg.org/#dom-button-type 
<https://html.spec.whatwg.org/#dom-button-type>
- https://html.spec.whatwg.org/#dom-textarea-inputmode 
<https://html.spec.whatwg.org/#dom-textarea-inputmode>
- https://html.spec.whatwg.org/#dom-fs-method 
<https://html.spec.whatwg.org/#dom-fs-method>
- https://html.spec.whatwg.org/#dom-fs-enctype 
<https://html.spec.whatwg.org/#dom-fs-enctype>
- https://html.spec.whatwg.org/#dom-fs-formenctype 
<https://html.spec.whatwg.org/#dom-fs-formenctype>
- https://html.spec.whatwg.org/#dom-fs-formmethod 
<https://html.spec.whatwg.org/#dom-fs-formmethod>

Having a per-standard implementation in the bindings would likely be better 
than many potentially non-compliant ones.

--
 Chris Dumez

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

Re: [webkit-dev] "ReflectOnly" IDL equivalent

[Chris Dumez]

Hi,

No, I do not believe WebKit supports ReflectOnly and this is not standard IDL 
either.

The way to do it in WebKit would be to use a regular DOMString attribute, as in 
the specification and implement this logic in the c++ getter for this 
attribute. See HTMLElement::dir() for an example.

We could also consider adding support for something like ReflectOnly in our 
bindings generator considering that this seems to be used quite a bit in the 
HTML specification and it would decrease code complexity a little.
I’d actually be in favor of that.

--
 Chris Dumez




> On Jun 1, 2017, at 10:57 PM, Yoav Weiss <y...@yoav.ws> wrote:
> 
> I'm working on the link preload feature and as part of that want to align it 
> to the spec from an IDL perspective.
> The `as` attribute is defined <https://html.spec.whatwg.org/#attr-link-as> as 
> an "enumerated attribute", which reflects only a finite set of known values. 
> That is important for feature detection purposes, so that developers can know 
> which `as` values are supported by the implementation.
> 
> In Blink this is done using `ReflectOnly 
> <https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/bindings/IDLExtendedAttributes.md#ReflectOnly_list_a>`
>  (pending CL <https://codereview.chromium.org/2903653005/>), and I'm 
> wondering what is the WebKit equivalent to implement attributes that are 
> limited to known values 
> <https://html.spec.whatwg.org/#limited-to-only-known-values>.
> 
> Thanks! :)
> Yoav
> ___
> 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] Another WPT bite

[Chris Dumez]

Our test importer script is perfectly able to rewrite those paths to use 
relative paths. However, Youenn, who imports and re-syncs most tests does not 
like this option I believe.
I think, part of the issue is that *some* tests do not do the right thing when 
loading over file:// (e.g. security ones) and it is not necessarily obvious 
just by looking at the test.

--
 Chris Dumez




> On May 12, 2017, at 2:10 PM, Simon Fraser <simon.fra...@apple.com> wrote:
> 
>> 
>> On May 12, 2017, at 12:04 PM, Alexey Proskuryakov <a...@webkit.org 
>> <mailto:a...@webkit.org>> wrote:
>> 
>> 
>>> 12 мая 2017 г., в 11:52, Ben Kelly <b...@wanderview.com 
>>> <mailto:b...@wanderview.com>> написал(а):
>>> 
>>> On Fri, May 12, 2017 at 2:26 PM, Rick Byers <rby...@chromium.org 
>>> <mailto:rby...@chromium.org>> wrote:
>>> On Fri, May 12, 2017 at 2:06 PM, Alexey Proskuryakov <a...@webkit.org 
>>> <mailto:a...@webkit.org>> wrote:
>>> Since imported WPT tests are very flaky, and are not necessarily written to 
>>> defend against important regressions, investigating issues with them is 
>>> relatively lower priority than investigating issues observed with WebKit 
>>> tests. So I would recommend not mixing tests for WebKit regressions with 
>>> WPT tests - if your test eventually ends up in LayoutTests/imported, it 
>>> will become a lot less effective.
>>> 
>>> FWIW this is absolutely NOT how we're treating this in chromium.  If this 
>>> is how things end up in practice then we will have failed massively in this 
>>> effort.
>>> 
>>> We figure if we want the web to behave consistently, we really have no 
>>> choice but to treat web-platform-tests as first class with all the 
>>> discipline we give to our own tests.  As such we are actively moving 
>>> <https://codereview.chromium.org/2877673004> many of our LayoutTests to 
>>> web-platform-tests and depending entirely on the regression prevention they 
>>> provide us from there.  Obviously there will be hiccups, but because our 
>>> product quality will depend on web-platform-tests being an effective and 
>>> non-flaky testsuite (and because we're starting to require most new 
>>> features have web-platform-tests before they ship), I'm confident that 
>>> we've got the incentives in place to lead to constant ratcheting up the 
>>> engineering discipline and quality of the test suite.
>>> 
>>> FWIW, mozilla also treats WPT as first class tests.  We're not actively 
>>> moving old tests to WPT like google, but all new tests (at least in DOM) 
>>> are being written in WPT.  Of course, we do have exceptions for some tests 
>>> that require gecko-specific features (forcing GC, etc).
>> 
>> 
>> We don't have a concept of "first class", but I hope that when choosing 
>> between looking into a simple test that was added for a known important bug, 
>> and looking into an imported test whose importance is unclear, any WebKit 
>> engineer will pick the former. And since no one can fix all the things, such 
>> prioritization makes imported tests less effective.
> 
> I just ran into a classic example of how a WPT incurred more overhead. I made 
> a code change that broke 
> LayoutTests/imported/w3c/web-platform-tests/cssom-view/elementFromPoint.html. 
> I tried loading it in Safari and it doesn't run the test code because it 
> can't find /resources/testharness.js when loaded from a local file.
> 
> So then I have to figure out how to fire up the WPT server 
> (run-webkit-httpd), then figure out which host to use (localhost or 
> 128.0.0.1?) and which port, and finally to figure out the right path to the 
> test.
> 
> There's no reason this test should not work when loaded from file://.
> 
> Simon
> 
> ___
> 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

Re: [webkit-dev] Another WPT bite

[Chris Dumez]

> On May 8, 2017, at 9:44 PM, Geoffrey Garen  wrote:
> 
>> Is it time to make testharness.js the recommended way of writing LayoutTests?
> 
> What are the costs and benefits of testharness.js?

Benefit: 
- Tests would be more easily upstreamable to web-platform-tests, which are run 
by all major browser engines. This would help a lot in terms of 
interoperability. As previously discussed, Gecko and Blink already do automated 
export of tests to web-platform-tests. I believe we should do in the same 
direction and contribute more tests back.
- It is quite powerful (see documentation [1], has things like Promise tests, 
Worker tests and advanced assertions.

Cost:
- People are not necessarily used to it so there is a little bit of a learning 
curve. It is well documented though [1].

> 
> We usually try to make regression tests reductions of some larger problem to 
> aid debugging and to make testing fast. But testharness.js is 95kB. That's 
> kind of the opposite of a reduction.

It is proposed as a replacement to js-test.js, which a lot of us are already 
using in our layout tests. Using js-test.js or testharness.js has rarely 
interfered in my reductions, although it has happened to me.
For some tests, we’ll probably not use any framework. However, for most of 
them, I personally don’t see an issue.

[1] http://testthewebforward.org/docs/testharness-library.html 



> 
> Geoff
> 
>> 
>> To continue moving forward, some of us are proposing to serve all tests in 
>> LayoutTests/wpt through the WPT server [1].
>> This would serve some purposes like increasing the use of WPT goodies: 
>> file-specific headers, templated tests (*.any.js), IDLParser, server-side 
>> scripts...
>> It could also ease test migration from WebKit to W3C WPT.
>> 
>> Some rules can guide whether adding tests to LayoutTests/wpt or 
>> LayoutTests/imported/w3c/web-platform-tests:
>> - WebKit specific tests (crash tests, tests using internals...) in 
>> LayoutTests/wpt
>> - Spec conformance/interoperability tests in 
>> LayoutTests/imported/w3c/web-platform-tests
>> 
>>y
>> 
>> [1]: bug 171479 
>> 
>> 
>> ___
>> 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] !!Tests for equality comparison

[Chris Dumez]

> On Apr 28, 2017, at 8:10 AM, Geoffrey Garen <gga...@apple.com> wrote:
> 
> Tests for numeric values where 0 indicates falsiness or emptiness should also 
> use if (x) / if (!x).
> 
>If (!collection.size())
>return;


I think if (collection.isEmpty()) looks even better :)

--
 Chris Dumez

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

Re: [webkit-dev] !!Tests for equality comparison

[Chris Dumez]

I also do not like this rule when it comes to integers.

I personally think JF’s proposal to allow == 0 sounds nice. I don’t think JF 
was suggesting rewriting existing code (which would indeed cause a lot of 
churn).

--
 Chris Dumez




> On Apr 27, 2017, at 4:30 PM, Geoffrey Garen <gga...@apple.com> wrote:
> 
> I’ve never really liked this style rule, and I’ve always felt like it snuck 
> into the style document without much discussion.
> 
> Even so, I usually tolerate it.
> 
> Geoff
> 
>> On Apr 27, 2017, at 4:06 PM, JF Bastien <jfbast...@apple.com 
>> <mailto:jfbast...@apple.com>> wrote:
>> 
>> Hello C++ fans!
>> 
>> The C++ style check currently say:
>> Tests for true/false, null/non-null, and zero/non-zero should all be done 
>> without equality comparisons
>> 
>> I totally agree for booleans and pointers… but not for integers. I know it’s 
>> pretty much the same thing, but I it takes me slightly longer to process 
>> code like this:
>> 
>> int numTestsForEqualityComparison = 0:
>> // Count ‘em!
>> // …
>> if (!numTestsForEqualityComparison)
>>   printf(“Good job!”);
>> 
>> I read it as “if not number of tests for equality comparison”. That's weird. 
>> It takes me every slightly longer to think about, and I’ve gotten it wrong a 
>> bunch of times already. I’m not trying to check for “notness", I’m trying to 
>> say “if there were zero tests for equality comparison”, a.k.a.:
>> 
>> if (numTestsForEqualityComparison == 0)
>>   printf(“Good job!”);
>> 
>> So how about the C++ style let me just say that? I’m not suggesting we 
>> advise using that style for integers everywhere, I’m just saying it should 
>> be acceptable to check zero/non-zero using equality comparison.
>> 
>> 
>> !!Thanks (i.e. many thanks),
>> 
>> JF
>> 
>> p.s.: With you I am, fans of Yoda comparison, but for another day this will 
>> be.
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org <mailto: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] bugs.webkit.org - Maintenance: 10.00 - 11.00am

[Chris Dumez]

Any update? It’s been a few minutes :)

--
 Chris Dumez




> On Mar 30, 2017, at 9:55 AM, Ling Ho <lin...@apple.com> wrote:
> 
> Hello WebKit developers,
> 
> We are flipping a switch to restart bugs.webkit.org on a new server between 
> 10-11am this morning. There should only be a few minutes of interruptions 
> while DNS records are being changed. Please delay saving any change till we 
> are back on the new server.
> 
> Thanks,
> ...
> ling
> ___
> 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] Bugzilla down?

[Chris Dumez]

Bugzilla seems down for me. I am getting certificate errors:


--
 Chris Dumez




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

Re: [webkit-dev] SVN trouble

[Chris Dumez]

It also seems bugzilla mail notifications may be down.

Chris Dumez

> On Feb 25, 2017, at 8:40 AM, Simon Fraser <simon.fra...@apple.com> wrote:
> 
> EWS is still down. Do we have an ETA?
> 
> Simon
> 
>>> On Feb 24, 2017, at 10:25 PM, Alexey Proskuryakov <a...@webkit.org> wrote:
>>> 
>>> 
>>> 24 февр. 2017 г., в 19:50, Chris Dumez <cdu...@apple.com> написал(а):
>>> 
>>> 
>>> 
>>> 
>>>> On Feb 24, 2017, at 11:41 AM, Alexey Proskuryakov <a...@webkit.org> wrote:
>>>> 
>>>> I believe that all infrastructure has recovered. We are currently looking 
>>>> into one unrelated issue with webkit-queues, so EWS and commit queue don't 
>>>> work.
>>>> 
>>>> - Alexey
>>> 
>>> 
>>> It looks like EWS is still down. Did it break again or is it just not fixed 
>>> yet?
>> 
>> 
>> It did work for a period of time, but no conclusive fix yet.
>> 
>> - Alexey
> 
> ___
> 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] SVN trouble

[Chris Dumez]

> On Feb 24, 2017, at 11:41 AM, Alexey Proskuryakov <a...@webkit.org> wrote:
> 
> I believe that all infrastructure has recovered. We are currently looking 
> into one unrelated issue with webkit-queues, so EWS and commit queue don't 
> work.
> 
> - Alexey


It looks like EWS is still down. Did it break again or is it just not fixed yet?

--
 Chris Dumez

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

Re: [webkit-dev] SVN trouble

[Chris Dumez]

> On Feb 24, 2017, at 11:41 AM, Alexey Proskuryakov <a...@webkit.org> wrote:
> 
> The only thing I did was block access to 
> cache/disk-cache/resources/shattered-2.pdf using authz (this is the file with 
> collision). I think that the reason why mirrors only updated now is that 
> someone committed to trunk, and thus invoked post-commit hooks.


Yes, I confirm manual commits work now :)

--
 Chris Dumez

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

Re: [webkit-dev] Upstreaming Tests from WebKit to Web Platform Tests

[Chris Dumez]

I also think we should do 2-way sync-ing of WPT tests, similarly to other 
browser engines. These tests are extremely useful for interoperability. Every 
major browser engine is involved.

Google also has a nice dashboard showing the pass rates for each WPT test in 
each major browser. It makes it easier to identify areas we should improve on.

I - for one - would be happy to write tests using testharness.js when it makes 
sense if it means I can just land them in WebKit with my code change and have 
them being automagically upstreamed, and run by other browsers as well.

Note that I am not advocating we use testharness.js for everything (although I 
would be OK with this). I propose we make it opt-in. We have a lot of tests 
that are not necessarily interesting to upstream. However, the ones that are, 
using testharness.js makes a lot of sense.

Chris Dumez

> On Feb 5, 2017, at 5:04 PM, youenn fablet <youe...@gmail.com> wrote:
> 
> I too am a big proponent of us moving more and more towards WPT.
> As part of the streams and fetch API implementation, most of the related 
> functional tests have been made as WPT.
> The benefits of having tests being improved and updated largely outweighs the 
> testharness.js initial cost.
> Somehow, these tests are becoming part of their related specs.
> 
> The two complaints I heard against testharness.js are:
> - They are less easy to debug as test harness.js does not print out messages 
> for each assert. There might be room for updating testharness to support that
> - Async tests are less easy to write. While this is probably true, 
> testharness has support for promise-based tests which are not verbose.
> WPT has also some benefits of its own, like the possibility to run easily the 
> same tests in worker/window environments, the flexible python-based server...
> 
> One complaint I heard against WPT tests is that they require being run behind 
> a server.
> This is becoming more and more true. 
> There is still a large portion of tests that could be run locally if we 
> update the paths to test harness.js/testharnessreport,js.
> I am not a big fan of modify any WPT test but maybe we should consider 
> switching back to modifying these paths at import and export time.
> 
> I would also like to go in a direction where writing WPT tests is the default 
> for WebKit layout test.
> For that to happen, we need an easy way to upstream tests from WebKit to WPT 
> GitHub.
> 
> I would tend to do the following:
> - Write/submit WPT tests in WebKit as regular WebKit testharness.js layout 
> tests through bugzilla
> - At commit queue time, automatically create a PR to WPT GitHub containing 
> the changes of the WebKit patch
> - Ask committer to fix the WPT PR should there be any issue with it 
> (committer being informed of such issues through bugzilla).
> 
>> Le dim. 5 févr. 2017 à 15:42, Ryosuke Niwa <rn...@webkit.org> a écrit :
>> Hi all,
>> 
>> Background
>> 
>> Web Platform Tests is a suite of tests written for W3C specifications=,
>> and Blink, Edge, Gecko, and WebKit all run them as a part of their 
>> continuous build system.
>> 
>> Historically, each working group had their own repository for tests,
>> but with CSS WG's tests finally getting merged into the Web Platform Tests,
>> we will have a single repository with all the tests from W3C.
>> 
>> A few of us attended a meeting to discuss the future of this test suite last 
>> Monday.
>> One of the major topic was that Blink and Gecko have adopted the process to 
>> write the tests
>> using testharness.js in their own test suites, and automatically merge into 
>> Web Platform Tests
>> without having to go through another round of code review for Web Platform 
>> Tests.
>> 
>> Given we do test-driven development in WebKit, I think WebKit should do the 
>> same.
>> This will benefit other browser vendors to catch their bugs with our tests, 
>> and we will also benefit
>> from other browser vendors "reviewing" our tests against their 
>> implementation and specifications.
>> 
>> In the long term, I believe this will drastically improve the 
>> interoperability of the Web,
>> and dramatically improve the quality of the tests we run against WebKit.
>> 
>> In fact, there was a general consensus that we should even upstream 
>> pass-if-not-crash tests
>> as well as style and layout invalidation tests to web-platform-tests as they 
>> tend to be undertested
>> right now and almost all engines have bugs in this area.
>> 
>> Problems & Questions
>> 
>> In order to auto-merge tests from WebKit to Web Platform Tests, there are a 
>> fe

Re: [webkit-dev] [webkit-reviewers] usage of auto

[Chris Dumez]

I usually like using auto / auto* as much as possible.

The one exception where I have found using auto confusing was for functions 
returning an std::optional. 

E.g.
auto value = maximum();
if (!value)
return;

I find that the check is confusing because it returns early if value is 0 in 
the case where maximum() returns an integer but checks if the value is set in 
the case the function returns an std::optional.

Chris Dumez

On Jan 10, 2017, at 9:51 PM, Darin Adler <da...@apple.com> wrote:

>> On Jan 10, 2017, at 9:49 PM, Darin Adler <da...@apple.com> wrote:
>> 
>>> On Jan 10, 2017, at 9:46 PM, Simon Fraser <simon.fra...@apple.com> wrote:
>>> 
>>> auto countOfThing = getNumberOfThings();
>>> ASSERT(countOfThing >= 0);  // Can’t tell by reading whether the ASSERT is 
>>> assured at compile time if countOfThing is unsigned
>> 
>> I understand wanting to know, but I am not certain this is a bad thing.
> 
> Sorry, let me say something different, but related:
> 
>int countOfThing = getNumberOfThings();
> 
> Can’t tell from the above code if getNumberOfThings() returns int or unsigned.
> 
> — Darin
> ___
> 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] Can no longer commit manually or via CQ?

[Chris Dumez]

Hi,

I can no longer commit locally (I am using git-svn). It complains about me 
having 30 or so local commits (which is not true).

Therefore, I tried using the commit queue but it fails to commit as well:
Comment 3 <https://bugs.webkit.org/show_bug.cgi?id=158943#c3>

ailed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', 
'--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 
'land-attachment', '--force-clean', '--non-interactive', 
'--parent-command=commit-queue', 281652, '--port=mac']" exit_code: 2 cwd: 
/Volumes/Data/EWS/WebKit

Last 500 characters of output:
1 Try to fix the iOS build after r202142 and r202224.
The copy of the patch that failed is found in:
   /Volumes/Data/EWS/WebKit/.git/rebase-apply/patch

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at /Volumes/Data/EWS/WebKit/Tools/Scripts/webkitdirs.pm line 2661.

Kr,
--
 Chris Dumez




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

Re: [webkit-dev] Removing ENABLE(DATA_TRANSFER_ITEMS)

[Chris Dumez]

Hi,

It is part of the HTML specification:
https://html.spec.whatwg.org/multipage/interaction.html#the-datatransferitemlist-interface
https://html.spec.whatwg.org/multipage/interaction.html#datatransferitem

Chrome seems to support it. It is also covered by the W3C web-platform tests.
Should we work on enabling this feature rather than removing it?

Kr,
--
 Chris Dumez




> On Dec 21, 2015, at 10:02 PM, Gyuyoung Kim <gyuyoung@webkit.org> wrote:
> 
> Hello,
> 
> It looks like no ports have used a data transfer items feature. Even the 
> feature hasn't been updated since 2011. If anyone doesn't have a plan to use 
> it in future, I plan to remove it. Any objections ?
> 
> Gyuyoung. 
> ___
> 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] [Proposal] Remove support for 'multipart/x-mixed-replace' main resources

[Chris Dumez]

Hi,

I would like to suggest we remove support for 'multipart/x-mixed-replace’ main 
resources while keeping support for multipart images.

Based on Chrome usage data, this feature is extremely rarely used by Web sites 
(less than 0.1% of page loads) [1]. This feature adds complexity to the 
loader and is a source of (security) bugs (e.g. [2] recently), current support 
also seems buggy.

Current support in Safari / WebKit:
- Support is not great is WebKit. If you load a Motion JPEG main resource for 
example, it will keep creating a new ImageDocument and all its DOM tree for 
every frame (tested on Safari / Mac).
- It looks like support is broken on Safari on iOS (I tried a Motion JPEG main 
resource on iOS8, I see the first frame then a blank page that never finishes 
loading).

Other browsers:
- Never supported by IE (including IE11) for any resource
- Chrome already dropped support for this (main resources only) almost 2 years 
ago [3].
- Firefox 37 still supports this based on local testing.

Again, I am only proposing dropping support for main resources. For e.g., 
having an IMG element in a page whose src attribute points to a Motion JPEG 
would still work as intended.

[1] https://code.google.com/p/chromium/issues/detail?id=249132 
https://code.google.com/p/chromium/issues/detail?id=249132
[2] https://bugs.webkit.org/show_bug.cgi?id=143979 
https://bugs.webkit.org/show_bug.cgi?id=143979
[3] http://src.chromium.org/viewvc/blink?view=revisionrevision=152363 
http://src.chromium.org/viewvc/blink?view=revisionrevision=152363

Kr,
--
 Chris Dumez - Apple Inc. - Cupertino, CA




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

Re: [webkit-dev] Safari browser on Mac OSX complains AudioContext.createMediaStreamSource is undefined !

[Chris Dumez]

Jer or Eric would know more about this but 
AudioContext.createMediaStreamSource() is behind a MEDIA_STREAM compile-time 
flag. It appears the mac port does not turn this flag on so Safari does not 
currently support the Media Stream API.

Kr,
--
 Chris Dumez - Apple Inc. - Cupertino, CA




 On Mar 19, 2015, at 9:35 AM, Sasi San sasikumar.gan...@gmail.com wrote:
 
 Hi-
 
 I am trying to get the live audio input from microphone using AudioContext. 
 Safari browser complains that the createMediaStreamSource is undefined. 
 here is my sample of JavaScript code. It's not able to create Audio source 
 node. So I am not able to get the audio sample from microphone in the 
 OnAudioProcess event handler.
 
 var micGain = audioContext.createGain();
 if(!audioContext.createScriptProcessor){
 micAudioProcessor = audioContext.createJavaScriptNode(bufferSize, 
 1, 1);
 console.log('createJavaScriptNode Done');
 } else {
 micAudioProcessor = 
 audioContext.createScriptProcessor(bufferSize, 1, 1);
console.log('createScriptProcessor Done');
 }
 
 // Create an AudioNode from the stream.
 if(typeof(audioContext.createMediaStreamSource) === 'function') {
 micInput = audioContext.createMediaStreamSource(inAudioStream);
 micInput.connect(micGain);
 }
 else {
 console.log('method createMediaStreamSource unavailable');
 }
 
 Please let me know why Safari is complaining on this API 
 createMediaStreamSource . Is there any other way I can get the live audio 
 sample from microphone on Safari browser???
 
 Thanks,
 Sasi
 
 ___
 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] run-webkit-tests and run-perf-tests scripts now use WebKitTestRunner by default

[Chris Dumez]

Hi,

I just wanted to let you know that run-webkit-tests and run-perf-tests now use 
WebKitTestRunner by default after [1].
Passing —webkit-test-runner or -2 to these scripts is no longer needed (and no 
longer works).

If you wish to run DumpRenderTree, you should pass —dump-render-tree or -1 to 
these scripts.

Kr,
--
 Chris Dumez - Apple Inc. - Cupertino, CA


[1] https://trac.webkit.org/r181093
 https://trac.webkit.org/r181093___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Pattern for singleton classes instance getters

[Chris Dumez]

Hi,

I made the changes in:
http://trac.webkit.org/changeset/179401
http://trac.webkit.org/changeset/179409

Kr,
--
Chris Dumez - Apple Inc.
Cupertino, CA




 On Jan 29, 2015, at 10:23 AM, Chris Dumez cdu...@apple.com wrote:
 
 Hi all,
 
 Thanks for the feedback.
 
 I submitted a coding style update proposal at Bug 141040 
 https://bugs.webkit.org/show_bug.cgi?id=141040.
 Based on the mailing list feedback, the proposal is to use a static member 
 function named singleton()”.
 
 I will also upload a patch to align our existing code unless there is any 
 objection.
 
 Kr,
 --
 Chris Dumez - Apple Inc.
 Cupertino, CA
 
 
 
 
 On Jan 28, 2015, at 9:26 PM, Ryosuke Niwa rn...@webkit.org 
 mailto:rn...@webkit.org wrote:
 
 On Wed, Jan 28, 2015 at 9:19 PM, Maciej Stachowiak m...@apple.com 
 mailto:m...@apple.com wrote:
 
 This may be a question of what jargon we’ve encountered, but to me, 
 “singleton clearly means the one unique instance of this class while 
 “instance means any instance of this class. If I hadn’t seen this thread, 
 I would interpret Class::instance() to mean “create a brand new instance of 
 this class” rather than “return the unique singleton instance of this class, 
 creating if necessary.
 
 Agreed.  I also prefer Class::singleton() over Class::instance().
 
 - R. Niwa
 ___
 webkit-dev mailing list
 webkit-dev@lists.webkit.org mailto: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] Pattern for singleton classes instance getters

[Chris Dumez]

Hi all,

Thanks for the feedback.

I submitted a coding style update proposal at Bug 141040 
https://bugs.webkit.org/show_bug.cgi?id=141040.
Based on the mailing list feedback, the proposal is to use a static member 
function named singleton()”.

I will also upload a patch to align our existing code unless there is any 
objection.

Kr,
--
Chris Dumez - Apple Inc.
Cupertino, CA




 On Jan 28, 2015, at 9:26 PM, Ryosuke Niwa rn...@webkit.org wrote:
 
 On Wed, Jan 28, 2015 at 9:19 PM, Maciej Stachowiak m...@apple.com 
 mailto:m...@apple.com wrote:
 
 This may be a question of what jargon we’ve encountered, but to me, 
 “singleton clearly means the one unique instance of this class while 
 “instance means any instance of this class. If I hadn’t seen this thread, 
 I would interpret Class::instance() to mean “create a brand new instance of 
 this class” rather than “return the unique singleton instance of this class, 
 creating if necessary.
 
 Agreed.  I also prefer Class::singleton() over Class::instance().
 
 - 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] Pattern for singleton classes instance getters

[Chris Dumez]

Yes, instance() is what I’ve seen mostly outside WebKit as well. This would be 
my preference.

Kr,
--
Chris Dumez - Apple Inc.
Cupertino, CA

 On Jan 28, 2015, at 8:44 PM, Michael Catanzaro mcatanz...@igalia.com wrote:
 
 On Wed, Jan 28, 2015 at 8:11 PM, Maciej Stachowiak m...@apple.com wrote:
 Yet another possibility is finding a better name than ‘shared’ for the 
 singleton pattern function, but I don’t have any better ideas. 
 Class::getSingleton() is more explicit but the extra verbosity doesn’t seem 
 helpful to me.
 
 I recommend Class::instance(), which is what I've seen used almost 
 exclusively outside of WebKit. It's what Scott Meyers used, and it's very 
 similar to the Gang of Four's choice of Class::Instance() and the Java 
 pattern Class.INSTANCE.
 
 That said, Class::singleton() is very attractive too.
 
 I've never seen Class::shared() used anywhere except WebKit. The first time I 
 saw this I had no clue what it was until I looked up the implementation.
 
 All of these can get quite annoying to type, especially when the name of the 
 class is long. (Sometimes I will keep a reference to the instance in a local 
 variable with a shorter name.) But I think they're easier to read than free 
 functions, and we should optimize for reading code, not writing it. Not a big 
 deal either way.
 ___
 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] Pattern for singleton classes instance getters

[Chris Dumez]

Fair enough, singleton() is very explicit indeed. I would be happy with this 
naming too.

--
Chris Dumez - Apple Inc.
Cupertino, CA

 On Jan 28, 2015, at 9:19 PM, Maciej Stachowiak m...@apple.com wrote:
 
 
 This may be a question of what jargon we’ve encountered, but to me, 
 “singleton clearly means the one unique instance of this class while 
 “instance means any instance of this class. If I hadn’t seen this thread, 
 I would interpret Class::instance() to mean “create a brand new instance of 
 this class” rather than “return the unique singleton instance of this class, 
 creating if necessary.
 
 Regards,
 Maciej
 
 On Jan 28, 2015, at 8:54 PM, Chris Dumez cdu...@apple.com 
 mailto:cdu...@apple.com wrote:
 
 Yes, instance() is what I’ve seen mostly outside WebKit as well. This would 
 be my preference.
 
 Kr,
 --
 Chris Dumez - Apple Inc.
 Cupertino, CA
 
 On Jan 28, 2015, at 8:44 PM, Michael Catanzaro mcatanz...@igalia.com 
 mailto:mcatanz...@igalia.com wrote:
 
 On Wed, Jan 28, 2015 at 8:11 PM, Maciej Stachowiak m...@apple.com 
 mailto:m...@apple.com wrote:
 Yet another possibility is finding a better name than ‘shared’ for the 
 singleton pattern function, but I don’t have any better ideas. 
 Class::getSingleton() is more explicit but the extra verbosity doesn’t 
 seem helpful to me.
 
 I recommend Class::instance(), which is what I've seen used almost 
 exclusively outside of WebKit. It's what Scott Meyers used, and it's very 
 similar to the Gang of Four's choice of Class::Instance() and the Java 
 pattern Class.INSTANCE.
 
 That said, Class::singleton() is very attractive too.
 
 I've never seen Class::shared() used anywhere except WebKit. The first time 
 I saw this I had no clue what it was until I looked up the implementation.
 
 All of these can get quite annoying to type, especially when the name of 
 the class is long. (Sometimes I will keep a reference to the instance in a 
 local variable with a shorter name.) But I think they're easier to read 
 than free functions, and we should optimize for reading code, not writing 
 it. Not a big deal either way.
 ___
 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

[webkit-dev] Pattern for singleton classes instance getters

[Chris Dumez]

Hi,

I noticed that we are currently not very consistent in WebKit in the way we 
implement singleton classes instance getters.
- Some classes use free functions (like MemoryCache, and PageCache until I 
updated it yesterday). e.g. memoryCache().xxx()
- Some classes are using static functions in the class (e.g. 
DatabaseProcess::shared(), PluginProcess::shared()).

As I said, I landed a patch yesterday so that the global page cache is now 
accessed via PageCache::shared() because I thought this was the currently 
preferred pattern (given it seems very common in WebKit2 code).
However, I thought I would email webkit-dev to make sure this is actually the 
case and make sure we agree on a given pattern (one way or another) for current 
and future code. We could then maybe document this
as part of our coding style.

Any feedback on this matter?

Kr,
--
Chris Dumez - Apple Inc.
Cupertino, CA




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

Re: [webkit-dev] EFL EWS bot failures

[Chris Dumez]

FYI, this still seems to be happening:
https://webkit-queues.appspot.com/results/6182699735711744

Kr,
--
Chris Dumez - Apple Inc.
Cupertino, CA

 On Jan 9, 2015, at 6:55 AM, Gyuyoung Kim gyuyoung@webkit.org wrote:
 
 Hello Ossy,
 
 Now I'm running two machines for EFL EWS and buildbot. However one of the 
 machines has poor performance.
 It looks the poor machine has caused this problem so far. Unfortunately I'm 
 not able to replace it with new machine now.
 But let me try to replace/upgrade the poor performance machine in near future.
 
 Thank you for your effort and notification about this issue.
 
 br,
 Gyuyoung.
 
 
 On Fri, Jan 9, 2015 at 8:52 PM, Osztrogonác Csaba o...@inf.u-szeged.hu 
 mailto:o...@inf.u-szeged.hu wrote:
 Hi Gyuyoung,
 
 thanks for fixing the bots.
 
 I noticed similar problem on the EFL EWS and buildbot many times.
 Unfortunately it can cause not only orange bubbles, but false
 positive redness too.
 
 I saw the same error long long time ago locally, and the reason
 was OOM on one of an overloaded icecc slave many times. And once
 a memory module error caused the same error.
 
 Nowadays I noticed another strange error on the EFL buildbot (2 or 3
 times):  kill old processes buildstep stucked in an infinite loop
 and killed after the 20 minutes timeout for several builds.
 
 Is there any chance to check and fix/replace the hardware of
 the EFL EWS and buildbot to make it as stable as previously?
 
 br,
 Ossy
 
 Gyuyoung Kim írta:
 Hello Darin,
 
 I'm sorry for the inconvenience about that. EFL EWS seems to work again.
 
 Gyuyoung.
 
 On Fri, Jan 9, 2015 at 4:02 AM, Darin Adler da...@apple.com 
 mailto:da...@apple.com mailto:da...@apple.com mailto:da...@apple.com 
 wrote:
 
 A lot of times recently the EFL bot has reported a failure that
 shows up as an orange bubble:
 
 c++: internal compiler error: Killed (program cc1plus)
 
 Like in this bug https://bugs.webkit.org/show_bug.cgi?id=140166 
 https://bugs.webkit.org/show_bug.cgi?id=140166.
 What's going on with that? Is there someone who can fix it?
 
 --- Darin
 ___
 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

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

Re: [webkit-dev] size_t vs unsigned in WTF::Vector API ?

[Chris Dumez]

The corresponding Blink bug did contain some performance data:
CrBug#229226 https://code.google.com/p/chromium/issues/detail?id=229226

Kr,
--
Chris Dumez - Apple Inc.
Cupertino, CA




 On Nov 20, 2014, at 11:32 AM, Geoffrey Garen gga...@apple.com wrote:
 
 I wonder what the downsides are to this approach.  Footprint of Vector?
 
 It looks like the original change was motivated by shrinking Vector:
 
   https://bugs.webkit.org/show_bug.cgi?id=97268 
 https://bugs.webkit.org/show_bug.cgi?id=97268
 
 Sadly, it didn’t include any data on the observed benefit :(.
 
 Geoff
 ___
 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] size_t vs unsigned in WTF::Vector API ?

[Chris Dumez]

Hi all,

I recently started updating the WTF::Vector API to use unsigned types instead 
of size_t [1][2], because:
- WTF::Vector is already using unsigned type for size/capacity internally to 
save memory on 64-bit, causing a mismatch between the API and the internal 
representation [3]
- Some reviewers have asked me to use unsigned for loop counters iterating over 
vectors (which looks unsafe as the Vector API, e.g. size(), returns a size_t).
- I heard from Joseph that this type mismatch is forcing us (and other projects 
using WTF) to disable some build time warnings
- The few people I talked to before making that change said we should do it

However, Alexey recently raised concerns about this change. it doesn't strike 
him as the right direction. 4Gb is not much, and we should have more of WebKit 
work with the right data types, not less.”.
I did not initially realize that this change was controversial, but now that it 
seems it is, I thought I would raise the question on webkit-dev to see what 
people think about this.

Kr,
--
Chris Dumez - Apple Inc.
Cupertino, CA


[1] http://trac.webkit.org/changeset/176275 
http://trac.webkit.org/changeset/176275
[2] http://trac.webkit.org/changeset/176293 
http://trac.webkit.org/changeset/176293
[3] http://trac.webkit.org/changeset/148891 
http://trac.webkit.org/changeset/148891
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] size_t vs unsigned in WTF::Vector API ?

[Chris Dumez]

Hi,

I believe the Vector class is already checking for overflows. I looked quickly 
and it seems that when trying to allocate / reallocate the Vector buffer, we 
will call CRASH() if:
(newCapacity  std::numeric_limitsunsigned::max() / sizeof(T))

Kr, 
--
Chris Dumez - Apple Inc.
Cupertino, CA




 On Nov 19, 2014, at 1:57 PM, Geoffrey Garen gga...@apple.com wrote:
 
 I don’t haver a specific opinion on size_t vs unsigned, but I have a related 
 point:
 
 If Vector uses unsigned, then it must RELEASE_ASSERT that it does not 
 overflow unsigned when growing. 
 
 Even if most normal content allocates  4GB, exploit content surely will try 
 to allocate  4GB.
 
 Chris, maybe you can make this change in trunk, since trunk uses unsigned?
 
 Geoff
 
 On Nov 19, 2014, at 1:50 PM, Alexey Proskuryakov a...@webkit.org 
 mailto:a...@webkit.org wrote:
 
 
 That makes good sense for internal implementation, do you think that class 
 API should also use a custom type, or should it use size_t?
 
 With Vector though, I don't know how we would differentiate code paths that 
 need large allocations from ones that don't. Nearly anything that is exposed 
 as a JS API or deals with external world can hit sizes over 4Gb. That's not 
 out of reach in most scenarios, not even for resources loaded from network.
 
 - Alexey
 
 
 19 нояб. 2014 г., в 13:19, Filip Pizlo fpi...@apple.com 
 mailto:fpi...@apple.com написал(а):
 
 ArrayBuffers are very special because they are part of ECMAScript.
 
 We use unsigned for the length, because once upon a time, that would have 
 been the right type; these days the right type would be a 53-bit integer.  
 So, size_t would be wrong.  I believe that ArrayBuffers should be changed 
 to use a very special size type; probably it wouldn't even be a primitive 
 type but a class that carefully checked that you never overflowed 53 bits.
 
 -Filip
 
 
 On Nov 19, 2014, at 12:54 PM, Alexey Proskuryakov a...@webkit.org 
 mailto:a...@webkit.org wrote:
 
 
 This is not exactly about Vector, but if one uses 
 FileReader.prototype.readAsArrayBuffer() on a large file, I think that it 
 overflows ArrayBuffer. WebKit actually crashes when uploading 
 multi-gigabyte files to YouTube, Google Drive and other similar services, 
 although I haven't checked whether it's because of ArrayBuffers, or 
 because of a use of int/unsigned in another code path.
 
 I think that we should use size_t everywhere except for perhaps a few key 
 places where memory impact is critical (and of course we need explicit 
 checks when casting down to an unsigned). Or perhaps the rule can be even 
 simpler, and unsigned may never be used for indices and sizes, period.
 
 - Alexey
 
 
 19 нояб. 2014 г., в 12:32, Filip Pizlo fpi...@apple.com 
 mailto:fpi...@apple.com написал(а):
 
 Whatever we do, the clients of Vector should be consistent about what 
 type they use.  I'm actually fine with Vector internally using unsigned 
 even if the API uses size_t, but right now we have lots of code that uses 
 a mix of size_t and unsigned when indexing into Vectors.  That's 
 confusing.
 
 If I picked one type to use for all Vector indices, it would be unsigned 
 rather than size_t.  Vector being limited to unsigned doesn't imply 4GB 
 unless you do Vectorchar.  Usually we have Vectors of pointer-width 
 things, which means 32GB on 64-bit systems (UINT_MAX * sizeof(void*)).  
 Even in a world where we had more than 32GB of memory to use within a 
 single web process, I would hope that we wouldn't use it all on a single 
 Vector and that if we did, we would treat that one specially for a bunch 
 of other sensible reasons (among them being that allocating a contiguous 
 slab of virtual memory of that size is rather taxing).  So, size_t would 
 buy us almost nothing since if we had a vector grow large enough to need 
 it, we would be having a bad time already.
 
 I wonder: are there cases that anyone remembers where we have tried to 
 use Vector to store more than UINT_MAX things?  Another interesting 
 question is: What's the largest number of things that we store into any 
 Vector?  We could use such a metric to project how big Vectors might get 
 in the future.
 
 -Filip
 
 
 On Nov 19, 2014, at 12:20 PM, Chris Dumez cdu...@apple.com 
 mailto:cdu...@apple.com wrote:
 
 Hi all,
 
 I recently started updating the WTF::Vector API to use unsigned types 
 instead of size_t [1][2], because:
 - WTF::Vector is already using unsigned type for size/capacity 
 internally to save memory on 64-bit, causing a mismatch between the API 
 and the internal representation [3]
 - Some reviewers have asked me to use unsigned for loop counters 
 iterating over vectors (which looks unsafe as the Vector API, e.g. 
 size(), returns a size_t).
 - I heard from Joseph that this type mismatch is forcing us (and other 
 projects using WTF) to disable some build time warnings
 - The few people I talked to before making that change said we should do 
 it
 
 However, Alexey recently

Re: [webkit-dev] size_t vs unsigned in WTF::Vector API ?

[Chris Dumez]

If we don’t want to crash on overflow, the callers can use the try*() API I 
believe (e.g. tryAppend()). This returns false (and does not resize the vector) 
instead of crashing, when we reach the size limit.

Kr,
--
Chris Dumez - Apple Inc.
Cupertino, CA




 On Nov 19, 2014, at 2:58 PM, Alexey Proskuryakov a...@webkit.org wrote:
 
 
 19 нояб. 2014 г., в 13:58, Filip Pizlo fpi...@apple.com 
 mailto:fpi...@apple.com написал(а):
 
 With Vector though, I don't know how we would differentiate code paths that 
 need large allocations from ones that don't. Nearly anything that is 
 exposed as a JS API or deals with external world can hit sizes over 4Gb. 
 That's not out of reach in most scenarios, not even for resources loaded 
 from network.
 
 Can you provide an example?
 
 Yes. XMLHttpRequest::m_binaryResponseBuilder keeps the downloaded data in a 
 Vector, so any time there is much data, something bad will happen. This is a 
 case that we should support, and not just crash as we would when we think 
 that only exploits would try to use as much memory.
 
 All code that is Blob related also uses Vectors, and of course Blobs can 
 legitimately be large.
 
 Crypto code uses Vectors internally for the data.
 
 These and related uses are all over the place - see also Vectors in 
 FormDataBuilder, data returned from FrameLoader::loadResourceSynchronously, 
 plug-in code that loads from network, SharedBuffer etc.
 
 - Alexey
 

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

[webkit-dev] Transition to is() / downcast() is complete

[Chris Dumez]

Hi all,

I just wanted to let you know that I have just landed the last patch porting 
the code base to using is() / downcast().
I have managed to remove the NODE_TYPE_CASTS() / TYPE_CASTS_BASE() macros so 
there should be no code using it anymore.

If you need to add is() / downcast() support for a specific class, please 
use the SPECIALIZE_TYPE_TRAITS_*() macro that is in
wtf/TypeCasts.h. Note however, that the template specializations are already 
generated for most HTML/SVG/MathML elements by
default so you rarely need manual traits specialization for those.

If you find remaining toXXX() casting functions in the code, please let me know 
as we don’t want to mix toXXX() and downcast().
Do let me know if you have improvement suggestions as well. One that is on my 
TODO list already is to support is(RefPtr), there are
not that many call sites that would benefit from this but it would be nice IMHO.

Kr,
--
Chris Dumez - Apple Inc.
Cupertino, CA

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

[webkit-dev] Behavior change: is(T*) is now doing a null check on the pointer argument

[Chris Dumez]

Hi,

I would like to bring attention to a behavior change I have just landed [1]. 
The is(T*) type checking function is now doing a null-check on the pointer 
argument instead of simply asserting that the pointer argument should not be 
null.

Please keep this in mind in your future patches:
- If you know the pointer cannot be null (because a null check was done earlier 
in the function), then call the is(T) overload taking a reference in 
argument to avoid the null check: e.g. “if (isHTMLDivElement(*node)) …
- Do not do an explicit null check on the pointer before calling is(T*). “if 
(node  isHTMLDivElement(node))” - if (isHTMLDivElement(node))

This change was made because:
- It simplifies the code a bit as it avoids a lot of explicit null checks.
- It is more consistent with downcast(T*) which will correctly cast a null 
pointer.
- Having a is(T*) overload in addition to is(T) was previously only 
helpful to avoid having to dereference the pointer. Dereferencing is just one 
‘*’ character and doesn’t decrease code readability much IMHO. However, getting 
rid of a lot of explicit null checks does simplify the code quite a bit.

FYI, all Node subclasses should now support is() / downcast() already. I am 
in the process of porting non-Node classes over as well for consistency 
(WorkerGlobalScope class, File class and Event subclasses were already ported 
for e.g.).
If you try to use is() / downcast() for a class that doesn’t support it 
yet, you should now get a clear build error (instead of the obscure linking 
error we would get previously).

Kr,
--
Chris Dumez - Apple Inc.
Cupertino, CA

[1]  http://trac.webkit.org/changeset/174225 
http://trac.webkit.org/changeset/174225___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

[webkit-dev] Type checking / casting helpers

[Chris Dumez]

Hi all,

I started working on automatically generating the type casting helpers for 
HTML/SVG/MathML Elements (e.g. toHTMLDivElement()). Until now, we were 
generating only the type checking helpers using make_names.pl (e.g. 
isHTMLDivElement()). The type casting helpers had to be manually defined using 
NODE_TYPE_CASTS() macro.

The type casting helpers are now automatically generated for most types. Part 
of the solution involved using a templated function for type casting because 
the types are forward-declared and we needed to do a static_cast() (a 
reinterpret_cast() could be used with forward declarations but wouldn’t be 
safe due to multiple inheritance).

I initially had macros in place so that toHTMLDivElement() would still work and 
would be equivalent to downcastHTMLDivElement(). The feedback I received is 
that we should get rid of these macros and just use isHTMLDivElement() / 
downcastHTMLDivElement() everywhere.
The new style is very close to C++’s is_classT() and Boost’s 
polymorphic_downcastT().

I actually started updating the code to do this but I should have emailed 
webkit-dev about this beforehand. I apologize for sending this message a bit 
late.

Please let me know if you have feedback / concerns / questions about this 
change. I hope that this email gives you a better understanding of why I am 
making this change.

As I said before, the code base is not fully ported yet so the current 
situation is not necessarily pretty. I will try and go through the transition 
as fast as I can, provided that people don’t raise any concerns about this.

Please also note that these new helpers still catch unnecessary type checks / 
casts. As a matter of fact, those are now caught at build time instead of 
linking time and should give you a nice “Unnecessary type check” / “Unnecessary 
type cast” static assertion.

Also note that the plan is to get rid of TYPE_CAST_BASE() macro entirely and 
extend is() / downcast() to all types, not just Nodes.

Kr,
--
Chris Dumez - Apple Inc.
Cupertino, CA




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