Closed Bug 1322622 Opened 4 years ago Closed 4 years ago

Load the content script when add-on bootstraps

Categories

(Toolkit :: Form Manager, enhancement, P3)

Product:
Toolkit
Component:
Form Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: lchang, Assigned: MattN)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M1])

Attachments

(4 files, 1 obsolete file)

Bug 1322622 - Rename the obsolete requestAutocomplete package out of the way of form autofill.
58 bytes, text/x-review-board-request
steveck
: review+
Details
Bug 1322622 - Define the BMO component for the formautofill extension.
58 bytes, text/x-review-board-request
steveck
: review+
Details
Bug 1322622 - Load form autofill JSMs from the built files and move JSMs out of content/.
58 bytes, text/x-review-board-request
steveck
: review+
Details
Bug 1322622 - Make form autofill handler a frame script and load from bootstrap.js.
58 bytes, text/x-review-board-request
steveck
: review+
Details 
In this bug we aim to create an empty content script to make further bugs be able to handle in parallel and rename some files to align with our architecture design.

Also, we should check a preference entry (e.g. "browser.formautofill.enabled"
) to decide whether or not initializing the add-on.
Assignee: nobody → schung
Status: NEW → ASSIGNED
Comment on attachment 8817595 [details]
Bug 1322622 - Move handler as content script and load at bootstrap,

https://reviewboard.mozilla.org/r/97828/#review98192

