801609 - Simplify popup and contextmenu handling and make the listener skippable

Status: RESOLVED FIXED

(Core :: XUL, defect)

Comment 1

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

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=d2e56528cd0d

Comment 2

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

Attachment: v2

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 671494 [details] [diff] [review]
v2

Review of attachment 671494 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable to me.

::: content/xul/content/src/nsXULElement.cpp
@@ +1573,3 @@
>  
>      // Add the popup as a listener on this element.
>      nsEventListenerManager* manager = GetListenerManager(true);

Why can you get rid of the null check on manager?

@@ -1603,5 @@
> -    nsresult rv = SetProperty(listenerAtom, popupListener,
> -                              PopupListenerPropertyDtor, true);
> -    NS_ENSURE_SUCCESS(rv, rv);
> -    // Want the property to have a reference to the listener.
> -    nsIDOMEventListener* listener = nullptr;

This old code looks kind of sketchy. I guess it is okay because popupListener has been set as a property.

::: content/xul/content/src/nsXULElement.h
@@ +327,3 @@
>  };
>  
>  // Make sure we have space for our bit

our bit -> our bits

::: content/xul/content/src/nsXULPopupListener.cpp
@@ +334,5 @@
>  
>    if (identifier.IsEmpty()) {
> +    if (type == nsGkAtoms::popup) {
> +      mElement->GetAttr(kNameSpaceID_None, nsGkAtoms::menu, identifier);
> +    } else if (type == nsGkAtoms::context) {

type can't change between when it is assigned and here, can it? Can you drop this branch, and put an assert in or something.  You could also test !mIsContext instead of type == nsGkAtoms::popup, but I guess that's probably any better.

It would also make slightly more sense to me if the order of these branches matched the mIsContext above, but no big deal.

Comment 4

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

> Why can you get rid of the null check on manager?
It would be fallible OOM

> type can't change between when it is assigned and here, can it? Can you drop
> this branch, and put an assert in or something.
I don't understand this comment

Comment 5

[Andrew McCreight [:mccr8]]

> It would be fallible OOM
Oh, okay, so because you pass in true, you know it will return non-null, but if you passed in false, it might return null, which is why it is called |GetListenerManager|.

> I don't understand this comment

It looks to me like |type| can only have one of two values, either |nsGkAtoms::context| or |nsGkAtoms::popup|. So I don't understand why you have to check if |type == context| after you know that it doesn't equal |popup|. Not a big deal, I'm just curious.

Comment 6

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

> It looks to me like |type| can only have one of two values, either
> |nsGkAtoms::context| or |nsGkAtoms::popup|. So I don't understand why you
> have to check if |type == context| after you know that it doesn't equal
> |popup|. Not a big deal, I'm just curious.
Ah, that. Leftover logic from the old code.

Comment 7

[Andrew McCreight [:mccr8]]

That was actually true in the old code, too, so it must have been some relic of an earlier change.

Comment 8

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

Attachment: v3

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/fc883f5a1a08