Bikeshed cleanup #1014

jakearchibald · 2016-11-24T16:22:35Z

Bikeshed is now building the spec without complaining, aside from the lack of privacy considerations.

I'm now going to go through a diff of this and the current spec and see if anything's broken.

I've made some intentional changes along the way, and I'll detail them here for review.

Once it's in a good place I'll merge in the changes that have been made to the spec since I started.

jakearchibald · 2016-11-24T16:23:02Z

+@annevk and @jungkees for reviewing. Apologies in advance.

jakearchibald · 2016-11-24T16:35:43Z

Found a few cases where the auto-linker is picking up the wrong thing. Fixing now…

jakearchibald · 2016-11-24T17:06:19Z

Haha ok it's pretty broken, don't bother reviewing until I wave. A few markdown issues related to nested lists caught me out. It's different to github.

jakearchibald · 2016-11-24T18:35:59Z

Ok, it's in a somewhat better shape now.

Here are the intentional (larger) changes I made

I removed the "storage considerations" section. It felt redundant as it's all handled by other specs.
I changed all the <em class="rfc2119" title="MUST"> style rfc2119 references to be plain <em>. The semantics of these terms are covered elsewhere.
Removed reference to sysapps "alarms" API
"script resource map", "scope to registration map" are now an infra maps
I've updated examples to use newer ES features like arrow functions
We had a few instances of return <a>context object</a>’s <a>relevant settings object</a> <var>client</var>, were client wasn't used. I replaced these with return <a>context object</a>’s <a>relevant settings object</a>

jungkees · 2016-11-25T06:03:05Z

Great move. Was keen to change the following:

"script resource map", "scope to registration map" are now an infra maps

I've updated examples to use newer ES features like arrow functions

I'll review.

jungkees · 2016-11-25T06:40:43Z

Three commits are missing from the master: eda15f6, c2a518b, ~~fd1fd35~~ (Done in 92cdc32)

I guess rebasing seems harder than merging them manually? If so, I can help. Also shaping V1 would be a job. Wouldn't it be easier to separate it out from the Nightly when we're ready for the last WD before moving to CR?

jungkees · 2016-11-25T07:09:22Z

I removed the "storage considerations" section. It felt redundant as it's all handled by other specs.

It seems we can keep Storage Considerations with some non-normative words? People tend to ask what happens if the storage with the Cache API gets full. Yeah, I agree better wording's needed to keep it.

annevk · 2016-11-25T08:20:39Z

I guess rebasing seems harder than merging them manually?

You need to rebase if you want to land this successfully.

jungkees · 2016-11-25T08:31:46Z

Suggestion: what about changing function names to lowercase? The history they are currently uppercase for each word is that I tried to use ECMAScript internal method notation in the early days. And later we dropped the double-brackets. I think we can just move to lowercase as in the other specs.

jungkees · 2016-11-25T08:37:48Z

@annevk, do you see 100 char wrapping rule is really helpful?

jakearchibald · 2016-11-25T09:29:05Z

@jungkees yeah, I'm aware that work on master continues as I'm working on this. Let's manually merge them once the branch is done.

When it comes to V1… hmm. I'd rather not introduce a whole new version. I'll take a look at how much effort it would be to convert V1.

jungkees · 2016-11-25T10:45:27Z

@jungkees yeah, I'm aware that work on master continues as I'm working on this. Let's manually merge them once the branch is done.

OK. The important bit would be to not loose the commit history. Let's leave the relevant description when manual merging is needed.

annevk · 2016-11-25T11:19:31Z

@jungkees wrapping tends to make diffs more readable, but sometimes not.

jakearchibald · 2016-11-25T11:22:30Z

I'm in two minds about it. The lack of wrapping make it a lot easier to do multi-cursor editing and find/replace during this PR.

annevk · 2016-11-25T11:24:39Z

Whenever I wrap for DOM, Fetch, etc. I leave terms unwrapped to make finding and replacing them easy, but HTML follows a different convention...

I don't really understand what you mean by manually merging btw. Is the plan to keep the git history linear?

jakearchibald · 2016-11-25T11:34:57Z

Sorry, yes, I mean "manually rebase" I guess. Intent is to remain linear.

jakearchibald · 2016-11-25T11:36:15Z

I've reviewed the diff with the HTML output from 0dcd798 (the current branch point) and it looks good to me. Going to give it a second reading to be sure.

Btw, temporarily re-adding the "storage considerations" section makes the diff a lot more readable.

…now. The whole spec isn’t here yet.

jungkees · 2016-11-29T07:12:33Z

@jakearchibald, I rebased it onto master and fixed some minor errors. I still encounters one bikeshed error though:

