634176 - LoginManager child script does not need to use WebProgressListener

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

Historically, the LoginManager XPCOM component used a WebProgressListener (WPL) to be able to listen for page loads, from session history. It did this globally using the docloader service and also used the listener to attach the DOMContentLoaded event.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/src/nsLoginManager.js#177
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/src/nsLoginManager.js#303

When we ported to e10s, we kept the WPL, but used the local docShell / webProgress instance and not the global docloader. We no longer needed to use the listener to attach the DOMContentLoaded event either. The only reason we used the listener in e10s was for listening for pages loaded from session history (back button).

We can do that using pageshow, instead of DOMContentLoaded, at the cost of being slightly later in handling the password auto-fill. "pageshow" fires slightly later than "DOMContentLoaded"

But removing the WPL is worth the small cost. WPL implemented in JS have been shown to slow pageloads.

This patch simply removes the WPL and uses "pageshow" event to handle the new pageloads and session history pageloads.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 512399 [details] [diff] [review]
patch

More eyes is better. And I will make some browser-chrome tests for this in m-b.

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

There is a few things I don't understand in the code you remove, I understand the need to use a WPL for pages already in the cache, but I wonder why this code path is checking the "remember" flag and not the traditional pageshow (previously DOMContentLoaded)?
  if (!this._pwmgr._remember)
    return;

(Same question for the check for an HTML Document)

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #2)
> There is a few things I don't understand in the code you remove, I understand
> the need to use a WPL for pages already in the cache, but I wonder why this
> code path is checking the "remember" flag and not the traditional pageshow
> (previously DOMContentLoaded)?
>   if (!this._pwmgr._remember)
>     return;
> 
> (Same question for the check for an HTML Document)

Thats easy to answer. It's a bug from when we ported the code to e10s. Previously, the WPL was used to attach the DOMContentLoaded event, which means both of those tests (rememeber and HTMLDocument) had to pass before we attached the event.

We need to add those checks back into our code, regardless of my changes.

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #3)
> (In reply to comment #2)
> > There is a few things I don't understand in the code you remove, I understand
> > the need to use a WPL for pages already in the cache, but I wonder why this
> > code path is checking the "remember" flag and not the traditional pageshow
> > (previously DOMContentLoaded)?
> >   if (!this._pwmgr._remember)
> >     return;
> > 
> > (Same question for the check for an HTML Document)
> 
> Thats easy to answer. It's a bug from when we ported the code to e10s.
> Previously, the WPL was used to attach the DOMContentLoaded event, which means
> both of those tests (rememeber and HTMLDocument) had to pass before we attached
> the event.
> 
> We need to add those checks back into our code, regardless of my changes.

Does it mean you're going to update your patch here?

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #4)

> > We need to add those checks back into our code, regardless of my changes.
> 
> Does it mean you're going to update your patch here?

Yeah

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch 2

Same as last patch, but I add 2 checks in the pageshow handler

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/f8ff4f26700f