Bug 120657

<rdar://problem/14903833>

It would be nice to make this automatic so we don't need to declare multiple shortcuts per platform. Something like Command key auto-maps to Control on non-Mac platforms?

Yea, I was thinking of WebInspector.KeyboardShortcut.Modifier.CommandOrControl which does expected per platform.

(In reply to comment #3) > Yea, I was thinking of WebInspector.KeyboardShortcut.Modifier.CommandOrControl which does expected per platform. That's what I want to do, I put EFL specifc in the bug because it need some work on MiniBrowser(WK2) and EWebLauncher(WK2) too :)

Sorry, I mean that I am working on EFL browsers too, to test this first.

Hello, In Source/WebInspectorUI/UserInterface/ContentBrowser.js we have this: ... if (!disableBackForward) { this._backKeyboardShortcut = new WebInspector.KeyboardShortcut(WebInspector.KeyboardShortcut.Modifier.Command | WebInspector.KeyboardShortcut.Modifier.Control, WebInspector.KeyboardShortcut.Key.Left, this._backButtonClicked.bind(this)); this._forwardKeyboardShortcut = new WebInspector.KeyboardShortcut(WebInspector.KeyboardShortcut.Modifier.Command | WebInspector.KeyboardShortcut.Modifier.Control, WebInspector.KeyboardShortcut.Key.Right, this._forwardButtonClicked.bind(this)); ... How could we move this to non-Mac platforms? I would like some suggestions. :)

(In reply to comment #6) > How could we move this to non-Mac platforms? > > I would like some suggestions. :) The earlier comment was a suggestion! For systems that want to use Control instead of Command we can add "WebInspector.KeyboardShortcut.Modifier.CommandOrControl" and map that value to the appropriate value ("Command" or "Control") for the platform that is running the frontend (InspectorFrontendHost.platform()).

(In reply to comment #7) > (In reply to comment #6) > > How could we move this to non-Mac platforms? > > > > I would like some suggestions. :) > > The earlier comment was a suggestion! > > For systems that want to use Control instead of Command we can add "WebInspector.KeyboardShortcut.Modifier.CommandOrControl" and map that value to the appropriate value ("Command" or "Control") for the platform that is running the frontend (InspectorFrontendHost.platform()). Hi Joseph, I am already following your suggestion :) I mean suggestions for these specific shortcuts: Command + Control + left and Command + Control + right How could we map this to non-Mac platforms?

(In reply to comment #8) > (In reply to comment #7) > > (In reply to comment #6) > > > How could we move this to non-Mac platforms? > > > > > > I would like some suggestions. :) > > > > The earlier comment was a suggestion! > > > > For systems that want to use Control instead of Command we can add "WebInspector.KeyboardShortcut.Modifier.CommandOrControl" and map that value to the appropriate value ("Command" or "Control") for the platform that is running the frontend (InspectorFrontendHost.platform()). > > Hi Joseph, I am already following your suggestion :) > > I mean suggestions for these specific shortcuts: > > Command + Control + left > and > Command + Control + right > > > How could we map this to non-Mac platforms? I would just do: WebInspector.KeyboardShortcut.Modifier.CommandOrControl | WebInspector.KeyboardShortcut.Modifier.Control, WebInspector.KeyboardShortcut.Key.Left Then ignore WebInspector.KeyboardShortcut.Modifier.Control if WebInspector.KeyboardShortcut.Modifier.CommandOrControl becomes Control.

Created attachment 212204 [details] Patch I followed Timothy's suggestions for the case above. It works! :) Tested the patch on Mac and Linux.

Comment on attachment 212204 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212204&action=review > Source/WebInspectorUI/UserInterface/KeyboardShortcut.js:96 > + return (InspectorFrontendHost.platform() === "mac") ? this.Command : this.Control; No need for the ().

(In reply to comment #11) > (From update of attachment 212204 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212204&action=review > > > Source/WebInspectorUI/UserInterface/KeyboardShortcut.js:96 > > + return (InspectorFrontendHost.platform() === "mac") ? this.Command : this.Control; > > No need for the (). Thanks for the reviewing, should I remove this () then? I can do it quickly :)

Comment on attachment 212204 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212204&action=review >>> Source/WebInspectorUI/UserInterface/KeyboardShortcut.js:96 >>> + return (InspectorFrontendHost.platform() === "mac") ? this.Command : this.Control; >> >> No need for the (). > > Thanks for the reviewing, should I remove this () then? I can do it quickly :) Sure.

Comment on attachment 212204 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212204&action=review >>> Source/WebInspectorUI/UserInterface/KeyboardShortcut.js:96 >>> + return (InspectorFrontendHost.platform() === "mac") ? this.Command : this.Control; >> >> No need for the (). > > Thanks for the reviewing, should I remove this () then? I can do it quickly :) Sure.

Comment on attachment 212204 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=212204&action=review >>> Source/WebInspectorUI/UserInterface/KeyboardShortcut.js:96 >>> + return (InspectorFrontendHost.platform() === "mac") ? this.Command : this.Control; >> >> No need for the (). > > Thanks for the reviewing, should I remove this () then? I can do it quickly :) I don't think we want to run this getter every time someone uses CommandOrControl. I suggest we just set it once. The IFH.platform() is not going to change. How about: WebInspector.KeyboardShortcut.Modifier = { … }; WebInspector.KeyboardShortcut.Modifier.CommandOrControl = InspectorFrontendHost.platform() === "mac" ? WebInspector.KeyboardShortcut.Modifier.Command : WebInspector.KeyboardShortcut.Modifier.Control; This only does the work once, ever, and should have the same result.

Created attachment 212213 [details] patch Removed () like Timothy said above.

Comment on attachment 212213 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=212213&action=review I fixed this issues and landed as http://trac.webkit.org/changeset/156198. > Source/WebInspectorUI/ChangeLog:3 > +2013-09-20 Marcelo Morais <m.morais@samsung.com> > + > + 2013-09-19 Marcelo Morais <m.morais@samsung.com> Should be only one header and not double indented. > Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:147 > + var os = (navigator.platform.match(/mac|win|linux/i) || ["other"])[0].toLowerCase(); > + return os; Can be one line. > Source/WebInspectorUI/UserInterface/KeyboardShortcut.js:96 > + return (InspectorFrontendHost.platform === "mac") ? this.Command : this.Control; platform is a function not a getter. So this is comparing the function as a string to "mac" which will always be false. This should be platform(). Also the () is still there.

(In reply to comment #17) > (From update of attachment 212213 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=212213&action=review > > I fixed this issues and landed as http://trac.webkit.org/changeset/156198. > > > Source/WebInspectorUI/ChangeLog:3 > > +2013-09-20 Marcelo Morais <m.morais@samsung.com> > > + > > + 2013-09-19 Marcelo Morais <m.morais@samsung.com> > > Should be only one header and not double indented. > > > Source/WebInspectorUI/UserInterface/InspectorFrontendHostStub.js:147 > > + var os = (navigator.platform.match(/mac|win|linux/i) || ["other"])[0].toLowerCase(); > > + return os; > > Can be one line. > > > Source/WebInspectorUI/UserInterface/KeyboardShortcut.js:96 > > + return (InspectorFrontendHost.platform === "mac") ? this.Command : this.Control; > > platform is a function not a getter. So this is comparing the function as a string to "mac" which will always be false. This should be platform(). Also the () is still there. Thanks Timothy! :)