Last Comment Bug 664058 - Kill AddEventListenerByIID/RemoveEventListenerByIID from layout
: Kill AddEventListenerByIID/RemoveEventListenerByIID from layout
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Jonas Sicking (:sicking)
:
Mentors:
Depends on: 717289 787845 678708 685470 693551 715882 715999
Blocks: 661297
  Show dependency treegraph
 
Reported: 2011-06-13 19:02 PDT by Jonas Sicking (:sicking)
Modified: 2012-09-05 00:00 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove from nsImageMap and nsSliderFrame (13.75 KB, patch)
2011-06-13 19:31 PDT, Jonas Sicking (:sicking)
bugs: review+
Details | Diff | Review
Remove from nsSplitterFrame (6.49 KB, patch)
2011-06-13 19:48 PDT, Jonas Sicking (:sicking)
bugs: review+
Details | Diff | Review
Remove from mathml code (7.30 KB, patch)
2011-06-13 19:48 PDT, Jonas Sicking (:sicking)
bugs: review+
Details | Diff | Review
Remove from nsDocumentViewer (9.64 KB, patch)
2011-06-13 19:49 PDT, Jonas Sicking (:sicking)
bugs: review+
Details | Diff | Review
Remove from nsListControlFrame (10.63 KB, patch)
2011-06-13 19:50 PDT, Jonas Sicking (:sicking)
bugs: review+
Details | Diff | Review
Remove from nsComboboxControlFrame (4.19 KB, patch)
2011-06-13 19:50 PDT, Jonas Sicking (:sicking)
bugs: review+
Details | Diff | Review
Remove from nsXULPopupManager (5.54 KB, patch)
2011-06-20 09:59 PDT, Jonas Sicking (:sicking)
bugs: review+
Details | Diff | Review
Remove from nsFileControlFrame (7.18 KB, patch)
2011-06-20 10:15 PDT, Jonas Sicking (:sicking)
bugs: review+
Details | Diff | Review
Remove from nsXULTooltipListener (15.96 KB, patch)
2011-06-20 10:46 PDT, Jonas Sicking (:sicking)
bugs: review+
Details | Diff | Review
Remove from nsMenuBarListener (9.39 KB, patch)
2011-06-20 10:52 PDT, Jonas Sicking (:sicking)
bugs: review+
Details | Diff | Review

Description Jonas Sicking (:sicking) 2011-06-13 19:02:50 PDT
This will be a whole set of patches. Will probably spread out landing them over a few days.
Comment 1 Jonas Sicking (:sicking) 2011-06-13 19:31:59 PDT
Created attachment 539090 [details] [diff] [review]
Remove from nsImageMap and nsSliderFrame
Comment 2 Jonas Sicking (:sicking) 2011-06-13 19:48:21 PDT
Created attachment 539094 [details] [diff] [review]
Remove from nsSplitterFrame
Comment 3 Jonas Sicking (:sicking) 2011-06-13 19:48:54 PDT
Created attachment 539095 [details] [diff] [review]
Remove from mathml code
Comment 4 Jonas Sicking (:sicking) 2011-06-13 19:49:33 PDT
Created attachment 539096 [details] [diff] [review]
Remove from nsDocumentViewer
Comment 5 Jonas Sicking (:sicking) 2011-06-13 19:50:16 PDT
Created attachment 539097 [details] [diff] [review]
Remove from nsListControlFrame
Comment 6 Jonas Sicking (:sicking) 2011-06-13 19:50:51 PDT
Created attachment 539098 [details] [diff] [review]
Remove from nsComboboxControlFrame
Comment 7 Jonas Sicking (:sicking) 2011-06-13 19:51:35 PDT
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 :)
Comment 8 Jonas Sicking (:sicking) 2011-06-20 09:59:20 PDT
Created attachment 540508 [details] [diff] [review]
Remove from nsXULPopupManager
Comment 9 Jonas Sicking (:sicking) 2011-06-20 10:15:20 PDT
Created attachment 540519 [details] [diff] [review]
Remove from nsFileControlFrame
Comment 10 Jonas Sicking (:sicking) 2011-06-20 10:46:07 PDT
Created attachment 540527 [details] [diff] [review]
Remove from nsXULTooltipListener
Comment 11 Jonas Sicking (:sicking) 2011-06-20 10:52:00 PDT
Created attachment 540531 [details] [diff] [review]
Remove from nsMenuBarListener
Comment 12 Olli Pettay [:smaug] 2011-06-27 11:59:17 PDT
I'll try to review these today or tomorrow. If I don't, please ping me.
Comment 13 Olli Pettay [:smaug] 2011-06-28 05:05:23 PDT
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.
Comment 14 Olli Pettay [:smaug] 2011-06-28 05:31:29 PDT
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;
}
Comment 15 Olli Pettay [:smaug] 2011-06-28 05:37:15 PDT
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;
}
Comment 16 Olli Pettay [:smaug] 2011-06-28 05:40:39 PDT
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.
Comment 17 Olli Pettay [:smaug] 2011-06-28 05:45:58 PDT
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.
Comment 18 Olli Pettay [:smaug] 2011-06-28 05:50:43 PDT
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;
}
Comment 19 Olli Pettay [:smaug] 2011-06-28 05:56:52 PDT
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));
>   }
>
Comment 20 Olli Pettay [:smaug] 2011-06-28 06:03:53 PDT
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.
Comment 21 Jonas Sicking (:sicking) 2011-06-28 10:27:42 PDT
(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)
Comment 24 Jonas Sicking (:sicking) 2011-06-30 14:21:47 PDT
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!!!
Comment 26 Trif Andrei-Alin[:AlinT] 2011-08-24 02:02:52 PDT
After landing this patches was this bug fixed or it has a major dependency on 678708?
Thanks.

Note You need to log in before you can comment on or make changes to this bug.