Closed Bug 1466208 Opened 5 years ago Closed 4 years ago

Cleanup and fix bugs in PresShell::HandleEvent

Categories

(Core :: DOM: UI Events & Focus Handling, enhancement, P2)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: masayuki)

References

Details

Attachments

(46 files)

Bug 1466208 - part 1: Create stack class to handle events in PresShell
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 2: Create PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret() to handle event with AccessbleCaretEventHub
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 3: Rewrite PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret() with early-return style
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 4: Create PresShell::EventHandler::GetCapturingContentFor() to retrieve capturing content for specific event
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 5: Create PresShell::EventHandler::GetRetargetEventDocument() retrieve retarget document of coming event
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 6: Clean up PresShell::EventHandler::GetRetargetEventDocument()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 7: Create PresShell::EventHandler::GetFrameForHandlingEventWith() to retrieve a frame which is necessary to handle event with another PresShell instance
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 8: Clean up PresShell::EventHandler::GetFrameForHandlingEventWith()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 9: Create PresShell::EventHandler::MaybeHandleEventWithAnotherPresShell() to handle event with another PresShell if necessary
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 10: Create PresShell::EventHandler::MaybeDiscardEvent() to check whether it's safe to handle the event
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 11: Create PresShell::EventHandler::MaybeDiscardOrDelayKeyboardEvent()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 12: Get rid of `frame` variable from PresShell::EventHandler::HandleEvent()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 13: Create PresShell::EventHandler::MaybeFlushThrottledStyles()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 14: Create PresShell::EventHandler::ComputeRootFrameToHandleEvent()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 15: Split PresShell::EventHandler::ComputeRootFrameToHandleEvent()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 16: Clean up PresShell::EventHandler::ComputeRootFrameToHandleEvent() and its helper methods with early-return style
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 17: Make PresShell::EventHandler::HandleEventWithPointerCapturingContentWithoutItsFrame() to handle event when there is pointer capturing content but it does not have primary frame
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 18: Create PresShell::EventHandler::MaybeDiscardOrDelayMouseEvent()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 19: Group PresShell, nsIFrame and nsIContent with a struct
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 20: Create PresShell::EventHandler::EventTargetData::MaybeRetargetToActiveDocument()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 21: Clean up PresShell::EventHandler::EventTargetData::MaybeRetargetToActiveDocument() with early-return style
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 22: Create PresShell::EventHandler::EventTargetData::ComputeElementFromFrame()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 23: Create PresShell::EventHandler::ComputeEventTargetFrameAndPresShellAtEventPoint()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 24: Move `overrideClickTarget` into EventTargetData
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 25: Create PresShell::EventHandler::DispatchPrecedingPointerEvent()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 26: Create PresShell::EventHandler::EventTargetData::UpdateTouchEventTarget()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 27: Create PresShell::EventHandler::HandleEventUsingCoordinates()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 28: Make PresShell::EventHandler::HandleEvent() handle non-using-coordinates events without frame before with frame case
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 29: Create PresShell::EventHandler::ComputeFocusedEventTargetElement()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 31: Create a PresShell::EventHandler::MaybeHandleEventWithAnotherPresShell() overload
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 32: Create PresShell::EventHandler::HandleEventAtFocusedContent() and PresShell::EventHandler::HandleEventWithFrameForPresShell()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 30: Create PresShell::EventHandler::AutoCurrentEventInfoSetter class
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 33: Create PresShell::EventHandler::RecordEventHandlingResponsePerformance()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 34: Create a helper class, PresShell::EventHandler::HandlingTimeAccumulator()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 35: Reduce one indent level in PresShell::EventHandler::HandleEventInternal()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 36: Create PresShell::EventHandler::PrepareToDispatchContextMenuEvent()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 37: Move trusted eMouseMove event preparation into the previous switch-case block
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 38: Create PresShell::EventHandler::PrepareToDispatchContextMenuEvent()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 39: Create PresShell::EventHandler::MaybeHandleKeyboardEventBeforeDispatch()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 40: Create PresShell::EventHandler::PrepareToDispatchEvent()
47 bytes, text/x-phabricator-request
Details | Review
Part 35 with ignoring whitespace changes
6.27 KB, patch
Details | Diff | Splinter Review
Bug 1466208 - part 41: Create PresShell::EventHandler::DispatchEvent()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 42: Clean up PresShell::EventHandler::DispatchEvent() with using early-return style
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 43: Create PresShell::EventHandler::FinalizeHandlingEvent()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 44: Rename PresShell::EventHandler::HandleEventInternal() to HandleEventWithCurrentEventInfo()
47 bytes, text/x-phabricator-request
Details | Review
Bug 1466208 - part 45: Rename `aFrame` of `HandleEvent()` to `aFrameForPresShell`
47 bytes, text/x-phabricator-request
Details | Review 
Follow-up from bug 1430813 comment #21:
> PresShell::HandleEvent is a horrible mess.
> Here we might reassign 'frame' to the root frame of some shell ancestor:
> https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/layout/base/PresShell.cpp#6911
> but the rest of HandleEvent quite happily continues to use 'this'
> to access members etc.
> 
> The IME block at the end for example makes the assumption that
> 'frame' belongs to 'this':
> https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/layout/base/PresShell.cpp#7268
> It even assigns mCurrentEventFrame = frame which is clearly
> a recipe for disaster (exploitable UAFs).

