Bug 234747

Summary: Updating the file name of attachment-backed images should automatically set the `alt` attribute
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: HTML EditingAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: akeerthi, cdumez, changseok, darin, esprehn+autocc, ews-watchlist, gyuyoung.kim, hi, megan_gardner, mifenton, thorton, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
For EWS
none
Return pointer instead of RefPtr
none
Use auto in a few more places none

Description Wenson Hsieh 2021-12-29 15:01:51 PST
rdar://85899879
Comment 1 Wenson Hsieh 2021-12-29 15:23:42 PST
Created attachment 448082 [details]
For EWS
Comment 2 Darin Adler 2021-12-30 17:45:37 PST
Comment on attachment 448082 [details]
For EWS

View in context: https://bugs.webkit.org/attachment.cgi?id=448082&action=review

> Source/WebCore/html/HTMLAttachmentElement.h:67
> +    RefPtr<HTMLImageElement> enclosingImageElement() const;

Someone has to write down our rules about RefPtr vs. raw pointer in new code. It seems clear this should be RefPtr in case we call any function using it as a this pointer, but not sure where we wrote that down.
Comment 3 Wenson Hsieh 2021-12-31 11:40:06 PST
Comment on attachment 448082 [details]
For EWS

View in context: https://bugs.webkit.org/attachment.cgi?id=448082&action=review

Thanks for the review!

>> Source/WebCore/html/HTMLAttachmentElement.h:67
>> +    RefPtr<HTMLImageElement> enclosingImageElement() const;
> 
> Someone has to write down our rules about RefPtr vs. raw pointer in new code. It seems clear this should be RefPtr in case we call any function using it as a this pointer, but not sure where we wrote that down.

+1 to this! I agree it would be great to have explicit guidelines around when to use RefPtr/Ref in the return type, vs. using RefPtr/Ref at the call site.

My understanding was that we should generally return RefPtr/Ref instead of raw pointers/references and use `auto` in call sites, unless there's a good reason to avoid the ref-churn (e.g. performance). That said, I do still tend to pass back raw pointers or references sometimes for methods that are just simple getters around a ref-counted object :P.
Comment 4 EWS 2021-12-31 11:47:53 PST
Committed r287494 (245629@main): <https://commits.webkit.org/245629@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448082 [details].
Comment 5 Darin Adler 2021-12-31 21:22:47 PST
Comment on attachment 448082 [details]
For EWS

View in context: https://bugs.webkit.org/attachment.cgi?id=448082&action=review

>>> Source/WebCore/html/HTMLAttachmentElement.h:67
>>> +    RefPtr<HTMLImageElement> enclosingImageElement() const;
>> 
>> Someone has to write down our rules about RefPtr vs. raw pointer in new code. It seems clear this should be RefPtr in case we call any function using it as a this pointer, but not sure where we wrote that down.
> 
> +1 to this! I agree it would be great to have explicit guidelines around when to use RefPtr/Ref in the return type, vs. using RefPtr/Ref at the call site.
> 
> My understanding was that we should generally return RefPtr/Ref instead of raw pointers/references and use `auto` in call sites, unless there's a good reason to avoid the ref-churn (e.g. performance). That said, I do still tend to pass back raw pointers or references sometimes for methods that are just simple getters around a ref-counted object :P.

My understanding was that we should use RefPtr/Ref for local variables and not for argument types or return types. But I am not sure that rule works as a general rule, given that a return type has *some* of the same properties as a local variable, when used as an argument to a function or to make a member function call. I’m sure the full answer depends on the lifetime of the returned raw pointer, but it’s hard to reason about that given side effects. Like returning something owned by "this", but then a new value is assigned during the lifetime of that return value.

And: a rule that contains the phrase "unless there’s a good reason" is difficult to follow, because of how subjective that is.

