Bug 219169 – Use final in generated callback code

• Component: Bindings
• Status: RESOLVED FIXED

[Don Olmstead]

Splitting of https://bugs.webkit.org/show_bug.cgi?id=219098 to change just the callback code.

[Don Olmstead]

Created attachment 414609 [details]
Patch

[Darin Adler]

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?

[Don Olmstead]

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

[EWS]

Committed r270115: <https://trac.webkit.org/changeset/270115>

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

[Radar WebKit Bug Importer]

<rdar://problem/71627628>

[Don Olmstead]

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