Although the code lives under layout/ it seems this code is mostly doing
event handling / IME, so I'm assuming DOM Events is appropriate.
Component: DOM: Events → Event Handling
This bug is scary.  Can we find an owner for it please?
Flags: needinfo?(overholt)
(In reply to Mats Palmgren (:mats) from comment #1)
> This bug is scary.  Can we find an owner for it please?

I'd like to work on this when I have much time. Do you think when should we fix this by?
(FYI: I'm working on redesigning editor for InputEvent.inputType and beforeinput event. Therefore, I don't have much time at least for a couple of months.)
(For comment 2. I agree that Masayuki would be great for this, assuming timelines align.)
Flags: needinfo?(overholt) → needinfo?(mats)
Sure, sounds good.
Flags: needinfo?(mats)

I think that it is really risky method to change. So, we should fix this bug slowly for detecting regressions.

Assignee: nobody → masayuki
Status: NEW → ASSIGNED
status-firefox62: affected → ---
Keywords: leave-open

Well, I was thinking that the best solution is to split each block in HandleEvent() and HandleEventInternal() to methods of PresShell. However, this approach makes too many small methods into PresShell. And badly, some of them may be designed only for specific situations and need to take caller's local variables.

PresShell is a class not only for event handling. So, for grouping event handling helpers, I think that we should create a nexted stack based class called EventHandler and HandleEvent() should use it. Then, methods can share some variables with members.

I think I like that approach. It could lead to quite elegant code.

PresShell::HandleEvent() and PresShell::HandleEventInternal() are too big.
Additionally, we have a lot of methods used only by them. So, if we'll
split those big methods, PresShell will have a lot of small methods which
are not grouped as a part of event handling. That's too bad because some
of them may depend on the calling order, etc.

So, for grouping them, PresShell should create a stack class instance to handle
each event. Then, we can store shared information in it only while we're
handling an event.

This patch creates PresShell::EventHandler and PresShell methods become
wrappers of the stack class, but this patch does not change any logic in the
code, i.e., just reorganizing existing methods.

Note that HandleEventWithTarget() and HandleEventInternal() need to take
WidgetEvent rather than WidgetGUIEvent. Additionally, some other methods
require WidgetGUIEvent to refer WidgetGUIEvent::mWidget. Therefore, this
patch does not make the new class store the event as a member.

PresShell::EventHandler::HandleEvent() is too big. That makes us difficult to
understand the flow of them. So, first of all, we should split the method to
smaller chunks. Then, we can understand what we're doing in HandleEvent() more.

This patch creates MaybeHandleEventWithAccessibleCaret() for first handling
block in HandleEvent(). Note that the following patch will clean it up.
I.e., this patch just moves the existing block into the new method.

Because of spinning out from PresShell::EventHandler::HandleEvent(), we can use
early-return style in MaybeHandleEventWithAccessibleCaret(). This patch
rewrites MaybeHandleEventWithAccessibleCaret() with the style.

PresShell::HandleEvent() treats capturing content only when received event is
related to pointing device. And it's used in 2 purposes. One is for computing
to target document of coming event. The other is for handling events using
coordinates. Therefore, if we create a helper method to retrieve it, we can
move the variable into smaller blocks.

In some cases, PresShell::EventHandler::HandleEvent() needs to call
HandleEvent() of another instance.

For retrieving the instance, we need to compute retarget document first.
This patch makes new method to retrieve it. The following patch will clean
up it.

Next, we need to look for a frame for first parameter of calling
PresShell::HandleEvent() of another PresShell instance. This patch creates
PresShell::EventHandler::GetFrameForHandlingEventWith() to do it.

Unfortunately, the result is used in 3 patterns. One is, the caller should
stop handling the event. Another one is, the caller should keep handling
the event by itself. The other is, the caller should call
PresShell::HandleEvent() of different PresShell instance. Therefore, this
patch makes the method take aFrame of the caller. Then, the caller can check
the last 2 patterns with check the result is same as aFrame. This is not so
smart approach, but I have no better idea without adding a bool argument or
making the return type bool and adding out argument of nsIFrame.

Let's move the redirection of coming event in
PresShell::EventHandler::HandleEvent() into a method. This makes the caller
easier to read.

It may not be safe to handle events even when
PresShell::EventHandler::HandleEvent(). In such case, we need to discard
received events with notifying somebody. This patch move this rare case
jobs into the new method, MaybeDiscardEvent(). Then, the caller, HandleEvnet(),
becomes easier to read.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/66c3b5b15c8e
part 1: Create stack class to handle events in PresShell r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/8b7c7e0317ef
part 2: Create PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret() to handle event with AccessbleCaretEventHub r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/c650507f7fee
part 3: Rewrite PresShell::EventHandler::MaybeHandleEventWithAccessibleCaret() with early-return style r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/9d38de19e569
part 4: Create PresShell::EventHandler::GetCapturingContentFor() to retrieve capturing content for specific event r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/7e9c7b742cb9
part 5: Create PresShell::EventHandler::GetRetargetEventDocument() retrieve retarget document of coming event r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/646991effcda
part 6: Clean up PresShell::EventHandler::GetRetargetEventDocument() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/e7930893f8f0
part 7: Create PresShell::EventHandler::GetFrameForHandlingEventWith() to retrieve a frame which is necessary to handle event with another PresShell instance r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/ea7d56b00394
part 8: Clean up PresShell::EventHandler::GetFrameForHandlingEventWith() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/1d868390c454
part 9: Create PresShell::EventHandler::MaybeHandleEventWithAnotherPresShell() to handle event with another PresShell if necessary r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/8d612e0b0258
part 10: Create PresShell::EventHandler::MaybeDiscardEvent() to check whether it's safe to handle the event r=smaug

PresShell::EventHandler::HandleEvent() discards or puts off to dispatch
the handling event if it's a keyboard event and event dispatching is
suppressed by the document.

This patch moves the block into the new method for making HandleEvent() simpler.

There is an unclear variable frame in PresShell::EventHandler::HandleEvent().
It's overwritten with different frame and its meanings is changed sometimes.
Finally, it's necessary only in the if (aGUIEvent->IsUsingCoordinates())
block. Therefore, we can move it into the block and rename it when them for
each purpose.

PresShell::EventHandler::HandleEvent() tries to flush pending animation first
when it decides frame to handle events using coordinates. This patch moves
the code into the new method.

In some reasons, handling event should be handled in specific frame even if
the coordinates are out of the frame. PresShell::EventHandler::HandleEvent()
computes it with popups, capturing content, etc. This patch moves the blocks
into new method for making HandleEvent() simpler.

Note that most of the code is just moved. The following patch will clean it
up.

PresShell::EventHandler::ComputeRootFrameToHandleEvent() computes root frame
to handle event with popup frame and/or capturing content. The former result
can be rewritten with the latter. So, for cleaning it up with early return
style, we need to split it to 2 methods.

Attachment #9041099 - Attachment description: Bug 1466208 - part 11: Create PresShell::EventHandler::MaybeDiscardOrPutOffToDispatchKeyboardEvent() → Bug 1466208 - part 11: Create PresShell::EventHandler::MaybeDiscardOrDelayKeyboardEvent()
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/7fd1b72f66c0
part 11: Create PresShell::EventHandler::MaybeDiscardOrDelayKeyboardEvent() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d8420442ba89
part 12: Get rid of `frame` variable from PresShell::EventHandler::HandleEvent() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/da71b4d4ad40
part 13: Create PresShell::EventHandler::MaybeFlushThrottledStyles() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/546288d07c7c
part 14: Create PresShell::EventHandler::ComputeRootFrameToHandleEvent() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/41be96f0dbbb
part 15: Split PresShell::EventHandler::ComputeRootFrameToHandleEvent() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/b79d7da6617b
part 16: Clean up PresShell::EventHandler::ComputeRootFrameToHandleEvent() and its helper methods with early-return style r=smaug

This patch moves the block, which discard or put the event into the delayed event
queue if handling event is a mouse event, to new method.

PresShell::EventHandler::HandleEvent() looks for PresShell, nsIFrame and
nsIContent a lot for aGUIEvent. Sometimes part of them are modified,
otherwise, all of them are modified by some reasons. Therefore, for
splitting each of the modifiers into separated methods, we need a struct
for making them as a group and usable for in/out parameter.

(If you have some ideas of better name, let me know.)

Now, we can create methods to update event target into EventTargetData().
This moves a block in PresShell::EventHandler::HandleEvent() to retarget
to active document into the new method.

This patch moves the block to compute event target of the event using
coordinates into the new method of PresShell::EventHandler::EventTargetData.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d8ad6016347f
part 17: Make PresShell::EventHandler::HandleEventWithPointerCapturingContentWithoutItsFrame() to handle event when there is pointer capturing content but it does not have primary frame r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d70e9638ff75
part 18: Create PresShell::EventHandler::MaybeDiscardOrDelayMouseEvent() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/86b52991b24a
part 19: Group PresShell, nsIFrame and nsIContent with a struct r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/1bcbf6ebcd45
part 20: Create PresShell::EventHandler::EventTargetData::MaybeRetargetToActiveDocument() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/4cb13cbe5f05
part 21: Clean up PresShell::EventHandler::EventTargetData::MaybeRetargetToActiveDocument() with early-return style r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/4a53016221d4
part 22: Create PresShell::EventHandler::EventTargetData::ComputeElementFromFrame() r=smaug

We cannot move each block into separated methods while computing EventTargetData
because we need to check capturing contents, etc. Therefore, only each block
should be moved to separated methods for now.

This moves a block which computes event target from point of the event. If
this can be moved to EventTargetData, it might be easier to understand, but
its helper method GetFrameToHandleNonTouchEvent() requires to access members
of EventHandler. Therefore, we need to treat EventTargetData as an out param
of the new method.

Currently, PresShell::EventHandler::HandleEvent() sets overrideClickTarget
only when Pointer Events is enabled and there is pointer capturing content,
and this is computed while dispatching a pointer event.

So, if we move it into EventTargetData, we can move the pointer event
dispatching block into a separated method and caller can receive it with
an EventTargetData instance which is anyway necessary to receive new
target frame after dispatching a pointer event.

Now, we can move the block dispatching preceding pointer event to separated
method. Then, we can hide the complicated retarget process after dispatching
a pointer event from HandleEvent().

After dispatching pointer events, PresShell::EventHandler::HandleEvent()
updates event target only when the event is a touch event. We should do it in
a new method of EventTargetData.

Although I don't know why this is done in
PresShell::EventHandler::DispatchPrecedingPointerEvent().

Now, the block in HandleEvent(), which handles event using coordinates is
less than 200 lines. Perhaps, this is good amount to be split to a method.

This patch just moves the block to a new method.

When the event is not handled with coordinates and there is no frame for
mPresShell, PresShell::EventHandler::HandleEvent() handles the events
simpler than the case there is a frame. Therefore, this patch moves the
else block of if (aFrame) and reduce the indent of if (aFrame) case.

Most remaining code in PresShell::EventHandler::HandleEvent() is what computes
event target of the event which should be handled on focused content. This
patch moves the part to the new method.

Additionally, moves nsIPresShell::gKeyDownTarget to
EventHandler::sLastKeyDownEventTargetElement and make it use StaticRefPtr.

Finally, for using Element* instead of nsIContent*, changes the result type
of Document::GetUnfocusedKeyEventTarget() to Element*.

If focused element is in another document,
PresShell::EventHandler::HandleEvent() needs to retarget the event to another
PresShell. This patch moves the case into new overload method,
MaybeHandleEventWithAnotherPresShell().

Additionally, removes PresShell::HandleRetargetedEvent() and makes
EventHandler::HandleRetargetedEvent() non-public because the new method
is the only user of them.

The remaining part of PresShell::EventHandler::HandleEvent() does:

  1. Handles the event at focused content.
  2. Handles the event with given frame which is a frame for mPresShell.

For making them clearer, this patch moves them into new methods.

With splitting HandleEvent() a lot, it becomes more difficult to keep
managing each set of calling PushCurrentEventInfo() and
PopCurrentEventInfo(). So, EventHandler should have a helper class
to push and pop current event info into/from the stack.

PresShell::EventHandler::HandleEventInsternal() recodes event handling
response performance with telemetry after it dispatches the event. We can move
it into new method simply.

PresShell::EventHandler::HandleEventInternal() needs to accumulate event
handling time per each event type. The handling start time needs to be
recoded before sending EventStateManager. Therefore, this patch makes the
helper class which is a stack class, recodes current time at construction
and calls Telemetry::AccumulateTimeDelta() at destruction automatically.

If aEvent requires frame but there is no event target,
PresShell::EventHandler::HandleEventInternal() just records the response
time. So, we can reduce one indent level in the big method.

Note that I'm not sure recoding the response time in such case because
the good values may make the average and median better. But this is
out of scope of bug 1466208.

If Shift state of eContextMenu event is active, we make it not fired on
web content. Additionally, if it's not time to open context menu, we shouldn't
dispatch it into the DOM. The new method prepare and check them.

Oddly, there are two trusted eMouseMove preparation code in
PresShell::EventHandler::HandleEventInternal(). One is in the switch
statement which is used only when aEvent is trusted. The other is after
TouchManager::PreHandleEvent() is called and after
AutoHandlingUserInputStatePusher is created. However, both of them do
nothing if the event is eMouseMove. Therefore, we can move the latter
into the former.

The first switch statement of PresShell::EventHandler::HandleEventInternal()
has 2 jobs:

  • Prepare something for specific event type.
  • Record the preparation time of some types of events to telemetry.

This intermixed code is not easy to understand and somebody may add new
preparation after recording them. So, even though the preparation time
becomes worse a couple of milliseconds, we should split those jobs.

The patch moves the latter job into the new method.

PresShell::EventHandler::HandleEventInternal() may handle Escape key before
dispatching it in some cases. This requires too many lines for somebody who
investigate the method for the other events. Therefore, this patch moves it
into the new method.

Additionally, this patch creates WidgetKeyboardEvent::IsUserInteraction()
because PresShell::EventHandler refers same result in 2 places. Finally,
its condition is not enough to what the comment wants to do since it does
not check some modifier keys. Therefore, this patch makes it check all
possible modifier keys too.

For making PresShell::EventHandler::HandleEventInternal() easier to read,
move the large switch statement for preparation into the new method.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/37f52c22967c
part 23: Create PresShell::EventHandler::ComputeEventTargetFrameAndPresShellAtEventPoint() r=smaug
Attachment #9046654 - Attachment description: Bug 1466208 - part 32: Create PresShell::EventHandler::AutoCurrentEventInfoSetter class → Bug 1466208 - part 30: Create PresShell::EventHandler::AutoCurrentEventInfoSetter class
Attachment #9046652 - Attachment description: Bug 1466208 - part 30: Create a PresShell::EventHandler::MaybeHandleEventWithAnotherPresShell() overload → Bug 1466208 - part 31: Create a PresShell::EventHandler::MaybeHandleEventWithAnotherPresShell() overload
Attachment #9046653 - Attachment description: Bug 1466208 - part 31: Create PresShell::EventHandler::HandleEventAtFocusedContent() and PresShell::EventHandler::HandleEventWithFrameForPresShell() → Bug 1466208 - part 32: Create PresShell::EventHandler::HandleEventAtFocusedContent() and PresShell::EventHandler::HandleEventWithFrameForPresShell()
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/51e72b19a617
part 24: Move `overrideClickTarget` into EventTargetData r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/c9052ac46602
part 25: Create PresShell::EventHandler::DispatchPrecedingPointerEvent() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/a6fd8376d3ec
part 26: Create PresShell::EventHandler::EventTargetData::UpdateTouchEventTarget() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/f05208c48b86
part 27: Create PresShell::EventHandler::HandleEventUsingCoordinates() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/deb832a494bb
part 28: Make PresShell::EventHandler::HandleEvent() handle non-using-coordinates events without frame before with frame case r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/664b3bc4a449
part 29: Create PresShell::EventHandler::ComputeFocusedEventTargetElement() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/29a17314e008
part 30: Create PresShell::EventHandler::AutoCurrentEventInfoSetter class r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/5b5c05af88e2
part 31: Create a PresShell::EventHandler::MaybeHandleEventWithAnotherPresShell() overload r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/a52bca8b0dd5
part 32: Create PresShell::EventHandler::HandleEventAtFocusedContent() and PresShell::EventHandler::HandleEventWithFrameForPresShell() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/5ad056528954
part 33: Create PresShell::EventHandler::RecordEventHandlingResponsePerformance() r=smaug

This is the part which actually handles the event. The new method should
notify EventStateManager of dispatching event before and after that, and
actually dispatch the event into the DOM.

Finally, we should move the last switch statement in HandleEventInternal()
to the new method. Then, `HandleEventInternal() does nothing complicated
things by itself.

In my understanding, PresShell::EventHandler::HandleEvent() may redirect
the event to another class or PresShell first. Otherwise, it computes
event target and sets current event info of mPresShell to it. Then, calls
HandleEventInternal() to dispatch the event. Then, HandleEventInternal()
may handle the event before dispatch, and/or prepare to dispatch, then,
finally dispatches the event and finalize the state of mPresShell and the
event. Therefore, HandleEventInternal() actually handles the event, but
the word, "internal" is not explicitly explain its different points from
HandleEvent(). Therefore, I think that HandleEventWithCurrentEventInfo()
is better name since HandleEvent() considers the current event info.

Now, other methods taking aFrame of HandleEvent() names the argument as
aFrameForPresShell. So, HandleEvent()'s aFrame should also be renamed.

This patch renames it and adds MOZ_CAN_RUN_SCRIPT and comment to
nsIPresShell::HandleEvent().

This is the final patch for bug 1466208.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/f10044217e8f
part 34: Create a helper class, PresShell::EventHandler::HandlingTimeAccumulator() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/22b988a39a32
part 35: Reduce one indent level in PresShell::EventHandler::HandleEventInternal() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/525dd00d5636
part 36: Create PresShell::EventHandler::PrepareToDispatchContextMenuEvent() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/e5e798d73ac6
part 37: Move trusted eMouseMove event preparation into the previous switch-case block r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/e0c39e6d8d2c
part 38: Create PresShell::EventHandler::PrepareToDispatchContextMenuEvent() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/199af6c43953
part 39: Create PresShell::EventHandler::MaybeHandleKeyboardEventBeforeDispatch() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/f9482e1d252f
part 40: Create PresShell::EventHandler::PrepareToDispatchEvent() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/d9ed942f6b00
part 41: Create PresShell::EventHandler::DispatchEvent() r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/3a421f33761b
part 42: Clean up PresShell::EventHandler::DispatchEvent() with using early-return style r=smaug
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/8620e56a5f38
part 43: Create PresShell::EventHandler::FinalizeHandlingEvent() r=smaug

Allow me this drive-by comment. As the TB sheriff I'm watching M-C merges and I see this bug every day :-)

Nice job, two parts missing to land, so you're done before the next branch date.

(In reply to Jorg K (GMT+1) from comment #171)

Allow me this drive-by comment. As the TB sheriff I'm watching M-C merges and I see this bug every day :-)

Nice job, two parts missing to land, so you're done before the next branch date.

Thanks and yes, all patches will be landed 67 cycle. Note that the last patches have already been in the queue to land the autoland. So, this bug should be fixed in next 12 (or 24) hours.

https://hg.mozilla.org/mozilla-central/rev/88a92eb3865e

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox67: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

Oh, failed to land part 45.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/4be0480f48d3
part 45: Rename `aFrame` of `HandleEvent()` to `aFrameForPresShell` r=smaug
Component: Event Handling → User events and focus handling

https://hg.mozilla.org/mozilla-central/rev/4be0480f48d3

Status: REOPENED → RESOLVED
Closed: 4 years ago → 4 years ago
status-firefox67: affected → fixed
Resolution: --- → FIXED
Regressions: 1555645
No longer regressions: 1555645
You need to log in before you can comment on or make changes to this bug.