Bug 209754 – Overrides of ActiveDOMObject::hasPendingActivity() should not need to query the base class's hasPendingActivity()

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

hasPendingActivity() overrides should make sure to query ActiveDOMObject::hasPendingActivity(). It is important because code may be using makePendingActivity().

[Ryosuke Niwa]

Maybe we can add an assert? Convert all call sites to call something else and make sure ActiveDOMObject::hasPendingActivity() is called in that function?

[Chris Dumez]

Created attachment 394935 [details]
Patch

[Darin Adler]

Comment on attachment 394935 [details]
Patch

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

How did you decide when to use || and when to use a separate if instead? What about when to do base class first and when to do the subclass first?

Could also consider a different design where the code calling hasPendingActivity deals with the rules currently followed by ActiveDOMObject::hasPendingActivity instead of requiring calling through to the base class to make this mistake impossible.

> Source/WebCore/dom/MessagePort.cpp:320
> +    if (ActiveDOMObject::hasPendingActivity())
> +        return true;

A little strange to do this after checking m_closed. Probably fine but I don’t really understand this fully.

> Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5558
>  bool WebGLRenderingContextBase::hasPendingActivity() const
>  {
> -    return false;
> +    return ActiveDOMObject::hasPendingActivity();
>  }

Why does this override exist at all? I think we should remove it instead.

[Chris Dumez]

