Re: [webkit-dev] maybe_unused vs UNUSED_PARAM

[Ryosuke Niwa via webkit-dev]

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

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

[Ryosuke Niwa via webkit-dev]

I don't think non-reviewers should be able to reject/block PRs as well.- R. NiwaOn Nov 29, 2023, at 07:45, Chris Dumez via webkit-dev  wrote:FYI, our official documentation on 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 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] Smart Pointers Usage Guidelines

[Ryosuke Niwa via webkit-dev]

> On Aug 21, 2023, at 4:51 PM, Ryosuke Niwa via webkit-dev 
>  wrote:
> 
>> On Aug 21, 2023, at 4:50 PM, Tim Horton  wrote:
>> 
>>> On Aug 21, 2023, at 4:42 PM, Ryosuke Niwa  wrote:
>>> 
>>>> On Aug 21, 2023, at 4:41 PM, Darin Adler  wrote:
>>>> 
>>>>> On Aug 21, 2023, at 4:39 PM, Ryosuke Niwa  wrote:
>>>>> 
>>>>> Alternatively, we could add a new member function which returns 
>>>>> CheckedPtr like `pageChecked()`.
>>>> 
>>>> Yes, I think that would be a good approach that would complement the 
>>>> static checker.
>>> 
>>> Okay, let’s go with this solution since others have expressed concerns for 
>>> ref churns in the past.
>> 
>> So, to be clear, this example:
>> 
>>> page()->document()->foo()
>> 
>> 
>> would become:
>> 
>>> page()->documentChecked()->foo()?
>> 
>> (only `checked` for the deepest getter before the complex call)?
> 
> Yes.

Now that I’m thinking about this more, we should probably call this 
checkedDocument() to be aligned with protectedX pattern.

- R. Niwa

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

Re: [webkit-dev] Smart Pointers Usage Guidelines

[Ryosuke Niwa via webkit-dev]

> On Aug 21, 2023, at 4:50 PM, Tim Horton  wrote:
> 
> 
> 
>> On Aug 21, 2023, at 4:42 PM, Ryosuke Niwa  wrote:
>> 
>> 
>> 
>>> On Aug 21, 2023, at 4:41 PM, Darin Adler  wrote:
>>> 
>>>> On Aug 21, 2023, at 4:39 PM, Ryosuke Niwa  wrote:
>>>> 
>>>> Alternatively, we could add a new member function which returns CheckedPtr 
>>>> like `pageChecked()`.
>>> 
>>> Yes, I think that would be a good approach that would complement the static 
>>> checker.
>> 
>> Okay, let’s go with this solution since others have expressed concerns for 
>> ref churns in the past.
> 
> So, to be clear, this example:
> 
>> page()->document()->foo()
> 
> 
> would become:
> 
>> page()->documentChecked()->foo()?
> 
> (only `checked` for the deepest getter before the complex call)?

Yes.

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

Re: [webkit-dev] Smart Pointers Usage Guidelines

[Ryosuke Niwa via webkit-dev]

> On Aug 21, 2023, at 4:41 PM, Darin Adler  wrote:
> 
>> On Aug 21, 2023, at 4:39 PM, Ryosuke Niwa  wrote:
>> 
>> Alternatively, we could add a new member function which returns CheckedPtr 
>> like `pageChecked()`.
> 
> Yes, I think that would be a good approach that would complement the static 
> checker.

Okay, let’s go with this solution since others have expressed concerns for ref 
churns in the past.

- R. Niwa

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

Re: [webkit-dev] Smart Pointers Usage Guidelines

[Ryosuke Niwa via webkit-dev]

> On Aug 21, 2023, at 4:34 PM, Darin Adler  wrote:
> 
> 
>> On Aug 21, 2023, at 4:31 PM, Tim Horton via webkit-dev 
>>  wrote:
>> 
>>> One subtle thing is that even when a member variable is already Ref / 
>>> RefPtr / CheckedRef / CheckedPtr, we must create another one in stack as 
>>> seen here:
>>> https://commits.webkit.org/267108@main
>> 
>> (I asked rniwa to send this mail because this patch surprised me, so I hope 
>> now we can chat about it).
>> 
>> The scope of this rule, and the … lack of elegance … at so many callsites 
>> worries me a bit. If it’s possible to automate enforcement, that might help 
>> with part of the problem, but it’s also just really not very pretty, and I 
>> wonder if someone can come up with some clever alternative solution before 
>> we go too far down this path (not me!).
>> 
>> Alternatively, it’s possible other people OK with this syntax/requirement 
>> and I should just get over it. What do you all think?
> 
> I hope we can make this better by using a getter function that returns a 
> CheckedPtr so instead of writing CheckedPtr { _page }, we would write page().

One drawback making a member function return CheckedPtr is that then code like 
this: `page()->document()->foo()` would cause a ref churn.

Maybe we don’t care about such ref churns? But then a simpler rule to deploy 
will be that every function must return CheckedRef/CheckedPtr/Ref/RefPtr.

Alternatively, we could add a new member function which returns CheckedPtr like 
`pageChecked()`.

- R. Niwa

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

[webkit-dev] Smart Pointers Usage Guidelines

[Ryosuke Niwa via webkit-dev]

Hi all,

It has been a while since I last announced the plan to adopt smart pointers 
using clang static analyzer:
https://lists.webkit.org/pipermail/webkit-dev/2020-September/031386.html

Here are some updates.


1. We’ve made a progress in implementing all the rules including rules for 
local variables in clang static analyzer:
https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Guidelines#rules-for-using-ref-counted-objects


2. We also have a new kind of smart pointers: CheckedRef 
 / 
CheckedPtr 
. These 
behave like Ref and RefPtr in that they increment & decrement a counter in an 
object but unlike them don’t extend the lifetime of the object. Instead, the 
destructor of the base object release asserts that there are no live CheckedRef 
/ CheckedPtr left.

I added a new section in the guide describing when to use each smart pointer 
type:
https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Guidelines#when-to-use-which-smart-pointer


3. I wanted to describe what applying these smart pointer rules mean. A lot of 
code changes needed for this work involves creating Ref / RefPtr / CheckedRef / 
CheckedPtr in stack:
https://commits.webkit.org/267082@main

One subtle thing is that even when a member variable is already Ref / RefPtr / 
CheckedRef / CheckedPtr, we must create another one in stack as seen here:
https://commits.webkit.org/267108@main

This is because these member variables can be cleared during the course of 
invoking a non-trivial function; or put it another way, it’s not immediately 
obvious from the code inspection that the object pointed to stays alive over 
the course of a non-trivial function call.

- R. Niwa

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

Re: [webkit-dev] Removal of trailing whitespace

[Ryosuke Niwa via webkit-dev]

> On Apr 12, 2023, at 12:34 PM, Chris Dumez  wrote:
> 
>> 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.

Yeah, it’s not great that PR got landed. In the future, it would be good to 
hold off landing these code changes until the discussion settles.

- 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

[Ryosuke Niwa via webkit-dev]

Yeah, enforcing that new or otherwise modified lines don’t have trailing 
whitespaces would be good.

- 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

Re: [webkit-dev] Removal of trailing whitespace

[Ryosuke Niwa via webkit-dev]

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

Re: [webkit-dev] WebKit Documentation

[Ryosuke Niwa via webkit-dev]

> On Mar 9, 2023, at 10:48 AM, Brandon Stewart via webkit-dev 
>  wrote:
> 
> Sorry for the delay on the documentation update.
> 
> For getting the documentation system online, we were trying to settle on a 
> subdomain.
> 
> What would people prefer we use as the official documentation location:
> (1) docs.webkit.org
> (2) developer.webkit.org
> (3) documentation.webkit.org

(1) or (3) seems good.

- R. Niwa

>> On Feb 28, 2023, at 12:39 AM, dpino via webkit-dev 
>>  wrote:
>> On 9/20/22 05:28, Brandon Stewart via webkit-dev wrote:
>> 
>>> Hi WebKit Developers,
>>> 
>>> Documentation is an important part of any open source project, especially 
>>> for a larger project like WebKit. Being able to ramp up during the 
>>> onboarding process, reading up on architectural decisions, and learning how 
>>> to perform common procedures are all features the documentation should 
>>> tackle. WebKit has a large set of documentation already, but it is 
>>> scattered around a wide range of platforms (Trac, GitHub Wiki, markdown 
>>> files in the code, Git commits, etc...), and some of the information is out 
>>> of date.
>>> 
>>> A few months ago, I started working on a new documentation solution based 
>>> on the DocC documentation framework. This provides an easy way to add and 
>>> edit documentation through markdown files. I have already ported a large 
>>> section of Trac, all of the GitHub Wiki, and all of the non third party 
>>> markdown files in the code over to this platform. I have tested this on 
>>> macOS and Linux and have found it works extremely well. (Windows should be 
>>> able to use WSL2 at the moment, while a few remaining issues get sorted 
>>> out). The only dependency for this project is a recent installation of 
>>> Swift.
>>> 
>>> You can already download the documentation and preview it locally, but we 
>>> are looking to publish it online for easy viewing. We were looking to get 
>>> your feedback if we would want to publish the documentation on GitHub Pages 
>>> or webkit.org.
>>> 
>>> The documentation source can be found at 
>>> https://github.com/webkit/documentation.
>>> 
>>> - Brandon
>>> ___
>>> webkit-dev mailing list
>>> webkit-dev@lists.webkit.org 
>>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>> H,
>> 
>> It has been more than 5 months since this new documentation repository was 
>> announced and as far as I know it hasn't been published yet. Is there any 
>> news when this site will be publicly available?
>> 
>> Right now in Igalia we need to publish some information publicly and we were 
>> discussing where it's the right place to put it.
>> 
>>   * WebKit Documentation (https://github.com/webkit/documentation.git), IMHO 
>> it doesn't make sense because the information needs to be public.
>> 
>>   * Trac (https://trac.webkit.org/wiki), since WebKit Documentation hasn't 
>> been published yet, the fact is that we have been updating Trac pages in the 
>> last months.
>> 
>>   * WebKit GitHub Wiki (https://github.com/WebKit/WebKit/wiki), it seems 
>> like the best candidate. It says that new documentation should live there 
>> instead of being added to Trac. But on the other hand when WebKit 
>> Documentation was announced some people advocated for keep using the GH Wiki 
>> but that idea was discarded. It seems like WebKit GitHub Wiki is were new 
>> documents should be published until WebKit Documentation site is not 
>> available? Please confirm whether this assumption is correct or not.
>> 
>> --
>> 
>> Diego
>> 
>> ___
>> 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] Smart Pointer Analysis Tool for WebKit

[Ryosuke Niwa via webkit-dev]

> On Jan 30, 2023, at 6:29 PM, Fujii Hironori via webkit-dev 
>  wrote:
> 
> On Tue, Jan 31, 2023 at 8:28 AM Ryosuke Niwa via webkit-dev 
>  wrote:
>> 
>> I’ve posted https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Rules
>> 
>> 
> Very nice.  Can I add the following exception for 
> webkit.UncountedLambdaCapturesChecker rule?
> 
> https://lists.webkit.org/pipermail/webkit-dev/2020-September/031405.html
> 
> > We probably also need to figure out a way to exempt all lambda functions
> > that never get stored anywhere. We have a bunch of helper functions like
> > WTF::map which just calls lambdas on each item while iterating over an
> > array, etc... and there is no need to create a separate Ref / RefPtr in
> > those cases since lambdas are never stored and re-used later.

Done.

- R. Niwa

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

Re: [webkit-dev] Smart Pointer Analysis Tool for WebKit

[Ryosuke Niwa via webkit-dev]

> On Jan 30, 2023, at 12:11 PM, Ryosuke Niwa via webkit-dev 
>  wrote:
> 
>> On Jan 30, 2023, at 10:47 AM, Alexey Proskuryakov  wrote:
>> 
>> Reviving this old thread, reading it again made me wish for two things:
>> 
>> - a wiki document that describes what we are trying to do, not just a thread 
>> which patches the proposal with clarifications;
> 
> Yeah, let me go make one.

I’ve posted https://github.com/WebKit/WebKit/wiki/Smart-Pointer-Usage-Rules

>> - a discussion of why we can postpone figuring out what to do with 
>> containers (like Vector or HashMap> RenderFragmentContainer*>).
> 
> This was probably an oversight on my part. The intention is to make member 
> variables / local variables of container type should also be using smart 
> pointers in its type: e.g. Vector> instead of Vector. 
> WeakHashMap> instead of 
> HashMap. I’ll try to clarify this in 
> the new doc I make.

Fixed in the new wiki documentation.

- R. Niwa

>>> 23 сент. 2020 г., в 1:54 PM, Jan Korous  написал(а):
>>> 
>>> Hi all,
>>> 
>>> I am an engineer at Security Tools team at Apple responsible for the 
>>> tooling support for this effort.
>>> 
>>>> On Sep 23, 2020, at 12:34 PM, Darin Adler  wrote:
>>>> 
>>>>> On Sep 23, 2020, at 12:18 PM, Ryosuke Niwa  wrote:
>>>>> 
>>>>> There are quite a few cases where data members are references but then 
>>>>> those can also be replaced by a simple member function which retrieves 
>>>>> the value of the smart pointer member variable and returns a reference.
>>>> 
>>>> I think this should be an explicit recommendation in the project of 
>>>> refactoring to follow these rules.
>>>> 
>>>>> For now, a trivial function is defined as a member function defined in 
>>>>> the class declaration whose definition simply returns a member variable 
>>>>> (the result of get() or a copy if the member variable is a smart pointer).
>>>> 
>>>> That seems like a rule that’s too narrow. I would not want a function to 
>>>> become non-trivial just because I moved it from being inline within the 
>>>> class definition to an inline below the class definition in the same 
>>>> header.
>>>> 
>>>> This rule worries me a lot right now; it seems like it could result in an 
>>>> explosion of local variable copies of arguments.
>>>> 
>>>>> We probably also need to figure out a way to exempt all lambda functions 
>>>>> that never get stored anywhere. We have a bunch of helper functions like 
>>>>> WTF::map which just calls lambdas on each item while iterating over an 
>>>>> array, etc... and there is no need to create a separate Ref / RefPtr in 
>>>>> those cases since lambdas are never stored and re-used later.
>>>> 
>>>> Does seem important. I am pretty sure I have seen this concept in other 
>>>> languages. We often try to use const Function& for one type of lambda 
>>>> argument and Function&& for the other type, but that’s far from complete.
>>> 
>>> Re: lambda captures - I think we have two ideas here.
>>> 
>>> 1. Allow WeakPtr captures.
>>> This makes sense to do but it implies we have to add the notion of 
>>> ownership to the rules. The thing is that WeakPtr is safe on its own (and 
>>> technically reference-counted) but it can’t be used as a safety measure in 
>>> the way of RefPtr or Ref since it doesn’t own the memory (not even in a 
>>> shared manner).
>>> 
>>> 2. Allow raw pointer/reference captures.
>>> This makes sense given you use generic algorithms in the codebase. I will 
>>> implement a new version of the checker - currently it is still based on 
>>> simple AST analysis and for this kind of reasoning we’ll need to use 
>>> symbolic execution in Clang Static Analyzer.
>>> 
>>> Thanks,
>>> 
>>> Jan
>>> 
>>>> — 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] Smart Pointer Analysis Tool for WebKit

[Ryosuke Niwa via webkit-dev]

> On Jan 30, 2023, at 10:47 AM, Alexey Proskuryakov  wrote:
> 
> Reviving this old thread, reading it again made me wish for two things:
> 
> - a wiki document that describes what we are trying to do, not just a thread 
> which patches the proposal with clarifications;

Yeah, let me go make one.

> - a discussion of why we can postpone figuring out what to do with containers 
> (like Vector or HashMap).

This was probably an oversight on my part. The intention is to make member 
variables / local variables of container type should also be using smart 
pointers in its type: e.g. Vector> instead of Vector. 
WeakHashMap> instead of 
HashMap. I’ll try to clarify this in the 
new doc I make.

- R. Niwa

>> 23 сент. 2020 г., в 1:54 PM, Jan Korous  написал(а):
>> 
>> Hi all,
>> 
>> I am an engineer at Security Tools team at Apple responsible for the tooling 
>> support for this effort.
>> 
>>> On Sep 23, 2020, at 12:34 PM, Darin Adler  wrote:
>>> 
>>>> On Sep 23, 2020, at 12:18 PM, Ryosuke Niwa  wrote:
>>>> 
>>>> There are quite a few cases where data members are references but then 
>>>> those can also be replaced by a simple member function which retrieves the 
>>>> value of the smart pointer member variable and returns a reference.
>>> 
>>> I think this should be an explicit recommendation in the project of 
>>> refactoring to follow these rules.
>>> 
>>>> For now, a trivial function is defined as a member function defined in the 
>>>> class declaration whose definition simply returns a member variable (the 
>>>> result of get() or a copy if the member variable is a smart pointer).
>>> 
>>> That seems like a rule that’s too narrow. I would not want a function to 
>>> become non-trivial just because I moved it from being inline within the 
>>> class definition to an inline below the class definition in the same header.
>>> 
>>> This rule worries me a lot right now; it seems like it could result in an 
>>> explosion of local variable copies of arguments.
>>> 
>>>> We probably also need to figure out a way to exempt all lambda functions 
>>>> that never get stored anywhere. We have a bunch of helper functions like 
>>>> WTF::map which just calls lambdas on each item while iterating over an 
>>>> array, etc... and there is no need to create a separate Ref / RefPtr in 
>>>> those cases since lambdas are never stored and re-used later.
>>> 
>>> Does seem important. I am pretty sure I have seen this concept in other 
>>> languages. We often try to use const Function& for one type of lambda 
>>> argument and Function&& for the other type, but that’s far from complete.
>> 
>> Re: lambda captures - I think we have two ideas here.
>> 
>> 1. Allow WeakPtr captures.
>> This makes sense to do but it implies we have to add the notion of ownership 
>> to the rules. The thing is that WeakPtr is safe on its own (and technically 
>> reference-counted) but it can’t be used as a safety measure in the way of 
>> RefPtr or Ref since it doesn’t own the memory (not even in a shared manner).
>> 
>> 2. Allow raw pointer/reference captures.
>> This makes sense given you use generic algorithms in the codebase. I will 
>> implement a new version of the checker - currently it is still based on 
>> simple AST analysis and for this kind of reasoning we’ll need to use 
>> symbolic execution in Clang Static Analyzer.
>> 
>> Thanks,
>> 
>> Jan
>> 
>>> — 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] Unsigned to avoid negative values

[Ryosuke Niwa via webkit-dev]

> On Jan 24, 2023, at 2:00 AM, Myles Maxfield via webkit-dev 
>  wrote:
> 
> I recently learned that the C++ core guidelines recommend against using 
> unsigned to avoid negative values. Section 4.4 on page 73 of The C++ 
> Programming Language says unsigned types should be used for bitfields and not 
> in an attempt to ensure values are positive. Some talks by people on the C++ 
> standards committee (e.g., Herb Sutter) recommend against using unsigned 
> types simply because the value is expected to by positive.
> 
> Should we be avoiding unsigneds for these purposes? WebKit uses unsigneds all 
> over the place, and I’m assuming a fair many of them are there to indicate 
> that negative values are avoided. The C++ recommendation goes against my 
> intuition that the type is there for clarity, to indicate expectations about 
> the meaning and behavior of its value. But if it’s standard practice to just 
> use int instead, perhaps we should update the style guide?
> 
> What do you think?

I don’t think we should change our coding style guidelines just because C++ 
core guideline says something.

- R. Niwa

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

Re: [webkit-dev] Stop Using Raw Pointers & References in New Code

[Ryosuke Niwa via webkit-dev]

> On Jan 24, 2023, at 5:30 AM, Alicia Boya García via webkit-dev 
>  wrote:
> 
> On 12/01/2023 06.21, Ryosuke Niwa via webkit-dev wrote:
>> I suggest we stop using raw pointers and references in any local or heap 
>> stored variable.
> I don't think this is feasible for code using non-WebKit libraries, and would 
> expect it to not apply to such cases.
> 
> For instance, many GLib and GStreamer objects are refcounted, and we even 
> have smart pointers for them in WebKit: GRefPtr, which refs/unref using RAII, 
> and GUniquePtr, which frees GLib non-refcounted objects.
> 
> We use both GRefPtr/GUniquePtr and raw pointers, but the meaning of each is 
> different: 
> 
> GRefPtr and GUniquePtr mean the code owns a reference, and will unref/free 
> when the variable goes out of scope. Raw pointer means the code is borrowing 
> a reference. The code will do something with the object, then leave it, 
> without establishing ownership of a reference to it. This is especially the 
> case for const raw pointers, which you are meant to use within scope and are 
> not allowed to free() them, as you are just borrowing them.
> 
That’s also the semantics of Ref/RefPtr in WebKit. But we’re expanding the use 
of Ref/RefPtr to be beyond just owners for memory safety. I don’t see how the 
situation is any different with GRefPtr/GUniquePtr. If an explicit ownership 
isn’t appropriate, then CheckedPtr/CheckedRef should be used instead.

- R. Niwa

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

Re: [webkit-dev] Stop Using Raw Pointers & References in New Code

[Ryosuke Niwa via webkit-dev]

> On Jan 13, 2023, at 11:06 AM, Ryosuke Niwa via webkit-dev 
>  wrote:
> 
>> On Jan 13, 2023, at 6:39 AM, Darin Adler  wrote:
>> 
>> I don’t think I can notice these patterns. I would be able to program this 
>> way if I had an analysis tool, but otherwise this simply seems too 
>> complicated.
> 
> Yeah, I agree.
> 
>> I can’t see the types of intermediate values to know if they are CheckedPtr 
>> or RefPtr or raw pointers or whatever. A variation on the first line in 
>> Element::clientLeft is a good example:
>> 
>>  document()->updateLayoutIgnorePendingStylesheets();
>> 
>> That looks perfectly good to me, nothing makes me think “the result of 
>> document is an unsafe reference to a heap-allocated object and only trivial 
>> functions can be called on that”; I should not need to be an expert on each 
>> function to be able to tell if code is correct or not. How can we choose a 
>> programming style where something simple like that is subtly wrong and 
>> dangerous and requires reviewers to look out for it?
>> 
>> I understand that if properly supported by a tool we could adapt to this and 
>> write code a whole new way. Moving to this style without a tool would almost 
>> literally require me to stop working on WebKit because I couldn’t correctly 
>> write or review even a short function.
> 
> I acknowledge that we can’t move to this new model today. So what I’m 
> suggesting is to adopt a smaller scope now: Use smart pointers in all local 
> variables and heap allocated values. This will not be 100% security proof 
> (because of the function return values) but it’s a much better point to get 
> to than where we are now.

Here’s a PR to make the style checker look for raw pointers & references in 
data members: https://github.com/WebKit/WebKit/pull/8907

I suggest we land this PR in 5 business days from now on unless someone objects.

- R. Niwa

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

Re: [webkit-dev] Stop Using Raw Pointers & References in New Code

[Ryosuke Niwa via webkit-dev]

> On Jan 13, 2023, at 6:39 AM, Darin Adler  wrote:
> 
> I don’t think I can notice these patterns. I would be able to program this 
> way if I had an analysis tool, but otherwise this simply seems too 
> complicated. 

Yeah, I agree.

> I can’t see the types of intermediate values to know if they are CheckedPtr 
> or RefPtr or raw pointers or whatever. A variation on the first line in 
> Element::clientLeft is a good example:
> 
>   document()->updateLayoutIgnorePendingStylesheets();
> 
> That looks perfectly good to me, nothing makes me think “the result of 
> document is an unsafe reference to a heap-allocated object and only trivial 
> functions can be called on that”; I should not need to be an expert on each 
> function to be able to tell if code is correct or not. How can we choose a 
> programming style where something simple like that is subtly wrong and 
> dangerous and requires reviewers to look out for it?
> 
> I understand that if properly supported by a tool we could adapt to this and 
> write code a whole new way. Moving to this style without a tool would almost 
> literally require me to stop working on WebKit because I couldn’t correctly 
> write or review even a short function.

