-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Spec rewrite: Update to new separate document/app API #57
Conversation
Putting up my draft of the spec rewrite. I haven't fully read over it for sanity, but I have to go on a 5-day trip so I'll just let you guys review it if you want. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM. There are a few series of comments where I've basically resolved my own questions, but I've left (most of) them in, as the perspective of a non-spec author trying to understand a spec 😆 😢
</p> | ||
<p> | ||
If the application's badge is <dfn>nothing</dfn>, the badge is said to be | ||
If a <a>badge</a> is <a>nothing</a>, it is said to be | ||
"<dfn>cleared</dfn>". Otherwise, it is said to be "<dfn>set</dfn>". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With regard to my previous comment about definitions, this is a useful definition of cleared
and set
, there is a meaning to cleared:
badgeValue
is an element of { nothing }
and set:
badgeValue
is an element of { flag, ...positiveIntegers }
which I can use and understand
(excuse my made up set notation)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what you're asking for here. Do you really want me to switch to set notation here?
This para is where we define "set" and "cleared" terms. I think it's quite understandable to say "If badge is nothing, it is cleared. Otherwise it is set."
Note that this isn't an appropriate place to define "nothing" (see above). This is essentially defining some new terms in terms of the already-defined terms.
More Haskell:
data SetOrCleared = Set | Cleared
badgeSet Nothing = Cleared
badgeSet _ = Set
Could we just rewrite all web specs in Haskell?
index.html
Outdated
example, on top of the document's icon or near its title). | ||
</p> | ||
<p class="issue"> | ||
Perhaps the badge should be associated with a <a data-cite= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would the implications of this be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #59 to discuss this.
index.html
Outdated
The user agent is not expected to display a badge associated with a | ||
document that is not a <a data-cite= | ||
"html#top-level-browsing-context">top-level browsing context</a>, | ||
although it is allowed to. A user agent does not need to store the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would this work? What might it look like?
I don't think we need to mention that user agents don't need to store the value if they don't use it: That's just an implementation detail. Even if they do use it, they don't have to store the value (except, maybe, as a generated image).
That said, I think this note can probably be removed. Essentially we're saying
You don't have to display a badge on a top level browsing context. You also aren't expected to display it on non-top level browsing contexts. But you can if you like. If you don't, you don't have to do anything.
I think we can just say that you can display badges for whatever browsing contexts you like, as long as the badge is in a place associated with that browsing context (i.e. don't display a badge on a top level context if it was set from a non-top level context).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't actually saying "You don't have to display a badge on a top level browsing context." Because if a browser doesn't display badges anywhere, they are expected (via a SHOULD below) not to expose the API at all. So there's an expectation that if you expose this API, you are displaying badges on top-level browsing contexts.
However, we want to call out that you don't have to (and in fact it's very unlikely that you will) display badges for non-top-level browsing contexts, even if you expose the method.
There is no normative text around top-level vs non-top-level browsing contexts here (perhaps we could make this a SHOULD NOT, though that seems a little strong). So I think it's important to call this out, so browser manufacturers aren't asking, "wait, what about non-top-level browsing contexts?" That's precisely what non-normative text is for --- for things that don't technically have to be spelled out but that we still want to call out our intentions.
</p> | ||
<p> | ||
If called from a service worker context, the badge is applied to | ||
all applications whose scope sits within the service worker's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Service worker scope, service worker scope specificity, and application scope and application scope specificity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what this means.
Note that this text is inside a note. It's meant to be informal.
index.html
Outdated
<p> | ||
This algorithm is used to decide which app(s) receive a badge when | ||
the {{NavigatorBadge/setAppBadge()}} method is called from various | ||
contexts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Results in a list of 0 or more apps, or an exception?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this is at the bottom of the algorithm. Specs are weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's at the top of the algorithm. This is a note explaining in more understandable terms what the algorithm is for. (Think of it as a comment above a function, followed by the function header, followed by the function body. This is presented in the exact order as you would see it in code.)
</section> | ||
<section> | ||
<h3> | ||
<dfn>clear()</dfn> function | ||
Determining the set of matching applications |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be above the definition of setAppBadge
or below clearAppBadge
. Seems kind of odd to have it in the middle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/shruggie. It's at the bottom, as a utility function.
index.html
Outdated
</li> | ||
<li> | ||
<a data-cite="infra#list-remove">Remove</a> all elements of | ||
<var>apps</var> other than the one with the longest (i.e., most |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems so backwards, but sure, I guess 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative (which I had originally) was to choose the longest from the set, then put it in a new set. Which seems redundant to create a second set, but if you want I can change it to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've done about 50% of the review... but reached "review fatigue"™️ (not because it's bad! it's great... just bit tired today and I've been looking at this for too long). Will try to pick up again tomorrow. Some suggestions that should make editing the spec much easier via enabling xref.
index.html
Outdated
notifying the user when the state of the application has changed (e.g., | ||
when new messages have arrived), without showing a more heavyweight | ||
This specification defines an API allowing web pages and <a data-cite= | ||
"appmanifest#installable-web-applications">web applications</a> to set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest not putting this dependency on manifest here... we had the same issue in the Push spec. Just liberally talk about "web applications", no need for a citation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But we are explicitly using manifest concepts, not just mentioning the concept of a "web application". Later on, we explicitly use the Manifest "within scope" algorithm to match against installed web apps. So we have a hard dependency on the Manifest spec anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.... ok, we probably then want to data-export=""
"installed web application" from Web Manifest.
index.html
Outdated
@@ -64,13 +65,16 @@ <h2> | |||
showing the unread count, which is updated whenever the client polls | |||
for mail from the server. | |||
</p> | |||
<p class="issue"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest we file actual bugs instead... if there is a bug filed:
<div class="issue" data-number="NN"></div>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, and added issue blocks for all outstanding issues that require direct action in the spec.
index.html
Outdated
<var>contents</var>, run the following steps: | ||
</p> | ||
<ol> | ||
<li>Let <var>result</var> be the result of <a>determining the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can "determining the matching applications" be done without IPC? To be safe, return a promise and continue in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been changed so the error detection happens in the xxxAppBadge methods. I don't think this needs to return a promise. Even if it requires IPC, that's fine because it takes place in the "in parallel" steps of those algorithms, so you can take as much time as you need to execute it.
index.html
Outdated
</li> | ||
</ol> | ||
</li> | ||
<li>Return a <a data-cite="webidl#a-promise-resolved-with">resolved |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be linked like this: "<a>a promise resolved with</a> undefined
" ... same with other ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
I find this language puzzling though, because even though it links correctly, it doesn't match the language used in WebIDL:
To create a resolved promise of type Promise, with x
and
To create a rejected promise of type Promise, with reason r
This suggests I should be using the language "Return a resolved promise with x" and "Return a rejected promise with reason r", not "Return a promise resolved with x". 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so from here, once the suggestions are in, I'd like to go through it again and do a quick JS implementation for sanity (or might even be able to do a rough Gecko/C++ implementation as the spec is petty simple). Looking pretty solid tho :)
</section> | ||
</section> | ||
<section class="informative"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this "normative", even if there are no normative requirements here.
<section class="informative"> | |
<section> |
I'd also suggest we add the accessibility consideration. But we can do that as a different PR to close #24.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we make something normative when it has no normative requirements? I've always seen security and privacy considerations sections be informative.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally trying to move away from that "non-normative" trend specially for priv/sec sections. There are specs that do put normative requirements in place, for example (though it is amongst some non-normative sections):
https://www.w3.org/TR/payment-request/#exposing-user-information
Having said that, we can always flip this switch later if we do use some RFC2119 style requirements.
Co-Authored-By: Marcos Cáceres <marcos@marcosc.com>
4d01853
to
48c6426
Compare
No longer blocks on the badge being set before returning.
Now uses respec referencing syntax as much as possible rather than <a data-cite>.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My replies (mostly). Some things I can't reply to because they're on an outdated commit, so I'll have to reply to those later.
GitHub is fookin bonkers.
index.html
Outdated
@@ -64,13 +65,16 @@ <h2> | |||
showing the unread count, which is updated whenever the client polls | |||
for mail from the server. | |||
</p> | |||
<p class="issue"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, and added issue blocks for all outstanding issues that require direct action in the spec.
index.html
Outdated
<a>nothing</a> at its discretion (for example, when the system is | ||
restarted). | ||
Each <a data-cite="dom#concept-document">document</a> and each | ||
<a data-cite="appmanifest#installable-web-applications">web |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above.
index.html
Outdated
The user agent is not expected to display a badge associated with a | ||
document that is not a <a data-cite= | ||
"html#top-level-browsing-context">top-level browsing context</a>, | ||
although it is allowed to. A user agent does not need to store the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't actually saying "You don't have to display a badge on a top level browsing context." Because if a browser doesn't display badges anywhere, they are expected (via a SHOULD below) not to expose the API at all. So there's an expectation that if you expose this API, you are displaying badges on top-level browsing contexts.
However, we want to call out that you don't have to (and in fact it's very unlikely that you will) display badges for non-top-level browsing contexts, even if you expose the method.
There is no normative text around top-level vs non-top-level browsing contexts here (perhaps we could make this a SHOULD NOT, though that seems a little strong). So I think it's important to call this out, so browser manufacturers aren't asking, "wait, what about non-top-level browsing contexts?" That's precisely what non-normative text is for --- for things that don't technically have to be spelled out but that we still want to call out our intentions.
</section> | ||
<section> | ||
<h3> | ||
<dfn>clear()</dfn> function | ||
Determining the set of matching applications |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/shruggie. It's at the bottom, as a utility function.
index.html
Outdated
<p> | ||
This algorithm is used to decide which app(s) receive a badge when | ||
the {{NavigatorBadge/setAppBadge()}} method is called from various | ||
contexts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's at the top of the algorithm. This is a note explaining in more understandable terms what the algorithm is for. (Think of it as a comment above a function, followed by the function header, followed by the function body. This is presented in the exact order as you would see it in code.)
</p> | ||
<p> | ||
If called from a service worker context, the badge is applied to | ||
all applications whose scope sits within the service worker's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what this means.
Note that this text is inside a note. It's meant to be informal.
index.html
Outdated
</li> | ||
<li> | ||
<a data-cite="infra#list-remove">Remove</a> all elements of | ||
<var>apps</var> other than the one with the longest (i.e., most |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alternative (which I had originally) was to choose the longest from the set, then put it in a new set. Which seems redundant to create a second set, but if you want I can change it to that.
</section> | ||
</section> | ||
<section class="informative"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we make something normative when it has no normative requirements? I've always seen security and privacy considerations sections be informative.
@marcoscaceres Do you know why all of my references are appearing under "Informative references"? Almost all (if not all) of them are in normative sections. I thought ReSpec is supposed to automatically put them in the right section these days. Ready for another review. |
Ah, you are probably missing a conformance section:
ReSpec uses the conformance section to know how to classify citations. |
As a side-effect, this properly puts references in the Normative heading.
Oh OK, that did it.
That's ... surprising. They don't seem related. I noticed that Web Share also had <section id="idl-index"></section> Should I add this? I decided not to as it didn't seem necessary, since there is only one IDL block in this spec, I figured this section is more for summarizing the IDL if there's a lot of it distributed all around the document. Edit: Wow, I love that ReSpec actually scans the document to figure out which RFC keywords I've used when it lists them all at the end there, instead of just listing all possible keywords. |
index.html
Outdated
@@ -8,7 +8,8 @@ | |||
<script src="https://www.w3.org/Tools/respec/respec-w3c-common" class= | |||
"remove"></script> | |||
<script class='remove'> | |||
var respecConfig = { | |||
var respecConfig = { | |||
xref: "web-platform", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicated here and below :)
xref: "web-platform", |
</section> | ||
</section> | ||
<section class="informative"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally trying to move away from that "non-normative" trend specially for priv/sec sections. There are specs that do put normative requirements in place, for example (though it is amongst some non-normative sections):
https://www.w3.org/TR/payment-request/#exposing-user-information
Having said that, we can always flip this switch later if we do use some RFC2119 style requirements.
<code><dfn data-cite= | ||
"!ECMASCRIPT#sec-native-error-types-used-in-this-standard-typeerror">TypeError</dfn></code> | ||
is defined by [[ECMASCRIPT]]. | ||
<dfn data-cite="appmanifest#installable-web-applications" data-lt= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please file a bug on Manifest to export these out? Then we can get rid of this 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sent w3c/manifest#823
</p> | ||
<ol> | ||
<li>If <var>contents</var> is omitted, set the application's badge to | ||
<a>flag</a>. | ||
<li>Return [=a promise resolved with=] undefined, and [=in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is still not correct as it could come back to bite us. This should run in parallel:
- request set the badge,
- possibly reject with an error (let's just add a note saying that we are still gathering implementation experience on what errors could results on different platform/implementations... it's an incubation after all)
- If no error, resolve the promise with undefined.
index.html
Outdated
</h3> | ||
<p> | ||
When the {{Navigator/clearClientBadge()}} method is called, return | ||
[=a promise resolved with=] undefined, and [=in parallel=], set the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above... we need enough scope in the spec that clearing could error... if it doesn't error, then we can resolve the promise with undefined.
Right, the key part that appears in a conformance section is, "As well as sections marked as non-normative, all authoring guidelines, diagrams, examples, and notes in this specification are non-normative. Everything else in this specification is normative." ReSpec knows how to "intelligently"™️ distinguish between "normative" from "non-normative" sections (e.g., notes, examples, figures, etc.). Using the those distinctions, ReSpec can classifies citations into normative and informative buckets. You can imagine a different conformance scheme where, for instance, "figures" could be treated as normative... or maybe special kinds of notes.
Yeah, we probably don't need it. |
Yeah I'm just surprised that |
Yeah, exactly... it defaults to informative. |
<li>Return [=a promise resolved with=] undefined, and [=in | ||
parallel=]: | ||
<ol> | ||
<li>Let |document:Document| be |navigator|'s [=relevant settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, relevant settings object for a normal object seems right, since navigator is presumably not the global object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@annevk Are you saying the current text (of this PR) is correct?
i.e. since this is a method on navigator
, it should get navigator
's relevant settings object, not the global settings object.
Could we not block on that? This PR has been going for weeks and it's an ongoing load on my mind. I don't see the point of splitting it into small pieces since this essentially rewrites everything. Splitting would mean we need to define incremental changes and keep everything consistent at each point. (In other words, it isn't just a matter of landing half the changes, then the other half; it would require artificially creating a sequence of small changes across the whole document to go from A to B, which I don't see the point of since we're already at B.) I am happy to write tests, but I don't want to block on it since this PR is done and tests haven't been started. |
I was kinda hoping we'd follow the WHATWG working mode (i.e., at minimum, don't land things in specs at least without tests). But also am sympathetic to having to context switch over several weeks/months, and how much of a drag that can be. The rationale for doing the tests (and at least a prototype) is that doing so often reveals a ton of spec bugs and forces us to check our assumptions. But we are not obliged to follow that working mode. As long as the followup bugs are filed, we are fine to make incremental fixes. At a minimum tho, at least like to see the returned promise aspect fixed before we merge - I'm happy to propose the text if you'd like. |
Yep, I totally understand. But I do want this to land before making further big changes, just because the cognitive overhead of working with a +300-line spec PR is too large. Since there are 0 WPTs for Badging (which has a very-out-of-date spec on master), I don't see the harm of making the spec match our current intentions while still having 0 tests. Fixing the test situation is a separate concern to updating the spec. Once we have tests, we'll follow the work mode of "change tests and spec in tandem". Happy to make small tweaks to fix up this PR, but I don't want to do another major rework of the algorithms, even if we both agree it's an improvement, without first landing this. That way, we can review the changes as a separate concern. |
I'm ok with this plan. Thanks for hearing me out. Let's get this landed and iterate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to just commit that tiny fix to the ReSpec config.
Thanks! OK landing. The two major AIs then are:
|
Also formalizes the algorithms a lot.
Closes #45.
Preview | Diff