Last Comment Bug 743975 - View event handling cleanup
: View event handling cleanup
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla17
Assigned To: Neil Deakin
:
Mentors:
Depends on: 783134 783139 783383 783899 784142 787818 801212
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-10 06:08 PDT by Neil Deakin
Modified: 2012-11-15 23:56 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - move theme and window size done events (15.11 KB, patch)
2012-04-10 06:54 PDT, Neil Deakin
no flags Details | Diff | Review
Part 2 - remove unused NS_CREATE and NS_TABCHANGE events (5.99 KB, patch)
2012-04-10 06:55 PDT, Neil Deakin
no flags Details | Diff | Review
Part 3 - remove accessibility events from widget, replace with a nsBaseWidget::GetAccessible method (21.83 KB, patch)
2012-04-10 06:56 PDT, Neil Deakin
no flags Details | Diff | Review
Part 4 - move uistatechanged event (8.54 KB, patch)
2012-04-10 06:57 PDT, Neil Deakin
no flags Details | Diff | Review
Part 5 - remove NS_DESTROY event (11.80 KB, text/plain)
2012-04-10 06:58 PDT, Neil Deakin
no flags Details
Part 6 - use a widget listener interface instead of the remaining events (148.25 KB, patch)
2012-04-10 07:00 PDT, Neil Deakin
no flags Details | Diff | Review
Part 7 - remove all the now unused events (12.77 KB, patch)
2012-04-10 07:01 PDT, Neil Deakin
no flags Details | Diff | Review
Part 8 - remove the event handler argument to widget creation methods (60.50 KB, patch)
2012-04-10 07:03 PDT, Neil Deakin
no flags Details | Diff | Review
Part 9 - remove the view wrapper (26.11 KB, patch)
2012-04-10 07:04 PDT, Neil Deakin
no flags Details | Diff | Review
Part 1 - move theme and window size done events (15.03 KB, patch)
2012-06-27 08:30 PDT, Neil Deakin
bugs: review-
Details | Diff | Review
Part 2 - remove unused NS_CREATE and NS_TABCHANGE events (5.99 KB, patch)
2012-06-27 08:31 PDT, Neil Deakin
bugs: review+
Details | Diff | Review
Part 3 - remove accessibility events from widget, replace with a nsBaseWidget::GetAccessible method (20.24 KB, patch)
2012-06-27 08:32 PDT, Neil Deakin
tbsaunde+mozbugs: review+
Details | Diff | Review
Part 4 - move uistatechanged event (8.54 KB, patch)
2012-06-27 08:33 PDT, Neil Deakin
bugs: review+
Details | Diff | Review
Part 5 - remove NS_DESTROY event (11.91 KB, patch)
2012-06-27 08:33 PDT, Neil Deakin
bugs: review+
Details | Diff | Review
Part 6 - use a widget listener interface instead of the remaining events (157.51 KB, patch)
2012-06-27 08:34 PDT, Neil Deakin
no flags Details | Diff | Review
Part 7 - add getpresshell method to listener (7.88 KB, patch)
2012-06-27 08:35 PDT, Neil Deakin
no flags Details | Diff | Review
Part 8 - remove all the now unused events (12.42 KB, patch)
2012-06-27 08:35 PDT, Neil Deakin
bugs: review+
Details | Diff | Review
Part 9 - remove the event handler argument to widget creation methods (64.05 KB, patch)
2012-06-27 08:36 PDT, Neil Deakin
no flags Details | Diff | Review
Part 10 - remove the view wrapper (26.36 KB, patch)
2012-06-27 08:37 PDT, Neil Deakin
no flags Details | Diff | Review
Part 1.2 - move theme and window size done events (17.07 KB, patch)
2012-07-10 09:42 PDT, Neil Deakin
bugs: review+
Details | Diff | Review
Part 6.2 - use a widget listener interface instead of the remaining events (158.37 KB, patch)
2012-08-01 05:01 PDT, Neil Deakin
tnikkel: review+
Details | Diff | Review
Part 7.2 - add getpresshell method to listener (7.61 KB, patch)
2012-08-01 05:01 PDT, Neil Deakin
tnikkel: review+
Details | Diff | Review
Part 9.2 - remove the event handler argument to widget creation methods (63.25 KB, patch)
2012-08-01 05:03 PDT, Neil Deakin
tnikkel: review+
Details | Diff | Review
Part 10.2 - remove the view wrapper (26.36 KB, patch)
2012-08-01 05:04 PDT, Neil Deakin
tnikkel: review+
Details | Diff | Review
Patch differences (183.80 KB, patch)
2012-08-01 11:22 PDT, Neil Deakin
no flags Details | Diff | Review
Rollup patch of windows changes (40.44 KB, patch)
2012-08-03 05:22 PDT, Neil Deakin
jmathies: review+
netzen: feedback+
Details | Diff | Review
Rollup patch of mac changes (29.66 KB, patch)
2012-08-03 05:26 PDT, Neil Deakin
smichaud: review+
Details | Diff | Review
Rollup patch of gtk changes (24.74 KB, patch)
2012-08-03 05:27 PDT, Neil Deakin
karlt: review+
Details | Diff | Review
Rollup patch of android changes (8.12 KB, patch)
2012-08-03 05:30 PDT, Neil Deakin
blassey.bugs: review+
Details | Diff | Review
Rollup of gonk/puppet widget changes (16.04 KB, patch)
2012-08-03 05:34 PDT, Neil Deakin
cjones.bugs: review-
Details | Diff | Review
Rollup of gonk/puppet widget changes, v2 (15.90 KB, patch)
2012-08-08 09:57 PDT, Neil Deakin
cjones.bugs: review+
Details | Diff | Review
fix DEBUG_smaug (1.29 KB, patch)
2012-08-16 10:45 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Review