I acknowledge that we can’t move to this new model today. So what I’m 
suggesting is to adopt a smaller scope now: Use smart pointers in all local 
variables and heap allocated values. This will not be 100% security proof 
(because of the function return values) but it’s a much better point to get to 
than where we are now.

- R. Niwa

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

Re: [webkit-dev] Stop Using Raw Pointers & References in New Code

[Ryosuke Niwa via webkit-dev]

> On Jan 12, 2023, at 9:05 PM, Ryosuke Niwa via webkit-dev 
>  wrote:
> 
> 
>> On Jan 12, 2023, at 8:37 PM, Darin Adler  wrote:
>> 
>>> On Jan 12, 2023, at 9:35 PM, Ryosuke Niwa  wrote:
>>> 
>>> One alternative is to make bar() return RefPtr although that would be a bit 
>>> heavy handed in the case of trivial function calls like this: 
>>> document().frame()->ownerElement()
>> 
>> I don’t quite follow. You just said that all arguments including this need 
>> to have RefPtr or something like it.
> 
> Right. The caller should keep an object alive.
> 
>> What makes it OK to call ownerElement without a RefPtr? What is a “trivial 
>> function” and how can we tell which functions are the trivial ones?
> 
> We made an exception to “trivial functions” to allow chaining like the one 
> above possible. A trivial function is any inlined accessor of a member 
> variable the form: Document& document() { return m_document.get(); } or 
> Frame* frame() { return m_frame.get(); }
> 
> Anything more complicated than that, or for any out-of-lined functions, we 
> should be storing “this” on a smart pointer type.
> 
> 
> Here’s an example:
> static void printNavigationErrorMessage(Frame& frame, const URL& activeURL, 
> const char* reason)
> {
> String message = "Unsafe JavaScript attempt to initiate navigation for 
> frame with URL '" + frame.document()->url().string() + "' from frame with URL 
> '" + activeURL.string() + "'. " + reason + "\n";
> 
> // FIXME: should we print to the console of the document performing the 
> navigation instead?
> frame.document()->domWindow()->printErrorMessage(message);
> }
> 
> This function, under our rule, should be rewritten like this:
> static void printNavigationErrorMessage(Frame& frame, const URL& activeURL, 
> const char* reason)
> {
> String message = "Unsafe JavaScript attempt to initiate navigation for 
> frame with URL '" + frame.document()->url().string() + "' from frame with URL 
> '" + activeURL.string() + "'. " + reason + "\n";
> 
> // FIXME: should we print to the console of the document performing the 
> navigation instead?
> RefPtr { frame.document()->domWindow() }->printErrorMessage(message);
> }
> 
> Here, it’s okay to call document() without first storing “frame” in a smart 
> pointer because it is a function argument. By transitive property, the caller 
> of this function should be keeping the frame object alive. It’s okay to call 
> domWindow() because it’s a trivial accessor function of the form:
> 
> DOMWindow* domWindow() const { return m_domWindow.get(); }
> 
> But it’s not okay to call printErrorMessage without first storing DOMWindow 
> in a RefPtr because it’s a non-trivial function, and domWindow() was not an 
> argument to this function so there is no caller to guarantee the lifetime of 
> it.
> 
> 
> Here’s another example:
> int Element::clientLeft()
> {
> document().updateLayoutIgnorePendingStylesheets();
> 
> if (auto* renderer = renderBox()) {
> auto clientLeft = LayoutUnit { roundToInt(renderer->clientLeft()) };
> return 
> convertToNonSubpixelValue(adjustLayoutUnitForAbsoluteZoom(clientLeft, 
> *renderer).toDouble());
> }
> return 0;
> }
> 
> This function should be rewritten to something like this assuming 
> RenderObject is updated to support CheckedPtr:
> int Element::clientLeft()
> {
> Ref { document() }->updateLayoutIgnorePendingStylesheets();
> 
> if (CheckedPtr renderer = renderBox()) {
> auto clientLeft = LayoutUnit { roundToInt(renderer->clientLeft()) };
> return 
> convertToNonSubpixelValue(adjustLayoutUnitForAbsoluteZoom(clientLeft, 
> *renderer).toDouble());
> }
> return 0;
> }
> 
> Here, we must store document() in Ref before calling 
> updateLayoutIgnorePendingStylesheets because 
> updateLayoutIgnorePendingStylesheets is a non-trivial function, and 
> document() is not one of the arguments. Similarly, we need to store 
> renderBox() in a smart pointer since clientLeft() is a non-trivial function. 
> But we don’t have to store “this” in Ref/RefPtr before calling 
> convertToNonSubpixelValue because “this” is an implicit function argument.

Oops, correction. convertToNonSubpixelValue is a static function which takes 
double & LegacyCSSOMElementMetricsRoundingStrategy so this is irrelevant there.

- R. Niwa

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

Re: [webkit-dev] Stop Using Raw Pointers & References in New Code

[Ryosuke Niwa via webkit-dev]

> On Jan 12, 2023, at 8:37 PM, Darin Adler  wrote:
> 
>> On Jan 12, 2023, at 9:35 PM, Ryosuke Niwa  wrote:
>> 
>> One alternative is to make bar() return RefPtr although that would be a bit 
>> heavy handed in the case of trivial function calls like this: 
>> document().frame()->ownerElement()
> 
> I don’t quite follow. You just said that all arguments including this need to 
> have RefPtr or something like it.

Right. The caller should keep an object alive.

> What makes it OK to call ownerElement without a RefPtr? What is a “trivial 
> function” and how can we tell which functions are the trivial ones?

We made an exception to “trivial functions” to allow chaining like the one 
above possible. A trivial function is any inlined accessor of a member variable 
the form: Document& document() { return m_document.get(); } or Frame* frame() { 
return m_frame.get(); }

Anything more complicated than that, or for any out-of-lined functions, we 
should be storing “this” on a smart pointer type.


Here’s an example:
static void printNavigationErrorMessage(Frame& frame, const URL& activeURL, 
const char* reason)
{
String message = "Unsafe JavaScript attempt to initiate navigation for 
frame with URL '" + frame.document()->url().string() + "' from frame with URL 
'" + activeURL.string() + "'. " + reason + "\n";

// FIXME: should we print to the console of the document performing the 
navigation instead?
frame.document()->domWindow()->printErrorMessage(message);
}

This function, under our rule, should be rewritten like this:
static void printNavigationErrorMessage(Frame& frame, const URL& activeURL, 
const char* reason)
{
String message = "Unsafe JavaScript attempt to initiate navigation for 
frame with URL '" + frame.document()->url().string() + "' from frame with URL 
'" + activeURL.string() + "'. " + reason + "\n";

// FIXME: should we print to the console of the document performing the 
navigation instead?
RefPtr { frame.document()->domWindow() }->printErrorMessage(message);
}

Here, it’s okay to call document() without first storing “frame” in a smart 
pointer because it is a function argument. By transitive property, the caller 
of this function should be keeping the frame object alive. It’s okay to call 
domWindow() because it’s a trivial accessor function of the form:

DOMWindow* domWindow() const { return m_domWindow.get(); }

But it’s not okay to call printErrorMessage without first storing DOMWindow in 
a RefPtr because it’s a non-trivial function, and domWindow() was not an 
argument to this function so there is no caller to guarantee the lifetime of it.


Here’s another example:
int Element::clientLeft()
{
document().updateLayoutIgnorePendingStylesheets();

if (auto* renderer = renderBox()) {
auto clientLeft = LayoutUnit { roundToInt(renderer->clientLeft()) };
return 
convertToNonSubpixelValue(adjustLayoutUnitForAbsoluteZoom(clientLeft, 
*renderer).toDouble());
}
return 0;
}

This function should be rewritten to something like this assuming RenderObject 
is updated to support CheckedPtr:
int Element::clientLeft()
{
Ref { document() }->updateLayoutIgnorePendingStylesheets();

if (CheckedPtr renderer = renderBox()) {
auto clientLeft = LayoutUnit { roundToInt(renderer->clientLeft()) };
return 
convertToNonSubpixelValue(adjustLayoutUnitForAbsoluteZoom(clientLeft, 
*renderer).toDouble());
}
return 0;
}

Here, we must store document() in Ref before calling 
updateLayoutIgnorePendingStylesheets because 
updateLayoutIgnorePendingStylesheets is a non-trivial function, and document() 
is not one of the arguments. Similarly, we need to store renderBox() in a smart 
pointer since clientLeft() is a non-trivial function. But we don’t have to 
store “this” in Ref/RefPtr before calling convertToNonSubpixelValue because 
“this” is an implicit function argument.

- R. Niwa

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

Re: [webkit-dev] Stop Using Raw Pointers & References in New Code

[Ryosuke Niwa via webkit-dev]

> On Jan 12, 2023, at 6:50 PM, Michael Catanzaro  wrote:
> 
> On Thu, Jan 12 2023 at 12:35:09 PM -0800, Ryosuke Niwa via webkit-dev 
>  wrote:
>> So… instead of:
>> foo(bar());
>> do:
>> foo(RefPtr { bar() }.get());
> 
> What's the value of creating a temporary RefPtr just to get at the underlying 
> raw pointer? Isn't this overkill?

The benefit is that bar() will be kept alive while the duration of call to foo. 
Without, whatever bar() returns can be dead before foo() finishes running, 
which can result in use-after-free.

An obvious alternative is to use smart pointer types on each function argument. 
But this has a few drawbacks:
The same rule can’t be applied to “this” since passing of “this" pointer is 
implicit in C++.
Ref churn when multiple functions are called with the same object; e.g.
foo = foo()
bar(foo); // ref/deref here
baz(foo); // ref/deref here again
Ref churn when a function argument is passed to another function; e.g.
void foo(RefPtr&& obj)
{
bar(obj); // ref/deref here even though obj is guaranteed to be alive 
throughout this function
}

- R. Niwa

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

Re: [webkit-dev] Stop Using Raw Pointers & References in New Code

[Ryosuke Niwa via webkit-dev]

> On Jan 12, 2023, at 1:27 PM, Darin Adler  wrote:
> 
>> On Jan 12, 2023, at 3:35 PM, Ryosuke Niwa via webkit-dev 
>>  wrote:
>> 
>>> On Jan 12, 2023, at 6:13 AM, Darin Adler  wrote:
>>> 
>>>> On Jan 12, 2023, at 12:21 AM, Ryosuke Niwa via webkit-dev 
>>>>  wrote:
>>>> 
>>>> assuming every local variable / variable in stack is stored in a smart 
>>>> pointer, function arguments are safe to be raw pointers / references via 
>>>> transitive property
>>> 
>>> What about the case where the function argument is the return value from 
>>> another function?
>> 
>> In those cases, the value should be stored in a local variable using a smart 
>> pointer first.
>> 
>> So… instead of:
>> foo(bar());
>> 
>> do:
>> foo(RefPtr { bar() }.get());
> 
> This seems impractical. How will I remember to do this?

The smart pointer analysis tool I've been working with the clang team should be 
able to tell you that once it becomes available. Until then, we probably need 
to rely on reviewers to notice these patterns.

One alternative is to make bar() return RefPtr although that would be a bit 
heavy handed in the case of trivial function calls like this: 
document().frame()->ownerElement()

- R. Niwa

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

Re: [webkit-dev] Stop Using Raw Pointers & References in New Code

[Ryosuke Niwa via webkit-dev]

> On Jan 12, 2023, at 6:13 AM, Darin Adler  wrote:
> 
>> On Jan 12, 2023, at 12:21 AM, Ryosuke Niwa via webkit-dev 
>>  wrote:
>> 
>> assuming every local variable / variable in stack is stored in a smart 
>> pointer, function arguments are safe to be raw pointers / references via 
>> transitive property
> 
> What about the case where the function argument is the return value from 
> another function?

In those cases, the value should be stored in a local variable using a smart 
pointer first.

So… instead of:
foo(bar());

do:
foo(RefPtr { bar() }.get());

- R. Niwa

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

[webkit-dev] Stop Using Raw Pointers & References in New Code

[Ryosuke Niwa via webkit-dev]

Hi all,

Alex recently added ThreadSafeWeakPtr 
.
 That should address the last remaining use case of raw pointers (T*) and 
references (T&).

I suggest we stop using raw pointers and references in any local or heap stored 
variable. Only cases where raw pointers and references are permitted are 
function arguments — assuming every local variable / variable in stack is 
stored in a smart pointer, function arguments are safe to be raw pointers / 
references via transitive property. See Dangerous Use of Smart Pointers 
 for 
a reference.

So, to recap, if we wanted shared ownership, or multiple pieces of code have to 
keep an object alive, use Ref 
 / RefPtr 
 with 
RefCounted 
 or 
ThreadSafeRefCounted 

 whichever is appropriate. To store a weak reference to an object, use WeakPtr 
 or 
ThreadSafeWeakPtr 
,
 whichever is appropriate. When weak semantics is not needed (i.e. we don’t 
need to clear the pointer when the object goes away) for an external reference, 
use CheckedRef 
 / 
CheckedPtr 
 so 
that we can release-assert when an object is about to get destroyed with 
outstanding external references.

I suppose we can consider exceptions to JSCell and other objects managed by GC 
but that should be more of an exception than norm. We should stop writing 
unsafe code going forward.

Any thoughts?

- R. Niwa

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

Re: [webkit-dev] Update commit log template to add placeholder for explanation of why a patch fixes a bug

[Ryosuke Niwa via webkit-dev]

> On Nov 17, 2022, at 2:46 PM, Michael Catanzaro  wrote:
> 
> On Thu, Nov 17 2022 at 02:41:35 PM -0800, Ryosuke Niwa  
> wrote:
>> We don´t want a description of what PR is; that´s obvious from diff. We want 
>> a description of why that PR fixes the bug.
> 
> Problem is, that is not very useful when the change is anything other than a 
> change that fixes a bug. :)

But every change in WebKit comes with a Bugzilla bug. If a bug is adding new 
feature, for example, one could still explain why implementing that feature is 
desirable and how it’s implemented.

- R. Niwa

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

Re: [webkit-dev] Update commit log template to add placeholder for explanation of why a patch fixes a bug

[Ryosuke Niwa via webkit-dev]

> On Nov 17, 2022, at 2:37 PM, Michael Catanzaro via webkit-dev 
>  wrote:
> 
> On Thu, Nov 17 2022 at 12:23:54 PM -0800, David Kilzer via webkit-dev 
>  wrote:
>> Any feedback on this change?
> 
> We could alternatively say "Explanation of this change (OOPS!)" or 
> "Explanation of this commit (OOPS!)" to be a little more general.

That’s strictly worse than “explain why this fixes the bug”.

We don’t want a description of what PR is; that’s obvious from diff. We want a 
description of why that PR fixes the bug.

- R. Niwa

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

Re: [webkit-dev] Update commit log template to add placeholder for explanation of why a patch fixes a bug

[Ryosuke Niwa via webkit-dev]

> On Nov 17, 2022, at 12:23 PM, David Kilzer via webkit-dev 
>  wrote:
> 
> The following PR adds placeholder text in the commit log template to remind 
> authors to explain why a change fixes a bug:
> 
> Bug 248012: Update commit message template to request a brief explanation of 
> why a PR fixes the bug
>  >
>  >
> 
> It looks like this:
> 
> Need a short description (OOPS!).
> Need the bug URL (OOPS!).
> Include a Radar link (OOPS!).
> 
> Reviewed by NOBODY (OOPS!).
> 
> Short explanation why this fixes the bug (OOPS!).
> 
> * path/to/File.cpp:
> (Method::Name):
> 
> Since this is effectively a policy change, Jonathan suggested I post it here 
> first before the change is landed.
> 
> I believe many WebKit contributors already do this anyway, and it’s always 
> been a best-practice to explain why a change fixes a bug (vs. what a patch 
> does, which is usually obvious by reading the code), so this text just serves 
> as a reminder to write a short explanation for every relevant patch.
> 
> Note that the line may be simply be deleted when it doesn’t apply (such as 
> gardening, a build fix, etc.).
> 
> Any feedback on this change?

Seems like a good idea.

- R. Niwa

