Closed Bug 845546 Opened 11 years ago Closed 11 years ago

port b2g identity implementation to desktop

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INVALID

People

(Reporter: jedp, Assigned: jedp)

References

Details

Attachments

(5 files, 7 obsolete files)

part 1 of 5; native sign-in using persona service (wip)
102.83 KB, patch
Details | Diff | Splinter Review
part 2 of 5; native sign-in using persona service - toolkit (wip)
53.47 KB, patch
Details | Diff | Splinter Review
part 3 of 5; native sign-in using persona service - dom (wip)
28.68 KB, patch
Details | Diff | Splinter Review
part 4 of 5; native sign-in using persona service - browser (wip)
78.32 KB, patch
MattN
: feedback-
Details | Diff | Splinter Review
part 5 of 5; native sign-in using persona service - b2g (wip)
25.22 KB, patch
Details | Diff | Splinter Review 
Host the persona dialog in an ephemeral hidden iframe; make it visible in chrome when user interaction is needed (e.g., in a PopupNotification doorhanger).  The DOM implementation should not need to change.
Work in progress.

There are some blantant ux issues, but I'd love to get some feedback on whether the overall implementation is sound.

Currently, signing into a website works; signing in with SignInToBrowser.request() from chrome is not fully functional.

Note - to test, use http://picl.123done.org - necessary because it loads a special version of the persona include.js from picl.personatest.org.
Attachment #720064 - Flags: feedback?(mixedpuppy)
Attachment #720064 - Attachment is patch: true
Comment on attachment 720064 [details] [diff] [review]
wip - native sign into website and sign into browser


I'm primarily reviewing the code overall rather than this patch, and am doing so from a branch with the patch applied.  I also dont have a deep understanding of this code/architecture yet, so I'm just hitting some low hanging fruit.

># HG changeset patch
># User Jed Parsons <jparsons@mozilla.com>
># Parent c65d59d33aa86b7e75bc420ea3beda6201e0aceb
>
>diff --git a/browser/base/content/browser-identity.js b/browser/base/content/browser-identity.js
>new file mode 100644
>--- /dev/null
>+++ b/browser/base/content/browser-identity.js

I would create a top level class here, such as IdentityUI, with an init that is handling all the addListener calls.  You can see where other inits are called in browser.js.   That will help avoid a bunch of globals.  Double quotes seems to be the standard in most of the firefox code.

>+XPCOMUtils.defineLazyModuleGetter(this, 'getLogger',
>+                                  'resource://gre/modules/identity/LogUtils.jsm');
>+
>+let _logger = getLogger('shim');
>+let log = _logger.log;
>+let reportError = _logger.reportError;

getting the logger/reportError like this defeats the lazy getter above, I noticed this in several files.  You can do something like this:

XPCOMUtils.defineLazyGetter(IdentityUI, "log", function () {
  if (!IdentityUI._log)
    IdentityUI._log = getLogger("shim").log;
  return IdentityUI._log;
});

In the modules, you can also just do:

