1494274 - Port clickSelectsAll and doubleClickSelectsAll behavior from urlbarBindings.xml to UrlbarInput.jsm

Status: RESOLVED FIXED

(Firefox :: Address Bar, task, P2)

Comment 1

[Dão Gottwald [:dao]]

Attachment: patch

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 9012809 [details] [diff] [review]
patch

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

I'm a bit worried by the lack of tests, this rewrite is a good occasion to improve on coverage, I think we should spend some time on those, especially for optional behaviors that are usually uncovered. That said, it's possible this is covered by existing tests we'll convert in the future?

::: browser/components/urlbar/UrlbarInput.jsm
@@ +339,5 @@
>        return;
>      }
>  
> +    Cc["@mozilla.org/widget/clipboardhelper;1"]
> +      .getService(Ci.nsIClipboardHelper)

can we memoize this in a defineLazyServiceGetter in the module scope?

::: browser/components/urlbar/UrlbarPrefs.jsm
@@ +49,5 @@
>    // fetching results.  However, we ignore this for the very first result (the
>    // "heuristic" result).  We fetch it as fast as possible.
>    ["delay", 50],
>  
> +  ["doubleClickSelectsAll", false],

Please add comments for each prefs, we are using this module also to better document the urlbar prefs, that will be useful for future insight of people working on experiments.

Comment 4

[Dão Gottwald [:dao]]

(In reply to Marco Bonardo [::mak] from comment #3)
> Please add comments for each prefs, we are using this module also to better
> document the urlbar prefs, that will be useful for future insight of people
> working on experiments.

I'm not sure what to document here. The pref name seems self-explanatory enough, and repeating "controls whether a (double) click selects all text" doesn't seem useful...

Comment 5

[Mark Banner (:standard8)]

It might be worth documenting why (because of how window managers work), which also leads to the fact the defaults are different on mac/windows versus linux.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Mark Banner (:standard8) from comment #5)
> It might be worth documenting why (because of how window managers work),

What do you mean by that?

> which also leads to the fact the defaults are different on mac/windows
> versus linux.

Afaik they're different because of the Primary selection on Linux. This would probably be better documented where we set the default for Linux, i.e. in firefox.js.

Comment 7

[Mark Banner (:standard8)]

(In reply to Dão Gottwald [::dao] from comment #6)
> (In reply to Mark Banner (:standard8) from comment #5)
> > It might be worth documenting why (because of how window managers work),
> 
> What do you mean by that?

Not just documenting what it does, but why it does it or why it is present. As you indicated, you can easily see that it controls the click selection. However, the last time I came across this (fairly recently), I had to do a reasonable amount of digging to find out that it was to do with pasting on linux versus other platforms.

I guess I was thinking of something like "Controls how click selection works on the urlbar. This is used to handle different ways of selecting text. For example, Linux uses a single middle-click to paste, and as a result turns this off, as otherwise the paste buffer is lost."

(I'm not 100% sure the example is correct, that's from what I remember).

> > which also leads to the fact the defaults are different on mac/windows
> > versus linux.
> 
> Afaik they're different because of the Primary selection on Linux. This
> would probably be better documented where we set the default for Linux, i.e.
> in firefox.js.

I'm torn here. firefox.js is in a different location from the code that implements the prefs, so you have to dig to find it if you don't know about it. On the flip side, firefox.js is the place that a lot of people will look, though you could say the same about all.js as well...

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/91d895e3296e
Port clickSelectsAll and doubleClickSelectsAll behavior from urlbarBindings.xml to UrlbarInput.jsm. r=mak

Comment 9

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/91d895e3296e