Cleaning up nsKeyboardLayout which doesn't use our coding style

RESOLVED FIXED in mozilla2.0b3

Status

()

Core
Widget: Win32
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

Trunk
mozilla2.0b3
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 4 obsolete attachments)

18.83 KB, patch
robarnold
: review-
Joe Drew (not getting mail)
: approval2.0+
Details | Diff | Splinter Review
38.58 KB, patch
jimm
: review+
Joe Drew (not getting mail)
: approval2.0+
Details | Diff | Splinter Review
21.66 KB, patch
jimm
: review+
Joe Drew (not getting mail)
: approval2.0+
Details | Diff | Splinter Review
10.65 KB, patch
masayuki
: review+
Joe Drew (not getting mail)
: approval2.0+
Details | Diff | Splinter Review
17.83 KB, patch
masayuki
: review+
Joe Drew (not getting mail)
: approval2.0+
Details | Diff | Splinter Review
17.03 KB, patch
robarnold
: review+
Details | Diff | Splinter Review
38.60 KB, patch
Details | Diff | Splinter Review
21.67 KB, patch
Details | Diff | Splinter Review
10.62 KB, patch
Details | Diff | Splinter Review
18.66 KB, patch
Details | Diff | Splinter Review
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.
Created attachment 453712 [details] [diff] [review]
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.
Attachment #453712 - Flags: review?(jmathies)
Created attachment 453713 [details] [diff] [review]
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.
Attachment #453713 - Flags: review?(jmathies)
Created attachment 453715 [details] [diff] [review]
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;
}

....
Attachment #453715 - Flags: review?(jmathies)
Created attachment 453716 [details] [diff] [review]
Patch v1.0 part.4 Use early return style

We should use early return style for reducing indentation.
Attachment #453716 - Flags: review?(jmathies)
Created attachment 453717 [details] [diff] [review]
Patch v1.0 part.4 (-U 8 -p -W)

You can use this file for review the part.4.
Created attachment 453718 [details] [diff] [review]
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.
Attachment #453718 - Flags: review?(jmathies)

Comment 7

8 years ago
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.
Attachment #453712 - Flags: review?(jmathies) → review+

Comment 8

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

nice!
Attachment #453713 - Flags: review?(jmathies) → review+

Comment 9

8 years ago
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!
Attachment #453715 - Flags: review?(jmathies) → review+

Comment 10

8 years ago
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.
Attachment #453716 - Flags: review?(jmathies) → review+

Comment 11

8 years ago
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.
Attachment #453718 - Flags: review?(jmathies) → review+
Created attachment 458951 [details] [diff] [review]
Patch v1.1 part.4 Use early return style

Thank you, Jimm!
Attachment #453716 - Attachment is obsolete: true
Attachment #453717 - Attachment is obsolete: true
Attachment #458951 - Flags: review+
Created attachment 458952 [details] [diff] [review]
Patch v1.1 part.5 fix some nits
Attachment #453718 - Attachment is obsolete: true
Attachment #458952 - Flags: review+
Created attachment 458954 [details] [diff] [review]
Patch v1.1 part.5 fix some nits
Attachment #458952 - Attachment is obsolete: true
Attachment #458954 - Flags: review+
These patches were landed without approval. Please request post-hoc approval and if you don't get it you must back them out.
(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"?
backed-out for requesting approval.
Status: RESOLVED → REOPENED
blocking2.0: --- → ?
Resolution: FIXED → ---
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.
Attachment #453712 - Flags: approval2.0?
Attachment #453713 - Flags: approval2.0?
Attachment #453715 - Flags: approval2.0?
Attachment #458951 - Flags: approval2.0?
Attachment #458954 - Flags: approval2.0?
(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

8 years ago
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

8 years ago
a=me for 2.0. I'm not sure why we're working on this now, though, unless this blocks actual blockers.
Attachment #453712 - Flags: approval2.0? → approval2.0+
Attachment #453713 - Flags: approval2.0? → approval2.0+
Attachment #453715 - Flags: approval2.0? → approval2.0+
Attachment #458951 - Flags: approval2.0? → approval2.0+
Attachment #458954 - Flags: approval2.0? → approval2.0+
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.
blocking2.0: ? → -
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.
Attachment #453712 - Flags: review+ → review-
The file name is _ns_KeyboardLayout.*.

When I change to use mozilla namespace, what file name should be better?
In other words, we should use namespace, we should rename the files.
blocking2.0: - → ?
blocking2.0: ? → ---
Created attachment 459732 [details] [diff] [review]
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.
Attachment #459732 - Flags: review?(tellrob)
Attachment #459732 - Flags: review?(jmathies)
Comment on attachment 459732 [details] [diff] [review]
Patch v2.0 part.1 Using namespace (mozilla::widget) and rename nsKeyboardLayout.* to KeyboardLayout.*

Looks great, thanks!
Attachment #459732 - Flags: review?(tellrob)
Attachment #459732 - Flags: review?(jmathies)
Attachment #459732 - Flags: review+
Created attachment 460464 [details] [diff] [review]
Patch v1.0.1 part.2 (for check-in)
Created attachment 460465 [details] [diff] [review]
Patch v1.0.1 part.3 (for check-in)
Attachment #460464 - Attachment description: Patch 1.0.1 part.2 (for check-in) → Patch v1.0.1 part.2 (for check-in)
Created attachment 460466 [details] [diff] [review]
Patch v1.1.1 part.4 (for check-in)
Created attachment 460467 [details] [diff] [review]
Patch v1.1.1 part.5 (for check-in)
You need to log in before you can comment on or make changes to this bug.