Closed
Bug 743975
Opened 13 years ago
Closed 12 years ago
View event handling cleanup
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Tracking
()
RESOLVED
FIXED
mozilla17
People
(Reporter: enndeakin, Assigned: enndeakin)
References
Details
Attachments
(17 files, 15 obsolete files)
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•13 years ago
|
||
This patch moves these events into direct methods in nsBaseWidget.
Assignee | ||
Comment 2•13 years ago
|
||
Assignee | ||
Comment 3•13 years ago
|
||
Assignee | ||
Comment 4•13 years ago
|
||
Assignee | ||
Comment 5•13 years ago
|
||
Assignee | ||
Comment 6•13 years ago
|
||
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•13 years ago
|
||
Assignee | ||
Comment 9•13 years ago
|
||
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•13 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•12 years ago
|
||
Assignee: nobody → enndeakin
Attachment #613577 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Assignee | ||
Comment 12•12 years ago
|
||
Assignee | ||
Comment 13•12 years ago
|
||
Attachment #613578 -
Attachment is obsolete: true
Attachment #613579 -
Attachment is obsolete: true
Assignee | ||
Comment 14•12 years ago
|
||
Assignee | ||
Comment 15•12 years ago
|
||
Attachment #613580 -
Attachment is obsolete: true
Attachment #613581 -
Attachment is obsolete: true
Assignee | ||
Comment 17•12 years ago
|
||
Assignee | ||
Comment 18•12 years ago
|
||
Assignee | ||
Comment 19•12 years ago
|
||
Attachment #613583 -
Attachment is obsolete: true
Attachment #613584 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #637129 -
Flags: review?(bugs)
Assignee | ||
Updated•12 years ago
|
Attachment #637130 -
Flags: review?(bugs)
Assignee | ||
Updated•12 years ago
|
Attachment #637131 -
Flags: review?(surkov.alexander)
Assignee | ||
Updated•12 years ago
|
Attachment #637132 -
Flags: review?(bugs)
Assignee | ||
Updated•12 years ago
|
Attachment #637133 -
Flags: review?(bugs)
Assignee | ||
Comment 21•12 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•12 years ago
|
Attachment #637135 -
Flags: review?(tnikkel)
Assignee | ||
Updated•12 years ago
|
Attachment #637136 -
Flags: review?(bugs)
Assignee | ||
Updated•12 years ago
|
Attachment #637138 -
Flags: review?(tnikkel)
Assignee | ||
Updated•12 years ago
|
Attachment #637140 -
Flags: review?(tnikkel)
Comment 22•12 years ago
|
||
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•12 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•12 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?)
Comment 25•12 years ago
|
||
(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•12 years ago
|
Attachment #637130 -
Flags: review?(bugs) → review+
Updated•12 years ago
|
Attachment #637132 -
Flags: review?(bugs) → review+
Comment 26•12 years ago
|
||
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 27•12 years ago
|
||
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 28•12 years ago
|
||
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•12 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?
Comment 30•12 years ago
|
||
That could be better. It is odd to call for example any ESM methods in widget level code.
Assignee | ||
Comment 31•12 years ago
|
||
Move some methods into PresShell
Attachment #637129 -
Attachment is obsolete: true
Attachment #640639 -
Flags: review?(bugs)
Comment 32•12 years ago
|
||
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 33•12 years ago
|
||
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•12 years ago
|
||
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•12 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 36•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 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 40•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 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 45•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 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.
Comment 50•12 years ago
|
||
(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.
Assignee | ||
Comment 52•12 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•12 years ago
|
||
Attachment #637134 -
Attachment is obsolete: true
Attachment #637134 -
Flags: review?(tnikkel)
Attachment #647913 -
Flags: review?(tnikkel)
Assignee | ||
Comment 54•12 years ago
|
||
Attachment #637135 -
Attachment is obsolete: true
Attachment #637135 -
Flags: review?(tnikkel)
Attachment #647914 -
Flags: review?(tnikkel)
Assignee | ||
Comment 56•12 years ago
|
||
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)
Comment 57•12 years ago
|
||
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•12 years ago
|
||
> For the review comments you didn't directly reply to, does that mean you made that change?
Yes.
Assignee | ||
Comment 61•12 years ago
|
||
Linux Try builds are at https://tbpl.mozilla.org/?tree=Try&rev=489a1cc71700
Attachment #648675 -
Flags: review?(karlt)
Assignee | ||
Comment 62•12 years ago
|
||
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•12 years ago
|
||
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 65•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #648677 -
Flags: review?(blassey.bugs) → review+
Comment 66•12 years ago
|
||
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•12 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•12 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 69•12 years ago
|
||
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•12 years ago
|
||
Attachment #648678 -
Attachment is obsolete: true
Attachment #650192 -
Flags: review?(jones.chris.g)
Updated•12 years ago
|
Attachment #647913 -
Flags: review?(tnikkel) → review+
Updated•12 years ago
|
Attachment #647914 -
Flags: review?(tnikkel) → review+
Updated•12 years ago
|
Attachment #647915 -
Flags: review?(tnikkel) → review+
Updated•12 years ago
|
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•12 years ago
|
||
Checked into inbound. Tryserver run: https://tbpl.mozilla.org/?tree=Try&rev=3f5aa701ae61
Comment 73•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment 74•12 years ago
|
||
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•