The default bug view has changed. See this FAQ.

View event handling cleanup

RESOLVED FIXED in mozilla17

Status

()

Core
Event Handling
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Neil Deakin, Assigned: Neil Deakin)

Tracking

Trunk
mozilla17
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(17 attachments, 15 obsolete attachments)

5.99 KB, patch
smaug
: review+
Details | Diff | Splinter Review
20.24 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
8.54 KB, patch
smaug
: review+
Details | Diff | Splinter Review
11.91 KB, patch
smaug
: review+
Details | Diff | Splinter Review
12.42 KB, patch
smaug
: review+
Details | Diff | Splinter Review
17.07 KB, patch
smaug
: review+
Details | Diff | Splinter Review
158.37 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
7.61 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
63.25 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
26.36 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
183.80 KB, patch
Details | Diff | Splinter Review
40.44 KB, patch
jimm
: review+
bbondy
: feedback+
Details | Diff | Splinter Review
29.66 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
24.74 KB, patch
karlt
: review+
Details | Diff | Splinter Review
8.12 KB, patch
blassey
: review+
Details | Diff | Splinter Review
15.90 KB, patch
cjones
: review+
Details | Diff | Splinter Review
1.29 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 613578 [details] [diff] [review]
Part 2 - remove unused NS_CREATE and NS_TABCHANGE events
(Assignee)

Comment 3

5 years ago
Created attachment 613579 [details] [diff] [review]
Part 3 - remove accessibility events from widget, replace with a nsBaseWidget::GetAccessible method
(Assignee)

Comment 4

5 years ago
Created attachment 613580 [details] [diff] [review]
Part 4 - move uistatechanged event
(Assignee)

Comment 5

5 years ago
Created attachment 613581 [details]
Part 5 - remove NS_DESTROY event
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
Created attachment 613583 [details] [diff] [review]
Part 7 - remove all the now unused events
(Assignee)

Comment 8

5 years ago
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.
(Assignee)

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
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.
(Assignee)

Comment 11

5 years ago
Created attachment 637129 [details] [diff] [review]
Part 1 - move theme and window size done events
Assignee: nobody → enndeakin
Attachment #613577 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 12

5 years ago
Created attachment 637130 [details] [diff] [review]
Part 2 - remove unused NS_CREATE and NS_TABCHANGE events
(Assignee)

Comment 13

5 years ago
Created attachment 637131 [details] [diff] [review]
Part 3 - remove accessibility events from widget, replace with a nsBaseWidget::GetAccessible method
Attachment #613578 - Attachment is obsolete: true
Attachment #613579 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
Created attachment 637132 [details] [diff] [review]
Part 4 - move uistatechanged event
(Assignee)

Comment 15

5 years ago
Created attachment 637133 [details] [diff] [review]
Part 5 - remove NS_DESTROY event
Attachment #613580 - Attachment is obsolete: true
Attachment #613581 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
Created attachment 637134 [details] [diff] [review]
Part 6 - use a widget listener interface instead of the remaining events
Attachment #613582 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
Created attachment 637135 [details] [diff] [review]
Part 7 - add getpresshell method to listener
(Assignee)

Comment 18

5 years ago
Created attachment 637136 [details] [diff] [review]
Part 8 - remove all the now unused events
(Assignee)

Comment 19

5 years ago
Created attachment 637138 [details] [diff] [review]
Part 9 - remove the event handler argument to widget creation methods
Attachment #613583 - Attachment is obsolete: true
Attachment #613584 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Created attachment 637140 [details] [diff] [review]
Part 10 - remove the view wrapper
Attachment #613586 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #637129 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #637130 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #637131 - Flags: review?(surkov.alexander)
(Assignee)

Updated

5 years ago
Attachment #637132 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #637133 - Flags: review?(bugs)
(Assignee)

Comment 21

5 years ago
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.
Attachment #637134 - Flags: review?(tnikkel)
(Assignee)

Updated

5 years ago
Attachment #637135 - Flags: review?(tnikkel)
(Assignee)

Updated

5 years ago
Attachment #637136 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #637138 - Flags: review?(tnikkel)
(Assignee)

Updated

5 years ago
Attachment #637140 - Flags: review?(tnikkel)
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()
Attachment #637131 - Flags: review?(surkov.alexander) → review+

Comment 23

5 years ago
(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

5 years ago
(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?)
(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

Updated

5 years ago
Attachment #637130 - Flags: review?(bugs) → review+

Updated

5 years ago
Attachment #637132 - Flags: review?(bugs) → review+
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;
}
Attachment #637133 - Flags: review?(bugs) → review+
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 :)
Attachment #637136 - Flags: review?(bugs) → review+
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?
Attachment #637129 - Flags: review?(bugs) → review-
(Assignee)

Comment 29

