linux: Move linux_util.h from gfx/ to ui/base/gtk.

linux: Move linux_util.h from gfx/ to ui/base/gtk.

The reason for this, is that I don't think this belongs to gfx/ directory. Also rename it to
menu_label_accelerator_util.* to make it clear what the conversion functions are about.

TEST=ui_unittests --gtest_filter=MenuLabelAcceleratorUtilTest*
R=tony@chromium.org
TBR=erg@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=135373

[tony, 2012-05-03 16:42:48]

These methods don't really have anything to do with localization. It has to do
with platform specific accelerator symbols.

That said, you could probably move the contents of linux_util.h into
ui/gfx/gtk_util.h.  Certainly ConvertAcceleratorsFromWindowsStyle and
EscapeWindowsStyleAccelerators are GTK+ specific helper functions.

RemoveWindowsStyleAccelerators is a bit more general purpose (like the mac
FixUpWindowsStyleLabel function), but it looks like it currently is only used by
code in chrome/browser/ui/gtk/ so I think it's OK to put it in gtk_util.h.

[tfarina, 2012-05-03 18:10:56]

On Thu, May 3, 2012 at 1:42 PM,  <tony@chromium.org> wrote:
> These methods don't really have anything to do with localization.
:/

> It has to do with platform specific accelerator symbols.
>
Yes.

> That said, you could probably move the contents of linux_util.h into
> ui/gfx/gtk_util.h.
I don't think graphics (ui/gfx/) is the right place for this.

>  Certainly ConvertAcceleratorsFromWindowsStyle and
> EscapeWindowsStyleAccelerators are GTK+ specific helper functions.
>
Or l10n, but you said it doesn't have anything to do with
localization, how about ui/base/gtk/gtk_util.h or
chrome/browser/ui/gtk_util.h so?

> RemoveWindowsStyleAccelerators is a bit more general purpose (like the mac
> FixUpWindowsStyleLabel function),
If I recall correctly that was also one of the reasons for me to move
it to l10n_util_linux, because of the presence of
FixUpWindowsStyleLabel on mac. I even thought in renaming the former
in some patch but I didn't get to it until now.

> but it looks like it currently is only
> used by
> code in chrome/browser/ui/gtk/ so I think it's OK to put it in gtk_util.h.
>
> http://codereview.chromium.org/10316013/



-- 
Thiago

[tfarina, 2012-05-03 18:12:23]

On Thu, May 3, 2012 at 3:10 PM, Thiago Farina <tfarina@chromium.org> wrote:
> On Thu, May 3, 2012 at 1:42 PM,  <tony@chromium.org> wrote:
>> These methods don't really have anything to do with localization.
> :/
>
>> It has to do with platform specific accelerator symbols.
>>
Now that you said accelerators and thinking one second more about
this, may be a better place would be ui/base/accelerators/. Since all
it is about is fixing windows style accelerator.

> Yes.
>
>> That said, you could probably move the contents of linux_util.h into
>> ui/gfx/gtk_util.h.
> I don't think graphics (ui/gfx/) is the right place for this.
>
>>  Certainly ConvertAcceleratorsFromWindowsStyle and
>> EscapeWindowsStyleAccelerators are GTK+ specific helper functions.
>>
> Or l10n, but you said it doesn't have anything to do with
> localization, how about ui/base/gtk/gtk_util.h or
> chrome/browser/ui/gtk_util.h so?
>
>> RemoveWindowsStyleAccelerators is a bit more general purpose (like the mac
>> FixUpWindowsStyleLabel function),
> If I recall correctly that was also one of the reasons for me to move
> it to l10n_util_linux, because of the presence of
> FixUpWindowsStyleLabel on mac. I even thought in renaming the former
> in some patch but I didn't get to it until now.
>
>> but it looks like it currently is only
>> used by
>> code in chrome/browser/ui/gtk/ so I think it's OK to put it in gtk_util.h.
>>
>> http://codereview.chromium.org/10316013/
>
>
>
> --
> Thiago



-- 
Thiago

[tfarina, 2012-05-03 18:14:23]

Perphas something along this line:
ui/base/accelerators/accelerator_gtk_util.h

