1407956 - Port bug 1407613 to TB: Remove dropmarker from the generic autocomplete binding

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Description

[Richard Marti (:Paenglab)]

Bug 1407613 removes the enablehistory and the autocomplete-history-dropmarker. As I see this, we don't use the enablehistory and by this also not the autocomplete-history-dropmarker. For me it's safe to remove this parts.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: no-enablehistory.patch

On https://hg.mozilla.org/integration/autoland/rev/b80b14cd26f3 you see what will be removed with the next merge. I haven't seen an issue without our patch. So it should be no problem when the patch lands later.

I removed all occurrences of enablehistory and autocomplete-history-dropmarker in our code. We use the autocomplete in gloda search and in composer on the "Insert Link" dialog.

Comment 2

[Jorg K (CEST = GMT+2)]

I've looked at this for a while. As introductory comments we should note the following:

Our most recent encounter with enablehistory was here, where it got removed:
https://hg.mozilla.org/comm-central/rev/325dac4668f2#l1.16
to fix bug 1403264. Maybe instead of removing it, we could have investigated a little more why it didn't work.

Autocomplete is of course most prominently used in recipient autocompletion.

I'll comment on the patch in a minute.

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8917765 [details] [diff] [review]
no-enablehistory.patch

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

Nothing will break if M-C land their removal of the use of enablehistory.

FRG, can you please look at the first hunk I commented on here. I personally wouldn't remove it. While you're at it, how do you feel about:
https://hg.mozilla.org/comm-central/rev/325dac4668f2#l1.16
Or who's the editor expert these days?

::: editor/ui/dialogs/content/EdDialogCommon.js
@@ -958,5 @@
>        // Don't bother with named anchors in Mail.
>        if (editor && (editor.flags & Components.interfaces.nsIPlaintextEditor.eEditorMailMask))
>        {
>          menupopup.remove();
> -        linkMenulist.removeAttribute("enablehistory");

I'm not happy with the (unnecessary) removal of this line.

XUL autocomplete still offers enablehistory, but FF removed all uses of it, and we don't use it either after:
https://hg.mozilla.org/comm-central/rev/325dac4668f2#l1.16

SM still used enablehistory and we shouldn't change behaviour here.

::: mail/base/content/extraCustomizeItems.xul
@@ -36,5 @@
>                   searchbutton="true"
>                   autocompletesearch="gloda"
>                   autocompletepopup="PopupGlodaAutocomplete"
>                   autocompletesearchparam="global"
> -                 enablehistory="false"

This removal and all the following are OK, I think.

Comment 4

[Richard Marti (:Paenglab)]

Comment on attachment 8917765 [details] [diff] [review]
no-enablehistory.patch

I'll add Stefan too as he already worked on the autocomplete-dropmartker removal for Mac.

Comment 5

[Frank-Rainer Grahl (:frg)]

IanN is probably the

Comment 6

[Frank-Rainer Grahl (:frg)]

Sorry hit ENTER too fast.

IanN is probably the last if ever editor expert. I bookmarked the m-c bug yesterday to look into it. Not sure what it does with our urlbar history but still on 56/2.53. We still have separate suite autocomplete bindings for this. Personally I dislike to current removals in toolkit and adding of additional obscure code in mozilla\browser but I doubt we will change anything by complaining... 

autocomplete is broken in SeaMonkey 2.55 / 58. I still hadn't time to look at it. 

> I personally wouldn't remove it.

The drop down marker acutally still works for me in 2.53/56 so Bug 1403264 could probably have been fixed. 

> SM still used enablehistory and we shouldn't change behaviour here.

Depending on the outcome of the whole 57+ mess I agree. But we might need to change the code to match fx behaviour anyway. For now I would leave linkMenulist.removeAttribute("enablehistory") in.

Comment 7

[Stefan [:stefanh]]

I'll just add a few comments before I go to bed (just came home):

Maybe this is obvious, but I don't see any explicit comment about this: SM doesn't use the toolkit autocomplete binding, it uses the binding and the css in mozilla/xpfe/components/autocomplete/resources/content (see mozilla/xpfe/components/autocomplete/jar.mn and https://dxr.mozilla.org/comm-central/rev/706e5fdd301201b23e96be9826b7e5b953d11776/suite/app.mozbuild#25). So unless I'm completely mistaken (I admit that this have happened before), the breakage in SM from bug 1407613 would be that the dropmarker icon is gone in Linux/Win. The Mac dropmarker should be OK since it's not the toolkit one.

I would also leave linkMenulist.removeAttribute("enablehistory") in. https://hg.mozilla.org/comm-central/rev/325dac4668f2#l1.16 should probably be investigated a bit, since it affected SM.

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to Stefan [:stefanh] from comment #7)
> https://hg.mozilla.org/comm-central/rev/325dac4668f2#l1.16 should probably
> be investigated a bit, since it affected SM.
Anchors are presented in a popup, see:
https://dxr.mozilla.org/comm-central/rev/706e5fdd301201b23e96be9826b7e5b953d11776/editor/ui/dialogs/content/EdDialogCommon.js#954

The history on the field didn't seem to do anything and was confusing, so we removed it. Maybe a good decision since M-C are also phasing it out. So I'm not sure that we wanted to re-establish it even if you could repair it to do something useful.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: no-enablehistory.patch

Same patch without the removal in editor/ui/dialogs/content/EdDialogCommon.js.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 8918201 [details] [diff] [review]
no-enablehistory.patch

OK, let's go with this. I'll check it in now.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6d771e7ff605
Port bug 1407613 to TB: Remove dropmarker from the generic autocomplete binding, remove use of enablehistory. r=jorgk