743975 - View event handling cleanup

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Neil Deakin]

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]

Attachment: Part 1 - move theme and window size done events

This patch moves these events into direct methods in nsBaseWidget.

Comment 2

[Neil Deakin]

Attachment: Part 2 - remove unused NS_CREATE and NS_TABCHANGE events

Comment 3

[Neil Deakin]

Attachment: Part 3 - remove accessibility events from widget, replace with a nsBaseWidget::GetAccessible method

Comment 4

[Neil Deakin]

Attachment: Part 4 - move uistatechanged event

Comment 5

[Neil Deakin]

Attachment: Part 5 - remove NS_DESTROY event

Comment 6

[Neil Deakin]

Attachment: 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]

Attachment: Part 7 - remove all the now unused events

Comment 8

[Neil Deakin]

Attachment: Part 8 - remove the event handler argument to widget creation methods

The widget listener interface can handle the event callback.

Comment 9

[Neil Deakin]

Attachment: 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]

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]

Attachment: Part 1 - move theme and window size done events

Comment 12

[Neil Deakin]

Attachment: Part 2 - remove unused NS_CREATE and NS_TABCHANGE events

Comment 13

[Neil Deakin]

Attachment: Part 3 - remove accessibility events from widget, replace with a nsBaseWidget::GetAccessible method

Comment 14

[Neil Deakin]

Attachment: Part 4 - move uistatechanged event

Comment 15

[Neil Deakin]

Attachment: Part 5 - remove NS_DESTROY event

Comment 16

[Neil Deakin]

Attachment: Part 6 - use a widget listener interface instead of the remaining events

Comment 17

[Neil Deakin]

Attachment: Part 7 - add getpresshell method to listener

Comment 18

[Neil Deakin]

Attachment: Part 8 - remove all the now unused events

Comment 19

[Neil Deakin]

Attachment: Part 9 - remove the event handler argument to widget creation methods

Comment 20

[Neil Deakin]

Attachment: Part 10 - remove the view wrapper

Comment 21

[Neil Deakin]

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)]

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 (:asurkov)]

(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 (:asurkov)]

(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)]

(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][[e-mail]]]

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][[e-mail]]]

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][[e-mail]]]

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]

(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][[e-mail]]]

That could be better. It is odd to call for example any ESM methods in widget level code.

Comment 31

[Neil Deakin]

Attachment: Part 1.2 - move theme and window size done events

Move some methods into PresShell

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

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)]

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)]

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]

(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)]

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)]

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)]

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]

> >     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)]

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)]

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)]

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)]

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]

> 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)]

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)]

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)]

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)]

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]

(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)]

(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)]

Ok, that's all the comments I have, with those addressed this looks good.

Comment 52

[Neil Deakin]

(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]

Attachment: Part 6.2 - use a widget listener interface instead of the remaining events

Comment 54

[Neil Deakin]

Attachment: Part 7.2 - add getpresshell method to listener

Comment 55

[Neil Deakin]

Attachment: Part 9.2 - remove the event handler argument to widget creation methods

Comment 56

[Neil Deakin]

Attachment: Part 10.2 - remove the view wrapper

Comment 57

[Timothy Nikkel (:tnikkel)]

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]

Attachment: Patch differences

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

Yes.

Comment 59

[Neil Deakin]

Attachment: Rollup patch of windows changes

Comment 60

[Neil Deakin]

Attachment: Rollup patch of mac changes

Comment 61

[Neil Deakin]

Attachment: Rollup patch of gtk changes

Linux Try builds are at https://tbpl.mozilla.org/?tree=Try&rev=489a1cc71700

Comment 62

[Neil Deakin]

Attachment: 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]

Attachment: 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]

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)]

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]]

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]]

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]

(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 (:karlt)]

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]

Attachment: Rollup of gonk/puppet widget changes, v2

Comment 71

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 650192 [details] [diff] [review]
Rollup of gonk/puppet widget changes, v2

Sorry for the review lag.

Comment 72

[Neil Deakin]

Checked into inbound.

Tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=3f5aa701ae61

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/340f7af0fb8e
https://hg.mozilla.org/mozilla-central/rev/1fdb203e1e04
https://hg.mozilla.org/mozilla-central/rev/92fdc7bd7f5f
https://hg.mozilla.org/mozilla-central/rev/e389f7bcd86d
https://hg.mozilla.org/mozilla-central/rev/481e19da0409
https://hg.mozilla.org/mozilla-central/rev/448410c2035e
https://hg.mozilla.org/mozilla-central/rev/4aca82dc0d41
https://hg.mozilla.org/mozilla-central/rev/54badf5d08b0
https://hg.mozilla.org/mozilla-central/rev/e66c290ab9fc
https://hg.mozilla.org/mozilla-central/rev/dfcc0507902c

Comment 74

[Olli Pettay [:smaug][[e-mail]]]

Attachment: fix DEBUG_smaug

Comment 75

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/57e59b2e017e