The default bug view has changed. See this FAQ.

Kill AddEventListenerByIID/RemoveEventListenerByIID from layout

RESOLVED FIXED in mozilla7

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

(Depends on: 2 bugs)

Trunk
mozilla7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments)

13.75 KB, patch
smaug
: review+
Details | Diff | Splinter Review
6.49 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.30 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.64 KB, patch
smaug
: review+
Details | Diff | Splinter Review
10.63 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.19 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.54 KB, patch
smaug
: review+
Details | Diff | Splinter Review
7.18 KB, patch
smaug
: review+
Details | Diff | Splinter Review
15.96 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.39 KB, patch
smaug
: review+
Details | Diff | Splinter Review
This will be a whole set of patches. Will probably spread out landing them over a few days.
OS: Mac OS X → All
Hardware: x86 → All
Created attachment 539090 [details] [diff] [review]
Remove from nsImageMap and nsSliderFrame
Assignee: nobody → jonas
Attachment #539090 - Flags: review?(Olli.Pettay)
Created attachment 539094 [details] [diff] [review]
Remove from nsSplitterFrame
Created attachment 539095 [details] [diff] [review]
Remove from mathml code
Created attachment 539096 [details] [diff] [review]
Remove from nsDocumentViewer
Created attachment 539097 [details] [diff] [review]
Remove from nsListControlFrame
Created attachment 539098 [details] [diff] [review]
Remove from nsComboboxControlFrame
These are all ready to review, but I figured I should spread the load a bit. If anyone wants to review them, don't let me stop you :)
Created attachment 540508 [details] [diff] [review]
Remove from nsXULPopupManager
Created attachment 540519 [details] [diff] [review]
Remove from nsFileControlFrame
Created attachment 540527 [details] [diff] [review]
Remove from nsXULTooltipListener
Created attachment 540531 [details] [diff] [review]
Remove from nsMenuBarListener
I'll try to review these today or tomorrow. If I don't, please ping me.
Comment on attachment 539090 [details] [diff] [review]
Remove from nsImageMap and nsSliderFrame


>+nsImageMap::HandleEvent(nsIDOMEvent* aEvent)
> {
>-  return ChangeFocus(aEvent, PR_TRUE);
>-}
>+  nsAutoString eventType;
>+  aEvent->GetType(eventType);
>+  PRBool focus = eventType.EqualsLiteral("focus");
>+  NS_ABORT_IF_FASLSE(focus == !eventType.EqualsLiteral("blur"),
>+                     "Unexpected event type");
You didn't compile this patch?
Or, as I assume, you just accidentally changed
the patch after compiling it.
Attachment #539090 - Flags: review?(Olli.Pettay) → review+

Updated

6 years ago
Attachment #539094 - Flags: review+
Comment on attachment 539095 [details] [diff] [review]
Remove from mathml code


