1149975 - Handle visibility of the login fill doorhanger anchor

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[:Paolo Amadini]

This controls the visibility of the login fill doorhanger anchor.

Comment 1

[:Paolo Amadini]

Attachment: Initial work

This is to check that the event system works. It shows and hides the icon based only on the presence of a login form.

This patch is implemented on top of the current other patches, when those are updated then the call to Doorhangers.find() can become a call to LoginDoorhangers.find().

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8586737 [details] [diff] [review]
Initial work

Review of attachment 8586737 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ +259,5 @@
> +   * the password fill doorhanger.
> +   */
> +  onLoginFormPresence(window, loginFormPresent) {
> +    let messageManager = messageManagerFromWindow(window);
> +    messageManager.sendAsyncMessage("RemoteLogins:onLoginFormPresence",

It seems like you can rename the existing "LoginStats:LoginEncountered" message and use it instead. loginFormPresent will always be true for that message. We can have a different LoginManagerContent handler for page navigations that sends the message where you currently have loginFormPresent == false.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm
@@ +522,5 @@
> +   * Called to indicate whether a login form on the currently loaded page is
> +   * present or not. This is one of the factors used to control the visibility
> +   * of the password fill doorhanger.
> +   */
> +  onLoginFormPresence(browser, loginFormPresent) {

The "on" prefix makes me think this is called only if a login form is present. Separating the message and using separate methods would help clarify this. If we want a single method then I think s/on/update/ in the method name makes the purpose more clear.

Comment 3

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #2)
> It seems like you can rename the existing "LoginStats:LoginEncountered"
> message and use it instead.

It makes sense, though this message is currently sent from onFormPassword, that is called both on Desktop and Android:

http://mxr.mozilla.org/mozilla-central/search?string=onFormPassword

The "updateLoginFormPresence" would have been Desktop-only instead. Actually, it might be a good thing to have a single set of events for all platforms and differentiate which UI to show later in the process. Do you think we should do that, or use the separate event for now?

Comment 4

[:Paolo Amadini]

I was also thinking we might land this glue code first, with the timeout that prevents accessing Services.logins right after startup and the wait on promiseInitialized, to see if it has a performance impact on first page load.

Do you know if Talos for try builds is now reliable enough to do meaningful comparisons?

Comment 5

[:Paolo Amadini]

Another aspect to keep into account is to ensure that, when we perform the fill, we are doing it in the same context as the one that caused the icon to be displayed.

In other words, if a page causes the anchor icon to be displayed, then the user opens the doorhanger with a pre-selected login for that page, we should ensure that, if a navigation happens in the background, we don't disrupt the interaction (we don't close the doorhanger unexpectedly, as the user may be navigating it), but we don't allow manual filling into a different site. To do that, we could disable the option to fill dynamically, or display some warning about filling credentials for a different site.

I've seen other code paths using a "requestId" to ensure the context is the same.

In this case, the situation is more complex, and I was thinking:

* On navigation, we send the formOrigin in addition to the form presence to the parent.
* If the panel is closed, we reset the filter and set the formOrigin to use. If the panel is open, we'll remember these values and use them only after the panel is closed by the user.
* If the panel is open, and the formOrigin of the panel doesn't match the new formOrigin, we update the panel UI, but don't change the filter or panel state yet.
* To avoid race conditions, when the user chooses to fill, we only carry out the operation in the child if the original formOrigin matches. If the user chose the special "fill into different site" UI, we can provide the new formOrigin instead, even if the panel hasn't been closed in the meantime.

Maybe we should also do the match based on additional information like the exact focused form? In case of pages with frames, do we have the ability to trigger this mechanism based on which frame is focused?

Note that the "fill into different site" interface might even not be implemented initially, if the background navigation case is rare enough - we just disable all filling buttons on navigation or focus change, and the user should close and reopen the panel to access the right passwords.

Can this logic based on formOrigin be applied also to the cases that currently use the requestId mechanism, or are they different enough?

Comment 6

[:Paolo Amadini]

Attachment: More work in progress

A possible implementation, but doesn't look stable yet after some testing.

Comment 7

[Matthew N. [:MattN]]

(In reply to :Paolo Amadini from comment #3)
> (In reply to Matthew N. [:MattN] from comment #2)
> > It seems like you can rename the existing "LoginStats:LoginEncountered"
> > message and use it instead.
> 
> It makes sense, though this message is currently sent from onFormPassword,
> that is called both on Desktop and Android:
> 
> http://mxr.mozilla.org/mozilla-central/search?string=onFormPassword
> 
> The "updateLoginFormPresence" would have been Desktop-only instead.
> Actually, it might be a good thing to have a single set of events for all
> platforms and differentiate which UI to show later in the process. Do you
> think we should do that, or use the separate event for now?

I think we should use one event that describes what event is actually happening and then different consumers can do what they want with the information. It's no problem if Android doesn't (initially?) use the notification for its UI.

(In reply to :Paolo Amadini from comment #4)
> I was also thinking we might land this glue code first, with the timeout
> that prevents accessing Services.logins right after startup and the wait on
> promiseInitialized, to see if it has a performance impact on first page load.
> 
> Do you know if Talos for try builds is now reliable enough to do meaningful
> comparisons?

My understanding is that Talos for Try builds is still the same hardware and configuration as m-c and integration branches so you should be able to compare between them. The only issue is that you need to use my newer version of compare-talos from https://compare-talos.paas.mozilla.org/ which properly separates PGO and non-PGO for non-try pushes in case you compare a try push to a non-try one with PGO.

Comment 8

[Matthew N. [:MattN]]

(In reply to :Paolo Amadini from comment #5)
> Another aspect to keep into account is to ensure that, when we perform the
> fill, we are doing it in the same context as the one that caused the icon to
> be displayed.
> 
> In other words, if a page causes the anchor icon to be displayed, then the
> user opens the doorhanger with a pre-selected login for that page, we should
> ensure that, if a navigation happens in the background, we don't disrupt the
> interaction (we don't close the doorhanger unexpectedly, as the user may be
> navigating it), but we don't allow manual filling into a different site. To
> do that, we could disable the option to fill dynamically, or display some
> warning about filling credentials for a different site.
> 
> I've seen other code paths using a "requestId" to ensure the context is the
> same.

Yeah, using an identifier (e.g. GUID) as a key to store extra information in the child (e.g. DOM element references) is what I had mentioned before when we were talking about the doorhanger for Android.

> In this case, the situation is more complex, and I was thinking:
> 
> * On navigation, we send the formOrigin in addition to the form presence to
> the parent.

This makes sense as an extra security check anyways and can also be useful to handle cross-origin subframes.

> * If the panel is closed, we reset the filter and set the formOrigin to use.
> If the panel is open, we'll remember these values and use them only after
> the panel is closed by the user.
> * If the panel is open, and the formOrigin of the panel doesn't match the
> new formOrigin, we update the panel UI, but don't change the filter or panel
> state yet.
> * To avoid race conditions, when the user chooses to fill, we only carry out
> the operation in the child if the original formOrigin matches. If the user
> chose the special "fill into different site" UI, we can provide the new
> formOrigin instead, even if the panel hasn't been closed in the meantime.

I'm not sure what you mean by '"fill into different site" UI'… Do you mean that they chose to fill in a login from a different domain, for example? Is this what you mean:
a) User is on example.com
b) User is choosing to fill in login from foo.com
c) Page navigates to example.com/another before b) finishes so we don't end up filling
d) We would have the panel pre-filtered for "foo.com"?

> Maybe we should also do the match based on additional information like the
> exact focused form? In case of pages with frames, do we have the ability to
> trigger this mechanism based on which frame is focused?

I think it's something we should keep in mind now that we're going to be showing the anchor more and providing a new fill mechanism. In some cases we know which form the user cares about since they may have used UI in the context menu or autocomplete dropdown to trigger the doorhanger opening. In the case where the user simply opened the doorhanger from the dismissed doorhanger icon after page load then we will have to be more careful. If all the frames are on the same origin then one option that was discussed was to fill all login forms on the page. When there are frames from different origins maybe we want an anchor-per-origin with the doorhanger stating the origin? Or maybe the doorhanger UI can somehow provide a dropdown to choose the form while it gets highlighted on the page? It seems like something to ask Ryan about but I hope we can find a limitation we can impose in the meantime so we don't necessarily have to solve this complex situation upfront.

> Note that the "fill into different site" interface might even not be
> implemented initially, if the background navigation case is rare enough - we
> just disable all filling buttons on navigation or focus change, and the user
> should close and reopen the panel to access the right passwords.

Yeah, I think this is fine initially (assuming I understand the case properly).

> Can this logic based on formOrigin be applied also to the cases that
> currently use the requestId mechanism, or are they different enough?

Are you asking if the existing consumers that use requestId should instead use the formOrigin?

Comment 9

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #7)
> I think we should use one event that describes what event is actually
> happening and then different consumers can do what they want with the
> information. It's no problem if Android doesn't (initially?) use the
> notification for its UI.

Sounds good, I'll consolidate these into one event. I'll have to make sure Android triggers it at the right time from its browser listeners as well, might need some help with testing.

I suppose updateLoginFormPresence can be a good name for what the event does.

> The only issue is that you need to use my newer
> version of compare-talos from https://compare-talos.paas.mozilla.org/ which
> properly separates PGO and non-PGO for non-try pushes in case you compare a
> try push to a non-try one with PGO.

Thanks for the pointer, this helps :-)

