1322622 - Load the content script when add-on bootstraps

Status: RESOLVED FIXED

Description

[Luke Chang [:lchang] (inactive)]

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.

Comment 1

[Steve Chung [:steveck]]

Attachment: Bug 1322622 - Move handler as content script and load at bootstrap,

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

Comment 2

[Steve Chung [:steveck]]

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

Comment 3

[Luke Chang [:lchang] (inactive)]

(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 4

[Steve Chung [:steveck]]

Comment on attachment 8817595 [details]
Bug 1322622 - Move handler as content script and load at bootstrap,

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

Comment 5

[Steve Chung [:steveck]]

Comment on attachment 8817595 [details]
Bug 1322622 - Move handler as content script and load at bootstrap,

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

Comment 6

[Steve Chung [:steveck]]

Comment on attachment 8817595 [details]
Bug 1322622 - Move handler as content script and load at bootstrap,

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

Comment 7

[Matthew N. [:MattN] (PM me if requests are blocking you)]

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)

Comment 8

[Matthew N. [:MattN] (PM me if requests are blocking you)]

Attachment: Bug 1322622 - Rename the obsolete requestAutocomplete package out of the way of form autofill.

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

Comment 9

[Matthew N. [:MattN] (PM me if requests are blocking you)]

Attachment: Bug 1322622 - Define the BMO component for the formautofill extension.

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

Comment 10

[Matthew N. [:MattN] (PM me if requests are blocking you)]

Attachment: Bug 1322622 - Load form autofill JSMs from the built files and move JSMs out of content/.

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

Comment 11

[Matthew N. [:MattN] (PM me if requests are blocking you)]

Attachment: Bug 1322622 - Make form autofill handler a frame script and load from bootstrap.js.

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

Comment 12

[Matthew N. [:MattN] (PM me if requests are blocking you)]

Comment on attachment 8819095 [details]
Bug 1322622 - Rename the obsolete requestAutocomplete package out of the way of form autofill.

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

Comment 13

[Matthew N. [:MattN] (PM me if requests are blocking you)]

Comment on attachment 8819096 [details]
Bug 1322622 - Define the BMO component for the formautofill extension.

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

Comment 14

[Matthew N. [:MattN] (PM me if requests are blocking you)]

Comment on attachment 8819097 [details]
Bug 1322622 - Load form autofill JSMs from the built files and move JSMs out of content/.

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

Comment 15

[Matthew N. [:MattN] (PM me if requests are blocking you)]

Comment on attachment 8819098 [details]
Bug 1322622 - Make form autofill handler a frame script and load from bootstrap.js.

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

Comment 16

[Steve Chung [:steveck]]

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.

Comment 17

[Steve Chung [:steveck]]

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 18

[Steve Chung [:steveck]]

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 19

[Matthew N. [:MattN] (PM me if requests are blocking you)]

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 20

[Matthew N. [:MattN] (PM me if requests are blocking you)]

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 21

[Matthew N. [:MattN] (PM me if requests are blocking you)]

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 22

[Steve Chung [:steveck]]

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/[e-mail]] 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 23

[Steve Chung [:steveck]]

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 24

[Matthew N. [:MattN] (PM me if requests are blocking you)]

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 25

[Matthew N. [:MattN] (PM me if requests are blocking you)]

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 26

[Matthew N. [:MattN] (PM me if requests are blocking you)]

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 27

[Matthew N. [:MattN] (PM me if requests are blocking you)]

Comment on attachment 8819097 [details]
Bug 1322622 - Load form autofill JSMs from the built files and move JSMs out of content/.

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

Comment 28

[Matthew N. [:MattN] (PM me if requests are blocking you)]

Comment on attachment 8819098 [details]
Bug 1322622 - Make form autofill handler a frame script and load from bootstrap.js.

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

Comment 29

[Steve Chung [:steveck]]

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.

Comment 30

[Steve Chung [:steveck]]

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.

Comment 31

[Steve Chung [:steveck]]

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.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
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

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb8eff895088
form autofill - clobber to fix duplicate file issues. r=clobber

Comment 34

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b62bae8d1375
https://hg.mozilla.org/mozilla-central/rev/25a0a38597b7
https://hg.mozilla.org/mozilla-central/rev/b8bd09ed36b6
https://hg.mozilla.org/mozilla-central/rev/d1f168a29eca
https://hg.mozilla.org/mozilla-central/rev/eb8eff895088