Closed Bug 1306549 Opened 8 years ago Closed 8 years ago

Clean up KeyboardLayout::InitNativeKey()

Tracking

()

Status:

RESOLVED FIXED

Milestone:

mozilla52

Tracking Flags:

Tracking

Status

firefox52

---

fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Whiteboard: tpi:+)

Attachments

(10 files)

Bug 1306549 part.1 KeyboardLayout::InitNativeKey() should use KeyboardLayout::IsDeadKey() 8 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900) 58 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1306549 part.2 Create overload methods which take ModifierKeyState instead of VirtualKey::ShiftState 8 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900) 58 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1306549 part.3 KeyboardLayout::InitNativeKey() should use GetUniCharsAndModifiers() instead of using VirtualKey::GetUniChars() directly 8 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900) 58 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1306549 part.4 Create KeyboardLayout::GetNativeUniCharsAndModifiers() for wrapping VirtualKey::GetNativeUniChars() 8 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900) 58 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1306549 part.5 Create KeyboardLayout::GetCompositeChar() for wrapping VirtualKey::GetCompositeChar() 8 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900) 58 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1306549 part.6 Create KeyboardLayout::ActivateDeadKeyState() 8 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900) 58 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1306549 part.7 Create KeyboardLayout::MaybeInitNativeKeyAsDeadKey() 8 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900) 58 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1306549 part.8 Get rid of \|virtualKey\| and \|isKeyDown\| from KeyboardLayout::InitNativeKey() and KeyboardLayout::MaybeInitNativeKeyAsDeadKey() 8 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900) 58 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1306549 part.9 Create KeyboardLayout::IsInDeadKeySequence() which returns true while it's in a dead key sequence 8 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900) 58 bytes, text/x-review-board-request	m_kato : review+	Details
Bug 1306549 part.10 Reorder member declaration of KeyboardLayout class 8 years ago Masayuki Nakano [:masayuki] (he/him)(JST, +0900) 58 bytes, text/x-review-board-request	m_kato : review+	Details

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

Assignee

Description

•

8 years ago

KeyboardLayout::InitNativeKey() handles a lot of cases with accessing a lot of members. This is not easier to understand what it does.

Let's clean up it with wrapping each part of the method with new methods.

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

Assignee

Comment 1

•

8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d0c20ae8a73

Comment hidden (mozreview-request)

KeyboardLayout::InitNativeKey() is very messy because it handles a lot of cases without helper methods.

It's important to make it simpler implementation for preventing regressions caused by some patches which are written with misunderstanding.  So, let's rewrite the method with helper method.

First, this patch make InitNativeKey() use IsDeadKey() instead of referring the table because calling IsDeadKey() is easier to understand.

Review commit: https://reviewboard.mozilla.org/r/82610/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82610/

Comment hidden (mozreview-request)

This patch gets rid of |shiftState| from KeyboardLayout::InitNativeKey() and make each caller clearer.  This must make other developers understand what modifier state is used at each call.

Review commit: https://reviewboard.mozilla.org/r/82612/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82612/

Comment hidden (mozreview-request)

When InitNativeKey() retrieves UniCharsAndModifiers for a key, it needs key index for the given virtual keycode.  Therefore, wrapping the code with GetUniCharsAndModifiers() makes InitNativeKey() code simpler since each call specifies the virtual keycode to the method instead of key index.

Review commit: https://reviewboard.mozilla.org/r/82614/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82614/

Comment hidden (mozreview-request)

Similar to VirtualKey::GetUniChars(), VirtualKey::GetNativeUniChars() needs key index.  So, it should be wrapped by a new helper method, KeyboardLayout::GetNativeUniCharsAndModifiers(), and KeyboardEvent::InitNativeKey() should use this instead of accessing its member's GetNativeUniChars().

Review commit: https://reviewboard.mozilla.org/r/82616/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82616/

Comment hidden (mozreview-request)

VirtualKey::GetCompositeChar() needs index of virtual keycode which may make around the caller messy.  So, KeyboardLayout::GetCompositeChar() should wrap it and KeyboardLayout::MaybeInitNativeKeyWithCompositeChar() should use it.

Review commit: https://reviewboard.mozilla.org/r/82618/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82618/

Comment hidden (mozreview-request)

There is DeactivateDeadKeyState() but not ActivateDeadKeyState().  There should be both of them and hides the dead key state management into them.

Review commit: https://reviewboard.mozilla.org/r/82620/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82620/

Comment hidden (mozreview-request)

This patch creates KeyboardLayout::MaybeInitNativeKeyAsDeadKey() for wrapping dead key handling code in KeyboardLayout::InitNativeKey().

This makes InitNativeKey() code simpler.  Now, any developers must be able to understand what InitNativeKey() does easier than before fixing this bug.

Review commit: https://reviewboard.mozilla.org/r/82622/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82622/

Comment hidden (mozreview-request)

For preventing wrapping long lines, old KeyboardLayout::InitNativeKey() uses two variables |virtualKey| and |isKeyDown| which are never modified.  However, they may be not clear for some developers where they came from.  Additionally, preceding patches reduced a lot of users of them and indent level.  Therefore, even if we remove these variables, we don't need additional line breaks in most cases.  So, removing them is better for easier to understand.

Review commit: https://reviewboard.mozilla.org/r/82624/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82624/

Comment hidden (mozreview-request)

Currently, checking if it's in dead key sequence directly refers mActiveDeadKey.  However, this isn't clear for most developers.  So, let's create a helper method, IsInDeadKeySequence(), and wrap the nonintuitive check.

Review commit: https://reviewboard.mozilla.org/r/82626/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82626/

Comment hidden (mozreview-request)

We usually declare class members with following order:

1. public members
2. private members
3. friends

However, current KeyboardLayout is declared with completely opposite order.  Let's use usual order for preventing mistakes.

Review commit: https://reviewboard.mozilla.org/r/82628/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/82628/

Makoto Kato [:m_kato]

Comment 12

•

8 years ago

mozreview-review

Comment on attachment 8796994 [details]
Bug 1306549 part.1 KeyboardLayout::InitNativeKey() should use KeyboardLayout::IsDeadKey()

https://reviewboard.mozilla.org/r/82610/#review81268

Attachment #8796994 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 13

•

8 years ago

mozreview-review

Comment on attachment 8796995 [details]
Bug 1306549 part.2 Create overload methods which take ModifierKeyState instead of VirtualKey::ShiftState

https://reviewboard.mozilla.org/r/82612/#review81286

Attachment #8796995 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 14

•

8 years ago

mozreview-review

Comment on attachment 8796996 [details]
Bug 1306549 part.3 KeyboardLayout::InitNativeKey() should use GetUniCharsAndModifiers() instead of using VirtualKey::GetUniChars() directly

https://reviewboard.mozilla.org/r/82614/#review81302

Attachment #8796996 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 15

•

8 years ago

mozreview-review

Comment on attachment 8796997 [details]
Bug 1306549 part.4 Create KeyboardLayout::GetNativeUniCharsAndModifiers() for wrapping VirtualKey::GetNativeUniChars()

https://reviewboard.mozilla.org/r/82616/#review81328

Attachment #8796997 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 16

•

8 years ago

mozreview-review

Comment on attachment 8796998 [details]
Bug 1306549 part.5 Create KeyboardLayout::GetCompositeChar() for wrapping VirtualKey::GetCompositeChar()

https://reviewboard.mozilla.org/r/82618/#review81346

Attachment #8796998 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 17

•

8 years ago

mozreview-review

Comment on attachment 8796999 [details]
Bug 1306549 part.6 Create KeyboardLayout::ActivateDeadKeyState()

https://reviewboard.mozilla.org/r/82620/#review81358

Attachment #8796999 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 18

•

8 years ago

mozreview-review

Comment on attachment 8797000 [details]
Bug 1306549 part.7 Create KeyboardLayout::MaybeInitNativeKeyAsDeadKey()

https://reviewboard.mozilla.org/r/82622/#review81620

Attachment #8797000 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 19

•

8 years ago

mozreview-review

Comment on attachment 8797001 [details]
Bug 1306549 part.8 Get rid of |virtualKey| and |isKeyDown| from KeyboardLayout::InitNativeKey() and KeyboardLayout::MaybeInitNativeKeyAsDeadKey()

https://reviewboard.mozilla.org/r/82624/#review81622

Attachment #8797001 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 20

•

8 years ago

mozreview-review

Comment on attachment 8797002 [details]
Bug 1306549 part.9 Create KeyboardLayout::IsInDeadKeySequence() which returns true while it's in a dead key sequence

https://reviewboard.mozilla.org/r/82626/#review81624

Attachment #8797002 - Flags: review?(m_kato) → review+

Makoto Kato [:m_kato]

Comment 21

•

8 years ago

mozreview-review

Comment on attachment 8797003 [details]
Bug 1306549 part.10 Reorder member declaration of KeyboardLayout class

https://reviewboard.mozilla.org/r/82628/#review81628

::: widget/windows/KeyboardLayout.h:748
(Diff revision 1)
> +  int32_t mActiveDeadKey;                 // -1 = no active dead-key
> +  VirtualKey::ShiftState mDeadKeyShiftState;
> +
> +  bool mIsOverridden : 1;
> +  bool mIsPendingToRestoreKeyboardLayout : 1;
> +

It is unnecessary to use bit fields such as "bool mIsOverrideen : 1".  Since most platform uses aligned allocation (32bit is 4, 64bit is 8), It doesn't reduce allocation size when bool member is smaller than 4/8.

Also, bit fields requires more code to bit to int.  So it should use bit fields for heap size if a lot of this objects are created.

Makoto Kato [:m_kato]

Comment 22

•

8 years ago

mozreview-review

Comment on attachment 8797003 [details]
Bug 1306549 part.10 Reorder member declaration of KeyboardLayout class

https://reviewboard.mozilla.org/r/82628/#review81632

Attachment #8797003 - Flags: review?(m_kato) → review+

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

Assignee

Comment 23

•

8 years ago

mozreview-review-reply

Comment on attachment 8797003 [details]
Bug 1306549 part.10 Reorder member declaration of KeyboardLayout class

https://reviewboard.mozilla.org/r/82628/#review81628

> It is unnecessary to use bit fields such as "bool mIsOverrideen : 1".  Since most platform uses aligned allocation (32bit is 4, 64bit is 8), It doesn't reduce allocation size when bool member is smaller than 4/8.
> 
> Also, bit fields requires more code to bit to int.  So it should use bit fields for heap size if a lot of this objects are created.

Good catch! I'll stop using the bit fields before landing.

Comment hidden (mozreview-request)

Comment on attachment 8796994 [details]
Bug 1306549 part.1 KeyboardLayout::InitNativeKey() should use KeyboardLayout::IsDeadKey()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82610/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8796995 [details]
Bug 1306549 part.2 Create overload methods which take ModifierKeyState instead of VirtualKey::ShiftState

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82612/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8796996 [details]
Bug 1306549 part.3 KeyboardLayout::InitNativeKey() should use GetUniCharsAndModifiers() instead of using VirtualKey::GetUniChars() directly

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82614/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8796997 [details]
Bug 1306549 part.4 Create KeyboardLayout::GetNativeUniCharsAndModifiers() for wrapping VirtualKey::GetNativeUniChars()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82616/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8796998 [details]
Bug 1306549 part.5 Create KeyboardLayout::GetCompositeChar() for wrapping VirtualKey::GetCompositeChar()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82618/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8796999 [details]
Bug 1306549 part.6 Create KeyboardLayout::ActivateDeadKeyState()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82620/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8797000 [details]
Bug 1306549 part.7 Create KeyboardLayout::MaybeInitNativeKeyAsDeadKey()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82622/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8797001 [details]
Bug 1306549 part.8 Get rid of |virtualKey| and |isKeyDown| from KeyboardLayout::InitNativeKey() and KeyboardLayout::MaybeInitNativeKeyAsDeadKey()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82624/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8797002 [details]
Bug 1306549 part.9 Create KeyboardLayout::IsInDeadKeySequence() which returns true while it's in a dead key sequence

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82626/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8797003 [details]
Bug 1306549 part.10 Reorder member declaration of KeyboardLayout class

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/82628/diff/1-2/

Pulsebot

Comment 34

•

8 years ago

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/54940e9b9a1b
part.1 KeyboardLayout::InitNativeKey() should use KeyboardLayout::IsDeadKey() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/bb92cb94dcac
part.2 Create overload methods which take ModifierKeyState instead of VirtualKey::ShiftState r=m_kato
https://hg.mozilla.org/integration/autoland/rev/2e45f263fb19
part.3 KeyboardLayout::InitNativeKey() should use GetUniCharsAndModifiers() instead of using VirtualKey::GetUniChars() directly r=m_kato
https://hg.mozilla.org/integration/autoland/rev/fd077161e636
part.4 Create KeyboardLayout::GetNativeUniCharsAndModifiers() for wrapping VirtualKey::GetNativeUniChars() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/adba65212c35
part.5 Create KeyboardLayout::GetCompositeChar() for wrapping VirtualKey::GetCompositeChar() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/9be247c306a1
part.6 Create KeyboardLayout::ActivateDeadKeyState() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/489e11449d8e
part.7 Create KeyboardLayout::MaybeInitNativeKeyAsDeadKey() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/7dbd5e61db37
part.8 Get rid of |virtualKey| and |isKeyDown| from KeyboardLayout::InitNativeKey() and KeyboardLayout::MaybeInitNativeKeyAsDeadKey() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/be15fd5862d3
part.9 Create KeyboardLayout::IsInDeadKeySequence() which returns true while it's in a dead key sequence r=m_kato
https://hg.mozilla.org/integration/autoland/rev/0bf2bbd0c300
part.10 Reorder member declaration of KeyboardLayout class r=m_kato

Tracy Walker [:tracy]

Updated

•

8 years ago

Priority: -- → P2

Whiteboard: tpi:+

Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)

Comment 35

•

8 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/54940e9b9a1b
https://hg.mozilla.org/mozilla-central/rev/bb92cb94dcac
https://hg.mozilla.org/mozilla-central/rev/2e45f263fb19
https://hg.mozilla.org/mozilla-central/rev/fd077161e636
https://hg.mozilla.org/mozilla-central/rev/adba65212c35
https://hg.mozilla.org/mozilla-central/rev/9be247c306a1
https://hg.mozilla.org/mozilla-central/rev/489e11449d8e
https://hg.mozilla.org/mozilla-central/rev/7dbd5e61db37
https://hg.mozilla.org/mozilla-central/rev/be15fd5862d3
https://hg.mozilla.org/mozilla-central/rev/0bf2bbd0c300

Status: ASSIGNED → RESOLVED

Closed: 8 years ago

status-firefox52: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla52

You need to log in before you can comment on or make changes to this bug.