Closed Bug 1417119 Opened 2 years ago Closed 2 years ago

Remove xpfe autocomplete component from mozilla-central

Categories

(Core :: XUL, task, P5)

task

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

(Blocks 1 open bug)

Details

(Whiteboard: [xbl-remove-unused])

Attachments

(2 files)

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
Whiteboard: [xbl-remove-unused]
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.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
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.
Attachment #8928208 - Flags: review?(frgrahl)
Depends on: 1418512
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.
Attachment #8928208 - Flags: review?(frgrahl) → review?(iann_bugzilla)
Priority: -- → P5
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.
(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
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 on attachment 8928208 [details]
Bug 1417119 - Remove xpfe autocomplete binding

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

LGTM r=me
Attachment #8928208 - Flags: review+
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.
Attachment #8928208 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8928208 [details]
Bug 1417119 - Remove xpfe autocomplete binding

SeaMonkey is building and working fine with this patch.
Attachment #8928208 - Flags: review?(frgrahl) → review+
Comment on attachment 8928208 [details]
Bug 1417119 - Remove xpfe autocomplete binding

Thanks for the reviews all, updated with small tweaks from Comment 11
Attachment #8928208 - Flags: review?(frgrahl) → review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/24499c5b6d3d
Remove xpfe autocomplete binding;r=iann_bugzilla+23131,Paolo
https://hg.mozilla.org/mozilla-central/rev/24499c5b6d3d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.