::: browser/extensions/formautofill/bootstrap.js:19
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "FormAutofillParent",
> +                            "resource://formautofill/FormAutofillParent.jsm");
> +
> +function startup() {
> +  // TODO: We'll need another specific pref for controlling form autofill
> +  if (!Services.prefs.getBoolPref("dom.forms.autocomplete.experimental")) {

Not sure how we could test this part, maybe xpcshell test
Attachment #8817595 - Flags: feedback?(MattN+bmo)
(In reply to Steve Chung [:steveck] from comment #2)
> Comment on attachment 8817595 [details]
> Bug 1322622 - Part 1: Add formautofill content script
> 
> https://reviewboard.mozilla.org/r/97828/#review98192
> 
> ::: browser/extensions/formautofill/bootstrap.js:19
> (Diff revision 1)
> > +XPCOMUtils.defineLazyModuleGetter(this, "FormAutofillParent",
> > +                            "resource://formautofill/FormAutofillParent.jsm");
> > +
> > +function startup() {
> > +  // TODO: We'll need another specific pref for controlling form autofill
> > +  if (!Services.prefs.getBoolPref("dom.forms.autocomplete.experimental")) {
> 
> Not sure how we could test this part, maybe xpcshell test

As Matt suggested yesterday, we should have our own entry for enable/disable add-on rather than using autocomplete's one.
Comment on attachment 8817595 [details]
Bug 1322622 - Move handler as content script and load at bootstrap,

https://reviewboard.mozilla.org/r/97828/#review98264

I'll make the below changes with my approach which avoids manually registering the resource URIs and separates chrome vs. resource.

::: browser/extensions/formautofill/bootstrap.js:14
(Diff revision 4)
> +XPCOMUtils.defineLazyModuleGetter(this, "FormAutofillParent",
> +                            "resource://formautofill/FormAutofillParent.jsm");

Nit: fix indentation

::: browser/extensions/formautofill/bootstrap.js:20
(Diff revision 4)
> +                            "resource://formautofill/FormAutofillParent.jsm");
> +
> +function startup() {
> +  // Besides this pref, we'll need dom.forms.autocomplete.experimental enabled
> +  // as well to make sure form autocomplete works correctly.
> +  if (!Services.prefs.getBoolPref("dom.forms.autocomplete.enabled")) {

Please use "browser.formautofill.enabled"

::: browser/extensions/formautofill/bootstrap.js:25
(Diff revision 4)
> +  Services.mm.loadFrameScript(
> +    "resource://formautofill/FormAutofillContent.js", true);

Nit: Put on one line

::: browser/extensions/formautofill/bootstrap.js:26
(Diff revision 4)
> +    return;
> +  }
> +
> +  FormAutofillParent.init();
> +  Services.mm.loadFrameScript(
> +    "resource://formautofill/FormAutofillContent.js", true);

This should be a chrome URI

::: browser/extensions/formautofill/content/FormAutofillContent.js:6
(Diff revision 4)
>  /* 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/. */
>  
>  /*
> - * Implements a service used by DOM content to request Form Autofill.
> + * Implements Form Autofill content process script.

This isn't a content process script, it's a frame script

::: modules/libpref/init/all.js:1192
(Diff revision 4)
> +// Enable from autocomplete system addon. Disabled before feature completed.
> +pref("dom.forms.autocomplete.enabled", false);

Put browser.formautofill.enabled in firefox.js (since it's only for browser)
Attachment #8817595 - Flags: review?(MattN+bmo)
Assignee: schung → MattN+bmo
Attachment #8817595 - Attachment is obsolete: true
Comment on attachment 8819098 [details]
Bug 1322622 - Make form autofill handler a frame script and load from bootstrap.js.

https://reviewboard.mozilla.org/r/98938/#review99234

Looks good, thnaks for the quick problem addressing.

::: browser/extensions/formautofill/test/unit/xpcshell.ini
(Diff revision 2)
>  [DEFAULT]
>  firefox-appdir = browser
>  head = head.js
> -tail = tail.js

I removed it in my original patch, but Luke suggested that maybe we should keep it since other ini kept the tail even it's not linked to any file. But I'm fine if we won't need this in the future.
Attachment #8819098 - Flags: review?(schung) → review+
Comment on attachment 8819096 [details]
Bug 1322622 - Define the BMO component for the formautofill extension.

https://reviewboard.mozilla.org/r/98934/#review99236

::: browser/extensions/formautofill/moz.build:25
(Diff revision 2)
>  XPCSHELL_TESTS_MANIFESTS += ['test/unit/xpcshell.ini']
>  
>  JAR_MANIFESTS += ['jar.mn']
> +
> +with Files('**'):
> +    BUG_COMPONENT = ('Toolkit', 'Form Manager')

Sorry but I'm not sure why we need this. It seems only related to component on Bugzilla.
Comment on attachment 8819095 [details]
Bug 1322622 - Rename the obsolete requestAutocomplete package out of the way of form autofill.

https://reviewboard.mozilla.org/r/98932/#review99240

::: toolkit/components/formautofill/jar.mn:6
(Diff revision 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/.
>  
>  toolkit.jar:
> -% content formautofill %content/formautofill/
> +% content requestautocomplete %content/requestautocomplete/

May I know the reason why we need to move requestAutocomplete out? And, why can't we simply remove it if it's obsoleted?
Comment on attachment 8819096 [details]
Bug 1322622 - Define the BMO component for the formautofill extension.

https://reviewboard.mozilla.org/r/98934/#review99236

> Sorry but I'm not sure why we need this. It seems only related to component on Bugzilla.

This isn't really related to this bug but I noticed this was missing while working on it. This just helps automated tools know what component to file bugs in for this directory.
Comment on attachment 8819098 [details]
Bug 1322622 - Make form autofill handler a frame script and load from bootstrap.js.

https://reviewboard.mozilla.org/r/98938/#review99234

> I removed it in my original patch, but Luke suggested that maybe we should keep it since other ini kept the tail even it's not linked to any file. But I'm fine if we won't need this in the future.

OK, makes no difference to me since it's easy to add back.
Comment on attachment 8819095 [details]
Bug 1322622 - Rename the obsolete requestAutocomplete package out of the way of form autofill.

https://reviewboard.mozilla.org/r/98932/#review99240

> May I know the reason why we need to move requestAutocomplete out? And, why can't we simply remove it if it's obsoleted?

I was leaving the code in case there was anything useful for Form Autofill or Payment Request to re-use (see bug 1270740 comment 3) but it should be removed eventually that was just more work as there are also DOM parts and tests.
Comment on attachment 8819097 [details]
Bug 1322622 - Load form autofill JSMs from the built files and move JSMs out of content/.

https://reviewboard.mozilla.org/r/98936/#review99252

::: browser/extensions/formautofill/jar.mn:6
(Diff revision 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/.
>  
>  [features/formautofill@mozilla.org] chrome.jar:
> -% resource formautofill %content/
> +% resource formautofill %res/

It seems we don't need the content folder here. Is it a convention that all the js/jsm under the content should be loaded in content process? If so, please remove the folder as well since we probably only need frame script instead of content script.
Comment on attachment 8819097 [details]
Bug 1322622 - Load form autofill JSMs from the built files and move JSMs out of content/.

https://reviewboard.mozilla.org/r/98936/#review99254

::: browser/extensions/formautofill/test/unit/xpcshell.ini:2
(Diff revision 2)
>  [DEFAULT]
> +firefox-appdir = browser

Although it's not important, it's just out of curiosity: May I know the purpose of this variable?
Comment on attachment 8819097 [details]
Bug 1322622 - Load form autofill JSMs from the built files and move JSMs out of content/.

https://reviewboard.mozilla.org/r/98936/#review99252

> It seems we don't need the content folder here. Is it a convention that all the js/jsm under the content should be loaded in content process? If so, please remove the folder as well since we probably only need frame script instead of content script.

The meaning of "content" in this context pre-dates e10s and refers to the [`content` package](https://developer.mozilla.org/en-US/docs/Chrome_Registration#content-instruction) in Chrome registration. Things in the `content` package are normally loaded into UI e.g. xhtml, xul and their supporting .js files. A frame script can be though of as being loaded into the context of the <xul:browser>. The [`resource` package](https://developer.mozilla.org/en-US/docs/Chrome_Registration#resource) is mostly used for JavaScript modules (JSMs) and sometimes non-theme images/CSS.

You will see in the fourth patch that I put the frame script in the `content` package and in `content` directory to match as that's the convention. JSMs normally don't go in the `content` subdirectory since they're `resource`s instead.
Comment on attachment 8819097 [details]
Bug 1322622 - Load form autofill JSMs from the built files and move JSMs out of content/.

https://reviewboard.mozilla.org/r/98936/#review99254

> Although it's not important, it's just out of curiosity: May I know the purpose of this variable?

There are some details in bug 1215378 and I thought it was necessary for "GreD" to be correct but it seems to work without that on OS X: https://reviewboard-hg.mozilla.org/gecko/rev/0051430de079#l5.18
Comment on attachment 8819097 [details]
Bug 1322622 - Load form autofill JSMs from the built files and move JSMs out of content/.

https://reviewboard.mozilla.org/r/98936/#review99254

> There are some details in bug 1215378 and I thought it was necessary for "GreD" to be correct but it seems to work without that on OS X: https://reviewboard-hg.mozilla.org/gecko/rev/0051430de079#l5.18

The relevant code is at https://dxr.mozilla.org/mozilla-central/rev/6dbc6e9f62a705d5f523cc750811bd01c8275ec6/testing/xpcshell/runxpcshelltests.py#1229-1235 btw.
Comment on attachment 8819096 [details]
Bug 1322622 - Define the BMO component for the formautofill extension.

https://reviewboard.mozilla.org/r/98934/#review99298

ok, thanks for the explanation.
Attachment #8819096 - Flags: review?(schung) → review+
Comment on attachment 8819095 [details]
Bug 1322622 - Rename the obsolete requestAutocomplete package out of the way of form autofill.

https://reviewboard.mozilla.org/r/98932/#review99316

::: toolkit/components/formautofill/jar.mn:6
(Diff revision 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/.
>  
>  toolkit.jar:
> -% content formautofill %content/formautofill/
> +% content requestautocomplete %content/requestautocomplete/

I think it's fine since bug 1270740 is still there.
Attachment #8819095 - Flags: review?(schung) → review+
Comment on attachment 8819097 [details]
Bug 1322622 - Load form autofill JSMs from the built files and move JSMs out of content/.

https://reviewboard.mozilla.org/r/98936/#review99318

LGTM after the explanation, thanks.
Attachment #8819097 - Flags: review?(schung) → review+
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/b62bae8d1375
Rename the obsolete requestAutocomplete package out of the way of form autofill. r=steveck
https://hg.mozilla.org/integration/autoland/rev/25a0a38597b7
Define the BMO component for the formautofill extension. r=steveck
https://hg.mozilla.org/integration/autoland/rev/b8bd09ed36b6
Load form autofill JSMs from the built files and move JSMs out of content/. r=steveck
https://hg.mozilla.org/integration/autoland/rev/d1f168a29eca
Make form autofill handler a frame script and load from bootstrap.js. r=steveck
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/eb8eff895088
form autofill - clobber to fix duplicate file issues. r=clobber
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.