820654 - Work - Clean up MetroInput's use of ModifierKeyState and nsIDOMTouch/nsDOMTouch

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: Input, defect)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Misc cleanup. Also removes the need for #include "KeyboardLayout.h" in MetroInput.cpp

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1

This will apply cleanly only after applying the patch in bug 819223

Comment 2

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 691172 [details] [diff] [review]
Patch v1

I'm rebasing this to apply to elm tip since the work in bug 819223 is slightly delayed.  I'll mark for review when I attach the updated patch

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2

Rebased to apply cleanly to elm tip

Comment 4

[Jim Mathies [:jimm]]

http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.cpp#111

What was it about ModifierKeyState that we didn't like? Is there any way we can incorporate the improved key detection code in InitModifiersForGeckoEvent into ModifierKeyState::InitInputEvent?

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I don't understand why the fix is necessary. If you don't want metro widget to depend on KeyboardLayout.h, you should move the related methods to WinUtils or something.

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v3

(In reply to Jim Mathies [:jimm] from comment #4)
> http://mxr.mozilla.org/mozilla-central/source/widget/windows/KeyboardLayout.
> cpp#111
> 
> What was it about ModifierKeyState that we didn't like? Is there any way we
> can incorporate the improved key detection code in
> InitModifiersForGeckoEvent into ModifierKeyState::InitInputEvent?

There isn't anything particularly bad about ModifierKeyState.  InitModifiersForGeckoEvent doesn't offer improved key detection; it basically mimics the functionality of ModifierKeyState.  I wrote the previous patch because
  1) I want to remove metro widget's need to include "KeyboardLayout.h" since "KeyboardLayout.h" defines classes that we shouldn't be using in Metro development.
  2) The current implementation of MetroWidget doesn't use ModifierKeyState very well; it creates a whole bunch of them when it could instead just have 1.

While the previous patch fixes these issues, a better fix would be to continue using ModifierKeyState in MetroInput, but to separate ModifierKeyState from "KeyboardLayout.h".  This is the solution suggested by Nakano-san.


(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #5)
> I don't understand why the fix is necessary. If you don't want metro widget
> to depend on KeyboardLayout.h, you should move the related methods to
> WinUtils or something.

I've attached a patch that simply moves ModifierKeyState out of KeyboardLayout.h/cpp and into its own ModifierKeyState.h/cpp.  Does this approach seem reasonable?

Comment 7

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

In the interest of making forward progress, I'm limiting the scope of this bug to just MetroInput cleanup: using a single ModifierKeyState that is a member of MetroInput, and utilizing nsIDOMTouch more effectively (so that we don't have to create our own TouchEntry class).

I've filed bug 825337 about separating ModifierKeyState from KeyboardLayout

Comment 8

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: patch v4

This patch cleans up MetroInput:
  It adds a ModifierKeyState member to MetroInput so that we don't have to create them all over the place
  It takes advantage of the nsIDOMTouch interface, making it so that we don't have to define and use a separate TouchEntry class
  It removes the "buttons" flag from MetroInput, since that is not as reliable as ModifierKeyState for keeping track of button state
  Miscellaneous other comment, whitespace, and naming fixes

Comment 9

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

I built this patch locally and did a sanity test to make sure it didn't break anything horribly.

Patch submitted to elm:
https://hg.mozilla.org/projects/elm/rev/3afea2350981

Build containing the patch:
https://tbpl.mozilla.org/?tree=Elm&rev=3afea2350981

Comment 10

[Matt Brubeck (:mbrubeck)]

Resolving bugs in the Firefox for Metro product that are fixed on the elm branch.  Sorry for the bugspam.  Search your email for "bugspam-elm" if you want to find and delete all of these messages at once.