Bugzilla

Paul O'Shannessy [:zpao] (not bugmail, email directly)

Comment 3

•

14 years ago

Moving to the reboot component, JpSecure.

Component: Jetpack → JpSecure

QA Contact: jetpack → jpsecure

Summary: private browsing status for jetpack plugins - JEP → Support private browsing

Assignee

Comment 4

•

14 years ago

Hijacking the bug for what is now JEP 116, which I'll be working on.

Assignee: nobody → paul

URL: https://wiki.mozilla.org/Labs/Jetpack...

Summary: Support private browsing → JEP 116 - Private Browsing

Paul O'Shannessy [:zpao] (not bugmail, email directly)

Assignee

Comment 5

•

14 years ago

Attached patch Patch v0.1 (WIP) (obsolete) — Details — Splinter Review

Just tossing up my WIP. It works, still needs a few more tests.

We talked about this on IRC yesterday, but the fact that I'm using __defineGetter/Setter__ on the exports object seems to be ok. AIUI, there's nothing in CommonJS saying we can't. It works. But if it becomes the case that we shouldn't do that, we won't be able to implement the API as it exists. We'd have to return a "service" object - require("private-browsing").service - lame.

I also just threw this in the jetpack-core package. It doesn't belong there (but it was easier to bootstrap this way). It should probably be in a browser package. As far as I know we haven't figured out the different app support yet... (PB is only in firefox, not in thunderbird, nor mobile, seamonkey might have ported it...)

Myk, can you give it a quick once over and let me know if there's anything I'm doing wrong?

Attachment #440654 - Flags: feedback?(myk)

Paul O'Shannessy [:zpao] (not bugmail, email directly)

Comment 6

•

14 years ago

Comment on attachment 440654 [details] [diff] [review]
Patch v0.1 (WIP)

>+// Does the private browsing service exist?
>+let pbService = false;

Nit: Why false?  Since pbService isn't a boolean, null or undefined would be better.