FATAL ERROR: Multiple declarations of the '' algorithm.

Do you have any clue? I think the current snapshot seems good-to-go after resolving the above issue. Re V1, do you have any suggestion how to shape it?

annevk · 2016-11-29T08:26:53Z

You can find out the line for an error with bikeshed spec -l: https://tabatkins.github.io/bikeshed/#cli-spec.

annevk · 2016-11-29T09:03:35Z

No, those ! indicate the abstract operation does not throw. They have meaning and should probably not be removed. Curious that Service Workers plays with ArrayBuffer though.

This reverts commit 5d841b6.

jakearchibald · 2016-11-29T09:19:56Z

@annevk oops. TIL.

Going to try to get bikeshed working locally so I can investigate that error. However, it seems to be failing with commits that I'm pretty certain it was working with a few days ago, so it might be a bikeshed issue.

jakearchibald · 2016-11-29T09:52:13Z

Yeah this is a bikeshed regression speced/bikeshed#884

mkruisselbrink

Awesome cleanup. Thanks for cleaning up my mess :) I haven't quite made it through the entire thing yet, but it looks good I think. Just some random ideas for possible future improvements.

mkruisselbrink · 2016-11-29T19:17:28Z

docs/index.bs


-  <p>The <a href="#dfn-service-worker">service worker</a> is designed first to redress this balance by providing a Web Worker context, which can be started by a runtime when navigations are about to occur. This event-driven worker is registered against an origin and a path (or pattern), meaning it can be consulted when navigations occur to that location. Events that correspond to network requests are dispatched to the worker and the responses generated by the worker may override default network stack behavior. This puts the <a href="#dfn-service-worker">service worker</a>, conceptually, between the network and a document renderer, allowing the <a href="#dfn-service-worker">service worker</a> to provide content for documents, even while offline.</p>
+  The <a for="/">service worker</a> is designed first to redress this balance by providing a Web Worker context, which can be started by a runtime when navigations are about to occur. This event-driven worker is registered against an origin and a path (or pattern), meaning it can be consulted when navigations occur to that location. Events that correspond to network requests are dispatched to the worker and the responses generated by the worker may override default network stack behavior. This puts the <a for="/">service worker</a>, conceptually, between the network and a document renderer, allowing the <a for="/">service worker</a> to provide content for documents, even while offline.


depending on how shorthand-rather-than-html you want to go, all these "<a for="/">service worker</a>" bits could be written as [=/service worker=].

I like it! [=/service worker=], [=ServiceWorkerGlobalScope/service worker=] work fine and are shorter to type.

Replaced all the <a for="thing">a dfn for thing</a> style anchors with [=thing/a dfn for thing=] style shorthands. I didn't change the existing <a>to dfn</a> (without for attribute) occurrences yet. Would it be better to change all of them to [=to dfn=]?

mkruisselbrink · 2016-11-29T19:35:27Z

docs/index.bs

-      </ol>
+      The <dfn method for="ServiceWorker"><code>postMessage(|message|, |transfer|)</code></dfn> method *must* run these steps:
+
+        1. If the {{ServiceWorker/state}} attribute value of the <a>context object</a> is "<code>redundant</code>", <a>throw</a> an "{{InvalidStateError}}" exception and abort these steps. 


"<code>redundant</code>" could also be written as {{"redundant"}} (or {{ServiceWorkerState/"redundant"}} to get proper cross-references for enum values.

{{ServiceWorkerState/"redundant"}} would be nice.

Changed them to {{"enumvalue"}} style. I didn't put ServiceWorkerState/ within the spec. Also I left them "<code>string</code>" when they represent just a string.

mkruisselbrink · 2016-11-29T19:39:35Z

docs/index.bs

+
+    A <a for="/">service worker</a> has an associated <dfn export id="dfn-service-worker-global-object" for="service worker">global object</dfn> (a {{ServiceWorkerGlobalScope}} object or null).
+
+    A <a for="/">service worker</a> is dispatched a set of <dfn export id="dfn-lifecycle-events">lifecycle events</dfn>, <a event for="ServiceWorkerGlobalScope">install</a> and <a event for="ServiceWorkerGlobalScope">activate</a>, and <dfn export id="dfn-functional-events">functional events</dfn> including <a event for="ServiceWorkerGlobalScope">fetch</a>.


You're a bit inconsistent with how you link to event types. Here you use the full <a ...> syntax, while in other places you would have written this as {{install!!event}} (or {{ServiceWorkerGlobalScope/install!!event}} if you want to be explicit about which install event you're talking about).

Let's use {{install!!event}}. I think it'd not be ambiguous without the event target within the spec.

