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

[Tim Horton via webkit-dev]

+1! The bugzilla-style “unofficial r=me” comment was much clearer for exactly 
these reasons.

> On Nov 28, 2023, at 10:27 AM, Chris Dumez via webkit-dev 
>  wrote:
> 
> Hi,
> 
> Back in the Bugzilla days, only reviewers were allowed to r+ or r- patches. 
> Non-reviewers were - of course - encouraged to do informal reviews but they 
> would do so by leaving comments. They would never r+ / r-.
> 
> Since we’ve moved to Github, it seems we have become a lot more lax about 
> this and I have seen non-reviewers approve and reject PRs, not just leaving 
> comments.
> My understanding is that there is no way to prevent this with Github but 
> could we at least make it a policy that non-reviewers should not approve / 
> reject PRs and only leave comments instead?
> 
> The reason I would like us to make enforce this rule is that I find it 
> confusing. We have a lot of new comers in the project and I do not always 
> know if a person is a reviewer or not yet. I imagine it may be even more 
> confusing for non-Apple people.
> 
> I have in some cases not reviewed patches because I had seen the "green 
> check” and thought the PR already had been approved.
> I have also seen cases of PRs rejected, asking the author to do more work, 
> that I didn’t feel was necessary.
> There is no easy way from the GitHub UI to tell if the person who 
> approved/rejected your PR is actually a reviewer, as far as I know.
> 
> What do you think?
> 
> Thanks,
> Chris Dumez.
> ___
> webkit-dev mailing list
> webkit-dev@lists.webkit.org
> https://lists.webkit.org/mailman/listinfo/webkit-dev

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

Re: [webkit-dev] Smart Pointers Usage Guidelines

[Tim Horton via webkit-dev]

> On Aug 21, 2023, at 4:39 PM, Ryosuke Niwa  wrote:
> 
> 
>> 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?

How can we tell!

> But then a simpler rule to deploy will be that every function must return 
> CheckedRef/CheckedPtr/Ref/RefPtr.

+1 to that rule instead of the one in Wenson’s patch.

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

Would rather Darin’s plan. I don’t want to have to think about CheckedPtr every 
5 seconds.

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

[Tim Horton 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().

That, for example, seems wildly preferable.

> — Darin

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

Re: [webkit-dev] Smart Pointers Usage Guidelines

[Tim Horton via webkit-dev]

> On Aug 21, 2023, at 4:25 PM, Ryosuke Niwa via webkit-dev 
>  wrote:
> 
> 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

(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?

> 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

___
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?

[Tim Horton via webkit-dev]

> On Feb 1, 2022, at 12:30 PM, Elliott Williams via webkit-dev 
>  wrote:
> 
> Hi webkit-dev,
> 
> 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.
> 
> 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.

Question: If they're flattened in the SDK, and not flattened in the source 
tree, which include path do we use when including a WTF header in e.g. WebCore?

Right now you say `#include `. In your world, would you 
say `#include `?

If no, how does that work in the case where you only have WTF from the SDK?

If yes, how does that work for the other build systems that are building with 
the full source tree? (maybe we have all directories in WTF listed as header 
search paths, or something? or maybe they look in `usr/lib/include/wtf` in the 
build directory?)

> 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

[webkit-dev] Aditya Keerthi is now a WebKit reviewer!

[Tim Horton via webkit-dev]

Hello!

I am delighted to announce that Aditya Keerthi is now a WebKit reviewer.

Now it's time for all of you to send him tricky patches and see how he does :)

Congratulations, Aditya!

--Tim
___
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

[Tim Horton via webkit-dev]

> On Apr 29, 2021, at 9:02 PM, Darin Adler  wrote:
> 
>> On Apr 29, 2021, at 9:01 PM, Tim Horton  wrote:
>> 
>> This is a huge problem for people on the Apple platforms using the Xcode 
>> projects. Nearly every time someone adds a file they have to add random 
>> headers to unrelated code.
> 
> OK, sorry, I must be out of touch with the rest of you about how difficult 
> this is. I’ve done that many times, and it was always easy for me.

I'm not saying it's hard (depending on your familiarity with the code that 
breaks, or if it's one of those 'using namespace' situations, or something even 
more subtle), but it is definitely highly annoying, and judging by the number 
of times I (and I'm sure others too) have had to explain what's up to other 
people it seems like "huge" is a only-maybe-slightly-overstated estimate :) 
Maybe others disagree!

> — Darin

___
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

[Tim Horton via webkit-dev]

> On Apr 29, 2021, at 7:15 PM, Darin Adler via webkit-dev 
>  wrote:
> 
>> On Apr 29, 2021, at 12:04 PM, Ross Kirsling via webkit-dev 
>>  wrote:
>> 
>> Yeah, I think it's important to clarify that nobody is "using 
>> non-Unified-Source building for their development", at least to my 
>> knowledge. Being broken by the shifting sands of unified sources is an 
>> everybody problem (or at the very least an "everybody that builds via CMake 
>> problem", which is ultimately an everybody problem).
> 
> How is this a bigger problem in CMake than for people on the Apple platforms 
> who are using Xcode projects? Can we create greater parity between the two?

I don't follow. This is a huge problem for people on the Apple platforms using 
the Xcode projects. Nearly every time someone adds a file they have to add 
random headers to unrelated code. 

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