Closed Bug 665632 Opened 9 years ago Closed 9 years ago

Kill AddEventListenerByIID/RemoveEventListenerByIID from satchel

Categories

(Toolkit :: Form Manager, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: sicking, Unassigned)

References

Details

Attachments

(1 file)

Attached patch Patch to fixSplinter Review
No description provided.
Attachment #540537 - Flags: review?(dolske)
Yay! I always thought this code was dumb. :)

From a quick skim: Why the removal of the IsEventTrusted(aEvent) calls? This was added in bug 511615. Does the test added there still somehow pass?
By passing false for the 4th argument when calling AddEventListener we explicitly only ask to get notified about trusted events.
Comment on attachment 540537 [details] [diff] [review]
Patch to fix

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

Woot. r+ with a couple tiny nits...

::: toolkit/components/satchel/nsFormFillController.cpp
@@ +690,5 @@
> +
> +  if (type.EqualsLiteral("blur")) {
> +    if (mFocusedInput)
> +      StopControllingInput();
> +  }

I'd just make all of these into if(...) { ...; return; } and skip the elses. I want to move this to JS at some point anyway, so that's closer to the switch/case this will become. OK either way, though...

@@ +749,5 @@
>  
>  
>  ////////////////////////////////////////////////////////////////////////
>  //// nsIDOMFocusListener
>  

Nit: nuke this too

@@ +939,5 @@
>  
>    if (!target)
>      return;
>  
> +  // XXX should this listen to "submit" as well?

Interesting!

Looks like nsFormFillController::Submit has been deadwood since at least Firefox 2. It doesn't seem correct to me anyway (eg, you might want to keep controlling the input even if the submission is canceled by some other listener).

In the end, the pagehide listener should take care of this if nothing else does.

So no need for a submit listener, and you can remove this comment.

@@ -1146,5 @@
> -                           static_cast<nsIDOMMouseListener *>(this),
> -                           PR_TRUE);
> -
> -  target->AddEventListener(NS_LITERAL_STRING("click"),
> -                           static_cast<nsIDOMMouseListener *>(this),

Ha. I was momentarily confused why this wasn't being added, then say it already was unused. >_<
Attachment #540537 - Flags: review?(dolske) → review+
Checked in, thanks for review!

http://hg.mozilla.org/mozilla-central/rev/383e60bc9089
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.