5 years ago
(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?
That could be better. It is odd to call for example any ESM methods in widget level code.
(Assignee)

Comment 31

5 years ago
Created attachment 640639 [details] [diff] [review]
Part 1.2 - move theme and window size done events

Move some methods into PresShell
Attachment #637129 - Attachment is obsolete: true
Attachment #640639 - Flags: review?(bugs)
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()?
Attachment #640639 - Flags: review?(bugs) → review+
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 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?
(Assignee)

Comment 35

5 years ago
(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 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 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 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.
(Assignee)

Comment 39

5 years ago
> >     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 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 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 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 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
(Assignee)

Comment 44

5 years ago
> 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 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 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 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 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?
(Assignee)

Comment 49

5 years ago
(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.
(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.
Ok, that's all the comments I have, with those addressed this looks good.
(Assignee)

Comment 52

5 years ago
(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.
(Assignee)

Comment 53

5 years ago
Created attachment 647913 [details] [diff] [review]
Part 6.2 - use a widget listener interface instead of the remaining events
Attachment #637134 - Attachment is obsolete: true
Attachment #637134 - Flags: review?(tnikkel)
Attachment #647913 - Flags: review?(tnikkel)
(Assignee)

Comment 54

5 years ago
Created attachment 647914 [details] [diff] [review]
Part 7.2 - add getpresshell method to listener
Attachment #637135 - Attachment is obsolete: true
Attachment #637135 - Flags: review?(tnikkel)
Attachment #647914 - Flags: review?(tnikkel)
(Assignee)

Comment 55

5 years ago
Created attachment 647915 [details] [diff] [review]
Part 9.2 - remove the event handler argument to widget creation methods
Attachment #647915 - Flags: review?(tnikkel)
(Assignee)

Comment 56

5 years ago
Created attachment 647916 [details] [diff] [review]
Part 10.2 - remove the view wrapper
Attachment #637138 - Attachment is obsolete: true
Attachment #637140 - Attachment is obsolete: true
Attachment #637138 - Flags: review?(tnikkel)
Attachment #637140 - Flags: review?(tnikkel)
Attachment #647916 - Flags: review?(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?
(Assignee)

Comment 58

5 years ago
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.
(Assignee)

Comment 59

5 years ago
Created attachment 648672 [details] [diff] [review]
Rollup patch of windows changes
Attachment #648672 - Flags: review?(netzen)
(Assignee)

Comment 60

5 years ago
Created attachment 648674 [details] [diff] [review]
Rollup patch of mac changes
Attachment #648674 - Flags: review?(smichaud)
(Assignee)

Comment 61

5 years ago
Created attachment 648675 [details] [diff] [review]
Rollup patch of gtk changes

Linux Try builds are at https://tbpl.mozilla.org/?tree=Try&rev=489a1cc71700
Attachment #648675 - Flags: review?(karlt)
(Assignee)

Comment 62

5 years ago
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
Attachment #648677 - Flags: review?(blassey.bugs)
(Assignee)

Comment 63

5 years ago
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.
Attachment #648678 - Flags: review?(jones.chris.g)
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.
Attachment #648678 - Flags: review?(jones.chris.g) → review-
Comment on attachment 648674 [details] [diff] [review]
Rollup patch of mac changes

Looks fine to me, though I haven't built or tested it.
Attachment #648674 - Flags: review?(smichaud) → review+
Attachment #648677 - Flags: review?(blassey.bugs) → review+
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.
Attachment #648672 - Flags: review?(netzen)
Attachment #648672 - Flags: review?(jmathies)
Attachment #648672 - Flags: feedback+

Comment 67

5 years ago
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.
Attachment #648672 - Flags: review?(jmathies) → review+
(Assignee)

Comment 68

5 years ago
(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 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".
Attachment #648675 - Flags: review?(karlt) → review+
(Assignee)

Comment 70

5 years ago
Created attachment 650192 [details] [diff] [review]
Rollup of gonk/puppet widget changes, v2
Attachment #648678 - Attachment is obsolete: true
Attachment #650192 - Flags: review?(jones.chris.g)
Attachment #647913 - Flags: review?(tnikkel) → review+
Attachment #647914 - Flags: review?(tnikkel) → review+
Attachment #647915 - Flags: review?(tnikkel) → review+
Attachment #647916 - Flags: review?(tnikkel) → review+
Comment on attachment 650192 [details] [diff] [review]
Rollup of gonk/puppet widget changes, v2

Sorry for the review lag.
Attachment #650192 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 72

5 years ago
Checked into inbound.

Tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=3f5aa701ae61
Depends on: 783134
Depends on: 783139
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Created attachment 652492 [details] [diff] [review]
fix DEBUG_smaug
https://hg.mozilla.org/mozilla-central/rev/57e59b2e017e

Updated

5 years ago
Depends on: 783383
(Assignee)

Updated

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