Closed Bug 1303469 Opened 8 years ago Closed 8 years ago

Implement a stub form autofill system extension

Categories

(Toolkit :: Form Manager, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: MattN, Assigned: MattN)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M0])

Attachments

(2 files)

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 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("webcompat@mozilla.org", resolve)
> +    resolve => AddonManager.getAddonByID("formautofill@mozilla.org", 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.
Attachment #8792161 - Flags: review?(rhelmer) → review+
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.
Attachment #8792162 - Flags: review?(rhelmer) → review+
(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 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 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!
Pushed by mozilla@noorenberghe.ca:
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
Pushed by mozilla@noorenberghe.ca:
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
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.
Flags: needinfo?(jmaher)
https://hg.mozilla.org/mozilla-central/rev/a01c29ae5295
https://hg.mozilla.org/mozilla-central/rev/fd81de7deaab
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
:mattn, thanks for checking with me- that looks just fine!
Flags: needinfo?(jmaher)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: