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.

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

nsKeyboardLayout.* inserted a white space before method's '(' and array's '['. We should remove them. And many lines have over 80 characters.

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; } ....

We should use early return style for reducing indentation.

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

* 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 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 on attachment 453713 [details] [diff] [review] Patch v1.0 part.2 Removing unnecessary white spaces and wrapping long lines nice!

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 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 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.

Thank you, Jimm!

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

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.

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.

(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.

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.

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

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 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.

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.

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 on attachment 459732 [details] [diff] [review] Patch v2.0 part.1 Using namespace (mozilla::widget) and rename nsKeyboardLayout.* to KeyboardLayout.* Looks great, thanks!

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