(In reply to Matthew N. [:MattN] from comment #8)
> I'm not sure what you mean by '"fill into different site" UI'… Do you mean
> that they chose to fill in a login from a different domain, for example? Is
> this what you mean:

I think the example I was thinking of is:

a) User is on example.com
b) User is choosing to fill in login from example.com
c) Page navigates to foo.com before b) finishes so we don't end up filling
d) We would have the panel pre-filtered for "example.com" until it's still open,
   and the button to fill would be disabled because it would do nothing

> I hope we can find a limitation we can impose in the
> meantime so we don't necessarily have to solve this complex situation
> upfront.

It could simply be that we send to the parent a formOrigin based on which form is focused.

> > Can this logic based on formOrigin be applied also to the cases that
> > currently use the requestId mechanism, or are they different enough?
> 
> Are you asking if the existing consumers that use requestId should instead
> use the formOrigin?

Yeah.

Comment 10

[:Paolo Amadini]

Boris, I've a question about the page loading process and DOMFormHasPassword, feel free to redirect if there's someone else who can answer.

What we're trying to do is to send to the parent a message saying "we're now loading a page from this origin and it has a password field / hasn't a password field yet". It's fine to send the message multiple times, but the password presence flag and the origin it refers to should be consistent.

I've tried to mix a network listener and an event listener but it isn't working too well. We need something to reset the password field presence flag before DOMFormHasPassword is called. We cannot use DOMContentLoaded since it is fired after that. I don't know if there can be a navigation that results in a different address in the location bar, but without a kind of DOM load event being fired eventually, but if so we'd have to handle that as well.