Description Neil Deakin 2012-04-10 06:08:31 PDT
This is a follow up to bug 703260, involving a bunch of miscellaneous patches to reduce the patch through the view code for event handling.

A number of things currently done though an event such as painting and window state and position notifications, don't need to be. These patches change this to use more direct calls.

The patches will be coming soon.
Comment 1 Neil Deakin 2012-04-10 06:54:48 PDT
Created attachment 613577 [details] [diff] [review]
Part 1 - move theme and window size done events

This patch moves these events into direct methods in nsBaseWidget.
Comment 2 Neil Deakin 2012-04-10 06:55:27 PDT
Created attachment 613578 [details] [diff] [review]
Part 2 - remove unused NS_CREATE and NS_TABCHANGE events
Comment 3 Neil Deakin 2012-04-10 06:56:34 PDT
Created attachment 613579 [details] [diff] [review]
Part 3 - remove accessibility events from widget, replace with a nsBaseWidget::GetAccessible method
Comment 4 Neil Deakin 2012-04-10 06:57:13 PDT
Created attachment 613580 [details] [diff] [review]
Part 4 - move uistatechanged event
Comment 5 Neil Deakin 2012-04-10 06:58:08 PDT
Created attachment 613581 [details]
Part 5 - remove NS_DESTROY event
Comment 6 Neil Deakin 2012-04-10 07:00:58 PDT
Created attachment 613582 [details] [diff] [review]
Part 6 - use a widget listener interface instead of the remaining events

This patch removes the event handling method from views, webshellwindows and nsWebBrowser and replaces it with an interface with methods that can be called directly. I could have added more methods for the earlier events, but those are only ever called on one of the above and don't rely on any fields defined within those objects.
Comment 7 Neil Deakin 2012-04-10 07:01:41 PDT
Created attachment 613583 [details] [diff] [review]
Part 7 - remove all the now unused events
Comment 8 Neil Deakin 2012-04-10 07:03:17 PDT
Created attachment 613584 [details] [diff] [review]
Part 8 - remove the event handler argument to widget creation methods

The widget listener interface can handle the event callback.
Comment 9 Neil Deakin 2012-04-10 07:04:55 PDT
Created attachment 613586 [details] [diff] [review]
Part 9 - remove the view wrapper

Don't think the reference counting it adds is needed. Removing it doesn't seem to cause any problems so the wrappers can just be removed.
Comment 10 Neil Deakin 2012-04-10 07:08:27 PDT
I'm not completely happy with the results here; I was hoping to eliminate or mostly eliminate the need to go through the view system for event handling.

I'm posting the patches as a work in progress, as I've spent too much time on this.

Note that applying only some of the patches (I think all except part 9) won't compile.
Comment 11 Neil Deakin 2012-06-27 08:30:30 PDT
Created attachment 637129 [details] [diff] [review]
Part 1 - move theme and window size done events
Comment 12 Neil Deakin 2012-06-27 08:31:35 PDT
Created attachment 637130 [details] [diff] [review]
Part 2 - remove unused NS_CREATE and NS_TABCHANGE events
Comment 13 Neil Deakin 2012-06-27 08:32:21 PDT
Created attachment 637131 [details] [diff] [review]
Part 3 - remove accessibility events from widget, replace with a nsBaseWidget::GetAccessible method
Comment 14 Neil Deakin 2012-06-27 08:33:09 PDT
Created attachment 637132 [details] [diff] [review]
Part 4 - move uistatechanged event
Comment 15 Neil Deakin 2012-06-27 08:33:43 PDT
Created attachment 637133 [details] [diff] [review]
Part 5 - remove NS_DESTROY event
Comment 16 Neil Deakin 2012-06-27 08:34:14 PDT
Created attachment 637134 [details] [diff] [review]
Part 6 - use a widget listener interface instead of the remaining events
Comment 17 Neil Deakin 2012-06-27 08:35:18 PDT
Created attachment 637135 [details] [diff] [review]
Part 7 - add getpresshell method to listener
Comment 18 Neil Deakin 2012-06-27 08:35:51 PDT
Created attachment 637136 [details] [diff] [review]
Part 8 - remove all the now unused events
Comment 19 Neil Deakin 2012-06-27 08:36:53 PDT
Created attachment 637138 [details] [diff] [review]
Part 9 - remove the event handler argument to widget creation methods
Comment 20 Neil Deakin 2012-06-27 08:37:53 PDT
Created attachment 637140 [details] [diff] [review]
Part 10 - remove the view wrapper
Comment 21 Neil Deakin 2012-06-27 08:46:54 PDT
Comment on attachment 637134 [details] [diff] [review]
Part 6 - use a widget listener interface instead of the remaining events

