1367077 - Ensure form fill (passwords, etc) working in GeckoView-based custom tabs and web apps

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

This might not be working because of the underlying JS/XUL changes, so we need to make sure this is working.

Comment 2

[Dylan Roeh (:droeh) (he/him)]

Updating the title to include web apps, I don't think there's a need for separate bugs as it will be functionally the same solution for both.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Jim can you take a look?

Comment 5

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1367077 - 1. Move startup utility functions into GeckoViewUtils;

Move `addLazyGetter` and `addLazyEventListener` utility functions from
GeckoViewStartup.js into GeckoViewUtils.jsm, so they can be used for
both Fennec and standalone GeckoView.

Also switch to "chrome-document-loaded" for loading
DownloadNotifications because that's later in the startup sequence.

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

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1367077 - 2. Move PromptService startup to BrowserCLH;

Use the BrowserCLH for PromptService startup, to consolidate startup
handling code and also to delay loading PromptService.

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

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1367077 - 3. Move form fill event listeners out of browser.js;

Move the form fill event listeners out of browser.js and into
BrowserCLH.js, and update them to support chrome windows, so we can
handle form fill events for Fennec, custom tabs, and PWAs.

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

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Bug 1367077 - 4. Remove LoginManagerParent.login;

Mobile code now loads LoginManagerParent lazily, similar to
nsBrowserGlue on desktop, so we no longer need LoginManagerParent.login.

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

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906688 [details]
Bug 1367077 - 1. Move startup utility functions into GeckoViewUtils;

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

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906689 [details]
Bug 1367077 - 2. Move PromptService startup to BrowserCLH;

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

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906690 [details]
Bug 1367077 - 3. Move form fill event listeners out of browser.js;

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

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8906688 [details]
Bug 1367077 - 1. Move startup utility functions into GeckoViewUtils;

https://reviewboard.mozilla.org/r/178416/#review183402

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8906689 [details]
Bug 1367077 - 2. Move PromptService startup to BrowserCLH;

https://reviewboard.mozilla.org/r/178418/#review183404

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8906705 [details]
Bug 1367077 - 4. Remove LoginManagerParent.login;

https://reviewboard.mozilla.org/r/178442/#review183412

Comment 15

[Kris Maglione [:kmag]]

Comment on attachment 8906690 [details]
Bug 1367077 - 3. Move form fill event listeners out of browser.js;

https://reviewboard.mozilla.org/r/178420/#review183418

::: mobile/android/chrome/content/browser.js:49
(Diff revision 2)
> -                                  "resource://gre/modules/LoginManagerContent.jsm");
> -
> -XPCOMUtils.defineLazyModuleGetter(this, "LoginManagerParent",
> -                                  "resource://gre/modules/LoginManagerParent.jsm");
> -
>  XPCOMUtils.defineLazyModuleGetter(this, "Task", "resource://gre/modules/Task.jsm");

Drive-by comment, while I'm here:

It would probably be a good idea to replace these `defineLazyModuleGetter` calls with a single `defineLazyModuleGetters` call, since the latter uses a JIT-friendly loop, and tends to be much faster. Similar for `defineLazyServiceGetters`.

I already did this with most of the mass lazy module defines in browser/ and toolkit/, but it would probably be good to do the same for mobile/.

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

Thanks! I filed bug 1399001.

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8906690 [details]
Bug 1367077 - 3. Move form fill event listeners out of browser.js;

https://reviewboard.mozilla.org/r/178420/#review183844

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906688 [details]
Bug 1367077 - 1. Move startup utility functions into GeckoViewUtils;

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

Comment 19

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906689 [details]
Bug 1367077 - 2. Move PromptService startup to BrowserCLH;

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

Comment 20

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906690 [details]
Bug 1367077 - 3. Move form fill event listeners out of browser.js;

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

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906705 [details]
Bug 1367077 - 4. Remove LoginManagerParent.login;

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

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83cf26e44284
1. Move startup utility functions into GeckoViewUtils; r=snorp
https://hg.mozilla.org/integration/autoland/rev/c6300312d42a
2. Move PromptService startup to BrowserCLH; r=snorp
https://hg.mozilla.org/integration/autoland/rev/8df5e093dd92
3. Move form fill event listeners out of browser.js; r=sebastian
https://hg.mozilla.org/integration/autoland/rev/0509b09c11fa
4. Remove LoginManagerParent.login; r=kmag

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for android mochitest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=130421861&repo=autoland

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/autoland/rev/04bf10e9a1f7640a282a7bd254c0fd7000863960