>+nsMathMLmactionFrame::MouseListener::HandleEvent(nsIDOMEvent* aEvent)
> {
>-  mOwner->MouseOver();
Could you add nsCOMPtr<nsIDOMMouseEvent> check here?


>+  nsAutoString eventType;
>+  aEvent->GetType(eventType);
>+  if (eventType.EqualsLiteral("mouseover")) {
>+    mOwner->MouseOver();
>+  }
>+  else if (eventType.EqualsLiteral("mouseclick")) {
>+    mOwner->MouseClick();
>+  }
>+  else if (eventType.EqualsLiteral("mouseout")) {
>+    mOwner->MouseOut();
>+  }
>+  else {
>+    NS_ABORT();
>+  }

if (expr) {
  stmt;
} else if (expr) {
  stmt;
}
Attachment #539095 - Flags: review+
Comment on attachment 539096 [details] [diff] [review]
Remove from nsDocumentViewer


> nsDocViewerFocusListener::HandleEvent(nsIDOMEvent* aEvent)
> {
>+  if(!mDocViewer)
>+    return NS_ERROR_FAILURE;
if (expr) {
  stmt;
}

Or, NS_ENSURE_STATE(mDocViewer);


>+
>+  nsCOMPtr<nsIPresShell> shell;
>+  nsresult rv = mDocViewer->GetPresShell(getter_AddRefs(shell));
>+  NS_ENSURE_SUCCESS(rv, rv);
No need for NS_ENSURE_SUCCESS
>+  NS_ENSURE_TRUE(shell, NS_ERROR_FAILURE);


>+  nsAutoString eventType;
>+  aEvent->GetType(eventType);
>+  if (eventType.EqualsLiteral("focus")) {
>+    // If selection was disabled, re-enable it.
>+    if(selectionStatus == nsISelectionController::SELECTION_DISABLED ||
>+       selectionStatus == nsISelectionController::SELECTION_HIDDEN)
>+    {
>+      selCon->SetDisplaySelection(nsISelectionController::SELECTION_ON);
>+      selCon->RepaintSelection(nsISelectionController::SELECTION_NORMAL);
>+    }
>+  }
>+  else {

if (expr) {
  stmt;
} else {
  stmt;
}
Attachment #539096 - Flags: review+
Comment on attachment 539097 [details] [diff] [review]
Remove from nsListControlFrame


>-  // mouse event listeners (both might destroy |this|)
>   nsresult MouseDown(nsIDOMEvent* aMouseEvent);
>   nsresult MouseUp(nsIDOMEvent* aMouseEvent);
>-
>-  // mouse motion listeners
>   nsresult MouseMove(nsIDOMEvent* aMouseEvent);
>   nsresult DragMove(nsIDOMEvent* aMouseEvent);
>-
>-  // key listener (might destroy |this|)
>   nsresult KeyPress(nsIDOMEvent* aKeyEvent);
Don't remove the "might destroy |this|" comments.
Attachment #539097 - Flags: review+
Comment on attachment 539098 [details] [diff] [review]
Remove from nsComboboxControlFrame


>+  NS_IMETHOD HandleEvent(nsIDOMEvent*)
>   {
>     mComboBox->ShowDropDown(!mComboBox->IsDroppedDown());
>     return NS_OK; 
>   }
Could you add nsIDOMMouseEvent check here.
I wouldn't want any changes to behavior in this kinds of clean-up patches.
Attachment #539098 - Flags: review+
Comment on attachment 540508 [details] [diff] [review]
Remove from nsXULPopupManager

>+nsXULPopupManager::HandleEvent(nsIDOMEvent* aEvent)
>+{
nsIDOMKeyEvent check somewhere here and pass
the keyevent to the KeyUp/Down/Press methods?

>+  nsAutoString eventType;
>+  aEvent->GetType(eventType);
>+  if (eventType.EqualsLiteral("keyup"))
>+    return KeyUp(aEvent);
>+  if (eventType.EqualsLiteral("keydown"))
>+    return KeyDown(aEvent);
>+  if (eventType.EqualsLiteral("keypress"))
>+    return KeyPress(aEvent);
if (expr) {
  stmt;
}
Attachment #540508 - Flags: review+
Comment on attachment 540519 [details] [diff] [review]
Remove from nsFileControlFrame

>diff --git a/layout/forms/nsFileControlFrame.cpp b/layout/forms/nsFileControlFrame.cpp
>--- a/layout/forms/nsFileControlFrame.cpp
>+++ b/layout/forms/nsFileControlFrame.cpp
>@@ -49,17 +49,16 @@
> #include "nsHTMLParts.h"
> #include "nsIDOMHTMLInputElement.h"
> #include "nsIFormControl.h"
> #include "nsINameSpaceManager.h"
> #include "nsCOMPtr.h"
> #include "nsIDOMElement.h"
> #include "nsIDOMDocument.h"
> #include "nsIDocument.h"
>-#include "nsIDOMMouseListener.h"
> #include "nsIPresShell.h"
> #include "nsXPCOM.h"
> #include "nsISupportsPrimitives.h"
> #include "nsIComponentManager.h"
> #include "nsPIDOMWindow.h"
> #include "nsIFilePicker.h"
> #include "nsIDOMMouseEvent.h"
> #include "nsINodeInfo.h"
>@@ -386,17 +385,17 @@ nsFileControlFrame::SetFocus(PRBool aOn,
> {
> }
> 
> PRBool ShouldProcessMouseClick(nsIDOMEvent* aMouseEvent)
> {
>   // only allow the left button
>   nsCOMPtr<nsIDOMMouseEvent> mouseEvent = do_QueryInterface(aMouseEvent);
>   nsCOMPtr<nsIDOMNSUIEvent> uiEvent = do_QueryInterface(aMouseEvent);
>-  NS_ENSURE_STATE(uiEvent);
>+  NS_ENSURE_TRUE(mouseEvent && uiEvent, PR_FALSE);
>   PRBool defaultPrevented = PR_FALSE;
>   uiEvent->GetPreventDefault(&defaultPrevented);
>   if (defaultPrevented) {
>     return PR_FALSE;
>   }
> 
>   PRUint16 whichButton;
>   if (NS_FAILED(mouseEvent->GetButton(&whichButton)) || whichButton != 0) {
>@@ -410,17 +409,17 @@ PRBool ShouldProcessMouseClick(nsIDOMEve
> 
>   return PR_TRUE;
> }
> 
> /**
>  * This is called when our capture button is clicked
>  */
> NS_IMETHODIMP
>-nsFileControlFrame::CaptureMouseListener::MouseClick(nsIDOMEvent* aMouseEvent)
>+nsFileControlFrame::CaptureMouseListener::HandleEvent(nsIDOMEvent* aMouseEvent)
> {
>   nsresult rv;
> 
>   NS_ASSERTION(mFrame, "We should have been unregistered");
>   if (!ShouldProcessMouseClick(aMouseEvent))
>     return NS_OK;
> 
>   // Get parent nsIDOMWindowInternal object.
>@@ -498,54 +497,48 @@ nsFileControlFrame::CaptureMouseListener
>     // May need to fire an onchange here
>     mFrame->mTextFrame->CheckFireOnChange();
>   }
> 
>   return NS_OK;
> }
> 
> /**
>- * This is called when our browse button is clicked
>- */
>-NS_IMETHODIMP
>-nsFileControlFrame::BrowseMouseListener::MouseClick(nsIDOMEvent* aMouseEvent)
>-{
>-  NS_ASSERTION(mFrame, "We should have been unregistered");
>-  if (!ShouldProcessMouseClick(aMouseEvent))
>-    return NS_OK;
>-  
>-  nsHTMLInputElement* input =
>-    nsHTMLInputElement::FromContent(mFrame->GetContent());
>-  return input ? input->FireAsyncClickHandler() : NS_OK;
>-}
>-
>-/**
>  * This is called when we receive any registered events on the control.
>- * We've only registered for drop, dragover and click events, and click events
>- * already call MouseClick() for us. Here, we handle file drops.
>+ * We've only registered for drop, dragover and click events.
>  */
> NS_IMETHODIMP
> nsFileControlFrame::BrowseMouseListener::HandleEvent(nsIDOMEvent* aEvent)
> {
>   NS_ASSERTION(mFrame, "We should have been unregistered");
>+
>+  nsAutoString eventType;
>+  aEvent->GetType(eventType);
>+  if (eventType.EqualsLiteral("click")) {
>+    if (!ShouldProcessMouseClick(aEvent))
>+      return NS_OK;
>+    
>+    nsHTMLInputElement* input =
>+      nsHTMLInputElement::FromContent(mFrame->GetContent());
>+    return input ? input->FireAsyncClickHandler() : NS_OK;
>+  }
>+
>   nsCOMPtr<nsIDOMNSUIEvent> uiEvent = do_QueryInterface(aEvent);
>   NS_ENSURE_STATE(uiEvent);
>   PRBool defaultPrevented = PR_FALSE;
>   uiEvent->GetPreventDefault(&defaultPrevented);
>   if (defaultPrevented) {
>     return NS_OK;
>   }
>   
>   nsCOMPtr<nsIDOMDragEvent> dragEvent = do_QueryInterface(aEvent);
>   if (!dragEvent || !IsValidDropData(dragEvent)) {
>     return NS_OK;
>   }
> 
>-  nsAutoString eventType;
>-  aEvent->GetType(eventType);
>   if (eventType.EqualsLiteral("dragover")) {
>     // Prevent default if we can accept this drag data
>     aEvent->PreventDefault();
>     return NS_OK;
>   }
> 
>   if (eventType.EqualsLiteral("drop")) {
>     aEvent->StopPropagation();
>@@ -833,11 +826,10 @@ nsFileControlFrame::ParseAcceptAttribute
>   // Empty loop body because aCallback is doing the work
>   while (tokenizer.hasMoreTokens() &&
>          (*aCallback)(tokenizer.nextToken(), aClosure));
> }
> 
> ////////////////////////////////////////////////////////////
> // Mouse listener implementation
> 
>-NS_IMPL_ISUPPORTS2(nsFileControlFrame::MouseListener,
>-                   nsIDOMMouseListener,
>+NS_IMPL_ISUPPORTS1(nsFileControlFrame::MouseListener,
>                    nsIDOMEventListener)
>diff --git a/layout/forms/nsFileControlFrame.h b/layout/forms/nsFileControlFrame.h
>--- a/layout/forms/nsFileControlFrame.h
>+++ b/layout/forms/nsFileControlFrame.h
>@@ -35,17 +35,17 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #ifndef nsFileControlFrame_h___
> #define nsFileControlFrame_h___
> 
> #include "nsBlockFrame.h"
> #include "nsIFormControlFrame.h"
>-#include "nsIDOMMouseListener.h"
>+#include "nsIDOMEventListener.h"
> #include "nsIAnonymousContentCreator.h"
> #include "nsICapturePicker.h"
> #include "nsCOMPtr.h"
> 
> #include "nsTextControlFrame.h"
> typedef   nsTextControlFrame nsNewFrame;
> 
> class nsIDOMDragEvent;
>@@ -107,37 +107,27 @@ public:
>   void ParseAcceptAttribute(AcceptAttrCallback aCallback, void* aClosure) const;
> 
>   nsIFrame* GetTextFrame() { return mTextFrame; }
> 
> protected:
> 
>   class MouseListener;
>   friend class MouseListener;
>-  class MouseListener : public nsIDOMMouseListener {
>+  class MouseListener : public nsIDOMEventListener {
>   public:
>     NS_DECL_ISUPPORTS
>     
>     MouseListener(nsFileControlFrame* aFrame) :
>       mFrame(aFrame)
>     {}
> 
>     void ForgetFrame() {
>       mFrame = nsnull;
>     }
>-    
>-    // We just want to capture the click events on our browse button
>-    // and textfield.
>-    NS_IMETHOD MouseDown(nsIDOMEvent* aMouseEvent) { return NS_OK; }
>-    NS_IMETHOD MouseUp(nsIDOMEvent* aMouseEvent) { return NS_OK; }
>-    NS_IMETHOD MouseClick(nsIDOMEvent* aMouseEvent) = 0;
>-    NS_IMETHOD MouseDblClick(nsIDOMEvent* aMouseEvent) { return NS_OK; }
>-    NS_IMETHOD MouseOver(nsIDOMEvent* aMouseEvent) { return NS_OK; }
>-    NS_IMETHOD MouseOut(nsIDOMEvent* aMouseEvent) { return NS_OK; }
>-    NS_IMETHOD HandleEvent(nsIDOMEvent* aEvent) { return NS_OK; }
> 
>   protected:
>     nsFileControlFrame* mFrame;
>   };
> 
>   class SyncDisabledStateEvent;
>   friend class SyncDisabledStateEvent;
>   class SyncDisabledStateEvent : public nsRunnable
>@@ -158,27 +148,26 @@ protected:
>   private:
>     nsWeakFrame mFrame;
>   };
> 
>   class CaptureMouseListener: public MouseListener {
>   public:
>     CaptureMouseListener(nsFileControlFrame* aFrame) : MouseListener(aFrame),
>                                                        mMode(0) {};
>-    NS_IMETHOD MouseClick(nsIDOMEvent* aMouseEvent);
>+    NS_DECL_NSIDOMEVENTLISTENER
>     PRUint32 mMode;
>   };
>   
>   class BrowseMouseListener: public MouseListener {
>   public:
>     BrowseMouseListener(nsFileControlFrame* aFrame) : MouseListener(aFrame) {};
>-     NS_IMETHOD MouseClick(nsIDOMEvent* aMouseEvent);
>-     NS_IMETHOD HandleEvent(nsIDOMEvent* aEvent);
>+    NS_DECL_NSIDOMEVENTLISTENER
> 
>-     static PRBool IsValidDropData(nsIDOMDragEvent* aEvent);
>+    static PRBool IsValidDropData(nsIDOMDragEvent* aEvent);
>   };
> 
>   virtual PRBool IsFrameOfType(PRUint32 aFlags) const
>   {
>     return nsBlockFrame::IsFrameOfType(aFlags &
>       ~(nsIFrame::eReplaced | nsIFrame::eReplacedContainsBlock));
>   }
>
Attachment #540519 - Flags: review+

Updated

6 years ago
Attachment #540527 - Flags: review+
Comment on attachment 540531 [details] [diff] [review]
Remove from nsMenuBarListener

>+
>+  NS_ABORT();
>+
>   return NS_OK;
Although it doesn't matter, this sure looks strange
NS_ABORT();
return NS_OK;

But that is still ok.
Attachment #540531 - Flags: review+
(In reply to comment #14)
> Comment on attachment 539095 [details] [diff] [review] [review]
> Remove from mathml code
> 
> 
> >+nsMathMLmactionFrame::MouseListener::HandleEvent(nsIDOMEvent* aEvent)
> > {
> >-  mOwner->MouseOver();
> Could you add nsCOMPtr<nsIDOMMouseEvent> check here?

Again, is this really needed given that we don't use the event object? Yes, I know that it means that we'll change behavior if there's buggy chrome code that dispatches mouse events using the wrong interface, but that doesn't seem important enough to protect against.

> >+  nsAutoString eventType;
> >+  aEvent->GetType(eventType);
> >+  if (eventType.EqualsLiteral("mouseover")) {
> >+    mOwner->MouseOver();
> >+  }
> >+  else if (eventType.EqualsLiteral("mouseclick")) {
> >+    mOwner->MouseClick();
> >+  }
> >+  else if (eventType.EqualsLiteral("mouseout")) {
> >+    mOwner->MouseOut();
> >+  }
> >+  else {
> >+    NS_ABORT();
> >+  }
> 
> if (expr) {
>   stmt;
> } else if (expr) {
>   stmt;
> }

That doesn't match the style of the rest of this file (with one exception)
Checked in the first 4 patches:

http://hg.mozilla.org/mozilla-central/rev/49d9e0f08eb5
http://hg.mozilla.org/mozilla-central/rev/2bf9296c7bd0
http://hg.mozilla.org/mozilla-central/rev/a41df1f97f0d
http://hg.mozilla.org/mozilla-central/rev/8ed7833d498b
Checked in 4 more:

http://hg.mozilla.org/mozilla-central/rev/adb41b3796e6
http://hg.mozilla.org/mozilla-central/rev/2c71a7c1cb48
http://hg.mozilla.org/mozilla-central/rev/a45f99f995c2
http://hg.mozilla.org/mozilla-central/rev/6f811ab91ac2
Checked in the last two to mozilla-inbound!

http://hg.mozilla.org/integration/mozilla-inbound/rev/e3cb39d085f3
http://hg.mozilla.org/integration/mozilla-inbound/rev/4e08121dcd1e

Thanks for all the reviews!!!
http://hg.mozilla.org/mozilla-central/rev/e3cb39d085f3
http://hg.mozilla.org/mozilla-central/rev/4e08121dcd1e
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Depends on: 678708
After landing this patches was this bug fixed or it has a major dependency on 678708?
Thanks.

Updated

6 years ago
Depends on: 685470

Updated

5 years ago
Depends on: 715882

Updated

5 years ago
Depends on: 693551

Updated

5 years ago
Depends on: 715999

Updated

5 years ago
Depends on: 717289

Updated

5 years ago
Depends on: 787845
You need to log in before you can comment on or make changes to this bug.