___
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 (; ; )`

[Ryosuke Niwa via webkit-dev]

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 list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] webkit-dev Digest, Vol 208, Issue 5

[Ryosuke Niwa via webkit-dev]

> On Sep 29, 2022, at 5:26 PM, Jonathan Bedard  wrote:
> 
>> On Sep 29, 2022, at 4:28 PM, Ryosuke Niwa  wrote:
>> 
>>> On Sep 29, 2022, at 11:48 AM, Jonathan Bedard via webkit-dev 
>>> mailto:webkit-dev@lists.webkit.org>> wrote:
>>> 
>>>>> On Sep 20, 2022, at 1:52 PM, Brent Fulgham >>>> <mailto:bfulg...@apple.com>> wrote:
>>>>> 
>>>>>> On Sep 20, 2022, at 1:16 AM, Ryosuke Niwa via webkit-dev 
>>>>>> mailto:webkit-dev@lists.webkit.org>> wrote:
>>>>>> 
>>>>>>> On Sep 19, 2022, at 2:28 PM, Brandon Stewart via webkit-dev 
>>>>>>> mailto:webkit-dev@lists.webkit.org> 
>>>>>>> <mailto:webkit-dev@lists.webkit.org 
>>>>>>> <mailto:webkit-dev@lists.webkit.org>>> wrote:
>>>>>>> 
>>>>>>> Documentation is an important part of any open source project, 
>>>>>>> especially for a larger project like WebKit. Being able to ramp up 
>>>>>>> during the onboarding process, reading up on architectural decisions, 
>>>>>>> and learning how to perform common procedures are all features the 
>>>>>>> documentation should tackle. WebKit has a large set of documentation 
>>>>>>> already, but it is scattered around a wide range of platforms (Trac, 
>>>>>>> GitHub Wiki, markdown files in the code, Git commits, etc...), and some 
>>>>>>> of the information is out of date.
>>>>>> 
>>>>>> This ultimately feels like this situation:
>>>>>> https://xkcd.com/927/ <https://xkcd.com/927/> <https://xkcd.com/927/ 
>>>>>> <https://xkcd.com/927/>>
>>>>>> 
>>>>>> I?ve been working on 
>>>>>> https://github.com/WebKit/WebKit/blob/main/Introduction.md 
>>>>>> <https://github.com/WebKit/WebKit/blob/main/Introduction.md> 
>>>>>> <https://github.com/WebKit/WebKit/blob/main/Introduction.md 
>>>>>> <https://github.com/WebKit/WebKit/blob/main/Introduction.md>> for the 
>>>>>> past couple of years, and I?d would have preferred to have a 
>>>>>> collaboration rather than a competition here.
>>> 
>>> Right now we have:
>>> https://github.com/WebKit/WebKit/wiki 
>>> <https://github.com/WebKit/WebKit/wiki>
>>> https://trac.webkit.org/ <https://trac.webkit.org/>
>>> https://webkit.org/getting-started/ <https://webkit.org/getting-started/>
>>> https://github.com/WebKit/WebKit/blob/main/Introduction.md 
>>> <https://github.com/WebKit/WebKit/blob/main/Introduction.md>
>>> 
>>> Brandon’s solution is designed to replace the first 2, and he has buy in 
>>> from the maintainers of the first two, when his solution is deployed, our 
>>> existing wikis will die.
>> 
>> I don’t think there is a community wide buy-in to replace either 
>> documentations / tools at this point. I don’t think people who happen to 
>> maintain the infrastructure get to have unilateral say on which tools will 
>> get supported or deprecated.
> 
> This is an effort to get community wide buy-in.

Right, and I’m pointing out that as of this moment, there isn’t community wide 
buy-in.

>>>>>>> I have tested this on macOS and Linux and have found it works extremely 
>>>>>>> well. (Windows should be able to use WSL2 at the moment, while a few 
>>>>>>> remaining issues get sorted out). The only dependency for this project 
>>>>>>> is a recent installation of Swift.
>>>>>> 
>>>>>> Previously, we?ve rejected Swift as a general purpose programming 
>>>>>> languages in WebKit: 
>>>>>> https://lists.webkit.org/pipermail/webkit-dev/2014-July/026722.html 
>>>>>> <https://lists.webkit.org/pipermail/webkit-dev/2014-July/026722.html> 
>>>>>> <https://lists.webkit.org/pipermail/webkit-dev/2014-July/026722.html 
>>>>>> <https://lists.webkit.org/pipermail/webkit-dev/2014-July/026722.html>> 
>>>>>> other than to allow existing C++ code to call into Swift API: 
>>>>>> https://lists.webkit.org/pipermail/webkit-dev/2021-June/031882.html 
>>>>>> <https://lists.webkit.org/pipermail/webkit-dev/2021-June/031882.html> 
>>>>>> <https://lists.webkit.org/pi

Re: [webkit-dev] webkit-dev Digest, Vol 208, Issue 5

[Ryosuke Niwa via webkit-dev]

> On Sep 29, 2022, at 11:48 AM, Jonathan Bedard via webkit-dev 
>  wrote:
> 
> 
>>> On Sep 20, 2022, at 1:52 PM, Brent Fulgham  wrote:
>>> 
>>>> On Sep 20, 2022, at 1:16 AM, Ryosuke Niwa via webkit-dev 
>>>>  wrote:
>>>> 
>>>>> On Sep 19, 2022, at 2:28 PM, Brandon Stewart via webkit-dev 
>>>>> mailto:webkit-dev@lists.webkit.org>> wrote:
>>>>> 
>>>>> Documentation is an important part of any open source project, especially 
>>>>> for a larger project like WebKit. Being able to ramp up during the 
>>>>> onboarding process, reading up on architectural decisions, and learning 
>>>>> how to perform common procedures are all features the documentation 
>>>>> should tackle. WebKit has a large set of documentation already, but it is 
>>>>> scattered around a wide range of platforms (Trac, GitHub Wiki, markdown 
>>>>> files in the code, Git commits, etc...), and some of the information is 
>>>>> out of date.
>>>> 
>>>> This ultimately feels like this situation:
>>>> https://xkcd.com/927/ <https://xkcd.com/927/>
>>>> 
>>>> I?ve been working on 
>>>> https://github.com/WebKit/WebKit/blob/main/Introduction.md 
>>>> <https://github.com/WebKit/WebKit/blob/main/Introduction.md> for the past 
>>>> couple of years, and I?d would have preferred to have a collaboration 
>>>> rather than a competition here.
> 
> Right now we have:
> https://github.com/WebKit/WebKit/wiki <https://github.com/WebKit/WebKit/wiki>
> https://trac.webkit.org/ <https://trac.webkit.org/>
> https://webkit.org/getting-started/ <https://webkit.org/getting-started/>
> https://github.com/WebKit/WebKit/blob/main/Introduction.md 
> <https://github.com/WebKit/WebKit/blob/main/Introduction.md>
> 
> Brandon’s solution is designed to replace the first 2, and he has buy in from 
> the maintainers of the first two, when his solution is deployed, our existing 
> wikis will die.

I don’t think there is a community wide buy-in to replace either documentations 
/ tools at this point. I don’t think people who happen to maintain the 
infrastructure get to have unilateral say on which tools will get supported or 
deprecated.

>>>>> I have tested this on macOS and Linux and have found it works extremely 
>>>>> well. (Windows should be able to use WSL2 at the moment, while a few 
>>>>> remaining issues get sorted out). The only dependency for this project is 
>>>>> a recent installation of Swift.
>>>> 
>>>> Previously, we?ve rejected Swift as a general purpose programming 
>>>> languages in WebKit: 
>>>> https://lists.webkit.org/pipermail/webkit-dev/2014-July/026722.html 
>>>> <https://lists.webkit.org/pipermail/webkit-dev/2014-July/026722.html> 
>>>> other than to allow existing C++ code to call into Swift API: 
>>>> https://lists.webkit.org/pipermail/webkit-dev/2021-June/031882.html 
>>>> <https://lists.webkit.org/pipermail/webkit-dev/2021-June/031882.html>
>>>> 
>>>> As such, I don?t think we should require having a functional Swift 
>>>> toolchain as a requirement for contributing to WebKit or its 
>>>> documentations either.
>>> 
>>> DocC is not a requirement to use or participate in this work. It?s simply a 
>>> ?port? of the documentation that works for our needs. If others prefer to 
>>> ?build? output documentation from Markdown using a different tool set, they 
>>> are welcome to contribute those build commands and instructions.
>> 
> 
> Something else worth calling out here is that contributing DocC compatible 
> markdown is similar to contributions to 
> https://github.com/WebKit/WebKit/tree/main/Websites/webkit.org 
> <https://github.com/WebKit/WebKit/tree/main/Websites/webkit.org> that 
> eventually end up on webkit.org <http://webkit.org/>, except that we have a 
> nice path to automating DocC deployment (either to GitHub pages or a 
> webkit.org <http://webkit.org/> property). We pretty frequently use 
> technologies in our services we don’t intend contributors to regularly run at 
> desk.

That sounds strictly worse than contributing to Trac wiki or Github wiki, or 
even Introduction.md then. Why are we making contribution to documentation 
harder than it already is?

>>> Our goal is to accumulate all relevant and open source documentation 
>>> related to WebKit in this repository so that we can generate searchabl

Re: [webkit-dev] WebKit Documentation

[Ryosuke Niwa via webkit-dev]

> On Sep 21, 2022, at 6:50 AM, Michael Catanzaro  wrote:
> 
> On Tue, Sep 20 2022 at 08:03:12 PM -0700, Ryosuke Niwa via webkit-dev 
>  wrote:
>> (2) is particularly important because many people who are new to WebKit 
>> often don´t know what they don´t know. This is why, for example, memory 
>> management section appears towards the beginning of the document and the 
>> information about adding IDL files is immediately followed by the discussion 
>> of JS wrapper lifecycles. With these information now split across multiple 
>> files, it´s easy for people to miss out on critical information. In the 
>> ideal world, people won´t be adding or editing IDL files before they 
>> understood how JS wrappers are managed because not knowing the latter is a 
>> sure way of introducing subtle GC bugs.
> 
> I didn't know any of this info about JS wrapper lifecycle until I read about 
> it in Introduction.md a couple months ago. It really does need to be very 
> prominent or developers won't find it. That said, I think we can find a way 
> to do this without keeping it all in one single page.

Yeah, I’m sure there are ways to make it prominent without putting all the 
information into a single md file. But we need to come up with a way to do that.

- R. Niwa

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

Re: [webkit-dev] WebKit Documentation

[Ryosuke Niwa via webkit-dev]

> On Sep 20, 2022, at 1:52 PM, Brent Fulgham  wrote:
> 
>> On Sep 20, 2022, at 1:16 AM, Ryosuke Niwa via webkit-dev 
>>  wrote:
>> 
>>> On Sep 19, 2022, at 2:28 PM, Brandon Stewart via webkit-dev 
>>> mailto:webkit-dev@lists.webkit.org>> wrote:
>>> 
>>> Documentation is an important part of any open source project, especially 
>>> for a larger project like WebKit. Being able to ramp up during the 
>>> onboarding process, reading up on architectural decisions, and learning how 
>>> to perform common procedures are all features the documentation should 
>>> tackle. WebKit has a large set of documentation already, but it is 
>>> scattered around a wide range of platforms (Trac, GitHub Wiki, markdown 
>>> files in the code, Git commits, etc...), and some of the information is out 
>>> of date.
>> 
>> This ultimately feels like this situation:
>> https://xkcd.com/927/ <https://xkcd.com/927/>
>> 
>> I’ve been working on 
>> https://github.com/WebKit/WebKit/blob/main/Introduction.md 
>> <https://github.com/WebKit/WebKit/blob/main/Introduction.md> for the past 
>> couple of years, and I’d would have preferred to have a collaboration rather 
>> than a competition here.
> 
> Ryosuke: Your document is outstanding, and is a large part of the content we 
> pulled into the current repo. I don’t think we view this as a competition; 
> rather we are trying to host a collection of Markdown content in a common 
> repo that does not have to be pulled and synced with WebKit source and tests. 
> This provides a lower bar for people interested in contributing documentation 
> as they do not have to download and sync Gigabytes of WebKit code to write 
> documentation.

Who are these people who want to contribute to WebKit’s documentation yet 
doesn’t already have a clone of WebKit repository? I just can’t think of people 
who would fit that description.

There was a reason behind why the entire Introduction.md was written as a 
single markdown file though. Namely:
Reader can look up unknown terms right away in the same page using browsers’ 
find feature
Let reader be aware of things they are unaware of.

(2) is particularly important because many people who are new to WebKit often 
don’t know what they don’t know. This is why, for example, memory management 
section appears towards the beginning of the document and the information about 
adding IDL files is immediately followed by the discussion of JS wrapper 
lifecycles. With these information now split across multiple files, it’s easy 
for people to miss out on critical information. In the ideal world, people 
won’t be adding or editing IDL files before they understood how JS wrappers are 
managed because not knowing the latter is a sure way of introducing subtle GC 
bugs.

>>> A few months ago, I started working on a new documentation solution based 
>>> on the DocC documentation framework.
>> 
>> Was there any discussion as to which documentation framework should be used? 
>> I’m personally not familiar with DooC documentation, and I’m  surprised that 
>> such an important decision was made unilaterally without much discussion on 
>> webkit-dev.
> 
> We discussed this internally, but today’s message is the first discussion on 
> this repository that we’ve opened on the webkit-dev mailing list. We wanted 
> to convince ourselves that the tool worked well, and was easy to use, and 
> could produce documentation output that would be useful for Apple engineers. 
> That is not the only intended audience for this work, but is the origin of 
> this effort which we wanted to make available to others to use and contribute 
> to.
> 
> Those of us who have worked on WebKit for some time can easily remember the 
> many discussions over the years about documentation efforts of various types. 
> Lots of people have strong opinions, but few ever contribute despite their 
> opinions of the right way for the work to be done. You are obviously part of 
> the group that has contributed heavily, so I am sad that you do not seem to 
> like our approach.

Right, it’s why I wrote Introduction.md in the first place.

>>> I have already ported a large section of Trac, all of the GitHub Wiki, and 
>>> all of the non third party markdown files in the code over to this platform.
>> 
>> I’m not certain if there is a community wide support that this is the right 
>> tool to migrate all our documentations. I for one certainly object to the 
>> use of DooC as the primary documentation tool.
> 
> DocC is one way of processing the Markdown content in this repo, and one that 
> works well with Xcode since it creates output that matches Apple 
&

Re: [webkit-dev] WebKit Documentation

[Ryosuke Niwa via webkit-dev]

> On Sep 20, 2022, at 6:04 AM, Michael Catanzaro  wrote:
> 
> On Tue, Sep 20 2022 at 01:16:53 AM -0700, Ryosuke Niwa via webkit-dev 
>  wrote:
>> I´ve been working on 
>> https://github.com/WebKit/WebKit/blob/main/Introduction.md for the past 
>> couple of years, and I´d would have preferred to have a collaboration rather 
>> than a competition here.
> 
> This Introduction.md is great work (I've learned a lot from it), but it's 
> also getting pretty long for a single document. At least, it significantly 
> exceeds my attention span. We could present this same info better if we split 
> it into multiple pages.

Yeah, I agree. I wanted to keep the document searchable; putting them all in a 
single document lets us use browser’s “find” function. GitHub wiki pages are 
searchable so splitting into multiple pages isn’t an issue there.

- R. Niwa

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

Re: [webkit-dev] WebKit Documentation

[Ryosuke Niwa via webkit-dev]

> On Sep 19, 2022, at 2:58 PM, Michael Catanzaro via webkit-dev 
>  wrote:
> 
> Why not double-down on the GitHub wiki? It's very easy to learn to use, and 
> there are edit buttons everywhere so there is no "distance" between the docs 
> and the ability to edit them. The easier it is to edit docs, the better we'll 
> do at keeping them up to date.

One drawback of wiki is that changes won’t be code reviewed. I find that the 
much of the existing WebKit documentations are filled with outdated, 
inaccurate, and/or incomplete information, and a part of the reason is because 
no formal review is done unlike our code. At least in my experience working on 
https://github.com/WebKit/WebKit/blob/main/Introduction.md 
, I find that code 
review to be generally helpful.

> I like Markdown, and am OK with editing Markdown files wherever they live, 
> but it's not very likely that I would install Swift and figure out how to 
> build a new project to to see what the result looks like. With GitHub, we can 
> easily preview results live to ensure we're not messing anything up.

I totally agree. In order for this new documentation effort to be successful, 
contribution needs be as simple & straight forward as possible.

- R. Niwa

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

Re: [webkit-dev] WebKit Documentation

[Ryosuke Niwa via webkit-dev]

> On Sep 19, 2022, at 2:28 PM, Brandon Stewart via webkit-dev 
>  wrote:
> 
> Documentation is an important part of any open source project, especially for 
> a larger project like WebKit. Being able to ramp up during the onboarding 
> process, reading up on architectural decisions, and learning how to perform 
> common procedures are all features the documentation should tackle. WebKit 
> has a large set of documentation already, but it is scattered around a wide 
> range of platforms (Trac, GitHub Wiki, markdown files in the code, Git 
> commits, etc...), and some of the information is out of date.

This ultimately feels like this situation:
https://xkcd.com/927/ 

I’ve been working on https://github.com/WebKit/WebKit/blob/main/Introduction.md 
 for the past 
couple of years, and I’d would have preferred to have a collaboration rather 
than a competition here.

> A few months ago, I started working on a new documentation solution based on 
> the DocC documentation framework.

Was there any discussion as to which documentation framework should be used? 
I’m personally not familiar with DooC documentation, and I’m  surprised that 
such an important decision was made unilaterally without much discussion on 
webkit-dev.

> I have already ported a large section of Trac, all of the GitHub Wiki, and 
> all of the non third party markdown files in the code over to this platform.

I’m not certain if there is a community wide support that this is the right 
tool to migrate all our documentations. I for one certainly object to the use 
of DooC as the primary documentation tool.

> I have tested this on macOS and Linux and have found it works extremely well. 
> (Windows should be able to use WSL2 at the moment, while a few remaining 
> issues get sorted out). The only dependency for this project is a recent 
> installation of Swift.

Previously, we’ve rejected Swift as a general purpose programming languages in 
WebKit: https://lists.webkit.org/pipermail/webkit-dev/2014-July/026722.html 
 other 
than to allow existing C++ code to call into Swift API: 
https://lists.webkit.org/pipermail/webkit-dev/2021-June/031882.html 


As such, I don’t think we should require having a functional Swift toolchain as 
a requirement for contributing to WebKit or its documentations either.

- R. Niwa

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

Re: [webkit-dev] WebKit Documentation

[Ryosuke Niwa via webkit-dev]

> On Sep 19, 2022, at 4:48 PM, Fujii Hironori via webkit-dev 
>  wrote:
> 
> Why not double-down on WebKit Git repository?
> The closer the document is to the source code, the easier to keep them 
> up-to-date.
> We can modify both the source code and the document in a single commit 
> through our review process.

The fact documentation is separated from code is a feature, not a bug. We don't 
want to make updating documentation a contribution requirement. Just like unit 
tests, this sort of thing can significantly hinder our willingness and 
capabilities to do large scale refactoring, renames, etc...

- R. Niwa

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

Re: [webkit-dev] Deployment of new EWS Non-Unified builder

[Ryosuke Niwa via webkit-dev]

> On Sep 8, 2022, at 5:41 AM, Adrian Perez de Castro via webkit-dev 
>  wrote:
> On Thu, 08 Sep 2022 02:27:53 + Alexey Proskuryakov via webkit-dev 
>  wrote:
>> Without non-unified EWS, or anyone fixing non-unified build manually:
>> - a smaller number of patches gets rejected for breaking existing EWS 
>> builders;
>> - it's sometimes harder to fix, because the errors could be in unfamiliar 
>> code (although how hard can it be to add an include, on average).
> 
> Not fixing the build ever would not work. If we don't land patches which are
> technically correct there will be always situations in which the build will
> randomly break.
> 
> Example: Buildbot (both EWS and post-commit) build with DEVELOPER_MODE=ON and
> ENABLE_EXPERIMENTAL_FEATURES=ON, while end user builds disable those. The
> changed options can result in shifted sources in the unified build compilation
> units. What passes as "yeah, it builds" in the bots could still fail later
> when producing releases. This applies to *all* ports.

This is not true. Build flags are if-def’s inside each file, not a condition on 
whether a given file is unified or not. Enabling or disabling any feature 
should not affect which set of cpp files are unified together.

- R. Niwa

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

[webkit-dev] Goal of Fixing non-Unified Build Failures (Was Deployment of new EWS Non-Unified builder)

[Ryosuke Niwa via webkit-dev]

> There is no such thing as "not maintaining the non-unified build"; there 
> never has been. We have covered that this is an *inherent* problem in a 
> unified build mechanism and that this would be an issue even if Mac were the 
> only platform.

It appears to me that we’re lacking the clarify in defining what problem(s) 
we’re trying to solve here. As far as I can tell, fixing non-unified builds is 
a potential solution to two problems:
Adding a new cpp file to Sources.txt causes random other file to trigger a 
compiler error because the set of cpp files unified together have changed / 
shuffled across.
Build failure may occur in an exotic port or configuration which unify a 
different set of cpp files than the supported configurations.

(1) might be a problem worth solving but there are multiple solutions to this 
problem. For example, we could stop lexicologically sorting cpp files in 
Sources.txt, and always append new cpp files at the end of each directory 
instead. This will avoid shuffling cpp files that are unified together when 
adding new cpp files. Someone who feels like it could come in & sort files 
occasionally, or maybe we should just forget about this entirely and let the 
chaos ensue. Either solution seems less costly than forcing every contributor 
to maintain non-unified builds functional.

While I’m sympathetic to the view that (2) is a problem or that it would be 
nice for the WebKit project to address the concerns of such exotic 
configurations or ports, I just don’t see how this differs from all other build 
failures that occur from WebKit code changes such as refactoring of platform 
abstraction. I grant that build failures which are effect of cpp files getting 
unified differently across different ports and configurations might be tricky 
to diagnose.

Even if we were to agree that (2) is a problem worth addressing, again, there 
are multiple solutions to this problem. We could, for example, stabilize cpp 
files that are unified across configurations / ports so that if a set of cpp 
files get unified under one configuration, they will always get unified in all 
other configurations. In practice, this might mean moving every platform 
specific file into a sub directory and unifying them separately for example.

- R. Niwa

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

Re: [webkit-dev] Deployment of new EWS Non-Unified builder

[Ryosuke Niwa via webkit-dev]

On Thu, Jun 2, 2022 at 4:28 AM Claudio Saavedra via webkit-dev
 wrote:
>
> On Wed, 2022-06-01 at 16:39 -0700, Ryosuke Niwa via webkit-dev wrote:
> > One day per month for one beginner sounds like a really low
> > maintenance cost compared to having every WebKit developer fix non-
> > unified builds at all times.
>
> I'm sorry, but this is not about having "every WK developer fix non-
> unified builds at all times", it's about everyone making sure that
> their patches are correct. A patch that uses API in one file without
> making sure the required headers are included is not a correct patch
> and unified builds are only hiding the issues. I don't see how this can
> be controversial.

We probably disagree on what we mean by "correct".
As far as I'm concerned, a patch is correct if all relevant ports can be built.

There is theoretical correctness of cpp or headers files including all
the necessary headers but that's merely theoretical outside of
non-unified builds. I don't think we necessarily want to spend all our
time thinking about & fixing theoretical problems until they actually
surface.

> Also, there is no pool of beginner developers who can be fixing missing
> includes all the time; even if we had spare manpower, it's more
> beneficial for the project (and themselves) if they spend that time
> doing gardening or fixing actual bugs.

Sorry, that was a typo / bad auto correction of the word "engineer".

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

Re: [webkit-dev] Deployment of new EWS Non-Unified builder

[Ryosuke Niwa via webkit-dev]

On Wed, Jun 1, 2022 at 16:10 Kirsling, Ross via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> I feel like this has been discussed adequately in the past, but one more
> time for good measure:
>
> Any two platforms which don't build the exact same set of files will
> undergo unification differently. That means that unification shifts are an
> inherent part of working on WebKit, embedder or otherwise.
>
> The only way to be certain that includes are done correctly in a given
> patch is to perform a non-unified build. This would be an unreasonable
> burden for local development, but that's exactly why an EWS builder is
> desirable.
>
> To have this appear like a non-issue is to ignore the work that Sony and
> Igalia have continually performed through the 5(?) years since unified
> builds were introduced. From experience, I know that it can take a person
> about a day per month to clean up includes on behalf of the entire WebKit
> community.
>

One day per month for one beginner sounds like a really low maintenance
cost compared to having every WebKit developer fix non-unified builds at
all times.

Each patch should be responsible for getting its own includes correct.
>

It's unclear that this makes sense given that we can already fix build
failures caused by different set of translation units getting unified for
WebKit ports that have their own EWS bots.

One day per person per month sounds like a totally reasonable cost for us
to ask of ports that don't yet to have EWS setup in the upstream WebKit
project.

Inevitably, such ports already face other more complex build failures
whenever we refactor WebKit or WebCore platform by, say, introducing new
client interface or pure virtual member function that has to be overridden
in each platform / port.

>
- R. Niwa

-- 
- R. Niwa
___
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

[Ryosuke Niwa via webkit-dev]

On Wed, May 11, 2022 at 08:12 Chris Dumez  wrote:

>
> On May 11, 2022, at 12:13 AM, Ryosuke Niwa via webkit-dev <
> webkit-dev@lists.webkit.org> 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 <
> webkit-dev@lists.webkit.org> 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 agree.

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.
>

Or you're making changes to things like introduction.md or perf dashboard
code, which has zero change of breaking WebKit builds or tests.

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.
>

My main issue is that I never want to make a local commit because it messes
with my branch states. Has the solution to use a separate file to write
commit message been implemented yet? I only want to commit something when
it's getting merged into the remote main.

- R. Niwa
-- 
- R. Niwa
___
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

[Ryosuke Niwa via webkit-dev]

On Tue, May 10, 2022 at 10:04 PM Chris Dumez  wrote:
>
> On May 10, 2022, at 9:27 PM, Ryosuke Niwa  wrote:
>>
>> Well, that's precisely my use case. I don't even write a patch in a pure 
>> Subversion checkout anymore these days.
>
> Also, I didn’t fully understand this comment. Did you mean that you are 
> already using git-svn and relying to `git-svn dcommit` for local committing?

No, I'm using Github clones to write patches then using Subversion
checkout to commit those patches.

- R. Niwa
___
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

[Ryosuke Niwa via webkit-dev]

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.
>
>> 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.
>
> - R. Niwa
>
> --
> - R. Niwa
___
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

[Ryosuke Niwa via webkit-dev]

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.

- R. Niwa
___
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

[Ryosuke Niwa via webkit-dev]

I don't think we should this. We haven't even had enough discussions about
whether we want to deprecate change logs or not.

On Tue, May 10, 2022 at 13:32 Jonathan Bedard via webkit-dev <
webkit-dev@lists.webkit.org> 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
>
-- 
- R. Niwa
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Introduction.md: JS wrapper & Node lifecycle management

[Ryosuke Niwa via webkit-dev]

Sorry, I broke the link for DOM nodes due to a typo. See
https://github.com/WebKit/WebKit/blob/main/Introduction.md#reference-counting-of-dom-nodes

On Sat, May 7, 2022 at 11:05 AM Ryosuke Niwa  wrote:
>
> Hi all,
>
> I've recently written up an overview of how the lifecycle of JS wrappers and 
> DOM nodes are managed in WebKit in Introduction.md.
>
> I'd recommend glancing over it even if you're an experienced WebKit 
> contributor since I've seen so many patches that have obvious bugs in these 
> two areas over the years.
>
> Also, let me know if you think of anything we should add or clarify in these 
> two sections of Introduction.md.
>
> - R. Niwa
>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

[webkit-dev] Introduction.md: JS wrapper & Node lifecycle management

[Ryosuke Niwa via webkit-dev]

Hi all,

I've recently written up an overview of how the lifecycle of JS wrappers

and DOM nodes

are managed in WebKit in Introduction.md
.

I'd recommend glancing over it even if you're an experienced WebKit
contributor since I've seen so many patches that have obvious bugs in these
two areas over the years.

Also, let me know if you think of anything we should add or clarify in
these two sections of Introduction.md.

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

Re: [webkit-dev] ChangeLog Deprecation Plan

[Ryosuke Niwa via webkit-dev]

On Mon, Apr 25, 2022 at 9:49 AM Jonathan Bedard via webkit-dev
 wrote:
>
> .. or robust solution. Bots need maintenance and intervention, and a bot with
> commit access has another set of issues. Repository admins occasionally
> rotating ChangeLogs is going to be less expensive than a bot doing it.
>
> Either way, it doesn't seem like this is a higher cost issue compared to
> all WebKit contributors having to change their workflows.
>
> We are moving to git. Workflows _are_ changing. I’m interested in making sure 
> the workflows can achieve the same things the old ones did, not in preserving 
> the old workflows.
>

Just because other parts of the workflow are changing, it doesn't mean
we should pile on even more changes. Honestly, this whole discussion
loses me hope that the WebKit project won't remain something I want to
keep contributing to in the future.

> folks familiar with git won?t realize what has happened until they see
> their PR diff), ?mandatory? is a bit tougher because we can?t prevent folks
> from filing a pull request without using tooling. At that point, we?re
> where we are today with PR template that encourages tool usage, explains
> how to craft a PR the ways tools do it and then reviewers acting as a
> gate-keeper. The last point is more about project policy than it is tooling.
>
> We should automatically reject such PR requests.
>
> We should not automatically reject PRs made in good faith.
>
> One of the reasons we’re moving to GitHub is to make the project more 
> approachable for new contributors. Having a machine rejects PRs because a 
> contributor is not familiar with project norms goes against this goal.

We need to. Otherwise, support for COMMIT_MESSAGE files would be
pretty much meaningless. I need those files as a reviewer, not as a
patch author.

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

Re: [webkit-dev] ChangeLog Deprecation Plan

[Ryosuke Niwa via webkit-dev]

On Tue, Apr 19, 2022 at 9:33 AM Jonathan Bedard via webkit-dev <
webkit-dev@lists.webkit.org> wrote:
>
> > To: Jonathan Bedard 
> > Cc: WebKit Development 
> > Subject: Re: [webkit-dev] ChangeLog Deprecation Plans
> > Message-ID:
> >   <
cabnrm613onu_8wjxeuhjvoh7vztc3t9r_ylnqgoj5if5as0...@mail.gmail.com>
> > Content-Type: text/plain; charset="utf-8"
> >
> > On Mon, Apr 18, 2022 at 8:30 AM Jonathan Bedard via webkit-dev <
> > webkit-dev@lists.webkit.org> wrote:
> >
> >> As we migrate WebKit from Subversion to git, I would like to migrate
the
> >> project away from ChangeLogs. The reason for this is that ChangeLogs
make
> >> some of the features of git hard to use, namely, cherry-picking commits
> >> between branches requires conflict resolution every time.
> >>
> >
> > Isn't this something that can be easily resolved with a merge script?
>
> For simple cases, yes. But with something like a multi-commit revert or
rebasing a feature onto a different branch, merge scripts stop working
pretty quickly. It’s usually, It might be possible to write a merge script
to handle these cases, but it’s by no means “easy” in the general case.
It’s also worth noting that the cases that are more complicated also tend
to be the ones with the most urgency attached to them.

It seems to me that you can write a script that just applies a patch while
ignoring changes to each change log file, then separately prepend each
change log entry. I don't see why such a script would behave differently
for reverting multiple commits or rebasing a feature onto a branch. In
those cases where codebase has enough divergence, merge conflicts are
probably more of an issue with the codebase itself, and not change log
entries.

> > Rotating ChangeLogs is also moderately difficult in git with locked down
> >> commit access like our project has, only repository administers would,
in
> >> practice, be able to rotate ChangeLogs.
> >>
> >
> > It seems like we can just automate this by introducing a change log
> > rotating bot, which has the same privilege as commit queue.
>
> Anything that starts with “we can just have a bot which…” is not a simple
or robust solution. Bots need maintenance and intervention, and a bot with
commit access has another set of issues. Repository admins occasionally
rotating ChangeLogs is going to be less expensive than a bot doing it.

Either way, it doesn't seem like this is a higher cost issue compared to
all WebKit contributors having to change their workflows.

> > So these two arguments seem rather weak.
> >
> > Lastly, ChangeLogs are uncommon in git based projects, so new
contributors
> >> will find them difficult to manage.
> >>
> >
> > This might be the strongest argument in favor of deprecating change
logs.
> >
> > 2) We need a way to comment on commit messages in review
> >> Current tooling sets the pull request description as the commit
message,
> >> ?Quote Reply? kind of provides a way to inline comment, although it?s
not
> >> the formal review UI
> >> *Proposal*: Tooling should support a ?COMMIT_MESSAGE? file in each pull
> >> request commit that becomes COMMIT_MESSAGE when a pull request is
landed
> >>
> >
> > This needs to be a mandatory / automatic system, not opt-in. I want to
> > comment on commit messages as a reviewer. As a patch author, I don't
care
> > whether it can be easily commented on or not.
> >
> > But if this is a required thing, then new contributors would have to
learn
> > that this file is auto-generated or needs to be edited manually in some
> > cases so getting rid of change logs may not necessarily reduce the
> > cognitive load in comparison to keeping the status quo (i.e. keeping
change
> > log files).
>
> We can make it automatic for folks using tooling (and in such a way that
folks familiar with git won’t realize what has happened until they see
their PR diff), “mandatory” is a bit tougher because we can’t prevent folks
from filing a pull request without using tooling. At that point, we’re
where we are today with PR template that encourages tool usage, explains
how to craft a PR the ways tools do it and then reviewers acting as a
gate-keeper. The last point is more about project policy than it is tooling.

We should automatically reject such PR requests.


> > 3) Edit commit messages while creating a change, not just when
committing
> >> the change
> >> The ?overwrite? workflow already sort of support this idea by using
amend
> >> commits instead of appending commits to an existing branch
> >> *Proposal*: The above ?COMMIT_MESSAGE? file workflow would allow
> >> iterative building of a commit message before committing
> >>
> >
> > I absolutely despise --amend commits. It's the most annoying thing I
have
> > to do whenever I'm creating patches in a git clone.
>
> I do not think it is the appropriate place for the WebKit project to make
judgments about personal preferences for local development. From my
discussions with folks, it seems that the project is pretty evenly split on
--amend 

Re: [webkit-dev] ChangeLog Deprecation Plans

[Ryosuke Niwa via webkit-dev]

On Mon, Apr 18, 2022 at 18:50 Fujii Hironori via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

>
> On Tue, Apr 19, 2022 at 6:55 AM Yusuke Suzuki via webkit-dev <
> webkit-dev@lists.webkit.org> wrote:
>
>> I think this is important. We are using commit message / ChangeLog as a
>> document tied to the change, and we are writing very detailed description
>> to make the intention / design of the change clear and making it as a good
>> document when we read the change via git-blame, bisect, using that header,
>> investigating how it works etc.
>> To make / keep this commit message / ChangeLog helpful, we need review,
>> and I think reviewing of these messages is critical to keep usefulness of
>> them.
>>
>
>
> I don't think commit messages and ChangeLogs are the best place for
> technical descriptions.
> We use them because we don't have a better place.
>
> libpas added the technical document in the repository in markdown.
>
> https://github.com/WebKit/WebKit/blob/main/Source/bmalloc/libpas/Documentation.md
>
> This makes it possible to change code and update the document in a single
> commit, and get reviewed.
> markdown is better than plain text. Updated documents are more useful than
> the historical wiki pages.
> It'd be nice if more documents are migrated into the repository.
>

Documentation gets obsolete relatively quickly. Code is the source of
truth. A lot of comments in WebKit are either outdated, incorrect, and/or
misleading. Change logs describe a change and thereby a specific point in
time of the repository and wouldn't carry the same issue.

It's also particularly useful to be able to see why a given line of code
was written. Was it an oversight? On purpose? Or perhaps the required
behavior different then and now? It's impractical and unproductive to add
such a comment on every line of our codebase, and that level of online
comment will add so much clutter to the codebase and would make it harder
to read the code.

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

Re: [webkit-dev] ChangeLog Deprecation Plans

[Ryosuke Niwa via webkit-dev]

On Mon, Apr 18, 2022 at 13:34 Ryosuke Niwa  wrote:

>
> On Mon, Apr 18, 2022 at 08:30 Jonathan Bedard via webkit-dev <
> webkit-dev@lists.webkit.org> wrote:
>
>> As we migrate WebKit from Subversion to git, I would like to migrate the
>> project away from ChangeLogs. The reason for this is that ChangeLogs make
>> some of the features of git hard to use, namely, cherry-picking commits
>> between branches requires conflict resolution every time. Rotating
>> ChangeLogs is also moderately difficult in git with locked down commit
>> access like our project has, only repository administers would, in
>> practice, be able to rotate ChangeLogs. Lastly, ChangeLogs are uncommon in
>> git based projects, so new contributors will find them difficult to manage.
>>
>> Currently, our git tooling makes very little effort to either support
>> ChangeLogs or to provide alternatives. I have listed bellow some of the
>> reasons I understand folks to like ChangeLogs along with possible git-based
>> solutions, if necessary.
>>
>> 1) Subversion commit messages are stored server side, local development
>> needs a copy
>> git doesn’t have this problem. We have a local record of commit messages
>> in every checkout.
>> 2) We need a way to comment on commit messages in review
>> Current tooling sets the pull request description as the commit message,
>> “Quote Reply” kind of provides a way to inline comment, although it’s not
>> the formal review UI
>> *Proposal*: Tooling should support a “COMMIT_MESSAGE” file in each pull
>> request commit that becomes COMMIT_MESSAGE when a pull request is landed
>> 3) Edit commit messages while creating a change, not just when committing
>> the change
>> The “overwrite” workflow already sort of support this idea by using amend
>> commits instead of appending commits to an existing branch
>> *Proposal*: The above “COMMIT_MESSAGE” file workflow would allow
>> iterative building of a commit message before committing
>>
>
> There is another issue to add here. Right now, change log entry exists for
> many sub directories of the respiratory like Source/WebCore and
> LayoutTests. With a commit message, however, there could be only one per
> patch / PR.
>
> This makes it harder to review each part separately. For example, I'd like
> to focus on core changes first and then onto test changes, or vice versa
> depending on patches I'm reviewing.
>
> So ideally, this commit message field mirror where change log files
> currently exist such that "commit message" for each sub directory will show
> up separately.
>
> 4) Using git commands to view commit messages is hard
>> There don’t seem to be many projects which have a solution for this. In
>> practice, it seems that reducing the scope of messages shown by restricting
>> history to a specific directory or even file is one solution, another is
>> shorter commit messages
>> *Proposal*: Have Tools/Scripts/git-webkit setup configure hooks in
>> contributors local git repositories to lay down CommitMessages.history
>> files on merge, checkout and commit which contain the last 5000 commit
>> messages. We can put these in similar places to where ChangeLogs are today,
>> although we would likely want them in fewer places because this will
>> increase local compute time on many git operations. We could also make this
>> a configurable setting so that engineers who are more comfortable with the
>> raw command line tooling do not have to deal with slower git operations.
>>
>> I like the COMMIT_MESSAGE and hooks proposals because they are opt-in.
>> Contributors who wish to use native git tooling to contribute and interact
>> with the project do not have to use either tool, but the tools are
>> compatible enough with native git workflows that contributors who find
>> editing and viewing commit messages primarily in a text editor
>>
>> Looking forward to the discussion,
>>
>> Jonathan Bedard
>> WebKit Continuous Integration
>> ___
>> webkit-dev mailing list
>> webkit-dev@lists.webkit.org
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>
> --
> - R. Niwa
>
-- 
- R. Niwa
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] ChangeLog Deprecation Plans

[Ryosuke Niwa via webkit-dev]

On Mon, Apr 18, 2022 at 8:30 AM Jonathan Bedard via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> As we migrate WebKit from Subversion to git, I would like to migrate the
> project away from ChangeLogs. The reason for this is that ChangeLogs make
> some of the features of git hard to use, namely, cherry-picking commits
> between branches requires conflict resolution every time.
>

Isn't this something that can be easily resolved with a merge script?

Rotating ChangeLogs is also moderately difficult in git with locked down
> commit access like our project has, only repository administers would, in
> practice, be able to rotate ChangeLogs.
>

It seems like we can just automate this by introducing a change log
rotating bot, which has the same privilege as commit queue.

So these two arguments seem rather weak.

Lastly, ChangeLogs are uncommon in git based projects, so new contributors
> will find them difficult to manage.
>

This might be the strongest argument in favor of deprecating change logs.

2) We need a way to comment on commit messages in review
> Current tooling sets the pull request description as the commit message,
> “Quote Reply” kind of provides a way to inline comment, although it’s not
> the formal review UI
> *Proposal*: Tooling should support a “COMMIT_MESSAGE” file in each pull
> request commit that becomes COMMIT_MESSAGE when a pull request is landed
>

This needs to be a mandatory / automatic system, not opt-in. I want to
comment on commit messages as a reviewer. As a patch author, I don't care
whether it can be easily commented on or not.

But if this is a required thing, then new contributors would have to learn
that this file is auto-generated or needs to be edited manually in some
cases so getting rid of change logs may not necessarily reduce the
cognitive load in comparison to keeping the status quo (i.e. keeping change
log files).

3) Edit commit messages while creating a change, not just when committing
> the change
> The “overwrite” workflow already sort of support this idea by using amend
> commits instead of appending commits to an existing branch
> *Proposal*: The above “COMMIT_MESSAGE” file workflow would allow
> iterative building of a commit message before committing
>

I absolutely despise --amend commits. It's the most annoying thing I have
to do whenever I'm creating patches in a git clone.

I like the COMMIT_MESSAGE and hooks proposals because they are opt-in.
> Contributors who wish to use native git tooling to contribute and interact
> with the project do not have to use either tool, but the tools are
> compatible enough with native git workflows that contributors who find
> editing and viewing commit messages primarily in a text editor
>

I don't think COMMIT_MESSAGE can be opt-in for the aforementioned reasons.

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

Re: [webkit-dev] GitHub Labels

[Ryosuke Niwa via webkit-dev]

On Thu, Mar 10, 2022 at 10:49 AM Jonathan Bedard via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> Hey folks,
>
> We’re in the final stage of bringing up support for GitHub pull-requests.
> To support this effort, we’re starting to add labels to our project. We
> intend to use labels as a replacement for commit-queue flags and
> Product/Component/Version fields in bugzilla. Before our tools are too
> reliant on specific label names, we wanted to solicit feedback to see if
> folks had specific opinions on certain categories. Bellow I have some
> preliminary thoughts on what labels the project would find helpful:
>
> EWS and Merge-Queue labels:
> merge-queue (green): Applied to send a PR to merge-queue (equivalent of a
> modern cq+)
> fast-merge-queue (green): Applied to send a PR to merge-queue, but skip
> building and testing
>

I don't like "fast merge" because who doesn't want fast merging? It doesn't
convey why that's different from regular merge, or why using this option
may not be always desirable. I also don't think "queue" adds much
information about these flags.

So why not simply:

   - merge
   - untested-merge or testless-merge?

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

Re: [webkit-dev] Great function for Cocoa platform, bridge_cast

[Ryosuke Niwa via webkit-dev]

On Wed, Feb 16, 2022 at 6:13 AM Darin Adler  wrote:

> I think of it as following the same naming pattern as downcast<> or
> static_cast<>, but you don’t have to specify a template argument since it
> infers it for you.
>

I guess the closer analogy might be with const_cast, which does the "right
thing" based on the type.

I still think a single name conversion is less clear since we don't
typically call out which type of an object is returned by a function (since
we don't really use Hungarian notation). We'd end up having to look up the
function declaration / definition to see whether it returns CF vs NS/UI.

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

Re: [webkit-dev] Great function for Cocoa platform, bridge_cast

[Ryosuke Niwa via webkit-dev]

On Tue, Feb 15, 2022 at 9:44 AM Darin Adler via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> For those of you doing work with Objective-C on Cocoa platforms, I want to
> draw your attention to a great new idiom. Back in October, David Kilzer
> added bridge_cast, a type-safe set of functions that convert an Objective-C
> pointer to a Core Foundation pointer or vice versa, preserving types across
> the toll-free bridging boundary. It’s better than traditional non-WTF
> idioms where you use casts that look like “(__bridge id)” because you don’t
> have to write the type, and the correct corresponding type is chosen for
> you.
>
> When you have a CFStringRef and need to use it as an NSString key and
> value in an Objective-C dictionary, for example, the idiom would be:
>
> bridge_cast(constantKey): bridge_cast(constantValue),
>
> Rather than the old:
>
> (__bridge id)constantKey: (__bridge id)constantValue,
>
> It converts to NSString *, not id, which is better for many contexts, and
> good here, too. Since the function works in both directions, it will also
> turn an NSDictionary into a CFDictionaryRef. And it works on both raw
> pointers and on RetainPtr. I find it’s even more welcome to have something
> that can convert a RetainPtr into RetainPtr
> without danger of getting the reference counting wrong, doing the right
> thing in both ARC and non-ARC source files, and optimizing the move cases
> appropriately.
>

Was there a particular reason we picked the same name for both conversions?
At first glance, the code would have been easier to read or understand if
it said to which direction we're doing the conversion: e.g.
toCF(constantKey), toNS(constantKey). We do this for WebCore/WebKit type
conversions.

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

Re: [webkit-dev] OK to flatten WTF's header directory?

[Ryosuke Niwa via webkit-dev]

On Tue, Feb 1, 2022 at 12:30 PM Elliott Williams via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> I’m working on fixing some ambiguities in our Xcode projects to permit
> adoption of Xcode’s new build system and better parallelize our builds. I
> noticed that WTF’s headers (/usr/lib/include/wtf) are atypical in that they
> aren’t copied into a single directory – they’re deeply nested and mirror
> the same directory hierarchy as exists in WTF’s sources.
>

What do you mean by this? Could you give us examples?

Is this intentional, and if not, would there be opposition to me flattening
> them into a single directory? Flattening them makes it easier to explain to
> Xcode what headers go where, which is useful for tracking dependencies
> between targets and ensuring proper build order. Headers would still be in
> their same source locations (alongside implementation files) but would be
> copied to the top-level /usr/lib/include/wtf directory at build time.
>
> The only other place in our projects [1] I’m aware of with deeply-nested
> headers is PAL, and there’s a bug to change that:
> https://bugs.webkit.org/show_bug.cgi?id=175705
>
> -Elliott
>
> [1]: libwebrtc also produces deeply-nested headers, but since it’s a
> third-party project, I don’t think our header conventions should apply.
> ___
> 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] WPT first test policy proposal

[Ryosuke Niwa via webkit-dev]

On Fri, Nov 19, 2021 at 1:06 PM Tim Nguyen via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> Hello everyone,
>
> I would like to start a discussion on a policy to enforce WPT usage as a
> first choice, that would be enforced via check-webkit-style on Changelog
> files.
>
> *Why use WPT?*
>
> Contributing to WPT has many benefits:
>
>- interoperability/compatibility issues with WPT we write may be
>detected by other browser vendors and we would get faster feedback and
>turnaround to fix them
>- creates/encourages discussion in case of disagreement with other
>browser vendors
>- WebKit greatly benefits from WPT coverage, it is time to provide our
>own coverage to other browsers
>- Improves WebKit’s score generally for WPT (which has tests mostly
>contributed by Chromium, Firefox at the moment)
>
>
> *Are there reasons to not use WPT?*
>
> Common reasons for not writing WPT that have been mentioned are:
>
>- "WPTs are less pleasant to write.”
>
> This is not true imo, the WPT harness is documented (
> https://web-platform-tests.org/writing-tests/index.html), unlike WebKit
> internals, making it easier for new contributors to figure out things.
>

This is absolutely still the case for me. WPT harness require so much
boilerplate even today.

I absolutely hate having to describe what I'm testing when the test code
speaks for itself. This is akin to our WebKit coding style of not adding
comments that repeat the code.

I also absolutely hate having to start a HTTP server just to test some DOM
Node API. There is absolutely no reason why we need to do that.

Also, why do we need to call setup or done just to avoid a single test()
function call? The harness should be automatically detecting that.

I can go on and on about all other issues I have with WPT.

>
>- “When you actually move a regression test to WPT proper, commit
>history is lost, and you don't know what kind of user facing problem it's
>preventing any more.”
>
> Use `` with a reference to the webkit bug.
> Ensure you actually export the WPT and merge the WPT PR as well!
>

This is yet another thing we have to do. We shouldn't be making writing
WebKit tests harder.

*Here is my current proposal:*
>
> Every LayoutTests/ changelog adding new test files would contain:
>

I'm opposed to introducing a policy like this in general. WebKit project
avoids adding new policies wherever possible.

To me, we make the experience of writing WPT tests so much better & so much
more pleasant that people would prefer writing them over js-test.js tests
on their own, not forced upon by a policy.

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

Re: [webkit-dev] Raw string literals

[Ryosuke Niwa via webkit-dev]

On Wed, Nov 17, 2021 at 2:59 PM Alex Christensen via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> Right now, our style checker disapproves of raw string literals, which
> were introduced in C++11.  It complains with this message:
>
> Multi-line string ("...") found.  This lint script doesn't do well with
> such strings, and may give bogus warnings.  They're ugly and unnecessary,
> and you should use concatenation instead".
>
> https://webkit.org/code-style-guidelines/ says nothing on the subject.  I
> find them quite useful and nice, especially with strings that contain lots
> of quotation marks that would otherwise need escaping.  Would anyone oppose
> to my changing our style checker to allow them if I ever get around to it?
>

I'm supportive of this proposed change to the style checker. But perhaps
the simplest solution is to just get rid of that rule since it's not
mentioned anywhere in the style guidelines.

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

Re: [webkit-dev] Fuzzy Reftest Plans, and Metadata Locations

[Ryosuke Niwa via webkit-dev]

On Thu, Oct 28, 2021 at 10:24 AM Sam Sneddon via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> As part of the ongoing work on GPU Process, we’re interested in adding
> support for reftest fuzzy matching (i.e., allowing a certain amount of
> tolerance when comparing the generated images).
>
> Our intention is to match the semantics of WPT’s reftests (
> https://web-platform-tests.org/writing-tests/reftests.html#fuzzy-matching
> ):
> 
> There are cases where we’ll want to apply these to the tests
> unconditionally, for example where varying behaviour is expected across
> ports (such as anti-aliasing differences), and in these cases for WPT tests
> these annotations should probably be exported upstream.
>
> The current plan, and work is underway to do this, is to support this
> syntax via parsing the HTML in Python when there is a hash mismatch, which
> should minimise the performance impact versus always reading this metadata.
>
> However, this doesn’t entirely suffice. There are cases where we might
> want to allow more tolerance on one platform or another, or vary based on
> GPU model or driver. As such, this requires not only platform specific
> metadata (i.e., similar to that which we have in TestExpectations files
> today), but also expectations with finer granularity.
>

Are we sure we really need that? What are examples of tests that do warrant
such a mechanism?

Generally, we want to keep our testing infrastructure as simple as possible.

One option is to extend the meta content to encode conditional variants,
> though this doesn’t work for WPT tests (unless we get buy-in to upstream
> these annotations into the upstream repo, though that might be desirable
> for the sake of results on wpt.fyi). We would need to be confident that
> this wouldn’t become unwieldy however; we wouldn’t want to end up with
> something like
> (if:port=Apple)maxDifference=1;totalPixels=10,(if:platform=iOS)maxDifference=10;totalPixels=20,(if:port=GTK)maxDifference=10;totalPixels=300.
>
> Another option is to extend TestExpectations to store more specific data
> (though again this might become unwieldy, as we’re unlikely to add new
> “platforms” based on every variable we might want to distinguish results
> on). This also means the metadata is far away from the test itself, and the
> TestExpectations files would continue to grow even further (and we already
> have 34k lines of TestExpectations data!). TestExpectations is also a
> rather horrible file format to modify the parser of.
>

I'm fine with either of the above options but I don't think we should
introduce this kind of micro syntax if we're going with meta.

We should probably specify a platform in a different attribute altogether.
e.g.


I really hate that WPT is using a micro-syntax for this. Why isn't this
simply a different content attribute like this:


There is also test-options.json which has most of the same downsides as
> TestExpectations, albeit without the pain in modifying the parser.
>
> Finally, we could add per-test or per-directory files alongside the tests.
> (Due to how things work, these could presumably also be in directories in
> platform/.) This I think is probably the best option as it keeps the
> metadata near the test, without needing to modify the test (which, per
> above, is problematic for WPT as we move to automatically exporting
> changes). One could imagine either a __dir__-metadata.json (to use a
> similar name to how WPT names directory-level metadata files) or a
> -expected-fuzzy.json file alongside each test.
>

Both of these two options seem worse than either encoding in the test or
putting in the test expectations. They invent a brand new mechanism to
store metadata for tests. We don't want to introduce yet another file /
mechanism people need to be aware of.

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

Re: [webkit-dev] -Wreturn-type and -Wredundant-move reminders

[Ryosuke Niwa via webkit-dev]

On Tue, Oct 19, 2021 at 1:15 PM Michael Catanzaro via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> A reminder about this common idiom:
>
> switch (...) {
> case Foo:
> return ...;
> case Bar:
> return ...;
> }
> RELEASE_ASSERT_NOT_REACHED();
>
> When it's intended that the code always returns inside the switch
> statement, the RELEASE_ASSERT_NOT_REACHED() is required to avoid
> tripping GCC's -Wreturn-type. If you forget it, I or somebody else will
> wind up adding it later to avoid the warning. Clang does not warn here,
> and this is the most common type of warning I clean up, so please don't
> forget! :) This warning is useful in other situations, and it seems
> nicer to placate GCC than to disable it.
>
> I'll sneak in another reminder: "return WTFMove()" is almost always not
> needed. Clang warns only if the move prevents return value
> optimization, but GCC will always warn if it detects the move is
> unneeded.
>

Can we add a style checker rule to detect at least simple cases?

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

Re: [webkit-dev] Network Information API reboot: request for early feedback

[Ryosuke Niwa via webkit-dev]

On Mon, Aug 30, 2021 at 2:58 AM Thomas Steiner  wrote:

> On Mon, Aug 30, 2021 at 11:06 AM Ryosuke Niwa  wrote:
>
>> On Mon, Aug 30, 2021 at 1:17 AM Thomas Steiner  wrote:
>>
>>> On Sun, Aug 29, 2021 at 1:00 AM Ryosuke Niwa  wrote:
>>>
>>>> I don't think exposing the information about whether the connection is
>>>> metered or not is acceptable from the privacy standpoint. Based on the IP
>>>> address of a user & this metered status, a website may even be able to tell
>>>> what kind of carrier plan a given user is in.
>>>>
>>>
>>> Thanks for the reply, Ryosuke! Just to clarify, the `metered` attribute
>>> would be a manual user setting, not a browser heuristic. This means you
>>> could easily mark your all-data included WiFi at home as metered if you
>>> wanted to, or, on the opposite end, mark your roaming data plan you
>>> purchased for a ton of money at the airport as unmetered. None of this
>>> happens without the user voluntarily revealing the information.
>>>
>>
>> I don't think it's fair to characterize any given user telling the OS to
>> reduce the data usage as a consent to be profiled by random websites. I
>> would certainly not expect such information to be exposed to ad trackers
>> and alike.
>>
>
> Apple seems to see no issue in exposing this information to iOS/iPadOS
> apps: https://developer.apple.com/videos/play/wwdc2019/712/?time=959.
>
>
>> You could make the same argument for turning on low power mode but
>> battery level being low or having low power mode turned on may reveal the
>> user's socioeconomic status or user's immediate need to take
>> certain actions. It can lead to egregious consequences like this:
>> https://www.theverge.com/2016/5/20/11721890/uber-surge-pricing-low-battery.
>> I definitely would not want to be seeing ads promoting new SIM cards or ads
>> for a cafe with free WiFi service nearby just because I requested my phone
>> to reduce data usage.
>>
>
> Yes, bad things like that can happen, and the fact that companies like
> Uber make "evil" use of available information is no secret. At the same
> time, companies that make "good" use of information, like Algolia's example
> (
> https://www.algolia.com/blog/engineering/netinfo-api-algolia-javascript-client/),
> hopefully outweigh the disadvantages. We don't prohibit hammers because
> they can be abused as a weapon.
>

Please refer to https://www.w3.org/TR/design-principles/#safe-to-browse.
It's not okay to add a new API which makes the web less safe & private for
users just because the API can be used for good purposes.

And again, the information is exposed to random native apps that can
> likewise profile you. I agree there is a difference between a random native
> app and a random website, but at the same time we have mitigations like
> third-party blocking (and ITP on Apple devices) that make such profiling
> harder and harder.
>

The web and native apps have fundamentally different security &
privacy models. We don't let websites access random files in your system
without an explicit user consent.

On Mon, Aug 30, 2021 at 6:54 AM Thomas Steiner  wrote:

> > None of this happens without the user voluntarily  revealing the
>> > information.
>>
>> How would that possibly work? A new type of permission prompt? It's
>> easy for users to decide whether a website should have geolocation or
>> microphone access, but the risk here is just extra entropy, which is
>> going to be real hard to explain to users.
>>
>
> The current thinking is that there would be no additional permission
> needed. Note that the proposal reduces the overall entropy compared to the
> current API, which exposes more information:
> https://wicg.github.io/netinfo/#networkinformation-interface (compared to
> https://raw.githack.com/tomayac/netinfo/relaunch/index.html#the-networkinformation-interface
> ).
>

That's an API WebKit has never supported and never will for various privacy
& security reasons. Also, I don't think the notion that this old API
exposed more information is necessarily true. The user actively choosing to
use low data mode is very much new information that websites couldn't
necessarily infer from the old API.

Overall, I don't think there is much left to discuss here. What you're
proposing will introduce a serious privacy concern, and it's not acceptable.

I'm going to stop replying to this thread going forward since I have other
things to do but please note that my lack of future response does not, in
any way, constitute a lack of signal or acceptance of an argument, idea, or
amendment to the proposal.

Apple's 

Re: [webkit-dev] Network Information API reboot: request for early feedback

[Ryosuke Niwa via webkit-dev]

On Mon, Aug 30, 2021 at 1:17 AM Thomas Steiner  wrote:

> On Sun, Aug 29, 2021 at 1:00 AM Ryosuke Niwa  wrote:
>
>> I don't think exposing the information about whether the connection is
>> metered or not is acceptable from the privacy standpoint. Based on the IP
>> address of a user & this metered status, a website may even be able to tell
>> what kind of carrier plan a given user is in.
>>
>
> Thanks for the reply, Ryosuke! Just to clarify, the `metered` attribute
> would be a manual user setting, not a browser heuristic. This means you
> could easily mark your all-data included WiFi at home as metered if you
> wanted to, or, on the opposite end, mark your roaming data plan you
> purchased for a ton of money at the airport as unmetered. None of this
> happens without the user voluntarily revealing the information.
>

I don't think it's fair to characterize any given user telling the OS to
reduce the data usage as a consent to be profiled by random websites. I
would certainly not expect such information to be exposed to ad trackers
and alike. You could make the same argument for turning on low power mode
but battery level being low or having low power mode turned on may reveal
the user's socioeconomic status or user's immediate need to take
certain actions. It can lead to egregious consequences like this:
https://www.theverge.com/2016/5/20/11721890/uber-surge-pricing-low-battery.
I definitely would not want to be seeing ads promoting new SIM cards or ads
for a cafe with free WiFi service nearby just because I requested my phone
to reduce data usage.

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

Re: [webkit-dev] Network Information API reboot: request for early feedback

[Ryosuke Niwa via webkit-dev]

I don't think exposing the information about whether the connection is
metered or not is acceptable from the privacy standpoint. Based on the IP
address of a user & this metered status, a website may even be able to tell
what kind of carrier plan a given user is in.

- R. Niwa

On Fri, Aug 20, 2021 at 7:51 AM Thomas Steiner via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> Hey WebKit folks,
>
> I have rebooted the Network Information API recently. This is all in a
> relatively early stage, but I thought now would be a good time to get your
> feedback on the proposal:
>
> - Motivational doc:
> https://docs.google.com/document/d/1RDA23zSNdDuIcxZTX9Xo3zlD2fxf8dZg9-e0YaJQv0g/edit?usp=sharing
> - Explainer: https://github.com/tomayac/netinfo/blob/relaunch/README.md
> - Spec draft: https://ghcdn.rawgit.org/tomayac/netinfo/relaunch/index.html
>
> Here is the short version:
>
> ```js
> // Is the current network a metered network according to the OS-level
> setting
> // in, e.g., Android or Windows, i.e., _without_ the UA guesstimating it.
> The UA
> // may provide its own (override) setting, though:
> navigator.connection.metered;
> // false
>
> // What is the sustained connection speed, as measured on the OS-level (à
> la
> // `nettop`) for a sliding window and bucketed in buckets of exponentially
> // growing size in bit per second, e.g., 25,000,000 (25 Mbit/s), 50,000,000
> // (50 Mbit/s). It's fine to report `Infinity` if the user agent doesn't
> want to
> // reveal more, or if the sustained speed isn't known yet.
> navigator.connection.metered;
> // 5000
>
> // Changes to either of the attributes are exposed via an event:
> navigator.connection.addEventListener("change", (event) => {
>   console.log(event);
> });
> ```
>
> Each of the attributes is accompanied by a client hint header that
> reflects the attribute:
>
> ```bash
> Sec-CH-Metered-Connection: 1
> Sec-CH-Sustained-Speed: 5000
> ```
>
> Thanks in advance for your thoughts, here, or in the motivational document.
>
> Cheers,
> Tom
> ___
> 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] Identifiers in Log and Blame

[Ryosuke Niwa via webkit-dev]

Seems like a good improvement but I really don't use command line tools to
see my blame. What I need is this getting applied to online tools like trac
and GitHub.

- R. Niwa

On Tue, Aug 17, 2021 at 10:57 AM Jonathan Bedard via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> Hi folks,
>
> As we move towards using Git as our version control system, more services
> and scripts will be using identifiers instead of revisions or hashes.
> Already, build.webkit.org, results.webkit.org and ews-build.webkit.org
>  all display identifiers alongside revisions.
> Early in the transition process, we added the git-webkit find command,
> which converts between hashes, revisions and identifiers.  Recently, we
> added the git-webkit log and git-webkit blame commands to better support
> identifiers and native Git checkouts.
>
> git-webkit log is a wrapper around git log or svn log (depending on your
> checkout) and annotates the output of those commands with identifiers and
> revisions. git-webkit log passes the arguments you provide it to your
> native source code management system, it’s output looks something like this:
>
> commit 240602@main (fe5476762fc34d2a5547b7d2d8116faa7275acd7, r281148)
> Author: Eric Hutchison 
> Date:   Tue Aug 17 17:46:39 2021 +
>
> [Monterey wk2 Release]
> performance-api/paint-timing/paint-timing-with-worker.html is a flaky crash.
> rdar://82036119.
> ...
>
> git-webkit blame is a wrapper around git blame or svn blame (again,
> depending on your checkout) and also annotates the output of these commands
> with identifiers:
>
> 230258@main (Keith Rollin2020-10-08 19:10:32 +  1) MODULES =
> Source Tools
> 184786@main (Jonathan Bedard 2017-02-02 18:42:02 +  2)
> 229628@main (Keith Rollin2020-09-22 18:37:51 +  3) define
> build_target_for_each_module
> 229628@main (Keith Rollin2020-09-22 18:37:51 +  4)  for dir
> in $(MODULES); do \
> 229628@main (Keith Rollin2020-09-22 18:37:51 +  5)
> ${MAKE} $@ -C $$dir PATH_FROM_ROOT=$(PATH_FROM_ROOT)/$${dir}; \
> 229628@main (Keith Rollin2020-09-22 18:37:51 +  6)
> exit_status=$$?; \
> 229628@main (Keith Rollin2020-09-22 18:37:51 +  7)  [
> $$exit_status -ne 0 ] && exit $$exit_status; \
> 229628@main (Keith Rollin2020-09-22 18:37:51 +  8)  done; true
> 229628@main (Keith Rollin2020-09-22 18:37:51 +  9) endef
> ...
>
> Both commands can switch the commit representation they display with the
> --identifier, --hash and --revision options.
>
> Additionally, for those using Git checkouts, the conversion from
> Subversion revisions to Git hashes no longer requires your checkout to be
> configured with git-svn. Contributors may find that something like git
> checkout r281146 satisfies whatever need they have to interact with
> Subversion from Git.
>
> All of this has been landed on trunk/main as of r280864/240404@main.
>
> 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

[webkit-dev] content-visibility (Was Re: Request For Position on CSS containment)

[Ryosuke Niwa via webkit-dev]

Please rename the subject when you're going to discuss the work on a new
feature.

On Thu, Jun 24, 2021 at 9:44 AM cathiechen via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> We made a lot of progress regarding CSS containment [1].
> Rob and I have finished the layout containment and size containment [2].\o/
> And the patches of paint containment and style containment are ready for
> review now [3].
>
> So we think now it's time to move on to content-visibility:
> (https://www.w3.org/TR/css-contain-2/#content-visibility)
>

That seems premature. Have we implemented all the perf optimizations for
layout, size, & paint containment? I'd rather not start piling on more
features before we get to a point where we're happy with the performance of
these features.

Since content-visibility depends on paint and style containment, we will do
> some specification research first, then prototype it based on Rob's patches.
>

Does the research part also include making a judgement call as to whether
it's a good idea at all? It's wholly unclear to me that content-visibility
is a feature we'd like to implement in WebKit given its implications to the
accessibility and other browser features.

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

[webkit-dev] WeakHashMap

[Ryosuke Niwa via webkit-dev]

Hi all,

I've added WeakHashMap to WTF in https://commits.webkit.org/r278803, which
allows the use of WeakPtr as keys. Like WeakHashSet, WeakHashMap does not
immediately delete the key or the value when the object pointed by
WeakPtr goes away (i.e. WeakPtrImpl::m_ptr becomes nullptr).

Instead, it relies on rehashing and amortized deletion to remove these
entries. This happens either when enough items are inserted or removed such
that the underlying HashMap goes through rehashing or when WeakHashMap's
entry is accessed (get, find, advancing an iterator, etc...) or mutation
(inserting, removing, etc...) more than twice the number of items in the
underlying HashMap.

Due to this amortized deletion behavior, there is no unbound growth of
HashMap even if no attempt to cleanup null WeakPtr is made. However, if
WeakHashMap is never accessed or mutated, all WeakPtrImpl as well as the
corresponding hash map values will be kept alive. In the situation where
this is undesirable (e.g. WeakHashMap could retain a large object as its
values or a large number of keys could become stale), call
WeakHashMap::nullReferences() to trigger the cleanup manually.

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

Re: [webkit-dev] Request for Position: Compute Pressure API

[Ryosuke Niwa via webkit-dev]

On Thu, May 6, 2021 at 10:55 AM Olivier Yiptong 
wrote:
>
> Hi, and thank you for your prompt response. Replies inline.
>>
>>1. CPU utilization isn't something which can be easily computed or
>>reasoned on asymmetric multi-core CPUs, not to mention the dynamic
>>adjustment of CPU frequency further complicates the matter.
>
>
> Are you alluding to CPU architectures like big.LITTLE? and of dynamic
clock frequencies?
>
> This proposal takes this into account, here's how:
>
...
>
> The goal of CPU Speed is to abstract away those details and provide clock
frequency data that is actionable and yet preserves user privacy.

The issue isn't that it's unclear to come up with some number that
represents the current CPU load. I'm sure we can come up with some number.
The issue lies in how such a number can be interpreted.

In our experience with iOS and macOS, CPU utilization & speed are poor
metric to use in order to adjust any software behavior because those things
are highly dynamic and respond quickly based on what an application is
doing. It's a lot better to *measure* the actual runtime taken to do a
specific task and adjust the behavior accordingly.

>>2. Whether the system itself is under a heavy CPU load or not should
not
>>have any bearing on how much CPU time a website is entitled to use
because
>>the background CPU utilization may spontaneously change, and the
reason of
>>a high or a low CPU utilization may depend on what the website is
doing;
>>e.g. a daemon which wakes up in a response to a network request or
some
>>file access.
>
> On the web, people write one-size-fits-all applications. Those need to
run across devices of varying compute power capabilities.
> For compute-intensive applications, this poses problems and leads to bad
UX in the absence of hints from the system.

This is precisely why we can't rely on CPU utilization or speed to
determine how fast the application or specific task thereof will complete.
There is a huge variability in each CPU's instructions per cycle, and how
much work can be performed in each cycle. The size of L1/L2/L3, cache
coherency mechanisms (with other cores potentially), prefetcher, the
capability and size of the branch predictor, etc... can all influence how
fast a given application will run. We can't estimate how fast an
application will run based purely on the percentage of CPU utilization or
at what fraction of the maximum frequency / power CPU is operating.

> The proposal does not enforce any additional usage of resources, but
instead allows apps to make informed decisions.
> It is common for compute-intensive applications to self-throttle to
provide a good UX.
> One example is gaming: reducing drawing distance, effects, texture sizes
or level of detail for geometries if it's affecting frame rate.

Those games are better off dynamically adjusting their behavior based on
how long each frame is taking to draw.

> On the Nintendo Switch, game engines have the feature to reduce the
rendering resolution when framerate drops occur or are anticipated.
> On the Nintendo Switch, the compute power capability depends on whether
the device is plugged in or in portable mode.
> There might also be thermal factors.

Here is the thing. On Apple's platforms, if an application adjusts itself
to do less work, then we'd start throttling CPU or stop using P-cores
automatically to conserve the battery so then you might drop frames because
CPU is running at a slower speed or no longer running in P-cores and takes
sometime to ramp up again. It is impractical for an application to respond
to these changes based on CPU utilization or speed information because the
states are changing so dynamically over time.

It's also highly inappropriate for a web app to assume that it can use all
the remaining CPU resources when there could be other windows and
applications the user is interacting with.

> For the Compute Pressure API, we've examined a few use-cases, and they
are detailed in the explainer.
> This is similar to video conferencing needs, reducing the number of
simultaneous video feeds, or diminishing the processing of image processing
effects.
>
> Indeed, the reason for the load might be intrinsic to the application, or
extrinsic, based on what's going on with the system.
> This API, as proposed, provides system-level hints that help applications
make informed decisions to provide a good UX for users.

We're very much uncomfortable with exposing this kind of invasive system
information in a Web API, and more importantly, web applications to adjust
its workload based on such information. JavaScriptCore's is a highly
sophisticated JIT engine that is known to perform very well across variety
of hardware classes and generations; yet it doesn't adjust its workload
based on CPU utilization or power state of the system. Given that, we are
highly skeptical with your premise that an API like this is needed to
create a performant application in the first 

Re: [webkit-dev] Request for Position: Compute Pressure API

[Ryosuke Niwa via webkit-dev]

On Wed, May 5, 2021 at 11:37 AM Olivier Yiptong via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

>
> We propose a new API that conveys the utilization of CPU resources on the
> user's device. This API targets applications that can trade off CPU
> resources for an improved user experience. For example, many applications
> can render video effects with varying degrees of sophistication. These
> applications aim to provide the best user experience, while avoiding
> driving the user's device in a high CPU utilization regime.
>
> High CPU utilization is undesirable because it strongly degrades the user
> experience. Many smartphones, tablets and laptops become uncomfortably hot
> to the touch. The fans in laptops and desktops become so loud that they
> disrupt conversations or the users’ ability to focus. In many cases, a
> device under high CPU utilization appears to be unresponsive, as the
> operating system may fail to schedule the threads advancing the task that
> the user is waiting for.
>
> Thanks!
>
>
>- Specification Title: Compute Pressure API
>- Specification URL: https://oyiptong.github.io/compute-pressure/
>- Explainger:
>https://github.com/oyiptong/compute-pressure/blob/main/README.md
>- ChromeStatus.com entry:
>https://chromestatus.com/features/5597608644968448
>- TAG design review request:
>https://github.com/w3ctag/design-reviews/issues/621
>- Mozilla Request for Position:
>https://github.com/mozilla/standards-positions/issues/521
>
> We do not support this proposal for the reasons including but not limited
to:

   1. CPU utilization isn't something which can be easily computed or
   reasoned on asymmetric multi-core CPUs, not to mention the dynamic
   adjustment of CPU frequency further complicates the matter.
   2. Whether the system itself is under a heavy CPU load or not should not
   have any bearing on how much CPU time a website is entitled to use because
   the background CPU utilization may spontaneously change, and the reason of
   a high or a low CPU utilization may depend on what the website is doing;
   e.g. a daemon which wakes up in a response to a network request or some
   file access.
   3. The proposal as it currently stands seems to allow a side channel
   communication between different top-level origins (i.e. bypasses storage
   partitioning). A possible attack may involve busy looping or doing some
   heavy computation in one origin and then observing that CPU utilization
   goes up in another. We've reported an attack of a similar nature in
   https://bugs.chromium.org/p/chromium/issues/detail?id=1126324.

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

Re: [webkit-dev] New EWS Non-Unified builder

[Ryosuke Niwa via webkit-dev]

On Fri, Apr 30, 2021 at 10:16 AM Alexey Proskuryakov via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> These points from my yesterday email remain without responses:
>
> 1. Cannot have an EWS without corresponding post-commit queue.
>

Yeah, we should first add a bot.

2. It doesn't appear like we looked into whether there are any ways to
> mitigate the problem other that this most costly one.


The reason build may break when a new file is added or removed is because
all existing files shift. We could mitigate this if we always added a new
file at the end of existing unified build files, and always left an empty
spot when removing a file. I believe we don't unify files across top-level
directories so I could imagine, for example, we can say that we always add
a file at the end of Sources.txt per top-level directory. We could then
periodically sort files lexicologically with an understanding that this
will require a lot of build fixes to be applied at the same time.

For ports that don't use unified builds, one solution might be to create
wrapper translation units or unified header files. Say we're compiling
A.cpp and B.cpp as Unified1.cpp, then we'd basically create the union of
header files included in A.cpp and B.cpp and put into Unified1.h. Then this
port will build wrapper translation units A-unified.cpp and B-unified.cpp
each of which respectively includes A.cpp and B.cpp as well as Unified1.h.

- R. Niwa

> 30 апр. 2021 г., в 8:43 AM, Darin Adler via webkit-dev <
> webkit-dev@lists.webkit.org> написал(а):
>
>
> > OK. I acknowledge my view on this is different from the others
> commenting here, perhaps because of different ideas about what is hard and
> requires expertise. I won’t stand in the way of changes you want to make.
> You know my view now.
> >
> > — 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] New EWS Non-Unified builder

[Ryosuke Niwa via webkit-dev]

On Thu, Apr 29, 2021 at 9:18 PM Darin Adler via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> > On Apr 29, 2021, at 9:06 PM, Tim Horton via webkit-dev <
> webkit-dev@lists.webkit.org> wrote:
> >
> > it is definitely highly annoying
>
> It’s possible that where my thinking differs from others on this is that I
> don’t think shifting annoying work from one set of commits (the ones adding
> a new file) to a different set (the ones unknowingly adding need for a new
> include for non-unified builds) is a significant improvement. Adding more
> EWS bubbles has a cost.
>

One key benefit of keeping non-unified builds working is that it would make
build brokerage more predictable. Today, when you're adding, removing, or
renaming an existing new file, it may compile just fine on the particular
port / platform you're using locally yet can still cause a build failure in
other ports / platforms since different ports / platforms almost always
compile a different set of translation units. If we kept unified builds
always working, then the build failure will happen more consistently,
making it easier to detect before uploading a patch instead of after
observing EWS failures or worse after landing commits because EWS didn't
have coverage.

Every time someone new starts working on the WebKit project, the topic of
mysterious build failures caused by the unified build system comes up as a
topic. It's yet another random thing a new developer would have to learn.
Anything we can do to reduce the total amount of random knowledge someone
has to have to have to be productive in the WebKit project is a positive
change in my opinion.

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

Re: [webkit-dev] Request for position on First-Party Sets

[Ryosuke Niwa via webkit-dev]

On Tue, Apr 13, 2021 at 1:52 PM Kaustubha Govind via webkit-dev
 wrote:
>
> [Resending after subscribing to webkit-dev, since my previous message bounced 
> back]
>
> On Tue, Apr 13, 2021 at 4:47 PM Kaustubha Govind  
> wrote:
>>
>> Hi Maciej, Webkit team,
>>
>> Now that First-Party Sets has been incubating within PrivacyCG for ~6 
>> months, I wanted to check with you to see if you have reconsidered your 
>> position on the proposal. It seems WebKit may intend to use First-Party Sets 
>> relationships to apply to browser policies other than cookie blocking 
>> (https://github.com/w3ctag/design-reviews/issues/342#issuecomment-801517385),
>>  but I wasn't sure whether to construe that as positive progress in your 
>> position.

I'm not certain in what way John's comment could be interpreted like
that but WebKit does not have such a plan and Apple's WebKit team
continues to oppose this proposal. I don't think there has been
substantive changes to address the various concerns we have raised so
far.

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

Re: [webkit-dev] Request for position: Aligning high-resolution timer granularity to cross-origin isolated capability

[Ryosuke Niwa via webkit-dev]

On Thu, Mar 18, 2021 at 12:26 AM Yoav Weiss via webkit-dev
 wrote:
>
> On Wed, Mar 17, 2021 at 5:51 PM Geoff Garen  wrote:
>>
>> For the 100 microsecond value — our research suggests that you need a much 
>> higher value in vulnerable contexts.
>>
>> For the guaranteed isolated case — have you considered the use of high 
>> precision time to carry out non-Spectre timing attacks?
>
> Could you elaborate on those 2 points?

We've made a conclusion, based on our prior research, that in order to
successfully mitigate Spectre / Meltdown class of attacks, we can't
allow 100μs precision timing measurements. As such, we have no plan or
desire to increase the precision of "high precision" time from 1ms to
100μs. I'm not going to provide details as to how or why due to the
nature of the topic.

The second point is that there are dangerous timing attacks besides
from Spectre/Meltdown that are effective with a precision meaningfully
higher than 100μs. This is why the precision of WebKit's high
resolution time had been reduced to 100μs in
https://trac.webkit.org/r209462 even prior to the issue of Spectre /
Meltdown were identified. There are a number of literatures on various
kinds of timing attacks possible, but again, I'd refrain from
disclosing details here.

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

Re: [webkit-dev] Request for Position on Sanitizer API

[Ryosuke Niwa via webkit-dev]

On Mon, Mar 15, 2021 at 7:32 AM Daniel Vogelheim via webkit-dev
 wrote:
>
> I'd like to request a position statement on the proposed Sanitizer API.
>
> The Sanitizer API wants to build an HTML Sanitizer right into the web 
> platform. The goal is to make it easier to build XSS-free web applications. 
> The intended contributions of the Sanitizer API are: Making a sanitizer more 
> easily accessible to web developers; be easy to use and safe by default; and 
> shift part of the maintenance burden to the platform.
>
> Currently available are an explainer and an early spec draft, and early 
> prototype implementations in Chromium & Firefox, behind flags.

I'm gathering more feedback internally at Apple but here's immediate
feedback I can give you: even if this was an useful API for web
developers, we won't use it to sanitize the content from / to the
system pasteboard (a.k.a clipboard on Windows) since we rely on style
& rendering information and apply various transformations such as
inlining all the style rules for that purpose. Secondly, we probably
won't reuse this code for sanitizing contents inside our engine since
using hash maps of element names and attribute names per element to
allow or block markup would be simply too inefficient. Reusing
concepts defined in this specification as a mechanism involved by
other specifications seems okay provided we agree that this API / spec
is an overall good idea based on more broader discussion.

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

Re: [webkit-dev] Position on emerging standard: Declarative Shadow DOM

[Ryosuke Niwa via webkit-dev]

On Tue, Feb 23, 2021 at 5:06 PM Mason Freed via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> > On Fri Feb 19 20:36:12 PST 2021 Ryosuke Niwa via webkit-dev <
> > webkit-dev at lists.webkit.org> wrote:
> >
> > I replied in the issues directly so that people outside of the WebKit
> > community can follow the discussion.
>
> Thanks! I've replied there also.
>

The issue with the semantics of getInnerHTML is problematic enough that
we'd object to this feature as long as that's included in its current shape.

> How does one specify a declarative shadow root to use a specific custom
> > element registry?
>
> Well, I do see this as something that is better designed as part of the
> scoped custom element registry proposal. But the basic idea would be to
> just to add an attribute that allows the declarative shadow root to opt
> out of automatically using the global registry:
>
> 
>
> and that would keep any custom elements contained within the shadow root
> from automatically upgrading based on the global registry. Maybe by just
> assigning an empty custom registry to that shadow root. We'll need to add
> an attribute to ShadowRoot, as part of the SCER proposal, to allow custom
> elements to then set the appropriate custom registry later.
>

That seems to indicate that none of the custom elements inside a
declarative shadow root can be upgraded until all its ancestor custom
elements which use a custom element registry have been upgraded. This would
make the declarative shadow DOM's perf benefit less attractive IMO.

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

Re: [webkit-dev] Position on emerging standard: Declarative Shadow DOM

[Ryosuke Niwa via webkit-dev]

On Fri, Feb 19, 2021 at 2:38 PM Mason Freed via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> On Thu Feb 18 17:08:18 PST 2021 Ryosuke Niwa via webkit-dev <
> webkit-dev at lists.webkit.org> wrote:
>
> > The latest proposal does solve much of the problems we've identified in
> the
> > past, and it's looking to be a promising direction. Do you have any
> example
> > website or app that you can share with which resulted in the observed 15%
> > improvement in FCP? That would be a very intriguing observation and would
> > substantiate the support for this feature.
>
> Glad to hear you think it's promising! The example is from the AMP team,
> and you can see more detail at the Origin Trial results summary here:
>
> https://docs.google.com/document/d/1QmBKxQLE81PsMzyPCvYhzqRAn4hxz61Jzk0uXJAhaVc/edit
>
>
> > However, as I commented on https://github.com/whatwg/dom/pull/892 and
> have
> > previously stated during the last F2F and other avenues, the currently
> > proposed semantics of getInnerHTML is deeply problematic. We want
> > consistent semantics across different kinds of Web API, and what's being
> > proposed is very much different from what we had discussed what we would
> do
> > for selection.
>
> I just replied to the comment you made yesterday on DOM issue #892. I
> looked
> back at the F2F minutes (
> https://www.w3.org/2020/03/23-components-minutes.html)
> to refresh my memory, and it turns out the current API was actually
> suggested
> (by annevk) at that meeting. I don't see any comments from you about the
> API
> shape there, but perhaps I missed it?
>
> In terms of API shape, can you clarify what you feel is deeply problematic
> about it? I assume you're talking about the need to pass in closed shadow
> roots, to enable them to be serialized. I see that your comment there
> suggests
> the requirement to pass in *all* shadow roots, even open ones. But the
> linked
> comment on selection actually refers to just closed shadow roots also. I'm
> unclear why you would want/need to pass in open shadow roots?
>

I replied in the issues directly so that people outside of the WebKit
community can follow the discussion.

> Also, have people figured out how scoped custom element registries can
> > integrate with this feature in the future? Given that's the other most
> > frequently requested feature, it would be very regrettable if we later
> > found out some inconsistencies or awkwardness in their integrations.
>
> Yes, I've talked several times to justinfagnani about Scoped Custom Element
> Registries, and he sees no problem integrating with DSD. We'll likely
> just need to add an attribute that opts the shadow root out of using the
> global registry, but that seems straightforward. Do you have any particular
> concerns?
>

How does one specify a declarative shadow root to use a specific custom
element registry?

- R. Niwa

On Thu, Feb 18, 2021 at 3:16 PM Mason Freed  wrote:
>
>> Hello WebKit,
>>
>> I just wanted to send a final heads-up that Chromium is intending to ship
>> the Declarative Shadow DOM feature. We haven't heard much back from WebKit
>> in the last 5 months or so, but in the meantime there has been some good
>> discussion with Mozilla and the broader community. Several more performance
>> investigations have been performed, around the overhead of Shadow DOM
>> generally, and around the potential polyfill alternatives for DSD. You can
>> see a summary of these, plus all of the other changes that we've
>> incorporated, in this comment on the Mozilla Standards Position thread
>> <https://github.com/mozilla/standards-positions/issues/335#issuecomment-781697858>.
>> These changes are the result of your (and the community's) involvement and
>> feedback, so thanks. At this point, we believe all of the feedback has been
>> addressed.
>>
>> Thanks,
>> Mason
>>
>>
>> On Tue, Aug 11, 2020 at 10:27 AM Mason Freed 
>> wrote:
>>
>>> Yes, thanks! Welcome back!
>>>
>>> On Mon, Aug 10, 2020 at 3:24 PM Ryosuke Niwa  wrote:
>>>
>>>> Hi,
>>>>
>>>> Sorry for the late reply. I've been on a medical leave. I just
>>>> commented on https://github.com/whatwg/dom/issues/831
>>>>
>>>> - R. Niwa
>>>>
>>>> On Tue, Aug 4, 2020 at 4:26 PM Mason Freed 
>>>> wrote:
>>>> >
>>>> > Hello WebKit folks,
>>>> >
>>>> > I just wanted to quickly ping this thread to see if there was any
>>>> interest in posting a position on declarative Shadow DOM. My orig

Re: [webkit-dev] Position on emerging standard: Declarative Shadow DOM

[Ryosuke Niwa via webkit-dev]

On Thu, Feb 18, 2021 at 3:17 PM Mason Freed via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> I just wanted to send a final heads-up that Chromium is intending to ship
> the Declarative Shadow DOM feature. We haven't heard much back from WebKit
> in the last 5 months or so, but in the meantime there has been some good
> discussion with Mozilla and the broader community. Several more performance
> investigations have been performed, around the overhead of Shadow DOM
> generally, and around the potential polyfill alternatives for DSD. You can
> see a summary of these, plus all of the other changes that we've
> incorporated, in this comment on the Mozilla Standards Position thread
> .
> These changes are the result of your (and the community's) involvement and
> feedback, so thanks. At this point, we believe all of the feedback has been
> addressed.
>

The latest proposal does solve much of the problems we've identified in the
past, and it's looking to be a promising direction. Do you have any example
website or app that you can share with which resulted in the observed 15%
improvement in FCP? That would be a very intriguing observation and would
substantiate the support for this feature.

However, as I commented on https://github.com/whatwg/dom/pull/892 and have
previously stated during the last F2F and other avenues, the currently
proposed semantics of getInnerHTML is deeply problematic. We want
consistent semantics across different kinds of Web API, and what's being
proposed is very much different from what we had discussed what we would do
for selection.

Also, have people figured out how scoped custom element registries can
integrate with this feature in the future? Given that's the other most
frequently requested feature, it would be very regrettable if we later
found out some inconsistencies or awkwardness in their integrations.

- R. Niwa

On Tue, Aug 11, 2020 at 10:27 AM Mason Freed 
> wrote:
>
>> Yes, thanks! Welcome back!
>>
>> On Mon, Aug 10, 2020 at 3:24 PM
>> - R. Niwa
>>  wrote:
>>
>>> Hi,
>>>
>>> Sorry for the late reply. I've been on a medical leave. I just
>>> commented on https://github.com/whatwg/dom/issues/831
>>>
>>> - R. Niwa
>>>
>>> On Tue, Aug 4, 2020 at 4:26 PM Mason Freed 
>>> wrote:
>>> >
>>> > Hello WebKit folks,
>>> >
>>> > I just wanted to quickly ping this thread to see if there was any
>>> interest in posting a position on declarative Shadow DOM. My original post
>>> here didn't gather much feedback. :-)
>>> >
>>> > Thanks,
>>> > Mason
>>> >
>>> >
>>> > On Tue, May 26, 2020 at 12:11 PM Mason Freed 
>>> wrote:
>>> >>
>>> >> Hello WebKit!
>>> >>
>>> >> I would like to request an official WebKit position on the
>>> Declarative Shadow DOM proposal. There have been some great comments and
>>> discussion from WebKit folks on the issue thread, but it is a bit unclear
>>> whether the overall proposal is something WebKit would support. This was
>>> brought up and discussed, e.g., on the DOM spec PR here:
>>> https://github.com/whatwg/dom/pull/858#issuecomment-623735890
>>> >>
>>> >> Please see below for all of the relevant supporting documents and
>>> discussions.
>>> >>
>>> >> Explainer:
>>> https://github.com/mfreed7/declarative-shadow-dom/blob/master/README.md
>>> >> WhatWG DOM Issue discussion: https://github.com/whatwg/dom/issues/831
>>> >> HTML Spec PR: https://github.com/whatwg/html/pull/5465
>>> >> DOM Spec PR: https://github.com/whatwg/dom/pull/858
>>> >> TAG review: https://github.com/w3ctag/design-reviews/issues/494
>>> >> Request for Mozilla Position:
>>> https://github.com/mozilla/standards-positions/issues/335
>>> >>
>>> >> Thanks,
>>> >> Mason Freed
>>> >>
>>> > ___
>>> > 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] Identifier Conversion Tooling

[Ryosuke Niwa via webkit-dev]

On Fri, Feb 12, 2021 at 2:58 PM Jonathan Bedard via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> Hello contributors,
>
> As we move forward with the transition to GitHub, we are starting to adopt
> identifiers  in tooling
> and infrastructure. This is going to take a few weeks, but you will start
> seeing links based on identifiers more frequently. During this transition
> period, not all tooling will work with identifiers, and it’s possible there
> are tools you rely on that aren’t in common use so will be slow in
> receiving support. To that end, I would like to share the tools available
> to translate between hashes, revisions and identifiers in WebKit, along
> with the Python libraries backing that tooling if you feel motivated to
> expedite the transition for a workflow that is particularly important to
> you.
>
> Tools/Scripts/git-webkit find  is the local script that can convert
> between revisions, hashes and identifiers. By default, it uses your local
> checkout, which means it may be restricted by your checkout’s
> configuration. For example, a pure Subversion checkout will be unable to
> convert hashes and a pure Git checkout will be unable to convert revisions,
> while a Git-Svn checkout will be able to translate all three formats.
>
> If you want to be certain that a hash or revision can be converted to an
> identifier, running Tools/Scripts/git-webkit -C
> https://svn.webkit.org/repository/webkit find  or 
> Tools/Scripts/git-webkit
> -C https://github.com/WebKit/Webkit find  are options. These command
> operate entirely on the network, instead of relying on your local checkout.
>
> Both variations of git-webkit find are ultimately thin wrappers around
> webkitscmpy local.Scm and remote.Scm classes, so if you find yourself
> working with Python code, consider leveraging the APIs directly.
>
> Lastly, along with redirecting to trac/GitHub, the site
> https://commits.webkit.org has a JSON API which vends a representation of
> a commit which includes the hash, revision and identifier and looks like
> this: https://commits.webkit.org/234036@main/json. commits.webkit.org is
> a service we are dedicated to maintaining permanently, since it allows us
> to vend commit links with identifiers instead of hashes.
>

Can we add some simple UI to that site so that it's more human friendly?

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

Re: [webkit-dev] Request for position: Critical-CH response header, part of Client Hints Reliability proposal

[Ryosuke Niwa via webkit-dev]

I'm still confused here. In what scenario would a browser decide to not
send client hints but later decide it's okay to send them?

On Thu, Jan 28, 2021 at 7:13 PM Aaron Tagliaboschi 
wrote:

> The Critical-CH header can trigger a request re-try. It's for situations
> where the browser could be unaware of the site's CH preferences (like the
> first navigation request to a site before the browser has received and
> stored CH preferences) or if a site has changed those references, and the
> site would rather drop the request and retry over getting a potentially
> "incomplete" request
>
> This would *not* override potential mitigations or reductions in
> fingerprinting surfaces imposed by the browser. Any headers that would be
> blocked would still be silently dropped.
>
> (cc davidben, mjs who I forgot to CC the first time)
>
> On Thu, Jan 28, 2021 at 9:35 PM Ryosuke Niwa  wrote:
>
>> What's the point of specifying Critical-CH as opposed to relying on CH
>> provided by the browser?
>>
>> Is the idea that some browsers may decide to hide some client hints to
>> reduce the fingerprinting surface?
>> If so, then this new header seems to just defeat that because a website
>> can specify all the client hints as critical.
>>
>> - R. Niwa
>>
>> On Wed, Jan 27, 2021 at 4:40 AM Aaron Tagliaboschi via webkit-dev <
>> webkit-dev@lists.webkit.org> wrote:
>>
>>> Explainer:
>>> https://github.com/WICG/client-hints-infrastructure/blob/master/reliability.md#critical-ch
>>> Draft Spec:
>>> https://tools.ietf.org/html/draft-davidben-http-client-hint-reliability-02#section-3
>>>
>>> The Client Hint Reliability proposal is a set of features aimed at
>>> making Client Hints
>>> <https://tools.ietf.org/html/draft-ietf-httpbis-client-hints-15> more
>>> reliably available and mitigating
>>> mis-matches between a site's preferences and the preferences stored in
>>> the browser. The idea
>>> behind the Critical-CH response header is to signal to browsers that
>>> there are hints the server
>>> would rather pay a round trip than not have not the first request. The
>>> basic algorithm is as follows:
>>>
>>> If, after receiving a request with Critical-CH and Accept-CH headers,
>>> there is a hint indicated in
>>> the Critical-CH header that the browser did not send but would not block
>>> sending, the browser
>>> should store the new CH preferences, drop the request, and start a new
>>> one with the new
>>> headers included.
>>>
>>> Aaron Tagliaboschi | Software Engineer, Chrome Trust & Safety
>>> ___
>>> 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: Critical-CH response header, part of Client Hints Reliability proposal

[Ryosuke Niwa via webkit-dev]

What's the point of specifying Critical-CH as opposed to relying on CH
provided by the browser?

Is the idea that some browsers may decide to hide some client hints to
reduce the fingerprinting surface?
If so, then this new header seems to just defeat that because a website can
specify all the client hints as critical.

- R. Niwa

On Wed, Jan 27, 2021 at 4:40 AM Aaron Tagliaboschi via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

> Explainer:
> https://github.com/WICG/client-hints-infrastructure/blob/master/reliability.md#critical-ch
> Draft Spec:
> https://tools.ietf.org/html/draft-davidben-http-client-hint-reliability-02#section-3
>
> The Client Hint Reliability proposal is a set of features aimed at making
> Client Hints
>  more
> reliably available and mitigating
> mis-matches between a site's preferences and the preferences stored in the
> browser. The idea
> behind the Critical-CH response header is to signal to browsers that there
> are hints the server
> would rather pay a round trip than not have not the first request. The
> basic algorithm is as follows:
>
> If, after receiving a request with Critical-CH and Accept-CH headers,
> there is a hint indicated in
> the Critical-CH header that the browser did not send but would not block
> sending, the browser
> should store the new CH preferences, drop the request, and start a new one
> with the new
> headers included.
>
> Aaron Tagliaboschi | Software Engineer, Chrome Trust & Safety
> ___
> 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] Github mirror is not updating

[Ryosuke Niwa via webkit-dev]

On Thu, Nov 26, 2020 at 11:33 PM Adrien Destugues via webkit-dev <
webkit-dev@lists.webkit.org> wrote:

>
> I noticed that the github mirror at https://github.com/webkit/webkit is
> not getting
> the latest commits from WebKit (it is now about a month behind). Is that
> intentional?
>

We're actively working on resolving this issue. This current disruption is
caused by GitHub repository preferring to Git repository created off of
accessing the source-of-the-truth subversion repository over HTTPS instead
of accessing it over HTTP. However, since we're in the midst of Git
transition and this transition requires us adding a new numeric
identifier to each commit message, we're doing that work so that we can
push one new Git repository to GitHub instead of temporarily fixing this
issue and then later causing a disruption again. Sorry for the
inconveniences & thanks for your understanding.

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

Re: [webkit-dev] Request for position on Element Timing

[Ryosuke Niwa via webkit-dev]

On Wed, Nov 25, 2020 at 8:04 AM Nicolás Peña Moreno  wrote:

> Chromium also does tiling for paint, but I'm still not sure about the
> relevance of that. In our implementation, we just observe that the loaded
> element has painted in the renderer process, and then wait for the
> presentation timestamp of the committed frame.
>

The relevance is that just because the tile gets painted, it doesn't mean
the content is also painted. Teasing out the two will involve some
housekeeping which isn't readily available.

At this point, I'm going to stop responding to this thread. However, the
lack of further response does not imply endorsement of this API nor does it
mean previously stated problems have become irrelevant or
adequately addressed. All the previously stated problems continue to exist
with the specification and as such, we do not support this API.

- R. Niwa

On Tue, Nov 24, 2020 at 1:41 PM Ryosuke Niwa  wrote:
>
>>
>> On Tue, Nov 24, 2020 at 8:23 AM Nicolás Peña Moreno 
>> wrote:
>>
>>> Thanks for taking the time to review. I received this on my spam folder
>>> for some reason so apologies for the delay in replying.
>>>
>>> On Tue, Nov 3, 2020 at 3:31 AM Ryosuke Niwa  wrote:
>>>
>>>> On Fri, Oct 30, 2020 at 1:58 PM Nicolás Peña Moreno 
>>>> wrote:
>>>> >
>>>> > Hi, I'd like to request WebKit's position on the Element Timing API,
>>>> which lets web developers annotate images or text whose performance they
>>>> care about. They can then obtain rendering timestamps from the
>>>> PerformanceObserver. For cross-origin images the detailed information is
>>>> gated on Timing-Allow-Origin. The proposed specification is at
>>>> https://wicg.github.io/element-timing/ and is currently shipped in
>>>> Chromium. Thanks!
>>>>
>>>> Apple's WebKit team reviewed this API and we have a few
>>>> concerns including but not limited to:
>>>>
>>>>- The proposed API exposes timing at which a given element is
>>>>painted. Implemented naively, this exposes the implementation detail of
>>>>what kind of compositing tiles are used on a given web page. Hiding this
>>>>implementation detail and recording the exact theoretical paint timing 
>>>> will
>>>>be prohibitively expensive to do on all websites.
>>>>
>>>> Note that this only requires exposing the paint timestamp when the
>>> developer requires it explicitly.
>>>
>>
>> I don't see how that's relevant.
>>
>>
>>> It is also implemented similarly to the 'mark paint timing' algorithm,
>>> which is already implemented in WebKit.
>>>
>>
>> Mark paint timing is easier to implement because the granularity is for
>> the whole document, not per element basis. Because WebKit splits the
>> viewport into multiple tiles, and paint invalidation & painting is done per
>> tile, there isn't an easy way to isolate elements being painted from how
>> tiles are generated.
>>
>>>
>>>>- The definition of the set of owned text nodes and how they
>>>>compute intersectionRect seems inadequate. It's unclear what "border 
>>>> box"
>>>>of *Text* node would mean. The spec doesn't seem to ever populate
>>>>"set of elements with rendered text" either.
>>>>
>>>>
>>> Indeed, the border box issue is tracked on
>>> https://github.com/WICG/element-timing/issues/33 and
>>> https://github.com/w3c/csswg-drafts/issues/4197. The set is populated
>>> on https://wicg.github.io/element-timing/#sec-element-processing.
>>>
>>>
>>>>
>>>>- The use of this API seems to incur a significant runtime as well
>>>>as memory cost.
>>>>
>>>> The computations and memory should be limited to the annotated
>>> elements, thus not impacting developers that do not use the API. I'll send
>>> a PR to make that better in the spec, and additional suggestions on
>>> mitigation are welcome.
>>>
>>
>> That is still a major concern since painting time is one of the most
>> costly operations that happens during page loads still.
>>
>> - R. Niwa
>>
>>
___
webkit-dev mailing list
webkit-dev@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-dev

Re: [webkit-dev] Request for position on Element Timing

[Ryosuke Niwa via webkit-dev]

On Tue, Nov 24, 2020 at 8:23 AM Nicolás Peña Moreno  wrote:

> Thanks for taking the time to review. I received this on my spam folder
> for some reason so apologies for the delay in replying.
>
> On Tue, Nov 3, 2020 at 3:31 AM Ryosuke Niwa  wrote:
>
>> On Fri, Oct 30, 2020 at 1:58 PM Nicolás Peña Moreno 
>> wrote:
>> >
>> > Hi, I'd like to request WebKit's position on the Element Timing API,
>> which lets web developers annotate images or text whose performance they
>> care about. They can then obtain rendering timestamps from the
>> PerformanceObserver. For cross-origin images the detailed information is
>> gated on Timing-Allow-Origin. The proposed specification is at
>> https://wicg.github.io/element-timing/ and is currently shipped in
>> Chromium. Thanks!
>>
>> Apple's WebKit team reviewed this API and we have a few
>> concerns including but not limited to:
>>
>>- The proposed API exposes timing at which a given element is
>>painted. Implemented naively, this exposes the implementation detail of
>>what kind of compositing tiles are used on a given web page. Hiding this
>>implementation detail and recording the exact theoretical paint timing 
>> will
>>be prohibitively expensive to do on all websites.
>>
>> Note that this only requires exposing the paint timestamp when the
> developer requires it explicitly.
>

I don't see how that's relevant.


> It is also implemented similarly to the 'mark paint timing' algorithm,
> which is already implemented in WebKit.
>

Mark paint timing is easier to implement because the granularity is for the
whole document, not per element basis. Because WebKit splits the viewport
into multiple tiles, and paint invalidation & painting is done per tile,
there isn't an easy way to isolate elements being painted from how tiles
are generated.

>
>>- The definition of the set of owned text nodes and how they compute
>>intersectionRect seems inadequate. It's unclear what "border box" of
>>*Text* node would mean. The spec doesn't seem to ever populate "set
>>of elements with rendered text" either.
>>
>>
> Indeed, the border box issue is tracked on
> https://github.com/WICG/element-timing/issues/33 and
> https://github.com/w3c/csswg-drafts/issues/4197. The set is populated on
> https://wicg.github.io/element-timing/#sec-element-processing.
>
>
>>
>>- The use of this API seems to incur a significant runtime as well as
>>memory cost.
>>
>> The computations and memory should be limited to the annotated elements,
> thus not impacting developers that do not use the API. I'll send a PR to
> make that better in the spec, and additional suggestions on mitigation are
> welcome.
>

That is still a major concern since painting time is one of the most costly
operations that happens during page loads still.

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

Re: [webkit-dev] Request for position on Element Timing

[Ryosuke Niwa]

On Fri, Oct 30, 2020 at 1:58 PM Nicolás Peña Moreno  wrote:
>
> Hi, I'd like to request WebKit's position on the Element Timing API,
which lets web developers annotate images or text whose performance they
care about. They can then obtain rendering timestamps from the
PerformanceObserver. For cross-origin images the detailed information is
gated on Timing-Allow-Origin. The proposed specification is at
https://wicg.github.io/element-timing/ and is currently shipped in
Chromium. Thanks!

Apple's WebKit team reviewed this API and we have a few concerns including
but not limited to:

   - The proposed API exposes timing at which a given element is painted.
   Implemented naively, this exposes the implementation detail of what kind of
   compositing tiles are used on a given web page. Hiding this
   implementation detail and recording the exact theoretical paint timing will
   be prohibitively expensive to do on all websites.
   - The definition of the set of owned text nodes and how they compute
   intersectionRect seems inadequate. It's unclear what "border box" of
   *Text* node would mean. The spec doesn't seem to ever populate "set of
   elements with rendered text" either.
   - The use of this API seems to incur a significant runtime as well as
   memory cost.

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

Re: [webkit-dev] Embedding Identifiers in Commit Messages

[Ryosuke Niwa]

On Mon, Nov 2, 2020 at 2:04 PM Jonathan Bedard  wrote:
>
> We appreciate everyone’s feedback on transitioning away from Subversion to 
> Git, I’ll be releasing an expected timeline of up-coming changes in the next 
> week before the contributors meeting.
>
> In the mean time, we’re preparing on adding identifiers to new commit 
> messages, that work is tracked in 
> https://bugs.webkit.org/show_bug.cgi?id=218407. At the moment, we’re likely 
> going to be appending the identifiers to commit messages (as the current 
> change proposes), but I wanted to provide a chance for folks to object to 
> this change before it becomes canonical.

I'm a bit confused here. It looks like the patch only affects commits
made via webkit-patch. Given there are a lot of people who don't use
webkit-patch land, I'm not certain this strategy is sound even in the
short term. Furthermore, the proposed patch seems to have a race
condition when multiple commits are made concurrency? Why don't we do
this in post commit hook instead?

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

Re: [webkit-dev] Request for a position on the Idle Detection API

[Ryosuke Niwa]

On Thu, Oct 29, 2020 at 12:54 PM Reilly Grant  wrote:
>
> On Wed, Oct 28, 2020 at 9:20 PM Ryosuke Niwa  wrote:
> >
> > On Wed, Oct 28, 2020 at 4:56 PM Reilly Grant  wrote:
> >>
> >> I would like to request an official position from the WebKit team on the 
> >> emerging Idle Detection API specification. I am aware that this API was 
> >> included in a list of APIs which you have decided not to implement due to 
> >> fingerprinting concerns. I assume that this objection was based on the 
> >> original explainer provided for this API.
> >
> > Our position has not changed. Our concerns are not limited to 
> > fingerprinting. There is an obvious privacy concern that this API lets a 
> > website observe whether a person is near the device or not. This could be 
> > used, for example, to start mining bitcoins when the user is not around or 
> > start deploying security exploits, etc...
>
> Thank you for expanding on your concerns. I agree that malicious sites
> may attempt to hide their activity from the user by waiting until they
> appear to not be paying attention. There are plenty of mechanisms
> currently available for this, for example, a site can already tell
> that it has been placed in the background and can observe that the
> user has not interacted with it in a long time, which likely means
> that the user is no longer at their computer.

If that were the case, then it seems like we don't need this API in
the first place.

> It is true that this
> capability would allow a site to be more precise about targeting a
> time when the user is not present. I think the mitigation in that
> case, especially for activity such as cryptocurrency mining, is the
> work that is being done elsewhere to define the semantics for
> throttling the work that sites are allowed to do in the background.

Throttling isn't enough to mitigate all security attacks. Some attacks
might be more of visual cue like going to full screen, etc...

> >> Since that list was posted the API has been extended to include a 
> >> permission that sites must acquire before being granted access to user 
> >> presence signals. I would like to start a conversation to understand the 
> >> fingerprinting risks you foresee from this API.
> >
> > This kind of action-at-a-distance permission prompt is problematic because 
> > it's unclear to the user why such a permission should be granted and for 
> > what purpose.
>
> It is the site's job to present a compelling case for why the user
> should grant it a permission.

That doesn't make any sense. We can't let the user make a judgement on
whether something is a good idea or not based on a text which is
supplied by malicious content.

> > Additionally, the use cases listed at 
> > https://github.com/WICG/idle-detection/blob/master/README.md are rather 
> > weak.
> >
> >> Chat application: presenting a user's status to other users and delivering 
> >> notifications to the device where the user is active.
> >
> > Why does delivering a notification to all devices considered bad? That's 
> > what happens to most notifications I receive and modern operating systems 
> > have ways to hide & dismiss old notifications anyway. It's also unclear how 
> > users are supposed to know of this use case when assessing whether to allow 
> > a permission for this API or not.
>
> Developers we have talked to (see the WICG discourse thread for
> supportive comments from Slack and Google Chat) have identified that
> receiving notifications on all their devices simultaneously is in fact
> a frequent user complaint. In the introduction section of the
> specification I explain the user scenario in more detail. Being able
> to hide or dismiss old notifications is helpful but does not address
> the core issue, which is that user's want to receive notifications on
> only the device they are currently using. The current tools for this
> are lacking because they cannot distinguish between the user leaving
> their computer and simply switching to another application.

That doesn't seem like a strong enough use case for this API. For
starters, there is no guarantee that the user won't immediately come
back to the device. Also, who is such a service supposed to know what
other device user might be using at any given point? We're definitely
not going to let a website know all the devices a given user might be
using at any given point. That's a very serious breach of the said
user's privacy. It seems to me that such a suppression / distribution
mechanism is best left for the underlying operating systems / web
browsers to handle.

I'm going to stop responding to this thread at this point because none
of the 

Re: [webkit-dev] Request for a position on the Idle Detection API

[Ryosuke Niwa]

On Wed, Oct 28, 2020 at 4:56 PM Reilly Grant  wrote:

> I would like to request an official position from the WebKit team on the
> emerging Idle Detection API  
> specification.
> I am aware that this API was included in a list of APIs
>  which you have decided not to
> implement due to fingerprinting concerns. I assume that this objection was
> based on the original explainer provided for this API.
>

Our position has not changed. Our concerns are not limited to
fingerprinting. There is an obvious privacy concern that this API lets a
website observe whether a person is near the device or not. This could be
used, for example, to start mining bitcoins when the user is not around or
start deploying security exploits, etc...


> Since that list was posted the API has been extended to include a
> permission that sites must acquire before being granted access to user
> presence signals. I would like to start a conversation to understand the
> fingerprinting risks you foresee from this API.
>

This kind of action-at-a-distance permission prompt is problematic because
it's unclear to the user why such a permission should be granted and for
what purpose.

Additionally, the use cases listed at
https://github.com/WICG/idle-detection/blob/master/README.md are rather
weak.

Chat application: presenting a user's status to other users and delivering
> notifications to the device where the user is active.


Why does delivering a notification to all devices considered bad? That's
what happens to most notifications I receive and modern operating systems
have ways to hide & dismiss old notifications anyway. It's also unclear how
users are supposed to know of this use case when assessing whether to allow
a permission for this API or not.

Showing timely notifications - e.g. deferring displaying feedback until the
> user returns to an active state.


Again, it's unclear why this is desirable. If I'm not at a computer, it's
okay for the notification to still arrive. I'd see it when I come back to
my computer.

Updating an outdated service worker when there's no unsaved state by
> triggering reloading of the tab.


This doesn't seem like something you'd need idle detection API to do. It's
sufficient to realize that you haven't recently received user inputs on
your website. I have plenty of tabs & windows that I don't touch for hours
if not days. Any websites loaded in such browsing contexts should consider
doing that kind of updates / synchronization. If the argument is that the
user may go back to such tabs / windows if they're currently present, then
this user idle detection API won't help either because the user may come
back to it at any moment.

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

Re: [webkit-dev] Request for position on import maps

[Ryosuke Niwa]

On Tue, Oct 27, 2020 at 2:23 PM Domenic Denicola  wrote:
>
> For the last couple of years myself some other Chrome folks have been working 
> on the import maps proposal. This allows controlling the behavior of 
> JavaScript import statements and import() expressions, in particular by 
> allowing the page to customize the translation of the module specifiers used 
> there into URLs. Developer reception of the feature has been very positive, 
> with continual prompting for when it'll be widely available in more browsers, 
> and a plethora of community-created tools and polyfills.
>
> Chrome is working toward shipping this in an imminent release, and we'd love 
> any thoughts or contributions from the WebKit community.

How does this feature supposed to work with CSP subresource integrity?
As far as I've read various specs and the proposal, it's not currently
possible to specify any integrity checks on modules loaded via import
this. This is a pretty serious downside because it would mean that any
remote server ever referenced by an import map becomes a security
liability for a given website. It's a lot worse compared to normal
scripts because of the action-at-a-distance of import maps. There is
no indication that a given module import could involve access to
cross-origin servers isn't obvious from where the import statement
appears.

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

Re: [webkit-dev] Request for position on Event Timing

[Ryosuke Niwa]

No. Our concerns have not been addressed.
We're continued to be concerned with the proliferation of these X timing APIs.

We would like to have a holistic study of these APIs and how they fit
together before we'll be comfortable implementing them.

- R. Niwa

On Thu, Oct 22, 2020 at 12:28 AM Rob Buis  wrote:
>
> Hi Ryosuke, Simon,
>
> Have the raised concerns been addressed now, given the document shared by 
> Nicolás and github issue discussions? I would like to avoid bitrot for my 
> prototype :)
>
> Regards,
>
> Rob.
>
> Am 30.09.20 um 00:10 schrieb Nicolás Peña Moreno:
>
> I've written a doc to explain our perspective on the use-cases of these two 
> APIs: 
> https://docs.google.com/document/d/1UrPQD0lOhHKgQy1oy1Cs0F9AsBb83q_bx8cAvu_sKrI.
>
>
> On Tue, Sep 29, 2020 at 5:16 PM Yoav Weiss  wrote:
>>
>> +Nicolás Peña
>>
>> On Sun, Aug 9, 2020 at 5:40 AM Ryosuke Niwa  wrote:
>>>
>>> On Fri, Aug 7, 2020 at 2:09 PM Rob Buis  wrote:
>>> >
>>> > I was not aware of Long Tasks API. However it seems to have a slightly
>>> > different focus (task vs. input events). Also I am mostly interested in
>>> > First Input Delay, and it was decided some time ago to not put it in
>>> > Long Tasks API (see
>>> > https://docs.google.com/document/d/1bYMLTkjcyOZR5Jt3vrulzMSoS32zOFtwyH33f6hW_C8/edit#).
>>>
>>> The concern still withstands. We don't have dozens of slightly
>>> different APIs that websites need to track for junks & delays during
>>> user interactions.
>>>
>>> It's also unclear how this first input delay works with a single page
>>> app which may have multiple transitions after a single page load from
>>> the browser engine's perspective. There had been some discussions
>>> about this in the past in Web Perf WG but I don't think we've come to
>>> any conclusion about it.
>>>
>>> In general, I'm hesitant to have any of these APIs implemented in
>>> WebKit without figuring out more coherent picture of how they'd all
>>> fit together to address underlying use cases.
>>>
>>> - R. Niwa
>>>
>>> > On 06.08.20 20:07, Simon Fraser wrote:
>>> > > Our feedback is that this API seems reasonable, but that there's 
>>> > > overlap with the "long tasks" API,
>>> > > and it's not clear if we need both.
>>> > >
>>> > > Simon
>>> > >
>>> > >> On Aug 6, 2020, at 10:43 AM, Rob Buis  wrote:
>>> > >>
>>> > >> Hi Webkit-Dev,
>>> > >>
>>> > >> I would like to get an official position from Webkit on the Event 
>>> > >> Timing Web Perf API.
>>> > >> Besides providing information about input event latency it can be used 
>>> > >> to obtain
>>> > >> First Input Timing metrics. This specification builds on the 
>>> > >> Performance Timeline
>>> > >> specification, which is implemented in Webkit. Chrome has implemented 
>>> > >> the Event
>>> > >> Timing API, see the chrome status entry below.
>>> > >>
>>> > >> - Specification: https://wicg.github.io/event-timing/
>>> > >> - Explainer: https://github.com/WICG/event-timing
>>> > >> - MDN: 
>>> > >> https://developer.mozilla.org/en-US/docs/Web/API/PerformanceEventTiming
>>> > >> - ChromeStatus: https://chromestatus.com/feature/5167290693713920
>>> > >> - Caniuse.com URL: 
>>> > >> https://caniuse.com/#feat=mdn-api_performanceeventtiming
>>> > >>
>>> > >> Regards,
>>> > >>
>>> > >> Rob.
>>> > >> ___
>>> > >> 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] Shrinking Git clone size (WebKit Transition to Git)

[Ryosuke Niwa]

Hi all,

While we're making the transition to Git, I think we should try to
shrink the Git clone size. Right now, it's 10GB and I suspect we can
cut it down quite a bit without sacrificing much. For example, we can
exclude any history for LayoutTests/platform/chromium* and
LayoutTests/platforn/*qt*. I bet that would reduce the clone size
considerably.

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

Re: [webkit-dev] Reducing / removing use of Makefile based DerivedSources.make

[Ryosuke Niwa]

On Sat, Oct 17, 2020 at 10:00 AM Sam Weinig  wrote:
>
> Hi webkit-dev,
>
> I’d like to propose, and gauge feedback on, reducing (with the goal of 
> ultimately removing) the use of Makefile based DerivedSources.make.
>
> My understanding is that currently only the Xcode based ports still use 
> DerivedSources.make, as all the CMake based ones have moved derived source 
> generation to within CMake, so that should limit the scope of who this might 
> affect.
>
>
> Why do we use Makefiles today?
>
> While I can’t recall the initial reasons for using Makefiles for derived 
> sources, over the years there have been a number of advantages to it that I 
> do know of. One clear advantage, that is no longer applicable, was code 
> sharing, as earlier in the project, at least the Apple Windows port did 
> utilize these Makefiles. Another was to work around some limitations in what 
> dependencies Xcode was able to track with build rules. It seems at least some 
> of the problems with build rules are no longer an issue, as we can now 
> specify inputs to build rules, but I don’t if other problems will still be 
> there, but for some prototyping I did, nothing yet has come up.
>
>
> What would we move to?
>
> As this only affects the Xcode based ports, we would move to distinct script 
> phases and build rules in the Xcode project.
>
>
> Why make this change? What’s the benefit?
>
> There are few reasons to consider this. One advantage is simplifying the 
> build system. Rather than two dependency systems (one for Xcode, one for the 
> Makefile) we reduce it down to one. And with additional knowledge of the 
> stages and dependencies, Xcode could potentially parallelize more phases. We 
> would would also save some time by avoiding invoking make in the first place.
>
> We also have a bit of an issue with make itself, as due to system 
> requirements, we are forever stuck with Make 3.81, which is coming up on 
> being 15 years old. More than once in the last year I have tried to 
> troubleshoot makefile issues, looking for resources on the web, only to be 
> stymied because the solutions I found required newer make.
>
>
> What are the downsides?
>
> One potential downside will be that it will be a bit harder for those without 
> Xcode to add new types of derived sources. I am not sure how much a real 
> problem in practice this will be, as editing project.pbxproj files is already 
> required for just adding new files, but I want to call it out anyway.

Do we need to dig up some kind of bespoke Xcode UI to add a new IDL
file after this? I always find all those build phase things in Xcode
to be impossible to edit.

> What are your thoughts on this? Are there additional reasons we might want to 
> stick with or move away from Makefile based derived sources?

Is there some way we can use something like Sources.txt to list the
files from which derived sources are created? DerivedSources.make is
annoying to edit but the equivalent CMake file is equally annoying to
edit. If we had a simple list of files like we do for unified sources,
it would make an average WebKit contributor's life way easier.

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

Re: [webkit-dev] WebKit Transition to Git

[Ryosuke Niwa]

On Tue, Oct 13, 2020 at 4:12 PM Konstantin Tokarev  wrote:
>
>
>
> 14.10.2020, 02:01, "Ryosuke Niwa" :
> > On Tue, Oct 13, 2020 at 3:53 PM Konstantin Tokarev  
> > wrote:
> >>  14.10.2020, 01:45, "Ryosuke Niwa" :
> >>  > On Tue, Oct 13, 2020 at 3:40 PM Konstantin Tokarev  
> >> wrote:
> >>  >> 14.10.2020, 01:30, "Ryosuke Niwa" :
> >>  >> > On Tue, Oct 13, 2020 at 2:37 PM Konstantin Tokarev 
> >>  wrote:
> >>  >> >> 13.10.2020, 22:33, "Maciej Stachowiak" :
> >>  >> >> >> On Oct 2, 2020, at 10:59 AM, Michael Catanzaro 
> >>  wrote:
> >>  >> >> >>
> >>  >> >> >> On Fri, Oct 2, 2020 at 6:36 pm, Philippe Normand 
> >>  wrote:
> >>  >> >> >>> Would you also consider preventing merge commits in order to 
> >> keep a
> >>  >> >> >>> clean mainline branch?
> >>  >> >> >>
> >>  >> >> >> Big +1 to blocking merge commits. Merge commits in a huge 
> >> project like WebKit would make commit archaeology very frustrating. (I 
> >> assume this is implied by the monotonic commit identifiers proposal, but 
> >> it doesn't exactly say that.)
> >>  >> >> >
> >>  >> >> > I’m assuming your objection is to regular merges, but how do you 
> >> feel about squash merges? Or do you think all PRs should be landed by 
> >> rebasing?
> >>  >> >>
> >>  >> >> I'm not Michael but will add my 2 dollars anyway :)
> >>  >> >>
> >>  >> >> In these two approaches commits inside PR have different meaning, 
> >> and workflow is different.
> >>  >> >>
> >>  >> >> Below I use a term "atomic change" to describe minimal code change 
> >> which is a self-contained work unit with following properties:
> >>  >> >> * It implements well-defined task which can be summarized as a 
> >> short English sentence (typical soft limit is 60 characters)
> >>  >> >> * It doesn't introduce defects (e.g. bugs, compilation breakages, 
> >> style errors, typos) which were discovered during review process
> >>  >> >> * It doesn't include any code changes unrelated to main topic. This 
> >> separation is sometimes subjective, but it's usually recommended to split 
> >> refactoring and implementation of feature based on that, bug fix and new 
> >> feature, big style change and fix or feature.
> >>  >> >>
> >>  >> >> AFAIU our current review process has similar requirements to 
> >> patches submitted to Bugzilla, though sometimes patches include unrelated 
> >> changes. This can be justified by weakness of webkit-patch/Bugzilla 
> >> tooling which has no support for patch series, and by fact that SVN 
> >> doesn't support keeping local patch series at all.
> >>  >> >>
> >>  >> >> 1. Workflow 1 - "Squash merge" policy
> >>  >> >>
> >>  >> >> * Whole PR is considered to be a single atomic change of WebKit 
> >> source tree. If work is supposed to be landed as a series of changes which 
> >> depend on each other (e.g. refactoring and feature based on it, or 
> >> individual separate features touching same parts of code), each change 
> >> needs a separate PR, and, as a consequence, only one of them can be 
> >> efficiently reviewed at the moment of time
> >>  >> >> * Commits in PR represent review iterations or intermediate 
> >> implementation progress
> >>  >> >> * Reviewers' comments are addressed by pushing new commits without 
> >> rewriting history, which works around GitHub's lack of "commit revisions". 
> >> Also this workflow has lower entry barrier for people who haven't mastered 
> >> git yet, as it requires only "git commit" and "git push" without rebases.
> >>  >> >>
> >>  >> >> 2. Workflow 2 - "Rebase" ("cherry-pick")) or "Merge" policy
> >>  >> >>
> >>  >> >> * PR is considered to be a series of atomic changes. If work 
> >> consists of several atomic changes, each commit represent an atomic change
> >>  >> >> * Review iterations are done by fixing commits in place and 
> >

Re: [webkit-dev] WebKit Transition to Git

[Ryosuke Niwa]

On Tue, Oct 13, 2020 at 3:53 PM Konstantin Tokarev  wrote:
>
>
> 14.10.2020, 01:45, "Ryosuke Niwa" :
> > On Tue, Oct 13, 2020 at 3:40 PM Konstantin Tokarev  
> > wrote:
> >>  14.10.2020, 01:30, "Ryosuke Niwa" :
> >>  > On Tue, Oct 13, 2020 at 2:37 PM Konstantin Tokarev  
> >> wrote:
> >>  >> 13.10.2020, 22:33, "Maciej Stachowiak" :
> >>  >> >> On Oct 2, 2020, at 10:59 AM, Michael Catanzaro 
> >>  wrote:
> >>  >> >>
> >>  >> >> On Fri, Oct 2, 2020 at 6:36 pm, Philippe Normand  
> >> wrote:
> >>  >> >>> Would you also consider preventing merge commits in order to keep a
> >>  >> >>> clean mainline branch?
> >>  >> >>
> >>  >> >> Big +1 to blocking merge commits. Merge commits in a huge project 
> >> like WebKit would make commit archaeology very frustrating. (I assume this 
> >> is implied by the monotonic commit identifiers proposal, but it doesn't 
> >> exactly say that.)
> >>  >> >
> >>  >> > I’m assuming your objection is to regular merges, but how do you 
> >> feel about squash merges? Or do you think all PRs should be landed by 
> >> rebasing?
> >>  >>
> >>  >> I'm not Michael but will add my 2 dollars anyway :)
> >>  >>
> >>  >> In these two approaches commits inside PR have different meaning, and 
> >> workflow is different.
> >>  >>
> >>  >> Below I use a term "atomic change" to describe minimal code change 
> >> which is a self-contained work unit with following properties:
> >>  >> * It implements well-defined task which can be summarized as a short 
> >> English sentence (typical soft limit is 60 characters)
> >>  >> * It doesn't introduce defects (e.g. bugs, compilation breakages, 
> >> style errors, typos) which were discovered during review process
> >>  >> * It doesn't include any code changes unrelated to main topic. This 
> >> separation is sometimes subjective, but it's usually recommended to split 
> >> refactoring and implementation of feature based on that, bug fix and new 
> >> feature, big style change and fix or feature.
> >>  >>
> >>  >> AFAIU our current review process has similar requirements to patches 
> >> submitted to Bugzilla, though sometimes patches include unrelated changes. 
> >> This can be justified by weakness of webkit-patch/Bugzilla tooling which 
> >> has no support for patch series, and by fact that SVN doesn't support 
> >> keeping local patch series at all.
> >>  >>
> >>  >> 1. Workflow 1 - "Squash merge" policy
> >>  >>
> >>  >> * Whole PR is considered to be a single atomic change of WebKit source 
> >> tree. If work is supposed to be landed as a series of changes which depend 
> >> on each other (e.g. refactoring and feature based on it, or individual 
> >> separate features touching same parts of code), each change needs a 
> >> separate PR, and, as a consequence, only one of them can be efficiently 
> >> reviewed at the moment of time
> >>  >> * Commits in PR represent review iterations or intermediate 
> >> implementation progress
> >>  >> * Reviewers' comments are addressed by pushing new commits without 
> >> rewriting history, which works around GitHub's lack of "commit revisions". 
> >> Also this workflow has lower entry barrier for people who haven't mastered 
> >> git yet, as it requires only "git commit" and "git push" without rebases.
> >>  >>
> >>  >> 2. Workflow 2 - "Rebase" ("cherry-pick")) or "Merge" policy
> >>  >>
> >>  >> * PR is considered to be a series of atomic changes. If work consists 
> >> of several atomic changes, each commit represent an atomic change
> >>  >> * Review iterations are done by fixing commits in place and 
> >> reuploading entire series using force push (of course if review discovers 
> >> that substantial part of work is missing it can be added as a new atomic 
> >> commit to the series)
> >>  >> * It's possible to review each commit in the series separately
> >>  >> * Workflow requires developers to have more discipline and experience 
> >> with using git rebase for history rewriting. Entry barrier can be lowered 
> &g

Re: [webkit-dev] WebKit Transition to Git

[Ryosuke Niwa]

On Tue, Oct 13, 2020 at 3:19 PM Michael Catanzaro  wrote:
>
> Detailed descriptions are very important. I don't think function-level
> changelogs are; documenting changes in individual functions is
> generally busywork to say what you can plainly see by just looking at
> the diff.

They certainly can be quite informative. e.g.
https://trac.webkit.org/changeset/268365/webkit/trunk/Source/WebCore/ChangeLog

It's true that you can certainly split each logical step into its own
commit but that seems like more of a busy work to me. I mostly use a
Subversion checkout to do my work, and even if I'm using a Git clone,
I normally wouldn't commit anything until the whole patch is written.
e.g. I wrote the entirety of https://trac.webkit.org/r268239 and
posted in one chunk other than a few WIP patches I had posted. Having
to go back & split that into multiple commits would be a total waste
of time.

> Regarding line-by-line commit review... well, it would be nice to have,
> of course.  But I don't think it's as important as you suggest.
> Problems with commit messages are usually general problems with the
> entire commit message rather than problems with a specific line of the
> commit message.

I disagree. I often have specific commentary on specific lines of
change logs like missing function-level comments or typos, or need
some elaboration on specific details.

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

Re: [webkit-dev] WebKit Transition to Git

[Ryosuke Niwa]

On Tue, Oct 13, 2020 at 3:40 PM Konstantin Tokarev  wrote:
>
> 14.10.2020, 01:30, "Ryosuke Niwa" :
> > On Tue, Oct 13, 2020 at 2:37 PM Konstantin Tokarev  
> > wrote:
> >>  13.10.2020, 22:33, "Maciej Stachowiak" :
> >>  >> On Oct 2, 2020, at 10:59 AM, Michael Catanzaro  
> >> wrote:
> >>  >>
> >>  >> On Fri, Oct 2, 2020 at 6:36 pm, Philippe Normand  
> >> wrote:
> >>  >>> Would you also consider preventing merge commits in order to keep a
> >>  >>> clean mainline branch?
> >>  >>
> >>  >> Big +1 to blocking merge commits. Merge commits in a huge project like 
> >> WebKit would make commit archaeology very frustrating. (I assume this is 
> >> implied by the monotonic commit identifiers proposal, but it doesn't 
> >> exactly say that.)
> >>  >
> >>  > I’m assuming your objection is to regular merges, but how do you feel 
> >> about squash merges? Or do you think all PRs should be landed by rebasing?
> >>
> >>  I'm not Michael but will add my 2 dollars anyway :)
> >>
> >>  In these two approaches commits inside PR have different meaning, and 
> >> workflow is different.
> >>
> >>  Below I use a term "atomic change" to describe minimal code change which 
> >> is a self-contained work unit with following properties:
> >>  * It implements well-defined task which can be summarized as a short 
> >> English sentence (typical soft limit is 60 characters)
> >>  * It doesn't introduce defects (e.g. bugs, compilation breakages, style 
> >> errors, typos) which were discovered during review process
> >>  * It doesn't include any code changes unrelated to main topic. This 
> >> separation is sometimes subjective, but it's usually recommended to split 
> >> refactoring and implementation of feature based on that, bug fix and new 
> >> feature, big style change and fix or feature.
> >>
> >>  AFAIU our current review process has similar requirements to patches 
> >> submitted to Bugzilla, though sometimes patches include unrelated changes. 
> >> This can be justified by weakness of webkit-patch/Bugzilla tooling which 
> >> has no support for patch series, and by fact that SVN doesn't support 
> >> keeping local patch series at all.
> >>
> >>  1. Workflow 1 - "Squash merge" policy
> >>
> >>  * Whole PR is considered to be a single atomic change of WebKit source 
> >> tree. If work is supposed to be landed as a series of changes which depend 
> >> on each other (e.g. refactoring and feature based on it, or individual 
> >> separate features touching same parts of code), each change needs a 
> >> separate PR, and, as a consequence, only one of them can be efficiently 
> >> reviewed at the moment of time
> >>  * Commits in PR represent review iterations or intermediate 
> >> implementation progress
> >>  * Reviewers' comments are addressed by pushing new commits without 
> >> rewriting history, which works around GitHub's lack of "commit revisions". 
> >> Also this workflow has lower entry barrier for people who haven't mastered 
> >> git yet, as it requires only "git commit" and "git push" without rebases.
> >>
> >>  2. Workflow 2 - "Rebase" ("cherry-pick")) or "Merge" policy
> >>
> >>  * PR is considered to be a series of atomic changes. If work consists of 
> >> several atomic changes, each commit represent an atomic change
> >>  * Review iterations are done by fixing commits in place and reuploading 
> >> entire series using force push (of course if review discovers that 
> >> substantial part of work is missing it can be added as a new atomic commit 
> >> to the series)
> >>  * It's possible to review each commit in the series separately
> >>  * Workflow requires developers to have more discipline and experience 
> >> with using git rebase for history rewriting. Entry barrier can be lowered 
> >> by providing step by step instructions like e.g. [1].
> >
> > I really dislike this workflow due to its inherent complexity. Having
> > to use Git is enough of a burden already. I don't want to deal with an
> > extra layer of complexity to deal with.
>
> There is simplified version of workflow 2 when you have only one commit in 
> PR. In this case you can easily edit this single commit with gic commit 
> --amend or GUI tools to address review comments. At the same time those who 
> are more comfortable with git can use longer patch series.

Except that reviewers would still have to review each commit
separately, and the time comes to revert someone's patch, we still
need to remember how to revert a sequence of commits that belong to a
single PR.

I don't feel comfortable accepting this level of new complexity into
our contribution process in addition to being forced to use Git &
GitHub.

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

Re: [webkit-dev] WebKit Transition to Git

[Ryosuke Niwa]

On Tue, Oct 13, 2020 at 3:19 PM Michael Catanzaro  wrote:
>
> I suppose what I'm describing is Konstantin's Workflow 2, which is
> overwhelmingly popular.
>
> On Tue, Oct 13, 2020 at 2:19 pm, Ryosuke Niwa  wrote:
> > Not squashing only helps if each commit can stand on its own. At that
> > point, I'd suggest such a sequence of commits be made into multiple
> > PRs instead of multiple commits in a single PR, each of which requires
> > a separate code review.
>
> Commits are certainly expected to stand on their own (not introduce
> defects). If they can't, then they should be combined into another
> commit! Hence, we should not approve MRs if the MR contains commits
> that fail to meet our usual standards. Such commits should just fail
> code review. (But if you aren't willing to review MRs commit-by-commit,
> then indeed you'll never notice when such problems exist.)

Right, so this will be a problem unless we're gonna review each commit
separately. But if we're doing that, we might as well as create a
separate PR for each commit.

If we have 5-10 separate commits each of which make substantial
changes, we don't all of them to be landed / merged all at once into
the main branch even if they're logically related. Without
intermediate test runs & perf test results, we wouldn't be able to
pin-point what caused it, in which case, we'd be likely forced to
revert all of them anyway.

Furthermore, reverting a part of a sequence of commits coming from a
single PR would be very confusing just reverting just one of many
patches posted on Bugzilla would be confusing.

For all these reasons, I don't see much benefit in allowing multiple
commits from being merged from PR.

> If I have to open a separate MR for each change, though, I'm going to
> wind up combining multiple lightly-related changes into one big commit,
> because a new MR for every tiny cleanup I want to make requires effort.
> Such commits may be common in WebKit, but they would fail code review
> in most other open source projects. E.g. in
> https://trac.webkit.org/changeset/268394/webkit I snuck in a drive-by
> one-line fix that's unrelated to the rest of the commit. I would rarely
> dare to do that outside WebKit, knowing it would probably fail review,
> but we do it commonly in WebKit because we discourage multiple patches
> per bug and don't want to create new bugs for every small cleanup.

That seems totally okay. I land one line fix like that all the time though.

> > This to me is a show stopper. When I'm trying to bisect an issue,
> > etc..., the biggest obstacle I face is any intermediate revisions
> > where builds are broken or otherwise non-functional. I don't think we
> > should let anyone merge a commit into the main branch unless the
> > commit meets the same standards as a regular Bugzilla patch we land
> > today.
>
> I agree. But I would say that a MR with such history should fail
> review, and be rewritten to not suffer from these problems.

The problem is that you're presuming that all WebKit reviewers would
be able to and would be willing to do that review. I certainly do not
feel comfortable having to do that given how troublesome it has been
in WPT land.

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

Re: [webkit-dev] WebKit Transition to Git

[Ryosuke Niwa]

On Tue, Oct 13, 2020 at 2:37 PM Konstantin Tokarev  wrote:
>
>
> 13.10.2020, 22:33, "Maciej Stachowiak" :
> >>  On Oct 2, 2020, at 10:59 AM, Michael Catanzaro  
> >> wrote:
> >>
> >>  On Fri, Oct 2, 2020 at 6:36 pm, Philippe Normand  wrote:
> >>>  Would you also consider preventing merge commits in order to keep a
> >>>  clean mainline branch?
> >>
> >>  Big +1 to blocking merge commits. Merge commits in a huge project like 
> >> WebKit would make commit archaeology very frustrating. (I assume this is 
> >> implied by the monotonic commit identifiers proposal, but it doesn't 
> >> exactly say that.)
> >
> > I’m assuming your objection is to regular merges, but how do you feel about 
> > squash merges? Or do you think all PRs should be landed by rebasing?
>
> I'm not Michael but will add my 2 dollars anyway :)
>
> In these two approaches commits inside PR have different meaning, and 
> workflow is different.
>
> Below I use a term "atomic change" to describe minimal code change which is a 
> self-contained work unit with following properties:
> * It implements well-defined task which can be summarized as a short English 
> sentence (typical soft limit is 60 characters)
> * It doesn't introduce defects (e.g. bugs, compilation breakages, style 
> errors, typos) which were discovered during review process
> * It doesn't include any code changes unrelated to main topic. This 
> separation is sometimes subjective, but it's usually recommended to split 
> refactoring and implementation of feature based on that, bug fix and new 
> feature, big style change and fix or feature.
>
> AFAIU our current review process has similar requirements to patches 
> submitted to Bugzilla, though sometimes patches include unrelated changes. 
> This can be justified by weakness of webkit-patch/Bugzilla tooling which has 
> no support for patch series, and by fact that SVN doesn't support keeping 
> local patch series at all.
>
> 1. Workflow 1 - "Squash merge" policy
>
> * Whole PR is considered to be a single atomic change of WebKit source tree. 
> If work is supposed to be landed as a series of changes which depend on each 
> other (e.g. refactoring and feature based on it, or individual separate 
> features touching same parts of code), each change needs a separate PR, and, 
> as a consequence, only one of them can be efficiently reviewed at the moment 
> of time
> * Commits in PR represent review iterations or intermediate implementation 
> progress
> * Reviewers' comments are addressed by pushing new commits without rewriting 
> history, which works around GitHub's lack of "commit revisions". Also this 
> workflow has lower entry barrier for people who haven't mastered git yet, as 
> it requires only "git commit" and "git push" without rebases.
>
> 2. Workflow 2 - "Rebase" ("cherry-pick")) or "Merge" policy
>
> * PR is considered to be a series of atomic changes. If work consists of 
> several atomic changes, each commit represent an atomic change
> * Review iterations are done by fixing commits in place and reuploading 
> entire series using force push (of course if review discovers that 
> substantial part of work is missing it can be added as a new atomic commit to 
> the series)
> * It's possible to review each commit in the series separately
> * Workflow requires developers to have more discipline and experience with 
> using git rebase for history rewriting. Entry barrier can be lowered by 
> providing step by step instructions like e.g. [1].

I really dislike this workflow due to its inherent complexity. Having
to use Git is enough of a burden already. I don't want to deal with an
extra layer of complexity to deal with.

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

Re: [webkit-dev] WebKit Transition to Git

[Ryosuke Niwa]

On Tue, Oct 13, 2020 at 1:57 PM Michael Catanzaro  wrote:
>
> On Tue, Oct 13, 2020 at 12:32 pm, Maciej Stachowiak 
> wrote:
> > I’m assuming your objection is to regular merges, but how do you
> > feel about squash merges? Or do you think all PRs should be landed by
> > rebasing?
>
> If we want a linear history (we do), all PRs ultimately have to land as
> fast-forward merges, one way or the other. I think rebasing is the
> simplest way to accomplish that, but squashes work too if all your
> changes are sufficiently self-contained to land as one commit.
>
> I suggest leaving this up to the discretion of the developer and
> reviewer rather than mandating one way or the other, because there are
> advantages and disadvantages to both approaches. If your local commit
> history is a mess, sometimes squashing it all into one commit is an
> easy way to make it acceptable for upstream. If you hate rewriting
> history to make it look good, and your MR isn't too huge, a squash
> might be appropriate. But more often than not, I'd say that would
> result in a worse commit history.

FWIW, I think we should always squash all commits and have a single
change log entry / commit message for all changes.

In the case we want to rebase and keep multiple commits, then we
should have a change log / descriptive commit message that follows the
current change log format for each commit, rather than a short
description of each change.

> > My own preference would be to require squash merge, because it keeps
> > the history simple for the main branch, but does not risk putting
> > intermediate revisions which may work or even build on the main
> > branch.
>
> The disadvantage is that if you squash before merging, you encourage
> fewer, bigger commits that might be harder to read and potentially much
> less fun to discover at the end of a bisect.

Not squashing only helps if each commit can stand on its own. At that
point, I'd suggest such a sequence of commits be made into multiple
PRs instead of multiple commits in a single PR, each of which requires
a separate code review.

> On the other hand, if you don't squash, it's indeed possible that your
> commit history might be messy, e.g. intermediate commits might break
> tests fixed by subsequent commits, or intermediate commits might not
> build at all.

This to me is a show stopper. When I'm trying to bisect an issue,
etc..., the biggest obstacle I face is any intermediate revisions
where builds are broken or otherwise non-functional. I don't think we
should let anyone merge a commit into the main branch unless the
commit meets the same standards as a regular Bugzilla patch we land
today.

> Sometimes I see the
> opposite, where too many changes are bundled into one big commit. (This
> is more common in WebKit, since we normally try to have one patch per
> bug, and splitting changes into multiple bugs is inconvenient.) But
> usually the developer submitting a MR has found a good balance between
> the two extremes. Ultimately, what's most important is landing a clean
> commit history with reviewable commits. Again, the scope of commits is
> subject to review just like code changes are.

I often review test changes in WPT or spec changes in WHATWG / W3C,
and I often find a PR with multiple commits to be annoying to review.
I'd rather have each PR have a single large commit to review. In
practice, I'd never review commit by commit because I need to see the
whole picture to review what's happening.

> Instead, I suggest
> developers should aggressively use 'git add -p' and 'git rebase -i' to
> selectively commit, rewrite, and reorder history to look good before
> opening the MR. This isn't optional for most open source projects: if
> you propose an MR with ugly commit history, it won't be merged until
> fixed. For a typical MR of moderate complexity, I'll use 'git rebase
> -i' at least a couple times before the history is clean enough for a
> MR, and to make fixup commits disappear into the most-appropriate
> commit in the sequence.

I'm afraid that this will still result in random intermediary commits
being merged into the main branch because there is no mechanical
enforcement.

> Regarding commit messages, I don't understand why there's any
> expectation that they need to be checked into files in order to be
> detailed and complete. If a commit message doesn't adequately describe
> what it's fixing, then the reviewer should open a review thread to
> complain and request more detailed commit messages.

The problem is that I've seen multiple projects which moved to Git and
did this, and their commit log message's quality materially degraded.
If GitHub and other tools allowed inline comments being added for each
line of the commit message, I have less of concern. Github doesn't.

> If a change is very
> straightforward and obvious, sometimes a quick sentence might suffice,
> but usually a good commit message should be at least a paragraph or
> two, and often more. Function-level 

Re: [webkit-dev] WebKit Transition to Git

[Ryosuke Niwa]

On Mon, Oct 5, 2020 at 5:13 PM Konstantin Tokarev  wrote:
>
>
> 05.10.2020, 23:41, "Yusuke Suzuki" :
> > I think security component is special in terms of how to handle it already 
> > (e.g. not posting a test with the patch etc.)
> > To me, handling non-security issues in GitHub and security issues in 
> > Bugzilla is OK.
> > By nature, security issues are not open. Since one of our motivation of 
> > moving to GitHub is openness for feedback collection, security issue in 
> > Bugzilla does not matter for this motivation.
> > Ideally, handling both in GitHub is better. But to me, rather than 
> > continuing using Bugzilla, using GitHub for non security issues sounds 
> > improvement.
>
> To me it sounds as a huge step backwards. Asides from situation with security 
> issues, it has other significant drawbacks in domain of issue triaging and 
> management:
>
> 1. Sub-par support for linking issues to each other
> 
>
> Traditional bug tracking systems (like Bugzilla or JIRA) have support of 
> "related" or "linked" issues. Most important relations are
>
> * A depends on B (B blocks A) - blockers and umbrella issues
> * B is duplicate of A
> * A and B are related in other unspecified way
>
> All GitHub can offer here now is mentions (and, to some extent, milestones 
> for case of "umbrella issues" [1]). Mention is created every time someone 
> uses "#" (e.g. "#123") in the text of issue or in the comment, where 
> number is a sequential number of issue or pull request [2]. When comment is 
> published in issue A which mentions issue B, there is a pseudo-comment added 
> to B, and subscribers of B receive email notification.
>
> At first glance this naive approach seems to work, but
>
> * There is no easily visible list of relations: if you are not closely 
> following all activity on A, to find all issues related to it you have to 
> browse through all its (pseudo-)comments, which in some cases might be long.
> * There is no *stateful* list of relations: if A was believed to have common 
> source with B, but then it was discovered they are not related, you cannot 
> delete relationship between A and B because there is no relationship, just a 
> set of comments.
> * "#" is not a safe reference format. Sometimes users' comments may 
> have other data in "#" format with a different meaning than 
> references to GitHub issues. For example, may the force be with you if 
> someone pastes gdb or lldb backtrace into comment without escaping it into 
> markdown raw text block (```). Also, GitHub parses mentions in git commit 
> messages, so care must be taken to avoid any occurrences of "#" with 
> a meaning different from reference to issue number.

