Closed Bug 744640 Opened 12 years ago Closed 6 years ago

create and implement user flow for obtaining affirmative assent to AITC privacy policy

Categories

(Web Apps Graveyard :: AppsInTheCloud, defect)

Product:
Web Apps Graveyard
Component:
AppsInTheCloud
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-kilimanjaro:+)

Status:
RESOLVED WONTFIX
Project Flags:
blocking-kilimanjaro +

People

(Reporter: bwalker, Unassigned)

References

Details

(Keywords: uiwanted, Whiteboard: [blocking-aitc+])

Attachments

(1 file, 9 obsolete files)

Implement AITC Privacy Policy
18.56 KB, patch
Details | Diff | Splinter Review 
We need to create and implement user flow for obtaining affirmative assent to AITC privacy policy.

Presumably, the first time a user connects to AITC, they need to be shown the AITC privacy policy and check a box indicating that they accept the policy.
Whiteboard: [marketplace-beta?]
Whiteboard: [marketplace-beta?]
OS: Mac OS X → All
Hardware: x86 → All
Nominating for k9o - this refers to AITC client implementation for k9o
blocking-kilimanjaro: --- → ?
blocking-kilimanjaro: ? → +
Depends on: k9o-webrt
Whiteboard: [blocking-aitc]
Whiteboard: [blocking-aitc] → [blocking-aitc+]
Jen, we need help in defining how this process will work for the user. Please advise. Let's chat F2F if you have questions about the technical restrictions, but it would be good to get an idea how product wants this exposed to the user first.
I'm working on this, but this is not a product question it's a UX question. 

The product requirement is that a user's privacy must be protected when using AITC. The user story is that the user will be able to opt-in/opt-out of privacy policy for the AITC service so that he can sync apps across devices. 

