Add GTK support for global Extension Commands

Add GTK support for global Extension Commands (CL based on Finnur's CL).

BUG=TBD

[Finnur, 2013-09-27 10:35:02]

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
File chrome/browser/extensions/global_shortcut_listener_gtk.cc (right):

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
chrome/browser/extensions/global_shortcut_listener_gtk.cc:22: static const
unsigned int kModifiersMasks[] = {
I would comment this with why we're keeping track of NUM_LOCK and friends and
why it is important to have all permutations of them.

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
chrome/browser/extensions/global_shortcut_listener_gtk.cc:23: 0,                
               // only specified modifiers.
I would rephrase this: No additional modifier.

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
chrome/browser/extensions/global_shortcut_listener_gtk.cc:64: if
(!is_listening_) {
In general, we prefer:

if (foo)
  return;
bar();

... instead of ...

if (!foo) {
  bar();
}

However, I don't think the if is needed since the DCHECK is enough. This is
basically a trade-off between gracefully handling errors everywhere vs.
minimizing the amount of code. 

The Chromium philosophy is the latter in general -- and you'll perhaps also note
(elsewhere in the codebase) that we prefer to crash on NULL pointer
de-referencing rather than add if-checks everywhere.

Same with StopListening.

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
chrome/browser/extensions/global_shortcut_listener_gtk.cc:91: // Because
XGrabKey only works on the exact modifiers mask, we should register
Yikes. :) That's unfortunate. :)
What about mod3 and mod4? Is there a mod6?

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
chrome/browser/extensions/global_shortcut_listener_gtk.cc:100: if
(err_tracker.FoundNewError()) {
So, what are the possible error scenarios here?

I think you already mentioned this will not fail if another app has registered
for the same shortcut, correct? No, wait -- maybe I'm misremembering, since this
doc (http://tronche.com/gui/x/xlib/input/XGrabKey.html) seems to indicate that
we'll get an error if there is a conflict.

In that case we have to think about partial success and how to handle that. For
example, if I ask for Ctrl+Shift+E and it fully succeeds in registering, except
for Ctrl+Shift+E+NumLock (because someone else registered for just that
shortcut). Are we better off not registering for any Ctrl+Shift+E combination at
all (current approach) or ignoring Ctrl+Shift+E+NumLock? I'm leaning towards the
current approach, but I'm not sure.

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
chrome/browser/extensions/global_shortcut_listener_gtk.cc:102: <<
accelerator.GetShortcutText();
Add the error number?

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
File chrome/browser/extensions/global_shortcut_listener_gtk.h (right):

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
chrome/browser/extensions/global_shortcut_listener_gtk.h:40: // Callback for
XEvents of the default root window.
nit: Line break above this line.

[zhchbin, 2013-09-27 13:47:20]

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
File chrome/browser/extensions/global_shortcut_listener_gtk.cc (right):

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
chrome/browser/extensions/global_shortcut_listener_gtk.cc:22: static const
unsigned int kModifiersMasks[] = {
On 2013/09/27 10:35:02, Finnur wrote:
> I would comment this with why we're keeping track of NUM_LOCK and friends and
> why it is important to have all permutations of them.

Done.

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
chrome/browser/extensions/global_shortcut_listener_gtk.cc:23: 0,                
               // only specified modifiers.
On 2013/09/27 10:35:02, Finnur wrote:
> I would rephrase this: No additional modifier.

Done.

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
chrome/browser/extensions/global_shortcut_listener_gtk.cc:64: if
(!is_listening_) {
On 2013/09/27 10:35:02, Finnur wrote:
> In general, we prefer:
> 
> if (foo)
>   return;
> bar();
> 
> ... instead of ...
> 
> if (!foo) {
>   bar();
> }
> 
> However, I don't think the if is needed since the DCHECK is enough. This is
> basically a trade-off between gracefully handling errors everywhere vs.
> minimizing the amount of code. 
> 
> The Chromium philosophy is the latter in general -- and you'll perhaps also
note
> (elsewhere in the codebase) that we prefer to crash on NULL pointer
> de-referencing rather than add if-checks everywhere.
> 
> Same with StopListening.

Done.

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
chrome/browser/extensions/global_shortcut_listener_gtk.cc:91: // Because
XGrabKey only works on the exact modifiers mask, we should register
On 2013/09/27 10:35:02, Finnur wrote:
> Yikes. :) That's unfortunate. :)
> What about mod3 and mod4? Is there a mod6?

Yeah, this problem has obsessed me since I working on similar feature of
node-webkit.
Mod3Mask is "right alt", and Mod4Mask is the windows key. Generally speaking,
these two keys have no state like NumLock.

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
chrome/browser/extensions/global_shortcut_listener_gtk.cc:100: if
(err_tracker.FoundNewError()) {
On 2013/09/27 10:35:02, Finnur wrote:
> So, what are the possible error scenarios here?
> 
> I think you already mentioned this will not fail if another app has registered
> for the same shortcut, correct? No, wait -- maybe I'm misremembering, since
this
> doc (http://tronche.com/gui/x/xlib/input/XGrabKey.html) seems to indicate that
> we'll get an error if there is a conflict.

Yes, we will get BadAccess if there is a conflict. But the err_tracker didn't
expose this error code to us. It seems that we only concern about the result
here.

> In that case we have to think about partial success and how to handle that.
For
> example, if I ask for Ctrl+Shift+E and it fully succeeds in registering,
except
> for Ctrl+Shift+E+NumLock (because someone else registered for just that
> shortcut). Are we better off not registering for any Ctrl+Shift+E combination
at
> all (current approach) or ignoring Ctrl+Shift+E+NumLock? I'm leaning towards
the
> current approach, but I'm not sure.

The reason why I choose current approach: keep consistence with "RegisterHotKey"
on windows. And if we ignore Ctrl+Shift+E+NumLock and continue, something wired
happen: we are reporting we register hot key successfully, but it works only
when the NumLock is on.

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
chrome/browser/extensions/global_shortcut_listener_gtk.cc:102: <<
accelerator.GetShortcutText();
On 2013/09/27 10:35:02, Finnur wrote:
> Add the error number?

It seems that we couldn't get the error number if using base::X11ErrorTracker
here. But we can change the X11ErrorTracker to get the X error code, we really
need it? For what? :)

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
File chrome/browser/extensions/global_shortcut_listener_gtk.h (right):

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
chrome/browser/extensions/global_shortcut_listener_gtk.h:40: // Callback for
XEvents of the default root window.
On 2013/09/27 10:35:02, Finnur wrote:
> nit: Line break above this line.

Done.

[Finnur, 2013-09-27 14:01:26]

Pretty happy with how this is turning out on GTK. Thanks for your work on this.
Do you want me to incorporate this into my changelist (and attribute it to you)
or do you want to do a followup CL after I check my stuff in?

Besides that question, just a couple of nits:

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
File chrome/browser/extensions/global_shortcut_listener_gtk.cc (right):

https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
chrome/browser/extensions/global_shortcut_listener_gtk.cc:102: <<
accelerator.GetShortcutText();
I thought we had the error, but I guess not. That's fine. No worries.

https://codereview.chromium.org/25007002/diff/5001/chrome/browser/extensions/...
File chrome/browser/extensions/global_shortcut_listener_gtk.cc (right):

https://codereview.chromium.org/25007002/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/global_shortcut_listener_gtk.cc:22: // The modifiers
masks used for grabing keys. Due to XGrabKey only works on
s/works/working/

https://codereview.chromium.org/25007002/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/global_shortcut_listener_gtk.cc:23: // exact
modifiers, we need to grab all key combination including zero or more
s/more/more of the/

https://codereview.chromium.org/25007002/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/global_shortcut_listener_gtk.cc:24: // following lock
keys: Num lock, Caps lock and Scroll lock. So that we can make
Remove 'lock keys'.

https://codereview.chromium.org/25007002/diff/5001/chrome/browser/extensions/...
chrome/browser/extensions/global_shortcut_listener_gtk.cc:25: // sure the
behavior of global command keep consistency with other platforms.
End this with: "behavior of global shortcuts is consistent on all platforms".

[zhchbin, 2013-09-27 14:39:36]

On 2013/09/27 14:01:26, Finnur wrote:
> Pretty happy with how this is turning out on GTK. Thanks for your work on
this.
> Do you want me to incorporate this into my changelist (and attribute it to
you)
> or do you want to do a followup CL after I check my stuff in?
> 
> Besides that question, just a couple of nits:
> 
>
https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
> File chrome/browser/extensions/global_shortcut_listener_gtk.cc (right):
> 
>
https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/glo...
> chrome/browser/extensions/global_shortcut_listener_gtk.cc:102: <<
> accelerator.GetShortcutText();
> I thought we had the error, but I guess not. That's fine. No worries.
> 
>
https://codereview.chromium.org/25007002/diff/5001/chrome/browser/extensions/...
> File chrome/browser/extensions/global_shortcut_listener_gtk.cc (right):
> 
>
https://codereview.chromium.org/25007002/diff/5001/chrome/browser/extensions/...
> chrome/browser/extensions/global_shortcut_listener_gtk.cc:22: // The modifiers
> masks used for grabing keys. Due to XGrabKey only works on
> s/works/working/
> 
>
https://codereview.chromium.org/25007002/diff/5001/chrome/browser/extensions/...
> chrome/browser/extensions/global_shortcut_listener_gtk.cc:23: // exact
> modifiers, we need to grab all key combination including zero or more
> s/more/more of the/
> 
>
https://codereview.chromium.org/25007002/diff/5001/chrome/browser/extensions/...
> chrome/browser/extensions/global_shortcut_listener_gtk.cc:24: // following
lock
> keys: Num lock, Caps lock and Scroll lock. So that we can make
> Remove 'lock keys'.
> 
>
https://codereview.chromium.org/25007002/diff/5001/chrome/browser/extensions/...
> chrome/browser/extensions/global_shortcut_listener_gtk.cc:25: // sure the
> behavior of global command keep consistency with other platforms.
> End this with: "behavior of global shortcuts is consistent on all platforms".

Thanks for the review. Will update tomorrow, just back from my lab (My computer
for developing is there).  I prefer a followup CL. It would be better due to the
difference of our timezone.

[Finnur, 2013-09-27 14:55:49]

No problem.


On Fri, Sep 27, 2013 at 2:39 PM, <zhchbin@gmail.com> wrote:

> On 2013/09/27 14:01:26, Finnur wrote:
>
>> Pretty happy with how this is turning out on GTK. Thanks for your work on
>>
> this.
>
>> Do you want me to incorporate this into my changelist (and attribute it to
>>
> you)
>
>> or do you want to do a followup CL after I check my stuff in?
>>
>
>  Besides that question, just a couple of nits:
>>
>
>
> https://codereview.chromium.**org/25007002/diff/1/chrome/**
>
browser/extensions/global_**shortcut_listener_gtk.cc<https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/global_shortcut_listener_gtk.cc>
>
>> File chrome/browser/extensions/**global_shortcut_listener_gtk.**cc
>> (right):
>>
>
>
> https://codereview.chromium.**org/25007002/diff/1/chrome/**
>
browser/extensions/global_**shortcut_listener_gtk.cc#**newcode102<https://codereview.chromium.org/25007002/diff/1/chrome/browser/extensions/global_shortcut_listener_gtk.cc#newcode102>
>
>> chrome/browser/extensions/**global_shortcut_listener_gtk.**cc:102: <<
>> accelerator.GetShortcutText();
>> I thought we had the error, but I guess not. That's fine. No worries.
>>
>
>
> https://codereview.chromium.**org/25007002/diff/5001/chrome/**
>
browser/extensions/global_**shortcut_listener_gtk.cc<https://codereview.chromium.org/25007002/diff/5001/chrome/browser/extensions/global_shortcut_listener_gtk.cc>
>
>> File chrome/browser/extensions/**global_shortcut_listener_gtk.**cc
>> (right):
>>
>
>
> https://codereview.chromium.**org/25007002/diff/5001/chrome/**
>
browser/extensions/global_**shortcut_listener_gtk.cc#**newcode22<https://codereview.chromium.org/25007002/diff/5001/chrome/browser/extensions/global_shortcut_listener_gtk.cc#newcode22>
>
>> chrome/browser/extensions/**global_shortcut_listener_gtk.**cc:22: // The
>> modifiers
>> masks used for grabing keys. Due to XGrabKey only works on
>> s/works/working/
>>
>
>
> https://codereview.chromium.**org/25007002/diff/5001/chrome/**
>
browser/extensions/global_**shortcut_listener_gtk.cc#**newcode23<https://codereview.chromium.org/25007002/diff/5001/chrome/browser/extensions/global_shortcut_listener_gtk.cc#newcode23>
>
>> chrome/browser/extensions/**global_shortcut_listener_gtk.**cc:23: //
>> exact
>> modifiers, we need to grab all key combination including zero or more
>> s/more/more of the/
>>
>
>
> https://codereview.chromium.**org/25007002/diff/5001/chrome/**
>
browser/extensions/global_**shortcut_listener_gtk.cc#**newcode24<https://codereview.chromium.org/25007002/diff/5001/chrome/browser/extensions/global_shortcut_listener_gtk.cc#newcode24>
>
>> chrome/browser/extensions/**global_shortcut_listener_gtk.**cc:24: //
>> following
>>
> lock
>
>> keys: Num lock, Caps lock and Scroll lock. So that we can make
>> Remove 'lock keys'.
>>
>
>
> https://codereview.chromium.**org/25007002/diff/5001/chrome/**
>
browser/extensions/global_**shortcut_listener_gtk.cc#**newcode25<https://codereview.chromium.org/25007002/diff/5001/chrome/browser/extensions/global_shortcut_listener_gtk.cc#newcode25>
>
>> chrome/browser/extensions/**global_shortcut_listener_gtk.**cc:25: //
>> sure the
>> behavior of global command keep consistency with other platforms.
>> End this with: "behavior of global shortcuts is consistent on all
>> platforms".
>>
>
> Thanks for the review. Will update tomorrow, just back from my lab (My
> computer
> for developing is there).  I prefer a followup CL. It would be better due
> to the
> difference of our timezone.
>
>
https://codereview.chromium.**org/25007002/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.