Fix #2: Clarify there is one battery promise per Document

[anssiko]

Review appreciated from @domenic @mounirlamouri et al.

[mounirlamouri]

Shouldn't we include workers too?

[anssiko]

@mounirlamouri I'd consider expose to workers a v2 feature (feel free to open a separate issue for that).

[mounirlamouri]

lgtm then

[mounirlamouri]

(I don't have write access so I can't merge.)

[domenic]

This isn't quite sufficient. When creating the BatteryManager object, you need to be clear what realm it's being created in.

[domenic]

For example, if I run code in window3 that does window2.navigator.getBattery.call(window1.navigator), which of the three windows' realms is the BatteryManager created in.

[anssiko]

@mounirlamouri You have write access now (sorry, my bad).

@domenic To be consistent with the rest of the platform, with which realm you'd suggest we associated the BatteryManager with in scenarios that involve nested browsing contexts?

[domenic]

I'm not sure. @bzbarsky may have an opinion. To rephrase the question, given code in window3 that does window2.navigator.getBattery.call(window1.navigator), which returns a promise to be fulfilled with a BatteryManager: which of the three windows' Realms is the BatteryManager created in?

For that matter, which of the three realms is the Promise created in? :-/

[domenic]

Ah, I found the thread I was searching for where @bzbarsky expressed an opinion previously: https://github.com/w3ctag/promises-guide/issues/46#issuecomment-171397028

So, now that I remember his reasoning, window1 is correct. Un-summoning @bzbarsky.

After a meeting I have to dash to I'll get back with some proposed spec text.

[domenic]

Suggested:

• Instead of each Document having a battery promise, make it each Navigator object. They are equivalent, but it will be easier to write the following spec text.
• Make the following updates to the getBattery() method definition:
  • Please number the steps, instead of using bullets.
  • Replace all references to "battery promise" with "this Navigator object's battery promise".
  • Replace step 2 with "Otherwise, set this Navigator object's battery promise to a newly created Promise, created in the Realm of this Navigator object."
  • Replace step 4 with "create a new BatteryManager object in the Realm of this Navigator object, and let battery be that object."

[bzbarsky]

It's not clear to me that Document and Navigator are equivalent here. Aren't Navigator objects per-Window, not per-Document?

Apart from that, I agree that using the Navigator object's realm for the Promise and the BatteryManager seems like the right thing.

[anssiko]

Would anyone object us merging this PR and use that for the Rec Track advancement, while we in parallel incubate @domenic's Realm patch https://github.com/w3c/battery/pull/3#issuecomment-209625181 in the Editor's Draft and after baked in release a Proposed (Edited) Recommendation that integrates these further clarifications?

I believe that'd minimize the process-related back-and-forths and allow us to make forward progress on all the fronts.

[domenic]

No, this PR is not sufficient for REC.

[anssiko]

Ok, I was not clear whether @bzbarsky was actually suggesting further changes to @domenic's patch https://github.com/w3c/battery/pull/3#issuecomment-209625181. If the patch is good as is, I'll update this PR.

[domenic]

Boris is right that Navigator and Document are not equivalent. In particular, given the navigation from the original about:blank document, Navigator will stay the same, while Document will vary. And when you use document.open(), Document will stay the same, while Navigator will vary.

That said, I think using Navigator is better. In those situations, I would expect the same promise to be returned for about:blank navigation, and a new promise to be returned after document.open(). But we should ask @mounirlamouri what he thinks as an implementer since he says in Blink uses Document.

If @mounirlamouri prefers Document, then amend my above to replace "this Navigator object's battery promise" with "this Navigator object's relevant settings object's global object's newest Document object's battery promise"

[anssiko]

@domenic @mounirlamouri Updated the PR, please take a look, thanks!

[domenic]

LGTM with nits. But definitely needs @mounirlamouri's input given that it's different than Blink's implementation.

[anssiko]

@domenic Much thanks for the review. I fixed your nits with f7c9a02.

@mounirlamouri Please let us know if you're happy about the prose in this PR or whether to go the other way described in https://github.com/w3c/battery/pull/3#issuecomment-210099639

[mounirlamouri]

lgtm. Actually, Blink has one BatteryManager per Navigator object. I misread the code. FTR, I did not implement this in Blink. I did implement most of it in Gecko though ;)

[anssiko]

Thanks!

Ping @dontcallmedom re possible process implications.

[dontcallmedom]

thanks; it will be easier to assess the process implications once we have an updated test suite and clarity as to whether the spec change does indeed match the existing browser behavior