Last Comment Bug 703260 - Reduce view usage in event handling
: Reduce view usage in event handling
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Neil Deakin (away until Oct 3)
:
:
Mentors:
Depends on:
Blocks: 740515
  Show dependency treegraph
 
Reported: 2011-11-17 06:40 PST by Neil Deakin (away until Oct 3)
Modified: 2012-03-29 11:25 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1 - remove view from HandleEvent (46.68 KB, patch)
2011-11-17 06:57 PST, Neil Deakin (away until Oct 3)
bugs: review+
roc: superreview+
Details | Diff | Splinter Review
Patch 2 - remove view usage from popup callbacks in popup manager (9.12 KB, patch)
2011-11-17 07:02 PST, Neil Deakin (away until Oct 3)
mats: review+
Details | Diff | Splinter Review
Patch 3 - remove nsIViewObserver (46.34 KB, patch)
2011-11-17 07:06 PST, Neil Deakin (away until Oct 3)
mats: review+
Details | Diff | Splinter Review

Description Neil Deakin (away until Oct 3) 2011-11-17 06:40:05 PST
Attached will be a few miscellaneous patches which remove some view usage from event handling / presshell / popup manager.
Comment 1 Neil Deakin (away until Oct 3) 2011-11-17 06:57:54 PST
Created attachment 575179 [details] [diff] [review]
Patch 1 - remove view from HandleEvent

This patch:
 - nsEventStateManager methods don't use the passed in view
 - change GetClientData to GetFrame as it only ever gets set to a frame.
 - pass the view's frame to nsPresShell::HandleEvent instead
 
Could break these out into separate patches if desired.

This passes all the tests, but am asking for sr because I'm not sure if I've missed some special event case that wants a view
Comment 2 Neil Deakin (away until Oct 3) 2011-11-17 07:02:24 PST
Created attachment 575180 [details] [diff] [review]
Patch 2 - remove view usage from popup callbacks in popup manager
Comment 3 Neil Deakin (away until Oct 3) 2011-11-17 07:06:01 PST
Created attachment 575181 [details] [diff] [review]
Patch 3 - remove nsIViewObserver

Only presshell implements it so just move the methods into nsIPresShell.
Comment 4 Olli Pettay [:smaug] 2011-11-17 10:43:21 PST
Comment on attachment 575179 [details] [diff] [review]
Patch 1 - remove view from HandleEvent

Nice.

I think you could now get rid of nsWeakView.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-17 16:52:25 PST
Comment on attachment 575179 [details] [diff] [review]
Patch 1 - remove view from HandleEvent

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

::: view/public/nsIView.h
@@ +251,5 @@
>      mNextSibling = reinterpret_cast<nsView*>(aSibling);
>    }
>  
>    /**
> +   * Set the view's root frame.

It might not be a root frame, so fix the comment

@@ +258,3 @@
>  
>    /**
> +   * Retrieve the view's root frame.

Ditto

::: view/public/nsIViewObserver.h
@@ +95,5 @@
>     * @param aHandled - whether the correct frame was found to
>     *                   handle the event
>     * @return error status
>     */
> +  NS_IMETHOD HandleEvent(nsIFrame*      aRootFrame,

Again, not necessarily a root frame.
Comment 6 Mats Palmgren (:mats) 2011-11-17 17:06:50 PST
Comment on attachment 575181 [details] [diff] [review]
Patch 3 - remove nsIViewObserver

In view/src/nsViewManager.cpp

>     case NS_SYSCOLORCHANGED:
>       {
>         // Hold a refcount to the observer. The continued existence of the observer will
>         // delay deletion of this view hierarchy should the event want to cause its
>         // destruction in, say, some JavaScript event handler.
>-        nsCOMPtr<nsIViewObserver> obs = GetViewObserver();
>-        if (obs) {
>-          obs->HandleEvent(aView->GetFrame(), aEvent, false, aStatus);
>+        if (mPresShell) {
>+          mPresShell->HandleEvent(aView->GetFrame(), aEvent, false, aStatus);

This needs to hold a strong ref on mPresShell before calling HandleEvent.
And s/observer/pres shell/ in the comment?
Comment 7 Neil Deakin (away until Oct 3) 2011-11-18 12:15:21 PST
(In reply to Mats Palmgren [:mats] from comment #6)

> This needs to hold a strong ref on mPresShell before calling HandleEvent.
> And s/observer/pres shell/ in the comment?

I'll change this, but the presshell/prescontext just fires off a nsIRunnable so it shouldn't matter.
Comment 9 :Ms2ger (⌚ UTC+1/+2) 2011-11-21 23:42:27 PST
Comment on attachment 575181 [details] [diff] [review]
Patch 3 - remove nsIViewObserver

>--- a/layout/base/nsPresShell.h
>+++ b/layout/base/nsPresShell.h
>@@ -309,40 +308,35 @@ public:
>   virtual void SetIgnoreViewportScrolling(bool aIgnore);
> 
>   virtual void SetDisplayPort(const nsRect& aDisplayPort);
> 
>   virtual nsresult SetResolution(float aXResolution, float aYResolution);
> 
>   //nsIViewObserver interface

Missed one :)

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