Reduce view usage in event handling

RESOLVED FIXED in mozilla11

Status

()

Core
Event Handling
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Neil Deakin (not available until Aug 9), Assigned: Neil Deakin (not available until Aug 9))

Tracking

Trunk
mozilla11
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

Attached will be a few miscellaneous patches which remove some view usage from event handling / presshell / popup manager.
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
Attachment #575179 - Flags: superreview?(roc)
Attachment #575179 - Flags: review?
Attachment #575179 - Flags: review? → review?(bugs)
Created attachment 575180 [details] [diff] [review]
Patch 2 - remove view usage from popup callbacks in popup manager
Attachment #575180 - Flags: review?(matspal)
Created attachment 575181 [details] [diff] [review]
Patch 3 - remove nsIViewObserver

Only presshell implements it so just move the methods into nsIPresShell.
Attachment #575181 - Flags: review?(matspal)

Updated

6 years ago
Attachment #575179 - Attachment is patch: true

Comment 4

6 years ago
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+

Updated

6 years ago
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 8

6 years ago
https://hg.mozilla.org/mozilla-central/rev/9298bebc43bf
https://hg.mozilla.org/mozilla-central/rev/e4a32b4ffd93
https://hg.mozilla.org/mozilla-central/rev/7e2695c9d94e
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
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 :)
Blocks: 740515
You need to log in before you can comment on or make changes to this bug.