Spec rewrite: Update to new separate document/app API by mgiuca · Pull Request #57 · w3c/badging

mgiuca · 2019-10-25T06:29:05Z

Also formalizes the algorithms a lot.

Closes #45.

mgiuca · 2019-10-25T06:29:58Z

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.

fallaciousreasoning

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 😆 😢

fallaciousreasoning · 2019-10-29T02:28:01Z

      <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>".


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)

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?

fallaciousreasoning · 2019-10-29T02:36:02Z

+        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=


What would the implications of this be?

Filed #59 to discuss this.

fallaciousreasoning · 2019-10-29T02:42:24Z

+        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


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).

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.

fallaciousreasoning · 2019-10-29T03:32:28Z

+          </p>
+          <p>
+            If called from a service worker context, the badge is applied to
+            all applications whose scope sits within the service worker's


Service worker scope, service worker scope specificity, and application scope and application scope specificity.

I don't know what this means.

Note that this text is inside a note. It's meant to be informal.

fallaciousreasoning · 2019-10-29T03:34:47Z

+          <p>
+            This algorithm is used to decide which app(s) receive a badge when
+            the {{NavigatorBadge/setAppBadge()}} method is called from various
+            contexts.


Results in a list of 0 or more apps, or an exception?

I see this is at the bottom of the algorithm. Specs are weird.

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.)

fallaciousreasoning · 2019-10-29T03:35:41Z

      <section>
        <h3>
-          <dfn>clear()</dfn> function
+          Determining the set of matching applications


Should this be above the definition of setAppBadge or below clearAppBadge. Seems kind of odd to have it in the middle.

/shruggie. It's at the bottom, as a utility function.

fallaciousreasoning · 2019-10-29T03:37:56Z

+              </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


This seems so backwards, but sure, I guess 😆

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.

marcoscaceres

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.

marcoscaceres · 2019-10-29T04:54:29Z

-        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


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.

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.

Hmm.... ok, we probably then want to data-export="" "installed web application" from Web Manifest.

marcoscaceres · 2019-10-29T04:58:51Z

        showing the unread count, which is updated whenever the client polls
        for mail from the server.
      </p>
+      <p class="issue">


I'd suggest we file actual bugs instead... if there is a bug filed:

<div class="issue" data-number="NN"></div>

Done, and added issue blocks for all outstanding issues that require direct action in the spec.

marcoscaceres · 2019-10-29T05:33:59Z

+          <var>contents</var>, run the following steps:
+        </p>
+        <ol>
+          <li>Let <var>result</var> be the result of <a>determining the


Can "determining the matching applications" be done without IPC? To be safe, return a promise and continue in parallel.

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.

marcoscaceres · 2019-10-29T05:36:06Z

+              </li>
+            </ol>
+          </li>
+          <li>Return a <a data-cite="webidl#a-promise-resolved-with">resolved


This should be linked like this: "<a>a promise resolved with</a> undefined" ... same with other ones.

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". 🤔

marcoscaceres

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 :)

marcoscaceres · 2019-10-30T03:25:16Z

+        </ol>
      </section>
    </section>
+    <section class="informative">


Let's make this "normative", even if there are no normative requirements here.

Suggested change

<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.

Why would we make something normative when it has no normative requirements? I've always seen security and privacy considerations sections be informative.

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>

No longer blocks on the badge being set before returning.

Now uses respec referencing syntax as much as possible rather than <a data-cite>.

mgiuca

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.

mgiuca · 2019-10-31T02:34:38Z

        showing the unread count, which is updated whenever the client polls
        for mail from the server.
      </p>
+      <p class="issue">


Done, and added issue blocks for all outstanding issues that require direct action in the spec.

mgiuca · 2019-10-31T02:40:04Z

-        <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


mgiuca · 2019-10-31T02:48:21Z

+        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


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.

mgiuca · 2019-11-01T00:52:15Z

      <section>
        <h3>
-          <dfn>clear()</dfn> function
+          Determining the set of matching applications


/shruggie. It's at the bottom, as a utility function.

mgiuca · 2019-11-01T00:54:18Z

+          <p>
+            This algorithm is used to decide which app(s) receive a badge when
+            the {{NavigatorBadge/setAppBadge()}} method is called from various
+            contexts.


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.)

mgiuca · 2019-11-01T00:57:01Z

+          </p>
+          <p>
+            If called from a service worker context, the badge is applied to
+            all applications whose scope sits within the service worker's


I don't know what this means.

Note that this text is inside a note. It's meant to be informal.

mgiuca · 2019-11-01T00:58:24Z

+              </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


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.

mgiuca · 2019-11-01T00:59:32Z

+        </ol>
      </section>
    </section>
+    <section class="informative">


Why would we make something normative when it has no normative requirements? I've always seen security and privacy considerations sections be informative.

mgiuca · 2019-11-01T05:26:57Z

@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.

marcoscaceres · 2019-11-01T08:38:18Z

Ah, you are probably missing a conformance section:

<section id="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.

mgiuca · 2019-11-04T04:14:47Z

Ah, you are probably missing a conformance section:
<section id="conformance"></section>

Oh OK, that did it.

ReSpec uses the conformance section to know how to classify citations.

That's ... surprising. They don't seem related.

I noticed that Web Share also had idl-index:

<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.

… object.

marcoscaceres · 2019-11-01T04:31:26Z

    <script class='remove'>