You mention above that we should use a RefPtr return type and use "auto" at call sites, but in this patch we use a RefPtr return type and use "RefPtr" at call sites.
Comment 6 Wenson Hsieh 2022-01-01 11:01:10 PST
(In reply to Darin Adler from comment #5)
> Comment on attachment 448082 [details]
> For EWS
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448082&action=review
> 
> >>> Source/WebCore/html/HTMLAttachmentElement.h:67
> >>> +    RefPtr<HTMLImageElement> enclosingImageElement() const;
> >> 
> >> Someone has to write down our rules about RefPtr vs. raw pointer in new code. It seems clear this should be RefPtr in case we call any function using it as a this pointer, but not sure where we wrote that down.
> > 
> > +1 to this! I agree it would be great to have explicit guidelines around when to use RefPtr/Ref in the return type, vs. using RefPtr/Ref at the call site.
> > 
> > My understanding was that we should generally return RefPtr/Ref instead of raw pointers/references and use `auto` in call sites, unless there's a good reason to avoid the ref-churn (e.g. performance). That said, I do still tend to pass back raw pointers or references sometimes for methods that are just simple getters around a ref-counted object :P.
> 
> My understanding was that we should use RefPtr/Ref for local variables and
> not for argument types or return types. But I am not sure that rule works as
> a general rule, given that a return type has *some* of the same properties
> as a local variable, when used as an argument to a function or to make a
> member function call. I’m sure the full answer depends on the lifetime of
> the returned raw pointer, but it’s hard to reason about that given side
> effects. Like returning something owned by "this", but then a new value is
> assigned during the lifetime of that return value.

I see — it sounds like this method's return type should really just be a raw pointer, then? (Especially since I already explicitly put it in a RefPtr, as you noted below).

> 
> And: a rule that contains the phrase "unless there’s a good reason" is
> difficult to follow, because of how subjective that is.
> 
> You mention above that we should use a RefPtr return type and use "auto" at
> call sites, but in this patch we use a RefPtr return type and use "RefPtr"
> at call sites.

Whoops, good catch — that's probably from when I was going back and forth when trying to decide between returning a raw pointer vs. RefPtr :P.
Comment 7 Wenson Hsieh 2022-01-01 11:13:24 PST
Reopening to attach new patch.
Comment 8 Wenson Hsieh 2022-01-01 11:13:26 PST
Created attachment 448155 [details]
Return pointer instead of RefPtr
Comment 9 Darin Adler 2022-01-01 11:18:10 PST
Comment on attachment 448155 [details]
Return pointer instead of RefPtr

View in context: https://bugs.webkit.org/attachment.cgi?id=448155&action=review

> Source/WebCore/ChangeLog:10
> +        Make a helper method on HTMLAttachmentElement return a raw pointer instead of a RefPtr. All call sites that
> +        store the result of this method in a local variable already use RefPtr.

I know I created the doubt here, but I am still not sure about this; if any caller wanted to call a function on the result of this, then I think we’d be happier with RefPtr.
Comment 10 Wenson Hsieh 2022-01-01 12:06:09 PST
(In reply to Darin Adler from comment #9)
> Comment on attachment 448155 [details]
> Return pointer instead of RefPtr
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=448155&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Make a helper method on HTMLAttachmentElement return a raw pointer instead of a RefPtr. All call sites that
> > +        store the result of this method in a local variable already use RefPtr.
> 
> I know I created the doubt here, but I am still not sure about this; if any
> caller wanted to call a function on the result of this, then I think we’d be
> happier with RefPtr.

Hm...fair enough. Maybe I'll just leave the return value as `RefPtr` but switch the `RefPtr` => `auto` at the call sites, then?
Comment 11 Wenson Hsieh 2022-01-01 12:07:21 PST
Created attachment 448158 [details]
Use auto in a few more places
Comment 12 EWS 2022-01-02 11:06:18 PST
Committed r287521 (245656@main): <https://commits.webkit.org/245656@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448158 [details].