Yeah, this is a pretty significant functional regression to me. I use
bug dependencies all the time (e.g.
https://bugs.webkit.org/showdependencytree.cgi?id=148695_resolved=1)
and not having this capability will significantly hinder my ability to
track & triage some bugs.

> 3. Sub-par attachments
> --
>
> Traditional bug trackers allow attaching files to issue. GitHub goes further 
> and allows to attach files to every comment. Enjoy the progress -  now you 
> can look for attached test cases and proposed patches through all comment 
> feed, instead of having them in one place at the top.
>
> Also, on test cases. You probably like this feature of Bugzilla when you can 
> attach self-contained HTML file to the issue and then simply open it by URL 
> in any browser including your build of WebKit to try it out. Forget this - 
> GitHub simply forbids HTML or JS attachments (without wrapping them in 
> archive):
>
> "We don’t support that file type. with a GIF, JPEG, JPG, PNG, DOCX, GZ, 
> LOG, PDF, PPTX, TXT, XLSX or ZIP."
>
> And yes, take care not to use tar.xz or tar.bz2 or any other unapproved 
> archive type.
>
> But you can attach funny cat picture to your comment and it will be displayed 
> inline :)

This is another massive functional regression. I open test cases on
Bugzilla without downloading all the time, not to mention that it's a
great way to test iOS devices as well. Not being able to do that would
significantly reduce my productivity.

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

