Remove preprocessing from browser/components/search/

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Gijs, Assigned: aryx)

Tracking

(Blocks: 1 bug)

38 Branch
Firefox 43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
Specifically:

browser/components/search/content/search.xml:# -*- Mode: HTML -*-

Fix or remove this modeline.

browser/components/search/content/search.xml:# This Source Code Form is subject to the terms of the Mozilla Public
browser/components/search/content/search.xml:# License, v. 2.0. If a copy of the MPL was not distributed with this
browser/components/search/content/search.xml:# file, You can obtain one at http://mozilla.org/MPL/2.0/.

The license header can be a comment.


browser/components/search/content/search.xml:#ifdef XP_MACOSX
browser/components/search/content/search.xml:#else
browser/components/search/content/search.xml:#endif
browser/components/search/content/search.xml:#ifdef XP_MACOSX
browser/components/search/content/search.xml:#else
browser/components/search/content/search.xml:#endif

Both of these are about ctrlKey vs. metaKey. I believe they can be replaced by just:

event.getModifierState("Accel")

browser/components/search/content/search.xml:#ifndef XP_MACOSX
browser/components/search/content/search.xml:#endif

This can be converted to a simple JS handler that gets attached in the constructor of the binding, or can be made to no-op (ensuring the event still gets propagated and handled correctly elsewhere!) on OSX.

Finally, this file needs to have its "*" removed in jar.mn
Created attachment 8652327 [details] [diff] [review]
patch, v1
Assignee: nobody → aryx.bugmail
Status: NEW → ASSIGNED
Attachment #8652327 - Flags: review?(gijskruitbosch+bugs)
(Reporter)

Comment 2

3 years ago
Comment on attachment 8652327 [details] [diff] [review]
patch, v1

Review of attachment 8652327 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, but see comments below.

::: browser/components/search/content/search.xml
@@ +492,5 @@
>              if (((aEvent instanceof KeyboardEvent) && aEvent.altKey) ^ newTabPref)
>                where = "tab";
> +            if ((aEvent instanceof MouseEvent) &&
> +                (aEvent.button == 1 || aEvent.getModifierState("Accel")))
> +            {

Nit: brace on end of previous line.

@@ +791,5 @@
>      <implementation implements="nsIObserver">
>        <constructor><![CDATA[
> +        Components.utils.import('resource://gre/modules/XPCOMUtils.jsm');
> +        XPCOMUtils.defineLazyModuleGetter(this, "AppConstants",
> +                                          "resource://gre/modules/AppConstants.jsm");

Looking at the net results for this file, I don't think we need to do this, see below.

@@ +871,5 @@
>          this._suggestMenuItem = element;
>          cxmenu.appendChild(element);
>  
> +        this.addEventListener("keypress",
> +          aEvent => { if (AppConstants.platform == "macosx" && aEvent.keyCode == KeyEvent.VK_F4) this.openSearch() }, true);

Nit: please re-wrap:

this.addEventListener("keypress", aEvent => {
  if (...)
    this.openSearch();
}, true);


tbh, considering this is the only use for this platform check in this file, the binding is only used for the search bar, and we don't need appconstants for anything else, I think you could just do:

navigator.platform.startsWith("Mac")

to check for OS X.
Attachment #8652327 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/4b4a9445706b
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
You need to log in before you can comment on or make changes to this bug.