My attempt was to listen for the initial STATE_START and STATE_IS_NETWORK, but the document itself isn't available at that point, so attempts to get its origin from window.document return null. I could use the request URI, but I fear I might get an origin that is inconsistent compared to the one I'll get from the document and the event later. Also, I'm not sure how this works with redirects.

Excerpt below, full patch attached:

>+let listener = {
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener,
>+                                         Ci.nsISupportsWeakReference]),
>+  onStateChange(webProgress, request, stateFlags, status) {
>+    if (webProgress.isTopLevel &&
>+        (stateFlags & Ci.nsIWebProgressListener.STATE_START)) {
>+      LoginManagerContent.updateLoginFormPresence(content, false);
>+    }
>+  },
>+};
>+docShell.QueryInterface(Ci.nsIInterfaceRequestor)
>+        .getInterface(Ci.nsIWebProgress)
>+        .addProgressListener(listener, Ci.nsIWebProgress.NOTIFY_STATE_NETWORK);
> addEventListener("DOMFormHasPassword", function(event) {
>   InsecurePasswordUtils.checkForInsecurePasswords(event.target);
>   LoginManagerContent.onFormPassword(event);
>+  LoginManagerContent.updateLoginFormPresence(content, true);
> });

Is there a simple way we could do what we want, with the events and network listeners we have, or do we have to define new events?