XPCOMUtils.defineLazyGetter(this, "log", function () ...

>+// BrowserID.internal.logout expects this to be on the function prototype.
>+// And who doesn't like a nice, spicy curry?
>+if (!Function.prototype.curry) {
>+  Function.prototype.curry = function() {

don't modify the function prototype.


>+const kIdentityDelegateWatch = 'identity-delegate-watch';

Get rid of the constants for the observer topics unless you really need them in other content files.  It might be better to define them as part of a class in one of the modules if this is necessary.

>+var options = {};
>+var isLoaded = false;
>+var func = null;

Get rid of the globals (put them in the class), many of them are too generically named for the global namespace.


>+function doInternalWatch() {
>+  log('doInternalWatch: isLoaded:', isLoaded, 'options:', options);
>+  if (options && isLoaded) {
>+    let BrowserID = content.wrappedJSObject.BrowserID;

I'd be highly suspect of wrappedJSObject access and security issues.  You should consider a different mechanism for this, perhaps a postmessage api.

>+addEventListener('DOMContentLoaded', function(e) {
>+  content.addEventListener('load', function(e) {
>+    log('content loaded');
>+    sendAsyncMessage('identity-delegate-loaded');
>+    isLoaded = true;
>+     // bring da func
>+     if (func) func();
>+  });
>+});

What content load are you trying to listen for here?  This feels like you could potentially end up with multiple load listeners.


>+addMessageListener(kIdentityDelegateRequest, function(aMessage) {
>+  log('received:', kIdentityDelegateRequest);
>+  options = aMessage.json;
>+  func = doInternalRequest;
>+  func();

You should just pass the json as an argument.  I am not clear about what you are doing with 'func'.  I see it is used in the load listener, but I feel like I am missing something.

>diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml
>--- a/browser/base/content/urlbarBindings.xml
>+++ b/browser/base/content/urlbarBindings.xml
>@@ -1117,264 +1117,70 @@

>+  <binding 
>+    id="identity-request-notification" 
>+    extends="chrome://global/content/bindings/notification.xml#popup-notification">

In my way-back memories of xbl, you have to be careful of what you do in constructor/destructors as they don't necessarily fire when you think they would.

I'm also wondering if some of these notifications can instead rely on popupshowing/popuphidden events, that may become clearer to me later.


>diff --git a/browser/modules/SignInToWebsite.jsm b/browser/modules/SignInToWebsite.jsm
>--- a/browser/modules/SignInToWebsite.jsm
>+++ b/browser/modules/SignInToWebsite.jsm

>+XPCOMUtils.defineLazyModuleGetter(this, 'getLogger',
>+                                  'resource://gre/modules/identity/LogUtils.jsm');
>+
>+let _logger = getLogger('SignIn');
>+let log = _logger.log;
>+let reportError = _logger.reportError;

ditto on lazy getters, check all the files for this.

>+function getUIForWindowID(aWindowId) {

I'll want to revisit this once I understand the code better.

>+function HiddenWindowLoadListener(aWindow, aCallback) {

It seems like you're waiting for the hidden window to be loaded, the doing the callback.  Unless I'm missing something, you should be able to do this with a regular load listener.

>+let HostFrame = {
>+  _listener: null,
>+  _iframe: null,
>+  _gotIframe: false,
>+
>+  get iframe() {
>+    return this._iframe;
>+  },

this getter doesn't seem to be used anywhere, just remove it.

>+  get _frame() {
>+    delete this._frame;
>+    return this._frame = Services.appShell.hiddenDOMWindow;
>+  },

calling this 'frame' is confusing later in the code, since there is _frame, _iframe, I'd rather see 'hiddenWindow'.  iirc deleting class members like this has some kind of memory/performance penalty in the js engine.

>+  getIframe: function HostFrame_getIframe(aOptions, aCallback) {
>+    if (this._gotIframe) {
>+      reportError("Can only get iframe once with HostFrame helper");
>+      return;
>     }

I am not seeing where _gotIframe is set, as well it seems you could just check this._iframe.

>+this.SignInToWebsiteUX = {
>+  init: function SignInToWebsiteUX_init() {
>+    this.contexts = {};
>+    Services.obs.addObserver(this, 'identity-controller-watch', false);
>+    Services.obs.addObserver(this, 'identity-controller-request', false);
>+    Services.obs.addObserver(this, 'identity-controller-logout', false);
>+    Services.obs.addObserver(this, 'identity-controller-canceled', false);
>+  },
>+
>+  uninit: function SignInToWebsiteUX_uninit() {
>+    Services.obs.removeObserver(this, 'identity-controller-watch');
>+    Services.obs.removeObserver(this, 'identity-controller-request');
>+    Services.obs.removeObserver(this, 'identity-controller-logout');
>+  },

should identity-controller-canceled be removed also?


>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js
>--- a/modules/libpref/src/init/all.js
>+++ b/modules/libpref/src/init/all.js
>@@ -375,17 +375,17 @@ pref("toolkit.telemetry.server", "https:
> pref("toolkit.telemetry.server_owner", "Mozilla");
> // Information page about telemetry (temporary ; will be about:telemetry in the end)
> pref("toolkit.telemetry.infoURL", "http://www.mozilla.com/legal/privacy/firefox.html#telemetry");
> // Determines whether full SQL strings are returned when they might contain sensitive info
> // i.e. dynamically constructed SQL strings or SQL executed by addons against addon DBs
> pref("toolkit.telemetry.debugSlowSql", false);
> 
> // Identity module
>-pref("toolkit.identity.enabled", false);
>+pref("dom.identity.enabled", false);
> pref("toolkit.identity.debug", false);

just curious, why a pref name change?

>diff --git a/toolkit/identity/LogUtils.jsm b/toolkit/identity/LogUtils.jsm
>--- a/toolkit/identity/LogUtils.jsm
>+++ b/toolkit/identity/LogUtils.jsm

it seems like we have a logging module somewhere...yes, downloads, I think sync has/had one too.  can you make a bug to consolidate those into some generic location?

browser/components/downloads/src/DownloadsLogger.jsm


>diff --git a/toolkit/identity/MinimalIdentity.jsm b/toolkit/identity/MinimalIdentity.jsm
>--- a/toolkit/identity/MinimalIdentity.jsm
>+++ b/toolkit/identity/MinimalIdentity.jsm

>+function getHiddenWindow() {
>+  return Services.appShell.hiddenDOMWindow;
>+}

not used, remove it.

>+function SignInToBrowser() {
>+  this._contexts = {};
>+}
>+
>+SignInToBrowser.prototype = {
>+  _createContext: function SignInToBrowser__createContext() {
>+    let id = topWindowID();

I need to understand the intended architecture better, as I'm not convinced about per-window persona login yet. iiuc, that is what you need a window id for.
Attachment #720064 - Flags: feedback?(mixedpuppy) → feedback+
(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> Comment on attachment 720064 [details] [diff] [review]
> wip - native sign into website and sign into browser
>
>
> I'm primarily reviewing the code overall rather than this patch, and am
> doing so from a branch with the patch applied.  I also dont have a deep
> understanding of this code/architecture yet, so I'm just hitting some low
> hanging fruit.

Many thanks for diving in and giving me such thorough feedback.

> >+++ b/browser/base/content/browser-identity.js
>
> I would create a top level class here, such as IdentityUI, with an init that
> is handling all the addListener calls.  You can see where other inits are
> called in browser.js.   That will help avoid a bunch of globals.  Double
> quotes seems to be the standard in most of the firefox code.

Thanks.  (Sorry, I got infected by single quotes in gaia land :)  I've completely refactored browser-identity.js to encapsulate all the logic in the IdentityShim class.

> >+XPCOMUtils.defineLazyModuleGetter(this, 'getLogger',
> >+
> >'resource://gre/modules/identity/LogUtils.jsm');
> >+
> >+let _logger = getLogger('shim');
> >+let log = _logger.log;
> >+let reportError = _logger.reportError;
>
> getting the logger/reportError like this defeats the lazy getter above, I
> noticed this in several files.  You can do something like this:

Yes, that was sloppy.  I've redone the logger and am now lazily getting a singleton logger.

> >+// BrowserID.internal.logout expects this to be on the function prototype.
> >+// And who doesn't like a nice, spicy curry?
> >+if (!Function.prototype.curry) {
> >+  Function.prototype.curry = function() {
>
> don't modify the function prototype.

Fixed.  The browserid internal api was expecting some callbacks to have a curry method on the function prototype.  I've landed a PR to fix this in browserid (https://github.com/mozilla/browserid/pull/3069) and removed the prototype modification here.

> >+const kIdentityDelegateWatch = 'identity-delegate-watch';
>
> Get rid of the constants for the observer topics unless you really need them
> in other content files.  It might be better to define them as part of a
> class in one of the modules if this is necessary.

Removed

> >+var options = {};
> >+var isLoaded = false;
> >+var func = null;
>
> Get rid of the globals (put them in the class), many of them are too
> generically named for the global namespace.

Done

> >+function doInternalWatch() {
> >+  log('doInternalWatch: isLoaded:', isLoaded, 'options:', options);
> >+  if (options && isLoaded) {
> >+    let BrowserID = content.wrappedJSObject.BrowserID;
>
> I'd be highly suspect of wrappedJSObject access and security issues.  You
> should consider a different mechanism for this, perhaps a postmessage api.

More on this after the next paragraph ...

 > >+addEventListener('DOMContentLoaded', function(e) {
 > >+  content.addEventListener('load', function(e) {
 > >+    log('content loaded');
 > >+    sendAsyncMessage('identity-delegate-loaded');
 > >+    isLoaded = true;
 > >+     // bring da func
 > >+     if (func) func();
 > >+  });
 > >+});
 >
 > What content load are you trying to listen for here?  This feels like you
 > could potentially end up with multiple load listeners.

I've refactored this a bit.  But basically what's happening is that we have a sandboxed iframe that sources the browserid/persona include.js with the result that the persona dialog is hosted in the iframe.  When the persona code loads, it exposes a JS api for some of the persona login/logout functions.  When the content is loaded and code is evaluated, this file, which is injected into the iframe, can access the BrowserID object in the iframe.

Regarding the security question of content.wrappedJSObject.BrowserID: I thought this would be an ok way to work with the hosted javascript in the sandboxed iframe; if it's not, I need to ponder this some more.

 >
 > >+addMessageListener(kIdentityDelegateRequest, function(aMessage) {
 > >+  log('received:', kIdentityDelegateRequest);
 > >+  options = aMessage.json;
 > >+  func = doInternalRequest;
 > >+  func();
 >
 > You should just pass the json as an argument.  I am not clear about what you
 > are doing with 'func'.  I see it is used in the load listener, but I feel
 > like I am missing something.

I've refactored this and hope I have made it clearer.

 > >diff --git a/browser/base/content/urlbarBindings.xml
 > >b/browser/base/content/urlbarBindings.xml
 > >--- a/browser/base/content/urlbarBindings.xml
 > >+++ b/browser/base/content/urlbarBindings.xml
 > >@@ -1117,264 +1117,70 @@
 >
 > >+  <binding
 > >+    id="identity-request-notification"
 > >+
 > >extends="chrome://global/content/bindings/notification.xml#popup-notification">
 >
 > In my way-back memories of xbl, you have to be careful of what you do in
 > constructor/destructors as they don't necessarily fire when you think they
 > would.

Yes, I've discovered some exciting results myself!  I'm beginning to think that I don't want to use a popup/doorhanger for this anyway; I'd like the login frame to persist if a user changes tabs, and as far as I can tell, the popup is always dismissed the moment your focus moves to something else.  So I think I want to implement the UI component differently.

 > I'm also wondering if some of these notifications can instead rely on
 > popupshowing/popuphidden events, that may become clearer to me later.

Good point.  If I find I need to stick with the popup in the near term, I will look into that.

 > >+function getUIForWindowID(aWindowId) {
 >
 > I'll want to revisit this once I understand the code better.
 >
 > >+function HiddenWindowLoadListener(aWindow, aCallback) {
 >
 > It seems like you're waiting for the hidden window to be loaded, the doing
 > the callback.  Unless I'm missing something, you should be able to do this
 > with a regular load listener.

I ignorantly copied this from another module; I've removed it.

 > >+let HostFrame = {
 > >+  _listener: null,
 > >+  _iframe: null,
 > >+  _gotIframe: false,
 > >+
 > >+  get iframe() {
 > >+    return this._iframe;
 > >+  },
 >
 > this getter doesn't seem to be used anywhere, just remove it.

Removed

 > >+  get _frame() {
 > >+    delete this._frame;
 > >+    return this._frame = Services.appShell.hiddenDOMWindow;
 > >+  },
 >
 > calling this 'frame' is confusing later in the code, since there is _frame,
 > _iframe, I'd rather see 'hiddenWindow'.  iirc deleting class members like
 > this has some kind of memory/performance penalty in the js engine.

I removed this.  Reading the documentation, I think I should always have access to the read-only hiddenDOMWindow, and nothing was gained by doing this anyway (or?)

 > >+  getIframe: function HostFrame_getIframe(aOptions, aCallback) {
 > >+    if (this._gotIframe) {
 > >+      reportError("Can only get iframe once with HostFrame helper");
 > >+      return;
 > >     }
 >
 > I am not seeing where _gotIframe is set, as well it seems you could just
 > check this._iframe.

Dead code.  Removed

 > >+this.SignInToWebsiteUX = {
 > >+  init: function SignInToWebsiteUX_init() {
 > >+    this.contexts = {};
 > >+    Services.obs.addObserver(this, 'identity-controller-watch', false);
 > >+    Services.obs.addObserver(this, 'identity-controller-request', false);
 > >+    Services.obs.addObserver(this, 'identity-controller-logout', false);
 > >+    Services.obs.addObserver(this, 'identity-controller-canceled', false);
 > >+  },
 > >+
 > >+  uninit: function SignInToWebsiteUX_uninit() {
 > >+    Services.obs.removeObserver(this, 'identity-controller-watch');
 > >+    Services.obs.removeObserver(this, 'identity-controller-request');
 > >+    Services.obs.removeObserver(this, 'identity-controller-logout');
 > >+  },
 >
 > should identity-controller-canceled be removed also?

Yes, thanks.

 >
 > >diff --git a/modules/libpref/src/init/all.js
 > >b/modules/libpref/src/init/all.js
 > >--- a/modules/libpref/src/init/all.js
 > >+++ b/modules/libpref/src/init/all.js
 > >@@ -375,17 +375,17 @@ pref("toolkit.telemetry.server", "https:
 > > pref("toolkit.telemetry.server_owner", "Mozilla");
 > > // Information page about telemetry (temporary ; will be about:telemetry
 > > in the end)
 > > pref("toolkit.telemetry.infoURL",
 > > "http://www.mozilla.com/legal/privacy/firefox.html#telemetry");
 > > // Determines whether full SQL strings are returned when they might
 > > contain sensitive info
 > > // i.e. dynamically constructed SQL strings or SQL executed by addons
 > > against addon DBs
 > > pref("toolkit.telemetry.debugSlowSql", false);
 > >
 > > // Identity module
 > >-pref("toolkit.identity.enabled", false);
 > >+pref("dom.identity.enabled", false);
 > > pref("toolkit.identity.debug", false);
 >
 > just curious, why a pref name change?

The original one was wrong, actually; it's supposed to be dom.identity.enabled and toolkit.identity.debug (unless i've totally mixed myself up)

 > >diff --git a/toolkit/identity/LogUtils.jsm b/toolkit/identity/LogUtils.jsm
 > >--- a/toolkit/identity/LogUtils.jsm
 > >+++ b/toolkit/identity/LogUtils.jsm
 >
 > it seems like we have a logging module somewhere...yes, downloads, I think
 > sync has/had one too.  can you make a bug to consolidate those into some
 > generic location?
 >
 > browser/components/downloads/src/DownloadsLogger.jsm

I have filed Bug 848569 and submitted a patch for this; I've asked the author of DownloadsLogger, csonne, for feedback.

 >
 > >diff --git a/toolkit/identity/MinimalIdentity.jsm
 > >b/toolkit/identity/MinimalIdentity.jsm
 > >--- a/toolkit/identity/MinimalIdentity.jsm
 > >+++ b/toolkit/identity/MinimalIdentity.jsm
 >
 > >+function getHiddenWindow() {
 > >+  return Services.appShell.hiddenDOMWindow;
 > >+}
 >
 > not used, remove it.

Removed

 > >+function SignInToBrowser() {
 > >+  this._contexts = {};
 > >+}
 > >+
 > >+SignInToBrowser.prototype = {
 > >+  _createContext: function SignInToBrowser__createContext() {
 > >+    let id = topWindowID();
 >
 > I need to understand the intended architecture better, as I'm not convinced
 > about per-window persona login yet. iiuc, that is what you need a window id
 > for.

I've since gotten some UI guidance that renders this approach to SignInToBrowser obsolete.  So I'll remove it from the patch.

Many thanks for all your feedback.  I've updated the patch and will continue to revise it.  Cheers, j
Attached patch native sign into website (v3) (obsolete) — — Splinter Review 
This patch adds a dynamic resize watcher for the hosted iframe.

This patch is also simplified by the removal of the SignInToBrowserService; we will be using that through a completely different UI.
Attachment #721933 - Attachment is obsolete: true
I don't think the doorhanger is the best container for the UI, but I don't know of a good alternative.  The problem with the doorhanger is that it cancels itself the moment you focus away from it.  It should be possible to change tabs, click elsewhere, copy and paste, etc., without canceling the flow.  

It would be better to have a chrome panel container that persisted over the content in a tab and that would not go away until it was dismissed either by completion of the dialog or by the user canceling it.

I don't know of any interface like that for Firefox.  Is there one?  Does it need to be written?
Ben, regarding the question in Comment #6, do you think the use of the doorhanger is a blocker for trying to get this landed in nightly, preffed off?
Flags: needinfo?(benadida)
(In reply to Jed Parsons [:jparsons] from comment #7)
> Ben, regarding the question in Comment #6, do you think the use of the
> doorhanger is a blocker for trying to get this landed in nightly, preffed
> off?

I don't think so, the previous preffed off stuff was worse :)
Flags: needinfo?(benadida)
(In reply to Jed Parsons [:jparsons] from comment #6)
> I don't think the doorhanger is the best container for the UI, but I don't
> know of a good alternative.  The problem with the doorhanger is that it
> cancels itself the moment you focus away from it.  It should be possible to
> change tabs, click elsewhere, copy and paste, etc., without canceling the
> flow.  

The problem that your server iframe gets unloaded when the popup gets hidden, right?

> It would be better to have a chrome panel container that persisted over the
> content in a tab and that would not go away until it was dismissed either by
> completion of the dialog or by the user canceling it.
> 
> I don't know of any interface like that for Firefox.  Is there one?  Does it
> need to be written?

The closest thing that I know is tab-modal prompts (used when a content page uses alert/confirm/etc.).  You could probably build on top of that? See toolkit/components/prompts/content/tabprompts.xml

(In reply to Ben Adida [:benadida] from comment #8)
> I don't think so, the previous preffed off stuff was worse :)

The previous UI persisted its state so you could continue where you left off and didn't cancel the flow.
(In reply to Matthew N. [:MattN] from comment #9)
> (In reply to Jed Parsons [:jparsons] from comment #6)
> > I don't think the doorhanger is the best container for the UI, but I don't
> > know of a good alternative.  The problem with the doorhanger is that it
> > cancels itself the moment you focus away from it.  It should be possible to
> > change tabs, click elsewhere, copy and paste, etc., without canceling the
> > flow.  
> 
> The problem that your server iframe gets unloaded when the popup gets
> hidden, right?

Actually, I'm more worried about the UX problem.  I think the user should be able to click around (maybe copy and paste things, maybe check another tab) before completing the flow.

> > It would be better to have a chrome panel container that persisted over the
> > content in a tab and that would not go away until it was dismissed either by
> > completion of the dialog or by the user canceling it.
> > 
> > I don't know of any interface like that for Firefox.  Is there one?  Does it
> > need to be written?
> 
> The closest thing that I know is tab-modal prompts (used when a content page
> uses alert/confirm/etc.).  You could probably build on top of that? See
> toolkit/components/prompts/content/tabprompts.xml

Ooh, I didn't know about those.  And there's some more documentation on mdn.  Is there a way to make them appear visually over chrome UI and not only over content?  I'll read up on these.  Thanks, Matthew!
(In reply to Jed Parsons [:jparsons] from comment #10)
> (In reply to Matthew N. [:MattN] from comment #9)
> > (In reply to Jed Parsons [:jparsons] from comment #6)
> > > I don't think the doorhanger is the best container for the UI, but I don't
> > > know of a good alternative.  The problem with the doorhanger is that it
> > > cancels itself the moment you focus away from it.  It should be possible to
> > > change tabs, click elsewhere, copy and paste, etc., without canceling the
> > > flow.  
> > 
> > The problem that your server iframe gets unloaded when the popup gets
> > hidden, right?
> 
> Actually, I'm more worried about the UX problem.  I think the user should be
> able to click around (maybe copy and paste things, maybe check another tab)
> before completing the flow.

I confused about the UX problem because PopupNotifications allow you to do that but the contents of the panel gets recreated. If you want to have the panel re-open when the tab is reactivated, that shouldn't be hard to do.

> > > It would be better to have a chrome panel container that persisted over the
> > > content in a tab and that would not go away until it was dismissed either by
> > > completion of the dialog or by the user canceling it.
> > > 
> > > I don't know of any interface like that for Firefox.  Is there one?  Does it
> > > need to be written?
> > 
> > The closest thing that I know is tab-modal prompts (used when a content page
> > uses alert/confirm/etc.).  You could probably build on top of that? See
> > toolkit/components/prompts/content/tabprompts.xml
> 
> Ooh, I didn't know about those.  And there's some more documentation on mdn.
> Is there a way to make them appear visually over chrome UI and not only over
> content?  I'll read up on these.  Thanks, Matthew!

If you mean, over chrome-privileged pages, then it should work the same.  If you mean above the browser's chrome (e.g tabstrip), then you would need to change what stack the tabmodalprompt (or equivalent replacement) gets appended to.
Attached patch native sign into website (v4) (obsolete) — — Splinter Review 
Hi, Ben; In response to comment #8, I'd like to land this patch so we can start iterating on it with a broader audience.  It's still preffed off by default, so there should be no new risks.
Attachment #722555 - Attachment is obsolete: true
Attachment #729185 - Flags: review?(benadida)
Comment on attachment 729185 [details] [diff] [review]
native sign into website (v4)

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

I like it! Go.
Attachment #729185 - Flags: review?(benadida) → review+

        Attachment #729185 -
        Attachment is obsolete: true

    
    

    
  
Whoops; MattN pointed out some trailing whitespace
Keywords: checkin-needed

    
    

    
  


  
Fixed whitespace nits

        Attachment #729216 -
        Attachment is obsolete: true

    
    

    
  
tbpl errors on module name change. fixing.
Keywords: checkin-needed

    
    

    
  


  
fixed broken tests

        Attachment #729226 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Jed Parsons [:jparsons] from comment #20)
> Here we go:
> https://tbpl.mozilla.org/?tree=Try&rev=84bdc2411284

You're joking, right? Filing a new failure as an intermittent issue when your patch directly touches that component? Backed out because guess what, B2G xpcshell failed on inbound too.
https://hg.mozilla.org/integration/mozilla-inbound/rev/56e0d7ad86f2

https://tbpl.mozilla.org/php/getParsedLog.php?id=21163849&tree=Mozilla-Inbound

09:44:38     INFO -  TEST-INFO | /builds/slave/test/build/tests/xpcshell/tests/b2g/components/test/unit/test_signintowebsite.js | running test ...
09:49:38  WARNING -  TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/xpcshell/tests/b2g/components/test/unit/test_signintowebsite.js | test failed (with xpcshell return code: -1), see following log:
09:49:38     INFO -  >>>>>>>
09:49:38     INFO -  Traceback (most recent call last):
09:49:38     INFO -    File "runtestsb2g.py", line 64, in launchProcess
09:49:38     INFO -      outputFile = XPCShellRemote.launchProcess(self, cmd, stdout, stderr, env, cwd)
09:49:38     INFO -    File "/builds/slave/test/build/tests/xpcshell/remotexpcshelltests.py", line 276, in launchProcess
09:49:38     INFO -      self.shellReturnCode = self.device.shell(cmd, f)
09:49:38     INFO -    File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/mozdevice/devicemanagerADB.py", line 138, in shell
09:49:38     INFO -      raise DMError("Timeout exceeded for shell call")
09:49:38     INFO -  DMError: Timeout exceeded for shell call
09:49:38     INFO -  <<<<<<<
09:49:39  WARNING -  TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/xpcshell/tests/b2g/components/test/unit/test_signintowebsite.js | Process still running after test!
09:49:39     INFO -  >>>>>>>
09:49:39     INFO -  Traceback (most recent call last):
09:49:39     INFO -    File "runtestsb2g.py", line 64, in launchProcess
09:49:39     INFO -      outputFile = XPCShellRemote.launchProcess(self, cmd, stdout, stderr, env, cwd)
09:49:39     INFO -    File "/builds/slave/test/build/tests/xpcshell/remotexpcshelltests.py", line 276, in launchProcess
09:49:39     INFO -      self.shellReturnCode = self.device.shell(cmd, f)
09:49:39     INFO -    File "/builds/slave/test/build/venv/local/lib/python2.7/site-packages/mozdevice/devicemanagerADB.py", line 138, in shell
09:49:39     INFO -      raise DMError("Timeout exceeded for shell call")
09:49:39     INFO -  DMError: Timeout exceeded for shell call
09:49:39     INFO -  <<<<<<<
09:49:39     INFO -  INFO | Result summary:
09:49:39     INFO -  INFO | Passed: 99
09:49:39  WARNING -  INFO | Failed: 2
09:49:39  WARNING -  One or more unittests failed.

    
    

    
  
Yup.  Sorry about that, everyone.

    
    

    
  
Did this get sr from a browser peer, and just didn't get flag updated?

It looks like it's mostly identity code, but there're some changes to urlbar, and some API changes (which typically warrant at least a cursory sr).

Doesn't hurt to ask, so CCing the likely suspects!

    
    

    
  
That patch is essentially all browser/ code, and so it requires review from a Firefox peer (or their delegate). Sounds like perhaps we should meet to discuss this?
Component: Identity → General
Product: Core → Firefox

    
    

    
  
Sorry, not "essentially all", I missed some parts of the patch. But there are significant browser/ changes there.

    
    

    
  
Err, yes, this clearly needed review from a browser peer as well as ui-review.

    
    

    
  
I'm happy to get together and discuss, and again, sorry for the screw-up.

    
    

    
  
Justin, Gavin, sorry this is my bad, I was pushing Jed to land this under the theory that this was mostly tweaks to an already agreed-upon implementation path from a few months ago, but I agree that there is more here. Let's meet.

    
    

    
  
Comment on attachment 729266 [details] [diff] [review]
native sign into website (r=benadida)

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

Dumping some initial comments after a quick first pass. I still need to understand what SignInToWebsite.jsm is doing, so I didn't look too closely at it yet. Let me know if that led to some nonsensical comments!

General issues:

* I'd like to think more about the impact of using remote content for all the UI here. This is going to be fairly frequently used (we hope!), but places UI responsiveness at the mercy of the network/server. Which somewhat contrary to other efforts to improve snappyness / responsiveness. I'll go ahead with some review comments on the as-is form, though.

* What's the difference between Identity.jsm and MinimalIdentity.jsm?

* You might consider spinning of the logging changes to a separate bug, just because that should all be dead simple, and easy peasy to get landed. But if it's all tangled up at this point, doesn't really matter.

* The usages of <destructor> and addObserver makes me worry about memory leaks. But I'm unclear on some of the other stuff going on here (ie, it may change significantly), so I'm not going to chase that down.

::: browser/base/content/browser-identity.js
@@ +2,5 @@
> +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
> +/* 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/. */
> +

Naming nit: all the other "browser-foo.js" files here are segments of the main browser.js, so having a browser-identity.js which is actually a frame script is confusing.

We do have a content.js for that, but from a quick skim this seems like a mix of parent and child code, so I'm not quite sure what's going on.

Alternatively maybe this should live in /toolkit/identity, and gBrowserInit.onLoad can loadFrameScript() it directly.

@@ +4,5 @@
> + * 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/. */
> +
> +// This JS shim contains the callbacks to fire DOMRequest events for
> +// navigator.pay API within the payment processor's scope.

navigatory.pay?

@@ +132,5 @@
> +
> +this.shim = null;
> +
> +addEventListener('DOMContentLoaded', function(e) {
> +  content.addEventListener('load', function(e) {

Why do you wait for |DOMContentLoaded|, but then in turn listen for |load|? Why not simply listen for |load|?

Although TBH, this means none of this shim will get hooked up for a page that's slow to load (say, a stalled image). And even DOMContentLoaded would be nice to avoid hooking into, since it's firing for _every_ page. Hmm, why is any of this in a frame script in the first place?

@@ +139,5 @@
> +    this.shim.init();
> +  });
> +});
> +
> +content.addEventListener('beforeunload', function(e) {

Nope. This breaks fastback caching. Use |pagehide|.

https://developer.mozilla.org/en-US/docs/Using_Firefox_1.5_caching

::: browser/base/content/urlbarBindings.xml
@@ +1128,4 @@
>  
> +    <content>
> +      <panel id="persona-container" anonid="persona-container" flex="1" />
> +    </content>

This should probably be a <popupnotificationcontent>.

@@ +1134,2 @@
>        <constructor><![CDATA[
> +        dump(" ** in identity xul constructor\n");

You shouldn't be landing debugging dump()s. [Formalized logging, behind a pref, is of course OK.]

@@ +1142,3 @@
>  
> +        // adopt the iframe and display it in the panel
> +        // XXX this does not work in FF3.6 or older

Thankfully mozilla-central is not interested in 3.6 support.

Also, please no "XXX" / "TODO" unless it references a filed followup bug.

@@ +1164,5 @@
> +      <destructor><![CDATA[
> +        if (!this.complete) {
> +          Services.obs.notifyObservers(this.messageSubject, "identity-delegate-canceled", null);
> +        }
> +        Services.obs.removeObserver(this, "identity-delegate-ui-close", false);

This should be a eventCallback, see:

https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/PopupNotifications.jsm#Notification_events

@@ +1175,4 @@
>          <body><![CDATA[
> +          // The only message we observe is identity-delegate-ui-close
> +          this.complete = true;
> +          this.notification.remove();

There's minimal code in this binding now, I suspect you'll be better off (and the code will be simpler) by setting this all up in SignInToWebsite.jsm (when the notification is created).

::: browser/modules/SignInToWebsite.jsm
@@ +16,3 @@
>  
> +const kIdentityScreen = 'https://picl.personatest.org/sign_in#NATIVE';
> +const kIdentityFrame = 'https://picl.personatest.org/communication_iframe';

personatest.org? Is this a testing server or a pre-deployment server? Presumably these are temporary URLs?

@@ +116,5 @@
> +  if (content) {
> +    let browser = content.QueryInterface(Ci.nsIInterfaceRequestor)
> +                         .getInterface(Ci.nsIWebNavigation)
> +                         .QueryInterface(Ci.nsIDocShell).chromeEventHandler;
> +    //let browser = someWindow.gBrowser;

Remove commented out code.

@@ +139,5 @@
> +  let mainAction = {
> +    label: UI.chromeWin.gNavigatorBundle.getString('identity.next.label'),
> +    accessKey: UI.chromeWin.gNavigatorBundle.getString('identity.next.accessKey'),
> +    callback: function() {} // required
> +  };

This should just be |mainAction= null|.

@@ +182,5 @@
> +   * true, attach the iframe to a xul panel in the popup notification.
> +   * Otherwise attach to a hidden document.
> +   */
> +  _createIframe: function HostFrame_createIframe(aOptions) {
> +    let srcURI = aOptions.showUI ? kIdentityScreen : kIdentityFrame;

What's |showUI| doing? What's going on in the hiddenDocument iframe? Not really sure what's going on here.

@@ +191,5 @@
> +
> +    this._iframe.setAttribute('mozbrowser', true);
> +    this._iframe.setAttribute('mozframetype', 'content');
> +    this._iframe.setAttribute('type', 'content');
> +    this._iframe.setAttribute('remote', true);

Hmm, what does |remote| do on desktop Firefox? Anything? Surprise us in the future? May want to remove this?

@@ +261,5 @@
>  
> +  _delegateLoaded: function pipe__delegateLoaded() {
> +    this.mm.sendAsyncMessage(this.options.message, this.options.rpOptions);
> +    //this.resizer = new DynamicResizeWatcher();
> +    //this.resizer.start(

Remove commented out code.

@@ +319,5 @@
> +
> +  observe: function SignInToWebsiteUX_observe(aSubject, aTopic, aData) {
> +    logger.log('controller observed:', aTopic);
> +    // XXX need to detect page unload of any of our flows
> +    // XXX we get strings from xul, and objects from elsewhere

What are these XXX for? Need bug numbers, if they're legit followups.

::: modules/libpref/src/init/all.js
@@ +388,1 @@
>  pref("toolkit.identity.debug", false);

Rename the debug pref too?

::: toolkit/identity/MinimalIdentity.jsm
@@ -247,5 @@
> -  },
> -
> -
> -  /*
> -   * XXX Bug 804229: Implement Identity Provider Functions

If this (and the other "XXX bug #####" comments being removed) are no longer relevant, you should close them.

    
    

    
  
Comment on attachment 729266 [details] [diff] [review]
native sign into website (r=benadida)

>diff --git a/browser/modules/SignInToWebsite.jsm b/browser/modules/SignInToWebsite.jsm

>+  _createIframe: function HostFrame_createIframe(aOptions) {

>+    this._iframe.setAttribute('mozbrowser', true);

I don't think this has any effect on desktop, given that dom.mozBrowserFramesEnabled defaults to false there.

>+    this._iframe.setAttribute('mozframetype', 'content');
>+    this._iframe.setAttribute('type', 'content');

"mozframetype" and "type" are redundant - an element uses one or the other depending on whether it's a XUL element or HTML element:
http://hg.mozilla.org/mozilla-central/annotate/e53044984f1b/content/base/src/nsFrameLoader.h#l381

So it seems like you want just the "mozframetype" here.

    
    

    
  
Justin, Gavin, thanks for your feedback and suggestions.  I'm working on some revisions now (unit tests and mocks, primarily), which I will try to submit today along with a full explanation of what SignInToWebsite is doing, and how the persona hidden vs visible iframes function.  Cheers, j

    
    

    
  


  
Part 1 of 5: Files removed from all components

        Attachment #729266 -
        Attachment is obsolete: true

        Attachment #738659 -
        Attachment description: part 1 of 5; native sign-in using persona service - dom (wip) → part 3 of 5; native sign-in using persona service - dom (wip)

    
    

    
  
Hi, all,

Since this patch represents a change in direction from previous identity work, it necessarily touches a lot of files (60, to be exact).  It also can't be implemented gradually, because the new direction is largely incompatible with the old.

To ease the pain and confusion, I have broken the patch into five parts.
1. the files that were deleted (across all components)
2-5. the parts of the patch that belong to toolkit, dom, browser, and b2g

The point of this patch is to provide a native IdentityService that makes use of the hosted Persona include code to perform client logic and communicate with the persona services.  This is how the identity service has already been implemented on b2g.  On desktop, it will still be considered experimental and will be preffed off.

We learned a lot from our previous implementation, and have chosen to go this route for several reasons:  It frees us from having to re-implement and build UI from scratch for the complex Persona dialog, a daunting task that was not fully completed with the previous native implementation.  It enables updates to Persona (security as well as UI) to be used instantly in firefox.  It saves us the headache of chasing a slowly moving target (Persona is still in beta) and trying to retain parity across two independently-developed projects; this way, the browser-centric component is in the browser, and the Persona-centric component is in persona.  

It works as follows:  The dom component provides navigator.mozId calls used by persona RPs (relying parties).  When a website running on firefox includes the Persona include.js file, the code makes the Persona navigator.id.* alias for navigator.mozId.*.  Otherwise, the website just runs the shimmed javascript as usual.  So web site authors don't do anything differently.

The navigator.mozId methods send observer notifications that are received by the IdentityService module (toolkit/identity).  This service will also provide methods for use by native code and add-ons for sign-in-to-browser functionality.  It is the identity hub in firefox.

To sign in to a website, the IdentityService in turn messages the SignInToWebsite module (browser/modules), which uses iframes to load and interact with hosted Persona code through the Persona internal api.  There are two separate iframes: If user interaction is needed (entering a password, eg), then the 'sign_in' frame is used.  Otherwise the 'communication_iframe' is loaded and the browserid internal_api calls are used.

A communication 'Pipe' inside the SignInToWebsite module does the work of creating a worker frame for sourcing the necessary Persona code.  Into this frame, the browser-identity.js file is injected.  This contains the chrome code calls that enable the iframe to message back from content to chrome with the results of the internal api calls.  The Pipe communicates the results back to the SignInToWebsite controller and disposes of the iframe.

In the present patch, the interactive iframe is hosted in a doorhanger.  This is not an optimal solution for the long-term, but is adequate for our present desire to have something to start working with and testing in nightly builds.

    
    

    
  
(In reply to Justin Dolske [:Dolske] from comment #30)
> Comment on attachment 729266 [details] [diff] [review]
> native sign into website (r=benadida)
> 
> Review of attachment 729266 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Dumping some initial comments after a quick first pass. 

Thank you for reading and for your comments and help!

> * I'd like to think more about the impact of using remote content for all
> the UI here.

Additional UI work is needed by the Persona team to make a dialog suitable for in-browser presentation.  We are already in our second pass at the dialog for hosting in b2g; desktop firefox will need to come next.  In the interim, I'm just using the b2g dialog.  For the purposes of testing, it should be sufficient for the short term, but yes, not for the long run.

> This is going to be fairly frequently used (we hope!), but
> places UI responsiveness at the mercy of the network/server. 
> Which somewhat contrary to other efforts to improve snappyness / 
> responsiveness. I'll go ahead with some review comments on the 
> as-is form, though.

This is true.  But persona only works as a networked service anyway.  The Persona team have been working diligently (Shane and François in particular) to put Persona on a diet.  There is more information about that effort here: https://id.etherpad.mozilla.org/b2g-persona-performance

In the end, we'll be able to have a footprint of all the js components of 90k or less.  With cacheing and local fonts, I think this should feel pretty snappy.

> * What's the difference between Identity.jsm and MinimalIdentity.jsm?

Identity.jsm was the original implementation from last year; I've renamed MinimalIdentity.jsm, which supplants Identity.jsm, to IdentityService and removed Identity.jsm.

The difference is that before, we were doing all of the logic, state management, primary and secondary IdP resolution, etc., inside Identity.jsm and friends.  We replace all that with IdentityService.jsm, which is a hub for bridging between dom calls or other chrome use and the SignInToWebsite module, which interacts with persona.

> * You might consider spinning of the logging changes to a separate bug, just
> because that should all be dead simple, and easy peasy to get landed. But if
> it's all tangled up at this point, doesn't really matter.

Yes, in retrospect I regret not having done that.  I'm afraid it's too late now :)

> * The usages of <destructor> and addObserver makes me worry about memory
> leaks. But I'm unclear on some of the other stuff going on here (ie, it may
> change significantly), so I'm not going to chase that down.

I'm trying to learn more about this in connection with your other comments about the eventCallback from the popup notification.  If I can put all this logic inside the chrome code, I would love that.

> ::: browser/base/content/browser-identity.js
> @@ +2,5 @@
> > +/* vim: set shiftwidth=2 tabstop=2 autoindent cindent expandtab: */
> > +/* 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/. */
> > +
> 
> Naming nit: all the other "browser-foo.js" files here are segments of the
> main browser.js, so having a browser-identity.js which is actually a frame
> script is confusing.

Names are important!  What about browser-identity-frame-script.js?

> We do have a content.js for that, but from a quick skim this seems like a
> mix of parent and child code, so I'm not quite sure what's going on.

Yes - the parent and child code enables the iframe that includes the persona logic to message back to the SignInToWebsite module.

> Alternatively maybe this should live in /toolkit/identity, and
> gBrowserInit.onLoad can loadFrameScript() it directly.
> 
> @@ +4,5 @@
> > + * 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/. */
> > +
> > +// This JS shim contains the callbacks to fire DOMRequest events for
> > +// navigator.pay API within the payment processor's scope.
> 
> navigatory.pay?

LOL.  That little fragment of DNA shows something of the evolutionary history of this approach.  Fixed :)

> @@ +132,5 @@
> > +
> > +this.shim = null;
> > +
> > +addEventListener('DOMContentLoaded', function(e) {
> > +  content.addEventListener('load', function(e) {
> 
> Why do you wait for |DOMContentLoaded|, but then in turn listen for |load|?
> Why not simply listen for |load|?

I confess I don't understand exactly how this works.  I think the issue has to do with timing of the loadFrameScript call?  In any case, if I remove the DOMContentLoaded listener-wrapper, it stops working.

> Although TBH, this means none of this shim will get hooked up for a page
> that's slow to load (say, a stalled image). And even DOMContentLoaded would
> be nice to avoid hooking into, since it's firing for _every_ page. Hmm, why
> is any of this in a frame script in the first place?

Is this going to load on every page?  It should only load on pages that call navigator.id methods.

I've tried to explain the frame script bit a little better in Comment #38.  Please let me know if that doesn't clarify sufficiently.

> @@ +139,5 @@
> > +    this.shim.init();
> > +  });
> > +});
> > +
> > +content.addEventListener('beforeunload', function(e) {
> 
> Nope. This breaks fastback caching. Use |pagehide|.
> 
> https://developer.mozilla.org/en-US/docs/Using_Firefox_1.5_caching

Thanks.  Fixed.

> ::: browser/base/content/urlbarBindings.xml
> @@ +1128,4 @@
> >  
> > +    <content>
> > +      <panel id="persona-container" anonid="persona-container" flex="1" />
> > +    </content>
> 
> This should probably be a <popupnotificationcontent>.

Hmm.  I tried that and it stopped working.  Tell me more - why not content?

> @@ +1134,2 @@
> >        <constructor><![CDATA[
> > +        dump(" ** in identity xul constructor\n");
> 
> You shouldn't be landing debugging dump()s. [Formalized logging, behind a
> pref, is of course OK.]

Blech!  No kidding.  Fixed.

> @@ +1142,3 @@
> >  
> > +        // adopt the iframe and display it in the panel
> > +        // XXX this does not work in FF3.6 or older
> 
> Thankfully mozilla-central is not interested in 3.6 support.

LOL.  Yes, as I was trying to figure out the iframe bit across chrome and xul, which I found rather challenging, I wrote down every silly little note I came across.  Silly little note removed.

> Also, please no "XXX" / "TODO" unless it references a filed followup bug.

Yes, before anything is flagged for submission, I will be sure to do that.

> @@ +1164,5 @@
> > +      <destructor><![CDATA[
> > +        if (!this.complete) {
> > +          Services.obs.notifyObservers(this.messageSubject, "identity-delegate-canceled", null);
> > +        }
> > +        Services.obs.removeObserver(this, "identity-delegate-ui-close", false);
> 
> This should be a eventCallback, see:

As above, I need to learn more about this.  Perhaps I can ping you on irc for some quick help?

> https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/
> PopupNotifications.jsm#Notification_events
> 
> @@ +1175,4 @@
> >          <body><![CDATA[
> > +          // The only message we observe is identity-delegate-ui-close
> > +          this.complete = true;
> > +          this.notification.remove();
> 
> There's minimal code in this binding now, I suspect you'll be better off
> (and the code will be simpler) by setting this all up in SignInToWebsite.jsm
> (when the notification is created).

Ditto above

> ::: browser/modules/SignInToWebsite.jsm
> @@ +16,3 @@
> >  
> > +const kIdentityScreen = 'https://picl.personatest.org/sign_in#NATIVE';
> > +const kIdentityFrame = 'https://picl.personatest.org/communication_iframe';
> 
> personatest.org? Is this a testing server or a pre-deployment server?
> Presumably these are temporary URLs?

Yes, these are temporary URLs.  There's a separate process going on to get the necessary native api enhancements needed for b2g and native desktop into the main Persona branch.  Until that merge is complete, we are using an ephemeral integration server.

> @@ +116,5 @@
> > +  if (content) {
> > +    let browser = content.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                         .getInterface(Ci.nsIWebNavigation)
> > +                         .QueryInterface(Ci.nsIDocShell).chromeEventHandler;
> > +    //let browser = someWindow.gBrowser;
> 
> Remove commented out code.

Yes.

> @@ +139,5 @@
> > +  let mainAction = {
> > +    label: UI.chromeWin.gNavigatorBundle.getString('identity.next.label'),
> > +    accessKey: UI.chromeWin.gNavigatorBundle.getString('identity.next.accessKey'),
> > +    callback: function() {} // required
> > +  };
> 
> This should just be |mainAction= null|.

Fixed

> @@ +182,5 @@
> > +   * true, attach the iframe to a xul panel in the popup notification.
> > +   * Otherwise attach to a hidden document.
> > +   */
> > +  _createIframe: function HostFrame_createIframe(aOptions) {
> > +    let srcURI = aOptions.showUI ? kIdentityScreen : kIdentityFrame;
> 
> What's |showUI| doing? What's going on in the hiddenDocument iframe? Not
> really sure what's going on here.

This signals to the SignInToWebsite delegate that the UI needs to be visible for this persona function.  This is necessary when the navigator.id.request() method is called (because the user needs to sign in).  For other operations like watch and logout, the ui is not needed, in which case the doorhanger does not appear.

> @@ +191,5 @@
> > +
> > +    this._iframe.setAttribute('mozbrowser', true);
> > +    this._iframe.setAttribute('mozframetype', 'content');
> > +    this._iframe.setAttribute('type', 'content');
> > +    this._iframe.setAttribute('remote', true);
> 
> Hmm, what does |remote| do on desktop Firefox? Anything? Surprise us in the
> future? May want to remove this?

removed :)

> @@ +261,5 @@
> >  
> > +  _delegateLoaded: function pipe__delegateLoaded() {
> > +    this.mm.sendAsyncMessage(this.options.message, this.options.rpOptions);
> > +    //this.resizer = new DynamicResizeWatcher();
> > +    //this.resizer.start(
> 
> Remove commented out code.

Uck. yes.

> @@ +319,5 @@
> > +
> > +  observe: function SignInToWebsiteUX_observe(aSubject, aTopic, aData) {
> > +    logger.log('controller observed:', aTopic);
> > +    // XXX need to detect page unload of any of our flows
> > +    // XXX we get strings from xul, and objects from elsewhere
> 
> What are these XXX for? Need bug numbers, if they're legit followups.

Yes, I will file all necessary follow-ups as we get close to landing.

> ::: modules/libpref/src/init/all.js
> @@ +388,1 @@
> >  pref("toolkit.identity.debug", false);
> 
> Rename the debug pref too?

These names accidentally morphed in a previous checkin of mine.

> ::: toolkit/identity/MinimalIdentity.jsm
> @@ -247,5 @@
> > -  },
> > -
> > -
> > -  /*
> > -   * XXX Bug 804229: Implement Identity Provider Functions
> 
> If this (and the other "XXX bug #####" comments being removed) are no longer
> relevant, you should close them.

Removed

Thank you again for your help!
j

    
    

    
  
> "mozframetype" and "type" are redundant - an element uses one or the other
> depending on whether it's a XUL element or HTML element:
> http://hg.mozilla.org/mozilla-central/annotate/e53044984f1b/content/base/src/
> nsFrameLoader.h#l381
> 
> So it seems like you want just the "mozframetype" here.

Aha!  Thanks for the clarification.  Fixed.

    
    

    
  
(In reply to Jed Parsons [:jparsons] from comment #39)
> Names are important!  What about browser-identity-frame-script.js?

"browser" is a bit redundant, identity-frame-script.js sounds good.

> I confess I don't understand exactly how this works.  I think the issue has
> to do with timing of the loadFrameScript call?  In any case, if I remove the
> DOMContentLoaded listener-wrapper, it stops working.

It sounds like maybe you're getting events that you're not expecting? Perhaps you need an event.target check, or to filter out load events for about:blank loads, or some such. Need to understand what's going on here better.

> Is this going to load on every page?  It should only load on pages that call
> navigator.id methods.

Frame scripts are loaded for every tab, and so the listener you're adding will be fired for effectively every load, yes. So actually it sounds like this should be triggered by the navigator.id getter rather than be loaded in a frame script?

> > This should probably be a <popupnotificationcontent>.
> 
> Hmm.  I tried that and it stopped working.  Tell me more - why not content?

I'm pretty confused by this binding - identity-request-notification is applied to a <popupnotification> element that lives inside a <panel> (the doorhanger panel). So it doesn't really make sense for its <content> to then also include a <panel>. It looks like that's just being used as a generic container for an iframe, though, so a better approach might be to not use PopupNotifications at all, and instead just use a custom panel (similar to socialActivatedNotification)?

    
    

    
  
Note that Part 3 here should pick up the change from attachment 742017 [details] [diff] [review] (in bug 861495). Trivial.

    
    

    
  
Comment on attachment 738660 [details] [diff] [review]
part 4 of 5; native sign-in using persona service - browser (wip)

This patch loads the identity selector and other resources from a Persona server which doesn't seem appropriate for a "decentralized protocol"[1]. This leads to various concerns:

1) To be properly decentralized, my browser shouldn't have to talk to persona.org if I'm using my own IdP.
2) Single point of failure: If the persona server or even just some portions are not working, I cannot login using my own IdP.
3) Privacy: Having the native browser UI connect to a Persona server to load the identity selector, possibly passing the site name and logo for site-specific theming[2] is an invasion of privacy IMO.
4) Performance (speed and memory usage): Having to load, parse and render an iframe containing the identity selector has higher memory overhead and takes much longer than showing a XUL list of identities. Consider regions that don't have fast internet. I know work has gone into reducing the footprint for B2G but it's still more overhead than not using an iframe.
5) Moving target: Having browser UI/UX hosted remotely could possibly make it a moving target for testing, bug filing, regression-range finding, etc. With a truly native BrowserID implementation, if an RP and IdP don't change and I'm able to login with Firefox 21 on the day of the release then this will continue to be true two weeks later. With a remotely-hosted unversioned UI, this isn't necessarily the case as the persona server could change and prevent me from logging into any BrowserID RPs. Making this work would require significant coordination between many teams, such as Identity, QA, Release Management, Firefox, etc., in order to understand what gets fixed when/where and how that aligns with the release trains.

Considering all of these issues, I don't think we should proceed with this approach. The existing approach in mozilla-central doesn't suffer from the problems above and is consistent with the development of other Firefox front-end features.

The main downsides I can remember about a XUL native UI (i.e. not remotely hosted) is that the BrowserID spec is still changing and so there may be times when we'd like to add/change/remove some feature with a faster turnaround time than riding the trains. I suggest that:
a) We only implement features (e.g. new properties such as siteName/siteLogo) that we're willing to have remain in the Firefox release channel for at least 6 weeks (when the next version is released).
b) If BrowserID/Persona is unstable enough that major changes may be needed, perhaps we shouldn't be doing a native implementation yet.
c) More experimental features can be tested on persona.org before implementing them natively.
d) Changes can ride the trains like every other WebAPI. See #5 above.

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Persona
[2] http://identity.mozilla.com/post/27122712140/new-feature-adding-your-websites-name-and-logo-to-the

        Attachment #738660 -
        Flags: feedback-

    
    

    
  
Ok folks.  This bug was opened over 6 months ago.  Federated sign in as implemented by persona has important strategic, security, and privacy properties.  I believe these objections are a case of perfect being the enemy of good, and demonstrate us, mozilla, shooting ourselves in the foot.

mattn, I think your points are reasonable, I think your heart is in the right place, but I think this is an area where we need to be pragmatic and sacrifice a perfect implementation for one that moves forward our initiatives.

Why do I claim this is a strategic neccesity?  Here's a couple ways Persona native can really help us:
1. a native implementation (regardless of its implementation details) in firefox desktop demonstrates to the world our commitment to persona, which is a barrier to adoption.
2. the popup window is percieved by many to be the worst part about persona, and it's crappy ux.  there's a huge security and ux win by having persona appear in a native container that could not be spoofed by content.
3. we have opportunities to improve the next version of our sync product which is now on a fast implementation track if we have native persona (email verification is a significant dropoff point, if we can reduce that user friction, we can get more people using sync, and make more people happy).

Now point by point by your completely reasonable concerns:

#1 Is an absolutely correct ultimate goal, and I can promise to make this so in a subsequent iteration.

#2 persona is one of the highest availability systems at mozilla.  We've got triple DC redundancy and at low cost and high speed we can land a DC on every continent on the planet.  Futher, in the next month we're going to move to a CDN which will make persona load times faster than popular competing sign-in systems.

And in over a year of running the service, we've had no multi-day outages.  Also, mozilla is building a NOC and one of the top 3 priorities for near-instantaneous discovery and escalation of service interruption - is persona  (the others are marketplace and product delivery).

#3 you're right.  But persona as it stands is more privacy protecting than any other social sign-in service - the privacy gain by people adopting persona as it stands is huge - the privacy regression between a hybrid browser native impl and the current state of the world is zero.  We're trying to change the world here, and we have to value our adoption strategy as much as we value the technical perfection of the implementation.

#4 You're right.  But if we can get away with it on a FirefoxOS device with 256mb ram, then why can't we on the desktop where we likely have over a gig?  Again, I can promise you that we'll go full native.

#5  We can apply the exact same pattern we do for firefoxos.  Our dedicated QA resources take full responsibility for testing for regression every two weeks.  If we affect firefox, we block a train from rolling.  If we screw up and roll a subtle regression, we rollback.  I'm directly accountable if we mess up.

Finally, the key downside of a XUL native UI is that we do not yet have a market proven product.  Things are looking extrememly good, however we need the ability to respond quickly when we get feedback from high value users of persona (like our own properties, or the community, or extremely large websites).  The process of proving ourselves in the market is going to be to listen and adapt.  We've made quick and decisive changes to win over folks behind mozillians, open badges, and tons of sites in the community.  Persona in Firefox though, will add required credibility to the product and be a sign of our commitment.  It will accelerate the project and give us an important tool to win over folks.

And if we commit to persona as it stands today, and jam it into a native UI, that adds a maintenence cost that will slow us down and make us less competitive.  Sure we can do it, and sure we can probably do it quickly.  But I've distributed a lot of client software in my day, and versioning and backwards compatibility and having to *break* users totally sucks, and totally ties your hands.  It's inevitable, but we've got about 4 little protocol changes we know we need to make and maybe about 6 months before we can get out of beta and commit.  

We've finally got an incredibly talented UX designer focused full time on persona and the impact he's had in the last two months is astounding.  Let's take this opportunity to learn a little more and have a product that absolutely shines.

I think the best way to push forward and see if we can't fundamentally change the way people sign in.  I agree we're going against the status quo here, but I think it's with very good reason.  

mattn, will you support us?

    
    

    
  
Lloyd: your arguments seem to stem from an assumption that any solution other than the one proposed would result in too much delay getting shipped or development friction, and I'm not confident that that needs to be the case (maybe it _has_ been the case, but we can do something different). Matt's reasons 4 and 5 are quite persuasive, and I don't think we get to push them off to "later". I think you/dolske/I need to get together and figure out how we get this done right, and fast.

    
    

    
  
Gavin: Yeah, I am worried that re-implementing the entirety of Persona's front-end logic natively could or should get done this year.  I expect there's going to be plenty of areas where we find we actually need to change the protocol and implementation in order for it to work well in native firefox and be truly de-centralized.  (mattn, jedp, and austin have already dug through this, confirmend that changes are required, and come up with a solid plan).  I think it is important for this that we don't cut corners.  We're going to be asking other browser vendors to implement persona, and they're going to have exactly the same concerns.

I want us to have the time and space to properly integrate persona into firefox.

My thinking is that, however, websites considering adopting persona need a strong commitment from mozilla to persona.  Also, the popup flow is a major source of concern for many.  Landing this patch, I believe, solves these *external* facing problems and buys us the time to do a second pass (which has already started) without time pressure.  I think this would help us parallelize persona adoption and supporting the sync effort, while we build the true native integration that we all want.

Side-note: The relationship to sync here is pretty straightforward - during set up users need to verify their email addresses, and that's what persona is all about.  I'm going to be working with product and the picl team to figure out the fit more succinctly. 

So let's totally get together, I'll try to explain in more depth why I think it's so value to land an imperfect intermediate step first, while we actively work toward a native integration that is truly distributed / more responsive / and has an optimized memory footprint.

    
    

    
  
Just to clarify, Sign in to the Web (Persona) work is tracked in Bug#882884 and is using a hybrid approach instead of the B2G style proposed here.

I'm going to close this bug invalid, as our strategy has changed with Fx Accounts. We can file a new bug with a plan for Sign in to the Browser when we're focused on Sign in to Desktop.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → INVALID

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: