Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Description • 12 years ago

In the payment flow, we do:

1. click 'pay'
2. payment ui opens in trusty ui; click login
3. persona login opens in trusty ui;
4. on successful login trusty ui closes

What should happen is that the payment ui should still be visible.

	Kumar McMillan [:kumar]
	Comment 1 • 12 years ago

This is currently blocking basecamp since it blocks the payment window from using persona

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 2 • 12 years ago

Zach and I are working on this together.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 3 • 12 years ago

Gaia pull request:

https://github.com/mozilla-b2g/gaia/pull/6121

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 4 • 12 years ago

Update to TrustedUIManager to enable nested invocations of trusted UI.

Github:  https://github.com/mozilla-b2g/gaia/pull/6121

Vivien, would you mind checking this out for us?

	Zachary Carter [:zaach]
	Comment 5 • 12 years ago

The PR has been updated on github with a fix for 807676. I'm not sure how to create a new attachment from it...

	Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
	Comment 6 • 12 years ago

Comment on attachment 677202 [details] [diff] [review]
make it possible to have a stack of trusty UIs

Fernando wrote this code. Let's ask him a review on it.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 7 • 12 years ago

This is the github patch Zach referred to in Comment #5, with the fix for Bug 807676

	Fernando Jiménez Moreno [:ferjm]
	Comment 8 • 12 years ago

Comment on attachment 677791 [details] [diff] [review]
make it possible to have a stack of trusty UIs

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

Nice patch! Only a few nits and comments.

::: apps/system/js/trusted_ui.js
@@ +40,4 @@
>    },
>  
>    open: function trui_open(name, frame, origin, chromeEventId) {
> +    // XXX origin is never used

It seems that origin is indeed not being used, so can we just remove it along with the comment, please?
Note that you'd also need to remove it from outside calls (payment.js and identity.js).

@@ +107,5 @@
> +    return this.currentStack[this.currentStack.length-1];
> +  },
> +
> +  _pushNewDialog: function trui_PushNewDialog(name, frame, chromeEventId) {
> +    // add some data- to the frame

nit: typo? ("data-")

@@ +146,4 @@
>  
> +    var dialog = this._dialogStacks[this._lastDisplayedApp].pop();
> +    this.container.removeChild(dialog.frame);
> +    // whooosh!

nit: remove this comment, please.

@@ +178,5 @@
>          WindowManager.restoreCurrentApp();
>          this.hide();
>          break;
>        case 'click':
> +        // click what??

nit: remove this comment, please.

@@ +184,4 @@
>            return;
>          this.close();
>          // Notify user closed the trustedUI
> +        this._dispatchCloseEvent(this._getTopDialog().chromeEventId);

When the trusted UI is closed we should probably dispatch the close event to all the dialogs in the stack.

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 9 • 12 years ago

Updates address Comment #8 by :ferjm

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 10 • 12 years ago

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #8)
> Comment on attachment 677791 [details] [diff] [review]
> make it possible to have a stack of trusty UIs
> 
> Review of attachment 677791 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice patch! Only a few nits and comments.

Thanks, Fernando, and thanks for giving us such a speedy review!

> > +    // XXX origin is never used
> 
> It seems that origin is indeed not being used, so can we just remove it
> along with the comment, please?
> Note that you'd also need to remove it from outside calls (payment.js and
> identity.js).

Done

> @@ +107,5 @@
> > +    return this.currentStack[this.currentStack.length-1];
> > +  },
> > +
> > +  _pushNewDialog: function trui_PushNewDialog(name, frame, chromeEventId) {
> > +    // add some data- to the frame
> 
> nit: typo? ("data-")

Fixed

> @@ +146,4 @@
> >  
> > +    var dialog = this._dialogStacks[this._lastDisplayedApp].pop();
> > +    this.container.removeChild(dialog.frame);
> > +    // whooosh!

Ohhhhh, all right :)

> @@ +178,5 @@
> >          WindowManager.restoreCurrentApp();
> >          this.hide();
> >          break;
> >        case 'click':
> > +        // click what??
> 
> nit: remove this comment, please.

Done

> @@ +184,4 @@
> >            return;
> >          this.close();
> >          // Notify user closed the trustedUI
> > +        this._dispatchCloseEvent(this._getTopDialog().chromeEventId);
> 
> When the trusted UI is closed we should probably dispatch the close event to
> all the dialogs in the stack.

Done, with the event dispatcher relocated so that a close event will be emitted for 
each dialog's own chromeEventId.

I've updated the pull request in gaia: https://github.com/jedp/gaia/commit/4b65c27318e1aace6f1f6862af9417975c9e5944

Fernando, can you merge the pull request in gaia?  Or do we need to find someone to help us with that?  

Many thanks!  Cheers,
j

	Fernando Jiménez Moreno [:ferjm]
	Comment 11 • 12 years ago

Sure, I'll be glad to merge the PR :). Can you squash all the commits in one and add 'r=ferjm' to the commit message, please? I am afraid that this is a requirement for all Gaia PR since feature freeze.

	Fernando Jiménez Moreno [:ferjm]
	Comment 12 • 12 years ago

Comment on attachment 678519 [details] [diff] [review]
make it possible to have a stack of trusty UIs

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

::: apps/system/js/trusted_ui.js
@@ +182,5 @@
> +        // If the user closed a trusty UI dialog, they probably meant
> +        // to close every dialog.
> +        for (var i=0, toClose=this.currentStack.length; i<toClose; i++) {
> +          this.close();
> +        }

nit: spaces (i = 0; i < toClose ...)

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 13 • 12 years ago

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #12)
> Comment on attachment 678519 [details] [diff] [review]
> make it possible to have a stack of trusty UIs
> 
> Review of attachment 678519 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: apps/system/js/trusted_ui.js
> @@ +182,5 @@
> > +        // If the user closed a trusty UI dialog, they probably meant
> > +        // to close every dialog.
> > +        for (var i=0, toClose=this.currentStack.length; i<toClose; i++) {
> > +          this.close();
> > +        }
> 
> nit: spaces (i = 0; i < toClose ...)

Fixed

And in response to Comment #11, it's been re-rebased and squashed accordingly.

Thanks again for your comments, Fernando, and for merging the PR in gaia.

Cheers,
j

	Fernando Jiménez Moreno [:ferjm]
	Comment 15 • 12 years ago

https://github.com/mozilla-b2g/gaia/commit/a67126c5cf366cc2d635f7f5433a8363a3a93df4

	Zachary Carter [:zaach]
	Comment 16 • 12 years ago

It appears parts of the patch were lost when Jed fixed those nits. Parts that dealt with frame scopes being clobbered, as reported in 807676. Reopening and creating a new PR with the fix that was lost.

	Zachary Carter [:zaach]
	Comment 17 • 12 years ago

Pointer to Github pull-request

	Zachary Carter [:zaach]
	Comment 18 • 12 years ago

Pointer to Github pull-request

	Zachary Carter [:zaach]
	Comment 19 • 12 years ago

Comment on attachment 681738 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/6413#attch-to-bugzilla

err, duplicate

	Jed Parsons (use needinfo, please) [:jedp, :jparsons] Assignee
	Comment 20 • 12 years ago

:kumar reports in Bug 805648, comment 11, that this bug is completely fixed for him.  I therefore mark it RESOLVED!