1417119 - Remove xpfe autocomplete component from mozilla-central

Status: RESOLVED FIXED

(Core :: XUL, task, P5)

Description

[Brian Grinstead [:bgrins]]

The autocomplete bindings and associated CSS in xpfe/components/autocomplete/ are used in mozilla-central, but they are used in comm-central suite and owned by suite. I suggest we move the code from m-c to c-c.

Thread: https://groups.google.com/d/msg/mozilla.dev.platform/EoJkW2qQIQQ/ao71CndwBwAJ

Comment 1

[Brian Grinstead [:bgrins]]

There are some tests that handle both autocomplete widgets:

- accessible/tests/mochitest/tree/test_combobox.xul
- accessible/tests/mochitest/tree/test_txtctrl.xul
- toolkit/content/tests/chrome/test_autocomplete_delayOnPaste.xul

We'll have to decide if we should remove the conditionals in those tests and migrate them to comm-central as well, or keep the tests as-is.

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: Bug 1417119 - Remove xpfe autocomplete binding

Review commit: https://reviewboard.mozilla.org/r/199438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/199438/

Comment 3

[Brian Grinstead [:bgrins]]

Comment on attachment 8928208 [details]
Bug 1417119 - Remove xpfe autocomplete binding

:frg, I'm not sure who to ask for review here so feel free to redirect. We can wait until it's moved to comm-central to land. See Comment 1 about the test changes.

Comment 4

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8928208 [details]
Bug 1417119 - Remove xpfe autocomplete binding

IanN should better review it.

There is some code in mozilla/toolkit/content/xul.css referencing it.

> /********** autocomplete textbox **********/

> /* SeaMonkey does not use the new toolkit's autocomplete widget */
> %ifdef MOZ_SUITE

The code inside probably needs to be removed too but I would let the ifdef stay and make it a ifnotdef suite. Nt sure if we can rename it to e.g autocomplete1 but that needs to be discussed in the SeaMonkey bug.

Comment 5

[Frank-Rainer Grahl (:frg)]

Attachment: xpefeautocompletecss.patch

Brian,

I moved the autocomplete to comm-central. Accidently generated some bugspam but this is something we need to remember for the next time.

You might want to remove the css inside the suite ifdef from toolkit/content/xul.css. Please keep the ifdef itself for now. See attached patch.

IanN is usually the right reviewer for comm-central but in this case he might have no objections if someone else does it. 

Our tests are somewhat broken right now. It is possibly be better if we reimplement them at a later time. I didn't move them and not sure if they can even be separated the way they are currently implemented. We probably need to move to unfiedcomplete sooner or later anyway.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Frank-Rainer Grahl (:frg) from comment #5)
> I moved the autocomplete to comm-central. Accidently generated some bugspam
> but this is something we need to remember for the next time.
> 
> You might want to remove the css inside the suite ifdef from
> toolkit/content/xul.css. Please keep the ifdef itself for now. See attached
> patch.

Thank you, I will fold it into the patch

> IanN is usually the right reviewer for comm-central but in this case he
> might have no objections if someone else does it. 

OK, I will request review from you but feel free to redirect to whoever you think is appropriate

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8928208 [details]
Bug 1417119 - Remove xpfe autocomplete binding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199438/diff/1-2/

Comment 8

[Brian Grinstead [:bgrins]]

This patch mostly just removes the folder, but as I mentioned in Comment 1 we have to make a decision about what to do with tests in accessible/ and toolkit/ - in this patch I just removed the conditionals but let me know if you think we should keep them.

Comment 9

[Brian Grinstead [:bgrins]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44c0bfc5e4448a3f80480ee28c7004cc193e6c63

Comment 10

[Ian Neal]

Comment on attachment 8928208 [details]
Bug 1417119 - Remove xpfe autocomplete binding

https://reviewboard.mozilla.org/r/199438/#review208710

LGTM r=me

Comment 11

[:Paolo Amadini]

Comment on attachment 8928208 [details]
Bug 1417119 - Remove xpfe autocomplete binding

https://reviewboard.mozilla.org/r/199438/#review208988

r+ for the Toolkit part.

::: accessible/tests/mochitest/tree/test_combobox.xul:126
(Diff revision 2)
> -      // XPFE and Toolkit autocomplete widgets differ.
> +      // Popup is lazily created, so not present in for toolkit autocomplete
>        var ac1h = document.getElementById("autocomplete");
> -      if ("clearResults" in ac1h) {
> +      SimpleTest.ok(ac1h, "Testing (New) Toolkit autocomplete widget. (ac1h)");

It doesn't seem these lines are testing anything, we might as well remove them.

::: accessible/tests/mochitest/tree/test_combobox.xul:166
(Diff revision 2)
> -        // Popup is always created.
> -        accTree.children.push(
> -          {
> +      // Popup is lazily created, so not present in for toolkit autocomplete
> +      var ac2cmp = document.getElementById("autocomplete2");
> +      SimpleTest.ok(ac2cmp, "Testing (New) Toolkit autocomplete widget. (ac2mp)");

Same here.

Comment 12

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8928208 [details]
Bug 1417119 - Remove xpfe autocomplete binding

SeaMonkey is building and working fine with this patch.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8928208 [details]
Bug 1417119 - Remove xpfe autocomplete binding

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199438/diff/2-3/

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8928208 [details]
Bug 1417119 - Remove xpfe autocomplete binding

Thanks for the reviews all, updated with small tweaks from Comment 11

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24499c5b6d3d
Remove xpfe autocomplete binding;r=iann_bugzilla+23131,Paolo

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/24499c5b6d3d