A UX design is needed and legal is needed for the copy.
Keywords: uiwanted
(In reply to Jennifer Arguello :ticachica from comment #3)
> I'm working on this, but this is not a product question it's a UX question. 
> 
> The product requirement is that a user's privacy must be protected when
> using AITC. The user story is that the user will be able to opt-in/opt-out
> of privacy policy for the AITC service so that he can sync apps across
> devices.
> 
> A UX design is needed and legal is needed for the copy. 

Needs more information, as this does not tell me enough to what were doing here (too vague requirements). Likely will be answered in bug 744639, but it's too early for UX to step in, cause we don't even know what the privacy policy is (or I don't know and haven't seen it on bug 744639). Do we have any progress on understanding what our privacy policy is in bug 744639? Do we someone from the privacy team to look at it? Who would define that policy?
(In reply to Jason Smith [:jsmith] from comment #4) 
> Needs more information, as this does not tell me enough to what were doing
> here (too vague requirements). Likely will be answered in bug 744639, but
> it's too early for UX to step in, cause we don't even know what the privacy
> policy is (or I don't know and haven't seen it on bug 744639). Do we have
> any progress on understanding what our privacy policy is in bug 744639? Do
> we someone from the privacy team to look at it? Who would define that policy?

You don't need the policy in hand to start working on the UX. We do know there is a privacy policy that should be displayed and users should accept. Let's do this with some place-holder text for now while we wait for legal to draft the actual document. The hard problem to figure out here is how the flow works.
(In reply to Anant Narayanan [:anant] from comment #5)
> (In reply to Jason Smith [:jsmith] from comment #4) 
> > Needs more information, as this does not tell me enough to what were doing
> > here (too vague requirements). Likely will be answered in bug 744639, but
> > it's too early for UX to step in, cause we don't even know what the privacy
> > policy is (or I don't know and haven't seen it on bug 744639). Do we have
> > any progress on understanding what our privacy policy is in bug 744639? Do
> > we someone from the privacy team to look at it? Who would define that policy?
> 
> You don't need the policy in hand to start working on the UX. We do know
> there is a privacy policy that should be displayed and users should accept.
> Let's do this with some place-holder text for now while we wait for legal to
> draft the actual document. The hard problem to figure out here is how the
> flow works.

Completely disagree. Not a good idea at all. The privacy policy affects how the UX is structured. It could be text, but do we know it's just text? Is there more to it? What are the major parts of the privacy policy? What parts should or shouldn't be displayed? How do we guarantee a user actually reads it?

Let's not make the same mistake we did on the geolocation bug for the desktop web runtime, where something got implemented, but spurred way too much disagreement downstream in the area of privacy, which now has lead to a followup P1 non-blocker to do the UI again. We'll waste engineering effort if we go forward too early.
(In reply to Jason Smith [:jsmith] from comment #6)
> Completely disagree. Not a good idea at all. The privacy policy affects how
> the UX is structured. It could be text, but do we know it's just text? Is
> there more to it? What are the major parts of the privacy policy? What parts
> should or shouldn't be displayed? How do we guarantee a user actually reads
> it?

You are over-thinking this.

We already know it's a wall of text. Just like: https://browserid.org/privacy. Every word of the policy must be displayed, and there is a requirement that the user click a button that's close to "accept". There is no way to guarantee a user will actually read it. This bug stems from a legal requirement. Most users click through text like that.

> Let's not make the same mistake we did on the geolocation bug for the
> desktop web runtime, where something got implemented, but spurred way too
> much disagreement downstream in the area of privacy, which now has lead to a
> followup P1 non-blocker to do the UI again. We'll waste engineering effort
> if we go forward too early.

This is nothing like the Geolocation bug. The Geolocation API does not have a privacy policy that is required to be read by the user. You're confusing a privacy policy document with general privacy protection measures for users.
(In reply to Anant Narayanan [:anant] from comment #7)
> (In reply to Jason Smith [:jsmith] from comment #6)
> > Completely disagree. Not a good idea at all. The privacy policy affects how
> > the UX is structured. It could be text, but do we know it's just text? Is
> > there more to it? What are the major parts of the privacy policy? What parts
> > should or shouldn't be displayed? How do we guarantee a user actually reads
> > it?
> 
> You are over-thinking this.

I don't think so, as I think the argument be made here is kinda of premature without knowing what the actual policy is.

> 
> We already know it's a wall of text. Just like:
> https://browserid.org/privacy. Every word of the policy must be displayed,
> and there is a requirement that the user click a button that's close to
> "accept". There is no way to guarantee a user will actually read it. This
> bug stems from a legal requirement. Most users click through text like that.

That's not well-designed at all to be brutally honest. I've been using BID for a while and didn't even know that existed! Seriously, we can do better than this.

Why not actually do this the "right" way instead of just doing a checkmark style of implementation of "it exists! we're good?" We should be delivering quality here, not something that's a rushed implementation without forethought.

> 
> > Let's not make the same mistake we did on the geolocation bug for the
> > desktop web runtime, where something got implemented, but spurred way too
> > much disagreement downstream in the area of privacy, which now has lead to a
> > followup P1 non-blocker to do the UI again. We'll waste engineering effort
> > if we go forward too early.
> 
> This is nothing like the Geolocation bug. The Geolocation API does not have
> a privacy policy that is required to be read by the user. You're confusing a
> privacy policy document with general privacy protection measures for users.

Not what I'm talking about - I'm talking about making decisions prematurely without connecting with the privacy guys and as a result, now having to re-implement something. I'm talking about the general piece of privacy and making sure that we do this right the first time.
Hi Jason,

> > We already know it's a wall of text. Just like:
> > https://browserid.org/privacy. Every word of the policy must be displayed,
> > and there is a requirement that the user click a button that's close to
> > "accept". There is no way to guarantee a user will actually read it. This
> > bug stems from a legal requirement. Most users click through text like that.
> 
> That's not well-designed at all to be brutally honest. I've been using BID
> for a while and didn't even know that existed! Seriously, we can do better
> than this.

I think you might not understand the goal we have.

We're pretty sure that the user understands what's going on, but we have a legal requirement to provide a privacy policy. Unfortunately, that requirement doesn't really take UX into account.

So, the issue is not "we should explain to users what's going on", which would be a UX problem. The issue is "we are required by law to display a privacy policy that is going to be pretty unusable, even though our lawyers have done an awesome job simplifying it as much as they possibly can."

> Why not actually do this the "right" way instead of just doing a checkmark
> style of implementation of "it exists! we're good?" We should be delivering
> quality here, not something that's a rushed implementation without
> forethought.

Because this is a legal requirement, not a design problem.

> Not what I'm talking about - I'm talking about making decisions prematurely
> without connecting with the privacy guys

This is connected with the privacy team. It's gone through data safety review and legal.

I'm always in favor of designing novel approaches to privacy, but sometimes it's just a legal requirement. This is one of those times.
Okay, makes sense Ben. Thanks for the explanation.
(In reply to Jason Smith [:jsmith] from comment #10)
> Okay, makes sense Ben. Thanks for the explanation.

I must admit, though, I really wish we could redesign these things with UX first. We've had these kinds of discussions around the under-13 age issue, where the legal requirement and proper design have zero in common...

So yeah, I share your frustration, but am resigned to what we must do to be in compliance.
(In reply to Jason Smith [:jsmith] from comment #8)
> > We already know it's a wall of text. Just like:
> > https://browserid.org/privacy. Every word of the policy must be displayed,
> > and there is a requirement that the user click a button that's close to
> > "accept". There is no way to guarantee a user will actually read it. This
> > bug stems from a legal requirement. Most users click through text like that.
> 
> That's not well-designed at all to be brutally honest. I've been using BID
> for a while and didn't even know that existed! Seriously, we can do better
> than this.
> 
> Why not actually do this the "right" way instead of just doing a checkmark
> style of implementation of "it exists! we're good?" We should be delivering
> quality here, not something that's a rushed implementation without
> forethought.

Such is the state of legal affairs. I know, it's sad. I'm all for innovating around privacy policies but this is not the place to do it. You're going to push the AITC release by months in attempting to do it right.

You've no doubt already accepted dozens of privacy policies and terms of services, most likely without reading them, already. BrowserID is one, we have one for Sync too: https://services.mozilla.com/privacy-policy/ and https://services.mozilla.com/tos/. Do you remember clicking "I agree?". Didn't think so.

> Not what I'm talking about - I'm talking about making decisions prematurely
> without connecting with the privacy guys and as a result, now having to
> re-implement something. I'm talking about the general piece of privacy and
> making sure that we do this right the first time.

That is not what this bug is about. This bug is about fulfilling a specific legal requirement for Mozilla to display a privacy policy and terms of service to users who choose to use our AITC server to store their app data.

It's hardly a "check-mark style of implementation". We've done this dozens of times before. If you're passionate about changing the way users are presented with privacy policies, I encourage it, but it must be done for all Mozilla products, not just AITC. It is a much harder problem than you think it is, and it is not feasible to block the release of AITC v1 for the right solution.

(If you're looking for inspiration, I recall we did some experimentation around privacy policies with creative commons style "privacy icons": http://www.azarask.in/blog/post/privacy-icons/)
Assignee: nobody → ndesaulniers
The server will send a HTTP 403 Forbidden if the user has pending legal documents to read and agree to.  The 403 response will also feature URL's to the legal documents and contextual information as to what the links are (eg. document titles).
Status: NEW → ASSIGNED
Running into trouble with loading iframes due to recent changes.  There's an assertion that was added to Nightly Debug on 6/29 that causes causes a segfault when trying to load an iframe in a new window whether the content is privileged or not.  For now, I can proceed with development if I comment out:
dom/base/nsJSEnvironment.cpp b/dom/base/nsJSEnvironment.cpp L1426
&
caps/src/nsScriptSecurityManager.cpp L2313

Both assertions are within #ifdef DEBUG blocks, but this may be an issue.
Anant asked me to upload the current state of my patch since I'll be out the rest of the week.  The code is functionally complete, but needs a mochitest, the debug code removed, the actual docs from legal, maybe FFUI review?, and the changes to the tokenserver moved to production.
Assuming that this depends on having TOS & PP text to land.

As comment 7 says, it's safe to assume that these will both be walls of text. However you don't need to display them on the signup flow: either you have a tiny scrolling box that makes the policy impossible to read, or you have a massive wall of text which makes the signup page impossible to use. Instead, you can use the following, verbatim:

[ ]  I'm okay with you handling this information as you explain in your privacy policy.

Where "privacy policy" is a link to the privacy policy. This allows users to read it in a separate tab at their leisure, then come back to check the box. CC'ing Jishnu (the lawyer who owns privacy in our products) for confirmation, though he's out right now. You'll need something similar for the TOS, but I'll let Jishnu advise on that.
Comment on attachment 650406 [details] [diff] [review]
Implement AITC Privacy Policy (with questions for gps, see code)

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

There are a lot of changes lumped together here. I'd like to see the prerequisite changes to the AITC server and token server client split out into their own patches. We may also want to split up the backend and frontend bits.

I'm not too keen on the frontend code living in the same modules as the backend. But, it is related, I'll give you that.

ToS acceptance for a token server service sounds like something other services will need to do. If so, we should definitely implement things such that this wheel doesn't need reinvented for every new service that comes along.

::: caps/src/nsScriptSecurityManager.cpp
@@ +2334,4 @@
>      // result (from a security perspective).
>  #ifdef DEBUG
>      nsIPrincipal *old = old_doGetObjectPrincipal(aObj);
> +    //MOZ_ASSERT(NS_SUCCEEDED(CheckSameOriginPrincipal(principal, old)));// Don't let me commit this!!!

r- :)

::: dom/base/nsJSEnvironment.cpp
@@ +1446,4 @@
>      nsJSPrincipals::get(JS_GetCompartmentPrincipals(js::GetObjectCompartment(aScopeObject)));
>    equal = false;
>    principal->Equals(scopeObjectPrincipal, &equal);
> +  //MOZ_ASSERT(equal);// Don't let me commit this!!!

Ditto.

::: services/aitc/modules/manager.js
@@ +68,5 @@
> +    try {
> +      cb(true);
> +      return;
> +    } catch (e) {
> +      self._log.error(new Error("AitC manager callback threw " + e));

CommonUtils.exceptionStr(e)

@@ +582,4 @@
>    /* Obtain a token from Sagrada token server, given a BrowserID assertion
>     * cb(err, token) will be invoked on success or failure.
>     */
> +  _getToken: function _getToken(assertion, cb, conditionFlag) {

Use default argument value for conditionFlag?

@@ +586,5 @@
> +    // Don't set a new pending assertion if the previous hasn't been cleared.
> +    if (!this._pendingAssertion) {
> +      this._log.info("Setting Pending Assertion");
> +      this._pendingAssertion = assertion;
> +    }

Please add a comment explaining why the pendingAssertion logic is the way it is.

Also - and I may be reading this incorrectly - it appears that you silently drop the passed assertion if one has been set already but you continue to create a client and obtain a token anyway. i.e. you issue a redundant/identical token request. Am I reading this right? Is this the expected behavior? Is silently dropping a (potentially newer) assertion passed into _getToken() the proper behavior or should a more severe error be raised?

@@ +613,5 @@
>        this._tokenDuration = parseInt(tok.duration, 10);
> +      if (this._pendingAssertion) {
> +        this._pendingAssertion = null;
> +        this._log.info("Clearing pending assertion");
> +      }

_pendingAssertion is only cleared out in case of success. That seems odd to me.

@@ +622,5 @@
> +    if (err && err.response && err.response.status &&
> +        err.response.status === 403) {
> +      this._log.info("Creating Privacy Policy window");
> +      if (this._dashboardWindow) {
> +        this._privacyPolicy(JSON.parse(err.response.body)["urls"]);

This seems like something that should be in the TokenServerClient. Maybe not the privacy policy window display code. But, the JSON parsing should definitely be in TokenServerClient. We may also want to add something into the arguments passed into the callback that say whether ToS need to be accepted, etc.

@@ +633,5 @@
> +      if (!this._dashboardVisited) {
> +        let msg = "403 received but dashboard has yet to be visited.";
> +        this._log.error(msg);
> +        cb(new Error(msg), null);
> +        return;

Find a way to rewrite this without the duplication.

@@ +643,5 @@
>      cb(msg, null);
>    },
>  
> +  // Create a window containing the privacy policy documents.
> +  _privacyPolicy: function _privacyPolicy(legalDocs) {

I'm not sure how I feel about all this window/dom code being in this file. This seems like something that is more the domain of browser/ or at least a separate standalone module.

@@ +692,5 @@
> +      win.document.getElementById("agree").addEventListener("click", agree);
> +      win.document.getElementById("decline").addEventListener("click",
> +        function () { win.close(); }); // win.close was not working? INVESTIGATE
> +    }
> +    win.addEventListener("DOMContentLoaded", onDOMContentLoaded);

My DOM skills aren't up to snuff to review this.

@@ +772,4 @@
>        }
>      }
>  
> +    // Check if we have a pending assertion first

Nit: period

@@ +777,5 @@
> +      this._log.info("Reusing pending assertion");
> +      processAssertion(this._pendingAssertion);
> +      return;
> +    }
> +    // Check if we can get assertion silently second

Ditto.

@@ +785,5 @@
> +        audience: DASHBOARD_URL,
> +        sameEmailAs: MARKETPLACE_URL
> +      });
> +    } catch (e) {
> +      this._log.debug("Error in _makeClient: " + e);

CommonUtils.exceptionStr.

Perhaps info or warn would be a more appropriate level?

::: services/aitc/tests/mochitest/browser_manager.js
@@ +10,5 @@
> +Cu.import("resource://services-aitc/manager.js", tmp);
> +Cu.import("resource://services-common/preferences.js", tmp2);
> +Cu.import("resource://services-aitc/client.js", tmp3);
> +Cu.import("resource://testing-common/services-common/aitcserver.js", tmp4);
> +Cu.import("resource://services-common/utils.js", tmp5);

You can create one variable and reuse it for all the imports, I believe. You should also name this variable something more descriptive than "tmp."

@@ +39,5 @@
> +function get_client_for_server(username, server) {
> +  let token = {
> +    id: "ID-HERE",
> +    key: "KEY-HERE",
> +    // should this be api_endpoint? also, L70 in client.js?

?

@@ +83,5 @@
> +  }
> +
> +  function ClientObserver () {
> +    let observerService = Cc["@mozilla.org/observer-service;1"].getService(
> +      Ci.nsIObserverService);

Nit: Reformat so the ".getService" aligns with the '[' from 'Cc['. You will find this convention throughout the source tree.

@@ +84,5 @@
> +
> +  function ClientObserver () {
> +    let observerService = Cc["@mozilla.org/observer-service;1"].getService(
> +      Ci.nsIObserverService);
> +    observerService.addObserver(this, "myTopicID", false);// give this a better topic

Please do that.

@@ +90,5 @@
> +  ClientObserver.prototype = {
> +    observe: function (subject, topic, data) {
> +      this.unregister();
> +      isnot(manager._client.token, oldToken, "New token acquired.");
> +      server.stop(finish);

I'm not familiar with the abort behavior of mochitests. If isnot() fails, what happens to the server created in this test file? Does that object get forcefully destroyed, resulting in the server stopping? Or, is it "leaked" and does it linger for the rest of the mochitest suite execution, causing future tests relying on this port to fail?

@@ +94,5 @@
> +      server.stop(finish);
> +    },
> +    unregister: function () {
> +      var observerService = Cc["@mozilla.org/observer-service;1"].getService(
> +        Ci.nsIObserverService);

style nit

@@ +99,5 @@
> +      observerService.removeObserver(this, "myTopicID");
> +    }
> +  };
> +
> +  function on_request () {

onRequest

@@ +106,5 @@
> +      code: 200,
> +      method: "OK",
> +      body: JSON.stringify({
> +        "id":"eyJleHBpcmVzIjogMTM0Mzg2MDE3OC43NjQ3NzkxLCAic2FsdCI6ICI0ZjQ0YTAiLCAidWlkIjogMCwgInNlcnZpY2VfZW50cnkiOiAiaHR0cHM6Ly9leGFtcGxlLmNvbSJ9faQI3d_DR7DQ4GNsX6JXqAmE9qA=",
> +        "key":"Kl6G4wg09lRfF2rurGgljPHBxjE=",

Please comment what these strings are or come from. These are also tempting to copy. Please comment whether it is OK for others to reuse.

Also, as part of digging into this myself, Python base64.b64decode() complained that id's value didn't have the proper padding. Intriguing.

@@ +112,5 @@
> +        "uid":0,
> +        "duration":300
> +      }),
> +      headers: {
> +        "content-type": "application/json"

Add charset?

@@ +118,5 @@
> +    };
> +  }
> +
> +  Services.ww.registerNotification(new MyWindowObserver());
> +  Preferences.set("services.aitc.tokenServer.url", "http://localhost:8080");

That's twice 8080 has been used. Please constify. Consider also lifting the hostname string into a constant.

@@ +136,5 @@
> +
> +  observer = new ClientObserver();
> +  client = get_client_for_server(USERNAME, server);
> +  manager = new AitcManager(function () {
> +    CommonUtils.nextTick(function () {

Why do you need nextTick?

@@ +140,5 @@
> +    CommonUtils.nextTick(function () {
> +      manager.userActive(window);
> +    });
> +  }, client);
> +}
\ No newline at end of file

General nit: no space between function name and ()

::: services/common/modules-testing/aitcserver.js
@@ +12,4 @@
>  ];
>  
>  Cu.import("resource://testing-common/httpd.js");
> +//Cu.import("resource://services-common/httpd.js");//debug code

Kill.

@@ +221,4 @@
>    _respondWithMockStatus: function _respondWithMockStatus(request, response) {
>      response.setStatusLine(request.httpVersion, this.mockStatus.code,
>        this.mockStatus.method);
> +    for (let key in this.mockStatus.headers) {

for (let [name, value] of this.mockStatus.headers) {

(This was added to the JS engine in the last week or two \o/

@@ +232,3 @@
>      this._onRequest();
>    },
>  

I'd prefer that the AITC server changes be split into its own review and possibly its own bug.

::: services/common/tests/unit/test_aitc_server.js
@@ +181,1 @@
>      

Nuke whitespace while you are here, please.

::: services/common/tokenserverclient.js
@@ +81,5 @@
>   * warning that you may be doing it wrong and that you should store full URIs
>   * instead.
>   *
> + * The XConditionsAcceptedFlag signals that the X-Conditions-Accepted will
> + * later need to be set.

What? I don't see this documented in the spec at http://docs.services.mozilla.com/token/apis.html. I'm hesitant to add this until I see documentation for it.

@@ +253,3 @@
>      for each (let k in ["id", "key", "api_endpoint", "uid"]) {
>        if (!(k in result)) {
>          let error = new TokenServerClientServerError("Expected key not " +

This patch should be a separate review request (if not bug) and needs a test.
Attachment #650406 - Flags: review?(gps)
Hi guys,

Just a note - we're still working on final text for PP. 

I vote for the following (pending Tom's input):

[ ] I agree to your Terms of Use and that you will handle my information as you explain in your Privacy Policy.

"Terms of Use" and "Privacy Policy" should be links that a user can click and then return to the sign up workflow.

Thanks
Jishnu
(In reply to jmenon from comment #21)
I'd rather have:
[ ] I agree to follow the rules, and I'm okay with you handling my information as you explain in your Privacy Policy.
(In reply to Gregory Szorc [:gps] from comment #20)
> Comment on attachment 650406 [details] [diff] [review]
> Implement AITC Privacy Policy (with questions for gps, see code)
> 
> Review of attachment 650406 [details] [diff] [review]:
> -----------------------------------------------------------------
> _pendingAssertion is only cleared out in case of success. That seems odd to
> me.
> 
This is because we want to save the _pendingAssertion in case of 403 error, so we can reuse it to get a new token.  The comments I've added should clear things up, so watch for that in the next revision of the patch.
(In reply to Gregory Szorc [:gps] from comment #20)
> Comment on attachment 650406 [details] [diff] [review]
> Implement AITC Privacy Policy (with questions for gps, see code)
> 
> Review of attachment 650406 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +90,5 @@
> > +  ClientObserver.prototype = {
> > +    observe: function (subject, topic, data) {
> > +      this.unregister();
> > +      isnot(manager._client.token, oldToken, "New token acquired.");
> > +      server.stop(finish);
> 
> I'm not familiar with the abort behavior of mochitests. If isnot() fails,
> what happens to the server created in this test file? Does that object get
> forcefully destroyed, resulting in the server stopping? Or, is it "leaked"
> and does it linger for the rest of the mochitest suite execution, causing
> future tests relying on this port to fail?
>
I have verified that a failed assertion in a mochitest does not prevent subsequent procedural lines of code from executing.
(In reply to Gregory Szorc [:gps] from comment #20)
> Comment on attachment 650406 [details] [diff] [review]
> Implement AITC Privacy Policy (with questions for gps, see code)
> 
> Review of attachment 650406 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +136,5 @@
> > +
> > +  observer = new ClientObserver();
> > +  client = get_client_for_server(USERNAME, server);
> > +  manager = new AitcManager(function () {
> > +    CommonUtils.nextTick(function () {
> 
> Why do you need nextTick?
>
We (Anant and I) have been using this pattern to wait until the manager has finished creating the AitcQueue.  While this may or may not be right, it what we do in all of our testing code.  There's probably a better way to guarantee the manager is good to go, but that should be a separate bug.
Attached patch Implement AITC Privacy Policy (obsolete) — — Splinter Review 
Almost all of :gps' reviews are implemented here, except the last two.  There's a new blocker I just filed for the api (bug 782790), and I'm not sure what "this patch" means in the last comment, and if it needs to block this patch?
Attachment #650406 - Attachment is obsolete: true
Attachment #651896 - Flags: review?(gps)
Comment on attachment 651896 [details] [diff] [review]
Implement AITC Privacy Policy

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

I reviewed everything except manager.js. I figure I owe you some feedback before I finish that...

::: services/aitc/AitcComponents.manifest
@@ +8,4 @@
>  
>  # Register resource aliases
>  resource services-aitc resource:///modules/services-aitc/
> +content aitc resource:///modules/services-aitc/
\ No newline at end of file

\ No newline at end of file

Content inside a modules directory feels wrong to me. We should at least expose a separate directory for content and code. MVC, blah blah.

We decided in bug 779370 that we're going to do away with the services-foo naming convention silliness.

::: services/aitc/modules/client.js
@@ +67,4 @@
>     *        (Object) A token as returned by CommonUtils.TokenServerClient.
>     */
>    updateToken: function updateToken(token) {
> +    this.uri = token.api_endpoint.replace(/\/+$/, "");

I'm guessing we don't have test coverage for this :(

::: services/aitc/modules/tos_window_builder.js
@@ +8,5 @@
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +function TOSWindowBuilder() {};
> +TOSWindowBuilder.build = function (legalDocs, cb) {

name function

@@ +44,5 @@
> +    win.document.getElementById("agree").addEventListener("click", agree);
> +    win.document.getElementById("decline").addEventListener("click",
> +      win.close.bind(win));
> +  }
> +  win.addEventListener("DOMContentLoaded", onDOMContentLoaded);

I only see the callback invoked if the user agrees to the terms.

I suspect that whatever calls this will be waiting on some event to occur before it resumes execution. We should also fire the callback on the following:

* ToS are explicitly declined
* The window is closed
* After a timeout? Although, this might be on the caller's side.

I just want to avoid a situation where a caller waits forever for a callback.

::: services/aitc/tests/Makefile.in
@@ +18,4 @@
>    mochitest/head.js \
>    mochitest/browser_id_simple.js \
>    mochitest/file_browser_id_mock.html \
> +  mochitest/browser_manager.js \

Let's find a better directory for this. I'm not sure what though.

Gavin?

::: services/aitc/tests/mochitest/browser_manager.js
@@ +12,5 @@
> +const AitcManager = loader.AitcManager;
> +const Preferences = loader.Preferences;
> +const AitcClient = loader.AitcClient;
> +const AITCServer10Server = loader.AITCServer10Server;
> +const CommonUtils = loader.CommonUtils;

I'm not an expert on mochitests. Must we do this for module importing? Surely there is a cleaner way. (I know you can't import directly because you leak.)

@@ +21,5 @@
> +const HOSTNAME = "http://localhost:" + TOKEN_SERVER_PORT;
> +
> +function importHelper() {
> +  
> +}

???

::: services/common/tokenserverclient.js
@@ +95,4 @@
>  function TokenServerClient() {
>    this._log = Log4Moz.repository.getLogger("Common.TokenServerClient");
>    this._log.level = Log4Moz.Level[Prefs.get("logger.level")];
> +  this.XConditionsAcceptedFlag = false;

No need for X in the property name. X is an implementation detail. Also, no need for "Flag." Truthy values are always flags ;)

@@ +167,5 @@
>      req.setHeader("authorization", "Browser-ID " + assertion);
> +    if (this.XConditionsAcceptedFlag) {
> +      // Reset Flag.
> +      this.XConditionsAcceptedFlag = false;
> +      req.setHeader("x-conditions-accepted", "1");

I believe we capitalize when setting headers so the case is preserved on the wire. Only on reading do we normalize to lower-case. FWIW, HTTP headers are case-insensitive. But, there is a convention of capitalizing what goes over the wire.

@@ +226,5 @@
>        let error = new TokenServerClientServerError("Non 200 response code: " +
>                                                     response.status);
>        error.response = response;
> +      if (response.status === 403) {
> +        error.urls = JSON.parse(response.body)["urls"];

This assumes the response body is valid JSON. We need to catch if it isn't.

@@ +252,4 @@
>        return;
>      }
>  
> +    // Should duration be added here?

It probably should be. Those token server people keep changing the API without telling me! Or, maybe they do and I just haven't filed a bug to track it...

@@ +268,4 @@
>      cb(null, {
>        id:       result.id,
>        key:      result.key,
> +      api_endpoint: result.api_endpoint,

Why did you change the API?

Yes, the callback fields aren't identical to what's in the HTTP response.
Depends on: 783437
Working on getting my patch working on top of Greg's from bug 783437, then adding changes from comment 27, then resubmitting for review.
got my patch to apply cleanly on top of Greg's and keep the tests green! w00t!  Working through Greg's suggestions now.
(In reply to Gregory Szorc [:gps] from comment #27)
> Comment on attachment 651896 [details] [diff] [review]
> Implement AITC Privacy Policy
> 
> Review of attachment 651896 [details] [diff] [review]:
> ::: services/aitc/modules/client.js
> @@ +67,4 @@
> >     *        (Object) A token as returned by CommonUtils.TokenServerClient.
> >     */
> >    updateToken: function updateToken(token) {
> > +    this.uri = token.api_endpoint.replace(/\/+$/, "");
> 
> I'm guessing we don't have test coverage for this :(

We do, I just also modified the failing tests when I modified the API.  But those have all been reverted now.

> ::: services/aitc/tests/Makefile.in
> @@ +18,4 @@
> >    mochitest/head.js \
> >    mochitest/browser_id_simple.js \
> >    mochitest/file_browser_id_mock.html \
> > +  mochitest/browser_manager.js \
> 
> Let's find a better directory for this. I'm not sure what though.
> 
> Gavin?
&
> ::: services/aitc/AitcComponents.manifest
> @@ +8,4 @@
> >  
> >  # Register resource aliases
> >  resource services-aitc resource:///modules/services-aitc/
> > +content aitc resource:///modules/services-aitc/
> \ No newline at end of file
> 
> \ No newline at end of file
> 
> Content inside a modules directory feels wrong to me. We should at least
> expose a separate directory for content and code. MVC, blah blah.
> 
> We decided in bug 779370 that we're going to do away with the services-foo
> naming convention silliness.

Ok, but I'm not sure where it should go, or even whether I understand the build system enough to move things and rename them.  And renaming should be handled in bug 779370.

> ::: services/aitc/tests/mochitest/browser_manager.js
> @@ +12,5 @@
> > +const AitcManager = loader.AitcManager;
> > +const Preferences = loader.Preferences;
> > +const AitcClient = loader.AitcClient;
> > +const AITCServer10Server = loader.AITCServer10Server;
> > +const CommonUtils = loader.CommonUtils;
> 
> I'm not an expert on mochitests. Must we do this for module importing?
> Surely there is a cleaner way. (I know you can't import directly because you
> leak.)

I asked :espadrine who's on devtools and who writes a lot of these, and he said that is the best way to do it.
Attached patch Implement AITC Privacy Policy (obsolete) — — Splinter Review 

        Attachment #651896 -
        Attachment is obsolete: true

        Attachment #651896 -
        Flags: review?(gps)

        Attachment #653539 -
        Flags: review?(gps)

    
    

    
  
Comment on attachment 653539 [details] [diff] [review]
Implement AITC Privacy Policy

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

This is /almost/ there. Please badger me incessantly once your submit the next patch.

::: services/aitc/modules/manager.js
@@ +581,3 @@
>    },
>  
>    /* Obtain a token from Sagrada token server, given a BrowserID assertion

Please add an empty line between the function summary line and the description paragraph.

@@ +582,5 @@
>  
>    /* Obtain a token from Sagrada token server, given a BrowserID assertion
> +   * cb(err, token) will be invoked on success or failure.  The BrowserID
> +   * assertion is saved (this._pendingAssertion), in case there is a 403
> +   * response from the token server.  The saved assertion is reused when

Nuke the 403 bit and replace with something stating that the token server requires conditions acceptance.

@@ +613,5 @@
> +    let self = this;
> +    function passBackErrorMsg(msg, cb) {
> +      self._log.error(msg);
> +      cb(new Error(msg), null);
> +    }

I'm normally for breaking out boilerplate code into functions. But, I'm not sure about this one...

@@ +625,5 @@
>        cb(null, tok);
>        return;
>      }
>  
> +    if (err && err.urls) {

Please key off of err.conditionsRequired

@@ +630,5 @@
> +      if (this._dashboardWindow) {
> +        this._log.info("Creating Privacy Policy window");
> +        TOSWindowBuilder.build(err.urls, function builtTOS(err) {
> +          if (err) {
> +            CommonUtils.exceptionStr(err);

You meant to log it, right?

@@ +637,5 @@
> +          self._log.info("User agreed to all.");
> +          // Retry getting a new token.
> +          self._makeClient(function makeClientAfterTOS(err, client) {
> +            if (err) {
> +              CommonUtils.exceptionStr(err);

self._log?

@@ +642,5 @@
> +            } else {
> +              self._client = client;
> +              // Notify tests.
> +              Cc["@mozilla.org/observer-service;1"].getService(
> +                Ci.nsIObserverService).notifyObservers(null, "TOSAgreement",

Given that observers are global throughout the application, "TOSAgreement" is too generic. How about "aitc:tos-agreement:accepted"?

@@ +752,5 @@
> +        audience: DASHBOARD_URL,
> +        sameEmailAs: MARKETPLACE_URL
> +      });
> +    } catch (e) {
> +      CommonUtils.exceptionStr(e);

log?

::: services/aitc/modules/tos_window_builder.js
@@ +7,5 @@
> +const EXPORTED_SYMBOLS = ["TOSWindowBuilder"];
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +function TOSWindowBuilder() {};

You don't actually need this function/type since you are storing all state in a closure.

@@ +27,5 @@
> +    iframe.setAttribute("src", uri);
> +    iframe.setAttribute("id", "tos_iframe");
> +    replaceAllChildren(win.document.getElementById("title"),
> +      win.document.createTextNode(title));
> +    replaceAllChildren(win.document.getElementById("iframe_container"),

I'm not a DOM expert. Is "iframe_container" a special name? I don't see it created explicitly.

@@ +40,5 @@
> +    }
> +  }
> +  function onDOMContentLoaded() {
> +    win.removeEventListener("DOMContentLoaded", onDOMContentLoaded);
> +    createIFrame(iter.next());

I guess this assumes legalDocs.length > 0. That's probably fair. Although, an explicit parameter validation might be warranted. Your call.

@@ +49,5 @@
> +  win.addEventListener("DOMContentLoaded", onDOMContentLoaded);
> +  win.addEventListener("unload", function unload() {
> +    cb(conditionsAccepted ? null : "User declined TOS.");
> +  });
> +};
\ No newline at end of file

Since I'm not a DOM expert, I'd appreciate a supplemental review on this file.

        Attachment #653539 -
        Flags: review?(gps)

    
    

    
  
Comment on attachment 653539 [details] [diff] [review]
Implement AITC Privacy Policy

Gavin: Can you look at the DOM code in tos_window_builder.js or delegate to someone who can, please?

        Attachment #653539 -
        Flags: feedback?(gavin.sharp)

    
    

    
  
Comment on attachment 653539 [details] [diff] [review]
Implement AITC Privacy Policy

getMostRecentWindow("navigator:browser") can return null (e.g. when you close all Windows on Mac).

Rather than opening a browser window and loading a tos.html URL in it, it seems like it would be much simpler to just Services.ww.openWindow() a XUL dialog that just contains an iframe, which loads tos.html and does the DOM stuff that TOSWindowBuilder.build does now.

    
    

    
  
(In reply to Gregory Szorc [:gps] from comment #32)
> Comment on attachment 653539 [details] [diff] [review]
> Implement AITC Privacy Policy
> 
> Review of attachment 653539 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: services/aitc/modules/tos_window_builder.js
> @@ +7,5 @@
> > +const EXPORTED_SYMBOLS = ["TOSWindowBuilder"];
> > +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> > +Cu.import("resource://gre/modules/Services.jsm");
> > +
> > +function TOSWindowBuilder() {};
> 
> You don't actually need this function/type since you are storing all state
> in a closure.
>

I just did this as a namespace kind of thing that way in manager.js I could call TOSWindowBuilder.build(legalDocs); ~manager.js:633

> @@ +27,5 @@
> > +    iframe.setAttribute("src", uri);
> > +    iframe.setAttribute("id", "tos_iframe");
> > +    replaceAllChildren(win.document.getElementById("title"),
> > +      win.document.createTextNode(title));
> > +    replaceAllChildren(win.document.getElementById("iframe_container"),
> 
> I'm not a DOM expert. Is "iframe_container" a special name? I don't see it
> created explicitly.

It's not created dynamically.  It's the id of a static element in tos.html (the html template I open).

    
    

    
  
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #34)
> Rather than opening a browser window and loading a tos.html URL in it, it
> seems like it would be much simpler to just Services.ww.openWindow() a XUL
> dialog that just contains an iframe, which loads tos.html and does the DOM
> stuff that TOSWindowBuilder.build does now.

So I didn't do this because I need to pass data into the JS that runs in the page, ie the list of urls and document titles that need to be iterated over, and a callback function.  The other problem is it's XUL.  I thought we were trying to replace sections of the code base written in XUL when possible, not add more?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Implement AITC Privacy Policy (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #653539 -
        Attachment is obsolete: true

        Attachment #653539 -
        Flags: feedback?(gavin.sharp)

        Attachment #654403 -
        Flags: review?(gps)

        Attachment #654403 -
        Flags: feedback?(gavin.sharp)

    
    

    
  
(In reply to Nick Desaulniers [:\n] from comment #36)
> So I didn't do this because I need to pass data into the JS that runs in the
> page, ie the list of urls and document titles that need to be iterated over,
> and a callback function.

You can pass that data to the HTML in the iframe the same way that you're doing it in that patch.

> The other problem is it's XUL.  I thought we were trying to replace sections of
> the code base written in XUL when possible, not add more?

It's not any more XULy than the existing patch - rather than using browser.xul in a kind of hacky way, you'd just use tosDialog.xul, which basically only contains an <html:iframe>.

    
    

    
  
Comment on attachment 654403 [details] [diff] [review]
Implement AITC Privacy Policy

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

I think it's good enough modulo some nits.

Please wait for gavin to sign off. Also, this requires AITC client changes from bug 783437 to land or it will fail tests. I'll ping for review on that bug.

I will likely land bug 783437 in inbound because services-central is being relegated to Sync craziness at the moment.

::: services/aitc/modules/manager.js
@@ +69,5 @@
> +    try {
> +      cb(true);
> +      return;
> +    } catch (e) {
> +      CommonUtils.exceptionStr(e);

log

@@ +583,5 @@
> +  /* Obtain a token from Sagrada token server, given a BrowserID assertion.
> +   *
> +   * cb(err, token) will be invoked on success or failure. The BrowserID
> +   * assertion is saved (this._pendingAssertion), in case there the token
> +   * server requires conditions o be accepted. The saved assertion is reused

o be?

        Attachment #654403 -
        Flags: review?(gps) → review+

    
    

    
  
Comment on attachment 654403 [details] [diff] [review]
Implement AITC Privacy Policy

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

::: services/aitc/modules/manager.js
@@ +644,5 @@
> +              self._client = client;
> +              // Notify tests.
> +              Cc["@mozilla.org/observer-service;1"].getService(
> +                Ci.nsIObserverService).notifyObservers(null,
> +                "aitc:tos-agreement:accepted", null);

Since you have time before checkin, please use Services.Obs.notifyObservers instead of this xpcom crap.

    
    

    
  
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #38)
> (In reply to Nick Desaulniers [:\n] from comment #36)
> > So I didn't do this because I need to pass data into the JS that runs in the
> > page, ie the list of urls and document titles that need to be iterated over,
> > and a callback function.
> 
> You can pass that data to the HTML in the iframe the same way that you're
> doing it in that patch.

Forgive me if I'm wrong, but I don't believe so.  If I say in the manager.js code:
let win = Services.ww.openWindow(...);
Then in the XUL doc, have the event listener for DOMContentLoaded try to create the iterator over the legalDocs object (the object I need to pass from the manager.js), wouldn't it then be a race condition for the manager.js code to pass in the legalDocs obj?  Also, it wouldn't work the same because the callback functions in tos_window_builder.js close over the variables passed in to the TOSWindowBuilder.build function.  How am I supposed to pass them in if the functions close over a passed variable?  Attach them to the window obj then try and read them off?  That's awful.  The problem I ran into when I first tried to use XUL to implement this was that I couldn't figure out how to pass the legalDocs object to the new window.  I could move attaching the DOMContentLoaded event listener attachment out of the XUL script code, but then I'd have to pull out the creation of the iterator, the createIframe and replaceAllChildren functions out from the xul code to the manager.js code (or globally attach them to the window object in the xul code since the agree function needs it, yuck).  And I don't want to have any code moved from the DOMContentLoaded event callbacks to a load event callback, because in order to test, I have to wait until load since it guarantees DOMContentLoaded has already executed.

I guess the gist of what I'm talking about is I need a way to open a new window and get a reference to it, and then pass in an object (or two) before the window's DOMContentLoaded function fires, guaranteed.  Does such a function/technique exist?

    
    

    
  
What is err.urls? It looks like just a relatively small array of [title, uri] arrays, which you could easily pass to tosDialog.xul as JSON via window.arguments (you may need a gross nsISupportsString for openWindow, but that isn't too bad).

    
    

    
  
Gavin was right.  I was able to use a xul window to consolidate the html template and window builder logic into a single file.  Tests are green!  Probably some issues with how I include things, also having the styles in the xul doc.  If they are quick fixes I can get them in, otherwise they NEED to be spun off into separate bugs (today is my last day at Mozilla, :( ).  Cleaning up patch now.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Implement AITC Privacy Policy (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #654403 -
        Attachment is obsolete: true

        Attachment #654403 -
        Flags: feedback?(gavin.sharp)

        Attachment #655081 -
        Flags: review?(gps)

        Attachment #655081 -
        Flags: feedback?(gavin.sharp)

    
    

    
  
Comment on attachment 655081 [details] [diff] [review]
Implement AITC Privacy Policy

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

Nit: Properly format the patch summary line.

I kinda wish there was a looser coupling between backend and frontend. But, we can address that in a follow-up if it ever becomes a problem.

I only skimmed the intradiff and saw that you only really changed the UI bits. I defer to Gavin for the final review of these parts (I don't trust my skills here).

        Attachment #655081 -
        Flags: review?(gps)

        Attachment #655081 -
        Flags: review?(gavin.sharp)

        Attachment #655081 -
        Flags: review+

        Attachment #655081 -
        Flags: feedback?(gavin.sharp)

    
    

    
  
Comment on attachment 655081 [details] [diff] [review]
Implement AITC Privacy Policy

You should fix all those "no newline at end of file" warnings :)

>diff --git a/services/aitc/modules/tos.xul b/services/aitc/modules/tos.xul

License header? (https://www.mozilla.org/MPL/headers/ - all new files should have them)

>+<xul:window width="600" height="600" onload=""

The window.open call uses 640x400. You can probably just get rid of these attributes?

>+<head>

Mixing XHTML/XUL like this isn't exactly what I was envisioning, sorry if I wasn't clear. I guess if it works it works, but I'm not sure there's any benefit to using XUL as the container in this situation. Does using straight XHTML not work? 

>+    Apps In The Cloud | Privacy Policy

This should be localized, right? Relatively easily doable with a DTD (see e.g. aboutSupport.xhtml|dtd).

>+    body {
>+      background: white;

When you set a background color, you should basically always also set a compatible foreground color (instead of relying on the default value being compatible, which might not happen if the user customized their font color or are using high-contrast system themes).

>+      function createIFrame(pair) {
>+        let [title, uri] = pair;
>+        let iframe = window.document.createElement("iframe");
>+        iframe.setAttribute("src", uri);
>+        iframe.setAttribute("id", "tos_iframe");

This is being inserted into a chrome document, so you'll want to set mozframetype="content" to avoid loading the arbitrary TOS URLs with chrome privileges.

>+  <div class="center">
>+    <p>
>+      I have read and agreed to the <span id="title"></span>.
>+    </p>
>+    <input type="button" value="Decline" id="decline" />
>+    <input type="button" value="Agree" id="agree" />

Presumably these strings need to be localized too.

        Attachment #655081 -
        Flags: review+ → review?(gps)

    
    

    
  
Comment on attachment 655081 [details] [diff] [review]
Implement AITC Privacy Policy

(bugzilla dumb)

        Attachment #655081 -
        Flags: review?(gps)

    
    

    
  
Sweet, converted the xul back to html.  Got almost everything done.  Will figure out the localization thing after lunch.

    
    

    
  
Gavin,
Can localization be split out into it's own bug?  The services team is going to have to figure out i10n as well, since they're sending the client side code strings that get appended to the DOM.  Also, I don't think the services code has the XUL documents and corresponding DTD files like the Firefox portions do.  I mean, there are style issues that need to be addressed as well as where the files end up residing.  For instance the screen sizing will end up being an issue down the road for Firefox OS.  But we should be creating separate bugs on these so we can iterate faster.  I'm submitting the current state of my patch, which you can r+ if you agree.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Implement AITC Privacy Policy (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #655081 -
        Attachment is obsolete: true

        Attachment #655081 -
        Flags: review?(gavin.sharp)

        Attachment #655154 -
        Flags: review?(gavin.sharp)

    
    

    
  


  
Updated bug status line format

        Attachment #655154 -
        Attachment is obsolete: true

        Attachment #655154 -
        Flags: review?(gavin.sharp)

        Attachment #655159 -
        Flags: review?(gavin.sharp)

    
    

    
  
Comment on attachment 655159 [details] [diff] [review]
Implement AITC Privacy Policy

Sounds like we don't need this anymore, or at least not in the near future?

        Attachment #655159 -
        Flags: review?(gavin.sharp)

    
    

    
  
Not in the near future (servers have been powered down, mostly just as a cost-savings measure - we could be back up in minutes). Will need to be revisited for future marketplace milestones.

    
    

    
  
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #52)
> Comment on attachment 655159 [details] [diff] [review]
> Implement AITC Privacy Policy
> 
> Sounds like we don't need this anymore, or at least not in the near future?

Gavin,

AITC is still on our roadmap, we will still need this. It will probably appear first on Android, probably in 2013.

    
    

    
  
No problem - feel free to re-request review when someone is actively working on this.
Assignee: nick → nobody

    
    

    
  
No assignee, updating the status.
Status: ASSIGNED → NEW

    
    

    
  
No assignee, updating the status.

    
    

    
  
No assignee, updating the status.

    
    

    
  
No assignee, updating the status.

    
    

    
  
Dead component, closing bugs in it.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX

    
  
Product: Web Apps → Web Apps Graveyard

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: