574340 - Cleaning up nsKeyboardLayout which doesn't use our coding style

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

This was spun off from bug 440457 comment 12.

nsKeyboardLayout.h and nsKeyboardLayout.cpp ignores the our coding style, we should clean up it, right now because nobody maintains these files in this 12 months.

Comment 1

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

Attachment: Patch v1.0 part.1 Adding "ns" prefix to the classes which are defined in nsKeyboardLayout.h

Classes which is defined in header files should be named with "ns" prefix.

Comment 2

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

Attachment: Patch v1.0 part.2 Removing unnecessary white spaces and wrapping long lines

nsKeyboardLayout.* inserted a white space before method's '(' and array's '['. We should remove them.

And many lines have over 80 characters.

Comment 3

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

Attachment: Patch v1.0 part.3 Changing control strucure and removing "else" after "if" block which returns always

Changing from:

if ()
{
}

to:

if () {
}

And removing else block if the previous if block always returns from the method.

I.e.,

if () {
  ...
  return;
} else {
  ....
}

to:

if () {
  ...
  return;
}

....

Comment 4

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

Attachment: Patch v1.0 part.4 Use early return style

We should use early return style for reducing indentation.

Comment 5

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

Attachment: Patch v1.0 part.4 (-U 8 -p -W)

You can use this file for review the part.4.

Comment 6

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

Attachment: Patch v1.0 part.5 fix some nits

* Use i for the loop counter name.
* Don't use rv for non-nsresult variable.
* Use () after assignment operators (=, |=).
* Remove redundant empty lines.
* Remove NUM_OF_KEYS because it's only used at definition of mVirtualKeys.

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 453712 [details] [diff] [review]
Patch v1.0 part.1 Adding "ns" prefix to the classes which are defined in nsKeyboardLayout.h

Technically I think we dropped the ns requirement. I don't mind adding it if you prefer it but I don't think it's required anymore.

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 453713 [details] [diff] [review]
Patch v1.0 part.2 Removing unnecessary white spaces and wrapping long lines

nice!

Comment 9

[Jim Mathies [:jimm]]

Comment on attachment 453715 [details] [diff] [review]
Patch v1.0 part.3 Changing control strucure and removing "else" after "if" block which returns always

more awesomeness!

Comment 10

[Jim Mathies [:jimm]]

Comment on attachment 453716 [details] [diff] [review]
Patch v1.0 part.4 Use early return style


>+  if (numOfChars == 0) {
>+    if (numOfUnshiftedChars == 0) {

+  if (!numOfChars) {
+    if (!numOfUnshiftedChars) {

>+               memcmp(aUniChars, unshiftedChars,
>+                      numOfChars * sizeof(PRUnichar)) == 0)) {

+               !memcmp(aUniChars, unshiftedChars,
+                      numOfChars * sizeof(PRUnichar)))) {

nits, the standard is ! vs ==0

Not much else I can find here, nice work.

Comment 11

[Jim Mathies [:jimm]]

Comment on attachment 453718 [details] [diff] [review]
Patch v1.0 part.5 fix some nits

>+  for (PRUint32 i = 0; i < aNumOfChars; i++) {

>+  for (PRUint32 i = aNumOfChars; i < len; i++) {

>+  PRUint32 i;
>+  PRUint32 len = NS_ARRAY_LENGTH(mShiftStates[aShiftState].Normal.Chars);
>+  for (i = 0; i < len && mShiftStates[aShiftState].Normal.Chars[i]; i++) {
..


How about 'idx' or 'index' instead?


>   BYTE kbdState[256];
>+  memset(kbdState, 0, sizeof(kbdState));
>..
>   PRUint16 shiftStatesWithBaseChars = 0;
> 
>-  memset(kbdState, 0, sizeof(kbdState));

yay!

>-  #define NUM_OF_KEYS   50
>-
>   HKL mKeyboardLayout;
> 
>-  nsVirtualKey mVirtualKeys[NUM_OF_KEYS];
>+  nsVirtualKey mVirtualKeys[50];

This seems like a step backward. Maybe move NUM_OF_KEYS to nsWindowDefs.h?

r+ w/follow ups.

Comment 12

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

Attachment: Patch v1.1 part.4 Use early return style

Thank you, Jimm!

Comment 13

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

Attachment: Patch v1.1 part.5 fix some nits

Comment 14

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

Attachment: Patch v1.1 part.5 fix some nits

Comment 15

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

http://hg.mozilla.org/mozilla-central/rev/2ced7d1fcaf0
http://hg.mozilla.org/mozilla-central/rev/c01b3b542cd7
http://hg.mozilla.org/mozilla-central/rev/836181948c47
http://hg.mozilla.org/mozilla-central/rev/da6064b28483
http://hg.mozilla.org/mozilla-central/rev/4c8820017f9f

Comment 16

[Dave Townsend [:mossop]]

These patches were landed without approval. Please request post-hoc approval and if you don't get it you must back them out.

Comment 17

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

(In reply to comment #16)
> These patches were landed without approval. Please request post-hoc approval
> and if you don't get it you must back them out.

What's the "post-hoc approval"?

Comment 18

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

backed-out for requesting approval.

Comment 19

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

Comment on attachment 453712 [details] [diff] [review]
Patch v1.0 part.1 Adding "ns" prefix to the classes which are defined in nsKeyboardLayout.h

These patches just clean up the nsKeyboardLayout which doesn't use our coding style and unreadable.

These patches has risks, so, we should fix this bug ASAP. But I don't think this bug should be fixed 4.0b2. 4.0b3 is better, probably.

Comment 20

[Dave Townsend [:mossop]]

(In reply to comment #17)
> (In reply to comment #16)
> > These patches were landed without approval. Please request post-hoc approval
> > and if you don't get it you must back them out.
> 
> What's the "post-hoc approval"?

It means you can request approval on patches that were accidentally landed without it to avoid needing to back them out.

Comment 21

[Jim Mathies [:jimm]]

These patches should be r+ for 2.0. They would have landed a week ago had I not been tied up on other things keeping me from reviews. They are primarily cosmetic changes in ime code, and are safe to take on trunk.

Comment 22

[Benjamin Smedberg]

a=me for 2.0. I'm not sure why we're working on this now, though, unless this blocks actual blockers.

Comment 23

[Rob Arnold [:robarnold]]

It makes no sense to me why we are adding the ns prefix to these classes when they should instead be added to the mozilla::widget namespace. It's less noise in history to do so and moreover is not a step backwards in style.

Comment 24

[Rob Arnold [:robarnold]]

Comment on attachment 453712 [details] [diff] [review]
Patch v1.0 part.1 Adding "ns" prefix to the classes which are defined in nsKeyboardLayout.h

This doesn't follow our coding style. See https://developer.mozilla.org/En/Developer_Guide/Coding_Style#C.2b.2b_Namespaces

mozilla::widget is the namespace already in use so it should probably just go in there.

Comment 25

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

The file name is _ns_KeyboardLayout.*.

When I change to use mozilla namespace, what file name should be better?

Comment 26

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

In other words, we should use namespace, we should rename the files.

Comment 27

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

Attachment: Patch v2.0 part.1 Using namespace (mozilla::widget) and rename nsKeyboardLayout.* to KeyboardLayout.*

This patch uses C++ name space for KeyboardLayout and its related classes.

And the file names are changed to KeyboardLayout.cpp and KeyboardLayout.h.

Comment 28

[Rob Arnold [:robarnold]]

Comment on attachment 459732 [details] [diff] [review]
Patch v2.0 part.1 Using namespace (mozilla::widget) and rename nsKeyboardLayout.* to KeyboardLayout.*

Looks great, thanks!

Comment 29

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

Attachment: Patch v1.0.1 part.2 (for check-in)

Comment 30

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

Attachment: Patch v1.0.1 part.3 (for check-in)

Comment 31

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

Attachment: Patch v1.1.1 part.4 (for check-in)

Comment 32

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

Attachment: Patch v1.1.1 part.5 (for check-in)

Comment 33

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

http://hg.mozilla.org/mozilla-central/rev/3a4d81f5c2c0
http://hg.mozilla.org/mozilla-central/rev/0849a93be040
http://hg.mozilla.org/mozilla-central/rev/c9942d8142e7
http://hg.mozilla.org/mozilla-central/rev/cccd41d4095b
http://hg.mozilla.org/mozilla-central/rev/cd5773ba28ed