mkruisselbrink · 2016-11-29T19:45:28Z

docs/index.bs


-      <p class="example">In the example in section 3.1.1, the value of <code>registration.scope</code>, obtained from <code>navigator.serviceWorker.ready.then(function(registration) { console.log(registration.scope); })</code> for example, will be "<code>https://example.com/</code>".</p>
+      <div class="example">
+        In the example in section 3.1.1, the value of <code>registration.scope</code>, obtained from <code>navigator.serviceWorker.ready.then(registration => console.log(registration.scope))</code> for example, will be "<code>https://example.com/</code>".


Maybe change section 3.1.1 to [[#service-worker-url]] to get auto-linking to that section? Although not sure if having the section title in the resulting link will make it more or less readable here.

Changed it. Enabling a link seems better.

mkruisselbrink · 2016-11-29T19:53:05Z

docs/index.bs

+        1. Run the following substeps <a>in parallel</a>:
+            1. Let |array| be an empty array.
+            1. <a for="map">For each</a> |key| → |value| of <a>scope to registration map</a>:
+                1. If the <a for="environment settings object">origin</a> of the result of <a lt="URL parser">parsing</a> |key| is the <a lt="same origin">same</a> as |client|'s <a for="environment settings object">origin</a>, and |value|'s <a>uninstalling flag</a> is unset, add the {{ServiceWorkerRegistration}} object associated with |value| to the |array|.


The first origin here should be <a for="url"> as it's referring to the origin of a url rather than that of a settings object (I know, I made quite a mess about linking to the correct origin definition whenever an origin was used).

mkruisselbrink · 2016-11-29T19:54:39Z

docs/index.bs

@@ -765,7 +716,7 @@ spec: rfc7231; urlPrefix: https://tools.ietf.org/html/rfc7231
        <tr>
          <td><dfn event id="service-worker-container-controllerchange-event"><code>controllerchange</code></dfn></td>
          <td>{{Event}}</td>
-          <td>The <a for="ServiceWorkerContainer">service worker client</a>'s <a>active service worker</a> changes. (See step 9.2 of the <a href="#activation-algorithm">Activate</a> algorithm. The <a href="#dfn-skip-waiting-flag">skip waiting flag</a> of a <a href="#dfn-service-worker">service worker</a> causes <a href="#activation-algorithm">activation</a> of the <a href="#dfn-service-worker-registration">service worker registration</a> to occur while <a href="#dfn-service-worker-client">service worker clients</a> are <a href="#dfn-use">using</a> the <a href="#dfn-service-worker-registration">service worker registration</a>, {{ServiceWorkerContainer/controller|navigator.serviceWorker.controller}} immediately reflects the <a href="#dfn-active-worker">active worker</a> as the <a href="#dfn-service-worker">service worker</a> that <a href="#dfn-control">controls</a> the <a href="#dfn-service-worker-client">service worker client</a>.)</td>
+          <td>The <a for="ServiceWorkerContainer">service worker client</a>'s <a>active service worker</a> changes. (See step 9.2 of the <a>Activate</a> algorithm. The <a>skip waiting flag</a> of a <a for="/">service worker</a> causes <a lt="activate">activation</a> of the <a for="/">service worker registration</a> to occur while <a for="/">service worker clients</a> are <a>using</a> the <a for="/">service worker registration</a>, {{ServiceWorkerContainer/controller|navigator.serviceWorker.controller}} immediately reflects the <a>active worker</a> as the <a for="/">service worker</a> that <a>controls</a> the <a for="/">service worker client</a>.)</td>


@tabatkins hmm, would be nice if bikeshed provided a way to link to specific steps in an algorithm that would get rendered as "step 9.2", but wouldn't require hard-coding and manually keeping up to date the numbering (like it already sort of supports for sections).

Yup, that's tracked as speced/bikeshed#796 already. ^_^

Yes, this will help a lot for both discussion and writing.

jungkees · 2016-11-30T01:17:00Z

@mkruisselbrink, your comments are great. I'll address them.

jungkees · 2016-11-30T06:18:38Z

@jakearchibald, I'm working on V1.

jakearchibald · 2016-11-30T06:22:49Z

Haha I'm running out of things to do on my flight home. Thanks for taking a look, let me know if there's anything I can do.

jungkees · 2016-11-30T08:08:28Z

Haha, that's OK. I just uploaded a patch having merged the changes to V1. Please review and merge this PR if it looks good.

jakearchibald · 2016-12-01T08:58:02Z

Aaaaand we're merged! Thank you @jungkees @mkruisselbrink and @annevk.

jakearchibald and others added 13 commits November 29, 2016 11:09

Re-importing the spec, making use of as many bikeshed features as I k…

8ac7cb5

…now. The whole spec isn’t here yet.

More

7fc510d

Fixing formatting

759b124

Tidy up & ordered lists

181b705

Be gone whitespace

d869ed0

All linking errors gone!

9ad8a77

Reindenting EVERYTHING

91e85aa

Correcting links pointing at the wrong places

93b9caa

Minor - fix typos

6e9d442

Correct a dfn closing tag location

589e1dc

Hints were breaking styling

e3b00d6

Some link fixes

e9ebc1f

Rebase and merge conflicts for the missed commits

87cc817

jungkees force-pushed the bikeshed-cleanup branch from d55cc7d to 87cc817 Compare November 29, 2016 07:05

Pretty sure these ! are typos

5d841b6

Revert "Pretty sure these ! are typos"

4c0af6e

This reverts commit 5d841b6.

mkruisselbrink reviewed Nov 29, 2016

View reviewed changes

Address @mkruisselbrink's comments: use bikeshed shorthands

157f048

Merge bikeshed cleanups to V1

c7b5845

jakearchibald merged commit bec528d into master Dec 1, 2016

jakearchibald deleted the bikeshed-cleanup branch December 1, 2016 08:57

jungkees changed the title ~~(WIP) Bikeshed cleanup~~ Bikeshed cleanup Dec 1, 2016

jakearchibald mentioned this pull request Dec 2, 2016

Lots of bikeshed usage issues #965

Closed


		<p>The <a href="#dfn-service-worker">service worker</a> is designed first to redress this balance by providing a Web Worker context, which can be started by a runtime when navigations are about to occur. This event-driven worker is registered against an origin and a path (or pattern), meaning it can be consulted when navigations occur to that location. Events that correspond to network requests are dispatched to the worker and the responses generated by the worker may override default network stack behavior. This puts the <a href="#dfn-service-worker">service worker</a>, conceptually, between the network and a document renderer, allowing the <a href="#dfn-service-worker">service worker</a> to provide content for documents, even while offline.</p>
		The <a for="/">service worker</a> is designed first to redress this balance by providing a Web Worker context, which can be started by a runtime when navigations are about to occur. This event-driven worker is registered against an origin and a path (or pattern), meaning it can be consulted when navigations occur to that location. Events that correspond to network requests are dispatched to the worker and the responses generated by the worker may override default network stack behavior. This puts the <a for="/">service worker</a>, conceptually, between the network and a document renderer, allowing the <a for="/">service worker</a> to provide content for documents, even while offline.


		A <a for="/">service worker</a> has an associated <dfn export id="dfn-service-worker-global-object" for="service worker">global object</dfn> (a {{ServiceWorkerGlobalScope}} object or null).

		A <a for="/">service worker</a> is dispatched a set of <dfn export id="dfn-lifecycle-events">lifecycle events</dfn>, <a event for="ServiceWorkerGlobalScope">install</a> and <a event for="ServiceWorkerGlobalScope">activate</a>, and <dfn export id="dfn-functional-events">functional events</dfn> including <a event for="ServiceWorkerGlobalScope">fetch</a>.

Bikeshed cleanup #1014

Bikeshed cleanup #1014

Conversation

jakearchibald commented Nov 24, 2016

jakearchibald commented Nov 24, 2016

jakearchibald commented Nov 24, 2016

jakearchibald commented Nov 24, 2016

jakearchibald commented Nov 24, 2016

jungkees commented Nov 25, 2016

jungkees commented Nov 25, 2016 • edited Loading

jungkees commented Nov 25, 2016

annevk commented Nov 25, 2016

jungkees commented Nov 25, 2016

jungkees commented Nov 25, 2016

jakearchibald commented Nov 25, 2016

jungkees commented Nov 25, 2016

annevk commented Nov 25, 2016

jakearchibald commented Nov 25, 2016

annevk commented Nov 25, 2016

jakearchibald commented Nov 25, 2016

jakearchibald commented Nov 25, 2016 • edited Loading

jungkees commented Nov 29, 2016

annevk commented Nov 29, 2016

annevk commented Nov 29, 2016

jakearchibald commented Nov 29, 2016

jakearchibald commented Nov 29, 2016

mkruisselbrink left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jungkees commented Nov 30, 2016

jungkees commented Nov 30, 2016

jakearchibald commented Nov 30, 2016

jungkees commented Nov 30, 2016

jakearchibald commented Dec 1, 2016

jungkees commented Nov 25, 2016 •

edited

Loading

jakearchibald commented Nov 25, 2016 •

edited

Loading