Closed Bug 703260 Opened 13 years ago Closed 13 years ago

Reduce view usage in event handling

Categories

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

defect
Not set
normal

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.
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
Closed: 13 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
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: