update Private Browsing API for forwards-compatibility with electrolysis

RESOLVED FIXED in 0.8

Status

defect
P1
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: myk, Assigned: irakli)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment, 5 obsolete attachments)

Reporter

Description

9 years ago
The Private Browsing API should be updated to be forwards-compatible with electrolysis.

In this message <http://groups.google.com/group/mozilla-labs-jetpack/browse_thread/thread/64493c37dfc816c1/>, bsmedberg says:

> onBeforeStart will not work: the browser is not going to wait for the jetpack
> to reply before entering private-browsing mode. There should be a single
> private-browsing.onStart() function which is called asynchronously.
> 
> similarly onStop/onAfterStop.
Reporter

Updated

9 years ago
Priority: -- → P1
Assignee: nobody → rFobic
Posted patch Implements changes (obsolete) — Splinter Review
Attachment #477964 - Flags: review?(myk)
Posted patch Updated documentation v2 (obsolete) — Splinter Review
Forgot to update docs in v1.
Attachment #477964 - Attachment is obsolete: true
Attachment #477964 - Flags: review?(myk)
Reporter

Comment 4

9 years ago
Comment on attachment 481875 [details] [diff] [review]
v3

> This module is available in all applications. However, only Firefox will ever
> transition into or out of private browsing mode. For all other applications,
>-`pb.active` will always return `false`, and none of your callbacks will actually
>-be run.
>+`pb.active` will always return `null`, and none of the events will be emitted.

active -> enabled

But what is the reasoning behind changing "active" to "enabled"?  There doesn't seem to be any connection to E10S compatibility, and "active" has served us well to date.

Similarly, why change "start" and "stop" to "enter" and "exit"?


>+ *  Irakli Gozalishivil <gozala@mozilla.com>

Nit: Gozalishivil -> Gozalishvili


>+// Not sure we'll be able to toggle state synchronously with E10S
>+Object.defineProperty(exports, 'enabled', {
>+  get: function() PrivateBrowsing.enabled,
>+  set: function(value) PrivateBrowsing.enabled = value
>+});

