Add IDL section and define IDL for all the members #613

kenchris · 2017-09-20T15:44:05Z

kenchris · 2017-09-20T15:47:06Z

@mgiuca I did a little screw up, so new PR now :-) Comments welcome

mgiuca

lgtm with some comments. Very nice work!

mgiuca · 2017-09-21T04:10:21Z

index.html

+          [NoInterfaceObject]
+          dictionary WebAppManifest {
+             TextDirectionType? dir;
+             USVString? lang;


Conversely to what I said yesterday, I think "lang", "iarc_rating_id", and any other "id" or other ASCII-only fields could be DOMString instead of USVString. Practically, it probably doesn't really matter, but the WebIDL spec says "if in doubt, use DOMString".

Agree. Let's change this to DOMString... it will make this less controversial if this later gets reviewed by browser folks.

mgiuca · 2017-09-21T04:13:24Z

index.html

+      <pre class="idl">
+        [NoInterfaceObject]
+        dictionary ImageResource {
+          USVString src;


mgiuca · 2017-09-21T04:13:30Z

index.html

+        [NoInterfaceObject]
+        dictionary ImageResource {
+          USVString src;
+          DOMString sizes;


I don't think this is required.

src is required at least. Would you assume it is "any" if sizes is not set? Maybe file an issue?

mgiuca · 2017-09-21T04:15:34Z

index.html

+      <pre class="idl">
+        [NoInterfaceObject]
+        dictionary ServiceWorkerRegistration {
+          USVString src;


mgiuca · 2017-09-21T04:16:00Z

index.html

+      <pre class="idl">
+        [NoInterfaceObject]
+        dictionary ExternalApplicationResource {
+          USVString platform;


marcoscaceres

Mostly nits, but a couple of things to change... looking good.

marcoscaceres · 2017-09-21T04:22:10Z

index.html

      </p>
+      <pre class="idl">
+          [NoInterfaceObject]


Remove [NoInterfaceObject] ... it's not an interface, it's a dictionary 😉. Also, you should never ever need to use NoInterfaceObject in a spec unless you are creating some kind of mix-in.

(I'll just emphasize, if you ever see NoInterfaceObject in a spec, yell at them... or tell me, and I will yell at them 📣😝)

Sure :-) done...

marcoscaceres · 2017-09-21T04:23:33Z

index.html

@@ -1592,17 +1590,40 @@ <h3 id="applying">
        <div class="issue" data-number="446"></div>
      </section>
    </section>
-    <section>
+    <section data-dfn-for="webappmanifest" data-link-for="webappmanifest">


The data-dfn-for="webappmanifest" data-link-for="webappmanifest" need to match actual IDL things, so, should be "WebAppManifest".

marcoscaceres · 2017-09-21T04:24:07Z

index.html

-        contains zero or more members. Each of the members are defined below,
-        as well as how their values are processed.
+        launched. A manifest consists of a top-level <dfn>WebAppManifest</dfn>
+        <a>object</a> that contains zero or more members.


It's a dictionary, not an object.

How do you want me to refer to that then. Object is defined as part of JSON, dictionary is not:

<p> As the manifest uses the JSON format, this specification relies on the types defined in [[!ECMA-404]] specification: namely <dfn>object</dfn>, <dfn>array</dfn>, <dfn>number</dfn>, <dfn>string</dfn>, <code>true</code>, <code>false</code>, and <code>null</code>. Strict type checking is not enforced by this specification. Instead, each member's definition specifies the steps required to process a particular member and what to do when a type does not match what is expected. </p>

marcoscaceres · 2017-09-21T04:24:33Z

index.html

-        as well as how their values are processed.
+        launched. A manifest consists of a top-level <dfn>WebAppManifest</dfn>
+        <a>object</a> that contains zero or more members.
+        Each of the members are defined below, as well as how their values are processed.


Nit: make sure you run tidy over the doc.

Any info on this tidy and how to install/run it?

marcoscaceres · 2017-09-21T04:25:27Z

index.html

@@ -1612,45 +1633,49 @@ <h3 id="applying">
          <code>dir</code> member


Can you replace this with: <dfn>dir</dfn>member, and the same for the rest?

you won't need <code>, ReSpec will automatically code them.

I actually think it was done this way so that you dont have cursive in the headers, which looks stupid. The next line does the dfn

marcoscaceres · 2017-09-21T04:26:44Z

index.html

        <p>
          The <dfn>directionality-capable members</dfn> are:
        </p>
        <ul>
          <li>
-            <a><code>description</code> member</a>.
+            <a><code>description</code></a> member.


s/<a><code>description</code></a> member./<a>description</a> member.

Same for the rest. You should never need <code> anywhere.

marcoscaceres · 2017-09-21T04:28:07Z

index.html

-          The <dfn data-lt="text-direction value">text-direction values</dfn>
-          are the following, implying that the value of the
+          The <dfn data-lt="text-direction value">text-direction values</dfn> defined by
+          <dfn>TextDirectionType</dfn>, are the following, implying that the value of the


The TextDirectionType should have it's own top-level section.

marcoscaceres · 2017-09-21T04:30:27Z

index.html

@@ -2090,8 +2124,8 @@ <h3 id="applying">
          <code>orientation</code> member
        </h3>
        <p>
-          The <dfn id="member-orientation"><code>orientation</code>
-          member</dfn> is a <a>string</a> that serves as the <a>default
+          The <dfn><code>orientation</code></dfn>


s/<dfn><code>orientation</code></dfn>/<dfn>orientation</dfn> member

As above, the dfn for this should occur inside a h3 element in its own section.

The above applies for all the things below.

yes I can remove from inside and , doing

marcoscaceres · 2017-09-21T04:31:27Z

index.html

+        [NoInterfaceObject]
+        dictionary ImageResource {
+          USVString src;
+          DOMString sizes;


I don't think this is required.

marcoscaceres · 2017-09-22T05:27:00Z

index.html

@@ -1261,7 +1259,7 @@ <h3 id="navigation-scope-security-considerations">
        </p>
        <p>
          A <a>manifest</a> is obtained and applied regardless of the
-          <a><code>media</code></a> attribute of the <a><code>link</code>
+          <a>media</a> attribute of the <a><code>link</code>


Nit: seems you missed a closing </a>. Also, please remove the <code>.

its there :-) comes a bit later

marcoscaceres · 2017-09-22T05:31:12Z

index.html

@@ -1574,7 +1572,7 @@ <h3 id="applying">
        <div>
          <p>
            Please note that the <a>start URL</a> is not necessarily the value
-            of the <a><code>start_url</code> member</a>: the user or user agent
+            of the <a data-link-for="WebAppManifest"><code>start_url</code></a> member: the user or user agent


Bit cleaner, you can do:

<a data-tl="WebAppManifest.start_url">start_url</a>

marcoscaceres · 2017-09-22T05:32:26Z

index.html

@@ -1592,17 +1590,39 @@ <h3 id="applying">
        <div class="issue" data-number="446"></div>
      </section>
    </section>
-    <section>
+    <section data-dfn-for="WebAppManifest" data-link-for="WebAppManifest">
      <h2>
        Manifest and its members


This should change to:

<h2> <dfn>WebAppManifest</dfn> dictionary </h2>

marcoscaceres · 2017-09-22T05:32:44Z

index.html

-        launched. A manifest consists of a top-level <a>object</a> that
-        contains zero or more members. Each of the members are defined below,
-        as well as how their values are processed.
+        launched. A manifest consists of a top-level <dfn>WebAppManifest</dfn>


See comment above... so here <a>WebAppManifest</a>

Actually, " A manifest consists of a top-level..." is redundant... remove this sentence. The WebIDL itself already says this.

marcoscaceres · 2017-09-22T05:35:31Z

index.html

      </p>
+      <pre class="idl">


Nit: I prefer seeing the IDL before the description... "an IDL block speaks a thousand words" 😂

marcoscaceres · 2017-09-22T05:39:46Z

index.html

@@ -2090,15 +2121,15 @@ <h3 id="applying">
          <code>orientation</code> member
        </h3>
        <p>
-          The <dfn id="member-orientation"><code>orientation</code>
-          member</dfn> is a <a>string</a> that serves as the <a>default
+          The <dfn>orientation</dfn>


Nit: the <dfn>orientation</dfn> and so on, should all be defined inside the h3s... like this:

<h3> <dfn>orientation</dfn> member </h3> <p>The <a>orientation</a> member bla bla..</p>

Applies to all of the ones below...

but that makes them all cursive :(

marcoscaceres · 2017-09-22T05:42:52Z

index.html

@@ -2995,13 +3035,13 @@ <h3 id="applying">
          </pre>
        </div>
      </section>
-      <section>
+      <section data-dfn-for="imageresource">


<section data-dfn-for="ImageResource" data-link-for="ImageResource">

marcoscaceres · 2017-09-22T05:43:46Z

index.html

@@ -3094,18 +3134,18 @@ <h3 id="applying">
          </li>
        </ol>
      </section>
-      <section>
+      <section data-dfn-for="imageresource">


You shouldn't need:

data-dfn-for="imageresource"

ReSpec will find it from the parent section.

Maybe it should, but it doesn't ... removing this and I get one more error

marcoscaceres · 2017-09-22T05:46:19Z

index.html

@@ -3234,41 +3274,40 @@ <h3 id="applying">
        Each member specifies which object a multi-purpose member can be used
        with.
      </p>
-      <section>
+      <section data-dfn-for="ExternalApplicationResource" data-link-for="ExternalApplicationResource">


platform should be defined in the ExternalApplicationResource section. If this is a shared definition, then we should make a note that two things share the same definition.

Maybe you can help me wth that in a follow up

marcoscaceres · 2017-09-22T05:47:41Z

index.html

@@ -3472,11 +3517,20 @@ <h3 id="applying">
    </section>
    <section>


Add:
data-dfn-for= "ExternalApplicationResource" and data-link-for="ExternalApplicationResource"

Please make sure all sections that define IDL things have both.

marcoscaceres · 2017-09-26T08:43:02Z

So... although we can define processing in IDL, we need to be careful if we are going to expose these on the web. In particular, “foo_bar” property names would make this API inconsistent with the rest of the platform.

It’s a bit similar (but obviously not the same) as CSS in JS, where “foo-bar” becomes “fooBar”. See casing rules https://w3ctag.github.io/design-principles/#casing-rules

marcoscaceres · 2017-10-02T08:33:10Z

index.html

          </li>
-          <li>If <a>Type</a>(<var>value</var>) is not "string":
+          <li>If <a>Type</a>(<var>V</var>) is <code>String</code>:


how come you changed these? I can live with them, but single letter attributes are not my favorite.

I like them shorter, makes it easier for me to read, plus aligned with WebIDL spec

plus aligned with WebIDL spec

That's something I'm hoping to fix. It's a dreadful practice.

I agree. It gets really confusing because you always need to go back to look up what something was. Also, the upper-case thing makes it look like a constant.

OK guys I will change it :-) "value" it is

marcoscaceres · 2017-10-02T08:39:34Z

🤔 overall, something doesn't feel right about how we are going about this... What I was expecting was:

Bytes over the wire => UTF-8 text => JSON Object => IDL Conversion => member normalization => return idlObejct.

I need to think a bit more.

marcoscaceres · 2017-10-02T08:41:44Z

To be bit more clear: we need to look at each member and see what kind of normalization we need to do (if any). I was expecting to see a fairly massive reduction in spec text, because at lot of the processing algorithms should go away when the do the IDL conversion.

kenchris · 2017-10-02T08:53:01Z

I discussed some of this processing with @tobie so he might have ideas

tobie · 2017-10-02T09:13:29Z

So "parse JSON from bytes" => ES => IDL Conversion. That's easy. What you do after depends on a lot of factors. What you want to work with (JSON, WebIDL, JS?), wether there's going to be JS APIs involved in the future, etc.

mgiuca

Hi Ken,

I've done a fairly thorough review of the changes. Here are my thoughts.

mgiuca · 2017-10-03T02:21:41Z

index.html

-          result of running the <a>steps for processing the
-          <code>orientation</code> member</a> with <var>manifest</var> and
-          <var>display mode</var> as arguments.
+          <li>Let <a>WebAppManifest</a> <var>manifest</var> be the result of


I think you should not literally say "converting json to IDL dictionary value"; although it's obvious from context, that doesn't explicitly state which dictionary type you are converting to. (In WebIDL, the algorithm is called "An ECMAScript value V is converted to an IDL dictionary type value" -- emphasis mine, meaning, this is the algorithm whenever a value is converted to a type which is declared as a dictionary).

So you should write:

Let <var>manifest</var> be the result of <a data-dfn="to-idl-dictionary-value">converting</a> <var>json</var> to a <a>WebAppManifest</a>.

@tobie Could you confirm?

Yes, your want to convert to that particular dictionary type. Not dictionaries in general. The json var is an ES object at this point, right?

yes, it should be - that is at least the intent :-)

mgiuca · 2017-10-03T02:39:16Z

index.html

-          <li>Let <var>language</var> of <var>parsed manifest</var> be the
-          result of running the <a>steps for processing the <code>lang</code>
+          <li>Let <var>manifest["<a>display</a>"]</var> be the result of
+          running the <a>steps for post-processing the <code>display</code>


Why not have the "steps for post-processing" each field take the field value, instead of the entire manifest?

So here it would say "running the steps for post-processing the display member with manifest["display"] as the argument", and then later on where that is defined, it would say "The algorithm takes a DisplayModeType value as an argument, and returns a DisplayModeType." Seems a bit cleaner this way.

This is a really good suggestion... now that we have explicit types, we should definitely make use of them.

I think that is a good idea too, but let's make that a follow up patch.

Up to you, but it seems it would actually make this patch smaller if you did it now (i.e., you would delete more lines without adding any).

mgiuca · 2017-10-03T02:42:07Z

index.html

      </h2>
+      <pre class="idl">
+          dictionary WebAppManifest {
+             TextDirectionType? dir;


Why are all of these types nullable (ending in ?)?

Dictionary members are always optional unless otherwise specified. Adding ? means the user can explicitly specify null as a value, which I don't think we want. So I would remove all the ?s.

Because you don't have to have these declared in your manifest, they are optional. I was looking for optional in the web idl spec, but I see your point that they are all optional because of them being in a dict. This means that I will have to change most of the algorithms again though :)

We've all made this mistake before :)

mgiuca · 2017-10-03T02:55:25Z

index.html

-          result of running the <a>steps for processing the
-          <code>description</code> member</a> with <var>manifest</var> as the
-          argument.
+          <li>Let <var>manifest["<a>start_url</a>"]</var> be the result of


So we're removing the Trim() logic for all the strings (except categories... why?). I'm not really comfortable with this until we know whether it will break sites in the wild.

Chromium, at least, certainly does trim the strings, so we would presumably remove that logic if it was removed from the manifest. I think we need to do an analysis of how many real-world manifests contain leading or trailing whitespace in these strings, and whether it would be safe to remove the trim() step. I am looking into this now.

For now, I think we should leave it, which unfortunately means each of the strings needs a post-processing step to trim it. However, for all the trim-only members, how about instead of writing "running the steps for post-processing X", you just write "Let manifest[X] be the result of running Trim(manifest[X])". That's reasonably short.

Unfortunately, you can't trim enum values like "display" after converting to the WebAppManifest dictionary type, because it will already be a TypeError. Instead, you need to pre-process by trimming the string values in "json" before converting.

Pre-processing the JSON seems really ugly, but a good fallback. Let's see what the data from your analysis says about breakage.

Looks like we can avoid all this with this breaking trim change

mgiuca · 2017-10-03T02:59:22Z

index.html

+             sequence&lt;USVString&gt;? categories;
+             DOMString? iarc_rating_id;
+             USVString? start_url;
+             DisplayModeType? display;


I think this can have a default "= "browser"". See discussion below.

mgiuca · 2017-10-03T03:10:16Z

index.html

          with <var>manifest</var>, <var>manifest URL</var>, and "screenshots"
          as arguments.
          </li>
-          <li>Return <var>parsed manifest</var>.
+          <li>Let <var>manifest["<a>related_applications</a>"]</var> be the
+          result of running the <a>steps for processing the


Why are this, and the next, ones called "processing" rather than "post-processing"?

mgiuca · 2017-10-03T03:13:11Z

index.html

-                  <li>Let <var>id</var> be the result of running the <a>steps
-                  for processing the <code>id</code> member of an
-                  application</a> with <var>potential application</var>.
+                  <li>Let <var>V'</var> be a new object created as if by the


Can this just destructively update V?

Just delete Steps 1--4, and use V instead of V'. Should be equivalent, and simpler.

We could, I guess. @marcoscaceres ?

Agree... I'm not sure what "'" signifies there.

Very common in math books :-) but let me change

Having an X' as the updated value for X is fine. The issue here is that you went to all this trouble to create a clone which isn't necessary if you just updated it.

Again, this looks good now and no further action required.

mgiuca · 2017-10-03T03:17:54Z

index.html

-                </ol>
-              </li>
-            </ol>
+          <li>Initialize <var>i</var> to be 0.


Does this have to be an index-based loop? Can you just write "For each element of A, e: Trim(e) and convert to ASCII lowercase"? (Optionally, "updating the value in A" if you are worried about someone taking it too literally.)

Nope. Actually WebIDL uses it this way and I was looking for examples on how to write it properly without doing indices. @tobie is there a preferred wording?

As a general rule, don't copy WebIDL's style... it's a very old spec and exhibits a ton of really bad practices (for legacy reasons).

mgiuca · 2017-10-03T03:18:28Z

index.html

              <li>
-                <a>Issue a developer warning</a> that the type is not
-                supported.
+                <a>Trim</a>(<var>V</var>) and convert <a>to ASCII lowercase</a>


Curious that this is the last remaining Trim(). As I said above, I think we should keep all the Trims, but if you are certain of taking them out, take this one out too.

I missed that one :-)

mgiuca · 2017-10-03T03:20:58Z

index.html

-          <a><code>categories</code> member</a>. However, the working group
-          maintains a <a href=
+          This specification does not define the particular values for
+          <dfn>USVString</dfn>s for a the <a>categories</a> member. However,


Drop the "a".

marcoscaceres · 2017-10-03T08:18:54Z

index.html

          </li>
-          <li>If <var>unprocessed applications</var> is an <a>array</a>, then:
+          <li>For <var>V</var> in <var>A</var>:


Nit: please use for each, IDL sequences are just arrays, so it's nice to keep the language that maps to things in JS. Also, please don't use the bracket syntax, just use:

For each <var>relatedApp</var> in <var>manifest</var>.<a>related_applications</a>.

Then we get a link back to related_applications, and don't need to guess on the type.

fixed markup above.

That translates nicely to:

for(const relatedApp of manifest.related_applications) {... do stuff ... }

Or, better:

manifest.related_applications.forEach(relatedApp => ... do stuff... )

@tobie said that we cannot use dot notation for these as it is WebIDL, have to use []

I don’t think that’s correct. Or, if it is, I’m inclined to willfully violate that. We do the same in Payment Request. And I’ve never seen any other spec use [] notation.

Apparently dot notation for Web IDL is not spec'ed. I went with brackets for now, but it will be easy to change (like in follow up) when dot notation gets spec'ed

Can you open a ticket against WebIDL for that? Thanks.

Done: whatwg/webidl#453

mgiuca · 2017-10-04T03:51:48Z

index.html

+             USVString background_color;
+             USVString scope;
+             ServiceWorkerRegistration serviceworker;
+             sequence&lt;ExternalApplicationResource&gt; related_applications;


= false (then remove the equivalent sentence down in its section).

mgiuca · 2017-10-04T04:48:14Z

index.html

-             ServiceWorkerRegistration? serviceworker;
-             sequence&lt;ExternalApplicationResource&gt;? related_applications;
-             boolean? prefer_related_applications;
+             TextDirectionType dir;


mgiuca · 2017-10-04T04:50:25Z

index.html

-             boolean? prefer_related_applications;
+             TextDirectionType dir;
+             DOMString lang;
+             USVString name;


Thought bubble: This should possibly be required (though let's not change it right now because the existing algorithm doesn't require it, and I want to change the semantics as little as possible).

Ok, let me file an issue

mgiuca · 2017-10-04T04:55:19Z

index.html

-          <li>Let <var>language</var> of <var>parsed manifest</var> be the
-          result of running the <a>steps for processing the <code>lang</code>
+          <li>Let <var>manifest["<a>display</a>"]</var> be the result of
+          running the <a>steps for post-processing the <code>display</code>


Up to you, but it seems it would actually make this patch smaller if you did it now (i.e., you would delete more lines without adding any).

mgiuca

A couple more things can be made default. Then I am quite happy with this.

marcoscaceres · 2017-10-13T05:08:06Z

I'm worried about the use of enums invalidating the whole manifest... We might need to make those DOMStrings instead of enums (particularly for backwards compat in the future).

marcoscaceres

I'm ok to go with this for now - we can experiment a bit. We need to start adding test tho.

kenchris · 2017-10-13T06:05:57Z

Yes, I agree. Didn't you have some tests we can start with?

marcoscaceres · 2017-10-13T06:09:07Z

The tests I have are very Gecko specific. We will need to write new ones. It will be a good opportunity to review the spec tho.

kenchris requested a review from marcoscaceres September 20, 2017 15:44

marcoscaceres mentioned this pull request Sep 21, 2017

Add IDL section and define IDL for all of the members #612

Closed

mgiuca approved these changes Sep 21, 2017

View reviewed changes

marcoscaceres requested changes Sep 21, 2017

View reviewed changes

marcoscaceres requested changes Sep 22, 2017

View reviewed changes

marcoscaceres mentioned this pull request Sep 22, 2017

Added missing space. #616

Closed

kenchris added 3 commits September 25, 2017 14:28

Add IDL section and define IDL for all the members

2dcc42c

Review comments

4412840

More review fixes

8018ba6

kenchris force-pushed the idl branch from 7984608 to 8018ba6 Compare September 25, 2017 12:28

kenchris force-pushed the idl branch 2 times, most recently from 2df72da to 1e6068f Compare September 28, 2017 16:06

Add IDL processing

50541f1

kenchris force-pushed the idl branch from 1e6068f to 50541f1 Compare September 28, 2017 16:08

marcoscaceres reviewed Oct 2, 2017

View reviewed changes

mgiuca requested changes Oct 3, 2017

View reviewed changes

marcoscaceres reviewed Oct 3, 2017

View reviewed changes

Fix review comments from Matt

278816f

mgiuca reviewed Oct 4, 2017

View reviewed changes

mgiuca requested changes Oct 4, 2017

View reviewed changes

kenchris added 4 commits October 10, 2017 11:51

Fix new comments from Matt

e4bfa1c

Fix fingerprints error

36d8a43

Give values directly to algorithms

f9f5fba

Review comments from Tobie

54af834

marcoscaceres approved these changes Oct 13, 2017

View reviewed changes

kenchris merged commit fdb8dfb into w3c:gh-pages Oct 13, 2017

mgiuca mentioned this pull request Sep 20, 2019

IDL: Make name required #622

Closed

		@@ -1612,45 +1633,49 @@ <h3 id="applying">
		<code>dir</code> member

		@@ -3472,11 +3517,20 @@ <h3 id="applying">
		</section>
		<section>

Add IDL section and define IDL for all the members #613

Add IDL section and define IDL for all the members #613

Conversation

kenchris commented Sep 20, 2017 • edited by pr-preview bot Loading

kenchris commented Sep 20, 2017

mgiuca 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

marcoscaceres 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

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

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

marcoscaceres commented Sep 26, 2017

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

marcoscaceres commented Oct 2, 2017

marcoscaceres commented Oct 2, 2017

kenchris commented Oct 2, 2017

tobie commented Oct 2, 2017 • edited Loading

mgiuca 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

kenchris commented Sep 20, 2017 •

edited by pr-preview bot

Loading

tobie commented Oct 2, 2017 •

edited

Loading