Closed
Bug 807122
Opened 12 years ago
Closed 12 years ago
successful login on embedded Trusty UI closes all Trusty UIs
Categories
(Firefox OS Graveyard :: Gaia::System, enhancement)
Firefox OS Graveyard
Gaia::System
Tracking
(blocking-basecamp:+)
RESOLVED
FIXED
blocking-basecamp | + |
People
(Reporter: jedp, Assigned: jedp)
References
Details
(Whiteboard: [LOE:M])
Attachments
(2 files, 4 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/html
|
Details |
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.
Updated•12 years ago
|
blocking-basecamp: --- → ?
Comment 1•12 years ago
|
||
This is currently blocking basecamp since it blocks the payment window from using persona
Updated•12 years ago
|
blocking-basecamp: ? → +
Updated•12 years ago
|
Blocks: basecamp-id
Assignee | ||
Comment 2•12 years ago
|
||
Zach and I are working on this together.
Assignee: nobody → jparsons
Assignee | ||
Comment 3•12 years ago
|
||
Gaia pull request:
https://github.com/mozilla-b2g/gaia/pull/6121
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?
Attachment #677202 -
Flags: review?(21)
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...
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.
Attachment #677202 -
Flags: review?(21) → review?(ferjmoreno)
Assignee | ||
Comment 7•12 years ago
|
||
This is the github patch Zach referred to in Comment #5, with the fix for Bug 807676
Attachment #677202 -
Attachment is obsolete: true
Attachment #677202 -
Flags: review?(ferjmoreno)
Attachment #677791 -
Flags: review?(ferjmoreno)
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.
Attachment #677791 -
Flags: review?(ferjmoreno) → review+
Assignee | ||
Comment 9•12 years ago
|
||
Updates address Comment #8 by :ferjm
Attachment #677791 -
Attachment is obsolete: true
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
Assignee | ||
Updated•12 years ago
|
Attachment #677791 -
Attachment is obsolete: false
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.
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 ...)
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
Assignee | ||
Comment 14•12 years ago
|
||
Attachment #677791 -
Attachment is obsolete: true
Attachment #678519 -
Attachment is obsolete: true
Comment 15•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Component: Identity → Gaia
Product: Core → Boot2Gecko
Version: Trunk → unspecified
Updated•12 years ago
|
No longer blocks: basecamp-id
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 17•12 years ago
|
||
Pointer to Github pull-request
Comment 18•12 years ago
|
||
Pointer to Github pull-request
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
Attachment #681738 -
Attachment is obsolete: true
Updated•12 years ago
|
Component: Gaia → Gaia::System
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!
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•