|
|
Created:
6 years, 6 months ago by Mark Dittmer Modified:
6 years, 6 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd webview:partitions usage to webview documentation
We currently implement a manifest configuration option: { ... webview: {
partitions: { ... } } }, but it is undocumented. This change adds it to the
introductory sections of the webview tag documentation.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274668
Patch Set 1 #
Total comments: 15
Patch Set 2 : Rewrite of resources-packaged-within-app documentation in response to code review comments #
Total comments: 21
Patch Set 3 : Added partition name pattern example to <webview>: "accessing packaged resources" documentation. Al… #Patch Set 4 : Link to partition subsection from packaged resources description #Messages
Total messages: 18 (0 generated)
I love it! Thanks Mark!
lgtm
The CQ bit was checked by markdittmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/markdittmer@chromium.org/308903002/1
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
Hey Ben, I'm Fady's intern this summer. I need an owner code review for this documentation change.
https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/templates/intros/webview_tag.html (right): https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/templates/intros/webview_tag.html:134: <h2 id="local_resources">Accessing package-local resources</h2> what does "package-local" mean? (rhetorical) https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/templates/intros/webview_tag.html:146: */ we typically just show some stub name and then ..., see like https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/templates/intros/webview_tag.html:147: permissions: [ please make sure this is valid json https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/templates/intros/webview_tag.html:168: manifest. List items may contain wildcards. In the above example all webviews don't repeat "above example" I think much of this paragraph could be included inline with the code samples, like // Webviews with "trusted" or "persist:trusted" may load any resource biginning in "local_" and ending in ".html", and any resource ending in ".png" or ".js". https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/templates/intros/webview_tag.html:173: where <code>partition="trusted"</code> or we attach specific meanings to the names of partitions... hmm. https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/templates/intros/webview_tag.html:176: in <code>.png</code> or <code>.js</code>. By omission, all other webviews my understanding of this paragraph is that the "*.png" and "*.js" in the example is implicit, so confusing including them (either that is confusing, or this is confusing). assuming my understanding of this paragraph is correct, you could say above: { // explanation of trusted "name": "trusted", // *.js, *.png, and .. are automatically whitelisted. // "accessible_resources": [...] // Anything else needs to be listed explicitly. ] } https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/templates/intros/webview_tag.html:177: cannot load any package-local resources. "by ommission..." should be right up the top, not at the end, because it's the entire point of this feature: <p> By default, webviews a prevented from loading any resources packaged with the app. However, a <code>"webview"</code> manifest key can be specified configuring <a href="link/to/where/partitions/are/explained">partitions</a> to access a list of accessible packaged resources. </p> <pre> .. the example </pre>
Addressed most comments. Still unsure as to whether the example code should be more abstract to avoid assumption that the chosen names, "static" and "trusted", have some special status. https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/templates/intros/webview_tag.html (right): https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/templates/intros/webview_tag.html:134: <h2 id="local_resources">Accessing package-local resources</h2> Do we have an existing piece of jargon for "files packaged within your application?" I tried to clarify what this means with the first sentence below. Should I, also, change this heading to "Accessing resources packaged in the app" or similar? On 2014/06/02 17:00:18, kalman wrote: > what does "package-local" mean? > > (rhetorical) https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/templates/intros/webview_tag.html:146: */ On 2014/06/02 17:00:18, kalman wrote: > we typically just show some stub name and then ..., see like > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... Done. https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/templates/intros/webview_tag.html:147: permissions: [ On 2014/06/02 17:00:18, kalman wrote: > please make sure this is valid json Done. https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/templates/intros/webview_tag.html:173: where <code>partition="trusted"</code> or The developer may or may not attach specific meaning to partitions. The example is intended to provide a practical naming of partitions. Should the example be more abstract (e.g., partitions "alpha" and "beta" allowed to access "foo.html" and "bar*.*"?) On 2014/06/02 17:00:18, kalman wrote: > we attach specific meanings to the names of partitions... hmm. https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/templates/intros/webview_tag.html:176: in <code>.png</code> or <code>.js</code>. By omission, all other webviews I don't understand your comment, so clearly the description is confusing. I've moved it to comments in the example code. Let me know if that clears things up. On 2014/06/02 17:00:18, kalman wrote: > my understanding of this paragraph is that the "*.png" and "*.js" in the example > is implicit, so confusing including them (either that is confusing, or this is > confusing). > > assuming my understanding of this paragraph is correct, you could say above: > > { > // explanation of trusted > "name": "trusted", > // *.js, *.png, and .. are automatically whitelisted. > // "accessible_resources": [...] > // Anything else needs to be listed explicitly. > ] > } > https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/templates/intros/webview_tag.html:177: cannot load any package-local resources. On 2014/06/02 17:00:18, kalman wrote: > "by ommission..." should be right up the top, not at the end, because it's the > entire point of this feature: > > <p> > By default, webviews a prevented from loading any resources packaged with the > app. However, a <code>"webview"</code> manifest key can be specified configuring > <a href="link/to/where/partitions/are/explained">partitions</a> to access a list > of accessible packaged resources. > </p> > > <pre> > .. the example > </pre> Done.
https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... File chrome/common/extensions/docs/templates/intros/webview_tag.html (right): https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:158: name: "static", Partitions can also have patterns. It might be useful to also show an example of this. C++ source: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/templates/intros/webview_tag.html (right): https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/templates/intros/webview_tag.html:134: <h2 id="local_resources">Accessing package-local resources</h2> On 2014/06/02 18:39:51, Mark Dittmer wrote: > Do we have an existing piece of jargon for "files packaged within your > application?" I tried to clarify what this means with the first sentence below. > Should I, also, change this heading to "Accessing resources packaged in the app" > or similar? > > On 2014/06/02 17:00:18, kalman wrote: > > what does "package-local" mean? > > > > (rhetorical) > "packaged resources" seems like a fine term to me. https://codereview.chromium.org/308903002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/templates/intros/webview_tag.html:173: where <code>partition="trusted"</code> or On 2014/06/02 18:39:51, Mark Dittmer wrote: > The developer may or may not attach specific meaning to partitions. The example > is intended to provide a practical naming of partitions. Should the example be > more abstract (e.g., partitions "alpha" and "beta" allowed to access "foo.html" > and "bar*.*"?) > > On 2014/06/02 17:00:18, kalman wrote: > > we attach specific meanings to the names of partitions... hmm. > ah. https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... File chrome/common/extensions/docs/templates/intros/webview_tag.html (right): https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:134: <h2 id="local_resources">Accessing resources packaged in the app</h2> so the concept of packaging should be familiar to developers, hence why I just suggested "[accessing] packaged resources". https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:139: packaged in the app via a <code>webview:partitions</code> section in the app webview.partitions not webview:partitions. webview.partitions looks like JS object dereferencing, which is correct. webview:partitions looks like an XML namespace :) https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:139: packaged in the app via a <code>webview:partitions</code> section in the app maybe I repeated "packaged in the app" twice in 2 sentences, but yeah, looks awkward. https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:142: following <code>manifest.json</code> snippet: put last sentence in a new paragraph https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:145: <pre> put data-filename="manifest.json" on this and it will generate some nicer docs. https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:147: name: "My extension", this is still not valid json. json has quotes around key names. (of course, comments are not valid json either, but let's just gloss over that) https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:154: // Webviews with partition "static" or "persist:static" may access regarding my confusion, the text gave me the impression that special significance was attached to names. which is false. I still kind of get that impression here. Perhaps say like // In this example, any <webview partition="static"> will have access to header.html, footer.html, and static.png. https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:161: // Webviews with partition "trusted" or "persist:trusted" may access similar comment to above. also what does "persist" mean? https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:170: // resources. ditto "packaged resources". you might also want to mention what happens with "accessible_resources": []
Addressed all comments except the "accessible_resources":[] comment. https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... File chrome/common/extensions/docs/templates/intros/webview_tag.html (right): https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:134: <h2 id="local_resources">Accessing resources packaged in the app</h2> On 2014/06/02 18:54:11, kalman wrote: > so the concept of packaging should be familiar to developers, hence why I just > suggested "[accessing] packaged resources". Done. https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:139: packaged in the app via a <code>webview:partitions</code> section in the app On 2014/06/02 18:54:11, kalman wrote: > webview.partitions not webview:partitions. > > webview.partitions looks like JS object dereferencing, which is correct. > > webview:partitions looks like an XML namespace :) Done. https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:142: following <code>manifest.json</code> snippet: On 2014/06/02 18:54:11, kalman wrote: > put last sentence in a new paragraph Done. https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:145: <pre> On 2014/06/02 18:54:11, kalman wrote: > put data-filename="manifest.json" on this and it will generate some nicer docs. Done. https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:147: name: "My extension", Got it. Quotes added. I did feel like the back-to-back "just use ellipses" and "make sure this is valid json" were somewhat contradictory. On 2014/06/02 18:54:11, kalman wrote: > this is still not valid json. json has quotes around key names. > > (of course, comments are not valid json either, but let's just gloss over that) https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:154: // Webviews with partition "static" or "persist:static" may access On 2014/06/02 18:54:11, kalman wrote: > regarding my confusion, the text gave me the impression that special > significance was attached to names. which is false. I still kind of get that > impression here. > > Perhaps say like > > // In this example, any <webview partition="static"> will have access to > header.html, footer.html, and static.png. Done. https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:158: name: "static", On 2014/06/02 18:45:19, fsamuel wrote: > Partitions can also have patterns. It might be useful to also show an example of > this. C++ source: > > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/... Done. https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:161: // Webviews with partition "trusted" or "persist:trusted" may access The "persist:" prefix gives the webview persistent storage. Storage is shared among webviews with the same partition name. See https://developer.chrome.com/apps/tags/webview#partition. Even without wildcards, the configuration for "foo" is activated for both partition="foo" and partition="persist:foo". On 2014/06/02 18:54:11, kalman wrote: > similar comment to above. also what does "persist" mean? https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:170: // resources. You mean, point out that this is identical to the default behaviour? On 2014/06/02 18:54:11, kalman wrote: > ditto "packaged resources". > > you might also want to mention what happens with "accessible_resources": []
lgtm https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... File chrome/common/extensions/docs/templates/intros/webview_tag.html (right): https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:161: // Webviews with partition "trusted" or "persist:trusted" may access On 2014/06/03 11:34:50, Mark Dittmer wrote: > The "persist:" prefix gives the webview persistent storage. Storage is shared > among webviews with the same partition name. See > https://developer.chrome.com/apps/tags/webview#partition. Even without > wildcards, the configuration for "foo" is activated for both partition="foo" and > partition="persist:foo". > > On 2014/06/02 18:54:11, kalman wrote: > > similar comment to above. also what does "persist" mean? > please link to that doc https://codereview.chromium.org/308903002/diff/10001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/webview_tag.html:170: // resources. On 2014/06/03 11:34:50, Mark Dittmer wrote: > You mean, point out that this is identical to the default behaviour? > > On 2014/06/02 18:54:11, kalman wrote: > > ditto "packaged resources". > > > > you might also want to mention what happens with "accessible_resources": [] > when you say it like that... don't worry about it.
The CQ bit was checked by markdittmer@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/markdittmer@chromium.org/308903002/50001
Message was sent while issue was closed.
Change committed as 274668 |