Comment 11

[Boris Zbarsky [:bzbarsky]]

So the question is at what point the new document actually becomes available?

The DOMWindowCreated event might work for that, I'd think.

Comment 12

[:Paolo Amadini]

Attachment: Work in progress

DOMWindowCreated works, it seems. Android portions untested.

Comment 13

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #9)
> > > Can this logic based on formOrigin be applied also to the cases that
> > > currently use the requestId mechanism, or are they different enough?
> > 
> > Are you asking if the existing consumers that use requestId should instead
> > use the formOrigin?
> 
> Yeah.

In fact, we're working on implementing a fill initiated from the panel later, by the user, and for the automatic fill we might have a similar mechanism, only that it's started automatically? On the other hand the panel is designed to handle the currently focused frame, or just fill for one origin, while autofill might have to work on multiple frames from different origins at the same time?

Comment 14

[:Paolo Amadini]

(In reply to Not doing reviews right now from comment #11)
> The DOMWindowCreated event might work for that, I'd think.

It does. Now I've found a different issue: back-forward navigation. The anchor icon should reflect whether the currently displayed page has any password field in it, but when I navigate back, naturally I don't get any of DOMWindowCreated or DOMFormHasPassword. So, do I have to handle some back-forward navigation events separately in the content script, and recover from somewhere the state indicating whether the icon should be shown? With which object would the state better be associated, the top DOM window? If so, which back-forward navigation event would allow me to access the DOM window I need?

Comment 15

[:Paolo Amadini]

In the meantime, I started two builds for Talos testing.

Without patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e236d6482b18
With patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9adbdf79362

Comment 16

[Boris Zbarsky [:bzbarsky]]

Usually pagehide/pageshow are the events you'd use for back/forward navigation stuff.

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8595345 [details] [diff] [review]
Work in progress

Review of attachment 8595345 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good (ignoring how LoginDoorhangers/Doorhangers are implemented)

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
@@ -265,5 @@
>  
> -    let doc = form.ownerDocument;
> -    let win = doc.defaultView;
> -    let messageManager = messageManagerFromWindow(win);
> -    messageManager.sendAsyncMessage("LoginStats:LoginEncountered");

Nit: I think it may be a bit cleaner to move the message stuff out of onFormPassword to a new updateLoginFormPresence method and leave the rest of onFormPassword separate. This keeps onFormPassword straightforward without having to do branching related to loginFormPresent.

Comment 18

[Tanvi Vyas[:tanvi]]

Handling of frames here is tricky.  In the current password manager, do we autofill or autocomplete in frames?  Does it matter if the frame is same origin or not (or same tld)?  If a.com iframes b.com/login.html which has a login form, does the UI ask if the user wants to save a password for a.com or b.com? 

Perhaps as a first pass, we should show the doorhanger only for top level pages that have a login form.  We can see how that goes and if users like it (it helps them login!) or if they are annoyed by it (this key doorhanger won't go away and i don't want to login!).  If users like it, then we can figure out the frame case and make sure we appropriately update the UI to make it clear to users that they are saving passwords for a domain that is different then what they see in the address bar.

Comment 19

[:Paolo Amadini]

Attachment: MozReview Request: bz://1149975/paolo

/r/8161 - Bug 1149975 - Handle visibility of the login fill doorhanger anchor. r=MattN

Pull down this commit:

hg pull -r 834fe1bf53180a51ce3bbac65b682392b230f1b0 https://reviewboard-hg.mozilla.org/gecko/

Comment 20

[:Paolo Amadini]

(In reply to Tanvi Vyas [:tanvi] from comment #18)
> In the current password manager, do we autofill or autocomplete in frames?

Yes!

> Does it matter if the frame is same origin or not (or same tld)?

No, it isn't important for a manual fill initiated from the page controls.

> If a.com iframes b.com/login.html which has a
> login form, does the UI ask if the user wants to save a password for a.com
> or b.com? 

I think it may ask for b.com or not ask at all, but it will never ask for a.com.

> Perhaps as a first pass, we should show the doorhanger only for top level
> pages that have a login form.  We can see how that goes and if users like it
> (it helps them login!) or if they are annoyed by it (this key doorhanger
> won't go away and i don't want to login!).  If users like it, then we can
> figure out the frame case and make sure we appropriately update the UI to
> make it clear to users that they are saving passwords for a domain that is
> different then what they see in the address bar.

So, what we're doing as a first step is that we're handling same-origin frames only, exactly because we would need to think of a good UX for the case where the origins differ.

Everything will be behind an about:config preference at first.

With regard to determining whether users find the icon useful, I was thinking we might have a way to check this by providing an option to hide it, but this is a separate discussion.

Comment 21

[:Paolo Amadini]

Comment on attachment 8601617 [details]
MozReview Request: bz://1149975/paolo

/r/8161 - Bug 1149975 - Handle visibility of the login fill doorhanger anchor. r=MattN

Pull down this commit:

hg pull -r e3a03aa6382a1167c550162bfcf9c8fc373b2d14 https://reviewboard-hg.mozilla.org/gecko/

Comment 22

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/8159/#review7063

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:552
(Diff revision 2)
> +#ifdef MOZ_SERVICES_HEALTHREPORT
> +    if (loginFormPresent) {
> +      recordFHRDailyCounter("numTotalLoginsEncountered");
> -  }
> +    }
> +#endif

I've just noticed this will now record cases where we updated the login form presence because of "pageshow", including back navigation, even without a new page load. Do you think we should simply keep the previous event associated to DOMFormHasPassword for the statistics?

Comment 23

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #21)
> hg pull -r e3a03aa6382a1167c550162bfcf9c8fc373b2d14
> https://reviewboard-hg.mozilla.org/gecko/

I believe that if you pull this revision you should be able to see the anchor icon in action, since it's not behind a preference yet.

Comment 24

[Tanvi Vyas[:tanvi]]

(In reply to :Paolo Amadini from comment #20)
> (In reply to Tanvi Vyas [:tanvi] from comment #18)
> > In the current password manager, do we autofill or autocomplete in frames?
> 
> Yes!
> 
> > If a.com iframes b.com/login.html which has a
> > login form, does the UI ask if the user wants to save a password for a.com
> > or b.com? 
> 
> I think it may ask for b.com or not ask at all, but it will never ask for
> a.com.

Wrote a quick test case for this with a random login page I found - http://people.mozilla.org/~tvyas/framed_password.html.  If we are asking to save a framed password, we include the origin of the frame, not the top level page.  Screenshot: http://people.mozilla.org/~tvyas/framed_login_doorhanger.png

Comment 25

[Matthew N. [:MattN]]

Comment on attachment 8601617 [details]
MozReview Request: bz://1149975/paolo

https://reviewboard.mozilla.org/r/8159/#review7095

Reminder to put this behind a pref. We should have tests for this new anchor icon eventually.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:322
(Diff revision 2)
> +      for (let i = 0; i < thisWindow.length; i++) {
> +        let frame = thisWindow[i];

Nit: thisWindow.frames is clearer IMO

::: mobile/android/chrome/content/browser.js:4226
(Diff revision 2)
> -        LoginManagerContent.onFormPassword(aEvent);
> +        LoginManagerContent.onDOMFormHasPassword(aEvent,

I would rather we didn't rename this since it's going to be tied less to the specific DOM event as I work on form-less login forms.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:351
(Diff revision 2)
> +  fillForm: function({ document, loginsFound, recipes }) {
> +    if (LoginUtils._getPasswordOrigin(document.documentURI) !=
> +        loginsFound[0].hostname) {
> +      return;
> +    }

We want to support filling passwords from other origins (e.g. subdomains/realms) so this check probably shouldn't stay.

Nit: use the newer method syntax (no `function`)

Comment 26

[Matthew N. [:MattN]]

Comment on attachment 8601617 [details]
MozReview Request: bz://1149975/paolo

There isn't a way to leave the flag while publishing comments. Those were just my initial comments but I wanted you to have them sooner than me finishing the review.

Comment 27

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #25)
> ::: mobile/android/chrome/content/browser.js:4226
> (Diff revision 2)
> > -        LoginManagerContent.onFormPassword(aEvent);
> > +        LoginManagerContent.onDOMFormHasPassword(aEvent,
> 
> I would rather we didn't rename this since it's going to be tied less to the
> specific DOM event as I work on form-less login forms.

This is why I renamed the method, because I find it clearer to indicate that it responds specifically to the DOM event (which is its argument). If we support login fields without a form, we'll probably end up moving part of the implementation or replacing the method altogether.

I could have split the onFormPassword-related calls to another function that doesn't take the event as the argument, just like updateLoginFormPresence. I didn't feel a strong need for separating those lines as those may end up being rewritten anyways.

I'll upload a new patch soon. There are still tweaks needed to the part following  LoginManagerParent.updateLoginFormPresence, but the rest of the code is ready for a final review.

I'll start working on a few base tests, and based on how long it takes I'll see whether to land them in this bug or a follow-up.

Comment 28

[:Paolo Amadini]

Comment on attachment 8601617 [details]
MozReview Request: bz://1149975/paolo

/r/8161 - Bug 1149975 - Handle visibility of the login fill doorhanger anchor. r=MattN

Pull down this commit:

hg pull -r fc4ead6cff56faf62dfdadcd78ccb238bdd697d7 https://reviewboard-hg.mozilla.org/gecko/

Comment 29

[:Paolo Amadini]

I filed bug 1163036 related to the annotation I made in the code about I/O at startup, as I think this may turn out not to be a big issue in practice since we're waiting on the initialization promise. Only if we see an actual Talos regression we may have to investigate adding a delay now.

I rewrote updateLoginFormPresence with DeferredTask and it looks simpler, and now I'm also waiting for the panel to be closed before updating its state. This seems to be working quite well!

Comment 30

[:Paolo Amadini]

Comment on attachment 8601617 [details]
MozReview Request: bz://1149975/paolo

/r/8161 - Bug 1149975 - Handle visibility of the login fill doorhanger anchor. r=MattN

Pull down this commit:

hg pull -r edfa10853034c3c666aedee419c008057fe87fde https://reviewboard-hg.mozilla.org/gecko/

Comment 31

[:Paolo Amadini]

https://reviewboard.mozilla.org/r/8159/#review7225

I've separated the anchor icon for the experimental fill doorhanger since we've forgotten to do that in the first implementation, and I've added a simple browser-chrome test for the manual fill.

I'll file a separate bug to get more detailed tests for the anchor behavior. Most UI tests instead will need to be written specifically for the fianl UI with the sliding subview.

Comment 32

[:Paolo Amadini]

Comment on attachment 8601617 [details]
MozReview Request: bz://1149975/paolo

/r/8161 - Bug 1149975 - Part 1 of 2 - Handle visibility of the login fill doorhanger anchor. r=MattN
/r/8647 - Bug 1149975 - Part 2 of 2 - Test manual login fill. r=MattN

Pull down these commits:

hg pull -r 6f54fc3e5725edd99358bf334c0f104e873383b3 https://reviewboard-hg.mozilla.org/gecko/

Comment 33

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43d231d65445

Comment 34

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/8161/#review7291

::: toolkit/components/passwordmgr/LoginDoorhangers.jsm:263
(Diff revision 5)
> +    if (login) {
> +      LoginManagerParent.fillForm({
> +        browser: this.browser,
> +        loginFormOrigin: this.loginFormOrigin,
> +        login,
> +      }).catch(Cu.reportError);
> +    }

Add an else with Cu.reportError for the case where the selected login can no longer be found.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:383
(Diff revision 5)
> +    if (!topState.loginFormForFill) {
> +      // The form may have been removed, or we have a different document now.
> +      return;
> +    }
> +    if (LoginUtils._getPasswordOrigin(topDocument.documentURI) !=
> +        loginFormOrigin) {
> +      // We navigated to a document from a different site before we had a chance
> +      // to indicate this change in the user interface.
> +      return;
> +    }

Add log(…) messages for debugging before both `return`s. These can probably replace the comments.

::: toolkit/components/passwordmgr/LoginManagerContent.jsm:755
(Diff revision 5)
> -  _fillForm : function (form, autofillForm, clobberPassword,
> +  _fillForm : function (form, autofillForm, clobberUsername, clobberPassword,
>                          userTriggered, foundLogins, recipes) {

We should probably get a bug on file to convert this method to take an options argument.

::: toolkit/components/passwordmgr/LoginManagerParent.jsm:614
(Diff revision 5)
> +    // Once this preference is removed, this version of the fill doorhanger
> +    // should be enabled for Desktop only, and not for Android or B2G.
> +    if (!Services.prefs.getBoolPref("signon.ui.experimental")) {
> +      return;
> +    }

I kind of wonder if desktop-specific UI code should be in a separate module eventually

Comment 35

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/8647/#review7295

::: toolkit/components/passwordmgr/test/browser/browser_filldoorhanger.js:7
(Diff revision 1)
> + * All these tests require the experimantal login fill UI to be enabled. We also

Typo: should be "experimental"

::: toolkit/components/passwordmgr/test/browser/browser_filldoorhanger.js:51
(Diff revision 1)
> +    Assert.equal(list.childNodes.length, 1);

Add a message as the third agument to make output more clear

Comment 36

[Matthew N. [:MattN]]

Comment on attachment 8601617 [details]
MozReview Request: bz://1149975/paolo

r=me with fixes

Comment 37

[:Paolo Amadini]

(In reply to Matthew N. [:MattN] from comment #34)
> We should probably get a bug on file to convert this method to take an
> options argument.

Filed bug 1164469.

> I kind of wonder if desktop-specific UI code should be in a separate module
> eventually

Probably so, especially if it becomes more complex than this, and ideally registered with the Login Manager by the host application.

> Add a message as the third agument to make output more clear

Done, though I think in general we should improve the output clarity in the test suite automatically without requiring a third argument, for example by using the source line as output text.

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ea97ed03aa16
https://hg.mozilla.org/integration/fx-team/rev/03fdeedb03af

Comment 39

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

https://hg.mozilla.org/mozilla-central/rev/ea97ed03aa16
https://hg.mozilla.org/mozilla-central/rev/03fdeedb03af

Comment 40

[Philip Chee]

Hi Paolo,

In non-e10s case (e.g. SeaMonkey) can LoginManagerParent.fillForm() be made to call LoginManagerContent.fillForm() directly instead of going through the message manager?

e.g.
if (Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT) {...}
else {
  LoginManagerContent.fillForm(...);
}

Comment 41

[:Paolo Amadini]

(In reply to Philip Chee from comment #40)
> In non-e10s case (e.g. SeaMonkey) can LoginManagerParent.fillForm() be made
> to call LoginManagerContent.fillForm() directly instead of going through the
> message manager?

In general, with e10s, we want the same code based on the Message Manager to work regardless of whether it is executed in a multi-process environment (Nightly) or not (Beta and Release for example, now I hear SeaMonkey as well). Is it just a curiosity or is there a specific issue you had?

Comment 42

[Kamil Jozwiak [:kjozwiak]]

Paolo, is there anything that QE can do here?

Comment 43

[:Paolo Amadini]

(In reply to Kamil Jozwiak [:kjozwiak] from comment #42)
> Paolo, is there anything that QE can do here?

Hm, being a completely new feature still under development, now I believe it makes more sense to do some QA later, when it will be less subject to change. Resetting the QA flag accordingly.

Comment 44

[Philip Chee]

(In reply to :Paolo Amadini from comment #41)
> (In reply to Philip Chee from comment #40)
> > In non-e10s case (e.g. SeaMonkey) can LoginManagerParent.fillForm() be made
> > to call LoginManagerContent.fillForm() directly instead of going through the
> > message manager?
> 
> In general, with e10s, we want the same code based on the Message Manager to
> work regardless of whether it is executed in a multi-process environment
> (Nightly) or not (Beta and Release for example, now I hear SeaMonkey as
> well). Is it just a curiosity or is there a specific issue you had?

Curiosity, also less work for me on the SeaMonkey side. ;)

Comment 46

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1149975 - Part 1 of 2 - Handle visibility of the login fill doorhanger anchor. r=MattN

Comment 47

[:Paolo Amadini]

Attachment: MozReview Request: Bug 1149975 - Part 2 of 2 - Test manual login fill. r=MattN