Comment 25

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906688 [details]
Bug 1367077 - 1. Move startup utility functions into GeckoViewUtils;

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

Comment 26

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906689 [details]
Bug 1367077 - 2. Move PromptService startup to BrowserCLH;

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

Comment 27

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906690 [details]
Bug 1367077 - 3. Move form fill event listeners out of browser.js;

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

Comment 28

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906705 [details]
Bug 1367077 - 4. Remove LoginManagerParent.login;

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

Comment 29

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906688 [details]
Bug 1367077 - 1. Move startup utility functions into GeckoViewUtils;

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

Comment 30

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906689 [details]
Bug 1367077 - 2. Move PromptService startup to BrowserCLH;

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

Comment 31

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906690 [details]
Bug 1367077 - 3. Move form fill event listeners out of browser.js;

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

Comment 32

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906705 [details]
Bug 1367077 - 4. Remove LoginManagerParent.login;

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

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3af67a62be1b
1. Move startup utility functions into GeckoViewUtils; r=snorp
https://hg.mozilla.org/integration/autoland/rev/215f47ca940c
2. Move PromptService startup to BrowserCLH; r=snorp
https://hg.mozilla.org/integration/autoland/rev/8a3e6fbd6511
3. Move form fill event listeners out of browser.js; r=sebastian
https://hg.mozilla.org/integration/autoland/rev/6c65c331b97f
4. Remove LoginManagerParent.login; r=kmag

Comment 34

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing mochitest-chrome's test_hidden_select_option.html and test_select_disabled.html on Android:

https://hg.mozilla.org/integration/autoland/rev/763af7d6686da5cabb27188dafd70d9ef07ce9f0
https://hg.mozilla.org/integration/autoland/rev/12144eb1c101220118c1d39b32e23ecc1677240e
https://hg.mozilla.org/integration/autoland/rev/5384d984aa5314e80d9a12bffe85e5f036681c4a
https://hg.mozilla.org/integration/autoland/rev/1083f0e1147bb86c69bc3074e5557b10f13d1d7a

Push which ran failing tests: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=84bd9434491c9391a6e961e63e15807224dbeb77&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=130708779&repo=autoland

 139 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_hidden_select_option.html | uncaught exception - ReferenceError: XPCOMUtils is not defined at @chrome://browser/content/SelectHelper.js:6:1
160 INFO TEST-UNEXPECTED-FAIL | mobile/android/tests/browser/chrome/test_select_disabled.html | uncaught exception - ReferenceError: XPCOMUtils is not defined at @chrome://browser/content/SelectHelper.js:6:1

Comment 35

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

See also these reftest failures: https://treeherder.mozilla.org/logviewer.html#?job_id=130708788&repo=autoland

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/682c9d79073c
1. Move startup utility functions into GeckoViewUtils; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/72e591f41777
2. Move PromptService startup to BrowserCLH; r=snorp
https://hg.mozilla.org/integration/mozilla-inbound/rev/17f9dd20314a
3. Move form fill event listeners out of browser.js; r=sebastian
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bb25516598f
4. Remove LoginManagerParent.login; r=kmag

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f01cd7cd431c
Follow-up to fix bustage; r=me on CLOSED TREE

Comment 39

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/682c9d79073c
https://hg.mozilla.org/mozilla-central/rev/72e591f41777
https://hg.mozilla.org/mozilla-central/rev/17f9dd20314a
https://hg.mozilla.org/mozilla-central/rev/5bb25516598f
https://hg.mozilla.org/mozilla-central/rev/f01cd7cd431c

Comment 40

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906688 [details]
Bug 1367077 - 1. Move startup utility functions into GeckoViewUtils;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178416/diff/5-6/

Comment 41

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906689 [details]
Bug 1367077 - 2. Move PromptService startup to BrowserCLH;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178418/diff/5-6/

Comment 42

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906690 [details]
Bug 1367077 - 3. Move form fill event listeners out of browser.js;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/178420/diff/5-6/

Comment 43

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906705 [details]
Bug 1367077 - 4. Remove LoginManagerParent.login;

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

Comment 44

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8906688 [details]
Bug 1367077 - 1. Move startup utility functions into GeckoViewUtils;

Beta request for all patches.

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Custom tabs feature slated for 57 would not have the form/login fill feature.
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Not really
[Why is the change risky/not risky?]: Well-tested locally and the patches only just missed the Beta merge by a few hours.
[String changes made/needed]: None

Comment 45

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8906688 [details]
Bug 1367077 - 1. Move startup utility functions into GeckoViewUtils;

This shouldn't need an uplift request to get into 57, since we are going to sync m-c to m-b again.