>+try {
>+  pbService = Cc["@mozilla.org/privatebrowsing;1"].
>+              getService(Ci.nsIPrivateBrowsingService);
>+}
>+catch (e) {
>+  throw "Error getting Private Browsing service";

When can this fail?  I thought the PB service component was part of the platform, but I could be wrong.

Nit: All of Atul's code throws new Errors instead of strings, so if only for consistency we should too.  Here and elsewhere.

>+// Make pb.active work
>+exports.__defineGetter__("active", function() {

Nit: A space between "function" and the parentheses, here and elsewhere.

>+// Add cancel
>+exports.cancel = function() {
>+  if (!canCancel)
>+    throw "Not in a cancellable state";

Nit: This message could be more helpful.  Also, the JEP doesn't mention an error, so it should be clarified.

I wonder, though... if PB can be canceled only in an onBefore callback whether the callback should be passed a one-time use object or better yet a function just for canceling.  Rather than a method on the module that can sometimes be called and sometimes not.

>+  cancelled = true;
>+}
>+
>+// Create function to serve as delegator for observer
>+function onBeforeEvent(aSubject, aData) {

Nit: Until someone forces us to do so, we're not using the "a" arg prefix.

>+  aSubject.QueryInterface(Ci.nsISupportsPRBool);
>+  // If aSubject is already true (by way of another observer), don't bother
>+  if (aSubject.data)
>+    return;

This comment could be more informative by explaining what it means if the subject is already true.

>+  //XXXzpao So about security...

What's that mean?

>+  if (aData == "enter")
>+    for (let callback in exports.onBeforeActivate)
>+      callback(exports);

I know the JEP specifies passing the module to the callback, but that seems kind of weird.  If I defined my callback as a method on the module:

  pbModule.onBeforeActivate = function () ...

Then I would expect |this| to be pbModule inside the function.  I guess that idea is complicated though if I do pbModule.onBeforeActivate.add(function () ...).  Meh, I think |this| == pbModule would still make sense there.  What do you think?

>+      //XXXzpao Should we still run all other onBeforeActivate if cancel() was called?

Hmm.  I don't have a strong opinion, but since these callbacks were all added by a single extension, I probably lean toward No... short circuit, yeah.  Either way the JEP should be clarified and the docs should mention it.

Oh actually the JEP says this for the inter-extension case:

> It should be noted that if another Jetpack or extension also
> implements these event handlers and cancels a transition, the
> event handler on the remaining Jetpacks will not be called.

Which you handle by the early return.

That seems bad to me:  If I register an observer some other extension shouldn't be able to interfere with that.  But if that's the case it should apply to the intra-extension case too.

>+  else
>+    for (let callback in exports.onBeforeDeactivate)
>+      callback(exports);

>+// We don't need to do anything special
>+function onEvent(aSubject, aData) {
>+  if (aData == "enter")
>+    for (let callback in exports.onActivate)
>+      callback();
>+  else
>+    for (let callback in exports.onDeactivate)
>+      callback();
>+}

Any time you're calling a user callback you should try-catch it and log the error with console.exception().  (The new jetpack-core/errors module has a catchAndLog() function that returns a function that try-catch-logs.  I asked Atul about adding another function that just try-catch-logs without wrapping it in a function, so feel free to add that if you want.)

Nit: Braces around both the if and else blocks, since they're each > one line.  Just my personal preference though.

>diff --git a/packages/jetpack-core/tests/test-private-browsing.js b/packages/jetpack-core/tests/test-private-browsing.js

>+exports.testSetActive = function(test) {
>+  reset();
>+  pb.active = true;
>+  test.assertEqual(pbService.privateBrowsingEnabled, true,
>+                   "private-browsing.active=true enables private browsing mode");
>+  pb.active = false;
>+  test.assertEqual(pbService.privateBrowsingEnabled, false,
>+                   "private-browsing.active=false disables private browsing mode");
>+}
>+
>+exports.testSimpleOnBeforeActivate = function(test) {
>+  reset();
>+  let count = 0;
>+  function simpleOnBeforeActivate(pbs) {

assertEqual(pbs, pb)

>+    count++;
>+  }
>+  pb.onBeforeActivate = simpleOnBeforeActivate;
>+  pb.active = true;
>+  test.assertEqual(count, 1, "All onBeforeActivate methods were called");

Hmm, are PB-related nsIObservers guaranteed to be notified immediately (synchronously) after turing on PB mode?

>+exports.testSimpleOnActivate = function(test) {
>+  reset();
>+  let count = 0;
>+  function simpleOnActivate() {
>+    count++;
>+  }
>+  pb.onActivate = simpleOnActivate;
>+  pb.active = true;
>+  test.assertEqual(count, 1, "simpleonActivate was called");
>+  pb.active = false;
>+
>+  // Now let's make it more complicated...
>+  function simpleOnActivate2() {
>+    count += 2;
>+  }
>+  pb.onActivate = [simpleOnActivate, simpleOnActivate2];
>+  pb.active = true;
>+  test.assertEqual(count, 4, "simpleOnActivate was called");

Nit: "all simpleOnActivate methods were called"

Attachment #440654 - Flags: feedback+

Assignee

Comment 7

•

14 years ago

(In reply to comment #6)
> (From update of attachment 440654 [details] [diff] [review])
> >+// Does the private browsing service exist?
> >+let pbService = false;
> 
> Nit: Why false?  Since pbService isn't a boolean, null or undefined would be
> better.

Left over from the way I started writing it. It really doesn't matter what value it has since we'll throw.

> >+try {
> >+  pbService = Cc["@mozilla.org/privatebrowsing;1"].
> >+              getService(Ci.nsIPrivateBrowsingService);
> >+}
> >+catch (e) {
> >+  throw "Error getting Private Browsing service";
> 
> When can this fail?  I thought the PB service component was part of the
> platform, but I could be wrong.

So the idl is in platform (specifically /netwerk), but the actual implementation is only in Firefox (it's in /browser). I know Fennec doesn't have it (yet?), I very much doubt Thunderbird has any intentions of implementing it, and AFAIK Seamonkey hasn't ported it, but I'll check.

I mentioned this to Dietrich and he actually suggested just doing it like you did with context menu and use xul-app to check for Firefox.

> Nit: All of Atul's code throws new Errors instead of strings, so if only for
> consistency we should too.  Here and elsewhere.

That's the sort of thing I'm looking for. Will fix.

> >+exports.__defineGetter__("active", function() {
> 
> Nit: A space between "function" and the parentheses, here and elsewhere.

Really? I haven't seen that in any Jetpack code besides yours, nor in any style guide. I'm not wild about that style, but I'm willing to make the change

> >+// Add cancel
> >+exports.cancel = function() {
> >+  if (!canCancel)
> >+    throw "Not in a cancellable state";
> 
> Nit: This message could be more helpful.  Also, the JEP doesn't mention an
> error, so it should be clarified.
> 
> I wonder, though... if PB can be canceled only in an onBefore callback whether
> the callback should be passed a one-time use object or better yet a function
> just for canceling.  Rather than a method on the module that can sometimes be
> called and sometimes not.

I'll follow up with Myk on these points. If we continue to throw, then yes, the message should be clarified. It might be something that we just want to do nothing though. Unless of course we do opt for a one time use object. I don't think that particular idea came up while Myk and I were discussing this, but we settled on something knowing it could change (since this is the first cancellable API)

> Nit: Until someone forces us to do so, we're not using the "a" arg prefix.

Fair.

> >+  aSubject.QueryInterface(Ci.nsISupportsPRBool);
> >+  // If aSubject is already true (by way of another observer), don't bother
> >+  if (aSubject.data)
> >+    return;
> 
> This comment could be more informative by explaining what it means if the
> subject is already true.

Fair. It just means that another observer (eg, another extension or something in Fx) has already canceled entering/exiting PB mode. The JEP specifies that onBefore* might not get called if it's already been canceled (which you noted below)

> >+  //XXXzpao So about security...
> 
> What's that mean?

Just that (with my limited knowledge of security efforts) this seems like a place where security needs to be considered since we're calling a method that might not really be "chrome privileged"

> 
> >+  if (aData == "enter")
> >+    for (let callback in exports.onBeforeActivate)
> >+      callback(exports);
> 
> I know the JEP specifies passing the module to the callback, but that seems
> kind of weird.  If I defined my callback as a method on the module:
> 
>   pbModule.onBeforeActivate = function () ...
> 
> Then I would expect |this| to be pbModule inside the function.  I guess that
> idea is complicated though if I do pbModule.onBeforeActivate.add(function ()
> ...).  Meh, I think |this| == pbModule would still make sense there.  What do
> you think?

Actually I think |this| == pbModule is already true... I'm not 100% sure if that makes sense though. I'll at least add a test case for the current condition so we know.

> >+      //XXXzpao Should we still run all other onBeforeActivate if cancel() was called?
> 
> Hmm.  I don't have a strong opinion, but since these callbacks were all added
> by a single extension, I probably lean toward No... short circuit, yeah. 
> Either way the JEP should be clarified and the docs should mention it.
> 
> Oh actually the JEP says this for the inter-extension case:
> 
> > It should be noted that if another Jetpack or extension also
> > implements these event handlers and cancels a transition, the
> > event handler on the remaining Jetpacks will not be called.
> 
> Which you handle by the early return.
> 
> That seems bad to me:  If I register an observer some other extension shouldn't
> be able to interfere with that.  But if that's the case it should apply to the
> intra-extension case too.

I'll followup with Ehsan & Myk about this whole subject. If you aren't comfortable with the conventions here, then something should probably change.

About the intra-extension canceling case, I erred on the side of the JEP which didn't specify, but I had doubts too, so that's why the XXX is there :)

> Any time you're calling a user callback you should try-catch it and log the
> error with console.exception().  (The new jetpack-core/errors module has a
> catchAndLog() function that returns a function that try-catch-logs.  I asked
> Atul about adding another function that just try-catch-logs without wrapping it
> in a function, so feel free to add that if you want.)

Ah ok. I didn't know that existed. If we don't already, we should have a "so you're implementing a JEP" guide with things like that.

> Hmm, are PB-related nsIObservers guaranteed to be notified immediately
> (synchronously) after turing on PB mode?

Yes they're synchronous (I can double check with Ehsan, but I looked at PB tests before writing mine)

Comment 8

•

14 years ago

(In reply to comment #7)
> > That seems bad to me:  If I register an observer some other extension shouldn't
> > be able to interfere with that.  But if that's the case it should apply to the
> > intra-extension case too.
> 
> I'll followup with Ehsan & Myk about this whole subject. If you aren't
> comfortable with the conventions here, then something should probably change.
> 
> About the intra-extension canceling case, I erred on the side of the JEP which
> didn't specify, but I had doubts too, so that's why the XXX is there :)

This is a common pattern for many similar observer notifications.  If someone cancels an even before you, you really want to respect that.  I think it's a good choice to not call the remaining cancel listeners if someone has canceled this notification before us, because that would prevent JEPs from "uncanceling" the notification.  That said, if I'm reading the code right, the cancel handler doesn't provide access to aSubject, so maybe the risk I'm talking about doesn't apply here.

> > Hmm, are PB-related nsIObservers guaranteed to be notified immediately
> > (synchronously) after turing on PB mode?
> 
> Yes they're synchronous (I can double check with Ehsan, but I looked at PB
> tests before writing mine)

Yes, they are synchronous.  The only thing to note here is that the nsISessionStore API is asynchronous, so by the time that "private-browsing" is issued, the session has not transitioned completely.  We have "private-browsing-transition-complete" for that.

Comment 9

•

14 years ago

Comment on attachment 440654 [details] [diff] [review]
Patch v0.1 (WIP)

>+// Make pb.active work
>+exports.__defineGetter__("active", function() {
>+  return pbService.privateBrowsingEnabled;

So what happens here if pbService == false/null?  I think we might return false here if PB is not available, for API consistency sake.

>+});
>+
>+exports.__defineSetter__("active", function(val) {
>+  pbService.privateBrowsingEnabled = val;

Similarly, we want this to be a no-op without PB around.

>+});
>+// Create function to serve as delegator for observer
>+function onBeforeEvent(aSubject, aData) {
>+  aSubject.QueryInterface(Ci.nsISupportsPRBool);
>+  // If aSubject is already true (by way of another observer), don't bother
>+  if (aSubject.data)
>+    return;
>+
>+  // Make sure these are set right
>+  canCancel = true;
>+  cancelled = false;
>+
>+  //XXXzpao So about security...
>+  if (aData == "enter")
>+    for (let callback in exports.onBeforeActivate)
>+      callback(exports);
>+      //XXXzpao Should we still run all other onBeforeActivate if cancel() was called?
>+  else

Please use:

    else if (aData == "exit")

here and in other places.

Attachment #440654 - Flags: feedback-

Comment 10

•

14 years ago

Comment on attachment 440654 [details] [diff] [review]
Patch v0.1 (WIP)

From reading through the comments on this bug and the patch, it looks like the extant issues raised so far are:

1. whether or not cancellation should suppress the calling of additional event callbacks;

2. what |this| should be inside event callbacks;

3. whether it would be better to provide a one-time-use object/function for cancelling activation;

4. style issues (space before anonymous function parentheses, brackets around multi-line conditional block).


To these I'll add:

5. the use of "activate/deactivate" terminology for this API;

6. whether we should provide an event callback property for the private-browsing-transition-complete notification;

7. what to do about extensions that asynchronously change state when starting/stopping private browsing.


Regarding these issues, here are my thoughts:

1. Cancellation should suppress the calling of additional event handlers registered by the same extension.  It shouldn't affect event handlers registered by other extensions, however, since we haven't yet worked out whether and how modules from different extensions should communicate and share state, and in the current model, there is no such sharing.

If/when we do (and this is not the only API that raises the issue; l10n may also want to share a local cache of translations), we may well decide that cancellation should suppress the calling of other extensions' callbacks.

At first glance, that seems like the better behavior, since the whole purpose of onBeforeActivate is to determine whether there is any reason to cancel activation of private browsing mode, and once a single extension (or, for that matter, core Firefox code) cancels activation, there is no point in checking to see if any other code would also want to cancel it.

However, I'm loathe to implement it this way now, since we haven't yet worked through the implications of cross-extension communication on our extension isolation model, the versioning of modules, and so on.

In any case, do implement intra-extension suppression.  And, in order to minimize bustage if we later implement inter-extension suppression, make sure to document that the purpose of the onBeforeActivate property is to enable extensions to cancel activation of private browsing mode, and they shouldn't change their state in that callback.


2. |this| should be the private browsing singleton inside event handler functions, as that is the most intuitive value for it to have given the way the functions are registered.

Atul expressed reservations about the use of |this| recently, and I'm going to follow up with him on those and figure out what our strategy should be for |this| in callbacks generally, so it's possible the approach will change, but go with |this| == the object on which callback was registered for now.


3. I share Drew's consternation about a persistent cancel method that can only be called from within onBefore(De)?Activate, and I agree that a one-time-use object/function passed as an argument to callbacks seems preferable, since it removes doubt about when cancellation can occur.

I would make it a function rather than an object and pass it via a "cancel" parameter.  Note, however, that the cancel function must continue to guard against being called outside of a callback, since callbacks that want to be naughty (or are incorrectly written) can still cache it and invoke it later.


4. I'm fairly agnostic about code style these days, and Drew has been driving efforts in this regard for the project, so we should look to him for guidance on these issues (although I'm a function() guy myself, natch!).

I'll just reinforce his suggestion about braces around multi-line conditional and loop blocks, since it's too easy to introduce bugs into them when they don't have braces.  FWIW, Douglas Crockford recommends braces around single-line blocks too, although in my experience it is only the multi-line ones that are prone to this kind of error.


5. I proposed the use of activate/deactivate back when we were using that terminology in several other APIs.  It comes from the DOM event standard.  However, further research (bug 548590, comment 19) recently revealed that the working group handling that standard is in the process of deprecating it, since it never caught on.

Therefore, we don't have as much reason to use it in other APIs, and that calls its utility in this one into question as well, especially given that activate/deactivate is significantly longer and more complex than alternatives like enter/leave and start/stop.

nsIPrivateBrowsingService uses a variety of terminology (active, enabled, enter, leave, started, exit, switch to).  Firefox's English localization, on the other hand, only uses the terminology start/stop, although the main support article about the feature <http://support.mozilla.com/en-US/kb/Private+Browsing> uses other terms synonymously (which makes sense given its audience).

I suggest we use start/stop (although retaining |active| as the boolean property that specifies whether or not the user is in private browsing mode and lets extensions start/stop private browsing), as I expect the casual developers Jetpack is targeting to be more familiar with the feature from Firefox's user interface than its underlying implementation.

In that case, the names of these event handlers would be onBeforeStart, onStart, onBeforeStop, and onStop.


6. It seems like it would be useful to provide a callback property for the private-browsing-transition-complete event (which, incidentally, nsIPrivateBrowsingService.idl doesn't document).  For some use cases, that property may be even more useful than onStart.  And given onBeforeStart/onBeforeStop and onStart/onStop properties, the natural name for this would be onAfterStart/onAfterStop.

My only concern with this is that elsewhere we use the present tense of event verbs for events that have just happened (f.e. onLoad, onClick) for consistency with the DOM, whereas here we use the present tense in a different way.  I don't have a better idea for how to name this event, however, and I suspect that the proposed pattern is reasonable enough, albeit inconsistent.


7. We don't have to solve this problem now, but if at some point we notice that extensions are storing state in a store they access asynchronously (f.e. a SQLite-backed in-memory database), then we'll want to address this.  Perhaps we can provide extensions with a hook for registering themselves as handling private browsing state changes asynchronously and make the service delay sending private-browsing-transition-complete until all such extensions have reported completion of their async transition.

Attachment #440654 - Flags: feedback?(myk) → feedback-

Paul O'Shannessy [:zpao] (not bugmail, email directly)

Comment 11

•

14 years ago

(In reply to comment #10)
> 2. |this| should be the private browsing singleton inside event handler
> functions, as that is the most intuitive value for it to have given the way the
> functions are registered.
> 
> Atul expressed reservations about the use of |this| recently, and I'm going to
> follow up with him on those and figure out what our strategy should be for
> |this| in callbacks generally, so it's possible the approach will change, but
> go with |this| == the object on which callback was registered for now.

FWIW the so-you're-implementing-a-JEP guide Dietrich and Paul and Felipe asked me to write addresses these questions (the Private Parts and Client Callbacks sections).  This is my current understanding, but let's update it if it's wrong.

https://wiki.mozilla.org/User:Adw/JEP-fu

Assignee

Comment 12

•

14 years ago

(In reply to comment #9)
> (From update of attachment 440654 [details] [diff] [review])
> >+// Make pb.active work
> >+exports.__defineGetter__("active", function() {
> >+  return pbService.privateBrowsingEnabled;
> 
> So what happens here if pbService == false/null?  I think we might return false
> here if PB is not available, for API consistency sake.

So I *think* we'll throw when requiring, in which case nothing is set on the exports object. I'll check with a XULRunner test run and see what happens (could be interesting since all the tests are expecting it to exist...). Perhaps throwing isn't the right call though.

Comment 13

•

14 years ago

(In reply to comment #12)
> So I *think* we'll throw when requiring, in which case nothing is set on the
> exports object.

You'll throw when the client requires, yes, and since you throw he won't have a handle to the exports object at all.  If it helps, context-menu does the same thing:

http://mxr.mozilla.org/labs-central/source/jetpack-sdk/packages/jetpack-core/lib/context-menu.js#40

Comment 14

•

14 years ago

Oh, and here's how the context-menu test handles that:

http://mxr.mozilla.org/labs-central/source/jetpack-sdk/packages/jetpack-core/tests/test-context-menu.js#1008

Paul O'Shannessy [:zpao] (not bugmail, email directly)

Comment 15

•

14 years ago

I would expect the APIs to not throw, and just pretend that we're not inside the private browsing mode if the service is unavailable.

Assignee

Comment 16

•

14 years ago

Attached patch Patch v0.2 (WIP) (obsolete) — Details — Splinter Review

I think I've covered everything from the feedback. I still need to finish writing tests but that won't take long - I wanted to throw something more up on the bug.

(In reply to comment #10)
> 6. It seems like it would be useful to provide a callback property for the
> private-browsing-transition-complete event (which, incidentally,
> nsIPrivateBrowsingService.idl doesn't document).  For some use cases, that
> property may be even more useful than onStart.  And given
> onBeforeStart/onBeforeStop and onStart/onStop properties, the natural name for
> this would be onAfterStart/onAfterStop.

So I have these in there now. My main concern is that of the on* APIs here, the onAfter* is the only one that is truly async. onBeforeStart will happen immediately after active = true, onStart will fire immediately after that, onAfterStart will fire some indeterminate time after that (in theory, that time will be much shorter going into PB mode that out)

> My only concern with this is that elsewhere we use the present tense of event
> verbs for events that have just happened (f.e. onLoad, onClick) for consistency
> with the DOM, whereas here we use the present tense in a different way.  I
> don't have a better idea for how to name this event, however, and I suspect
> that the proposed pattern is reasonable enough, albeit inconsistent.

I agree that it's a bit weird. But I think that most cases aren't going to use onAfterStart (and perhaps we could even downplay it's usefulness - I think it's really only important for consumers who may want to open/close a tabs).

Attachment #440654 - Attachment is obsolete: true

Comment 17

•

14 years ago

Comment on attachment 442194 [details] [diff] [review]
Patch v0.2 (WIP)

Looks great.

>diff --git a/packages/jetpack-core/lib/private-browsing.js b/packages/jetpack-core/lib/private-browsing.js

>+// Currently, only Firefox implements the private browsing service.
>+if (!require("xul-app").is("Firefox")) {
>+  throw new Error([
>+    "The private-browsing module currently supports only Firefox, as Firefox is ",
>+    "the only Mozilla application that currently implements it. As others",
>+    "(e.g. Fennec) implement it, we will add support for them."
>+  ].join(""));
>+}

Ehsan makes a good point about just pretending PB mode is inactive if the service is unavailable.

>+function onBeforeEvent (subject, data) {

>+  // If subject is already true (by way of another observer), don't bother
>+  if (subject.data)
>+    return;

Myk noted some concern in comment 10 point 1 about returning early here.  Maybe nothing firm was decided though.

>+      errors.catchAndLog(function () {
>+        callback.apply(exports, [function() cancelled = true]);
>+      })();

What do you think about modifying errors.js so catchAndLog does what its name says it'll do and no more, and adding a new method that does what catchAndLog does now (return a function)?  Atul and I talked about that, bug 556940 comment 10 if you're interested.

Nit: Use call() instead of apply() to save the array creation.

>+// Add observers
>+observers.add("private-browsing-cancel-vote", onBeforeEvent);
>+observers.add("private-browsing", onEvent);
>+observers.add("private-browsing-transition-complete", onAfterEvent);

Nit: It would be slightly easier to grok the roles of these callbacks if they were named something like onPbCancelVote, onPbTransitioning, onPbTransitioned.

Attachment #442194 - Flags: feedback+

Paul O'Shannessy [:zpao] (not bugmail, email directly)

Comment 18

•

14 years ago

Comment on attachment 442194 [details] [diff] [review]
Patch v0.2 (WIP)

As I had said before, and adw mentioned again, this should still handle the case where the PB service is not available (should be a trivial change).  Marking f- for now, but with that change, f=me from the private browsing standpoint.

Attachment #442194 - Flags: feedback-

Assignee

Comment 19

•

14 years ago

(In reply to comment #17)
> Ehsan makes a good point about just pretending PB mode is inactive if the
> service is unavailable.

Seems like this is the way to go.

> >+function onBeforeEvent (subject, data) {
> 
> >+  // If subject is already true (by way of another observer), don't bother
> >+  if (subject.data)
> >+    return;
> 
> Myk noted some concern in comment 10 point 1 about returning early here.  Maybe
> nothing firm was decided though.

So I'm pretty sure this lines up with what Myk wants. If the transition was canceled by something else, we won't run any of our callbacks. If one of our callbacks cancels it, the rest of the callbacks won't be run.

> >+      errors.catchAndLog(function () {
> >+        callback.apply(exports, [function() cancelled = true]);
> >+      })();
> 
> What do you think about modifying errors.js so catchAndLog does what its name
> says it'll do and no more, and adding a new method that does what catchAndLog
> does now (return a function)?  Atul and I talked about that, bug 556940 comment
> 10 if you're interested.

For sake of not blocking this, I'd rather get it in as is, and go back. I'll gladly write it though!

> >+// Add observers
> >+observers.add("private-browsing-cancel-vote", onBeforeEvent);
> >+observers.add("private-browsing", onEvent);
> >+observers.add("private-browsing-transition-complete", onAfterEvent);
> 
> Nit: It would be slightly easier to grok the roles of these callbacks if they
> were named something like onPbCancelVote, onPbTransitioning, onPbTransitioned.

I can give them better names.

Paul O'Shannessy [:zpao] (not bugmail, email directly)

Assignee

Comment 20

•

14 years ago

Attached patch Patch v0.3 — Details — Splinter Review

Now with complete docs, tests, and (I think) everything else.

Attachment #442194 - Attachment is obsolete: true

Attachment #443657 - Flags: review?(myk)

Comment 21

•

14 years ago

Comment on attachment 443657 [details] [diff] [review]
Patch v0.3

Sorry for the delay reviewing this!


># User Paul OâShannessy <paul@oshannessy.com>
...
>+ *  Paul OâShannessy <paul@oshannessy.com>

Heh, funky.  This character horkage is apparently a Bugzilla "Attachment Details" bug (the patch itself is just fine).


>diff --git a/packages/jetpack-core/docs/private-browsing.md b/packages/jetpack-core/docs/private-browsing.md

>+This is a boolean. You can read this attribute to determine if private browsing mode is active. You can also set the attribute to enter or exit private browsing.

Nit: it would be handy to wrap this line at 80 columns.


>+### Adding and Removing callback functions ###

Nit: I would capitalize "Callback Functions" here, making this "Adding and Removing Callback Functions", per the "capitalization of all words, except for internal articles, prepositions, and conjunctions" variant of title case <http://en.wikipedia.org/wiki/Title_case#Headings_and_publication_titles>.


>diff --git a/packages/jetpack-core/lib/private-browsing.js b/packages/jetpack-core/lib/private-browsing.js

>+function onBeforeTransition(subject, data) {
>+  subject.QueryInterface(Ci.nsISupportsPRBool);
>+  // If subject is already true (by way of another observer), don't bother
>+  if (subject.data)
>+    return;
>+
>+  if (data == "enter") {
>+    for (let callback in exports.onBeforeStart) {
>+      let cancelled = false;
>+      // Since we're calling a user-defined callback, we need to catchAndLog it.
>+      errors.catchAndLog(function () {
>+        callback.call(exports, function () cancelled = true);
>+      })();
>+
>+      // If cancel() was called, then we want to make sure the PB transition is
>+      // cancelled and also stop executing any other callbacks we have.
>+      if (cancelled) {
>+        subject.data = true;
>+        break;
>+      }
>+    }
>+  }
>+  else if (data == "exit") {
>+    for (let callback in exports.onBeforeStop) {
>+      let cancelled = false;
>+      errors.catchAndLog(function () {
>+        callback.call(exports, function () cancelled = true);
>+      })();
>+      if (cancelled) {
>+        subject.data = true;
>+        break;
>+      }
>+    }
>+  }
>+}

Nit: you could significantly reduce the size of this function by factoring out the data check, e.g. to something like:

let callbacks = data == "enter" ? exports.onBeforeStart :
                data == "exit"  ? exports.onBeforeStop  : [];
for (let callback in callbacks) {
  ...
}


>diff --git a/packages/jetpack-core/tests/test-private-browsing.js b/packages/jetpack-core/tests/test-private-browsing.js

>+// Tests that active has the same value as the private browsing service expects

Nit: per the code style guide <https://wiki.mozilla.org/Labs/Jetpack/Reboot/Style_Guide>, complete sentences should be punctuated.


>+exports.testGetActive = function(test) {

Nit: per Drew, anonymous function expressions should be written with a space between the "function" operator and its open parenthesis.


Otherwise this looks great!  All issues are nits, so fix as you see fit.  r=myk!

Attachment #443657 - Flags: review?(myk) → review+

Paul O'Shannessy [:zpao] (not bugmail, email directly)

Comment 22

•

14 years ago

> >+// Tests that active has the same value as the private browsing service expects
> 
> Nit: per the code style guide
> <https://wiki.mozilla.org/Labs/Jetpack/Reboot/Style_Guide>, complete sentences
> should be punctuated.

Erm, except that this isn't actually a complete sentence (nor are the other test descriptions in this file)!  So I guess the nit would then be that the fragment should either become a complete sentence or not be capitalized, i.e.:

  // This tests that active has the same value as the private browsing service expects.

or:

  // tests that active has the same value as the private browsing service expects

Assignee

Comment 23

•

14 years ago

Checked in (with nits fixed) as http://hg.mozilla.org/labs/jetpack-sdk/rev/572de4f51df2

Target Milestone: Future → 0.4

Paul O'Shannessy [:zpao] (not bugmail, email directly)

Assignee

Updated

•

14 years ago

Status: NEW → RESOLVED

Closed: 14 years ago → 14 years ago

Resolution: --- → FIXED

Paul O'Shannessy [:zpao] (not bugmail, email directly)

Assignee

Updated

•

14 years ago

Depends on: 565338