Paul O'Shannessy [:zpao] (not bugmail, email directly) Reporter
	Description • 15 years ago

When we get the power notification service, it would be great to write sessionstore.js less often.

In bug 506132 we reduced the frequency dramatically, but like Vlad said there, it'd be ideal to have finer control for battery vs plugged-in.

	Tim Taubert [:ttaubert] (inactive)
	Comment 1 • 11 years ago

This is a great idea. I think we should use navigator.battery now that it landed moons ago and should be stable. Here's an example of how to use it:

http://hg.mozilla.org/mozilla-central/file/45128af17739/dom/battery/test/test_battery_basics.html

With battery.charging=false we should definitely write less often to disk. With battery.level<0.3 we could lower the impact even further.

	Tim Taubert [:ttaubert] (inactive)
	Comment 2 • 11 years ago

As a first version it would be great to increase the sessionstore write interval to 1s if battery.charging=false. We should add a preference named browser.sessionstore.interval_battery with a default value of 60000 (ms) and read that.

	Wayne Mery (:wsmwk)
	Comment 3 • 11 years ago

perhaps a different bug, but very frequently (>50% ?) I open a new tab via thunderbird and immediately sessionstore.js is being written.  Very irritating because the jank is immediate, locking firefox.  

I don't understand why there isn't at least a delay between opening tab and sesion save, so I'm wondering if there isn't some type of bug.

	Anup Kumar [:reznord]
	Comment 4 • 11 years ago

Hi, 

I am willing to work on this bug. So please I request you to assign it to me.

Thanks in Advance,

Regards,
A. Anup

	Tim Taubert [:ttaubert] (inactive)
	Comment 5 • 11 years ago