-      var respecConfig = {
+       var respecConfig = {
+        xref: "web-platform",


This is duplicated here and below :)

Suggested change

xref: "web-platform",

marcoscaceres · 2019-11-04T04:32:12Z

+        </ol>
      </section>
    </section>
+    <section class="informative">


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.

marcoscaceres · 2019-11-04T04:33:13Z

-        <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=


Could you please file a bug on Manifest to export these out? Then we can get rid of this 🎉

Sent w3c/manifest#823

marcoscaceres · 2019-11-04T04:36:46Z

        <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


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.

marcoscaceres · 2019-11-04T04:38:08Z

+        </h3>
+        <p>
+          When the {{Navigator/clearClientBadge()}} method is called, return
+          [=a promise resolved with=] undefined, and [=in parallel=], set the


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.

marcoscaceres · 2019-11-04T04:45:07Z

That's ... surprising. They don't seem related.

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.

<section id="idl-index"></section>

Yeah, we probably don't need it.

mgiuca · 2019-11-04T04:53:47Z

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.

Yeah I'm just surprised that conformance does that and not a general rule of ReSpec. Or are you saying that it's by design, because if you don't include conformance then NOTHING is normative?

marcoscaceres · 2019-11-04T05:00:37Z

Yeah I'm just surprised that conformance does that and not a general rule of ReSpec. Or are you saying that it's by design, because if you don't include conformance then NOTHING is normative?

Yeah, exactly... it defaults to informative.

marcoscaceres · 2019-11-08T06:30:33Z

+          <li>Return [=a promise resolved with=] undefined, and [=in
+          parallel=]:
+            <ol>
+              <li>Let |document:Document| be |navigator|'s [=relevant settings


HTML defines a "relevant settings object for a global object O", which we could perhaps use instead? it's currently not exported from HTML tho... @annevk or @domenic can probably confirm if that would be correct to use here?

No, relevant settings object for a normal object seems right, since navigator is presumably not the global object.

@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.

mgiuca · 2019-11-11T05:04:27Z

@marcoscaceres :

Maybe we can split #57 one up into smaller pieces? I'd like for us to land that together with the web platform tests. I'd be happy to help with that and/or the tests if needed.

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.

marcoscaceres · 2019-11-11T05:48:59Z

Could we not block on that? This PR has been going for weeks and it's an ongoing load on my mind... 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.

mgiuca · 2019-11-11T06:14:30Z

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.

marcoscaceres · 2019-11-11T06:26:15Z

I'm ok with this plan. Thanks for hearing me out. Let's get this landed and iterate.

marcoscaceres

Might want to just commit that tiny fix to the ReSpec config.

mgiuca · 2019-11-11T06:41:48Z

Thanks! OK landing.

The two major AIs then are:

Write web-platform-tests. (Note: We have Chrome-specific tests in https://cs.chromium.org/chromium/src/third_party/blink/web_tests/badging/ but they use Chrome's internal IPC mocking, so we need to probably convert them to manual tests.)
Modify the algorithm to resolve the promise later (Should OS failures setting the badge reject the promise? #61). Let me have a crack at this.,

mgiuca requested review from fallaciousreasoning and marcoscaceres October 25, 2019 06:29

fallaciousreasoning reviewed Oct 29, 2019

View reviewed changes

marcoscaceres requested changes Oct 29, 2019

View reviewed changes

marcoscaceres reviewed Oct 30, 2019

View reviewed changes

Comment thread index.html Outdated

marcoscaceres reviewed Oct 30, 2019

View reviewed changes

mgiuca mentioned this pull request Oct 31, 2019

Should OS failures setting the badge reject the promise? #61

Closed

mgiuca and others added 7 commits November 1, 2019 13:40

Update spec to the new API in w3c#55.

22cf1a5

More writing.

e1c9fd2

Update examples.

7c8d189

Updated abstract.

da1e022

Apply suggestions from code review

f415691

Co-Authored-By: Marcos Cáceres <marcos@marcosc.com>

Partially respond to Marcos and Jay review.

c3d75a1

Mostly finish responding.

48c6426

mgiuca force-pushed the separation-spec branch from 4d01853 to 48c6426 Compare November 1, 2019 02:41

mgiuca added 2 commits November 1, 2019 14:42

Fix parallelization of the algorithms.

09e3702

No longer blocks on the badge being set before returning.

Clean up linkification.

3ef5832

Now uses respec referencing syntax as much as possible rather than <a data-cite>.

mgiuca commented Nov 1, 2019

View reviewed changes

Added client examples.

940041b

mgiuca mentioned this pull request Nov 1, 2019

Add examples of client/document badging #58

Closed

Use set, not sequence.

8213d85

Added conformance section.

c75d437

As a side-effect, this properly puts references in the Normative heading.

mgiuca added 2 commits November 4, 2019 15:33

Use navigator's relevant settings object, instead of current settings…

5ef37a1

… object.

Remove issue 59 (closed).

6037e53

mgiuca mentioned this pull request Nov 4, 2019

Do we really want to add Badge API to Navigator? #56

Closed

marcoscaceres reviewed Nov 4, 2019

View reviewed changes

mgiuca mentioned this pull request Nov 4, 2019

PR Preview is 500 internal server erroring tobie/pr-preview#44

Closed

marcoscaceres reviewed Nov 8, 2019

View reviewed changes

marcoscaceres approved these changes Nov 11, 2019

View reviewed changes

Remove duplicated xref.

4cf22fb

mgiuca merged commit ba02318 into w3c:master Nov 11, 2019

Uh oh!

Conversation

mgiuca commented Oct 25, 2019 • edited by pr-preview Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mgiuca commented Oct 25, 2019

Uh oh!

fallaciousreasoning left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fallaciousreasoning Oct 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgiuca Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgiuca Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcoscaceres left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcoscaceres Nov 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mgiuca commented Oct 25, 2019 •

edited by pr-preview Bot

Loading

fallaciousreasoning Oct 29, 2019 •

edited

Loading

mgiuca Oct 31, 2019 •

edited

Loading

mgiuca Oct 31, 2019 •

edited

Loading

marcoscaceres Nov 1, 2019 •

edited

Loading