(In reply to Darin Adler from comment #3)
> Comment on attachment 394935 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394935&action=review
> 
> How did you decide when to use || and when to use a separate if instead?

Sometimes the return statement already looked so complicated that I felt an early if check & return looked clearer. This was just my own personal judgement.


> What about when to do base class first and when to do the subclass first?

Thinking about this more, I think we should always return true if the base class method returns true. See explanation below.

> 
> Could also consider a different design where the code calling
> hasPendingActivity deals with the rules currently followed by
> ActiveDOMObject::hasPendingActivity instead of requiring calling through to
> the base class to make this mistake impossible.

Hmm. This would indeed be a better design. I will give this a shot and if I come up with something good, will upload it for review instead of this.

> 
> > Source/WebCore/dom/MessagePort.cpp:320
> > +    if (ActiveDOMObject::hasPendingActivity())
> > +        return true;
> 
> A little strange to do this after checking m_closed. Probably fine but I
> don’t really understand this fully.

Hmm. I is probably safer to check this first. You could imagine closing a port, and the action of closing the port scheduling an event to be dispatched asynchronously (using makePendingActivity()).
If this happened, you'd want the wrapper to stay alive long enough to dispatch the event. Will fix.

> 
> > Source/WebCore/html/canvas/WebGLRenderingContextBase.cpp:5558
> >  bool WebGLRenderingContextBase::hasPendingActivity() const
> >  {
> > -    return false;
> > +    return ActiveDOMObject::hasPendingActivity();
> >  }
> 
> Why does this override exist at all? I think we should remove it instead.

Fair point. Will remove.

[Ryosuke Niwa]

Can we also add an assertion?

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #5)
> Can we also add an assertion?

With Darin's proposal, I don't think there will be any need for an assertion. I am working on that patch.

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #6)
> (In reply to Ryosuke Niwa from comment #5)
> > Can we also add an assertion?
> 
> With Darin's proposal, I don't think there will be any need for an
> assertion. I am working on that patch.

Ok.

[Chris Dumez]

Created attachment 394973 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 394973 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        To address the issue, ActiveDOMObject::hasPendingActivity() is no longer virtual and
> +        checks both m_pendingActivityCount and a virtual virtualHasPendingActivity() function.

It's confusing that m_pendingActivityCount can be 0 and ActiveDOMObject::hasPendingActivity() can still return true.
How about calling the former m_pendingActivity*Token*Count and this subclass-specific pending activity concept hasInternalPendingActivity.
Then we can rename ActiveDOMObject::makePendingActivity to ActiveDOMObject::makePendingActivityToken as well.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #9)
> Comment on attachment 394973 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394973&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        To address the issue, ActiveDOMObject::hasPendingActivity() is no longer virtual and
> > +        checks both m_pendingActivityCount and a virtual virtualHasPendingActivity() function.
> 
> It's confusing that m_pendingActivityCount can be 0 and
> ActiveDOMObject::hasPendingActivity() can still return true.

I buy that but this has always been the case.

> How about calling the former m_pendingActivity*Token*Count and this
> subclass-specific pending activity concept hasInternalPendingActivity.
> Then we can rename ActiveDOMObject::makePendingActivity to
> ActiveDOMObject::makePendingActivityToken as well.

Not a big fan of the naming suggested but I agree we can likely make things a bit clearer through renaming. I need to give it more thought to see if I can come up with better naming.

[Chris Dumez]

Created attachment 394980 [details]
Patch

[Darin Adler]

Comment on attachment 394973 [details]
Patch

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

A little clunky to have virtual in the name, but I like it!

> Source/WebCore/dom/ActiveDOMObject.h:76
> +    // FIXME: Drop this method.

Nitpick: "function" instead of "method"?

> Source/WebCore/dom/ActiveDOMObject.h:85
> +    // FIXME: Drop this method.

Ditto.

[Chris Dumez]

(In reply to Darin Adler from comment #12)
> Comment on attachment 394973 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394973&action=review
> 
> A little clunky to have virtual in the name, but I like it!
> 
> > Source/WebCore/dom/ActiveDOMObject.h:76
> > +    // FIXME: Drop this method.
> 
> Nitpick: "function" instead of "method"?
> 
> > Source/WebCore/dom/ActiveDOMObject.h:85
> > +    // FIXME: Drop this method.
> 
> Ditto.

Why isn't is a method? It can operate on data members.

[Chris Dumez]

Comment on attachment 394980 [details]
Patch

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

> Source/WebCore/dom/ActiveDOMObject.h:153
> +    unsigned m_pendingActivityInstancesCount { 0 };

I renamed this to address the confusion that Ryosuke was talking about.

[Geoffrey Garen]

I wonder if ActiveDOMObject::hasPendingActivity should be non-virtual, checking m_pendingActivityCount and then calling out to a virtual hasPendingActivityVirtual() or something like that. That would eliminate this class of bugs.

[Geoffrey Garen]

(In reply to Geoffrey Garen from comment #15)
> I wonder if ActiveDOMObject::hasPendingActivity should be non-virtual,
> checking m_pendingActivityCount and then calling out to a virtual
> hasPendingActivityVirtual() or something like that. That would eliminate
> this class of bugs.

Oh, LOL, that's what you did.

[Darin Adler]

(In reply to Chris Dumez from comment #13)
> Why isn't is a method? It can operate on data members.

C++ has member functions.

Objective-C has methods.

Obviously you can use other terminology besides what is used to document the language. If you want to use "method" as a term of art for non-const member functions in C++ that is OK, I guess.

But I personally like using the actual terms from the language's own specification to avoid ambiguity. I thought this was consensus but maybe not.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #15)
> I wonder if ActiveDOMObject::hasPendingActivity should be non-virtual,
> checking m_pendingActivityCount and then calling out to a virtual
> hasPendingActivityVirtual() or something like that. That would eliminate
> this class of bugs.

Isn't that exactly what my patch does? :) except that I called the method virtualHasPendingActivity() and you called it hasPendingActivityVirtual().

[Geoffrey Garen]

Comment on attachment 394980 [details]
Patch

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

r=me

>> Source/WebCore/dom/ActiveDOMObject.h:153
>> +    unsigned m_pendingActivityInstancesCount { 0 };
> 
> I renamed this to address the confusion that Ryosuke was talking about.

I think we usually say "m_pendingActivityInstanceCount", without the plural.

[Chris Dumez]

Created attachment 394984 [details]
Patch

[Chris Dumez]

(In reply to Geoffrey Garen from comment #19)
> Comment on attachment 394980 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394980&action=review
> 
> r=me
> 
> >> Source/WebCore/dom/ActiveDOMObject.h:153
> >> +    unsigned m_pendingActivityInstancesCount { 0 };
> > 
> > I renamed this to address the confusion that Ryosuke was talking about.
> 
> I think we usually say "m_pendingActivityInstanceCount", without the plural.

Fixed.

[Chris Dumez]

(In reply to Darin Adler from comment #17)
> (In reply to Chris Dumez from comment #13)
> > Why isn't is a method? It can operate on data members.
> 
> C++ has member functions.

Oh OK. I have have calling these methods all these years :) Fixed.

[Ryosuke Niwa]

Comment on attachment 394984 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        checks both m_pendingActivityCount and a virtual virtualHasPendingActivity() function.

Can we call virtualHasPendingActivity hasPendingActivityInternal instead?
That's usually what we call these internal / override-only functions.

[EWS]

Committed r259252: <https://trac.webkit.org/changeset/259252>

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

[Radar WebKit Bug Importer]

<rdar://problem/61081753>

[Geoffrey Garen]

> Can we call virtualHasPendingActivity hasPendingActivityInternal instead?
> That's usually what we call these internal / override-only functions.

I'm not familiar with the use of Internal in this way. Can you give an example?

I see "...Impl" used by RenderFlexibleBox's isFlexibleBoxImpl. 

I see "virtual..." used by InlineBox's logicalHeight.

"Internal" strikes me as surprising because usually "internal" usually means something that others can't poke at. But this function facilitates overrides by subclasses.

[Ryosuke Niwa]

(In reply to Geoffrey Garen from comment #26)
> > Can we call virtualHasPendingActivity hasPendingActivityInternal instead?
> > That's usually what we call these internal / override-only functions.
> 
> I'm not familiar with the use of Internal in this way. Can you give an
> example?
> 
> I see "...Impl" used by RenderFlexibleBox's isFlexibleBoxImpl. 
> 
> I see "virtual..." used by InlineBox's logicalHeight.
> 
> "Internal" strikes me as surprising because usually "internal" usually means
> something that others can't poke at. But this function facilitates overrides
> by subclasses.

I don't think it's strange. It communicates that it's not a function that can be called externally. It's internal to the class and its subclasses, not a part of a public API.

e.g.
Node
    virtual Ref<Node> cloneNodeInternal(Document&, CloningOperation) = 0;

CSSStyleDeclaration
    virtual RefPtr<CSSValue> getPropertyCSSValueInternal(CSSPropertyID) = 0;
    virtual String getPropertyValueInternal(CSSPropertyID) = 0;
    virtual ExceptionOr<bool> setPropertyInternal(CSSPropertyID, const String& value, bool important) = 0;

GraphicsLayer
    virtual void setOpacityInternal(float) { }