Bug 209754

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

Created attachment 394935 [details]
Patch

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.

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

Can we also add an assertion?

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

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

Created attachment 394973 [details]
Patch

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.

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

Created attachment 394980 [details]
Patch

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.

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

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.

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.

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

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

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

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.

Created attachment 394984 [details]
Patch

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

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

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.

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

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

<rdar://problem/61081753>

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

(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) { }