Consider consolidating navigate() and reload() more

[annevk]

It seems that if you throw earlier for state, you could put some of the other error handling in the shared steps. Potentially that could even be done for state, but that gets a little awkward it seems given the reload() setup.

[domenic]

The reason I've put state serialization last is that it's very observable, e.g. it triggers getters. So, it seems slightly better to delay that until we're certain we're not in a different error case.

In our implementation we indeed consolidated the two error cases (not fully active and unload counter) into a "perform shared navigation checks" method. But in the spec you don't gain as much... you basically replace the two checks with

1. Let maybeErrorResult be the result of [performing shared navigation checks] given this.
2. If maybeErrorResult is not null, then return it.

Do you think that's still a worthwhile cleanup?

[annevk]

You raise a good point. I think that state serialization can cause a document to become inactive, e.g., by removing it's embedder element. I think that would argue for performing state serialization first, no?

[domenic]

Oh, wow, good point. Yes, I guess we should move it first. The alternative is performing the fully active check (and the unload check??) both before and after, but that's weird.

I guess we can see the serialization as an extension of the Web IDL argument conversion, so in that sense doing it as early as possible is reasonable.

Will fix in implementation/tests/spec.

[annevk]

Nit: the state checks can also be moved to the shared algorithm now.