Hi, > On 02 Jun 2015, at 00:58, timeless <timel...@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