Bug 219169

Summary: Use final in generated callback code
Product: WebKit Reporter: Don Olmstead <don.olmstead>
Component: BindingsAssignee: Don Olmstead <don.olmstead>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, darin, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Don Olmstead 2020-11-19 11:15:23 PST
Splitting of https://bugs.webkit.org/show_bug.cgi?id=219098 to change just the callback code.
Comment 1 Don Olmstead 2020-11-19 12:29:44 PST
Created attachment 414609 [details]
Patch
Comment 2 Darin Adler 2020-11-19 12:59:37 PST
Comment on attachment 414609 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Generated callbacks descend from ContextDestructionObserver and are final. The 
> +        scriptExecutionContext method is not virtual so adding override errors. Remove the virtual
> +        for that case and mark the destructor as final.

When you put final on a destructor that seems to imply that the entire class could be marked final. Yet I don’t see that change here.

Thrilled that we discovered that scriptExecutionContext was unnecessarily virtual. And it’s safe if the class is indeed final. Otherwise, I worry that some derived class might want to override.

Do you know what code uses the scriptExecutionContext function?
Comment 3 Don Olmstead 2020-11-20 07:37:22 PST
(In reply to Darin Adler from comment #2)
> Comment on attachment 414609 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=414609&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Generated callbacks descend from ContextDestructionObserver and are final. The 
> > +        scriptExecutionContext method is not virtual so adding override errors. Remove the virtual
> > +        for that case and mark the destructor as final.
> 
> When you put final on a destructor that seems to imply that the entire class
> could be marked final. Yet I don’t see that change here.

The code originally had the generated classes marked as final.
 
> Thrilled that we discovered that scriptExecutionContext was unnecessarily
> virtual. And it’s safe if the class is indeed final. Otherwise, I worry that
> some derived class might want to override.
> 
> Do you know what code uses the scriptExecutionContext function?

I just happened upon this patch because I was running clang-tidy's modernizer for using override over the code and it found a bunch of cases in the generated bindings.

I did look in the derived sources to see what classes are actually generated. There are 33 in total. Callback classes derive from ActiveDOMCallback which descends from ContextDestructionObserver where scriptExecutionContext is defined in this hierarchy. The only classes that don't are the Storage ones defined in ENABLE(QUOTA) but those seem like errors since they don't descend from ContextDestructionObserver.

In a subsequent patch we might want to look at removing this generated method but we should double check everything.
Comment 4 EWS 2020-11-20 07:46:12 PST
Committed r270115: <https://trac.webkit.org/changeset/270115>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 414609 [details].
Comment 5 Radar WebKit Bug Importer 2020-11-20 07:47:18 PST
<rdar://problem/71627628>
Comment 6 Don Olmstead 2020-11-20 08:41:19 PST
(In reply to Don Olmstead from comment #3)
> (In reply to Darin Adler from comment #2)
> > Thrilled that we discovered that scriptExecutionContext was unnecessarily
> > virtual. And it’s safe if the class is indeed final. Otherwise, I worry that
> > some derived class might want to override.
> > 
> > Do you know what code uses the scriptExecutionContext function?
> 
> I just happened upon this patch because I was running clang-tidy's
> modernizer for using override over the code and it found a bunch of cases in
> the generated bindings.
> 
> I did look in the derived sources to see what classes are actually
> generated. There are 33 in total. Callback classes derive from
> ActiveDOMCallback which descends from ContextDestructionObserver where
> scriptExecutionContext is defined in this hierarchy. The only classes that
> don't are the Storage ones defined in ENABLE(QUOTA) but those seem like
> errors since they don't descend from ContextDestructionObserver.
> 
> In a subsequent patch we might want to look at removing this generated
> method but we should double check everything.

The quota callbacks being wrong brought me to https://bugs.webkit.org/show_bug.cgi?id=219206 which removes the quota module.