655267 - move the synth mouse move handling from the view manager to the pres shell

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Comment 1

[Timothy Nikkel (:tnikkel)]

Attachment: patch

Comment 2

[Timothy Nikkel (:tnikkel)]

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)]

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)]

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)]

Attachment: patch v2

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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)]

Comment on attachment 531071 [details] [diff] [review]
patch v2

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

Comment 8

[Timothy Nikkel (:tnikkel)]

(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)]

Ok, sorry.

Hopefully you can share code with HandleEvent, though.

Comment 10

[Timothy Nikkel (:tnikkel)]

(In reply to comment #9)
> Hopefully you can share code with HandleEvent, though.

Yes.

Comment 11

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/916987d88134