Re: [manifest] Manifest for web application; review ...
> On 17 Jun 2015, at 18:11, Kostiainen, Anssi > wrote: > >> On 02 Jun 2015, at 00:58, timeless wrote: >> >> http://www.w3.org/TR/2015/WD-appmanifest-20150212/ > > Thanks for the detailed review. The pull request with you review feedback > baked in is at: > > https://github.com/w3c/manifest/pull/383 > > The changes should land after being reviewed, assuming no concerns. The changes you proposed have landed: http://w3c.github.io/manifest/ [...] >>> 8.4 src member >>> Let value be the result of calling the [[GetOwnProperty]] internal method >>> of icon passing "src" as the argument. >>> Let type be Type(value). >>> If type is not "string", >>> or value is the empty string, then: >> ^ this specific or branch isn't really necessary, one could just fall >> through to the Trim path... >>> If type is not "undefined", issue a developer warning that the type is >>> unsupported. >>> Return undefined. >>> If Trim(value) is the empty string, then return undefined. >> >> (I didn't read past this point) Any further feedback would be much appreciated. For example, a similar, very detailed review for the remaining parts of the document. Thanks, -Anssi
Re: [manifest] Manifest for web application; review ...
Hi,
> On 02 Jun 2015, at 00:58, timeless wrote:
>
> http://www.w3.org/TR/2015/WD-appmanifest-20150212/
Thanks for the detailed review. The pull request with you review feedback baked
in is at:
https://github.com/w3c/manifest/pull/383
The changes should land after being reviewed, assuming no concerns.
>> However, installed web applications and their data could be seen as "high
>> value" (particulaly [sic] from a privacy perspective).
>
>> For instance, the web application:
>> contains a manifest with at least a name member and a suitable icon.
> ...
>> has been explicitly marked by the user as one that they value and trust
>> (e.g., by bookmarking or "staring" it).
>
> Drop:
>> And so on...
Done.
> ... it isn't a "for instance".
Reworded.
>> has been explicitly marked by the user as one that they value and trust
>> (e.g., by bookmarking or "staring" [sic] it).
>
> "starring"
Fixed (already earlier).
>> If the URL being navigated to is not within scope of the navigation scope,
>
> `being navigated to` is awkward.
Fixed.
>> then the user agent MUST behave as if the application context is not allowed
>> to navigate:
>
> The `:` here is odd.
Dropped.
>> this provides the ability for the user agent to perform the navigation in a
>> different browsing context
>> - or in a different user agent entirely.
>
>> If during the handle redirects step of HTML's navigate algorithm,
>> if the redirect URL is not within scope,
>
> You can drop the second `if`, you're in an `If`.
Removed.
>> abort HTML's navigation algorithm with a SecurityError.
>
>
>> In such a case, the manifest is applied to all URLs the application context
>> is navigated to (see related security considerations).
>
> The parenthetical should link somewhere. (Even if it's just the next section)
Added a link.
>> In particular, if a scope member is declared in the manifest,
>> it is not possible to navigate the top-level browsing context to somewhere
>> outside the scope while the manifest is being applied.
>
> `is being` is awkward, you should be able to drop `being`.
Fixed.
>> The concept of a deep link is useful in that it allows hyperlinking from one
>> installed application to another.
>
> There should be a red warning to people that this can happen and thus
> that they shouldn't assume that such a navigation was intentionally
> initiated by the user for the purpose they think it was
> (This may be obvious to you/me, but it won't be to developers)
Added a note.
>> Each display mode, except browser, has a recursive fallback display mode,
>> which is the display mode that the user agent can try to use if it doesn't
>> support a particular display mode.
>
> recursive => iterative (?) -- it isn't really recursing, it just has a
> fallback.
Dropped "resursive".
>> 7.4 icons member
>> Otherwise, if unprocessed icons is not undefined:
>> Issue a developer warning that the type is not supported.
>
> If this path isn't an extension point, then having it return early (as
> in 7.3 scope member) would be easier to read.
A fix for this was already landed as the algorithm was reworked.
>> If value is not part of the display modes values , issue a developer warning
>> that the value is unsupported.
>
> There's a stray whitespace before `,`
Fixed.
>> 7.5 display member
>
>> If Type(value) is not "string" or value is not part of the display modes
>> values:
>
> You didn't "Trim" before performing the "part of" (undefined)
> operation (you trim below):
Added the missing Trim().
>> Otherwise, Trim(value) and set value to be the result.
>
>> The possible values is one of the OrientationLockType enum defined in
>> [SCREEN-ORIENTATION].
>
> is one => are those
Fixed.
>> 7.7 start_url member
>
>> A user agent MAY also allow the end-user to modify the URL when, for
>> instance, a bookmark for the web application is being create [sic] or any
>> time thereafter.
>
> create => created
>
>> If type is not "string" or value is the empty string, then:
>> If type is not "undefined", issue a developer warning that the type is
>> unsupported.
>
> what if { start_url: " " } ?
> (I suppose it could be technically valid, although I question that...)
I believe we'd like to rely on the URL parsing defined in the URL spec. Please
let us know if we should make this a special case.
>> If type is not "string" or value is the empty string, then:
>> If url is failure:
>
> you inconsistently use `, then`
Fixed.
>> 8. Icon object and its members
>> For all intents and purposes, an icon object is functionally equivalent to
>> link element whose rel attribute is icon in a Document.
>
> should `icon` be quoted/marked up?
Was already fixed.
>> For example, the following policy restricts loading icons the
>> icons.example.com domain. Thus, trying to load icons from "other.com" would
>> fail.
>
> I can't follow this example, could you please provide an example
> manifest URL for the example?
Fixed the example,
[manifest] Manifest for web application; review ...
http://www.w3.org/TR/2015/WD-appmanifest-20150212/
> However, installed web applications and their data could be seen as "high
> value" (particulaly [sic] from a privacy perspective).
> For instance, the web application:
> contains a manifest with at least a name member and a suitable icon.
...
> has been explicitly marked by the user as one that they value and trust
> (e.g., by bookmarking or "staring" it).
Drop:
> And so on...
... it isn't a "for instance".
> has been explicitly marked by the user as one that they value and trust
> (e.g., by bookmarking or "staring" [sic] it).
"starring"
> If the URL being navigated to is not within scope of the navigation scope,
`being navigated to` is awkward.
> then the user agent MUST behave as if the application context is not allowed
> to navigate:
The `:` here is odd.
> this provides the ability for the user agent to perform the navigation in a
> different browsing context
> - or in a different user agent entirely.
> If during the handle redirects step of HTML's navigate algorithm,
> if the redirect URL is not within scope,
You can drop the second `if`, you're in an `If`.
> abort HTML's navigation algorithm with a SecurityError.
> In such a case, the manifest is applied to all URLs the application context
> is navigated to (see related security considerations).
The parenthetical should link somewhere. (Even if it's just the next section)
> In particular, if a scope member is declared in the manifest,
> it is not possible to navigate the top-level browsing context to somewhere
> outside the scope while the manifest is being applied.
`is being` is awkward, you should be able to drop `being`.
> The concept of a deep link is useful in that it allows hyperlinking from one
> installed application to another.
There should be a red warning to people that this can happen and thus
that they shouldn't assume that such a navigation was intentionally
initiated by the user for the purpose they think it was
(This may be obvious to you/me, but it won't be to developers)
> Each display mode, except browser, has a recursive fallback display mode,
> which is the display mode that the user agent can try to use if it doesn't
> support a particular display mode.
recursive => iterative (?) -- it isn't really recursing, it just has a fallback.
> 7.4 icons member
> Otherwise, if unprocessed icons is not undefined:
> Issue a developer warning that the type is not supported.
If this path isn't an extension point, then having it return early (as
in 7.3 scope member) would be easier to read.
> If value is not part of the display modes values , issue a developer warning
> that the value is unsupported.
There's a stray whitespace before `,`
> 7.5 display member
> If Type(value) is not "string" or value is not part of the display modes
> values:
You didn't "Trim" before performing the "part of" (undefined)
operation (you trim below):
> Otherwise, Trim(value) and set value to be the result.
> The possible values is one of the OrientationLockType enum defined in
> [SCREEN-ORIENTATION].
is one => are those
> 7.7 start_url member
> A user agent MAY also allow the end-user to modify the URL when, for
> instance, a bookmark for the web application is being create [sic] or any
> time thereafter.
create => created
> If type is not "string" or value is the empty string, then:
> If type is not "undefined", issue a developer warning that the type is
> unsupported.
what if { start_url: " " } ?
(I suppose it could be technically valid, although I question that...)
> If type is not "string" or value is the empty string, then:
> If url is failure:
you inconsistently use `, then`
> 8. Icon object and its members
> For all intents and purposes, an icon object is functionally equivalent to
> link element whose rel attribute is icon in a Document.
should `icon` be quoted/marked up?
> For example, the following policy restricts loading icons the
> icons.example.com domain. Thus, trying to load icons from "other.com" would
> fail.
I can't follow this example, could you please provide an example
manifest URL for the example?
> 8.2 density member
> The algorithm thanks [sic] an icon object as an argument and returns a
> positive number.
You're welcome.
> 1.1 Return 1.0.
> 4.1 Issue a developer warning.
> 4.2 Let result be 1.0.
Any particular reason to Let instead of Return? (you returned earlier)
> 5 Return result.
> 8.3 sizes member
> When multiple icon objects are available,
> a user agent can use the value to decide which icon is most suitable for a
> display context
Any reason this is `can` instead of `may`?
> 4 If type is not "string", then:
> 4.1 If type is not "undefined", issue a developer warning that the type is
> unsupported.
Normally you return, why not here?
> 8.4 src member
> Let value be the result of calling the [[GetOwnProperty]] internal method of
> icon passing "src" as the argument.
> Let type be Type(value).
> If t
