Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8352042:
Update contentSettings extension API docs. (Closed)
|
Created:
9 years, 2 months ago by Bernhard Bauer Modified:
9 years ago Reviewers:
Kathy Walrath Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionUpdate contentSettings extension API docs.
BUG=71067
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107097
Patch Set 1 #
Total comments: 36
Patch Set 2 : review #
Total comments: 26
Patch Set 3 : review #Patch Set 4 : show methods and events in TOC #Patch Set 5 : '' #Patch Set 6 : '' #
Total comments: 2
Patch Set 7 : '' #
Total comments: 2
Patch Set 8 : fix link #Messages
Total messages: 17 (0 generated)
Please review.
On 2011/10/20 11:58:15, Bernhard Bauer wrote: > Please review. You'll need an OWNER for the `extension_api.json` change.
On 2011/10/20 12:07:40, mkwst wrote: > On 2011/10/20 11:58:15, Bernhard Bauer wrote: > > Please review. > > You'll need an OWNER for the `extension_api.json` change. That's Kathy :)
At a glance; Kathy should do a deeper pass. http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... File chrome/common/extensions/docs/static/contentSettings.html (right): http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:5: The content settings module allows you Weird linebreaks here. http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:12: You can set patterns that match multiple origins, like <code>youtube.com</code> How about "Patterns can match multiple origins..."? It might also be worthwhile to introduce the concept more generally, maybe: "Content settings are applied to specific websites via <a href='...'>match patterns</a> that define a setting's scope. `http://*.youtube.com/` matches youtube.com and all it's subdomains, for example. For further details, see <a href='...'>the pattern documentation</a>." Kathy, what do you think? http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:16: Note that there is a restriction on the pattern syntax: For <code>http</code>, "on the pattern syntax for Content Settings." http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:37: for the specific plug-in, the general content settings for plug-ins are checked. Example? http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... File chrome/common/extensions/docs/static/experimental.contentSettings.html (right): http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/experimental.contentSettings.html:5: it's supported! Hooray!
http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/api/ex... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:8179: "description": "The resource identifier for the given content type." Do you want to state whether these IDs are stable in the sense of "If you find one, it is guaranteed to keep its name in the future"? http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:8187: "description": "The only content type using resource identifiers is <a href=\"contentSettings.html#property-plugins\"><var>plugins</var></a>. For more information, see <a href=\"contentSettings.html#resource-identifiers\">Resource Identifiers.</a>" nit end with </a>." (move period) http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:8187: "description": "The only content type using resource identifiers is <a href=\"contentSettings.html#property-plugins\"><var>plugins</var></a>. For more information, see <a href=\"contentSettings.html#resource-identifiers\">Resource Identifiers.</a>" #resource-identifiers is not a valid anchor http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:8336: "description": "Whether to allow cookies and other local data to be set by websites. One of<br><var>allow</var>: Accept cookies,<br><var>block</var>: Block cookies,<br><var>session_only</var>: Accept cookies only for the current session. Default is <var>allow</var>.<br>The primary URL is the URL representing the cookie origin. The secondary URL is the URL of the top-level frame.", <br> before "Default is..." also in other occurrences http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:8344: "description": "Whether to show images. One of<br><var>allow</var>: Show images,<br><var>block</var>: Don't show images. Default is <var>allow</var>.<br>The primary URL is the main-frame URL. The secondary URL is not used.", Whether to *display*? (also other occurrences in this text) http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:8352: "description": "Whether to run Javascript. One of<br><var>allow</var>: Run Javascript,<br><var>block</var>: Don't run Javascript. Default is <var>allow</var>.<br>The primary URL is the main-frame URL. The secondary URL is not used.", Whether to *execute* Java*S*cript (also other occurrences in the text) http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... File chrome/common/extensions/docs/static/contentSettings.html (right): http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:7: cookies, Javascript, and plug-ins. Explain here why it is called "content settings". http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:17: <code>https</code> and <code>ftp</code> URLs the path has to be a wildcard URLs *comma* http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:22: <h3 id="pattern-precedence">Pattern Precedence</h3> also explain primary and secondary pattern here? http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:30: <h2 id="resourceIdentifiers">Resource Identifiers</h2> How do you learn about valid identifiers? http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:42: in your extension's manifest to use the API.</p> This section usually goes to the top and contains an example.
Updated. Staged version at http://www/~bauerb/extension_docs/contentSettings.html. http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/api/ex... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:8179: "description": "The resource identifier for the given content type." On 2011/10/20 22:32:12, battre wrote: > Do you want to state whether these IDs are stable in the sense of "If you find > one, it is guaranteed to keep its name in the future"? Done (in static/). http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:8187: "description": "The only content type using resource identifiers is <a href=\"contentSettings.html#property-plugins\"><var>plugins</var></a>. For more information, see <a href=\"contentSettings.html#resource-identifiers\">Resource Identifiers.</a>" On 2011/10/20 22:32:12, battre wrote: > nit end with </a>." (move period) Done. http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:8187: "description": "The only content type using resource identifiers is <a href=\"contentSettings.html#property-plugins\"><var>plugins</var></a>. For more information, see <a href=\"contentSettings.html#resource-identifiers\">Resource Identifiers.</a>" On 2011/10/20 22:32:12, battre wrote: > #resource-identifiers is not a valid anchor fixed. http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:8336: "description": "Whether to allow cookies and other local data to be set by websites. One of<br><var>allow</var>: Accept cookies,<br><var>block</var>: Block cookies,<br><var>session_only</var>: Accept cookies only for the current session. Default is <var>allow</var>.<br>The primary URL is the URL representing the cookie origin. The secondary URL is the URL of the top-level frame.", On 2011/10/20 22:32:12, battre wrote: > <br> before "Default is..." also in other occurrences Done. http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:8344: "description": "Whether to show images. One of<br><var>allow</var>: Show images,<br><var>block</var>: Don't show images. Default is <var>allow</var>.<br>The primary URL is the main-frame URL. The secondary URL is not used.", On 2011/10/20 22:32:12, battre wrote: > Whether to *display*? (also other occurrences in this text) I'm using the same wording as in chrome://settings/content. http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/api/ex... chrome/common/extensions/api/extension_api.json:8352: "description": "Whether to run Javascript. One of<br><var>allow</var>: Run Javascript,<br><var>block</var>: Don't run Javascript. Default is <var>allow</var>.<br>The primary URL is the main-frame URL. The secondary URL is not used.", On 2011/10/20 22:32:12, battre wrote: > Whether to *execute* Java*S*cript (also other occurrences in the text) Changed capitalization of JavaScript. Kept the "execute" as per chrome://settings/content. http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... File chrome/common/extensions/docs/static/contentSettings.html (right): http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:5: The content settings module allows you On 2011/10/20 12:17:31, mkwst wrote: > Weird linebreaks here. Fixed. http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:12: You can set patterns that match multiple origins, like <code>youtube.com</code> On 2011/10/20 12:17:31, mkwst wrote: > How about "Patterns can match multiple origins..."? > > It might also be worthwhile to introduce the concept more generally, maybe: > "Content settings are applied to specific websites via <a href='...'>match > patterns</a> that define a setting's scope. `http://*.youtube.com/` matches > http://youtube.com and all it's subdomains, for example. For further details, see <a > href='...'>the pattern documentation</a>." Kathy, what do you think? Done. http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:16: Note that there is a restriction on the pattern syntax: For <code>http</code>, On 2011/10/20 12:17:31, mkwst wrote: > "on the pattern syntax for Content Settings." Done. http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:17: <code>https</code> and <code>ftp</code> URLs the path has to be a wildcard On 2011/10/20 22:32:12, battre wrote: > URLs *comma* Done. http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:22: <h3 id="pattern-precedence">Pattern Precedence</h3> On 2011/10/20 22:32:12, battre wrote: > also explain primary and secondary pattern here? Done. http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:30: <h2 id="resourceIdentifiers">Resource Identifiers</h2> On 2011/10/20 22:32:12, battre wrote: > How do you learn about valid identifiers? Done. http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:37: for the specific plug-in, the general content settings for plug-ins are checked. On 2011/10/20 12:17:31, mkwst wrote: > Example? Done. http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:42: in your extension's manifest to use the API.</p> On 2011/10/20 22:32:12, battre wrote: > This section usually goes to the top and contains an example. Done.
http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... File chrome/common/extensions/docs/static/contentSettings.html (right): http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:2: I see no methods in the staged version of this page. Do they have "nodoc" or something? For example, there's no getResourceIdentifier() method. http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:6: websites can use features such as cookies, Javascript, and plug-ins. Javascript -> JavaScript http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:26: <h2 id="patterns">Content Setting Patterns</h2> We use Sentence caps for sections (Title Caps for page names), so: Content Setting Patterns -> Content setting patterns http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:31: <a href="match_patterns.html">Match Pattern documentation</a>.</p> I like Mike's suggestion, but I'd tweak it a bit to avoid passive voice and make the example a little clearer. Maybe: You can use patterns to specify the websites that each content setting affects. For example, 'http://*.youtube.com/' specifies youtube.com and all of its subdomains. The syntax for content setting patterns is the same as for <a href="match_patterns.html">match patterns</a>, with a few differences: http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:33: Note that there are some differences between patterns for content settings and No need for a separate paragraph, I think. The previous paragraph made me think that content setting patterns *were* match patterns. I rewrote to make clearer that there are some differences. http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:36: <code>https</code>, and <code>ftp</code> URLs the path has to be a wildcard URLs the -> URLs, the has to be -> must be (use "must" whenever something's absolute -- it's clearer that way) http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:37: (<code>/*</code>). For <code>file</code> URLs, the path has to be completely has to be -> must be http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:38: specified and is <strong>not</strong> allowed to contain wildcards.</li> is not allowed to -> <strong>must not</strong. http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:46: <h3 id="pattern-precedence">Pattern Precedence</h3> Pattern Precedence -> Pattern precedence http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:51: <p>For example, the following patterns are ordered by precedence: Add </p> at the end http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:52: <ul> Use a <ol> instead of <ul> here, since the order matters. (bullets imply that order is unimportant) http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:58: There are three ways in which one pattern can be more specific than another Add <p> tag; rewrite to be more precise and avoid "There are". Maybe: <p> Three kinds of wildcards affect how specific a pattern is: </p> http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:61: <li>Wildcards in the port (like <code>http://www.example.com:*/*</code>)</li> like -> for example, (in this and the following 2 items) http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:68: hostname, scheme, port. That means that for example the list of URLs above is That means that for example -> For example, "the list of URLs above..." is imprecise -- I didn't know whether you means the first list in the section or the second. Change the rest of the paragraph to be more precise. Maybe: For example, the following URLs are ordered by precedence: <ol> <li> http://www.example.com:*/* <br> Specifies the hostname and scheme </li> <li> *:/www.example.com:123/* <br> Not as high, because although it specifies the hostname, it doesn't specify the scheme </li> <li> http://*.example.com:123/* <br> Lower because although it specifies the port and scheme, it has a wildcard in the hostname </li> http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:74: <h2 id="primary-secondary">Primary and Secondary Patterns</h2> Primary and Secondary Patterns -> Primary and secondary patterns http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:81: Some content types also can take additional URLs into account. For example, delete "also" (redundant") http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:88: If there are multiple rules with primary and secondary patterns, the rule with Rewrite to avoid "there are" and be clearer. Maybe: If there are multiple rules with -> If multiple rules have http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:89: the more specific primary pattern takes precedence. If there are multiple rules If there are multiple rules with -> If multiple rules have http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:92: pairs is ordered by precedence: Add </p> at the end of this paragraph. http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:93: <table> Add a table header. Perhaps also add a column for the order, to match the <ol>s above and make the precedence crystal clear: <tr><th>Precedence</th><th>Primary pattern</th><th>Secondary pattern</th> <tr><td>1</td><td>... http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:110: <h2 id="resource-identifiers">Resource Identifiers</h2> Resource Identifiers -> Resource identifiers http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:113: sub-types of a content type. Currently, the only content type that supports GLOBAL: sub-type -> subtype http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:121: For example, if there is a content setting rule with the resource identifier there is a content setting rule with the -> a content setting rule has the http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:128: <a href=""><code>getResourceIdentifiers</code></a> method. The returned list can Add "()" after method names. E.g.: <code>getResourceIdentifiers()</code> http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:128: <a href=""><code>getResourceIdentifiers</code></a> method. The returned list can What should this link to? http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:134: It'd be nice to have a brief example of using the content settings API here, in a <pre> section. and actually... I can't find any examples in the Samples page. I mean, there are 3 samples that use contentSettings, but no .js files are listed there.
On 2011/10/21 16:54:59, kathyw wrote: > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > File chrome/common/extensions/docs/static/contentSettings.html (right): > > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:2: > > I see no methods in the staged version of this page. Do they have "nodoc" or > something? For example, there's no getResourceIdentifier() method. Hm, that must be some weirdness in the generator template. I'll look into it. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:6: websites can use > features such as cookies, Javascript, and plug-ins. > > Javascript -> JavaScript done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:26: <h2 > id="patterns">Content Setting Patterns</h2> > > We use Sentence caps for sections (Title Caps for page names), so: > > Content Setting Patterns -> Content setting patterns done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:31: <a > href="match_patterns.html">Match Pattern documentation</a>.</p> > I like Mike's suggestion, but I'd tweak it a bit to avoid passive voice and make > the example a little clearer. Maybe: > > You can use patterns to specify the websites that each content setting affects. > For example, 'http://*.youtube.com/' > specifies http://youtube.com and all of its subdomains. > The syntax for content setting patterns is the same as for > <a href="match_patterns.html">match patterns</a>, > with a few differences: done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:33: Note that there > are some differences between patterns for content settings and > > No need for a separate paragraph, I think. The previous paragraph made me think > that content setting patterns *were* match patterns. I rewrote to make clearer > that there are some differences. > > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:36: > <code>https</code>, and <code>ftp</code> URLs the path has to be a wildcard > URLs the -> URLs, the > has to be -> must be > > (use "must" whenever something's absolute -- it's clearer that way) done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:37: (<code>/*</code>). > For <code>file</code> URLs, the path has to be completely > > has to be -> must be done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:38: specified and is > <strong>not</strong> allowed to contain wildcards.</li> > > is not allowed to -> <strong>must not</strong. done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:46: <h3 > id="pattern-precedence">Pattern Precedence</h3> > > Pattern Precedence -> Pattern precedence done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:51: <p>For example, > the following patterns are ordered by precedence: > > Add </p> at the end done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:52: <ul> > > Use a <ol> instead of <ul> here, since the order matters. > > (bullets imply that order is unimportant) done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:58: There are three > ways in which one pattern can be more specific than another > Add <p> tag; rewrite to be more precise and avoid "There are". Maybe: > > <p> > Three kinds of wildcards affect how specific a pattern is: > </p> done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:61: <li>Wildcards in > the port (like <code>http://www.example.com:*/*</code>)</li> > > like -> for example, > > (in this and the following 2 items) done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:68: hostname, scheme, > port. That means that for example the list of URLs above is > > That means that for example -> For example, > > "the list of URLs above..." is imprecise -- I didn't know whether you means the > first list in the section or the second. Change the rest of the paragraph to be > more precise. Maybe: > > For example, the following URLs are ordered by precedence: > <ol> > <li> http://www.example.com:*/* > <br> > Specifies the hostname and scheme </li> > <li> *:/www.example.com:123/* > <br> > Not as high, because although it specifies the hostname, it doesn't specify > the scheme </li> > <li> http://*.example.com:123/* > <br> > Lower because although it specifies the port and scheme, it has a wildcard > in the hostname </li> done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:74: <h2 > id="primary-secondary">Primary and Secondary Patterns</h2> > > Primary and Secondary Patterns -> Primary and secondary patterns done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:81: Some content types > also can take additional URLs into account. For example, > > delete "also" (redundant") done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:88: If there are > multiple rules with primary and secondary patterns, the rule with > Rewrite to avoid "there are" and be clearer. Maybe: > > If there are multiple rules with > -> > If multiple rules have done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:89: the more specific > primary pattern takes precedence. If there are multiple rules > > If there are multiple rules with > -> > If multiple rules have done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:92: pairs is ordered > by precedence: > > Add </p> at the end of this paragraph. done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:93: <table> > Add a table header. Perhaps also add a column for the order, to match the <ol>s > above and make the precedence crystal clear: > > <tr><th>Precedence</th><th>Primary pattern</th><th>Secondary pattern</th> > <tr><td>1</td><td>... done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:110: <h2 > id="resource-identifiers">Resource Identifiers</h2> > > Resource Identifiers -> Resource identifiers done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:113: sub-types of a > content type. Currently, the only content type that supports > > GLOBAL: sub-type -> subtype done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:121: For example, if > there is a content setting rule with the resource identifier > > there is a content setting rule with the > -> > a content setting rule has the > > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:128: <a > href=""><code>getResourceIdentifiers</code></a> method. The returned list can > Add "()" after method names. E.g.: > > <code>getResourceIdentifiers()</code> done. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:128: <a > href=""><code>getResourceIdentifiers</code></a> method. The returned list can > What should this link to? Oops, forgot to add the target. Fixed. > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > chrome/common/extensions/docs/static/contentSettings.html:134: > It'd be nice to have a brief example of using the content settings API here, in > a <pre> section. > > and actually... I can't find any examples in the Samples page. I mean, there are > 3 samples that use contentSettings, but no .js files are listed there. Hm, that might be because of the way the extension works; it doesn't directly call chrome.contentSettings.plugins.set etc, but it accesses them via a subscript, i.e. chrome.contentSettings[type].set
On 2011/10/24 09:03:58, Bernhard Bauer wrote: > On 2011/10/21 16:54:59, kathyw wrote: > > > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > > File chrome/common/extensions/docs/static/contentSettings.html (right): > > > > > http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... > > chrome/common/extensions/docs/static/contentSettings.html:2: > > > > I see no methods in the staged version of this page. Do they have "nodoc" or > > something? For example, there's no getResourceIdentifier() method. > > Hm, that must be some weirdness in the generator template. I'll look into it. OK, I updated the generator template so it shows methods and events for types in the table of contents. This changes every file in the generated documentation though :( I could also pull the change out into a separate CL, if that makes it easier to review.
>> > I see no methods in the staged version of this page. Do they have >> > "nodoc" or >> > something? For example, there's no getResourceIdentifier() method. > >> Hm, that must be some weirdness in the generator template. I'll look into >> it. > > OK, I updated the generator template so it shows methods and events for > types in > the table of contents. This changes every file in the generated > documentation > though :( I could also pull the change out into a separate CL, if that makes > it > easier to review. > > http://codereview.chromium.org/8352042/ Oh, interesting... Yes, let's make that a separate review. -k- -k-
http://codereview.chromium.org/8352042/diff/17001/chrome/common/extensions/do... File chrome/common/extensions/docs/static/contentSettings.html (right): http://codereview.chromium.org/8352042/diff/17001/chrome/common/extensions/do... chrome/common/extensions/docs/static/contentSettings.html:38: that port. If no port number is specified, the pattern matches all ports. I'm sorry, I missed the earlier design of this feature. But did you consider enabling ports for the other parts of the extension system that use match patterns? It stinks to have so many special cases.
http://codereview.chromium.org/8352042/diff/17001/chrome/common/extensions/do... File chrome/common/extensions/docs/static/contentSettings.html (right): http://codereview.chromium.org/8352042/diff/17001/chrome/common/extensions/do... chrome/common/extensions/docs/static/contentSettings.html:38: that port. If no port number is specified, the pattern matches all ports. On 2011/10/24 15:59:24, Aaron Boodman wrote: > I'm sorry, I missed the earlier design of this feature. But did you consider > enabling ports for the other parts of the extension system that use match > patterns? It stinks to have so many special cases. Hm, I think we wanted to keep compatibility with the existing behavior? http://codereview.chromium.org/7229012 was the original review, for context.
LGTM, after you fix one bad link. http://codereview.chromium.org/8352042/diff/16005/chrome/common/extensions/do... File chrome/common/extensions/docs/contentSettings.html (right): http://codereview.chromium.org/8352042/diff/16005/chrome/common/extensions/do... chrome/common/extensions/docs/contentSettings.html:494: <a href="method-ContentSetting-getResourceIdentifiers"> This is a bad link. Perhaps remove the link for this CL, and add the right one for the next? -k-
On 2011/10/24 16:18:26, Bernhard Bauer wrote: > http://codereview.chromium.org/8352042/diff/17001/chrome/common/extensions/do... > File chrome/common/extensions/docs/static/contentSettings.html (right): > > http://codereview.chromium.org/8352042/diff/17001/chrome/common/extensions/do... > chrome/common/extensions/docs/static/contentSettings.html:38: that port. If no > port number is specified, the pattern matches all ports. > On 2011/10/24 15:59:24, Aaron Boodman wrote: > > I'm sorry, I missed the earlier design of this feature. But did you consider > > enabling ports for the other parts of the extension system that use match > > patterns? It stinks to have so many special cases. > > Hm, I think we wanted to keep compatibility with the existing behavior? > http://codereview.chromium.org/7229012 was the original review, for context. Thanks for the reminder. I added this to the list of breaking changes to consider at some point: http://dev.chromium.org/developers/design-documents/extensions/breaking-changes
lgtm
http://codereview.chromium.org/8352042/diff/16005/chrome/common/extensions/do... File chrome/common/extensions/docs/contentSettings.html (right): http://codereview.chromium.org/8352042/diff/16005/chrome/common/extensions/do... chrome/common/extensions/docs/contentSettings.html:494: <a href="method-ContentSetting-getResourceIdentifiers"> Fixed. The anchor is already there, it's just the link to it that's missing in the table of contents.
Ignore... Just trying to get this stupid issue out of my queue... http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... File chrome/common/extensions/docs/static/contentSettings.html (right): http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:10: <h2 id="patterns">Content Setting Patterns</h2> Content Setting Patterns -> Content setting patterns (we use Title Case only in page names; headings use Sentence case) http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:12: You can set patterns that match multiple origins, like <code>youtube.com</code> I agree with your comment, Mike. I'd tweak it a bit. Maybe: You use <em>match patterns</em> to specify the websites in each content setting's scope. For example, the match pattern 'http://*.youtube.com/' specifies youtube.com and all of its subdomains. For details, see _Match Patterns_. http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:22: <h3 id="pattern-precedence">Pattern Precedence</h3> Pattern Precedence -> Pattern precedence http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:30: <h2 id="resourceIdentifiers">Resource Identifiers</h2> Resource Identifiers -> Resource identifiers http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:33: sub-types of a content type. Currently, the only content type that supports sub-types -> subtypes http://codereview.chromium.org/8352042/diff/1/chrome/common/extensions/docs/s... chrome/common/extensions/docs/static/contentSettings.html:41: <p>You must declare the "contentSettings" permission Usually the manifest section goes higher up and has a certain format (with example code). See bookmarks.html for a typical example. http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... File chrome/common/extensions/docs/static/contentSettings.html (right): http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:2: I see no methods in the staged version of this page. Do they have "nodoc" or something? For example, there's no getResourceIdentifier() method. http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:6: websites can use features such as cookies, Javascript, and plug-ins. Javascript -> JavaScript http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:26: <h2 id="patterns">Content Setting Patterns</h2> We use Sentence caps for sections (Title Caps for page names), so: Content Setting Patterns -> Content setting patterns http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:31: <a href="match_patterns.html">Match Pattern documentation</a>.</p> I like Mike's suggestion, but I'd tweak it a bit to avoid passive voice and make the example a little clearer. Maybe: You can use patterns to specify the websites that each content setting affects. For example, 'http://*.youtube.com/' specifies youtube.com and all of its subdomains. The syntax for content setting patterns is the same as for <a href="match_patterns.html">match patterns</a>, with a few differences: http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:33: Note that there are some differences between patterns for content settings and No need for a separate paragraph, I think. The previous paragraph made me think that content setting patterns *were* match patterns. I rewrote to make clearer that there are some differences. http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:36: <code>https</code>, and <code>ftp</code> URLs the path has to be a wildcard URLs the -> URLs, the has to be -> must be (use "must" whenever something's absolute -- it's clearer that way) http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:37: (<code>/*</code>). For <code>file</code> URLs, the path has to be completely has to be -> must be http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:38: specified and is <strong>not</strong> allowed to contain wildcards.</li> is not allowed to -> <strong>must not</strong. http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:46: <h3 id="pattern-precedence">Pattern Precedence</h3> Pattern Precedence -> Pattern precedence http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:51: <p>For example, the following patterns are ordered by precedence: Add </p> at the end http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:52: <ul> Use a <ol> instead of <ul> here, since the order matters. (bullets imply that order is unimportant) http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:58: There are three ways in which one pattern can be more specific than another Add <p> tag; rewrite to be more precise and avoid "There are". Maybe: <p> Three kinds of wildcards affect how specific a pattern is: </p> http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:61: <li>Wildcards in the port (like <code>http://www.example.com:*/*</code>)</li> like -> for example, (in this and the following 2 items) http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:68: hostname, scheme, port. That means that for example the list of URLs above is That means that for example -> For example, "the list of URLs above..." is imprecise -- I didn't know whether you means the first list in the section or the second. Change the rest of the paragraph to be more precise. Maybe: For example, the following URLs are ordered by precedence: <ol> <li> http://www.example.com:*/* <br> Specifies the hostname and scheme </li> <li> *:/www.example.com:123/* <br> Not as high, because although it specifies the hostname, it doesn't specify the scheme </li> <li> http://*.example.com:123/* <br> Lower because although it specifies the port and scheme, it has a wildcard in the hostname </li> http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:74: <h2 id="primary-secondary">Primary and Secondary Patterns</h2> Primary and Secondary Patterns -> Primary and secondary patterns http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:81: Some content types also can take additional URLs into account. For example, delete "also" (redundant") http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:88: If there are multiple rules with primary and secondary patterns, the rule with Rewrite to avoid "there are" and be clearer. Maybe: If there are multiple rules with -> If multiple rules have http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:89: the more specific primary pattern takes precedence. If there are multiple rules If there are multiple rules with -> If multiple rules have http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:92: pairs is ordered by precedence: Add </p> at the end of this paragraph. http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:93: <table> Add a table header. Perhaps also add a column for the order, to match the <ol>s above and make the precedence crystal clear: <tr><th>Precedence</th><th>Primary pattern</th><th>Secondary pattern</th> <tr><td>1</td><td>... http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:110: <h2 id="resource-identifiers">Resource Identifiers</h2> Resource Identifiers -> Resource identifiers http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:113: sub-types of a content type. Currently, the only content type that supports GLOBAL: sub-type -> subtype http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:121: For example, if there is a content setting rule with the resource identifier there is a content setting rule with the -> a content setting rule has the http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:128: <a href=""><code>getResourceIdentifiers</code></a> method. The returned list can Add "()" after method names. E.g.: <code>getResourceIdentifiers()</code> http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:128: <a href=""><code>getResourceIdentifiers</code></a> method. The returned list can What should this link to? http://codereview.chromium.org/8352042/diff/5002/chrome/common/extensions/doc... chrome/common/extensions/docs/static/contentSettings.html:134: It'd be nice to have a brief example of using the content settings API here, in a <pre> section. and actually... I can't find any examples in the Samples page. I mean, there are 3 samples that use contentSettings, but no .js files are listed there. http://codereview.chromium.org/8352042/diff/16005/chrome/common/extensions/do... File chrome/common/extensions/docs/contentSettings.html (right): http://codereview.chromium.org/8352042/diff/16005/chrome/common/extensions/do... chrome/common/extensions/docs/contentSettings.html:494: <a href="method-ContentSetting-getResourceIdentifiers"> This is a bad link. Perhaps remove the link for this CL, and add the right one for the next? |
