Closed Bug 703260 Opened 8 years ago Closed 8 years ago

Reduce view usage in event handling

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: enndeakin, Assigned: enndeakin)

References

Details

Attachments

(3 files)

Attached will be a few miscellaneous patches which remove some view usage from event handling / presshell / popup manager.
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
Attachment #575179 - Flags: superreview?(roc)
Attachment #575179 - Flags: review?
Attachment #575179 - Flags: review? → review?(bugs)
Only presshell implements it so just move the methods into nsIPresShell.
Attachment #575181 - Flags: review?(matspal)
Attachment #575179 - Attachment is patch: true
Comment on attachment 575179 [details] [diff] [review]
Patch 1 - remove view from HandleEvent

Nice.

I think you could now get rid of nsWeakView.
Attachment #575179 - Flags: review?(bugs) → review+
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.
Attachment #575179 - Flags: superreview?(roc) → superreview+
Attachment #575180 - Flags: review?(matspal) → review+
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?
Attachment #575181 - Flags: review?(matspal) → review+
(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 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 :)
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.