930855 - Replace WidgetEvent::Is*DerivedClass() with As*Event()

Status: RESOLVED FIXED

(Core :: Widget, defect)

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 822124 [details] [diff] [review]
Patch

I don't understand. Why replace non-virtual method with a virtual one?

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 822124 [details] [diff] [review]
Patch

(In reply to Olli Pettay [:smaug] from comment #2)
> Comment on attachment 822124 [details] [diff] [review]
> Patch
> 
> I don't understand. Why replace non-virtual method with a virtual one?

The reason is that the methods are non-virtual but they call virtual methods (As*Event()) internally. So, I believe that it doesn't make sense we keep having duplicated methods for different return type.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 822124 [details] [diff] [review]
Patch

Ah, indeed.

(Though, I guess at some point we'll want IsFooEvent() methods which aren't virtual)

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/340cb402065a

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #4)
> (Though, I guess at some point we'll want IsFooEvent() methods which aren't
> virtual)

As you said, if IsFooEvent() should return true even if an instance inherits the class, I don't think we need them because they're just wrappers of AsFooEvent() != nullptr, isn't it?

I was suggested that we should implement AsFooEvent() as non-virtual method, but the suggested patch was more complicated than current code. Therefore, it was not granted. See bug 920425 comment 14.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/340cb402065a