Closed
Bug 1322622
Opened 6 years ago
Closed 6 years ago
Load the content script when add-on bootstraps
Categories
(Toolkit :: Form Manager, enhancement, P3)
Toolkit
Form Manager
Tracking
()
RESOLVED
FIXED
mozilla53
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)
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.
Updated•6 years ago
|
Assignee: nobody → schung
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 2•6 years ago
|
||
mozreview-review |
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
Updated•6 years ago
|
Attachment #8817595 -
Flags: feedback?(MattN+bmo)
Reporter | ||
Comment 3•6 years ago
|
||
(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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•6 years ago
|
||
mozreview-review |
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)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Assignee: schung → MattN+bmo
Assignee | ||
Updated•6 years ago
|
Attachment #8817595 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 16•6 years ago
|
||
mozreview-review |
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 17•6 years ago
|
||
mozreview-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 18•6 years ago
|
||
mozreview-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/#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?
Assignee | ||
Comment 19•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 20•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 21•6 years ago
|
||
mozreview-review-reply |
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•6 years ago
|
||
mozreview-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/#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 23•6 years ago
|
||
mozreview-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/#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?
Assignee | ||
Comment 24•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 25•6 years ago
|
||
mozreview-review-reply |
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
Assignee | ||
Comment 26•6 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 29•6 years ago
|
||
mozreview-review |
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 30•6 years ago
|
||
mozreview-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 31•6 years ago
|
||
mozreview-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+
Comment 32•6 years ago
|
||
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
Comment 33•6 years ago
|
||
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/eb8eff895088 form autofill - clobber to fix duplicate file issues. r=clobber
Comment 34•6 years ago
|
||
bugherder |
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Updated•6 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•