On Thu, May 3, 2012 at 3:12 PM, Thiago Farina <tfarina@chromium.org> wrote:
> On Thu, May 3, 2012 at 3:10 PM, Thiago Farina <tfarina@chromium.org> wrote:
>> On Thu, May 3, 2012 at 1:42 PM,  <tony@chromium.org> wrote:
>>> These methods don't really have anything to do with localization.
>> :/
>>
>>> It has to do with platform specific accelerator symbols.
>>>
> Now that you said accelerators and thinking one second more about
> this, may be a better place would be ui/base/accelerators/. Since all
> it is about is fixing windows style accelerator.
>
>> Yes.
>>
>>> That said, you could probably move the contents of linux_util.h into
>>> ui/gfx/gtk_util.h.
>> I don't think graphics (ui/gfx/) is the right place for this.
>>
>>>  Certainly ConvertAcceleratorsFromWindowsStyle and
>>> EscapeWindowsStyleAccelerators are GTK+ specific helper functions.
>>>
>> Or l10n, but you said it doesn't have anything to do with
>> localization, how about ui/base/gtk/gtk_util.h or
>> chrome/browser/ui/gtk_util.h so?
>>
>>> RemoveWindowsStyleAccelerators is a bit more general purpose (like the mac
>>> FixUpWindowsStyleLabel function),
>> If I recall correctly that was also one of the reasons for me to move
>> it to l10n_util_linux, because of the presence of
>> FixUpWindowsStyleLabel on mac. I even thought in renaming the former
>> in some patch but I didn't get to it until now.
>>
>>> but it looks like it currently is only
>>> used by
>>> code in chrome/browser/ui/gtk/ so I think it's OK to put it in gtk_util.h.
>>>
>>> http://codereview.chromium.org/10316013/
>>
>>
>>
>> --
>> Thiago
>
>
>
> --
> Thiago



-- 
Thiago

[tony, 2012-05-03 18:25:39]

ui/base/accelerators are a different type of accelerator.  I don't think we want
to put this there.

chrome/browser/ui/gtk/gtk_util.h would be good, except we use this header in
chrome/browser/tab_contents, which shouldn't reach into chrome/browser/ui.

I think ui/base/gtk/ would be an OK place for these.  I would maybe rename the
files from linux_util.* to menu_label_accelerator_util.* or something.

[tfarina, 2012-05-03 18:54:25]

On 2012/05/03 18:25:39, tony wrote:
> I think ui/base/gtk/ would be an OK place for these.  I would maybe rename the
> files from linux_util.* to menu_label_accelerator_util.* or something.
Moved to there. I like menu_label_accelerator_util.*

http://codereview.chromium.org/10316013/diff/8003/chrome/browser/ui/gtk/menu_...
File chrome/browser/ui/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/10316013/diff/8003/chrome/browser/ui/gtk/menu_...
chrome/browser/ui/gtk/menu_gtk.cc:310: GtkWidget *toplevel_window =
gtk_widget_get_toplevel(menu_);
nit: GtkWidget* :)

http://codereview.chromium.org/10316013/diff/8003/chrome/browser/ui/gtk/menu_...
chrome/browser/ui/gtk/menu_gtk.cc:317: std::string converted_label =
ui::ConvertAcceleratorsFromWindowsStyle(label);
nit: this is unused :/

http://codereview.chromium.org/10316013/diff/8003/chrome/browser/ui/gtk/menu_...
chrome/browser/ui/gtk/menu_gtk.cc:318: GtkWidget* menu_item =
BuildMenuItemWithLabel(label, command_id);
shouldn't this be converted_label?

[tony, 2012-05-03 19:14:32]

LGTM

http://codereview.chromium.org/10316013/diff/8003/chrome/browser/ui/gtk/menu_...
File chrome/browser/ui/gtk/menu_gtk.cc (right):

http://codereview.chromium.org/10316013/diff/8003/chrome/browser/ui/gtk/menu_...
chrome/browser/ui/gtk/menu_gtk.cc:310: GtkWidget *toplevel_window =
gtk_widget_get_toplevel(menu_);
On 2012/05/03 18:54:26, tfarina wrote:
> nit: GtkWidget* :)

Feel free to fix it in this patch.

http://codereview.chromium.org/10316013/diff/8003/chrome/browser/ui/gtk/menu_...
chrome/browser/ui/gtk/menu_gtk.cc:318: GtkWidget* menu_item =
BuildMenuItemWithLabel(label, command_id);
On 2012/05/03 18:54:26, tfarina wrote:
> shouldn't this be converted_label?

That's interesting.  We should fix in a separate patch.  Maybe we're double
accounting for this in chrome/browser/ui/gtk/*.