Closed
Bug 794680
Opened 12 years ago
Closed 12 years ago
Create a persona dialog for use on B2G
Categories
(Core Graveyard :: Identity, enhancement)
Core Graveyard
Identity
Tracking
(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)
People
(Reporter: jedp, Assigned: jedp)
References
Details
(Whiteboard: [LOE:M] [qa-][feature-complete])
Attachments
(2 files, 11 obsolete files)
(deleted),
text/html
|
vingtetun
:
review+
|
Details |
(deleted),
patch
|
Details | Diff | Splinter Review |
Depends on:
- serving persona include.js #793909
- implementation of the Trusty UI #768943
Comment 1•12 years ago
|
||
Important to note - read https://bugzilla.mozilla.org/show_bug.cgi?id=768943#c116. For persona, the v1 blocker is to have a trusted UI dialog, but doesn't necessarily have to be a "global" abstract trusted UI component, as that does not block v1. The nav.mozPay implementation currently heavily hard codes against a trusted UI with mozPay, so we may need to implement a separate trusted UI component for persona. If that's not a good solution for v1 and we definitely need to refactor to global trusted UI component in bug 794999, then please let everyone know, nom the bug to block, and explain why.
Comment 2•12 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #1)
> Important to note - read
> https://bugzilla.mozilla.org/show_bug.cgi?id=768943#c116. For persona, the
> v1 blocker is to have a trusted UI dialog, but doesn't necessarily have to
> be a "global" abstract trusted UI component, as that does not block v1. The
> nav.mozPay implementation currently heavily hard codes against a trusted UI
> with mozPay, so we may need to implement a separate trusted UI component for
> persona. If that's not a good solution for v1 and we definitely need to
> refactor to global trusted UI component in bug 794999, then please let
> everyone know, nom the bug to block, and explain why.
Jonas - Does this align with your understanding? Or am I out in left field?
Assignee | ||
Comment 3•12 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #1)
> The nav.mozPay implementation currently heavily hard codes against
> a trusted UI with mozPay, so we may need to implement a separate
> trusted UI component for persona.
That's fine by me.
I've opened #795023 for a Trusted UI for nav.id
Assignee | ||
Updated•12 years ago
|
Yes, let's please not involve bug 794999. Instead bug 795023 seems like a more doable solution.
Honestly, i'm not even sure that we need to have bug 795023. We can probably just do the work in this bug. I think that would be less confusing.
Comment 6•12 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #5)
> Honestly, i'm not even sure that we need to have bug 795023. We can probably
> just do the work in this bug. I think that would be less confusing.
Works for me. https://github.com/mozilla-b2g/gaia/issues/5323 tracks anything needed on the Gaia side.
Updated•12 years ago
|
Assignee: nobody → ferjmoreno
Whiteboard: [LOE:M]
Comment 7•12 years ago
|
||
Assignee: ferjmoreno → benadida
Status: NEW → ASSIGNED
Comment 8•12 years ago
|
||
Comment 9•12 years ago
|
||
Comment on attachment 667613 [details] [diff] [review]
WIP - 2012-10-03 - gecko patches for connecting to b2g identity ui
Don't forget to add the following to b2g/installer/package-manifest.in so that the files are packaged for distribution:
@BINPATH@/components/identity.xpt
@BINPATH@/components/nsDOMIdentity.js
@BINPATH@/components/nsIDService.js
@BINPATH@/components/Identity.manifest
Like we did for desktop:
https://hg.mozilla.org/mozilla-central/diff/a335f5f3e103/browser/installer/package-manifest.in
https://hg.mozilla.org/mozilla-central/diff/88070ff09ccd/browser/installer/package-manifest.in
Updated•12 years ago
|
Assignee | ||
Comment 10•12 years ago
|
||
Attachment #667613 -
Attachment is obsolete: true
Attachment #670075 -
Flags: review?(benadida)
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Jed Parsons [:jparsons] from comment #10)
> Created attachment 670075 [details] [diff] [review]
> gecko patch for connecting to b2g identity ui
I am pushing to try now ...
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #667617 -
Attachment is obsolete: true
Attachment #670075 -
Attachment is obsolete: true
Attachment #670075 -
Flags: review?(benadida)
Attachment #670250 -
Flags: review?(benadida)
Assignee | ||
Comment 13•12 years ago
|
||
patch v2 adds logout - that was missing from the former patch
Comment 14•12 years ago
|
||
Comment on attachment 670250 [details] [diff] [review]
connect gecko to b2g identity ui, v2
looking good, thanks Jed.
Attachment #670250 -
Flags: review?(benadida) → review+
Comment 15•12 years ago
|
||
note that this patch also addresses bug #798442
Comment 16•12 years ago
|
||
4 day old patch with a review+... Does this need a checkin-needed flag?
Comment 18•12 years ago
|
||
(In reply to Dietrich Ayala (:dietrich) from comment #16)
> 4 day old patch with a review+... Does this need a checkin-needed flag?
That's cause I'm slow and I should have just pushed it. Will do that today. Thanks!
Comment 19•12 years ago
|
||
(In reply to Ben Adida [:benadida] from comment #18)
> (In reply to Dietrich Ayala (:dietrich) from comment #16)
> > 4 day old patch with a review+... Does this need a checkin-needed flag?
>
> That's cause I'm slow and I should have just pushed it. Will do that today.
> Thanks!
Wait a second. Doesn't this need gaia approval? At-risk features need to get approval to land.
Comment 20•12 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #19)
> (In reply to Ben Adida [:benadida] from comment #18)
> > (In reply to Dietrich Ayala (:dietrich) from comment #16)
> > > 4 day old patch with a review+... Does this need a checkin-needed flag?
> >
> > That's cause I'm slow and I should have just pushed it. Will do that today.
> > Thanks!
>
> Wait a second. Doesn't this need gaia approval? At-risk features need to get
> approval to land.
When I mean to say that btw, I mean the pull request on the Gaia side. Not the code placed here.
Comment 21•12 years ago
|
||
Blocking+ does not need driver approval. Only non-blocking work needs approval at this point.
Comment 22•12 years ago
|
||
(In reply to Dietrich Ayala (:dietrich) from comment #21)
> Blocking+ does not need driver approval. Only non-blocking work needs
> approval at this point.
This is at-risk feature however. At-risk features do need driver approval.
Comment 23•12 years ago
|
||
(In reply to Jason Smith [:jsmith] from comment #22)
> (In reply to Dietrich Ayala (:dietrich) from comment #21)
> > Blocking+ does not need driver approval. Only non-blocking work needs
> > approval at this point.
>
> This is at-risk feature however. At-risk features do need driver approval.
Got the clarification from Dietrich - this one actually can land.
Comment 24•12 years ago
|
||
Comment 25•12 years ago
|
||
(In reply to Jed Parsons [:jparsons] from comment #11)
> (In reply to Jed Parsons [:jparsons] from comment #10)
> > Created attachment 670075 [details] [diff] [review]
> > gecko patch for connecting to b2g identity ui
>
> I am pushing to try now ...
jparsons, do you have your Try push handy? (This bug's push triggered some leaks on Mac OS, and I'm trying to narrow down which csets are definitely-innocent vs. maybe-guilty.)
Comment 26•12 years ago
|
||
Jason: since when is this at risk? We're on schedule with the implementation plan, as I've been reporting twice a week.
Assignee | ||
Comment 27•12 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #25)
> (In reply to Jed Parsons [:jparsons] from comment #11)
> > (In reply to Jed Parsons [:jparsons] from comment #10)
> > > Created attachment 670075 [details] [diff] [review]
> > > gecko patch for connecting to b2g identity ui
> >
> > I am pushing to try now ...
>
> jparsons, do you have your Try push handy? (This bug's push triggered some
> leaks on Mac OS, and I'm trying to narrow down which csets are
> definitely-innocent vs. maybe-guilty.)
Daniel, yes: https://tbpl.mozilla.org/?tree=Try&rev=9b6e173298dd
Please let me know if you see any leaks I need to fill.
Comment 28•12 years ago
|
||
This was indeed the culprit (at least, stuff went green when philor backed it out). Backout cset is:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a8f277d19df
This hit timeouts / test-failures in identity mochitests on Mochitest-2 & Mochitest-5, on all platforms. (Those are visible in the Try push, FWIW...)
The leaks aren't visible in the Try push (maybe they're from something that changed in the patch since then?), but they're pretty easy to hit now. This leaked on basically all debug unit tests on all platforms.
Here are some sample logs with leaks:
https://tbpl.mozilla.org/php/getParsedLog.php?id=16182715&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=16182228&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=16182177&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=16182138&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=16182295&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=16182560&tree=Mozilla-Inbound
...and some sample logs with the test failures/timeouts:
https://tbpl.mozilla.org/php/getParsedLog.php?id=16182493&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=16182365&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=16182194&tree=Mozilla-Inbound
Flags: in-testsuite+
Version: unspecified → Trunk
Comment 29•12 years ago
|
||
The leaks aren't visible on that try push because we can only detect leaks on debug builds, and that was an opt-only try push.
Comment 30•12 years ago
|
||
Yup, just noticed that -- the try run didn't have any debug builds, hence no leaks reported.
FWIW, here's the m-i push where this landed. Most of the orange there is from the leaks: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=31642ee5a1c9
(don't be alarmed by the fact that some test-runs are missing there -- those were just coalesced into a later cycle, I think)
Comment 31•12 years ago
|
||
Also: while you're fixing this, could you add a newline at the end of the added file "test_minimalidentity.js"? it's missing a final-newline, as shown by the last quoted line below (from the attached patch):
>+++ b/toolkit/identity/tests/unit/test_minimalidentity.js
>@@ -0,0 +1,95 @@
>+"use strict";
[...]
>+function run_test() {
>+ run_next_test();
>+}
>\ No newline at end of file
Assignee | ||
Comment 32•12 years ago
|
||
Daniel and Phil, thanks very much for the details. I'll get on this tomorrow and stop the leaks.
Comment 33•12 years ago
|
||
Comment on attachment 670250 [details] [diff] [review]
connect gecko to b2g identity ui, v2
Review of attachment 670250 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/identity/DOMIdentity.jsm
@@ +216,3 @@
> for (let message of this.messages) {
> + // func.call(aWindow.messageManager, message, this);
> + ppmm.addMessageListener(message, this);
This new line should be like the previous line so that it removes the listener when !aRegister. This could cause a leak.
::: toolkit/identity/MinimalIdentity.jsm
@@ +35,5 @@
> +}
> +
> +function IDService() {
> + Services.obs.addObserver(this, "quit-application-granted", false);
> + Services.obs.addObserver(this, "identity-auth-complete", false);
Where is the identity-auth-complete observer getting used and removed?
Comment 34•12 years ago
|
||
Test failures were in the following suites (run from the obj.-dir):
TEST_PATH=dom/identity/ make -C . mochitest-plain
TEST_PATH=toolkit/identity/tests/mochitest/ make -C . mochitest-plain
Comment 35•12 years ago
|
||
Pointer to Github pull-request
Comment 36•12 years ago
|
||
Comment on attachment 673209 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/5854
Zaach, could you squash all the commits in one, please?
Attachment #673209 -
Flags: review?(21)
Comment 37•12 years ago
|
||
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #36)
> Comment on attachment 673209 [details]
> Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/5854
>
> Zaach, could you squash all the commits in one, please?
Fernando: we kept it as two commits because the first one has been waiting for review for a few days, and we didn't want to mess up the review process. Do you really want this squashed?
Comment 38•12 years ago
|
||
(In reply to Ben Adida [:benadida] from comment #37)
> (In reply to Fernando Jiménez Moreno [:ferjm] from comment #36)
> > Comment on attachment 673209 [details]
> > Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/5854
> >
> > Zaach, could you squash all the commits in one, please?
>
> Fernando: we kept it as two commits because the first one has been waiting
> for review for a few days, and we didn't want to mess up the review process.
> Do you really want this squashed?
In that case, if I am not wrong, we have two options here:
1. Squash all the commits in one referring only to this bug and close Bug 795023.
or
2. Remove the last two commits from the pull request, cause they refer to Bug 795023 and send a new pull request with the commits squashed in one attaching it to Bug 795023.
I am afraid that since feature freeze there's need to be one PR per bugzilla bug :\, so it can be easily reverted in case of failure.
Dietrich can you confirm that, please?
Flags: needinfo?(dietrich)
Comment 40•12 years ago
|
||
Comment on attachment 670250 [details] [diff] [review]
connect gecko to b2g identity ui, v2
Review of attachment 670250 [details] [diff] [review]:
-----------------------------------------------------------------
r- for the files that will land in b2g/. You may also want to remove some commented codes in dom/.
::: b2g/components/SignInToWebsite.jsm
@@ +9,5 @@
> +"use strict";
> +
> +const EXPORTED_SYMBOLS = ["SignInToWebsiteController"];
> +
> +const Cc = Components.classes;
Cc is unused.
@@ +24,5 @@
> + "resource://gre/modules/identity/LogUtils.jsm");
> +
> +// JS shim that contains the callback functions that
> +// live within the identity UI provisioning frame.
> +// (NOT THE SAME THING AS IdP)
Can you extend on what is 'idP' ?
@@ +25,5 @@
> +
> +// JS shim that contains the callback functions that
> +// live within the identity UI provisioning frame.
> +// (NOT THE SAME THING AS IdP)
> +const kIdentityShimFile = "chrome://browser/content/identity.js";
Is this file missing from the PR?
@@ +27,5 @@
> +// live within the identity UI provisioning frame.
> +// (NOT THE SAME THING AS IdP)
> +const kIdentityShimFile = "chrome://browser/content/identity.js";
> +
> +// Type of MozChromEvents to handle payment dialogs.
nit: MozChrom -> MozChrome
nit: I believe you don't want to handle payment dialogs here :)
@@ +82,5 @@
> + return uuidgen.generateUUID().toString();
> + },
> +
> + doWatch: function SignInToWebsiteController_doWatch(aOptions) {
> + // for now, just say we're ready
Why? Can we remove this code for now, just until is is ready and it is clear why this stuff happens?
@@ +97,5 @@
> + },
> +
> + /*
> + *
> + */
No comment so I believe you can just remove those.
@@ +109,5 @@
> + _openDialogAndSendMessage: function SignInToWebsiteController_openDialogAndSendMessage(aRpId, aMessage, aOptions) {
> + let browser = Services.wm.getMostRecentWindow("navigator:browser");
> + let content = browser.getContentWindow();
> + if (!content) {
> + // aErrorCb.onresult("NO_CONTENT_WINDOW");
No dead code please.
@@ +127,5 @@
> + log("id after is ", id);
> + let msg = evt.detail;
> + if (msg.id != id) {
> + log("mozContentEvent. evt.detail.id != ", id, msg);
> + content.removeEventListener("mozContentEvent", getAssertion);
So if there is 1 mozContentEvent for something else you're not going to listen the assertion anymore?
@@ +157,5 @@
> + mm.sendAsyncMessage(aMessage, aOptions);
> +
> + log("done load frame script");
> +
> + content.removeEventListener("mozContentEvent", getAssertion);
I would move this code right after the msg.id != id check. So it is obvious that it is removed as soon as we have a matching id.
::: b2g/components/test/unit/head_identity.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const Cc = Components.classes;
Cc is unused.
@@ +3,5 @@
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +const Cr = Components.results;
Cr is unused.
@@ +48,5 @@
> +}
> +
> +// create a mock "doc" object, which the Identity Service
> +// uses as a pointer back into the doc object
> +function mock_doc(aIdentity, aOrigin, aDoFunc) {
Why is it not camelCase?
@@ +67,5 @@
> +
> +// mimicking callback funtionality for ease of testing
> +// this observer auto-removes itself after the observe function
> +// is called, so this is meant to observe only ONE event.
> +function makeObserver(aObserveTopic, aObserveFunc) {
Why is it camelCase while the other parts are not?
@@ +76,5 @@
> +
> + observe: function (aSubject, aTopic, aData) {
> + if (aTopic == aObserveTopic) {
> + aObserveFunc(aSubject, aTopic, aData);
> + Services.obs.removeObserver(observer, aObserveTopic);
Personally I will remove the observer before the aObserverFunc, you never know if this one will fail or not.
@@ +90,5 @@
> +function setup_test_identity(identity, cert, cb) {
> + // set up the store so that we're supposed to be logged in
> + let store = get_idstore();
> +
> + function keyGenerated(err, kpo) {
What is kpo ?
@@ +101,5 @@
> +
> +// takes a list of functions and returns a function that
> +// when called the first time, calls the first func,
> +// then the next time the second, etc.
> +function call_sequentially() {
This function seems unused. Please remove it.
::: b2g/components/test/unit/test_signintowebsite.js
@@ +1,1 @@
> +"use strict";
It miss the file header.
@@ +26,5 @@
> + do_test_finished();
> + run_next_test();
> + });
> +
> + mockedDoc.doCoffee();
I have no fucking clue what this stuff does. Is there a way to give it a real name?
::: dom/identity/DOMIdentity.jsm
@@ +16,3 @@
> XPCOMUtils.defineLazyModuleGetter(this, "IdentityService",
> "resource://gre/modules/identity/Identity.jsm");
> +*/
Do you really want to leave dead code?
@@ +137,2 @@
> .QueryInterface(Ci.nsIFrameLoaderOwner)
> + .frameLoader.messageManager;*/
Same comment as above.
::: dom/identity/nsDOMIdentity.js
@@ +491,4 @@
> .getInterface(Ci.nsIWebNavigation)
> .QueryInterface(Ci.nsIInterfaceRequestor)
> .getInterface(Ci.nsIContentFrameMessageManager);
> + */
Same comment as above.
::: toolkit/identity/MinimalIdentity.jsm
@@ +78,5 @@
> + }
> + return null;
> + },
> +
> + // RP stuff
That's not really a useful comment for someone like me that jump into this code :)
::: toolkit/identity/tests/Makefile.in
@@ +12,5 @@
>
> MODULE = test_identity
> XPCSHELL_TESTS = unit
>
> +DIRS = chrome mochitest
Remove the trailing whitespace. Only change in this file.
Attachment #670250 -
Flags: review-
Comment 41•12 years ago
|
||
Comment on attachment 673209 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/5854
r+ for the Gaia parts with the 2 nits addressed.
I'm a bit uncomfortable with the persona frame beeing in-process since it can slow the entire phone if the site is slow to load (same as the payment frame). I would like cjones to have a word on that.
Attachment #673209 -
Flags: review?(jones.chris.g)
Attachment #673209 -
Flags: review?(21)
Attachment #673209 -
Flags: review+
Comment 42•12 years ago
|
||
Why is this in-process to begin with?
Comment 43•12 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) from comment #40)
> r- for the files that will land in b2g/. You may also want to remove some
> commented codes in dom/.
This part of the code was already r+ (by me) and is awaiting some memory leak fixes (on Mac OS). I'll ask Jed to look at your comments, of course, for the next rev, but let's not revisit with a different set of criteria.
Comment 44•12 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) from comment #41)
> I'm a bit uncomfortable with the persona frame beeing in-process since it
> can slow the entire phone if the site is slow to load (same as the payment
> frame).
As you point out, we followed exactly the example from nav.pay. If there is a different path, then please let us know, but then nav.pay shouldn't have landed.
Quick reminder: we're working towards 10/26 feature freeze. There is time to fix bugs afterwards, but can we work to land the features ASAP, assuming no large problems?
Comment 45•12 years ago
|
||
(In reply to Ben Adida [:benadida] from comment #43)
> (In reply to Vivien Nicolas (:vingtetun) from comment #40)
> > r- for the files that will land in b2g/. You may also want to remove some
> > commented codes in dom/.
>
> This part of the code was already r+ (by me) and is awaiting some memory
> leak fixes (on Mac OS). I'll ask Jed to look at your comments, of course,
> for the next rev, but let's not revisit with a different set of criteria.
That's why I said r- for the b2g/ files. The others are out of my scope but are in yours.
Comment 46•12 years ago
|
||
(In reply to Ben Adida [:benadida] from comment #44)
> (In reply to Vivien Nicolas (:vingtetun) from comment #41)
> > I'm a bit uncomfortable with the persona frame beeing in-process since it
> > can slow the entire phone if the site is slow to load (same as the payment
> > frame).
>
> As you point out, we followed exactly the example from nav.pay. If there is
> a different path, then please let us know, but then nav.pay shouldn't have
> landed.
>
Maybe. But I want to raised the issue before spreading it all over the place.
> Quick reminder: we're working towards 10/26 feature freeze. There is time to
> fix bugs afterwards, but can we work to land the features ASAP, assuming no
> large problems?
We are on the same line here.
So in order to go fast please let's fix the comments I made for *both* patches - there are some serious questions I have like the one with the missing identity.js file. And I would like Chris to have a chance to know about the non-e10s state of the art for such features.
Comment 47•12 years ago
|
||
Code in the dom module should be reviewed by a dom peer. There is commented out code in there. That's not going to pass review. jst can help cleaning up the code and find a reviewer.
nav.pay is out of process now (bug 804080). We can follow the same approach here and file a follow-up bug. Worst case we can ship without it, but we likely do want it if we can get it done in time. As Vivien points out, in process is a big risk for memory leaks and responsiveness (we can't reclaim leaked memory in the chrome process).
Comment 48•12 years ago
|
||
(In reply to Andreas Gal :gal from comment #47)
> Code in the dom module should be reviewed by a dom peer. There is commented
> out code in there. That's not going to pass review. jst can help cleaning up
> the code and find a reviewer.
I don't believe this is the case because:
(a) this code is entirely for nav.id, so it also fits inside the identity module
(b) I've agreed with DOM peers that anything that requires significant architectural changes would be reviewed by them, too. This does not qualify, as it is merely a reversal to the use of cpmm/ppmm for messaging, which was how the DOM peers had recommended implementation the first time around.
I'm pushing back on this because I'm trying to meet the deadline (this Friday). If we add reviewers now, it's pretty clear we won't land this in time, given that there is more code in the pipeline that's stuck on these reviews.
> nav.pay is out of process now (bug 804080). We can follow the same approach
> here and file a follow-up bug.
Great, thanks, I've done that in bug 804143.
Comment 49•12 years ago
|
||
(In reply to Ben Adida [:benadida] from comment #48)
> (In reply to Andreas Gal :gal from comment #47)
> > Code in the dom module should be reviewed by a dom peer. There is commented
> > out code in there. That's not going to pass review. jst can help cleaning up
> > the code and find a reviewer.
>
> I don't believe this is the case because:
>
> (a) this code is entirely for nav.id, so it also fits inside the identity
> module
Please just remove the dead code.
It would take far less time to do what Vivien requested, which matches longstanding project-wide standards on not commenting out code, than it took to argue about it on shaky "it also fits" grounds.
If the file in question is really part of two modules (not possible by construction according to Mozilla's definition of "module"), then both modules' reviewers would need to be happy.
> (b) I've agreed with DOM peers that anything that requires significant
> architectural changes would be reviewed by them, too. This does not qualify,
> as it is merely a reversal to the use of cpmm/ppmm for messaging, which was
> how the DOM peers had recommended implementation the first time around.
Things change, but project-wide review standards against commented-out code are old as the hills.
> I'm pushing back on this because I'm trying to meet the deadline (this
> Friday).
There is no way a Friday deadline requires commenting out instead of deleting code. Hg will remember, no need to keep the old code in a comment.
/be
Comment 50•12 years ago
|
||
> Hg will remember, no need to keep the old code in a comment.
Git remembers too ;-).
/be
Assignee | ||
Comment 51•12 years ago
|
||
Thanks, :vingtetun for your feedback. I'm preparing another patch now that should address your questions and comments.
Assignee | ||
Comment 52•12 years ago
|
||
Attachment #670250 -
Attachment is obsolete: true
Attachment #673921 -
Flags: review?(benadida)
Assignee | ||
Comment 53•12 years ago
|
||
I have submitted v3 of this patch
(In reply to Vivien Nicolas (:vingtetun) from comment #40)
> Comment on attachment 670250 [details] [diff] [review]
> connect gecko to b2g identity ui, v2
>
> Review of attachment 670250 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> r- for the files that will land in b2g/. You may also want to remove some
> commented codes in dom/.
Yes. done.
> ::: b2g/components/SignInToWebsite.jsm
> @@ +9,5 @@
> > +"use strict";
> > +
> > +const EXPORTED_SYMBOLS = ["SignInToWebsiteController"];
> > +
> > +const Cc = Components.classes;
>
> Cc is unused.
removed
> @@ +24,5 @@
> > + "resource://gre/modules/identity/LogUtils.jsm");
> > +
> > +// JS shim that contains the callback functions that
> > +// live within the identity UI provisioning frame.
> > +// (NOT THE SAME THING AS IdP)
>
> Can you extend on what is 'idP' ?
I've removed the comment. It's not pertinent for the patch.
> @@ +25,5 @@
> > +
> > +// JS shim that contains the callback functions that
> > +// live within the identity UI provisioning frame.
> > +// (NOT THE SAME THING AS IdP)
> > +const kIdentityShimFile = "chrome://browser/content/identity.js";
>
> Is this file missing from the PR?
yes, it sure was!
> @@ +27,5 @@
> > +// live within the identity UI provisioning frame.
> > +// (NOT THE SAME THING AS IdP)
> > +const kIdentityShimFile = "chrome://browser/content/identity.js";
> > +
> > +// Type of MozChromEvents to handle payment dialogs.
>
> nit: MozChrom -> MozChrome
> nit: I believe you don't want to handle payment dialogs here :)
true :) fixed.
> @@ +82,5 @@
> > + return uuidgen.generateUUID().toString();
> > + },
> > +
> > + doWatch: function SignInToWebsiteController_doWatch(aOptions) {
> > + // for now, just say we're ready
>
> Why? Can we remove this code for now, just until is is ready and it is clear
> why this stuff happens?
It was our milestone last week to get the watch() function fully implemented. We've done that, and the comment has been removed
> @@ +97,5 @@
> > + },
> > +
> > + /*
> > + *
> > + */
>
> No comment so I believe you can just remove those.
>
> @@ +109,5 @@
> > + _openDialogAndSendMessage: function SignInToWebsiteController_openDialogAndSendMessage(aRpId, aMessage, aOptions) {
> > + let browser = Services.wm.getMostRecentWindow("navigator:browser");
> > + let content = browser.getContentWindow();
> > + if (!content) {
> > + // aErrorCb.onresult("NO_CONTENT_WINDOW");
>
> No dead code please.
sorry. removed.
> @@ +127,5 @@
> > + log("id after is ", id);
> > + let msg = evt.detail;
> > + if (msg.id != id) {
> > + log("mozContentEvent. evt.detail.id != ", id, msg);
> > + content.removeEventListener("mozContentEvent", getAssertion);
>
> So if there is 1 mozContentEvent for something else you're not going to
> listen the assertion anymore?
This has been fixed. The listener should only be removed once a message with the correct id has been received. So no, no listeners should have been removed if messages with a different id were received.
> @@ +157,5 @@
> > + mm.sendAsyncMessage(aMessage, aOptions);
> > +
> > + log("done load frame script");
> > +
> > + content.removeEventListener("mozContentEvent", getAssertion);
>
> I would move this code right after the msg.id != id check. So it is obvious
> that it is removed as soon as we have a matching id.
Agreed. It is there now.
> ::: b2g/components/test/unit/head_identity.js
> @@ +1,4 @@
> > +/* Any copyright is dedicated to the Public Domain.
> > + * http://creativecommons.org/publicdomain/zero/1.0/ */
> > +
> > +const Cc = Components.classes;
>
> Cc is unused.
removed
> @@ +3,5 @@
> > +
> > +const Cc = Components.classes;
> > +const Ci = Components.interfaces;
> > +const Cu = Components.utils;
> > +const Cr = Components.results;
>
> Cr is unused.
also removed
> @@ +48,5 @@
> > +}
> > +
> > +// create a mock "doc" object, which the Identity Service
> > +// uses as a pointer back into the doc object
> > +function mock_doc(aIdentity, aOrigin, aDoFunc) {
>
> Why is it not camelCase?
corrected
> @@ +67,5 @@
> > +
> > +// mimicking callback funtionality for ease of testing
> > +// this observer auto-removes itself after the observe function
> > +// is called, so this is meant to observe only ONE event.
> > +function makeObserver(aObserveTopic, aObserveFunc) {
>
> Why is it camelCase while the other parts are not?
fixed non_camel_case things
> @@ +76,5 @@
> > +
> > + observe: function (aSubject, aTopic, aData) {
> > + if (aTopic == aObserveTopic) {
> > + aObserveFunc(aSubject, aTopic, aData);
> > + Services.obs.removeObserver(observer, aObserveTopic);
>
> Personally I will remove the observer before the aObserverFunc, you never
> know if this one will fail or not.
Thanks for catching that. Done.
> @@ +90,5 @@
> > +function setup_test_identity(identity, cert, cb) {
> > + // set up the store so that we're supposed to be logged in
> > + let store = get_idstore();
> > +
> > + function keyGenerated(err, kpo) {
>
> What is kpo ?
The key generation functions are not needed in this test, so I've removed all this.
> @@ +101,5 @@
> > +
> > +// takes a list of functions and returns a function that
> > +// when called the first time, calls the first func,
> > +// then the next time the second, etc.
> > +function call_sequentially() {
>
> This function seems unused. Please remove it.
I've added more tests that use it now.
> ::: b2g/components/test/unit/test_signintowebsite.js
> @@ +1,1 @@
> > +"use strict";
>
> It miss the file header.
>
> @@ +26,5 @@
> > + do_test_finished();
> > + run_next_test();
> > + });
> > +
> > + mockedDoc.doCoffee();
>
> I have no **** clue what this stuff does. Is there a way to give it a
> real name?
I've added a comment explaining. I merely added a bogus function (doCoffee) to smoke-test the mocked docs and their message handlers. I guess I was thinking about coffee ...
> ::: dom/identity/DOMIdentity.jsm
> @@ +16,3 @@
> > XPCOMUtils.defineLazyModuleGetter(this, "IdentityService",
> > "resource://gre/modules/identity/Identity.jsm");
> > +*/
>
> Do you really want to leave dead code?
nope. removed.
> @@ +137,2 @@
> > .QueryInterface(Ci.nsIFrameLoaderOwner)
> > + .frameLoader.messageManager;*/
>
> Same comment as above.
removed
> ::: dom/identity/nsDOMIdentity.js
> @@ +491,4 @@
> > .getInterface(Ci.nsIWebNavigation)
> > .QueryInterface(Ci.nsIInterfaceRequestor)
> > .getInterface(Ci.nsIContentFrameMessageManager);
> > + */
>
> Same comment as above.
removed
> ::: toolkit/identity/MinimalIdentity.jsm
> @@ +78,5 @@
> > + }
> > + return null;
> > + },
> > +
> > + // RP stuff
>
> That's not really a useful comment for someone like me that jump into this
> code :)
Adding documentation
> ::: toolkit/identity/tests/Makefile.in
> @@ +12,5 @@
> >
> > MODULE = test_identity
> > XPCSHELL_TESTS = unit
> >
> > +DIRS = chrome mochitest
>
> Remove the trailing whitespace. Only change in this file.
removed
Assignee | ||
Comment 54•12 years ago
|
||
Attachment #673921 -
Attachment is obsolete: true
Attachment #673921 -
Flags: review?(benadida)
Attachment #673930 -
Flags: review?(benadida)
Assignee | ||
Comment 55•12 years ago
|
||
Looks like I still have a leak:
https://tbpl.mozilla.org/?tree=Try&rev=6fa69e4dce90
Looking for it now
Assignee | ||
Comment 56•12 years ago
|
||
Attachment #673930 -
Attachment is obsolete: true
Attachment #673930 -
Flags: review?(benadida)
Attachment #673991 -
Flags: review?(benadida)
Comment on attachment 673209 [details]
Pointer to Github pull request: https://github.com/mozilla-b2g/gaia/pull/5854
Anything that loads code from the network *CANNOT*, I repeat *CAN* *NOT*, run in the b2g process.
This is a P1 blocking security issue.
Attachment #673209 -
Flags: review?(jones.chris.g) → review-
Comment 58•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #57)
> Anything that loads code from the network *CANNOT*, I repeat *CAN* *NOT*,
> run in the b2g process.
Chris: makes sense, but can I ask that we allow this code to land so that testing can begin, and mark the followup issue, bug 804143 as the P1 blocking security issue?
Note that this is exactly what happened with payments, it was landed without OOP, and then OOP was added. I'm not arguing that we shouldn't do it, only that we should pipeline so that QA can start its work.
(In reply to Ben Adida [:benadida] from comment #58)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #57)
> > Anything that loads code from the network *CANNOT*, I repeat *CAN* *NOT*,
> > run in the b2g process.
>
> Chris: makes sense, but can I ask that we allow this code to land so that
> testing can begin, and mark the followup issue, bug 804143 as the P1
> blocking security issue?
>
If it's a matter of setting remote=true somewhere, I would really prefer we didn't allow the possibility of forgetting this.
> Note that this is exactly what happened with payments, it was landed without
> OOP, and then OOP was added. I'm not arguing that we shouldn't do it, only
> that we should pipeline so that QA can start its work.
I didn't know about that! If I had, I would have r-'d just as hard there ;).
Comment 60•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #59)
> If it's a matter of setting remote=true somewhere, I would really prefer we
> didn't allow the possibility of forgetting this.
Agreed, but first echos from my team is that it's a bit more complicated than that. I think we'll get to it next week. This Friday is the agreed-upon feature freeze for identity & payments, which is why I'm pushing for this.
There's no doubt we will do this, we understand the non-negotiable requirement, but payments is going to get stuck on ID integration, and QA is stuck too.
Anyways, that's my last pitch, please consider it, and let me know your final call :)
Comment 61•12 years ago
|
||
Comment on attachment 673991 [details] [diff] [review]
connect gecko to gaia identity ui, v4
r+ with two nits below:
>+++ b/b2g/chrome/content/identity.js
>+XPCOMUtils.defineLazyServiceGetter(this, "cpmm",
>+ "@mozilla.org/childprocessmessagemanager;1",
>+ "nsIMessageSender");
looks like this isn't used, let's remove it.
>+ BrowserID.internal.watch(function(aParams) {
Let's add a pointer to the Internal API documentation:
https://github.com/mozilla/browserid/wiki/Front-End-Development
Attachment #673991 -
Flags: review?(benadida) → review+
(In reply to Ben Adida [:benadida] from comment #60)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #59)
> > If it's a matter of setting remote=true somewhere, I would really prefer we
> > didn't allow the possibility of forgetting this.
>
> Agreed, but first echos from my team is that it's a bit more complicated
> than that. I think we'll get to it next week. This Friday is the agreed-upon
> feature freeze for identity & payments, which is why I'm pushing for this.
>
Can you help me understand what the complication is?
Comment 63•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #62)
> Can you help me understand what the complication is?
I'll check. Looks like payments tweaks was tiny, I must have been looking at something else. Thanks for pushing, will get back to you asap.
Comment 64•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #62)
> (In reply to Ben Adida [:benadida] from comment #60)
> > (In reply to Chris Jones [:cjones] [:warhammer] from comment #59)
> > > If it's a matter of setting remote=true somewhere, I would really prefer we
> > > didn't allow the possibility of forgetting this.
> >
> > Agreed, but first echos from my team is that it's a bit more complicated
> > than that. I think we'll get to it next week. This Friday is the agreed-upon
> > feature freeze for identity & payments, which is why I'm pushing for this.
> >
>
> Can you help me understand what the complication is?
Setting the frame to remote broke how we close the dialog, so we'll need to do some refactoring there. I'm taking a look at payments to see if they did anything additional to have it work with OOP.
That's surprising. Are you sure that wasn't a bug that's already been fixed?
Comment 66•12 years ago
|
||
Once again, Chris, the issue is not that we can't solve this, I'm quite certain we can, the issue is that our landing pipeline is getting incredibly delayed, and we're about to blow past the deadlines.
Can you make a call on whether we can land what we have, and fix this in a separate p1 blocking bug? It would help us a lot.
If not, don't worry, we'll figure out the technical constraint, but we will likely miss the Friday deadline. That would be really unfortunate.
Comment 67•12 years ago
|
||
Setting aside the question of deadlines and landing without this issue fixed, I'm happy to provide assistance if you have any specific questions.
Ben, I haven't gotten a clear answer yet on whether this won't work if landed with remote=true. I'm trying to get information to understand that, which is what I need to make the call.
Comment 69•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #65)
> That's surprising. Are you sure that wasn't a bug that's already been fixed?
It still occurs against the latest Gaia, at least.
Comment 70•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #68)
> Ben, I haven't gotten a clear answer yet on whether this won't work if
> landed with remote=true.
I'm confused, in https://bugzilla.mozilla.org/show_bug.cgi?id=794680#c64 Zach said:
> Setting the frame to remote broke how we close the dialog, so
> we'll need to do some refactoring there. I'm taking a look
> at payments to see if they did anything additional to have
> it work with OOP.
In other words, setting remote=true breaks our code (which was modeled on an earlier version of the payments stuff.)
I think we know more or less what the path is to fixing this, but it goes a good bit beyond setting remote=true. Justin, thanks for the offer to help, we'll definitely take you up on it soon if we can't figure this out!
Do you think we need to fully figure this out before you can make a decision on landing this patch that doesn't have the fix? That seems like the equivalent of simply waiting until we have OOP implemented, and thus not landing this patch.
So I would propose that you make a call based on the information we have so far, the fact that it will take us a few days to get OOP & our functionality working properly together, and the promise that we will get it done but, in the meantime, we want to start enabling QA and payments integration.
I understand if that's not enough, but if so let's determine that now and move quickly to finding the solution.
Updated•12 years ago
|
Flags: needinfo?(dietrich)
Comment 71•12 years ago
|
||
(In reply to Ben Adida [:benadida] from comment #70)
> Do you think we need to fully figure this out before you can make a decision
> on landing this patch that doesn't have the fix? That seems like the
> equivalent of simply waiting until we have OOP implemented, and thus not
> landing this patch.
>
> So I would propose that you make a call based on the information we have so
> far, the fact that it will take us a few days to get OOP & our functionality
> working properly together, and the promise that we will get it done but, in
> the meantime, we want to start enabling QA and payments integration.
>
I'm fine to land the code without remote=true if you file a bug for it and ask for blocking-basecamp? on it.
Thanks for the quick fixes.
Comment 72•12 years ago
|
||
(In reply to Vivien Nicolas (:vingtetun) from comment #71)
> I'm fine to land the code without remote=true if you file a bug for it and
> ask for blocking-basecamp? on it.
Did you one better: just marked bug 804143 blocking-basecamp+
Comment 73•12 years ago
|
||
Comment on attachment 673991 [details] [diff] [review]
connect gecko to gaia identity ui, v4
DOM changes look good, but two nits below that should be fixed while we're here:
- In DOMIdentity.jsm:
let DOMIdentity = {
// nsIMessageListener
receiveMessage: function DOMIdentity_receiveMessage(aMessage) {
+ log("**received message", aMessage);
let msg = aMessage.json;
Fix indentation.
// Target is the frame message manager that called us and is
// used to send replies back to the proper window.
- let targetMM = aMessage.target
+ /* let targetMM = aMessage.target
.QueryInterface(Ci.nsIFrameLoaderOwner)
- .frameLoader.messageManager;
+ .frameLoader.messageManager;*/
+ let targetMM = aMessage.target;
Remove the commented out code, as others noted as well.
r=jst with that!
Assignee | ||
Comment 74•12 years ago
|
||
Addresses jst's comments.
Fixes a bug in DOMIdentity.jsm that was causing test_watch in test_minimalidentity to fail
I still seem to have a ppmm memory leak that's showing up in mochitests.
Attachment #673991 -
Attachment is obsolete: true
Comment 75•12 years ago
|
||
cjones: one last request to get this patch landed while we work on OOP.
We're making progress, but we've got a heisenbug that sometimes causes a permission error, sometimes doesn't, and it's pretty clear we won't get to the bottom of it without some more involved work.
We'd love to still make the Friday feature deadline, and then follow up by fixing this bug. And of course we'll fix the bug as soon as possible.
So, there you have it, please r+ if you can, and we'll keep working on OOP.
I need a better diagnosis than "heisenbug". jlebar has outstanding questions in the OOP bug. This discussion may all be moot if it's something easy :).
I just think there's a huge disconnect here. You're asking for permission to land an sg:crit patch. The standard for that is extremely high. The Friday deadline isn't at risk yet.
Comment 77•12 years ago
|
||
cjones: we don't have a better diagnosis at this time. That's the issue. Sometimes it works, sometimes a permission error is thrown.
It looks like you want us to solve this before we decide whether to land without the fix, which makes no sense: if we had a complete diagnosis, we would also be minutes away from a patch. And then we wouldn't need this discussion.
So I'm going to conclude the inevitable here, which is that you don't want to land this without this issue taken care of. I'll mark the OOP issue a blocker for this.
Yes, this almost certainly means missing Friday's deadline.
There were open questions in bug 804143 for over a day. jlebar is actively helping. I don't believe you're much interested in devoting time to OOP, which is exactly what I'm afraid of.
The way these decisions work is, we need information to decide on a trade-off. I'm asking for information because what I have currently is insufficient.
Comment 79•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #78)
> There were open questions in bug 804143 for over a day. jlebar is actively
> helping. I don't believe you're much interested in devoting time to OOP,
> which is exactly what I'm afraid of.
That was my fault. I was having problems with my build most of yesterday so I was unable to try his suggestions.
Assignee | ||
Comment 80•12 years ago
|
||
(In reply to [:cjones] and [:zaach] from comment #78 and comment #79)
I also spent much of yesterday and yesterday night investigating this from the gecko side (b2g/components/SignInToWebsite.jsm), though without success. I should have messaged out more clearly that I was actively working on it. I'm grateful to you and jlebar for your help on this perplexing problem.
Thanks, sorry for the misunderstanding. We really honestly just want to help :).
Assignee | ||
Comment 82•12 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #28)
I've found the memory leak and patched it. Local mochitests look good. Now trying to package things up so they work for both b2g device and desktop and also for firefox desktop.
Assignee | ||
Comment 83•12 years ago
|
||
My focus in this patch was to fix the memory leaks noted in Comment #28.
I still have mochitests 2 and 5 timing out. Working on that next.
https://tbpl.mozilla.org/?tree=Try&rev=06653e30f528
Attachment #674740 -
Attachment is obsolete: true
Assignee | ||
Comment 84•12 years ago
|
||
(In reply to Jed Parsons [:jparsons] from comment #83)
submitted follow-up bug 805602 to restore mochitests
Assignee | ||
Comment 85•12 years ago
|
||
Attachment #675133 -
Attachment is obsolete: true
Attachment #675295 -
Flags: review?(benadida)
Comment 86•12 years ago
|
||
Comment on attachment 675295 [details] [diff] [review]
connect gecko to gaia identity ui, v8
This looks great. r+ assuming fix to one small nit:
>+++ b/b2g/chrome/content/identity.js
>+ if (!content) {
>+ return;
>+ }
Now that we use the magic variable "content", I don't see how this check is useful. Let's remove it.
Attachment #675295 -
Flags: review?(benadida) → review+
Comment 87•12 years ago
|
||
cjones: looks like this is already pref'ed off, and we will not pref it on until bug 804143 is addressed. If you can r+, that would be awesome.
Assignee | ||
Comment 88•12 years ago
|
||
Addresses issue in Comment #86
Attachment #675295 -
Attachment is obsolete: true
Updated•12 years ago
|
Attachment #673209 -
Flags: review-
By convention, all available prefs are defined in modules/libpref/src/init/all.js. So we need an entry like this there:
pref("dom.identity.enabled", false);
r=me with that.
Assignee | ||
Comment 90•12 years ago
|
||
Per Comment #89, ensure that toolkit.identity is off by default.
Attachment #675457 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 91•12 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 92•12 years ago
|
||
Comment 93•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Updated•12 years ago
|
Comment 94•12 years ago
|
||
status-firefox18:
--- → fixed
status-firefox19:
--- → fixed
Updated•12 years ago
|
Whiteboard: [LOE:M] [qa-] → [LOE:M] [qa-][feature-complete]
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•