Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1127273004:
Update documentation for "file_system_provider" manifest section. (Closed)
|
Created:
5 years, 7 months ago by mtomasz Modified:
5 years, 7 months ago Reviewers:
not at google - send to devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, benwells Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate documentation for "file_system_provider" manifest section.
TEST=Checked manually with a test server.
BUG=None
Committed: https://crrev.com/16659538bfb5de7212c3b97d1ce5e2ea03fc7b2a
Cr-Commit-Position: refs/heads/master@{#329582}
Patch Set 1 #Patch Set 2 : Fixed. #
Total comments: 2
Patch Set 3 : Fixed. #
Total comments: 3
Patch Set 4 : Fixed + merged with 1084283004 #
Total comments: 12
Patch Set 5 : Fixes. #
Total comments: 4
Patch Set 6 : Fixed. #
Messages
Total messages: 21 (6 generated)
mtomasz@chromium.org changed reviewers: + benwells@chromium.org
@benwells: PTAL. Thanks.
Is this section implemented already?
It's in CQ: https://codereview.chromium.org/1077823005/
benwells@chromium.org changed reviewers: + kalman@chromium.org - benwells@chromium.org
benwells@chromium.org changed reviewers: + benwells@chromium.org
ok then ... -me +kalman since he reviewed that CL and knows about this change more than I do ... https://codereview.chromium.org/1127273004/diff/20001/chrome/common/extension... File chrome/common/extensions/docs/templates/articles/manifest/file_system_provider.html (right): https://codereview.chromium.org/1127273004/diff/20001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/manifest/file_system_provider.html:1: <h1 id="file_handlers">Manifest - File Handlers</h1> Is this meant to be the app file handler documentation?
https://codereview.chromium.org/1127273004/diff/20001/chrome/common/extension... File chrome/common/extensions/docs/templates/articles/manifest/file_system_provider.html (right): https://codereview.chromium.org/1127273004/diff/20001/chrome/common/extension... chrome/common/extensions/docs/templates/articles/manifest/file_system_provider.html:1: <h1 id="file_handlers">Manifest - File Handlers</h1> On 2015/05/08 03:18:08, benwells wrote: > Is this meant to be the app file handler documentation? Accidental change. Reverted.
https://codereview.chromium.org/1127273004/diff/40001/chrome/common/extension... File chrome/common/extensions/docs/templates/intros/fileSystemProvider.html (right): https://codereview.chromium.org/1127273004/diff/40001/chrome/common/extension... chrome/common/extensions/docs/templates/intros/fileSystemProvider.html:2: <p>You must declare the "fileSystemProvider" permission and section Is "and" actually true here, or is it an "and/or" situation? If the former, without questioning why, you should make the manifest key imply requesting the permission (in fact, you could consider doing this anyway). If the latter, you should briefly comment that in the manifest.json section. People just cargo-cult code and if the sample is as you've written it, that's what people will write. Generally speaking for this documentation, it's unclear to me why I'd want to specify the permission vs the manifest, and in fact what the manifest section does. What you've got here is a bit improvement - at least it's *present* in the documentation - but it'd be nice to see these things fleshed out. Also, some issues in the docs I see generated in https://chrome-apps-doc.appspot.com/_patch/1127273004/apps/fileSystemProvider """ The file_system_provider section must be declared as follows: configurable (boolean) - required Whether configuring via onConfigureRequested is supported. multiple_mounts (boolean) - required Whether multiple (more than one) mounted file systems are supported. source (enum of "file", "device", or "network") - required For file the source is a file passed via onLaunched event. For device contents are fetched from an external device (eg. plugged via USB), without using file_handlers. Finally, for device source, contents should be fetched via network. """ Problems: (a) I can't find this onConfigureRequested function, because it's nodoc. You should un-nodoc it, and also, use $(ref:onConfigureRequested) so that it generates a link. (b) The onLaunched event is in runtime, so likewise use $(ref:runtime.onLaunched). (c) file/network/device should be "file"/"network"/"device"
https://codereview.chromium.org/1127273004/diff/40001/chrome/common/extension... File chrome/common/extensions/docs/templates/intros/fileSystemProvider.html (right): https://codereview.chromium.org/1127273004/diff/40001/chrome/common/extension... chrome/common/extensions/docs/templates/intros/fileSystemProvider.html:2: <p>You must declare the "fileSystemProvider" permission and section On 2015/05/08 16:47:39, kalman wrote: > Is "and" actually true here, or is it an "and/or" situation? It's currently optional, as I wanted to keep backward compatibility while encouraging new developers to declare both. However, I guess it's OK to break apps on dev, so let me make it mandatory in a follow up CL. > > If the former, without questioning why, you should make the manifest key imply > requesting the permission (in fact, you could consider doing this anyway). > > If the latter, you should briefly comment that in the manifest.json section. > People just cargo-cult code and if the sample is as you've written it, that's > what people will write. > > Generally speaking for this documentation, it's unclear to me why I'd want to > specify the permission vs the manifest, and in fact what the manifest section > does. What you've got here is a bit improvement - at least it's *present* in the > documentation - but it'd be nice to see these things fleshed out. Permission is simply for the API. And the section specifies some extras. We do the same for fileBrowserHandler API. Do you suggest to (1) automatically imply a permission when "file_system_provider" section is present or (2) rename the manifest section to something more meaningful, like "file_system_provider_capabilities"/"provider_capabilities"? > > Also, some issues in the docs I see generated in > https://chrome-apps-doc.appspot.com/_patch/1127273004/apps/fileSystemProvider > > > """ > The file_system_provider section must be declared as follows: > > configurable (boolean) - required > > Whether configuring via onConfigureRequested is supported. > > multiple_mounts (boolean) - required > > Whether multiple (more than one) mounted file systems are supported. > > source (enum of "file", "device", or "network") - required > > For file the source is a file passed via onLaunched event. For device contents > are fetched from an external device (eg. plugged via USB), without using > file_handlers. Finally, for device source, contents should be fetched via > network. > """ > > Problems: > (a) I can't find this onConfigureRequested function, because it's nodoc. You > should un-nodoc it, and also, use $(ref:onConfigureRequested) so that it > generates a link. Done. Merged both CLs. > (b) The onLaunched event is in runtime, so likewise use > $(ref:runtime.onLaunched). Done. > (c) file/network/device should be "file"/"network"/"device" Done.
Some more little comments, lgtm, if you want to do things like the screenshot then fine as a follow-up to keep reviews rolling here. However, if there is somebody more knowledgeable about the fSP API that can take a look at this, that would be nice. https://codereview.chromium.org/1127273004/diff/40001/chrome/common/extension... File chrome/common/extensions/docs/templates/intros/fileSystemProvider.html (right): https://codereview.chromium.org/1127273004/diff/40001/chrome/common/extension... chrome/common/extensions/docs/templates/intros/fileSystemProvider.html:2: <p>You must declare the "fileSystemProvider" permission and section > rename the manifest section to something more meaningful, like "file_system_provider_capabilities"/"provider_capabilities" looks like you did that? FWIW I would also have been ok with keeping it "file_system_provider" and make that imply the "fileSystemProvider" permission; you'd then document the permission as "implying the file_system_provider manifest key with default values of <whatever>". https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... File chrome/common/extensions/api/file_system_provider.idl (right): https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... chrome/common/extensions/api/file_system_provider.idl:32: // Mode of opening a file. Used by <code>onOpenFileRequested</code>. These should be $(ref:onOpenFileRequested) (there are many instances in this file). https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... File chrome/common/extensions/api/manifest_types.json (right): https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... chrome/common/extensions/api/manifest_types.json:186: "description": "Whether configuring via <code>onConfigureRequested</code> is supported." configurable/multiple_mounts should actually be optional, with omission implying false. I didn't notice this until looking at the generated docs. Is there a reasonable default for source? Having it required is ok I suppose. https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... File chrome/common/extensions/docs/templates/intros/fileSystemProvider.html (right): https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... chrome/common/extensions/docs/templates/intros/fileSystemProvider.html:8: "name": "My {{?is_apps}}app{{:is_apps}}extension{{/is_apps}}", you should be able to use {{platform}} for this, it already does the extension/apps decision. https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... chrome/common/extensions/docs/templates/intros/fileSystemProvider.html:27: </p> It would be nice to mention somewhere what the difference is in the UI, e.g. screenshots are great. https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... chrome/common/extensions/docs/templates/intros/fileSystemProvider.html:51: In order to write file systems which are file handlers (source is <code>"file" Try to get the "file" part on a single line, like (source is <code>"file"</code>) so that it doesn't render with a space between the "file" and the closing parenthesis. https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... chrome/common/extensions/docs/templates/intros/fileSystemProvider.html:52: </code>) the provider must be a packaged app, as the <code>onLaunched</code> provide the full path to onLaunched, to make it clear.
https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... File chrome/common/extensions/api/file_system_provider.idl (right): https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... chrome/common/extensions/api/file_system_provider.idl:32: // Mode of opening a file. Used by <code>onOpenFileRequested</code>. On 2015/05/12 00:08:51, kalman wrote: > These should be $(ref:onOpenFileRequested) (there are many instances in this > file). I didn't know we can do that in IDL/json. Done. https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... File chrome/common/extensions/api/manifest_types.json (right): https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... chrome/common/extensions/api/manifest_types.json:186: "description": "Whether configuring via <code>onConfigureRequested</code> is supported." On 2015/05/12 00:08:51, kalman wrote: > configurable/multiple_mounts should actually be optional, with omission implying > false. I didn't notice this until looking at the generated docs. > > Is there a reasonable default for source? Having it required is ok I suppose. Not really a good default for source. Done in a separate CL: https://codereview.chromium.org/1127283003/ https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... File chrome/common/extensions/docs/templates/intros/fileSystemProvider.html (right): https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... chrome/common/extensions/docs/templates/intros/fileSystemProvider.html:8: "name": "My {{?is_apps}}app{{:is_apps}}extension{{/is_apps}}", On 2015/05/12 00:08:51, kalman wrote: > you should be able to use {{platform}} for this, it already does the > extension/apps decision. Done. https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... chrome/common/extensions/docs/templates/intros/fileSystemProvider.html:27: </p> On 2015/05/12 00:08:51, kalman wrote: > It would be nice to mention somewhere what the difference is in the UI, e.g. > screenshots are great. UI is still evolving. Currently I added some short information about "configure" and "multiple_mounts". Once the UI is stabilized I'll add screenshots. https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... chrome/common/extensions/docs/templates/intros/fileSystemProvider.html:52: </code>) the provider must be a packaged app, as the <code>onLaunched</code> On 2015/05/12 00:08:51, kalman wrote: > provide the full path to onLaunched, to make it clear. Could you clarify?
mtomasz@chromium.org changed reviewers: - benwells@chromium.org
Dones. @kalman: PTAL one more time, as there were plenty of changes. Thanks.
lgtm https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... File chrome/common/extensions/docs/templates/intros/fileSystemProvider.html (right): https://codereview.chromium.org/1127273004/diff/60001/chrome/common/extension... chrome/common/extensions/docs/templates/intros/fileSystemProvider.html:52: </code>) the provider must be a packaged app, as the <code>onLaunched</code> On 2015/05/12 08:01:19, mtomasz wrote: > On 2015/05/12 00:08:51, kalman wrote: > > provide the full path to onLaunched, to make it clear. > > Could you clarify? I mean that onLaunched is an event in chrome.app.runtime, but this makes it looks like it's an event in fSP. You can use $(ref:app.runtime.onLaunched) to fix it. https://codereview.chromium.org/1127273004/diff/80001/chrome/common/extension... File chrome/common/extensions/api/file_system_provider.idl (right): https://codereview.chromium.org/1127273004/diff/80001/chrome/common/extension... chrome/common/extensions/api/file_system_provider.idl:388: // Options for the <code>Notify) method. typo? https://codereview.chromium.org/1127273004/diff/80001/chrome/common/extension... File chrome/common/extensions/docs/templates/intros/fileSystemProvider.html (right): https://codereview.chromium.org/1127273004/diff/80001/chrome/common/extension... chrome/common/extensions/docs/templates/intros/fileSystemProvider.html:17: <a name="manifest-source"></a>"source": "network" is this to get an anchor target? Another reasonable way to do that is with <span id="manifest-source">...</span>, i'm not sure which is more idiomatic, but creating an empty <a> element always seemed a bit weird to me.
https://codereview.chromium.org/1127273004/diff/80001/chrome/common/extension... File chrome/common/extensions/api/file_system_provider.idl (right): https://codereview.chromium.org/1127273004/diff/80001/chrome/common/extension... chrome/common/extensions/api/file_system_provider.idl:388: // Options for the <code>Notify) method. On 2015/05/12 20:58:34, kalman wrote: > typo? Oops. Done. https://codereview.chromium.org/1127273004/diff/80001/chrome/common/extension... File chrome/common/extensions/docs/templates/intros/fileSystemProvider.html (right): https://codereview.chromium.org/1127273004/diff/80001/chrome/common/extension... chrome/common/extensions/docs/templates/intros/fileSystemProvider.html:17: <a name="manifest-source"></a>"source": "network" On 2015/05/12 20:58:34, kalman wrote: > is this to get an anchor target? Another reasonable way to do that is with <span > id="manifest-source">...</span>, i'm not sure which is more idiomatic, but > creating an empty <a> element always seemed a bit weird to me. Good point. <a name> seems deprecated in html5. Replaced with <span id>. Done.
The CQ bit was checked by mtomasz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1127273004/#ps100001 (title: "Fixed.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1127273004/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/16659538bfb5de7212c3b97d1ce5e2ea03fc7b2a Cr-Commit-Position: refs/heads/master@{#329582} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
