Comment 107 for bug 1228250

Comment on attachment 8915954
Bug 143038 Make users can scroll contents horizontally with vertical wheel operation with a modifier

https://reviewboard.mozilla.org/r/186794/#review192674

This is complicated enough, that I think I should re-read this after those small nits are fixed.

::: commit-message-19b32:26
(Diff revision 1)
> +restoring.
> +
> +So, this patch does NOT change any wheel event information on web apps. Only
> +changes its default action. This is same behavior as Chromium.
> +
> +Note that with this patch, users cannot navigate the tab's history with

Could you explain this. Currently shift+vertical wheel is back-forward. Does this new behavior override that feature always or only when it is enabled explicitly?

How is shift+horizontal supposed to work for back-forward if shift+vertical is for scrolling? One often gets both horizontal and vertical scrolling when using touchpad.

::: dom/events/EventStateManager.h:652
(Diff revision 1)
> * If an .override_x value is -1, same as the
> * corresponding mActions value.
> */
> Action mOverriddenActionsX[COUNT_OF_MULTIPLIERS];
>
> + // XXX Modifier is better than Modifiers. However, it's defined in

That seems like a silly reason to not use Modifier.
If you're really worried about the build time change, why not just move Modifier to its own header?

::: dom/events/EventStateManager.cpp:5691
(Diff revision 1)
> + }
> +
> + Index index = GetIndexFor(aEvent->mModifiers &
> + ~mModifierToTreatVertialWheelAsHorizontalScroll);
> + Init(index);
> + // We need to cache this result in the widget. Some methods of this class

Cache the result in the widget? I don't see anything stored in the widget.

::: dom/events/EventStateManager.cpp:5710
(Diff revision 1)
> return INDEX_DEFAULT;
> }
> + if (!NeedToTreatAsHorizontalScroll(aEvent)) {
> + return GetIndexFor(aEvent->mModifiers);
> + }
> +#if 0

No ifdef 0 code, please

::: dom/events/EventStateManager.cpp:5837
(Diff revision 1)
> Index index = GetIndexFor(aEvent);
> Init(index);
>
> + // If the event should be treated as horizontal wheel operation, deltaY
> + // should be applied mMultiplierX. Note that deltaX and deltaZ are always
> + // 0 in such case. Therefore, we only need to use temporary variable only

Why they are 0?

::: dom/events/EventStateManager.cpp:5842
(Diff revision 1)
> + // 0 in such case. Therefore, we only need to use temporary variable only
> + // for deltaY. Additionally, if the event is being handled by default
> + // handler, the deltaX and deltaY may be swapped. Therefore, we need to
> + // use mMultiplierX for deltaY only when the event should be treated as
> + // horizontal scroll and mDeltaY isn't 0.
> + auto multiplierForDeltaY = mMultiplierY[index];

Could you not use auto here. With auto I need to go to the mMultiplierY definition to see what the type of multiplierForDeltaY is.

::: dom/events/EventStateManager.cpp:5886
(Diff revision 1)
> aEvent->mOverflowDeltaX /= mMultiplierX[index];
> }
> - if (mMultiplierY[index]) {
> - aEvent->mOverflowDeltaY /= mMultiplierY[index];
> +
> + // If the event should be treated as horizontal wheel operation, deltaY
> + // should be applied mMultiplierX. Note that deltaX and deltaZ are always
> + // 0 in such case. Therefore, we only need to use temporary variable only

Again, why they are 0?

::: dom/events/EventStateManager.cpp:5891
(Diff revision 1)
> + // 0 in such case. Therefore, we only need to use temporary variable only
> + // for deltaY. Additionally, if the event is being handled by default
> + // handler, the deltaX and deltaY may be swapped. Therefore, we need to
> + // use mMultiplierX for deltaY only when the event should be treated as
> + // horizontal scroll and mDeltaY isn't 0.
> + auto multiplierForDeltaY = mMultiplierY[index];

Don't use auto, pretty please.

::: dom/events/WheelHandlingHelper.h:217
(Diff revision 1)
> + */
> +class MOZ_STACK_CLASS AutoTemporarilyWheelDeltaSwapper final
> +{
> +public:
> + /**
> + * @param aWheelEvent A wheel event. Must not be nullptr.

If so, perhaps make the ctor take WidgetWheelEvent& and store using that type too. That self-documents that it must not ever-never be null