Re: [webkit-dev] WebKit Transition to Git

[Ryosuke Niwa]

On Sun, Oct 4, 2020 at 2:41 PM Konstantin Tokarev  wrote:
>
> 02.10.2020, 19:46, "Jonathan Bedard" :
> > Monotonic Commit Identifiers
> > Of great interest to Apple’s engineers has been retaining some kind of 
> > ordered tag we can use to refer to commits to make defending CI and 
> > bisection easier. We’ve developed a scheme for this that assigns commits an 
> > ordered identifier per-branch, outlined in 
> > https://trac.webkit.org/wiki/commit-identifiers, designed to be used 
> > alongside git hashes. These identifiers can be used in our current 
> > Subversion repository, and we would like to start using them before the 
> > project has transitions to git.
>
> AFAIU, this is very close to what `git describe` does: you give it git hash, 
> it gives you new identifier consisting of 3 parts:
>
> --

We don't want to be using a number from the closest tag. We need a
contiguous monotonically increasing number on the main branch.

> Note that resulting identifier can be used in all git operations which 
> require git reference (like `git log` or `git show`).
> So, if you push git tags to main repository (maybe lightweight tags, if you 
> are planning to have lots of them), there is no need to invent any other 
> identifiers.
>
> As for bisection, git bisect works just fine when given two commit hashes.

