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

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.

https://hg.mozilla.org/mozilla-central/rev/66c3b5b15c8e

https://hg.mozilla.org/mozilla-central/rev/8b7c7e0317ef

https://hg.mozilla.org/mozilla-central/rev/c650507f7fee

https://hg.mozilla.org/mozilla-central/rev/9d38de19e569

https://hg.mozilla.org/mozilla-central/rev/7e9c7b742cb9

https://hg.mozilla.org/mozilla-central/rev/646991effcda

https://hg.mozilla.org/mozilla-central/rev/e7930893f8f0

https://hg.mozilla.org/mozilla-central/rev/ea7d56b00394

https://hg.mozilla.org/mozilla-central/rev/1d868390c454

https://hg.mozilla.org/mozilla-central/rev/8d612e0b0258

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.

https://hg.mozilla.org/mozilla-central/rev/7fd1b72f66c0

https://hg.mozilla.org/mozilla-central/rev/d8420442ba89

https://hg.mozilla.org/mozilla-central/rev/da71b4d4ad40

https://hg.mozilla.org/mozilla-central/rev/546288d07c7c

https://hg.mozilla.org/mozilla-central/rev/41be96f0dbbb

https://hg.mozilla.org/mozilla-central/rev/b79d7da6617b

This patch splits the part handling event when there is pointer capturing
content but it does not have frame.

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.

https://hg.mozilla.org/mozilla-central/rev/d8ad6016347f

https://hg.mozilla.org/mozilla-central/rev/d70e9638ff75

https://hg.mozilla.org/mozilla-central/rev/86b52991b24a

https://hg.mozilla.org/mozilla-central/rev/1bcbf6ebcd45

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

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

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.

https://hg.mozilla.org/mozilla-central/rev/37f52c22967c

https://hg.mozilla.org/mozilla-central/rev/51e72b19a617

https://hg.mozilla.org/mozilla-central/rev/c9052ac46602

https://hg.mozilla.org/mozilla-central/rev/a6fd8376d3ec

https://hg.mozilla.org/mozilla-central/rev/f05208c48b86

https://hg.mozilla.org/mozilla-central/rev/deb832a494bb

https://hg.mozilla.org/mozilla-central/rev/664b3bc4a449

https://hg.mozilla.org/mozilla-central/rev/29a17314e008

https://hg.mozilla.org/mozilla-central/rev/5b5c05af88e2

https://hg.mozilla.org/mozilla-central/rev/a52bca8b0dd5

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.

https://hg.mozilla.org/mozilla-central/rev/5ad056528954

https://hg.mozilla.org/mozilla-central/rev/f10044217e8f

https://hg.mozilla.org/mozilla-central/rev/22b988a39a32

https://hg.mozilla.org/mozilla-central/rev/525dd00d5636

https://hg.mozilla.org/mozilla-central/rev/e5e798d73ac6

https://hg.mozilla.org/mozilla-central/rev/e5e798d73ac6

https://hg.mozilla.org/mozilla-central/rev/e0c39e6d8d2c
https://hg.mozilla.org/mozilla-central/rev/199af6c43953

https://hg.mozilla.org/mozilla-central/rev/f9482e1d252f

https://hg.mozilla.org/mozilla-central/rev/d9ed942f6b00

https://hg.mozilla.org/mozilla-central/rev/3a421f33761b

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.

https://hg.mozilla.org/mozilla-central/rev/8620e56a5f38

(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

https://hg.mozilla.org/integration/autoland/rev/88a92eb3865e6482155111c684ba56bf73612ee3

Oh, failed to land part 45.

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