Rework key generator algorithm

[inexorabletash]

Address the spec gaps pointed out in #147

This pins the key generator to the maximum value (2^53). The logic around incrementing/updating the generator is made algorithmic rather than being duplicated between the object store storage operation steps and the prose definition of the key generator.

[inexorabletash]

@aliams can you review?

[brettz9]

From my perspective, it looks good, but I have a few concerns:

1. Nit: I think in the context of spec language, "possibly update" sounds better than "maybe update".

2. I believe in the algorithm to "maybe update the key generator":

5. If |value| is greater than |generator|'s [=key generator/current number=],
   then set |generator|'s [=key generator/current number=] to |value|.

should be something like the following (unless the former behavior is being changed):

5. If |value| is greater than |generator|'s [=key generator/current number=],
   then set |generator|'s [=key generator/current number=] to |value| (rounded up to the nearest integer if it has a decimal value).

(Note that it is safe to use this language since you set value before this step to a maximum of 2^53.)

3. In the algorithm to "generate a key", with these steps:

3. If |key| is greater than 2⁵³ (9007199254740992), then return failure.

4. Increase |generator|'s [=key generator/current number=] by 1.

...an implementation note should perhaps be added to mention that if key is already at 9007199254740992, then incrementing by one in the likes of ECMAScript will still lead to 9007199254740992 and thus failure will never be reached (and a value may be unintentionally overwritten). An alternative to adding an implementation note (and I think a safer one) would be changing the max instead to Number.MAX_SAFE_INTEGER (i.e., 2^53 - 1) here and elsewhere in the spec.

[inexorabletash]

Thanks @brettz9 !

Yes, I missed both the floor() step and incrementing. :(

I've hopefully corrected the steps, and made a note about where the limit comes from, although I didn't go into "what if..." detail.

Re: altering the maximum:

We already have tests around the boundary condition and that the generator may produce 9007199254740992 so I don't think we want to reduce that. In a pure JS implementation I scraped together to sanity check my logic, I used e.g.:

    this.current += 1;
    if (this.current === key)
      this.current = Infinity;

and

    if (value >= this.current) {
      this.current = value + 1;
      if (this.current === value)
        this.current = Infinity;
    }

An implementation using doubles could do that as well. I expect most implementations use an int64 or uint64 internally and can use the increment beyond 9007199254740992 safely.

[brettz9]

Actually, you don't want to add the incrementing step there. Those steps you added are for setting the key generator when the key is not undefined. Apologies, I had forgotten even this step should include incrementing.

(Maybe it would be more clear to say "possibly update the current number" or "possibly update the key generator current number" instead of "possibly update the key generator" since the latter has stronger connotations of incrementing.)

[inexorabletash]

...I had forgotten even this step should include incrementing.

As had I. :)

[brettz9]

I see one other issue...

The current number definition states the following:

A [=key generator=] has a current number. The
[=key generator/current number=] is always a positive integer less
than or equal to 2⁵³ (9007199254740992).

However the steps to "generate a key" and "possibly update the key generator" both allow for a current number to be set to a number one higher than 9007199254740992 as does the following:

When the [=key generator/current number=] of a key generator reaches above the
value 2⁵³ (9007199254740992) any attempts to use the
key generator to generate a new [=/key=] will result in a
"{{ConstraintError}}" {{DOMException}}.

I believe the current number definition ought to be reworded, e.g., like this:

A [=key generator=] has a current number. The
[=key generator/current number=] is always a positive integer less
than or equal to 2⁵³ + 1 (9007199254740993).

[brettz9]

Perhaps this text:

When the [=key generator/current number=] of a key generator reaches above the
value 2⁵³ (9007199254740992) any attempts to use the
key generator to generate a new [=/key=] will result in a
"{{ConstraintError}}" {{DOMException}}.

ought to be clarified as well to make clear that reaching above that value for the first time won't return the exception:

When the [=key generator/current number=] of a key generator reaches above the
value 2⁵³ (9007199254740992) any subsequent attempts to use the
key generator to generate a new [=/key=] will result in a
"{{ConstraintError}}" {{DOMException}}.

[inexorabletash]

Nice, thanks. I'll incorporate those suggestions.

[inexorabletash]

Tests coming in web-platform-tests/wpt#5065