1303469 - Implement a stub form autofill system extension

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Matthew N. [:MattN]]

In order to make it easier to parallelize initial development of the form autofill feature during the kickoff work week next week, we should land a stub system extension for everyone to build off of.

Comment 1

[Matthew N. [:MattN]]

Attachment: Bug 1303469 - Stub form autofill system extension.

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

Comment 2

[Matthew N. [:MattN]]

Attachment: Bug 1303469 - Initial formautofill .eslintrc.

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

Comment 3

[Robert Helmer [:rhelmer]]

Comment on attachment 8792161 [details]
Bug 1303469 - Stub form autofill system extension.

https://reviewboard.mozilla.org/r/79374/#review77908

It's a bit annoying having to copy/paste so much, thanks for slogging through that! Standard8 is putting together a good stub template to use which will help next time.

I'll file a bug to fix the comments I made here - we should reduce the delta between the various extensions wherever possible.

::: browser/extensions/formautofill/bootstrap.js:5
(Diff revision 1)
> -/* -*- indent-tabs-mode: nil; js-indent-level: 2 -*- */
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -// for now we ship a stub, that can be upgraded as-needed after release
> +"use strict";

We should fix this in the `webcompat` copy too.

::: browser/extensions/formautofill/install.rdf.in:18
(Diff revision 1)
>      <em:version>1.0</em:version>
>      <em:type>2</em:type>
>      <em:bootstrap>true</em:bootstrap>
>      <em:multiprocessCompatible>true</em:multiprocessCompatible>
>  
> -    <!-- Target Application this theme can install into,
> +    <!-- Target Application this extension can install into,

Ditto

::: browser/extensions/formautofill/test/browser/browser_check_installed.js:7
(Diff revision 1)
> -// make sure that the stub is present and enabled
> +add_task(function* test_enabled() {
> -add_task(function* stub_enabled() {
>    let addon = yield new Promise(
> -    resolve => AddonManager.getAddonByID("[e-mail]", resolve)
> +    resolve => AddonManager.getAddonByID("[e-mail]", resolve)
>    );
> -  isnot(addon, null, "Webcompat stub addon should exist");
> +  isnot(addon, null, "Check addon exists");

Removing the redundant add-on name is a good idea.

Comment 4

[Robert Helmer [:rhelmer]]

Comment on attachment 8792162 [details]
Bug 1303469 - Initial formautofill .eslintrc.

https://reviewboard.mozilla.org/r/79376/#review77910

lgtm, can take or leave my comment.

::: browser/extensions/formautofill/test/browser/.eslintrc:3
(Diff revision 1)
> +{
> +  "extends": [
> +    "../../../../../testing/mochitest/browser.eslintrc"

I assume you're doing this to get all the extra rules we're able to enable for newer code like WebExtensions, right? I wish we had a clearer way to express that's the intent rather than this far-off `extends`. 

Maybe consider adding a comment?

We're probably a little ways off being able to enable this in `tookit/.eslintrc` but when we do this can be cleaned up so I don't think it's anything to stress over.

Comment 5

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #3)
> I'll file a bug to fix the comments I made here - we should reduce the delta
> between the various extensions wherever possible.

Bug 1303514

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8792162 [details]
Bug 1303469 - Initial formautofill .eslintrc.

https://reviewboard.mozilla.org/r/79376/#review77910

> I assume you're doing this to get all the extra rules we're able to enable for newer code like WebExtensions, right? I wish we had a clearer way to express that's the intent rather than this far-off `extends`. 
> 
> Maybe consider adding a comment?
> 
> We're probably a little ways off being able to enable this in `tookit/.eslintrc` but when we do this can be cleaned up so I don't think it's anything to stress over.

No, every directory for mochitest-browser-chrome tests has this "extends" and it sets up the environment for those test files e.g. head.js and browser.js scope, defines test functions, etc. See https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/browser.eslintrc

I think perhaps this comment was meant for the other .eslintrc file?

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8792161 [details]
Bug 1303469 - Stub form autofill system extension.

https://reviewboard.mozilla.org/r/79374/#review77908

It actually wasn't bad at all IMO :) I'm aware of Standard8's repo but it's setup more for a separate repo, not as much for an extension developed in m-c from what I saw.

> Ditto

Yeah, I actually did make this change in the commit as it seemed clear that it was wrong. I wasn't sure if the other changes would also be wantd for webcompat so was waiting to see what you would say.

> Removing the redundant add-on name is a good idea.

Thanks for the speedy reviews!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/03f631f0fca7
Stub form autofill system extension. r=rhelmer
https://hg.mozilla.org/integration/autoland/rev/86b10c4dce3b
Initial formautofill .eslintrc. r=rhelmer

Comment 9

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/9fcea68f98ef for unexpected file access in xperf, oddly only PGO not opt, https://treeherder.mozilla.org/logviewer.html#?job_id=3655070&repo=autoland

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/a01c29ae5295
Stub form autofill system extension. r=rhelmer
https://hg.mozilla.org/integration/fx-team/rev/fd81de7deaab
Initial formautofill .eslintrc. r=rhelmer

Comment 11

[Matthew N. [:MattN]]

Relanded with https://hg.mozilla.org/integration/fx-team/diff/a01c29ae5295/testing/talos/talos/xtalos/xperf_whitelist.json

Joel, I landed this with the change since it was trivial and the same as was done for flyweb and webcompat and I saw you didn't review the flyweb change. I was in a rush to re-land since I was hoping to have this directory landed so development can start during a form autofill work week this week. Let me know if you have any problems with that change.

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a01c29ae5295
https://hg.mozilla.org/mozilla-central/rev/fd81de7deaab

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

:mattn, thanks for checking with me- that looks just fine!