Closed Bug 801609 Opened 7 years ago Closed 7 years ago

Simplify popup and contextmenu handling and make the listener skippable

Categories

(Core :: XUL, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: smaug, Assigned: smaug)

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
Attached patch v2Splinter Review
Attachment #671494 - Flags: review?(continuation)
Attachment #671410 - Attachment is obsolete: true
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.
Attachment #671494 - Flags: review?(continuation) → review+
> 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
> 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.
> 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.
That was actually true in the old code, too, so it must have been some relic of an earlier change.
Attached patch v3Splinter Review
https://hg.mozilla.org/mozilla-central/rev/fc883f5a1a08
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.