Re: [manifest] Manifest for web application; review ...

Hi,

> On 02 Jun 2015, at 00:58, timeless <timeless@gmail.com> 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, made it more elaborate for clarity.

>> 8.2 density member
>> The algorithm thanks [sic] an icon object as an argument and returns a positive number.
> 
> You're welcome.

Was already fixed.

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

Fixed with Return.

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

Fixed, MAY is more suitable given this is in normative prose.

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

Should return here as well, as "parse value as if it was a [HTML] sizes attribute" if the value is not "string" does not make sense. Fixed.

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

Thanks,

-Anssi

Received on Wednesday, 17 June 2015 15:12:14 UTC