Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2257933003:
[Extensions] Add starter documentation for extension features (Closed)
|
Created:
4 years, 4 months ago by Devlin Modified:
4 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Extensions] Add starter documentation for extension features
Add some basic documentation about extension feature files. I've gotten
a few questions about them lately, and it's good to have a place to
point people.
I'll try to add some more in coming patches as time allows.
BUG=639162
Committed: https://crrev.com/8a23c0c14d2433e77b8046a778651a553ab68a9a
Cr-Commit-Position: refs/heads/master@{#413254}
Patch Set 1 #
Total comments: 26
Patch Set 2 : lazyboy's #Patch Set 3 : asargent's #Messages
Total messages: 12 (4 generated)
rdevlin.cronin@chromium.org changed reviewers: + asargent@chromium.org, lazyboy@chromium.org
Antony and Istiaque, can you both take a look at this? There's some gaps, but I figured this was a good start for a digestible piece.
this is great, thanks! \o/ lgtm with some nits/suggestions. https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/_features.md (right): https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:17: [\_api\_features](https://chromium.googlesource.com/chromium/src/+/master/chrome/common/extensions/api/_api_features.json): Precede each files with "* " for bullets? https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:100: In this case, `feature1.child` will effective have the properties "will effectively have the properties:" https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:121: The `blacklist` property specifies a list of ID hashes for extensions that Add a todo to describe about how these hashes can be found? I definitely had trouble figuring this out in the past. https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:137: The `command_line_switch` property specifies a command line switch that must be s/command line switch/command line switch (without --) https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:148: The only accepted value is `false` (since true is the default). not sure: The only accepted boolean value is.. to stress this is boolean and not string "false". https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:156: Accepted values are a list of strings from `blessed_extension`, Contexts are fun stuff and also source of confusion. Add a todo to describe these? https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:204: The `internal` property specifies whether or not a feature is considered This is not exposed to developers or something that expands the "internal" word a bit. https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:259: TODO(devlin): Move documentation for how to create ID hashes, possibly move Ah, I notice this now, consolidate my comments above for TODO here too then.
https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/_features.md (right): https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:17: [\_api\_features](https://chromium.googlesource.com/chromium/src/+/master/chrome/common/extensions/api/_api_features.json): On 2016/08/19 17:58:55, lazyboy wrote: > Precede each files with "* " for bullets? Done. https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:100: In this case, `feature1.child` will effective have the properties On 2016/08/19 17:58:55, lazyboy wrote: > "will effectively have the properties:" Done. https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:121: The `blacklist` property specifies a list of ID hashes for extensions that On 2016/08/19 17:58:55, lazyboy wrote: > Add a todo to describe about how these hashes can be found? I definitely had > trouble figuring this out in the past. Added a note to see _api_features.json, since there's already the TODO at the bottom to move more of the documentation in here. https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:137: The `command_line_switch` property specifies a command line switch that must be On 2016/08/19 17:58:55, lazyboy wrote: > s/command line switch/command line switch (without --) Done, but put in the "accepted values" sentence. https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:148: The only accepted value is `false` (since true is the default). On 2016/08/19 17:58:55, lazyboy wrote: > not sure: The only accepted boolean value is.. > to stress this is boolean and not string "false". sgtm. Clarity never hurts. (Though it would just be a compiler error if they tried "false") https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:156: Accepted values are a list of strings from `blessed_extension`, On 2016/08/19 17:58:55, lazyboy wrote: > Contexts are fun stuff and also source of confusion. Add a todo to describe > these? Already done at the bottom. :) https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:204: The `internal` property specifies whether or not a feature is considered On 2016/08/19 17:58:55, lazyboy wrote: > This is not exposed to developers or something that expands the "internal" word > a bit. Done. https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:259: TODO(devlin): Move documentation for how to create ID hashes, possibly move On 2016/08/19 17:58:55, lazyboy wrote: > Ah, I notice this now, consolidate my comments above for TODO here too then. TODOs for hashes and contexts are already here, so I think we're set. :)
lgtm with a few suggestions https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/_features.md (right): https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:60: (These concepts are covered more later.) suggestion: "...are covered more later in this document" https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:75: With complex features, if either of the definitions are matched, the feature suggestion: you might also say something like "they are logically OR'd together" https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:82: it's name, where a child feature is the parent's name followed by a '.' and the grammar nazi sez: "it's" -> "its" https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:108: property `"noparent": true`. It would be nice to add a sentence here with an example of why you'd want to have a feature be a child of another but not inherit any features. https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:192: `legacy_packaged_app`, `platform_app`, `shared_module`, and `theme`. Possibly worth mentioning that "platform_app" means "non-legacy packaged app" in terms of public documentation? (and that's the term which we use elsewhere in the code for these)
https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/_features.md (right): https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:60: (These concepts are covered more later.) On 2016/08/19 19:33:16, Antony Sargent wrote: > suggestion: "...are covered more later in this document" Done. https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:75: With complex features, if either of the definitions are matched, the feature On 2016/08/19 19:33:16, Antony Sargent wrote: > suggestion: you might also say something like "they are logically OR'd together" > I like it, done. https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:82: it's name, where a child feature is the parent's name followed by a '.' and the On 2016/08/19 19:33:16, Antony Sargent wrote: > grammar nazi sez: "it's" -> "its" > d'oh! I catch this in reading, but never writing. Thanks. :) https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:108: property `"noparent": true`. On 2016/08/19 19:33:16, Antony Sargent wrote: > It would be nice to add a sentence here with an example of why you'd want to > have a feature be a child of another but not inherit any features. Done. https://codereview.chromium.org/2257933003/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/_features.md:192: `legacy_packaged_app`, `platform_app`, `shared_module`, and `theme`. On 2016/08/19 19:33:16, Antony Sargent wrote: > Possibly worth mentioning that "platform_app" means "non-legacy packaged app" in > terms of public documentation? (and that's the term which we use elsewhere in > the code for these) I've added a TODO for this. I'm still trying to figure out where best to put some of the documentation like this, since I'm not sure we want to duplicate the documentation in the code, so for some of these it may just become "See Manifest::Type".
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lazyboy@chromium.org, asargent@chromium.org Link to the patchset: https://codereview.chromium.org/2257933003/#ps40001 (title: "asargent's")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Extensions] Add starter documentation for extension features Add some basic documentation about extension feature files. I've gotten a few questions about them lately, and it's good to have a place to point people. I'll try to add some more in coming patches as time allows. BUG=639162 ========== to ========== [Extensions] Add starter documentation for extension features Add some basic documentation about extension feature files. I've gotten a few questions about them lately, and it's good to have a place to point people. I'll try to add some more in coming patches as time allows. BUG=639162 Committed: https://crrev.com/8a23c0c14d2433e77b8046a778651a553ab68a9a Cr-Commit-Position: refs/heads/master@{#413254} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8a23c0c14d2433e77b8046a778651a553ab68a9a Cr-Commit-Position: refs/heads/master@{#413254} |