Even without E10S, I think private browsing state changes are asynchronous.
Attachment #481875 - Flags: review?(myk) → review-
Duplicate of this bug: 598930
(In reply to comment #4)
> Comment on attachment 481875 [details] [diff] [review]
> v3
> 
> > This module is available in all applications. However, only Firefox will ever
> > transition into or out of private browsing mode. For all other applications,
> >-`pb.active` will always return `false`, and none of your callbacks will actually
> >-be run.
> >+`pb.active` will always return `null`, and none of the events will be emitted.
> 
> active -> enabled
> 
> But what is the reasoning behind changing "active" to "enabled"?  There doesn't
> seem to be any connection to E10S compatibility, and "active" has served us
> well to date.
> 
> Similarly, why change "start" and "stop" to "enter" and "exit"?
> 

1. I thought that "start" and "stop" in contrast to "enter" "exit" does not expresses the fact that action is complete.
2. Unlike now original implementation was calling 'start', 'stop' before transition was complete which matched naming better.
3. That was also the naming used by nsIPrivateBrowsingService itself.

That being said I can see that this change will break some peoples code so maybe we better stick to original naming.     

> 
> >+ *  Irakli Gozalishivil <gozala@mozilla.com>
> 
> Nit: Gozalishivil -> Gozalishvili
> 
> 
> >+// Not sure we'll be able to toggle state synchronously with E10S
> >+Object.defineProperty(exports, 'enabled', {
> >+  get: function() PrivateBrowsing.enabled,
> >+  set: function(value) PrivateBrowsing.enabled = value
> >+});
> 
> Even without E10S, I think private browsing state changes are asynchronous.
Posted patch v4 (obsolete) — Splinter Review
Pull request: http://github.com/mozillalabs/jetpack-sdk/pull/16
Attachment #477975 - Attachment is obsolete: true
Attachment #481875 - Attachment is obsolete: true
Attachment #482519 - Flags: review?(myk)

Comment 8

9 years ago
(In reply to comment #4)
> >+// Not sure we'll be able to toggle state synchronously with E10S
> >+Object.defineProperty(exports, 'enabled', {
> >+  get: function() PrivateBrowsing.enabled,
> >+  set: function(value) PrivateBrowsing.enabled = value
> >+});
> 
> Even without E10S, I think private browsing state changes are asynchronous.

Async in what sense?
Comment on attachment 482519 [details] [diff] [review]
v4

Reviewing since Myk is out...

>diff --git a/packages/addon-kit/docs/private-browsing.md b/packages/addon-kit/docs/private-browsing.md

>@@ -17,11 +18,11 @@ in a variable.
> <api name="active">
> @property {boolean}
> 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
>+mode is active. You can also set the attribute to start or stops private
> browsing.
> </api>
> 
>-    // If private browsing is active, do something
>+    // If browser is in private browsing mode
>     if (pb.active)
>       doSomething();

Please don't change this part of the documentation.  It's better the way it is.

>-## Callbacks ##
>+##Events##

## Events ##

>+When browser starts or stops private browsing mode following events are
>+emitted:

When the browser starts or stops private browsing mode, the following events are emitted:

>+###start###

### start ###

>+    pb.on('start', function() {
>+      // do something when browser starts private mode

// Do something when the browser starts private browsing mode.

Also, the private-browsing doc currently uses double quotes for strings, so please continue to do so, here and everywhere else.

>+###stop###

### stop ###

>+    pb.on('stop', function() {
>+      // do something when browser stops private mode

// Do something when the browser stops private browsing mode.

> ## Supported Applications ##
> 
> This module is available in all applications. However, only Firefox will ever
> transition into or out of private browsing mode. For all other applications,
>-`pb.active` will always return `false`, and none of your callbacks will actually
>-be run.
>+`pb.active` will always return `null`, and none of the events will be emitted.

Why change this?  If the app doesn't support private browsing, then private browsing is not enabled, so pb.active should be false.  Please keep it false.

>diff --git a/packages/addon-kit/lib/private-browsing.js b/packages/addon-kit/lib/private-browsing.js

>+const { EventEmitter } = require('events');
>+const { setTimeout } = require('timer');
>+
>+const ON_START = 'start',
>+      ON_STOP = 'stop',
>+      ON_TRANSITION = 'private-browsing-transition-complete';

The private-browsing code currently declares all consts with a `const` per identifier and uses double quotes for strings, so please continue to do so, here and everywhere.  Please always be consistent with the style of the code you're editing.

>+const PrivateBrowsing = EventEmitter.compose({

Since this is a singleton and not a class, it should be lowercase, i.e., privateBrowsing.

>+  constructor: function PrivateBrowsing() {
>+    // We only need to add observers if pbService exists.
>+    this._emit = this._emit.bind(this);
>+    // report errors from listeners
>+    this.on('error', console.error);

Use console.exception(), and make sure exception() is bound to console.

>+    if (pbService) {
>+      observers.add(ON_TRANSITION, this.onTransition.bind(this));
>+      this._active = pbService.privateBrowsingEnabled;
>     }

>+  onTransition: function onTransition() {
>+    let active = this._active = pbService.privateBrowsingEnabled;
>+    setTimeout(this._emit, 0, exports.active ? ON_START : ON_STOP);

`active` is local to this function and you don't actually seem to use it anywhere.  Did you mean to pass it to setTimeout, instead of exports.active?

>+  set active(value) {
>+    if (pbService) pbService.privateBrowsingEnabled = !!value

if (pbService)
  pbService.privateBrowsingEnabled = !!value;

Always end statements in semicolons.

Do you need to remove observers when the module is unloaded?  I ask because in other modules like simple-storage you removed observers on module unload.  Won't you leak otherwise?

>diff --git a/packages/addon-kit/tests/test-private-browsing.js b/packages/addon-kit/tests/test-private-browsing.js

>+  exports.testEnter = function(test) {

Please use "start" and "stop" terminology as the API does, so call this testStart or something.

>     test.waitUntilDone();
>-    reset();
>-    pb.onAfterStart = function () {
>-      test.assert(true, "onAfterStart was called");
>+    pb.on('start', function onEnter() {
>+      test.assertEqual(
>+        pbService.privateBrowsingEnabled,
>+        true,
>+        'private mode is active when "start" event is emitted'
>+      );
>+      test.assertEqual(
>+        pb.active,
>+        true,
>+        '`active` is `true` when "start" event is emitted'
>+      );

test.assert(pbService.privateBrowsingEnabled, ...) and test.assert(pb.active, ...) would be simpler...

>+  exports.testExit = function(test) {

Again, testStop or something would be a better name.

>+    test.waitUntilDone();
>+    pb.on('stop', function onExit() {
>+    test.assertEqual(

Fix the indentation here.

>+  exports.testBothListeners = function(test) {

This test is testing that the order in which "start" event listeners called is meaningful.  Is that intentional?  Unless there is a good reason, which I may be missing, we should not be guaranteeing to clients or relying on in our tests the order in which listeners are called.

Moreover, it assumes that all start listeners will be called before all stop listeners, even when one of those start listeners stops private browsing.  That's brittle.

What's the purpose of this test?
Attachment #482519 - Flags: review?(myk) → review-
Reporter

Comment 10

9 years ago
Note also that it isn't necessary to purge the documentation of all references to "entering" and "leaving" private browsing mode in prose describing the functionality of the module.  It's perfectly legitimate to use those synonyms even though the events themselves are called "start" and "stop" (just as it would be legitimate to use "starts" and "stops" to describe "enter" and "leave" events).  In fact, it may even be preferable, to the extent that such synonyms help explain the events better than simply repeating their names.
(In reply to comment #9)
> Comment on attachment 482519 [details] [diff] [review]
> v4
> 
> Reviewing since Myk is out...
> 
> >diff --git a/packages/addon-kit/docs/private-browsing.md b/packages/addon-kit/docs/private-browsing.md
> 
> >@@ -17,11 +18,11 @@ in a variable.
> > <api name="active">
> > @property {boolean}
> > 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
> >+mode is active. You can also set the attribute to start or stops private
> > browsing.
> > </api>
> > 
> >-    // If private browsing is active, do something
> >+    // If browser is in private browsing mode
> >     if (pb.active)
> >       doSomething();
> 
> Please don't change this part of the documentation.  It's better the way it is.
> 
> >-## Callbacks ##
> >+##Events##
> 
> ## Events ##
> 
> >+When browser starts or stops private browsing mode following events are
> >+emitted:
> 
> When the browser starts or stops private browsing mode, the following events
> are emitted:
> 
> >+###start###
> 
> ### start ###
> 
> >+    pb.on('start', function() {
> >+      // do something when browser starts private mode
> 
> // Do something when the browser starts private browsing mode.
> 
> Also, the private-browsing doc currently uses double quotes for strings, so
> please continue to do so, here and everywhere else.
> 
> >+###stop###
> 
> ### stop ###
> 
> >+    pb.on('stop', function() {
> >+      // do something when browser stops private mode
> 
> // Do something when the browser stops private browsing mode.
> 
> > ## Supported Applications ##
> > 
> > This module is available in all applications. However, only Firefox will ever
> > transition into or out of private browsing mode. For all other applications,
> >-`pb.active` will always return `false`, and none of your callbacks will actually
> >-be run.
> >+`pb.active` will always return `null`, and none of the events will be emitted.
> 
> Why change this?  If the app doesn't support private browsing, then private
> browsing is not enabled, so pb.active should be false.  Please keep it false.
> 
> >diff --git a/packages/addon-kit/lib/private-browsing.js b/packages/addon-kit/lib/private-browsing.js
> 
> >+const { EventEmitter } = require('events');
> >+const { setTimeout } = require('timer');
> >+
> >+const ON_START = 'start',
> >+      ON_STOP = 'stop',
> >+      ON_TRANSITION = 'private-browsing-transition-complete';
> 
> The private-browsing code currently declares all consts with a `const` per
> identifier and uses double quotes for strings, so please continue to do so,
> here and everywhere.  Please always be consistent with the style of the code
> you're editing.
> 
> >+const PrivateBrowsing = EventEmitter.compose({
> 
> Since this is a singleton and not a class, it should be lowercase, i.e.,
> privateBrowsing.
> 
> >+  constructor: function PrivateBrowsing() {
> >+    // We only need to add observers if pbService exists.
> >+    this._emit = this._emit.bind(this);
> >+    // report errors from listeners
> >+    this.on('error', console.error);
> 
> Use console.exception(), and make sure exception() is bound to console.
> 
> >+    if (pbService) {
> >+      observers.add(ON_TRANSITION, this.onTransition.bind(this));
> >+      this._active = pbService.privateBrowsingEnabled;
> >     }
> 
> >+  onTransition: function onTransition() {
> >+    let active = this._active = pbService.privateBrowsingEnabled;
> >+    setTimeout(this._emit, 0, exports.active ? ON_START : ON_STOP);
> 
> `active` is local to this function and you don't actually seem to use it
> anywhere.  Did you mean to pass it to setTimeout, instead of exports.active?
> 
> >+  set active(value) {
> >+    if (pbService) pbService.privateBrowsingEnabled = !!value
> 
> if (pbService)
>   pbService.privateBrowsingEnabled = !!value;
> 
> Always end statements in semicolons.
> 
> Do you need to remove observers when the module is unloaded?  I ask because in
> other modules like simple-storage you removed observers on module unload. 
> Won't you leak otherwise?
> 
> >diff --git a/packages/addon-kit/tests/test-private-browsing.js b/packages/addon-kit/tests/test-private-browsing.js
> 
> >+  exports.testEnter = function(test) {
> 
> Please use "start" and "stop" terminology as the API does, so call this
> testStart or something.
> 
> >     test.waitUntilDone();
> >-    reset();
> >-    pb.onAfterStart = function () {
> >-      test.assert(true, "onAfterStart was called");
> >+    pb.on('start', function onEnter() {
> >+      test.assertEqual(
> >+        pbService.privateBrowsingEnabled,
> >+        true,
> >+        'private mode is active when "start" event is emitted'
> >+      );
> >+      test.assertEqual(
> >+        pb.active,
> >+        true,
> >+        '`active` is `true` when "start" event is emitted'
> >+      );
> 
> test.assert(pbService.privateBrowsingEnabled, ...) and test.assert(pb.active,
> ...) would be simpler...
> 
> >+  exports.testExit = function(test) {
> 
> Again, testStop or something would be a better name.
> 
> >+    test.waitUntilDone();
> >+    pb.on('stop', function onExit() {
> >+    test.assertEqual(
> 
> Fix the indentation here.
> 
> >+  exports.testBothListeners = function(test) {
> 
> This test is testing that the order in which "start" event listeners called is
> meaningful.  Is that intentional?  Unless there is a good reason, which I may
> be missing, we should not be guaranteeing to clients or relying on in our tests
> the order in which listeners are called.
> 

That's a test which was there and I believe there is a good reason for it. See my next comment. We can guarantee the order in which listeners are called EventEmitter calls them in order they were added.

> Moreover, it assumes that all start listeners will be called before all stop
> listeners, even when one of those start listeners stops private browsing. 
> That's brittle.
> 
> What's the purpose of this test?

From my perspective this is verification that all the "start" listeners are called before switching to normal mode happens. I think it's important, otherwise it would mean that only some listeners are called. We can guarantee that since changing the mode happens asynchronously (in next turn of the event loop) while all the listeners are called synchronously in the same turn of the event loop.
Posted patch v5 (obsolete) — Splinter Review
Attachment #482519 - Attachment is obsolete: true
Attachment #482870 - Flags: review?(adw)
(In reply to comment #11)
> From my perspective this is verification that all the "start" listeners are
> called before switching to normal mode happens. I think it's important,
> otherwise it would mean that only some listeners are called. We can guarantee
> that since changing the mode happens asynchronously (in next turn of the event
> loop) while all the listeners are called synchronously in the same turn of the
> event loop.

But that's only in this particular implementation, right?  When e10s lands, private browsing changes will happen in the browser chrome process, and that process can't wait until all of the jetpack process's start listeners are called before potentially changing private browsing state again.  I agree that calling only some listeners and not others would be bad, but that's not what I'm saying; I'm only saying that we can't guarantee the ordering of start and stop, especially when a start listener stops private browsing.  (Or at least such a guarantee would be pretty tricky to get right and the upsides wouldn't be worth the cost.)
(In reply to comment #13)
> (In reply to comment #11)
> > From my perspective this is verification that all the "start" listeners are
> > called before switching to normal mode happens. I think it's important,
> > otherwise it would mean that only some listeners are called. We can guarantee
> > that since changing the mode happens asynchronously (in next turn of the event
> > loop) while all the listeners are called synchronously in the same turn of the
> > event loop.
> 
> But that's only in this particular implementation, right?  When e10s lands,
> private browsing changes will happen in the browser chrome process, and that
> process can't wait until all of the jetpack process's start listeners are
> called before potentially changing private browsing state again. 

I don't think it will be an issue with e10s land either since messages across processes will be enqueued and delivered in the right order at least that's my expectation. I do agree though that in case if some listener blocks queue for a long time and meanwhile user switched mode some listeners may be called and when browser is in the different state. Anyway that doesn't harms ordering.

> calling only some listeners and not others would be bad, but that's not what
> I'm saying; I'm only saying that we can't guarantee the ordering of start and
> stop, especially when a start listener stops private browsing.  (Or at least
> such a guarantee would be pretty tricky to get right and the upsides wouldn't
> be worth the cost.)

I do disagree switching mode form listener must happen in the next turn of the event queue and that's when it will be delivered to the UI process. I do think it's pretty easy to arrange.

I can remove this test if that's what you are suggesting but I do believe it's better to keep it. If it happens so that for some reason order can't be guaranteed we will remove it then.
Comment on attachment 482870 [details] [diff] [review]
v5

>+const privateBrowsing = EventEmitter.compose({
>+  constructor: function PrivateBrowsing() {
>+    // Binding method to instance since it will be used with `setTimeout`.
>+    this._emit = this._emit.bind(this);
>+    // Report unhandled errors from listeners
>+    this.on("error", console.exception.bind(console));
>+    unload.when(this._destructor.bind(this));

This is really subtle, but please use unload.ensure(this) here instead of when().  Atul explains why at bug 547417 comment 19.

>+  exports.testStart = function(test) {
>+    test.waitUntilDone();
>+    pb.on("start", function onStart() {
>+      test.assert(pbService.privateBrowsingEnabled,
>+                  "private mode is active when \"start\" event is emitted");
>+      test.assert(pb.active,
>+                  "`active` is `true` when \"start\" event is emitted");

Sorry, I didn't mean to force you to escape quotes.  Using single quotes to avoid escaping is totally cool.  I just meant that in general use double quotes if the module you're in does.

> else {
>   // tests for the case where private browsing doesn't exist
>@@ -282,13 +169,15 @@ else {
>     // should have been called. We'll just test one callback since they are
>     // under the same code path.
>     let wasActivated = false;
>+
>     pb.onStart = function () {
>       wasActivated = true;
>     }
>+

Does pb.onStart() still mean anything after these API changes?  Actually what testNoImpl is doing doesn't seem necessary... Could you replace all of that with a simple test.pass("This app does not support private-browsing") or something?

(In reply to comment #11)
> > This test is testing that the order in which "start" event listeners called is
> > meaningful.  Is that intentional?  Unless there is a good reason, which I may
> > be missing, we should not be guaranteeing to clients or relying on in our tests
> > the order in which listeners are called.
> > 
> 
> That's a test which was there and I believe there is a good reason for it. See
> my next comment. We can guarantee the order in which listeners are called
> EventEmitter calls them in order they were added.

OK, I still don't think that we should guarantee or rely on the order in which start listeners or stop listeners are called, but I guess it's not a big deal.

r- for reason in comment 13 and the unload.when() issue.  Other than that, looks OK to me.
Attachment #482870 - Flags: review?(adw) → review-
Posted patch v6Splinter Review
Attachment #482870 - Attachment is obsolete: true
Attachment #482890 - Flags: review?(adw)
(In reply to comment #15)
> Comment on attachment 482870 [details] [diff] [review]
> v5
> 
> >+const privateBrowsing = EventEmitter.compose({
> >+  constructor: function PrivateBrowsing() {
> >+    // Binding method to instance since it will be used with `setTimeout`.
> >+    this._emit = this._emit.bind(this);
> >+    // Report unhandled errors from listeners
> >+    this.on("error", console.exception.bind(console));
> >+    unload.when(this._destructor.bind(this));
> 
> This is really subtle, but please use unload.ensure(this) here instead of
> when().  Atul explains why at bug 547417 comment 19.
> 
> >+  exports.testStart = function(test) {
> >+    test.waitUntilDone();
> >+    pb.on("start", function onStart() {
> >+      test.assert(pbService.privateBrowsingEnabled,
> >+                  "private mode is active when \"start\" event is emitted");
> >+      test.assert(pb.active,
> >+                  "`active` is `true` when \"start\" event is emitted");
> 
> Sorry, I didn't mean to force you to escape quotes.  Using single quotes to
> avoid escaping is totally cool.  I just meant that in general use double quotes
> if the module you're in does.
> 
> > else {
> >   // tests for the case where private browsing doesn't exist
> >@@ -282,13 +169,15 @@ else {
> >     // should have been called. We'll just test one callback since they are
> >     // under the same code path.
> >     let wasActivated = false;
> >+
> >     pb.onStart = function () {
> >       wasActivated = true;
> >     }
> >+
> 
> Does pb.onStart() still mean anything after these API changes?  Actually what
> testNoImpl is doing doesn't seem necessary... Could you replace all of that
> with a simple test.pass("This app does not support private-browsing") or
> something?
> 
> (In reply to comment #11)
> > > This test is testing that the order in which "start" event listeners called is
> > > meaningful.  Is that intentional?  Unless there is a good reason, which I may
> > > be missing, we should not be guaranteeing to clients or relying on in our tests
> > > the order in which listeners are called.
> > > 
> > 
> > That's a test which was there and I believe there is a good reason for it. See
> > my next comment. We can guarantee the order in which listeners are called
> > EventEmitter calls them in order they were added.
> 
> OK, I still don't think that we should guarantee or rely on the order in which
> start listeners or stop listeners are called, but I guess it's not a big deal.
> 

Ok let's keep it so far then. If we cant guarantee nor order nor state of browser then what is the point of having listeners in first place ?

> r- for reason in comment 13 and the unload.when() issue.  Other than that,
> looks OK to me.
and btw I kept one assert in testNoImpl to make sure that pb.active is `false` as documentation suggests that.
Comment on attachment 482890 [details] [diff] [review]
v6

I only meant it's the ordering of start listeners relative to other start listeners (and stop listeners to other stop listeners) that I'm OK with.

But, I thought more about what you said about queueing in the event loop in the jetpack process, and I think you're right.  Even if a start listener stops private browsing and the chrome process notifies the jetpack process about the new stop event before all start listeners are notified, the jetpack process event queue has a backlog of start listeners that it has to go through before it gets to the new stop event.  Thanks for being patient with me.

> else {
>   // tests for the case where private browsing doesn't exist
>   exports.testNoImpl = function (test) {
>     test.assertEqual(pb.active, false,
>                      "pb.active returns false when private browsing isn't supported");
>-
>-
>-    // Setting pb.active = true shouldn't have any effect. Also, no callbacks
>-    // should have been called. We'll just test one callback since they are
>-    // under the same code path.
>-    let wasActivated = false;
>-    pb.onStart = function () {
>-      wasActivated = true;
>-    }
>-    pb.active = true;
>-    test.assertEqual(pb.active, false,
>-                     "pb.active returns false even when set to true");
>-    test.assertEqual(wasActivated, false,
>-                     "onStart callback wasn't run when PB isn't supported");
>-  }
>+  };

Oh, nice.
Attachment #482890 - Flags: review?(adw) → review+
landed by: de840ab37caf

Thanks for the last moment reviews! You have been quite patient as well :)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Reporter

Comment 21

9 years ago
The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".
Component: Jetpack SDK → General
Product: Mozilla Labs → Add-on SDK
QA Contact: jetpack-sdk → general
Version: Trunk → unspecified
You need to log in before you can comment on or make changes to this bug.