Closed Bug 1186017 Opened 4 years ago Closed 4 years ago

[GTK] nsGtkIMModule should be renamed to mozilla::widget::IMContextWrapper

Categories

(Core :: Widget: Gtk, enhancement)

x86
Linux
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

Details

(Keywords: inputmethod)

Attachments

(2 files)

I think that nsGtkIMModule should be renamed to mozilla::widget::IMContextWrapper or something. It seems that "Gtk" is redundant and ending with "Module" isn't clear what it does. Currently, we're using a class named KeymapWrapper for handling keyboard event. Therefore, I think that IMContextWrapper is better name in widget/gtk/.

If you have better idea, let me know.
Summary: [GTK] nsGtkIMModule should be renamed to mozilla::widget::IMModuleHandler → [GTK] nsGtkIMModule should be renamed to mozilla::widget::IMContextWrapper
Renaming widget/gtk/nsGtkIMModule.* -> widget/gtk/IMContextWrapper.*

Replacing:

nsGtkIMModule -> (mozilla::widget::)IMContextWrapper
"GtkIMModule(%p):" in the logs -> "GTKIM: %p "
sLastFocusedModule -> sLastFocusedContext

And improving some log text.
Attachment #8637793 - Flags: review?(m_kato)
Attachment #8637793 - Flags: review?(karlt)
Ah, and also:

Foo *aBar or Foo &aBar -> Foo* aBar or Foo& aBar
Attachment #8637793 - Flags: review?(m_kato) → review+
Comment on attachment 8637793 [details] [diff] [review]
Rename nsGtkIMModule to mozilla::widget::IMContextWrapper

>+    'IMContextWrapper.cpp', # methods for logging conflict with other files
>+    'nsGtkKeyUtils.cpp', # methods for logging conflict with other files

Which methods are these?
Is this conflict caused by the rename or namespace change?

People spent some time making unified sources work, so I don't want to revert that.  If you can find a solution there, then the rest is good.
Flags: needinfo?(masayuki)
Attachment #8637793 - Flags: review?(karlt) → review-
Comment on attachment 8637793 [details] [diff] [review]
Rename nsGtkIMModule to mozilla::widget::IMContextWrapper

I filed bug 1186821 for that.

If you think that we should keep unifiedable forcibly, I can move them into different namespace temporarily. Do you want that?
Flags: needinfo?(masayuki) → needinfo?(karlt)
> Which methods are these?

E.g., GetBoolName().

> Is this conflict caused by the rename or namespace change?

Yes. They are not in same namespace.
Would static inline GetBoolName() resolve the problem until a common location can be found for the function?  Each definition would need to be the same.
Flags: needinfo?(karlt) → needinfo?(masayuki)
(In reply to Karl Tomlinson (ni?:karlt) from comment #7)
> Would static inline GetBoolName() resolve the problem until a common
> location can be found for the function?

Unfortunately, no.

> Each definition would need to be the same.

Yes, of course.

And I tried with like using namespace hack:

namespace mozilla {
namespace widget {

namespace keymapwrappehelper {

} // namespace keymapwrappehelper

using namespace keymapwrappehelper;

} // namespace widget
} // namespace mozilla

namespace mozilla {
namespace widget {

namespace imcontextwrapperhelper {

} // namespace imcontextwrapperhelper 

using namespace imcontextwrapperhelper;

} // namespace widget
} // namespace mozilla

However, gcc doesn't stop |using namespace keymapwrappehelper;| when widget namespace is closed. So, in IMContextWrapper.cpp, |using namespace keymapwrappehelper;| is alive :-(

Currently, the only GetBoolName() conflicts. Perhaps, giving different name is better solution for now. How about you?
Flags: needinfo?(masayuki) → needinfo?(karlt)
Comment on attachment 8637793 [details] [diff] [review]
Rename nsGtkIMModule to mozilla::widget::IMContextWrapper

I don't know what the right solution is.
Perhaps something like WidgetUtils.cpp would be OK.

Other options like macros and renaming will probably just make this harder to move later, so let's go with this for now, but can you move one of the files, probably nsGtkKeyUtils because that is not changed significantly in this patch, back into UNIFIED_SOURCES please?
Flags: needinfo?(karlt)
Attachment #8637793 - Flags: review- → review+
Comment on attachment 8638392 [details] [diff] [review]
part.2 Rename GetBoolName() in IMContextWrapper.cpp to ToChar()

I don't mind if you want to do this for now, assuming you intend to find a suitable place to put these common utilities before adding more copies of the same function, but I'm not sure it is worth it.  Please ensure you have read and considered comment 9.

>-static const char*
>-GetBoolName(bool aBool)
>+static inline const char*
>+ToChar(bool aBool)
Attachment #8638392 - Flags: review?(karlt) → review+
> Please ensure you have read and considered comment 9.

Yeah, of course. I don't like to use macro because if it will need to create a class derived from nsAutoCString, it's impossible. And I don't mind to rename the method for now because it's better for reducing build time.
https://hg.mozilla.org/mozilla-central/rev/3e83a54b7fd2
https://hg.mozilla.org/mozilla-central/rev/58e46bf71dd1
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.