Timothy, hopefully you can review some of this. I plan to also attach rollup patches for each platform for review by appropriate peers of the widget/ portions.
Comment 22 Trevor Saunders (:tbsaunde) 2012-06-27 10:58:52 PDT
Comment on attachment 637131 [details] [diff] [review]
Part 3 - remove accessibility events from widget, replace with a nsBaseWidget::GetAccessible method

surkov is away, and I've delt with most of this more than him recently so, stealing review.

>+// XXXndeakin what is all this for? Accessibility should be receiving these
>+// notifications of gtk the same as other platforms.

this stuff was added in bug 215232.  This is specific to gtk because we only fire these events to Atk on linux, they don't exist on other platforms.

>+Accessible*
>+nsBaseWidget::GetAccessible()
>+{
>+  nsCOMPtr<nsIPresShell> presShell = GetPresShell(this, mClientData);

do you actually need to take a ref there?

>+  // Accessible creation might be not safe so use IsSafeToRunScript to
>+  // make sure it's not created at unsafe times.
>+  nsCOMPtr<nsIAccessibilityService> accService =
>+    do_GetService("@mozilla.org/accessibilityService;1");

nit, use services::GetAccessibilityService()
Comment 23 alexander :surkov 2012-06-27 22:20:02 PDT
(In reply to Neil Deakin from comment #13)
> Created attachment 637131 [details] [diff] [review]
> Part 3 - remove accessibility events from widget, replace with a
> nsBaseWidget::GetAccessible method

thank you, that's what I wanted for a long time.
Comment 24 alexander :surkov 2012-06-27 22:21:58 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #22)
> Comment on attachment 637131 [details] [diff] [review]
> Part 3 - remove accessibility events from widget, replace with a
> nsBaseWidget::GetAccessible method
> 
> surkov is away, and I've delt with most of this more than him recently so,
> stealing review.

thanks for doing this btw it sounds that we need a review from peer (roc?)
Comment 25 Trevor Saunders (:tbsaunde) 2012-06-27 22:36:34 PDT
(In reply to alexander :surkov from comment #23)
> (In reply to Neil Deakin from comment #13)
> > Created attachment 637131 [details] [diff] [review]
> > Part 3 - remove accessibility events from widget, replace with a
> > nsBaseWidget::GetAccessible method
> 
> thank you, that's what I wanted for a long time.

agreed.

(In reply to alexander :surkov from comment #24)
> (In reply to Trevor Saunders (:tbsaunde) from comment #22)
> > Comment on attachment 637131 [details] [diff] [review]
> > Part 3 - remove accessibility events from widget, replace with a
> > nsBaseWidget::GetAccessible method
> > 
> > surkov is away, and I've delt with most of this more than him recently so,
> > stealing review.
> 
> thanks for doing this btw it sounds that we need a review from peer (roc?)

see comment 21
Comment 26 Olli Pettay [:smaug] 2012-07-03 14:21:28 PDT
Comment on attachment 637133 [details] [diff] [review]
Part 5 - remove NS_DESTROY event


> void
>+nsBaseWidget::NotifyWindowDestroyed()
>+{
>+  if (!mClientData)
>+    return;
>+
>+  nsCOMPtr<nsIBaseWindow> window(do_QueryInterface(static_cast<nsISupports *>(mClientData)));
>+  if (window)
>+    window->Destroy();
>+}
if (expr) {
  stmt;
}
Comment 27 Olli Pettay [:smaug] 2012-07-03 14:24:19 PDT
Comment on attachment 637136 [details] [diff] [review]
Part 8 - remove all the now unused events

r+ assuming the code dispatching this stuff is removed in some other patch :)
Comment 28 Olli Pettay [:smaug] 2012-07-03 14:54:01 PDT
Comment on attachment 637129 [details] [diff] [review]
Part 1 - move theme and window size done events

This is somewhat ugly. Moving content/ / layout/ logic to widget/

Why do you want to do this?
Comment 29 Neil Deakin 2012-07-03 18:14:09 PDT
(In reply to Olli Pettay [:smaug] from comment #28)
> This is somewhat ugly. Moving content/ / layout/ logic to widget/
> 
> Why do you want to do this?

I'm not sure what I've moved out of layout that you're objecting to here. I moved code from nsPresShell to widget so that notifications are done with direct function calls rather than events, and to remove handling within view code.

Would you rather I add separate methods to nsPresShell to be called by the widgets as needed?
Comment 30 Olli Pettay [:smaug] 2012-07-04 00:11:25 PDT
That could be better. It is odd to call for example any ESM methods in widget level code.
Comment 31 Neil Deakin 2012-07-10 09:42:25 PDT
Created attachment 640639 [details] [diff] [review]
Part 1.2 - move theme and window size done events

Move some methods into PresShell
Comment 32 Olli Pettay [:smaug] 2012-07-10 09:49:20 PDT
Comment on attachment 640639 [details] [diff] [review]
Part 1.2 - move theme and window size done events


>+++ b/layout/base/nsIPresShell.h
Update IID


>+  virtual void WindowDoneSizeMove() = 0;
Somewhat odd method name.
Perhaps SizeMoveDone()? Or WindowSizeMoveDone()?
Comment 33 Timothy Nikkel (:tnikkel) 2012-07-12 13:12:18 PDT
Comment on attachment 637134 [details] [diff] [review]
Part 6 - use a widget listener interface instead of the remaining events

>+class nsIWidgetListener
>+  virtual nsIXULWindow* GetXULWindow() { return nsnull; }
>+  virtual nsIView* GetView() { return nsnull; }

It looks like you use GetXULWindow returning null to mean this is a view. It would be good to formalize this in the interface, with a comment at least.
Comment 34 Timothy Nikkel (:tnikkel) 2012-07-16 12:50:50 PDT
Comment on attachment 637134 [details] [diff] [review]
Part 6 - use a widget listener interface instead of the remaining events

>+nsresult nsViewManager::DispatchEvent(nsGUIEvent *aEvent, nsIView* aView, nsEventStatus* aStatus)
>-        if (aEvent->message == NS_DEACTIVATE) {
>-          // if a window is deactivated, clear the mouse capture regardless
>-          // of what is capturing
>-          nsIPresShell::ClearMouseCapture(nsnull);
>-        }

This got removed, but nothing added to replace it. Is it not needed anymore?
Comment 35 Neil Deakin 2012-07-16 13:03:51 PDT
(In reply to Timothy Nikkel (:tn) from comment #34)
> >+nsresult nsViewManager::DispatchEvent(nsGUIEvent *aEvent, nsIView* aView, nsEventStatus* aStatus)
> >-        if (aEvent->message == NS_DEACTIVATE) {
> >-          // if a window is deactivated, clear the mouse capture regardless
> >-          // of what is capturing
> >-          nsIPresShell::ClearMouseCapture(nsnull);
> >-        }
> 
> This got removed, but nothing added to replace it. Is it not needed anymore?

NS_DEACTIVATE is only called on top level windows, so that code shouldn't ever be called.
Comment 36 Timothy Nikkel (:tnikkel) 2012-07-16 15:08:11 PDT
Comment on attachment 637134 [details] [diff] [review]
Part 6 - use a widget listener interface instead of the remaining events

The widget/android/nsWindow.cpp changes seem to go against the indentation in that file (4 vs 2 spaces).
Comment 37 Timothy Nikkel (:tnikkel) 2012-07-16 19:42:23 PDT
Comment on attachment 637134 [details] [diff] [review]
Part 6 - use a widget listener interface instead of the remaining events

>@@ -2564,32 +2579,32 @@ NSEvent* gLastDragMouseDownEvent = nil;
>-  bool painted;
>+  bool painted = false;
>   {
>     nsBaseWidget::AutoLayerManagerSetup
>       setupLayerManager(mGeckoChild, targetContext, BasicLayerManager::BUFFER_NONE);
>-    painted = mGeckoChild->DispatchWindowEvent(paintEvent);
>+    mGeckoChild->PaintWindow(region);
>   }

So 'painted' is always going to be false?
Comment 38 Timothy Nikkel (:tnikkel) 2012-07-17 08:37:15 PDT
Comment on attachment 637134 [details] [diff] [review]
Part 6 - use a widget listener interface instead of the remaining events

>+  /**
>+   * Called when a window is moved to location (x, y). Returns true if the
>+   * notification was handled.
>+   */
>+  virtual bool WindowMoved(nsIWidget* aWidget, PRInt32 aX, PRInt32 aY) { return false; }
>+
>+  /**
>+   * Called when a window is resized to (width, height). Returns true if the
>+   * notification was handled.
>+   */
>+  virtual bool WindowResized(nsIWidget* aWidget, PRInt32 aWidth, PRInt32 aHeight) { return false; }

We should document what these are exactly. If they are for the inner (client) area, or the outer area. And what they are relative to.
Comment 39 Neil Deakin 2012-07-17 13:06:25 PDT
> >     nsBaseWidget::AutoLayerManagerSetup
> >       setupLayerManager(mGeckoChild, targetContext, BasicLayerManager::BUFFER_NONE);
> >-    painted = mGeckoChild->DispatchWindowEvent(paintEvent);
> >+    mGeckoChild->PaintWindow(region);
> >   }
> 
> So 'painted' is always going to be false?

I'll fix this. painted should be set from the return value here and this is causing the autocomplete results to be blank.
Comment 40 Timothy Nikkel (:tnikkel) 2012-07-18 08:59:40 PDT
Comment on attachment 637134 [details] [diff] [review]
Part 6 - use a widget listener interface instead of the remaining events

>+  /**
>+   * Called when the z-order of the window is changed. Returns true if the
>+   * notification was handled.
>+   */
>+  virtual bool ZLevelChanged(bool aImmediate, nsWindowZ *aPlacement,
>+                             nsIWidget* aRequestBelow, nsIWidget** aActualBelow) { return false; }

This needs more documentation explaining how it is used. Like the meaning of each parameter and the return value.
Comment 41 Timothy Nikkel (:tnikkel) 2012-07-18 10:20:00 PDT
Comment on attachment 637134 [details] [diff] [review]
Part 6 - use a widget listener interface instead of the remaining events

>+nsWebShellWindow::WindowResized(nsIWidget* aWidget, PRInt32 aWidth, PRInt32 aHeight)
>+  return false;
>+}

The old event handler returned status nsEventStatus_eConsumeNoDefault, a direct translation of that would be to return true here?
Comment 42 Timothy Nikkel (:tnikkel) 2012-07-18 10:51:23 PDT
Comment on attachment 637134 [details] [diff] [review]
Part 6 - use a widget listener interface instead of the remaining events

>+nsWebShellWindow::SizeModeChanged(nsSizeMode sizeMode)
>+    if (sizeMode == nsSizeMode_Fullscreen) {
>+      ourWindow->SetFullScreen(true);
>+    }

http://hg.mozilla.org/mozilla-central/rev/1b0294c7b952 added an else to this that didn't get transferred.
Comment 43 Timothy Nikkel (:tnikkel) 2012-07-18 11:03:25 PDT
Comment on attachment 637134 [details] [diff] [review]
Part 6 - use a widget listener interface instead of the remaining events

>-bool nsWindow::DispatchFocus(PRUint32 aEventType)
>-    NPEvent pluginEvent;
>-
>-    switch (aEventType)
>-    {
>-      case NS_ACTIVATE:
>-        pluginEvent.event = WM_SETFOCUS;
>-        break;
>-      case NS_DEACTIVATE:
>-        pluginEvent.event = WM_KILLFOCUS;
>-        break;
>-      case NS_PLUGIN_ACTIVATE:
>-        pluginEvent.event = WM_KILLFOCUS;
>-        break;
>-      default:
>-        break;
>-    }
>-
>-    event.pluginEvent = (void *)&pluginEvent;

This pluginEvent stuff is no longer needed? It wasn't immediately obvious to me that it isn't
Comment 44 Neil Deakin 2012-07-18 11:50:39 PDT
> This pluginEvent stuff is no longer needed? It wasn't immediately obvious to me that it
> isn't

There's only one user of each of those three events. nsWebShellWindow for NS_ACTIVATE and NS_DEACTIVATE and nsObjectFrame for NS_PLUGIN_ACTIVATE. The first two don't use the pluginEvent for anything. I'm not completely sure for NS_PLUGIN_ACTIVATE, so I'll investigate again.
Comment 45 Timothy Nikkel (:tnikkel) 2012-07-18 12:01:26 PDT
Comment on attachment 637134 [details] [diff] [review]
Part 6 - use a widget listener interface instead of the remaining events

On Windows we send the Paint event/notification to mWidgetListener. Isn't this the top level window? Don't we want to send it to the attached view instead?

And for size changes, we used to send those events to both the top level window and the attached view.
Comment 46 Timothy Nikkel (:tnikkel) 2012-07-19 13:44:32 PDT
Comment on attachment 637134 [details] [diff] [review]
Part 6 - use a widget listener interface instead of the remaining events

>+bool nsChildView::PaintWindow(nsIntRegion aRegion)
>+  // If there is no listener, use the parent popup's listener if that exists.
>+  if (!listener && mParentWidget) {
>+    nsWindowType type;
>+    mParentWidget->GetWindowType(type);
>+    if (type == eWindowType_popup) {
>+      widget = mParentWidget;
>+      listener = mParentWidget->GetWidgetListener();
>+    }
>+
>+    if (!listener)
>+      listener = mParentWidget->GetWidgetListener();
>+  }

I don't understand why we want the "if (!listener) listener = mParentWidget->GetWidgetListener();". The rest of the code is how the old code did it. This is the only difference that I can see.
Comment 47 Timothy Nikkel (:tnikkel) 2012-07-19 18:15:10 PDT
Comment on attachment 637138 [details] [diff] [review]
Part 9 - remove the event handler argument to widget creation methods

>+     * aUseAttachedEvents true to send events to the attached widget.
>      * aContext The new device context for the view
>      */
>-    NS_IMETHOD AttachViewToTopLevel(EVENT_CALLBACK aViewEventFunction,
>+    NS_IMETHOD AttachViewToTopLevel(bool aUseAttachedEvents,
>                                     nsDeviceContext *aContext) = 0;

What aUseAttachedEvents means should be explained better in the comments.
Comment 48 Timothy Nikkel (:tnikkel) 2012-07-19 19:49:56 PDT
Comment on attachment 637138 [details] [diff] [review]
Part 9 - remove the event handler argument to widget creation methods

>@@ -1461,32 +1460,36 @@ NS_IMETHODIMP nsChildView::DispatchEvent
>+  nsIWidgetListener* listener = mWidgetListener;
>+
>   nsCOMPtr<nsIWidget> kungFuDeathGrip = do_QueryInterface(mParentWidget ? mParentWidget : this);
>   if (mParentWidget) {
>     nsWindowType type;
>     mParentWidget->GetWindowType(type);
>     if (type == eWindowType_popup) {
>       // use the parent popup's widget if there is no listener
>-      nsIWidgetListener* listener = nsnull;
>       if (event->widget)
>         listener = event->widget->GetWidgetListener();
>       if (!listener)
>         event->widget = mParentWidget;
>     }
>+
>+    if (!listener)
>+      listener = mParentWidget->GetWidgetListener();
>   }

This seems like a significant logic change, is it correct?
Comment 49 Neil Deakin 2012-07-20 07:15:45 PDT
(In reply to Timothy Nikkel (:tn) from comment #45)
> Comment on attachment 637134 [details] [diff] [review]
> Part 6 - use a widget listener interface instead of the remaining events
> 
> On Windows we send the Paint event/notification to mWidgetListener. Isn't
> this the top level window? Don't we want to send it to the attached view
> instead?

If the window is a child window, mWidgetListener should be a view.

> 
> And for size changes, we used to send those events to both the top level
> window and the attached view.

We still do. ViewWrapper::WindowResized and nsWebShellWindow::WindowResized both handle resizes.
Comment 50 Timothy Nikkel (:tnikkel) 2012-07-20 07:52:12 PDT
(In reply to Neil Deakin from comment #49)
> (In reply to Timothy Nikkel (:tn) from comment #45)
> > Comment on attachment 637134 [details] [diff] [review]
> > On Windows we send the Paint event/notification to mWidgetListener. Isn't
> > this the top level window? Don't we want to send it to the attached view
> > instead?
> 
> If the window is a child window, mWidgetListener should be a view.

It's a top level window. Except for plugins and popups we don't have any child windows on Windows. But I see that part 9 fixes this problem.

> > And for size changes, we used to send those events to both the top level
> > window and the attached view.
> 
> We still do. ViewWrapper::WindowResized and nsWebShellWindow::WindowResized
> both handle resizes.

I see later patches fix this too.
Comment 51 Timothy Nikkel (:tnikkel) 2012-07-20 10:04:05 PDT
Ok, that's all the comments I have, with those addressed this looks good.
Comment 52 Neil Deakin 2012-07-20 10:49:25 PDT
(In reply to Neil Deakin from comment #44)
> > This pluginEvent stuff is no longer needed? It wasn't immediately obvious to me that it
> > isn't
> 
> There's only one user of each of those three events. nsWebShellWindow for
> NS_ACTIVATE and NS_DEACTIVATE and nsObjectFrame for NS_PLUGIN_ACTIVATE. The
> first two don't use the pluginEvent for anything. I'm not completely sure
> for NS_PLUGIN_ACTIVATE, so I'll investigate again.

Actually, DispatchFocus is never called with NS_PLUGIN_ACTIVATE so this code isn't used currently.
Comment 53 Neil Deakin 2012-08-01 05:01:07 PDT
Created attachment 647913 [details] [diff] [review]
Part 6.2 - use a widget listener interface instead of the remaining events
Comment 54 Neil Deakin 2012-08-01 05:01:52 PDT
Created attachment 647914 [details] [diff] [review]
Part 7.2 - add getpresshell method to listener
Comment 55 Neil Deakin 2012-08-01 05:03:06 PDT
Created attachment 647915 [details] [diff] [review]
Part 9.2 - remove the event handler argument to widget creation methods
Comment 56 Neil Deakin 2012-08-01 05:04:05 PDT
Created attachment 647916 [details] [diff] [review]
Part 10.2 - remove the view wrapper
Comment 57 Timothy Nikkel (:tnikkel) 2012-08-01 08:23:56 PDT
Is it easy to provide an interdiff?

For the review comments you didn't directly reply to, does that mean you made that change?
Comment 58 Neil Deakin 2012-08-01 11:22:17 PDT
Created attachment 648019 [details] [diff] [review]
Patch differences

> For the review comments you didn't directly reply to, does that mean you made that change?

Yes.
Comment 59 Neil Deakin 2012-08-03 05:22:58 PDT
Created attachment 648672 [details] [diff] [review]
Rollup patch of windows changes
Comment 60 Neil Deakin 2012-08-03 05:26:19 PDT
Created attachment 648674 [details] [diff] [review]
Rollup patch of mac changes
Comment 61 Neil Deakin 2012-08-03 05:27:55 PDT
Created attachment 648675 [details] [diff] [review]
Rollup patch of gtk changes

Linux Try builds are at https://tbpl.mozilla.org/?tree=Try&rev=489a1cc71700
Comment 62 Neil Deakin 2012-08-03 05:30:40 PDT
Created attachment 648677 [details] [diff] [review]
Rollup patch of android changes

I'm only able to test this so far as I don't work on android builds normally. Tests all pass, but I'm hoping someone will run the builds and report on any problems.

Android try builds are at https://tbpl.mozilla.org/?tree=Try&rev=a857b01aa631
Comment 63 Neil Deakin 2012-08-03 05:34:44 PDT
Created attachment 648678 [details] [diff] [review]
Rollup of gonk/puppet widget changes

I've no idea how to test these widgets or what I would be looking for when testing, so hopefully someone can run the builds and report any issues. All tests pass.

Builds are at https://tbpl.mozilla.org/?tree=Try&rev=5464e86b4a4d  See previous comments for linux and android builds.
Comment 64 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-03 15:16:03 PDT
Comment on attachment 648678 [details] [diff] [review]
Rollup of gonk/puppet widget changes

>diff --git a/widget/gonk/nsWindow.cpp b/widget/gonk/nsWindow.cpp

>+            nsIWidgetListener* listener = win->GetWidgetListener();
>+            if (listener) {

if (nsIWidgetListener* listener = win->GetWidgetListener()) {

>@@ -219,77 +217,82 @@ nsWindow::DoDraw(void)

>-    event.region = gWindowToRedraw->mDirtyRegion;
>     gWindowToRedraw->mDirtyRegion.SetEmpty();

Need to save the dirty region before emptying it, for the clip setup
below.

>+        oglm->SetClippingRegion(gWindowToRedraw->mDirtyRegion);

(Note above.)

>+        if (listener)

if (nsIWidgetListener* listener = gWindowToRedraw->GetWidgetListener())

>     } else if (mozilla::layers::LAYERS_BASIC == lm->GetBackendType()) {

>+            gfxUtils::PathFromRegion(ctx, gWindowToRedraw->mDirtyRegion);

(And here.)

>+            nsIWidgetListener* listener = gWindowToRedraw->GetWidgetListener();
>+            if (listener)

if (nsIWidgetListener* listener = gWindowToRedraw->GetWidgetListener())

>+              listener->PaintWindow(gWindowToRedraw, gWindowToRedraw->mDirtyRegion, false, false);

(And here.)

>+            Framebuffer::Present(gWindowToRedraw->mDirtyRegion);

(And here.)

> void
> nsWindow::BringToTop()
> {

>+        nsIWidgetListener* listener = sTopWindows[0]->GetWidgetListener();
>+        if (listener)

if (nsIWidgetListener* listener = sTopWindows[0]->GetWidgetListener()) {

>diff --git a/widget/xpwidgets/PuppetWidget.cpp b/widget/xpwidgets/PuppetWidget.cpp

>+PuppetWidget::Paint()

>   // reset repaint tracking
>   mDirtyRegion.SetEmpty();

Same problem here, resetting dirty region before setting paint/clip
bounds.

>+    debug_DumpPaintEvent(stderr, this, mDirtyRegion,

(Note above.)

I don't believe the clipping/paint-rect issue would cause correctness
problems, but it should cause perf problems that I'm not sure we have
sufficient test infra to catch :/.  So r- for that.
Comment 65 Steven Michaud [:smichaud] (Retired) 2012-08-06 12:59:58 PDT
Comment on attachment 648674 [details] [diff] [review]
Rollup patch of mac changes

Looks fine to me, though I haven't built or tested it.
Comment 66 Brian R. Bondy [:bbondy] 2012-08-07 10:23:18 PDT
Comment on attachment 648672 [details] [diff] [review]
Rollup patch of windows changes

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

This looks reasonable to me; however, I'd like for jimm to take a pass since I don't understand the effect of all the changes.
Comment 67 Jim Mathies [:jimm] 2012-08-07 11:15:56 PDT
Comment on attachment 648672 [details] [diff] [review]
Rollup patch of windows changes

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

woohoo!

::: widget/windows/nsWindow.cpp
@@ +3877,5 @@
>  
>    event.pluginEvent = (void *)&pluginEvent;
>  
>    // call the event callback
> +  if (nullptr != mWidgetListener) {

nit - !mWidgetListener

@@ +3934,5 @@
> +  if (!aIsActivate && BlurEventsSuppressed())
> +    return;
> +
> +  if (!mWidgetListener)
> +    return;    

nit - splinter says there's white space on the end of the return.

@@ -4519,5 @@
>        result = true;
>        break;
>  
> -    case WM_DISPLAYCHANGE:
> -      DispatchStandardEvent(NS_DISPLAYCHANGED);

No longer needed?

@@ +5009,5 @@
>        break;
>  
>      case WM_KILLFOCUS:
>        if (sJustGotDeactivate) {
> +        DispatchFocusToTopLevelWindow(false);

Is this correct? 'result' decides if we fall through to the default window procedure. This would be true if the old dispatch in DispatchFocusToTopLevelWindow returned nsEventStatus_eConsumeNoDefault.

@@ +6935,3 @@
>  
>    // Prevent the widget from sending additional events.
> +  mWidgetListener = nullptr;

Might as well clear mAttachedWidgetListener here as well.
Comment 68 Neil Deakin 2012-08-07 11:47:22 PDT
(In reply to Jim Mathies [:jimm] from comment #67)
> > -    case WM_DISPLAYCHANGE:
> > -      DispatchStandardEvent(NS_DISPLAYCHANGED);
> 
> No longer needed?
> 

The only code that responds to this event does nothing but set the status to nsEventStatus_eConsumeDoDefault, and the return result is ignored here anyway, so sending this event has no purpose.


> >      case WM_KILLFOCUS:
> >        if (sJustGotDeactivate) {
> > +        DispatchFocusToTopLevelWindow(false);
> 
> Is this correct? 'result' decides if we fall through to the default window
> procedure. This would be true if the old dispatch in
> DispatchFocusToTopLevelWindow returned nsEventStatus_eConsumeNoDefault.

If I'm reading the code right, DispatchFocusToTopLevelWindow either returns false or calls an event listener which sets nsEventStatus_eIgnore, which results in DispatchFocusToTopLevelWindow returning false as well, so 'result' will always end up being 'false' which it should already be initialized to.
Comment 69 Karl Tomlinson (ni?:karlt) 2012-08-07 23:31:00 PDT
Comment on attachment 648675 [details] [diff] [review]
Rollup patch of gtk changes

>-    nsGUIEvent event(true, NS_ACTIVATE, this);
>-    nsEventStatus status;
>-    DispatchEvent(&event, status);
>+
>+    if (mWidgetListener)
>+      mWidgetListener->WindowRaised();
> }
> 
> void
> nsWindow::DispatchDeactivateEvent(void)
> {
>-    nsGUIEvent event(true, NS_DEACTIVATE, this);
>-    nsEventStatus status;
>-    DispatchEvent(&event, status);
>+    if (mWidgetListener)
>+      mWidgetListener->WindowLowered();

Can these methods be called WindowActivated/WindowDeactivated
(or WindowFocused/WindowUnfocused)?

Window stacking order is independent of activation, so Raised/Lowered is
misleading.

>-    // Dispatch WILL_PAINT to allow scripts etc. to run before we
>-    // dispatch PAINT
>+    nsIntRegion region;
>+
>+    // Dispatch paint request to allow scripts etc. to run before we paint
>     {
>-        nsEventStatus status;
>-        nsPaintEvent willPaintEvent(true, NS_WILL_PAINT, this);
>-        willPaintEvent.willSendDidPaint = true;
>-        DispatchEvent(&willPaintEvent, status);
>-
>-        // If the window has been destroyed during WILL_PAINT, there is
>+        if (mWidgetListener)
>+          mWidgetListener->WillPaintWindow(this, true);
>+
>+        // If the window has been destroyed during the paint request, there is
>         // nothing left to do.
>         if (!mGdkWindow)
>             return TRUE;
>     }

This is not a "paint request" so "Dispatch WillPaint notification to" or "Dispatch WillPaintWindow to" would be clearer.  Similarly for "during the paint
request".

Please move the region declaration to after this block.

I assume WillPaintWindow's arg should be called aWillSendDidPaint.
A flags argument for aSentWillPaint, aWillSendDidPaint would make this easier
to read.

>-        LOGFOCUS(("NS_ACTIVATE event is blocked [%p]\n", (void *)this));
>+        LOGFOCUS(("activation is blocked [%p]\n", (void *)this));

"Activated notification is blocked" or similar.

>+// XXXndeakin what is all this for? Accessibility should be receiving these
>+// notifications of gtk the same as other platforms.

I don't know the answer, but I guess you mean "notifications from GTK".
Comment 70 Neil Deakin 2012-08-08 09:57:53 PDT
Created attachment 650192 [details] [diff] [review]
Rollup of gonk/puppet widget changes, v2
Comment 71 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-14 23:39:04 PDT
Comment on attachment 650192 [details] [diff] [review]
Rollup of gonk/puppet widget changes, v2

Sorry for the review lag.
Comment 72 Neil Deakin 2012-08-15 11:55:58 PDT
Checked into inbound.

Tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=3f5aa701ae61
Comment 74 Olli Pettay [:smaug] 2012-08-16 10:45:25 PDT
Created attachment 652492 [details] [diff] [review]
fix DEBUG_smaug
Comment 75 Olli Pettay [:smaug] 2012-08-16 11:42:49 PDT
https://hg.mozilla.org/mozilla-central/rev/57e59b2e017e

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