"git bisect" is wholly inadequate for our purposes. Not only can
building between each step can take ages (15-40min depending on a
machine) even for correctness bisections, some of our performance
tests will require many hours to measure statistically significant
results. On top of that, many of our bisections can take weeks if not
months to conduct and could span hundreds of commits in between. In
some cases, we have to compare time series charts of performance data
across multiple operating systems and device classes. Furthermore, we
have to communicate all this information between different teams
working on the bisection, someone who wrote the original patch, QA
people testing various builds, etc... It's extremely important that we
have a **human readable number** which is easy to memorize and intuit
during these analyses and communications.

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

Re: [webkit-dev] WebKit Transition to Git

[Ryosuke Niwa]

On Sat, Oct 3, 2020 at 2:25 AM Adrien Destugues
 wrote:
>
> On Fri, Oct 02, 2020 at 07:05:21PM -0500, Michael Catanzaro wrote:
> > > I realize that Gerrit might not integrate at all with hosting the repo
> > > on Github, but has any thought been given to this aspect of the
> > > transition?
> >
> > That sounds like it would be a significant barrier to contribution, and
> > frankly defeat the point of switching. If we have serious concerns with
> > GitHub's code review functionality (which, again, I'm not familiar with),
> > then we should just use GitLab and have everything in one place. (GitLab
> > accepts GitHub logins via OAuth, both on gitlab.com and self-hosted
> > instances, so the barrier to contributing remains low either way.)
>
> Gerrit accepts GitHub and other OAuth providers as well, so that's not a
> problem. We have been using this for Haiku code reviews for a few years
> now, and interestingly we got some complaints from people who don't want
> to have a Github account (for various reasons) and won't use our code
> review tool because of that.
>
> I think the integration referred to was rather in terms of having
> reviews synchronized between Gerrit and Github pull requests, which is
> also possible, but I think if the point is to use Github, it doesn't
> work this way: if your workflow is too different from the standard way
> to use Github, people will still be confused by it and it will still be
> a barrier to contribution.

But using Gerrit would make that situation any better either.

> I think having to create an account on a website isn't the main thing
> preventing people to contribute anyway? It's more about having to use
> project-specific tools to prepare the patch for submission (in the case
> of WebKit, having to write the commit message in the Changelog file, for
> example).

It's about all those things. We've definitely heard of people
complaining or refusing to create a Bugzilla account to report bugs.
I've gotta say I'm very much concerned about getting rid of change
logs when we move to Git. We put a lot of useful information about
what was causing the bug, how we fixed it, and why we fixed the way we
did in a change log. I've seen a few projects which transitioned to
Git and somehow lost the rigor in maintaining an equally high quality
commit message, partly because most code review tools don't let you
add inline comments to commit messages.

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