Last Comment Bug 655267 - move the synth mouse move handling from the view manager to the pres shell
: move the synth mouse move handling from the view manager to the pres shell
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: View Rendering (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Timothy Nikkel (:tnikkel)
:
: Jet Villegas (:jet)
Mentors:
Depends on: 679356
Blocks: 655263
  Show dependency treegraph
 
Reported: 2011-05-06 08:45 PDT by Timothy Nikkel (:tnikkel)
Modified: 2011-08-16 08:07 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (42.71 KB, patch)
2011-05-06 08:46 PDT, Timothy Nikkel (:tnikkel)
no flags Details | Diff | Splinter Review
patch v2 (43.01 KB, patch)
2011-05-09 09:56 PDT, Timothy Nikkel (:tnikkel)
roc: review+
Details | Diff | Splinter Review

Description Timothy Nikkel (:tnikkel) 2011-05-06 08:45:47 PDT

    
Comment 1 Timothy Nikkel (:tnikkel) 2011-05-06 08:46:41 PDT
Created attachment 530641 [details] [diff] [review]
patch
Comment 2 Timothy Nikkel (:tnikkel) 2011-05-06 14:36:31 PDT
This also adds a check for activeness of the document when searching for views that the mouse is over which will be needed when hidden tabs aren't in the hidden view they get from decks.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-08 23:23:16 PDT
Comment on attachment 530641 [details] [diff] [review]
patch

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

All that Find...View code is ugly! Can we just use the frame tree instead? Note the code in PresShell::HandleEvent that uses nsXULPopupManager to find the popup containing a point.

::: layout/base/nsPresShell.cpp
@@ +6091,5 @@
> +{
> +  // allow new event to be posted while handling this one only if the
> +  // source of the event is a scroll (to prevent infinite reflow loops)
> +  if (aFromScroll)
> +    mSynthMouseMoveEvent.Forget();

{}

@@ +6465,5 @@
> +    nsPresContext* rootPresContext = mPresContext->GetRootPresContext();
> +    if (rootPresContext) {
> +      PresShell* rootPresShell =
> +        static_cast<PresShell*>(rootPresContext->PresShell());
> +      if (rootPresShell) {

Combine this code with the previous occurrence using a helper method GetRootPresShell()?
Comment 4 Timothy Nikkel (:tnikkel) 2011-05-09 09:06:20 PDT
I couldn't figure out how to use Splinter to reply.

(In reply to comment #3)
> All that Find...View code is ugly! Can we just use the frame tree instead?
> Note the code in PresShell::HandleEvent that uses nsXULPopupManager to find
> the popup containing a point.

The only important thing about finding the right view is that we call DispatchSynthMouseMove on the right presshell (see bug 302561 comment 32). So in the future I plan to make this code iterate any popups (both comboboxes and XUL popups) and then iterate over subdocuments to see where to dispatch the event. I didn't want to make this code traverse the entire cross doc frame tree. I just wanted to straight forwardly move this code from the view manager to the pres shell. This moves us in the direction we want to go, doesn't make any code worse, and limits risk.

> ::: layout/base/nsPresShell.cpp
> @@ +6091,5 @@
> > +{
> > +  // allow new event to be posted while handling this one only if the
> > +  // source of the event is a scroll (to prevent infinite reflow loops)
> > +  if (aFromScroll)
> > +    mSynthMouseMoveEvent.Forget();
> 
> {}

Ok.

> @@ +6465,5 @@
> > +    nsPresContext* rootPresContext = mPresContext->GetRootPresContext();
> > +    if (rootPresContext) {
> > +      PresShell* rootPresShell =
> > +        static_cast<PresShell*>(rootPresContext->PresShell());
> > +      if (rootPresShell) {
> 
> Combine this code with the previous occurrence using a helper method
> GetRootPresShell()?

Done.
Comment 5 Timothy Nikkel (:tnikkel) 2011-05-09 09:56:10 PDT
Created attachment 531071 [details] [diff] [review]
patch v2
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-09 17:40:35 PDT
(In reply to comment #4)
> The only important thing about finding the right view is that we call
> DispatchSynthMouseMove on the right presshell (see bug 302561 comment 32).
> So in the future I plan to make this code iterate any popups (both
> comboboxes and XUL popups) and then iterate over subdocuments to see where
> to dispatch the event.

I think PresShell::HandleEvent already does both of those for you --- almost. The code to locate a popup and redirect events to it is under "if (framePresContext == rootPresContext && frame == FrameManager()->GetRootFrame())". The code to redirect to the correct subdocument is around "// Handle the event in the correct shell."

The one thing that I think is missing is that nsLayoutUtils::GetPopupFrameForEventCoordinates doesn't know about nsComboboxControlFrames because they don't interact with nsXULPopupManager. So something would need to be fixed there.

> I didn't want to make this code traverse the entire
> cross doc frame tree. I just wanted to straight forwardly move this code
> from the view manager to the pres shell. This moves us in the direction we
> want to go, doesn't make any code worse, and limits risk.

Yes, fair enough.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-09 17:41:15 PDT
Comment on attachment 531071 [details] [diff] [review]
patch v2

Review of attachment 531071 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 8 Timothy Nikkel (:tnikkel) 2011-05-09 21:25:16 PDT
(In reply to comment #6)
> (In reply to comment #4)
> > The only important thing about finding the right view is that we call
> > DispatchSynthMouseMove on the right presshell (see bug 302561 comment 32).
> > So in the future I plan to make this code iterate any popups (both
> > comboboxes and XUL popups) and then iterate over subdocuments to see where
> > to dispatch the event.
> 
> I think PresShell::HandleEvent already does both of those for you ---
> almost. The code to locate a popup and redirect events to it is under "if
> (framePresContext == rootPresContext && frame ==
> FrameManager()->GetRootFrame())". The code to redirect to the correct
> subdocument is around "// Handle the event in the correct shell."

The things that I think are important to happen in the correct presshell are the things in PresShell::DispatchSynthMouseMove, specifically the hover generation and flush on hover generation change. Of course PresShell::HandleEvent on the root document can handle the mouse event, it handles all our mouse events!

> The one thing that I think is missing is that
> nsLayoutUtils::GetPopupFrameForEventCoordinates doesn't know about
> nsComboboxControlFrames because they don't interact with nsXULPopupManager.
> So something would need to be fixed there.

Yes, I didn't make this clear enough in my previous comment.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-09 21:37:33 PDT
Ok, sorry.

Hopefully you can share code with HandleEvent, though.
Comment 10 Timothy Nikkel (:tnikkel) 2011-05-09 21:49:49 PDT
(In reply to comment #9)
> Hopefully you can share code with HandleEvent, though.

Yes.
Comment 11 Timothy Nikkel (:tnikkel) 2011-05-11 12:27:41 PDT
http://hg.mozilla.org/mozilla-central/rev/916987d88134

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