(In reply to Anup Allamsetty from comment #4)
> I am willing to work on this bug. So please I request you to assign it to me.

Great! Should you have any questions, don't hesitate to ask!

	Tim Taubert [:ttaubert] (inactive)
	Comment 6 • 11 years ago

(In reply to Wayne Mery (:wsmwk) from comment #3)
> perhaps a different bug, but very frequently (>50% ?) I open a new tab via
> thunderbird and immediately sessionstore.js is being written.  Very
> irritating because the jank is immediate, locking firefox.  

This is definitely another bug. Can you please file a new issue? Thanks!

	Tim Taubert [:ttaubert] (inactive)
	Comment 7 • 11 years ago

Looks like this is free again.

	Manish Goregaokar [:manishearth]
	Comment 8 • 11 years ago

Split the pref into 4, where the defaults are as follows:

Charger, High battery:   10000
Charger, Low battery:    10000
Unplugged, High battery: 60000
Unplugged, Low battery:  120000

Low battery is defined by being below 30% charged.

There probably is room for improvement on the actual values.

	Tim Taubert [:ttaubert] (inactive)
	Comment 9 • 11 years ago

Comment on attachment 795093 [details] [diff] [review]
Patch 0.1

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

Thanks for your patch, Manish! You unfortunately changed SessionStore code for the Metro (Windows 8) version of Firefox. This bug is about the universal Desktop version, though. You will need to change code in browser/components/sessionstore/src/SessionSaver.jsm. It should be sufficient to modify the gInterval getter here.

::: b2g/app/b2g.js
@@ +67,5 @@
>  pref("browser.sessionstore.resume_from_crash_timeout", 60); // minutes
> +pref("browser.sessionstore.interval", 10000); // milliseconds (Interval  when device is charging)
> +pref("browser.sessionstore.interval_battery_not_charging", 60000); // milliseconds (Interval when device is unplugged)
> +pref("browser.sessionstore.interval_battery_low_charging", 10000); // milliseconds (Interval when battery is low but charging)
> +pref("browser.sessionstore.interval_battery_low_not_charging", 120000); // milliseconds (Interval when battery is low and unplugged)

This is the wrong file, the preference should be added to browser/app/profile/firefox.js.

Also I think we shouldn't really overcomplicate this as we don't have good or any measurements on how this affects the power consumption. Adding a single browser.sessionstore.interval_battery preference with a default value of 60000 should be sufficient.

	Manish Goregaokar [:manishearth]
	Comment 10 • 11 years ago

Ah, firefox.js makes more sense, I was confused as to where b2g and metro came into the picture.


Added a new patch. (I agree that we don't really need to complicate this, so now it just checks whether or not the battery is charging)

Thanks for the input!

	Tim Taubert [:ttaubert] (inactive)
	Comment 11 • 11 years ago

Comment on attachment 795375 [details] [diff] [review]
Patch 0.2

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

Thanks, Manish. Looking good so far!

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +47,5 @@
> +
> +    // Cancel any pending runs and call runDelayed() with
> +    // zero to apply the newly configured interval.
> +    SessionSaverInternal.cancel();
> +    SessionSaverInternal.runDelayed(0);

This should only be done when battery.charging=false. The code in gInterval needs to be adapted to only call runDelayed() when battery.charging=true.

@@ +163,5 @@
>        return;
>      }
>  
>      // Interval until the next disk operation is allowed.
> +    delay = Math.max(this._lastSaveTime + (navigator.battery.charging?gInterval:gIntervalBattery) - Date.now(), delay, 0);

We could add another function named getInterval() - above createSupportsString() -  that returns gInterval or gIntervalBattery, I think that would be a little easier on the eyes.

::: browser/app/profile/firefox.js
@@ +820,5 @@
>  
>  // minimal interval between two save operations in milliseconds
>  pref("browser.sessionstore.interval", 15000);
> +// interval  when device is unplugged
> +pref("browser.sessionstore.interval_battery_not_charging", 60000);

Let's call this interval_battery.

	Manish Goregaokar [:manishearth]
	Comment 12 • 11 years ago

Hmm, I'm not too sure why we need to check for the battery in the observer as the observer will probably be only called once in a blue moon (when someone goes to about:config and changes the pref). But I'll add it, no harm in doing so (and it makes the code better).

Thanks.

	Tim Taubert [:ttaubert] (inactive)
	Comment 13 • 11 years ago

Yes, it's certainly not a very common occurrence but I think it's the correct thing to do.

	Manish Goregaokar [:manishearth]
	Comment 15 • 11 years ago

Out of curiosity, how is scope being handled here? Looking at createSupportsString() (and the new getInterval() ), both are in global scope. As far as I can tell, the JSM file will be imported elsewhere and those functions will be global-scoped, too. Is that necessary for functions which only need to be used within that module?

	Tim Taubert [:ttaubert] (inactive)
	Comment 16 • 11 years ago

Comment on attachment 795874 [details] [diff] [review]
Patch 0.3

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

Thanks! r=me with the nits below fixed.

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +27,5 @@
>  
>    // Observer that updates the cached value when the preference changes.
>    Services.prefs.addObserver(PREF, () => {
>      this.gInterval = Services.prefs.getIntPref(PREF);
> +    

There seems to be some white space on the line here?

@@ +30,5 @@
>      this.gInterval = Services.prefs.getIntPref(PREF);
> +    
> +    if (navigator.battery.charging) {
> +      // Cancel any pending runs and call runDelayed() with
> +      // zero to apply the newly configured interval.

"call runDelayed() with zero" is a little specific. "call runDelayed()" is totally sufficient.

@@ +44,5 @@
> +  const PREF = "browser.sessionstore.interval_battery";
> +
> +  // Observer that updates the cached value when the preference changes.
> +  Services.prefs.addObserver(PREF, () => {
> +    this.gIntervalBattery = Services.prefs.getIntPref(PREF);

Add a new line after "this.gIntervalBattery = ..." please.

@@ +47,5 @@
> +  Services.prefs.addObserver(PREF, () => {
> +    this.gIntervalBattery = Services.prefs.getIntPref(PREF);
> +    if (!navigator.battery.charging) {
> +      // Cancel any pending runs and call runDelayed() with
> +      // zero to apply the newly configured interval.

Please remove the "with zero" here as well.

@@ +62,5 @@
> +  if (navigator.battery.charging) {
> +    return gInterval;
> +  }
> +  return gIntervalBattery;
> +  

This empty line should be removed.

	Tim Taubert [:ttaubert] (inactive)
	Comment 17 • 11 years ago

(In reply to Manish Goregaokar [:manishearth] from comment #15)
> Out of curiosity, how is scope being handled here? Looking at
> createSupportsString() (and the new getInterval() ), both are in global
> scope. As far as I can tell, the JSM file will be imported elsewhere and
> those functions will be global-scoped, too. Is that necessary for functions
> which only need to be used within that module?

No, those things are only global per module. JSMs only export stuff listed in EXPORTED_SYMBOLS which is mostly defined at the top of files.

	Manish Goregaokar [:manishearth]
	Comment 18 • 11 years ago

Fixed as per comment #16.

	Manish Goregaokar [:manishearth]
	Comment 19 • 11 years ago

(In reply to Tim Taubert [:ttaubert] from comment #17)

> No, those things are only global per module. JSMs only export stuff listed
> in EXPORTED_SYMBOLS which is mostly defined at the top of files.

Ah, I see. Thanks for the clarification.

	Tim Taubert [:ttaubert] (inactive)
	Comment 20 • 11 years ago

Comment on attachment 795879 [details] [diff] [review]
Patch 0.4

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

I just noticed that this of course doesn't run at all in a JSM. 'navigator' is only defined for windows but not in module scope. What we should do is introduce a isBatteryCharging() function that does:

function isBatteryCharging() {
  let navigator = Services.wm.getMostRecentWindow("navigator:browser").navigator;
  return navigator.battery.charging;
}

Sorry, that's what I get for doing reviews that early in the morning :)

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +26,5 @@
>    const PREF = "browser.sessionstore.interval";
>  
>    // Observer that updates the cached value when the preference changes.
>    Services.prefs.addObserver(PREF, () => {
> +    this.gInterval = Services.prefs.getIntPref(PREF); 

Nit: trailing white space.

@@ +62,5 @@
> +function getInterval(){
> +  if (navigator.battery.charging) {
> +    return gInterval;
> +  }
> +  return gIntervalBattery; 

Nit: trailing white space.

	Manish Goregaokar [:manishearth]
	Comment 21 • 11 years ago

Fix comment #20

	Manish Goregaokar [:manishearth]
	Comment 22 • 11 years ago

Hmm, I had a suspicion that `window` wouldn't be loaded or in scope but I didn't know how to force-fetch it. Thanks :)

Is there any style guide for Mozilla code?

	Tim Taubert [:ttaubert] (inactive)
	Comment 23 • 11 years ago

Comment on attachment 795888 [details] [diff] [review]
Patch 0.5

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

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +26,5 @@
>    const PREF = "browser.sessionstore.interval";
>  
>    // Observer that updates the cached value when the preference changes.
>    Services.prefs.addObserver(PREF, () => {
> +    this.gInterval = Services.prefs.getIntPref(PREF); 

Nit: trailing white space

@@ +58,5 @@
> +  return Services.prefs.getIntPref(PREF);
> +});
> +
> +
> +function isBatteryCharging() {

Please add a comment describing what this function does.

@@ +59,5 @@
> +});
> +
> +
> +function isBatteryCharging() {
> +  let navigator = Services.wm.getMostRecentWindow("navigator:browser").navigator;

Of course, getMostRecentWindow() can return null. We need to guard against this and should just return true in that case. However, in case of Mac OS X we can end up with no windows and thus we should probably go a different route:

function isBatteryCharging() {
  return Services.appShell.hiddenDOMWindow.navigator.battery.charging;
}

We always have a hidden window that is used for different purposes. We should just use its navigator. Sorry again, I wish we had a BatteryManager service that would work without a window binding.

@@ +68,5 @@
> +function getInterval(){
> +  if (isBatteryCharging()){
> +    return gInterval;
> +  }
> +  return gIntervalBattery; 

Nit: trailing white space

	Tim Taubert [:ttaubert] (inactive)
	Comment 24 • 11 years ago

(In reply to Manish Goregaokar [:manishearth] from comment #22)
> Is there any style guide for Mozilla code?

We have a small guide here:

https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

But this doesn't necessarily apply to all files anymore. Use your best judgement and look at the code surrounding your changes, we try to keep styles per file as that improves readability the most.

	Manish Goregaokar [:manishearth]
	Comment 25 • 11 years ago

Done, thanks :)

	Tim Taubert [:ttaubert] (inactive)
	Comment 26 • 11 years ago

Comment on attachment 796042 [details] [diff] [review]
Patch 0.6

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

Thanks Manish, that looks great but unfortunately it is missing the change to browser/app/profile/firefox.js?

	Manish Goregaokar [:manishearth]
	Comment 27 • 11 years ago

Ah, oops. Here it is again.

	Tim Taubert [:ttaubert] (inactive)
	Comment 28 • 11 years ago

Comment on attachment 796601 [details] [diff] [review]
Patch 0.7

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

Thanks Manish! That looks great.

Can you please prepare the patch for checkin? Here's a good post that describes how to do that:

http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

	Manish Goregaokar [:manishearth]
	Comment 29 • 11 years ago

Added commit data

	Tim Taubert [:ttaubert] (inactive)
	Comment 30 • 11 years ago

Comment on attachment 796720 [details] [diff] [review]
Patch 0.8

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

Thanks! No need to ask me for review again. I'll check it in soon!

	Manish Goregaokar [:manishearth]
	Comment 31 • 11 years ago

Ah, alright. I'm not entirely sure how these things work :P 

Thanks!

	Tim Taubert [:ttaubert] (inactive)
	Comment 32 • 11 years ago

https://hg.mozilla.org/integration/fx-team/rev/2514aac648b2

Thanks Manish, great work! The patch landed on the fx-team branch and will be merged to our main branch (mozilla-central) later today or tomorrow and will then be in Nightly.

Hope to see you around for another few "good first bugs" :)

	Manish Goregaokar [:manishearth]
	Comment 33 • 11 years ago

Thanks! :D

I intend to work on Bug 910670 next, it's something I always wanted for the proxy system.

	Tim Taubert [:ttaubert] (inactive)
	Comment 34 • 11 years ago

Sorry, I had to back this out:

https://hg.mozilla.org/integration/fx-team/rev/996762943f30

Turns out I misunderstood the Battery API. With my machine fully charged, battery.charging=false. That makes sense but I'm not sure how we should detect that a battery is currently discharging.

	Manish Goregaokar [:manishearth]
	Comment 35 • 11 years ago

(In reply to Tim Taubert [:ttaubert] (on PTO starting Fri Aug 30th, back Mon Sep 9th) from comment #34)
> Turns out I misunderstood the Battery API. With my machine fully charged,
> battery.charging=false. That makes sense but I'm not sure how we should
> detect that a battery is currently discharging.

Seems like an easy enough fix, add a battery.level checker. I'm assuming that when the battery is fully charged, battery.charging will be false but battery.level will be 1.0.


Alternatively, we just check battery.chargingTime (https://developer.mozilla.org/en-US/docs/Web/API/BatteryManager.chargingTime). This is Infinity if the battery is discharging, and some number if not.

	David Teller [:Yoric] - still alive but not very active
	Comment 36 • 11 years ago

> Seems like an easy enough fix, add a battery.level checker. I'm assuming that when the battery is fully charged, battery.charging will be false but battery.level will be 1.0

I wouldn't be sure, given that many cannot load up to 100% or something such. I believe that chargingTime is a better idea.

	Manish Goregaokar [:manishearth]
	Comment 37 • 11 years ago

This patch checks isFinite(battery.chargingTime) instead.

	David Teller [:Yoric] - still alive but not very active
	Comment 38 • 11 years ago

Comment on attachment 797195 [details] [diff] [review]
session.patch

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

This looks good to me.
As mentioned below, we'll need a preference to fake isBatteryCharging, to ensure that session restore still works when the battery is charging.

::: browser/app/profile/firefox.js
@@ +819,5 @@
>  pref("browser.sessionstore.resume_session_once", false);
>  
>  // minimal interval between two save operations in milliseconds
>  pref("browser.sessionstore.interval", 15000);
> +// interval  when device is unplugged

Nit: "minimal interval between two save operations when device is unplugged"

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +39,5 @@
>  
>    return Services.prefs.getIntPref(PREF);
>  });
>  
> +XPCOMUtils.defineLazyGetter(this, "gIntervalBattery", function () {

Nit: This will need some documentation, like "gInterval".

@@ +60,5 @@
> +
> +
> +// Check if battery is charging
> +function isBatteryCharging(){
> +   return isFinite(Services.appShell.hiddenDOMWindow.navigator.battery.chargingTime); //chargingTime is Infinity when discharging (or when the remaining time cannot be determined)

Nit: Could you cut that comments into several lines?

@@ +61,5 @@
> +
> +// Check if battery is charging
> +function isBatteryCharging(){
> +   return isFinite(Services.appShell.hiddenDOMWindow.navigator.battery.chargingTime); //chargingTime is Infinity when discharging (or when the remaining time cannot be determined)
> +}

Also, we will need a preference to be able to fake isBatteryCharging().

@@ +65,5 @@
> +}
> +
> +
> +// Get the current session store interval based on battery status
> +function getInterval(){

This should probably be called |getSavingInterval()|.

	Manish Goregaokar [:manishearth]
	Comment 39 • 11 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #38)

> As mentioned below, we'll need a preference to fake isBatteryCharging, to
> ensure that session restore still works when the battery is charging.

Can't we achieve the same effect by setting .interval_battery to the value in .interval? Seems a bit redundant to have an extra preference for this.

I'll get to work on the rest.

	David Teller [:Yoric] - still alive but not very active
	Comment 40 • 11 years ago

The idea is not to test the actual value of the interval but to test that all code paths work.

	Manish Goregaokar [:manishearth]
	Comment 41 • 11 years ago

Done, I think. I added a three-state `browser.sessionstore.fake_battery` preference. We may want to later change it to a general "battery faking" preference that fakes all battery-behavior, but for that we will need to make changes to the module that handles `navigator.battery`.

	David Teller [:Yoric] - still alive but not very active
	Comment 42 • 11 years ago

Comment on attachment 803794 [details] [diff] [review]
Patch 0.10

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

::: browser/app/profile/firefox.js
@@ +823,5 @@
> +// Minimal interval between two save operations when device is unplugged
> +pref("browser.sessionstore.interval_battery", 60000);
> +// Preference to fake battery charging status.
> +// 0 = Don't fake, 1 = always charging, 2= always unplugged
> +pref("browser.sessionstore.fake_battery", 0);

Nit: Perhaps "browser.sessionstore.debug.fake_battery", just to make sure that this setting is not meant to be tweaked in production code.
Also, "2 =", with whitespace.

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +62,5 @@
> +
> +// Check if battery is charging
> +function isBatteryCharging(){
> +   //Fake battery status if the fake_battery preference is set
> +   let fakePref=Services.prefs.getIntPref("browser.sessionstore.fake_battery");

Could you cache the value of this preference somewhere?

@@ +64,5 @@
> +function isBatteryCharging(){
> +   //Fake battery status if the fake_battery preference is set
> +   let fakePref=Services.prefs.getIntPref("browser.sessionstore.fake_battery");
> +   if(fakePref != 0){
> +     return !(fakePref-1); //true if fakePref==1, otherwise false

Nit: whitespace between "if" and "(".

@@ +67,5 @@
> +   if(fakePref != 0){
> +     return !(fakePref-1); //true if fakePref==1, otherwise false
> +   }
> +
> +   return isFinite(Services.appShell.hiddenDOMWindow.navigator.battery.chargingTime); 

Nit: whitespace at the end of the line.

	Manish Goregaokar [:manishearth]
	Comment 43 • 11 years ago

Fixed nits. I moved the pref over to a const FAKEPREF, and renamed it.

	David Teller [:Yoric] - still alive but not very active
	Comment 44 • 11 years ago

Comment on attachment 805292 [details] [diff] [review]
Patch 0.11

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

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +25,3 @@
>  XPCOMUtils.defineLazyGetter(this, "gInterval", function () {
>    const PREF = "browser.sessionstore.interval";
> +  const FAKEPREF=Services.prefs.getIntPref("browser.sessionstore.debug..ake_battery");

Typo.
Also, you don't react if the preference changes, so we can't use this preference for testing anymore.

	David Teller [:Yoric] - still alive but not very active
	Comment 46 • 11 years ago

Comment on attachment 805297 [details] [diff] [review]
Patch 0.12

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

Trivial nit, but otherwise, this looks good.

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +67,5 @@
> +  Services.prefs.addObserver(PREF, () => {
> +    this.gFakePref = Services.prefs.getIntPref(PREF);
> +
> +    }
> +  

Nit: layout is a bit weird.

	David Teller [:Yoric] - still alive but not very active
	Comment 47 • 11 years ago

Could you add a test that performs saving, with all three possible values of the faking preference?

	Manish Goregaokar [:manishearth]
	Comment 48 • 11 years ago

Turns out it wasn't trivial, had missed a close paren there (deleted the wrong line).

	Manish Goregaokar [:manishearth]
	Comment 49 • 11 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #47)
> Could you add a test that performs saving, with all three possible values of
> the faking preference?

Hmm, I'm not exactly sure how to write tests. I could try, but I don't know where to start. (How to write tests, how to check if the session was stored, etc etc)

Also, such a test would take minutes (not seconds) to run, I guess, as we need to wait for the sessionstore to happen.

	David Teller [:Yoric] - still alive but not very active
	Comment 50 • 11 years ago

Comment on attachment 806030 [details] [diff] [review]
Patch 0.13

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

Looks good to me. No need to ask for further feedback atm, but please get started with tests.

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +66,5 @@
> +  // Observer that updates the cached value when the preference changes.
> +  Services.prefs.addObserver(PREF, () => {
> +    this.gFakePref = Services.prefs.getIntPref(PREF);
> +  },false);
> +  

Nit: whitespace.

	Tim Taubert [:ttaubert] (inactive)
	Comment 51 • 11 years ago

Comment on attachment 806030 [details] [diff] [review]
Patch 0.13

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

Having a fake battery pref doesn't sound like a great idea to me. I think we should take a look at the EmulatorBattery [1] and see if we can write a couple of marionette tests for this? There's a couple of tests here [2] that make use of this.

Sorry for chiming in so late, I didn't have a better solution to this until I stumbled over the EmulatorBattery yesterday.

[1] https://developer.mozilla.org/en-US/docs/Marionette/EmulatorBattery
[2] https://mxr.mozilla.org/mozilla-central/source/dom/battery/test/marionette/

	Manish Goregaokar [:manishearth]
	Comment 52 • 11 years ago

Seems good. But I don't know how to write tests :/

Testing this will involve quite a few things -- crashing the browser and restarting to ensure that the session was saved; I'm not sure how to do that in a test.

	David Teller [:Yoric] - still alive but not very active
	Comment 53 • 11 years ago

Manish, if you take a look at the tests in sessionstore/tests, you will see many things that we can test without having to crash the browser.

	Manish Goregaokar [:manishearth]
	Comment 54 • 11 years ago

OK, I'll have a look, thanks.

I'll be busy for the next two weeks though, won't be able to look into it until then.

	David Teller [:Yoric] - still alive but not very active
	Comment 55 • 11 years ago

Hi Manish, did you get some time to continue your work on this?

	manishsmail
	Comment 56 • 11 years ago

Shoot, I forgot. Having a look now.

The test `browser_sessionStorage.js` in the folder you mentioned seems to be the one I need to extend. However, it doesn't check the timing; it seems to set the time pref to 0 and then immediately check for session storage. 

I'll have to figure out how to *reliably* wait for x seconds in the test.

	David Teller [:Yoric] - still alive but not very active
	Comment 57 • 11 years ago

(In reply to manishsmail from comment #56)
> Shoot, I forgot. Having a look now.
> 
> The test `browser_sessionStorage.js` in the folder you mentioned seems to be
> the one I need to extend. However, it doesn't check the timing; it seems to
> set the time pref to 0 and then immediately check for session storage.

That one is about DOM storage, which your patch doesn't touch, so that's probably not what you want.

> I'll have to figure out how to *reliably* wait for x seconds in the test.

You want to test that sessions are saved after an interval X when on battery and an interval Y when not on battery. Here's how you can do it:
- set emulator battery to emulate a "not on battery state" (I don't know how to do this, but look at comment 51);
- set the on-battery delay to 5 years or something equally huge;
- set the off-battery delay to 1 ms or something equally tiny;
- open a few tabs on well-known URLs (let's call this state 1);
- wait for 100 ms;
- check file sessionstore.js to ensure that it stores state 1, i.e. the well-known URLs appear;
(you have confirmed that the off-battery delay is used, rather than the on-battery delay)
- change emulator battery state;
- exchange on-battery and off-battery delay;
- change the state to state 2;
- wait for 100 ms;
- check file sessionstore.js to ensure that it stores state 2.

	Manish Goregaokar [:manishearth]
	Comment 58 • 11 years ago

I'm away till Dec 26ish; I'll have a look then.

	David Teller [:Yoric] - still alive but not very active
	Comment 59 • 10 years ago

Any news?

	Manish Goregaokar [:manishearth]
	Comment 60 • 10 years ago

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #59)
> Any news?

I have a half-written test, but I have a midsemester exam for this and the next week, so I'm not doing any larger patches. I plan to write this test and another mochitest from a different bug afterwards. 

Feel free to reassign if this would be too long :)

	Manish Goregaokar [:manishearth]
	Comment 61 • 10 years ago

(In reply to Tim Taubert [:ttaubert] from comment #51)
> Comment on attachment 806030 [details] [diff] [review]
> Having a fake battery pref doesn't sound like a great idea to me. I think we
> should take a look at the EmulatorBattery [1] and see if we can write a
> couple of marionette tests for this? There's a couple of tests here [2] that
> make use of this.

I just had a look at this; it seems like EmulatorBattery is a marionette thing, and mach is telling me that marionette tests can only be run in a b2g context. The MDN page makes it look like a FxOS thing too.

I guess we'll have to stick with a plain mochitest, then. Or is there a way to run marionette for desktop Fx?

	Manish Goregaokar [:manishearth]
	Comment 62 • 10 years ago

Same patch, doesn't contain any tests.

	Manish Goregaokar [:manishearth]
	Comment 63 • 10 years ago

This is the associated mochitest. Very much a work in progress, I just wanted to have a working test.

I yet have to add:

 - Checking the storage if it is the *correct* URL, not just verifying the length
 - Making sure that it does not sessionstore too fast. Since interval pref changes reset the sessionstore timer (they call `runDelayed(0)`), this ought to be easy.
 - I also have to split it into multiple add_task() calls.


I also can switch over to chaining callbacks (I tried that initially and it works), but I prefer `yield`ing things, even if it makes me feel as if I'm writing Python.

Just want some feedback at this stage, to make sure I'm going in the right direction.

	Tim Taubert [:ttaubert] (inactive)
	Comment 64 • 10 years ago

(In reply to Manish Goregaokar [:manishearth] from comment #61)
> (In reply to Tim Taubert [:ttaubert] from comment #51)
> > Comment on attachment 806030 [details] [diff] [review]
> > Having a fake battery pref doesn't sound like a great idea to me. I think we
> > should take a look at the EmulatorBattery [1] and see if we can write a
> > couple of marionette tests for this? There's a couple of tests here [2] that
> > make use of this.
> 
> I just had a look at this; it seems like EmulatorBattery is a marionette
> thing, and mach is telling me that marionette tests can only be run in a b2g
> context. The MDN page makes it look like a FxOS thing too.

Marionette can totally run on desktop. Here's a log from a recent fx-team push:

https://tbpl.mozilla.org/php/getParsedLog.php?id=34761634&tree=Fx-Team

> I guess we'll have to stick with a plain mochitest, then. Or is there a way
> to run marionette for desktop Fx?

We are also working on bug 923606 to have additional Marionette tests covering sessionstore's startup and shutdown paths.

	Tim Taubert [:ttaubert] (inactive)
	Comment 65 • 10 years ago

The whiteboard is lying, sorry.

	Manish Goregaokar [:manishearth]
	Comment 66 • 10 years ago

(In reply to Tim Taubert [:ttaubert] from comment #64)

> Marionette can totally run on desktop. Here's a log from a recent fx-team
> push:
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=34761634&tree=Fx-Team

(Not sure what I need to look for in the log)

So how would I go about running a marionette test? Like I said, mach tells me that it can't be run without a b2g context.

	Tim Taubert [:ttaubert] (inactive)
	Comment 67 • 10 years ago

(In reply to Manish Goregaokar [:manishearth] from comment #66)
> (Not sure what I need to look for in the log)

Nothing, just wanted to show that we are running Marionette tests on desktop.

> So how would I go about running a marionette test? Like I said, mach tells
> me that it can't be run without a b2g context.

|./mach marionette-test| works for me locally. You need to build with |ENABLE_MARIONETTE=1| in your .mozconfig and I think there might have been other steps involved in getting Marionette to run locally [1].

[1] https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette

	Manish Goregaokar [:manishearth]
	Comment 68 • 10 years ago

Ah, I was looking at some of the Marionette Javascript tests (some of which are FxOS only[1]).

I'll rebuild and see what I can do.

 [1]: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/Python_Marionette

	David Teller [:Yoric] - still alive but not very active
	Comment 69 • 10 years ago

Comment on attachment 8376776 [details] [diff] [review]
Mochitest (WIP)

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

Missing file.

	Manish Goregaokar [:manishearth]
	Comment 70 • 10 years ago

Whoops, forgot to hg add.

Anyway, we're probably switching to Marionette on this, so we can abandon work on the mochitest, I guess.

	David Teller [:Yoric] - still alive but not very active
	Comment 71 • 10 years ago

Comment on attachment 8377117 [details] [diff] [review]
Mochitest (WIP)

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

That looks ok, but I believe that ttaubert has more precise ideas than me on how to run tests for this bug, so you should see with him.

::: browser/components/sessionstore/test/browser_506975_sessionStorage_battery.js
@@ +9,5 @@
> +var tab;
> +
> +/**
> + * This test ensures that setting, modifying and restoring sessionStorage data
> + * works as expected. aaaa

That comment is not very useful.

@@ +18,5 @@
> +  let timer = Components.classes["@mozilla.org/timer;1"].createInstance(Components.interfaces.nsITimer);
> +  timer.initWithCallback(function(){deferred.resolve()}, time, Components.interfaces.nsITimer.TYPE_ONE_SHOT); 
> +  return deferred.promise;
> +}
> +add_task(function(){

function*, these days

@@ +21,5 @@
> +}
> +add_task(function(){
> +  Services.prefs.setIntPref("browser.sessionstore.interval_battery", 1000)
> +  Services.prefs.setIntPref("browser.sessionstore.interval", 60000)
> +  Services.prefs.setIntPref("browser.sessionstore.debug.fake_battery", 2)

Please explain what you're doing with these prefs and why you're doing it.

@@ +22,5 @@
> +add_task(function(){
> +  Services.prefs.setIntPref("browser.sessionstore.interval_battery", 1000)
> +  Services.prefs.setIntPref("browser.sessionstore.interval", 60000)
> +  Services.prefs.setIntPref("browser.sessionstore.debug.fake_battery", 2)
> +  tab = gBrowser.addTab(URL+Math.random());

"let tab"

@@ +24,5 @@
> +  Services.prefs.setIntPref("browser.sessionstore.interval", 60000)
> +  Services.prefs.setIntPref("browser.sessionstore.debug.fake_battery", 2)
> +  tab = gBrowser.addTab(URL+Math.random());
> +  let browser = tab.linkedBrowser;
> +  yield promiseWait(1500);

Please document why you're waiting 1500ms.

@@ +26,5 @@
> +  tab = gBrowser.addTab(URL+Math.random());
> +  let browser = tab.linkedBrowser;
> +  yield promiseWait(1500);
> +  let storage = JSON.parse(ss.getTabState(tab));
> +  is(storage.entries.length,1,"sessionStorage correctly saved for non-charging battery state");

Please check that the URL is the random URL you computed above.

	Manish Goregaokar [:manishearth]
	Comment 72 • 10 years ago

Yep, was going to include those changes but I'll first try and see if the marionette test was possible.  Will finish work on that soon.

	Manish Goregaokar [:manishearth]
	Comment 73 • 10 years ago

So I was trying to duplicate the battery test[1]. 

While testing it, it turns out that runEmulatorCommand, the method the test uses to interact with the battery, doesn't seem to work.

I get the following error:

Traceback (most recent call last):
  File "/home/muon/Desktop/Programming/Mozilla/build-central/testing/marionette/client/marionette/marionette_test.py", line 163, in run
    testMethod()
  File "/home/muon/Desktop/Programming/Mozilla/build-central/testing/marionette/client/marionette/marionette_test.py", line 490, in runTest
    filename=os.path.basename(self.jsFile))
  File "/home/muon/Desktop/Programming/Mozilla/build-central/testing/marionette/client/marionette/marionette.py", line 1032, in execute_js_script
    line=None)
  File "/home/muon/Desktop/Programming/Mozilla/build-central/testing/marionette/client/marionette/marionette.py", line 609, in _send_message
    response = self._handle_emulator_cmd(response)
  File "/home/muon/Desktop/Programming/Mozilla/build-central/testing/marionette/client/marionette/marionette.py", line 619, in _handle_emulator_cmd
    "No emulator in this test to run command against")
MarionetteException: MarionetteException: No emulator in this test to run command against

Since the Marionette docs for runEmulatorCommand just link to the Android emulator page[2], I suspect that this particular set of tests is run only for Fennec (given that the TEST-UNEXPECTED-FAIL doesn't trigger on try builds, but it does trigger when I explicitly run the test locally).

So I'm not sure if the battery emulator thing is accessible via the desktop firefox marionette API.

Any way we can work around this?

An alternate solution is to expose hooks for battery testing to mochitest. Should be doable with observers.

 [1]: http://hg.mozilla.org/mozilla-central/file/45128af17739/dom/battery/test/test_battery_basics.html
 [2]: http://developer.android.com/tools/devices/emulator.html#console

	Tim Taubert [:ttaubert] (inactive)
	Comment 74 • 10 years ago

Hmmm, it looks like we're using Android to test and emulate the battery on B2G. It does make sense in hindsight but I was falsely assuming we had the same capabilities for desktop. I think we should file a bug that would implement a fake battery for desktop and file another dependent bug that would then add sessionstore tests.

If we really want unit-test level coverage here we could as well add a Battery.jsm module (that maybe doesn't even rely on the hiddenWindow) that we can feed with mock data in test mode. We might have other code in the future that wants to change behavior based on battery state.

How does the second solution sound? I think that's quite feasible and wouldn't require finding someone to implement a fake battery for us. If we go that route we should file another bug that blocks this one and add some unit tests for the new module as well.

	David Teller [:Yoric] - still alive but not very active
	Comment 75 • 10 years ago

I haven't seen any activity on this bug in a while.
Manish, are you still working on it?

	Manish Goregaokar [:manishearth]
	Comment 76 • 10 years ago

(In reply to David Rajchenbach Teller [:Yoric] from comment #75)
> I haven't seen any activity on this bug in a while.
> Manish, are you still working on it?

Yes, I am, this is currently blocked on bug 978857 (needs review)

	David Teller [:Yoric] - still alive but not very active
	Comment 77 • 10 years ago

So, now that bug 978857 has landed, I'm waiting for this eagerly :)

	Manish Goregaokar [:manishearth]
	Comment 80 • 10 years ago

Try push: https://tbpl.mozilla.org/?tree=Try&rev=15cc4d5d2cb7

I'm not sure, but sometimes the test fails if the browser doesn't have focus.

	Manish Goregaokar [:manishearth]
	Comment 81 • 10 years ago

Try passes, however one of my testing patches for Marionette wiggled its way into the queue. (I didn't ask try to run Marionette, so it still passes)

	David Teller [:Yoric] - still alive but not very active
	Comment 82 • 10 years ago

Comment on attachment 8453811 [details] [diff] [review]
Updated patch using Battery.jsm

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

Looks good to me, but I still have a few questions.

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +10,5 @@
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
>  
>  Cu.import("resource://gre/modules/Timer.jsm", this);
> +Cu.import("resource://gre/modules/Battery.jsm", this);

I believe that you can make this lazy, to avoid startup-time stampede.

@@ +26,5 @@
>    "resource:///modules/sessionstore/SessionFile.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "PrivateBrowsingUtils",
>    "resource://gre/modules/PrivateBrowsingUtils.jsm");
>  
> +// Minimal interval between two save operations (in milliseconds) when device is charging.

"plugged" sounds clearer than "charging"

@@ +36,5 @@
>      this.gInterval = Services.prefs.getIntPref(PREF);
>  
> +    if (isBatteryCharging()){
> +      // Cancel any pending runs and call runDelayed()
> +      // to apply the newly configured interval.

Why only if the battery is charging?

	David Teller [:Yoric] - still alive but not very active
	Comment 83 • 10 years ago

Comment on attachment 8453813 [details] [diff] [review]
Mochitest

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

The test still needs some work.

::: browser/components/sessionstore/test/browser_506975_sessionStorage_battery.js
@@ +5,5 @@
> +
> +
> +const URL = "http://mochi.test:8888?rand="
> +let imported = Components.utils.import("resource://gre/modules/Battery.jsm", this);
> +var tab;

s/var/tab/g
Also, this doesn't need to be global.

@@ +9,5 @@
> +var tab;
> +
> +/**
> + * This test ensures that setting, modifying and restoring sessionStorage data
> + * works as expected. aaaa

?

@@ +11,5 @@
> +/**
> + * This test ensures that setting, modifying and restoring sessionStorage data
> + * works as expected. aaaa
> + */
> + 

Nit: Whitespace

@@ +15,5 @@
> + 
> +function promiseWait(time){
> +  let deferred = Promise.defer();
> +  let timer = Components.classes["@mozilla.org/timer;1"].createInstance(Components.interfaces.nsITimer);
> +  timer.initWithCallback(function(){deferred.resolve()}, time, Components.interfaces.nsITimer.TYPE_ONE_SHOT); 

Nit: whitespace.

@@ +16,5 @@
> +function promiseWait(time){
> +  let deferred = Promise.defer();
> +  let timer = Components.classes["@mozilla.org/timer;1"].createInstance(Components.interfaces.nsITimer);
> +  timer.initWithCallback(function(){deferred.resolve()}, time, Components.interfaces.nsITimer.TYPE_ONE_SHOT); 
> +  return deferred.promise;

Let's make this new-style Promise.
Instead of `Promise.defer()`, use `new Promise(resolve => ...)`.

@@ +17,5 @@
> +  let deferred = Promise.defer();
> +  let timer = Components.classes["@mozilla.org/timer;1"].createInstance(Components.interfaces.nsITimer);
> +  timer.initWithCallback(function(){deferred.resolve()}, time, Components.interfaces.nsITimer.TYPE_ONE_SHOT); 
> +  return deferred.promise;
> +}

Nit: add a newline, please.

@@ +18,5 @@
> +  let timer = Components.classes["@mozilla.org/timer;1"].createInstance(Components.interfaces.nsITimer);
> +  timer.initWithCallback(function(){deferred.resolve()}, time, Components.interfaces.nsITimer.TYPE_ONE_SHOT); 
> +  return deferred.promise;
> +}
> +add_task(function(){

function*

@@ +19,5 @@
> +  timer.initWithCallback(function(){deferred.resolve()}, time, Components.interfaces.nsITimer.TYPE_ONE_SHOT); 
> +  return deferred.promise;
> +}
> +add_task(function(){
> +  Services.prefs.setIntPref("browser.sessionstore.interval_battery", 1000)

Let's make it `100`, no need to wait more.

@@ +21,5 @@
> +}
> +add_task(function(){
> +  Services.prefs.setIntPref("browser.sessionstore.interval_battery", 1000)
> +  Services.prefs.setIntPref("browser.sessionstore.interval", 60000)
> +  imported.Debugging.fake = true;

You should set and unset the `fake` flag in a `function* init()` and a `function* cleanup`.

@@ +27,5 @@
> +  Battery.chargingTime = Infinity;
> +  Battery.dischargingTime = 50;
> +  tab = gBrowser.addTab(URL+Math.random());
> +  yield promiseWait(1500);
> +  let storage = JSON.parse(ss.getTabState(tab));

No, `ss.getTabState` will not prove that anything was saved. For this, you need to read and parse `SessionFile.Paths.recovery`.

@@ +28,5 @@
> +  Battery.dischargingTime = 50;
> +  tab = gBrowser.addTab(URL+Math.random());
> +  yield promiseWait(1500);
> +  let storage = JSON.parse(ss.getTabState(tab));
> +  is(storage.entries.length,1,"sessionStorage correctly saved for non-charging battery state");

You should check that the tab you just opened appears in the sessionstore.

@@ +42,5 @@
> +  let storage = JSON.parse(ss.getTabState(tab));
> +  is(storage.entries.length,1,"sessionStorage correctly saved for charging battery state");
> +  gBrowser.removeTab(tab);
> +  Services.prefs.setIntPref("browser.sessionstore.interval_battery", 60000)
> +  Services.prefs.setIntPref("browser.sessionstore.interval", 15000)

clearUserPref – this also goes in `cleanup`, btw.

	Manish Goregaokar [:manishearth]
	Comment 84 • 10 years ago

> @@ +36,5 @@
> >      this.gInterval = Services.prefs.getIntPref(PREF);
> >  
> > +    if (isBatteryCharging()){
> > +      // Cancel any pending runs and call runDelayed()
> > +      // to apply the newly configured interval.
> 
> Why only if the battery is charging?

Because if the battery is not charging, the other interval is being used so we don't need to update on a change of this interval.

	Manish Goregaokar [:manishearth]
	Comment 85 • 10 years ago

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo") from comment #83)
> Comment on attachment 8453813 [details] [diff] [review]
> Mochitest
> 
> Review of attachment 8453813 [details] [diff] [review]:
> -----------------------------------------------------------------

> @@ +16,5 @@
> > +function promiseWait(time){
> > +  let deferred = Promise.defer();
> > +  let timer = Components.classes["@mozilla.org/timer;1"].createInstance(Components.interfaces.nsITimer);
> > +  timer.initWithCallback(function(){deferred.resolve()}, time, Components.interfaces.nsITimer.TYPE_ONE_SHOT); 
> > +  return deferred.promise;
> 
> Let's make this new-style Promise.
> Instead of `Promise.defer()`, use `new Promise(resolve => ...)`.

How will I do this? The resolve handler seems to be set by the timer and called by the harness.


> No, `ss.getTabState` will not prove that anything was saved. For this, you
> need to read and parse `SessionFile.Paths.recovery`.

Would this code work?

function promiseRead() {
  let decoder = new TextDecoder();
  let promise = OS.File.read(SessionFile.Paths.recovery);
  promise = promise.then(
  function onSuccess(array) {
      return JSON.parse(decoder.decode(array));
    }
  );
  return promise
}

function inFile(url, parsedFile) {
  for (let win of parsedFile.windows) {
    for (let tab of win.tabs) {
      for (let entry of tab.entries) {
        if (entry.url == url) {
          return true;
        }
      }
    }
  }
  return false;
}

(and then yielding on promiseRead)

It seems to work but the entry is missing (maybe due to the small time period). There is an entry in ss though.

	David Teller [:Yoric] - still alive but not very active
	Comment 86 • 10 years ago

(In reply to Manish Goregaokar [:manishearth] from comment #85)
> > @@ +16,5 @@
> > > +function promiseWait(time){
> > > +  let deferred = Promise.defer();
> > > +  let timer = Components.classes["@mozilla.org/timer;1"].createInstance(Components.interfaces.nsITimer);
> > > +  timer.initWithCallback(function(){deferred.resolve()}, time, Components.interfaces.nsITimer.TYPE_ONE_SHOT); 
> > > +  return deferred.promise;
> > 
> > Let's make this new-style Promise.
> > Instead of `Promise.defer()`, use `new Promise(resolve => ...)`.
> 
> How will I do this? The resolve handler seems to be set by the timer and
> called by the harness.


function promiseWait(time) {
  return Promise(resolve => {
     let timer = ...
     timer.initWithCallback(resolve, ...);
  });
}

> 
> 
> > No, `ss.getTabState` will not prove that anything was saved. For this, you
> > need to read and parse `SessionFile.Paths.recovery`.
> 
> Would this code work?
> 
> function promiseRead() {
>   let decoder = new TextDecoder();
>   let promise = OS.File.read(SessionFile.Paths.recovery);
>   promise = promise.then(
>   function onSuccess(array) {
>       return JSON.parse(decoder.decode(array));
>     }
>   );
>   return promise
> }

You don't need the decoder. Just add argument `{encoding: "utf-8"}` to `OS.File.read`.
Also, if you want to read and parse, that can be a two-liner:

let string = yield OS.File.read(SessionFile.Paths.recovery, { encoding: "utf-8" });
let data = JSON.parse(string);
 
> function inFile(url, parsedFile) {
>   for (let win of parsedFile.windows) {
>     for (let tab of win.tabs) {
>       for (let entry of tab.entries) {
>         if (entry.url == url) {
>           return true;
>         }
>       }
>     }
>   }
>   return false;
> }

That would work. However, for this test, I would be satisfied with just `string.contains(url)`.

> It seems to work but the entry is missing (maybe due to the small time
> period). There is an entry in ss though.

Well, if the entry is missing, we have a problem :/

	David Teller [:Yoric] - still alive but not very active
	Comment 87 • 10 years ago

Comment on attachment 8454078 [details] [diff] [review]
Updated patch using Battery.jsm +nits

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

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +40,5 @@
> +      // Cancel any pending runs and call runDelayed()
> +      // to apply the newly configured interval.
> +      SessionSaverInternal.cancel();
> +      SessionSaverInternal.runDelayed(0);
> +    }

I believe that we should cancel/runDelayed regardless of whether the battery is charging or not.

	Tim Taubert [:ttaubert] (inactive)
	Comment 88 • 10 years ago

Comment on attachment 8454078 [details] [diff] [review]
Updated patch using Battery.jsm +nits

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

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +18,5 @@
>  XPCOMUtils.defineLazyModuleGetter(this, "console",
>    "resource://gre/modules/devtools/Console.jsm");
> +XPCOMUtils.defineLazyModuleGetter(this, "Battery",
> +  "resource:///modules/sessionstore/Battery.jsm");
> +  XPCOMUtils.defineLazyModuleGetter(this, "PrivacyFilter",

Nit: wrong indentation

@@ +37,5 @@
>      this.gInterval = Services.prefs.getIntPref(PREF);
>  
> +    if (isBatteryCharging()){
> +      // Cancel any pending runs and call runDelayed()
> +      // to apply the newly configured interval.

Can we combine the two into one observer and just call cancel() and runDelayed(0) in there then? I wouldn't event mind getting rid of the lazy getters and reading the prefs in runDelayed() every time, that would probably make the code easier and shouldn't hurt performance.

@@ +69,5 @@
> +// Check if battery is charging
> +function isBatteryCharging(){
> +   return isFinite(Battery.chargingTime);
> +   // chargingTime is Infinity when discharging
> +   // or when the remaining time cannot be determined

Shouldn't we just use |Battery.charging| here? AFAICT from the spec that should be true whenever we're plugged in, even if the battery is full. Having this one line in a separate function doesn't seem worthwhile if we can make this just return .charged.

@@ +76,5 @@
> +// Get the current session store interval based on battery status
> +function getSavingInterval(){
> +  if (isBatteryCharging()){
> +    return gInterval;
> +  }

Can you please move this code into runDelayed()? Seems pointless to have a separate function for what would be a one-liner.

	Manish Goregaokar [:manishearth]
	Comment 90 • 10 years ago

Mochitest runs now!

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=124ff78a184b

	Tim Taubert [:ttaubert] (inactive)
	Comment 91 • 10 years ago

Comment on attachment 8470063 [details] [diff] [review]
Updated mochitest

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

::: browser/components/sessionstore/test/browser.ini
@@ +126,5 @@
>  skip-if = true
>  [browser_491577.js]
>  [browser_495495.js]
>  [browser_500328.js]
> +[browser_506975_sessionStorage_battery.js]

We could leave the bug number of that file name I think.

::: browser/components/sessionstore/test/browser_506975_sessionStorage_battery.js
@@ +4,5 @@
> +"use strict";
> +
> +
> +const URL = "http://mochi.test:8888?rand=";
> +let imported = Components.utils.import("resource://gre/modules/Battery.jsm", this);

let {Battery, Debug} = Cu.import("resource://gre/modules/Battery.jsm", {});

@@ +8,5 @@
> +let imported = Components.utils.import("resource://gre/modules/Battery.jsm", this);
> +
> +/**
> + * This test ensures that setting, modifying and restoring sessionStorage data
> + * works as expected.

The comment seems wrong...

@@ +14,5 @@
> +
> +function promiseWait(time) {
> +  return new Promise(resolve => {
> +    let timer = Components.classes["@mozilla.org/timer;1"].createInstance(Components.interfaces.nsITimer);
> +    timer.initWithCallback(resolve, time, Components.interfaces.nsITimer.TYPE_ONE_SHOT); 

This should use Timer.jsm and setTimeout().

@@ +20,5 @@
> +}
> +
> +
> +add_task(function* init() {
> +  imported.Debugging.fake = true;

Should be set to false by registerCleanupFunction() in case we break or time out.

@@ +25,5 @@
> +  Battery.charging = false;
> +  Battery.chargingTime = Infinity;
> +  Battery.dischargingTime = 50;
> +  Services.prefs.setIntPref("browser.sessionstore.interval_battery", 100)
> +  Services.prefs.setIntPref("browser.sessionstore.interval", 60000)

Nit: missing semicolons. Also please put the pref names in constants at the top and use that. We should also do for both:

registerCleanupFunction(() => Services.prefs.clearUserPref(PREF));

@@ +31,5 @@
> +
> +add_task(function* runtest() {
> +  let url = URL + Math.random();
> +  let tab = gBrowser.addTab(url);
> +  yield promiseWait(1500);

1500 should be a properly named constant at the top.

@@ +33,5 @@
> +  let url = URL + Math.random();
> +  let tab = gBrowser.addTab(url);
> +  yield promiseWait(1500);
> +  let storage = JSON.parse(ss.getTabState(tab));
> +  is(storage.entries.length,1,"sessionStorage correctly saved for non-charging battery state");

What does the .getTabState() call check here? The interval on battery doesn't affect that, does it?

@@ +34,5 @@
> +  let tab = gBrowser.addTab(url);
> +  yield promiseWait(1500);
> +  let storage = JSON.parse(ss.getTabState(tab));
> +  is(storage.entries.length,1,"sessionStorage correctly saved for non-charging battery state");
> +  let string = yield OS.File.read(SessionFile.Paths.recovery, { encoding: "utf-8" });

let state = yield promiseRecoveryFileContents();

@@ +35,5 @@
> +  yield promiseWait(1500);
> +  let storage = JSON.parse(ss.getTabState(tab));
> +  is(storage.entries.length,1,"sessionStorage correctly saved for non-charging battery state");
> +  let string = yield OS.File.read(SessionFile.Paths.recovery, { encoding: "utf-8" });
> +  isnot(string.indexOf(url), -1, "Sessionstore correctly saved to disk for non-charging battery state")

ok(state.contains(url), " ... ");

@@ +42,5 @@
> +  Battery.charging = true;
> +  Battery.chargingTime = 100;
> +  Battery.dischargingTime = Infinity;
> +  Services.prefs.setIntPref("browser.sessionstore.interval_battery", 60000)
> +  Services.prefs.setIntPref("browser.sessionstore.interval", 100)

Nit: newline here please to make it a little easier to read

@@ +43,5 @@
> +  Battery.chargingTime = 100;
> +  Battery.dischargingTime = Infinity;
> +  Services.prefs.setIntPref("browser.sessionstore.interval_battery", 60000)
> +  Services.prefs.setIntPref("browser.sessionstore.interval", 100)
> +  let url = URL + Math.random();

url = URL + Math.random();

@@ +47,5 @@
> +  let url = URL + Math.random();
> +  tab = gBrowser.addTab(url);
> +  yield promiseWait(1500);
> +  let storage = JSON.parse(ss.getTabState(tab));
> +  is(storage.entries.length,1,"sessionStorage correctly saved for charging battery state");

Same question about .getTabState() here.

@@ +49,5 @@
> +  yield promiseWait(1500);
> +  let storage = JSON.parse(ss.getTabState(tab));
> +  is(storage.entries.length,1,"sessionStorage correctly saved for charging battery state");
> +  let string = yield OS.File.read(SessionFile.Paths.recovery, { encoding: "utf-8" });
> +  isnot(string.indexOf(url), -1, "Sessionstore correctly saved to disk for charging battery state")

Same as above.

	Tim Taubert [:ttaubert] (inactive)
	Comment 92 • 10 years ago

Comment on attachment 8470059 [details] [diff] [review]
Main patch with nits fixed and Battery.jsm usage

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

::: browser/components/sessionstore/src/SessionSaver.jsm
@@ +27,5 @@
>    "resource:///modules/sessionstore/SessionFile.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "PrivateBrowsingUtils",
>    "resource://gre/modules/PrivateBrowsingUtils.jsm");
>  
> +function observeBatteryPref(obj, pref, property) {

observeBatterPref() is a bit misleading... we observe save interval prefs here. Should pick a better name.

@@ +145,5 @@
>        return;
>      }
>  
>      // Interval until the next disk operation is allowed.
> +    delay = Math.max(this._lastSaveTime + (Battery.charging ? gInterval : gIntervalBattery) - Date.now(), delay, 0);

let interval = Battery.charging ? gInterval : gIntervalBattery;
delay = Math.max(this._lastSaveTime + interval - Date.now(), delay, 0);

	David Teller [:Yoric] - still alive but not very active
	Comment 93 • 10 years ago

Any news?

	Manish Goregaokar [:manishearth]
	Comment 94 • 10 years ago

Just need to rebase the patch. Will do in a moment

	Manish Goregaokar [:manishearth]
	Comment 96 • 10 years ago

Regarding ss.getTabState(), this is just to get the sessionstore state for that particular tab. I could use getWindowState instead if you want.

	Tim Taubert [:ttaubert] (inactive)
	Comment 97 • 10 years ago

Comment on attachment 8506970 [details] [diff] [review]
Main patch

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

::: browser/components/sessionstore/SessionSaver.jsm
@@ +43,3 @@
>  
> +observeSaveIntervalPref(this, "browser.sessionstore.interval", "gInterval")
> +observeSaveIntervalPref(this, "browser.sessionstore.interval_battery", "gIntervalBattery")

nit: Please add missing semicolons.

	Tim Taubert [:ttaubert] (inactive)
	Comment 99 • 10 years ago

Comment on attachment 8506975 [details] [diff] [review]
Main patch

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

Should have mentioned you don't need to ask for review again :) Thanks!

	Tim Taubert [:ttaubert] (inactive)
	Comment 100 • 10 years ago

Comment on attachment 8506972 [details] [diff] [review]
Mochitest

Did you address any of the review comments? Looks like the old test?

	Manish Goregaokar [:manishearth]
	Comment 101 • 10 years ago

I didn't rename the test, I did everything else. Renamed and reuploaded.

	Tim Taubert [:ttaubert] (inactive)
	Comment 102 • 10 years ago

Comment on attachment 8506981 [details] [diff] [review]
Mochitest

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

::: browser/components/sessionstore/test/browser_sesionStorage_battery.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */

That file name is missing an "s" :)

@@ +9,5 @@
> +const PREF_CHARGING = "browser.sessionstore.interval";
> +const WAIT = 2000;
> +const LARGE_DURATION = 60000;
> +const SMALL_DURATION = 100;
> +let imported = Components.utils.import("resource://gre/modules/Battery.jsm", this);

let {Battery, Debug} = Cu.import("resource://gre/modules/Battery.jsm", {});

@@ +39,5 @@
> +  let tab = gBrowser.addTab(url);
> +  yield promiseWait(WAIT);
> +  let storage = JSON.parse(ss.getTabState(tab));
> +  is(storage.entries.length, 1, "sessionStorage correctly saved for non-charging battery state");
> +  let state = yield promiseRecoveryFileContents();

So... totally my fault but promiseRecoveryFileContents() does actually force saving state :(

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/head.js#276

Can you please switch this back to the OS.File call?

@@ +53,5 @@
> +  url = URL + Math.random();
> +  tab = gBrowser.addTab(url);
> +  yield promiseWait(WAIT);
> +  storage = JSON.parse(ss.getTabState(tab));
> +  is(storage.entries.length,1,"sessionStorage correctly saved for charging battery state");

Nit:please,leave,some,space,after,commas

@@ +54,5 @@
> +  tab = gBrowser.addTab(url);
> +  yield promiseWait(WAIT);
> +  storage = JSON.parse(ss.getTabState(tab));
> +  is(storage.entries.length,1,"sessionStorage correctly saved for charging battery state");
> +  state = yield promiseRecoveryFileContents();

Same here with OS.File.

@@ +61,5 @@
> +});
> +
> +add_task(function* cleanup() {
> +  Services.prefs.clearUserPref("browser.sessionstore.interval_battery")
> +  Services.prefs.clearUserPref("browser.sessionstore.interval")

Nit: missing semicolons.

@@ +62,5 @@
> +
> +add_task(function* cleanup() {
> +  Services.prefs.clearUserPref("browser.sessionstore.interval_battery")
> +  Services.prefs.clearUserPref("browser.sessionstore.interval")
> +  imported.Debugging.fake = false;

Should move this to init() and call it with registerCleanupFunction(). So even when the test times out we clean up properly.

	Manish Goregaokar [:manishearth]
	Comment 103 • 10 years ago

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=1982005fbef0

	Tim Taubert [:ttaubert] (inactive)
	Comment 104 • 10 years ago

Comment on attachment 8507005 [details] [diff] [review]
Mochitest

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

Excellent, thank you.

	Carsten Book [:Tomcat]
	Comment 105 • 10 years ago

https://hg.mozilla.org/integration/fx-team/rev/be4c66d5dc65
https://hg.mozilla.org/integration/fx-team/rev/76caa4994120

	Carsten Book [:Tomcat]
	Comment 106 • 10 years ago

sorry had to back this out for bc1 test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=944887&repo=fx-team on all platforms

	Manish Goregaokar [:manishearth]
	Comment 107 • 10 years ago

I think I'd uploaded an outdated patch, refreshed and rebased: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e44d7310565e

	David Teller [:Yoric] - still alive but not very active
	Comment 108 • 9 years ago

Manish, what's the status of this?

	Manish Goregaokar [:manishearth]
	Comment 109 • 9 years ago

I'm not sure. It still randomly fails on try, and I can't tell why that happens (not able to repro locally).

Feel free to take it over.