|
|
Created:
7 years, 5 months ago by vadimt Modified:
7 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, not at google - send to devlin, evan.peterson.ep_gmail.com Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding dev documentation for chrome.location
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=214927
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 8
Patch Set 5 : CR comments #Patch Set 6 : #Patch Set 7 : #
Total comments: 7
Patch Set 8 : #
Total comments: 14
Patch Set 9 : More CR comments #Patch Set 10 : More CR comments #Patch Set 11 : Disallowing chrome.location for hosted apps #Patch Set 12 : #
Total comments: 8
Patch Set 13 : Final comments. #
Messages
Total messages: 19 (0 generated)
https://codereview.chromium.org/19763008/diff/8001/chrome/common/extensions/d... File chrome/common/extensions/docs/templates/intros/location.html (right): https://codereview.chromium.org/19763008/diff/8001/chrome/common/extensions/d... chrome/common/extensions/docs/templates/intros/location.html:2: Use the <code>chrome.location</code> API to retrieve geographical location of I suspect this paragraph belongs in the "description" field of the API definition, but I'm not sure where that goes in .idl files. In a .json, it would be https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext.... Maybe it's the comment above the namespace for a .idl file? You can cd to chrome/common/extensions/docs and run server2/preview.py to see how your change will look, even though the public preview server is broken. Ben, it would be nice to have a howto for this. What's an appropriate place to put it? Maybe something under http://www.chromium.org/developers/design-documents/extensions/how-the-extens... (Have you updated those pages at all for the new documentation system?) https://codereview.chromium.org/19763008/diff/8001/chrome/common/extensions/d... chrome/common/extensions/docs/templates/intros/location.html:5: <h2 id="manifest">Manifest</h2> Since there's nothing for someone to do in the manifest other than specify the API permission, I think we should omit this section. It'll be mentioned in the "Permissions" section of the header table anyway. ... Woah, I found a new file. You should update docs/templates/json/intro_tables.json to make sure chrome.location gets a Permissions section. This really needs better documentation... https://codereview.chromium.org/19763008/diff/8001/chrome/common/extensions/d... File chrome/common/extensions/docs/templates/public/extensions/location.html (right): https://codereview.chromium.org/19763008/diff/8001/chrome/common/extensions/d... chrome/common/extensions/docs/templates/public/extensions/location.html:1: {{+partials.standard_extensions_api api:apis.location intro:intros.location}} It looks like chrome.location is enabled for apps too, so you need to put the a similar file in public/apps/location.html. Looks like that should contain: {{+partials.standard_apps_api api:apis.location intro:intros.location}}
https://codereview.chromium.org/19763008/diff/8001/chrome/common/extensions/d... File chrome/common/extensions/docs/templates/intros/location.html (right): https://codereview.chromium.org/19763008/diff/8001/chrome/common/extensions/d... chrome/common/extensions/docs/templates/intros/location.html:2: Use the <code>chrome.location</code> API to retrieve geographical location of On 2013/07/23 20:37:29, Jeffrey Yasskin wrote: > I suspect this paragraph belongs in the "description" field of the API > definition, but I'm not sure where that goes in .idl files. In a .json, it would > be > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext.... > Maybe it's the comment above the namespace for a .idl file? > > You can cd to chrome/common/extensions/docs and run server2/preview.py to see > how your change will look, even though the public preview server is broken. There is a README in chrome/common/extensions/docs but I guess this needs to be more obvious. > > Ben, it would be nice to have a howto for this. What's an appropriate place to > put it? Maybe something under > http://www.chromium.org/developers/design-documents/extensions/how-the-extens... > (Have you updated those pages at all for the new documentation system?) Definitely not. https://codereview.chromium.org/19763008/diff/8001/chrome/common/extensions/d... chrome/common/extensions/docs/templates/intros/location.html:5: <h2 id="manifest">Manifest</h2> On 2013/07/23 20:37:29, Jeffrey Yasskin wrote: > Since there's nothing for someone to do in the manifest other than specify the > API permission, I think we should omit this section. It'll be mentioned in the > "Permissions" section of the header table anyway. > > ... Woah, I found a new file. You should update > docs/templates/json/intro_tables.json to make sure chrome.location gets a > Permissions section. This really needs better documentation... We are supposed to be automatically generating the permissions. Dang. I'll get somebody to work on it.
I've synced to latest, but git cl upload throws an exception. So, I've uploaded with --bypass-hooks. Can this be fixed? Output: D:\src\Chromium1\src>git cl upload Using 50% similarity for rename/copy detection. Override with --similarity. Loaded authentication cookies from c:/Utils/depot_tools/.codereview_upload_cookies Running presubmit upload checks ... Traceback (most recent call last): File "docs\server2\integration_test.py", line 23, in <module> from local_renderer import LocalRenderer File "D:\src\Chromium1\src\chrome\common\extensions\docs\server2\local_renderer.py", line 9, in <module> from server_instance import ServerInstance File "D:\src\Chromium1\src\chrome\common\extensions\docs\server2\server_instance.py", line 12, in <module> from manifest_data_source import ManifestDataSource File "D:\src\Chromium1\src\chrome\common\extensions\docs\server2\manifest_data_source.py", line 5, in <module> from collections import OrderedDict ImportError: cannot import name OrderedDict ** Presubmit ERRORS ** IntegrationTest failed! Presubmit checks took 3.8s to calculate. https://codereview.chromium.org/19763008/diff/8001/chrome/common/extensions/d... File chrome/common/extensions/docs/templates/intros/location.html (right): https://codereview.chromium.org/19763008/diff/8001/chrome/common/extensions/d... chrome/common/extensions/docs/templates/intros/location.html:2: Use the <code>chrome.location</code> API to retrieve geographical location of On 2013/07/23 20:37:29, Jeffrey Yasskin wrote: > I suspect this paragraph belongs in the "description" field of the API > definition, but I'm not sure where that goes in .idl files. In a .json, it would > be > https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/common/ext.... > Maybe it's the comment above the namespace for a .idl file? > > You can cd to chrome/common/extensions/docs and run server2/preview.py to see > how your change will look, even though the public preview server is broken. > > Ben, it would be nice to have a howto for this. What's an appropriate place to > put it? Maybe something under > http://www.chromium.org/developers/design-documents/extensions/how-the-extens... > (Have you updated those pages at all for the new documentation system?) Running preview.py results in: D:\src\Chromium1\src\chrome\common\extensions\docs>python server2/preview.py Traceback (most recent call last): File "server2/preview.py", line 40, in <module> from local_renderer import LocalRenderer File "D:\src\Chromium1\src\chrome\common\extensions\docs\server2\local_renderer.py", line 9, in <module> from server_instance import ServerInstance File "D:\src\Chromium1\src\chrome\common\extensions\docs\server2\server_instance.py", line 12, in <module> from manifest_data_source import ManifestDataSource File "D:\src\Chromium1\src\chrome\common\extensions\docs\server2\manifest_data_source.py", line 5, in <module> from collections import OrderedDict ImportError: cannot import name OrderedDict I'm not a big specialist in Python; does anyone have an idea what's happening. (Please don't answer that this simply cannot work on Windows :)) https://codereview.chromium.org/19763008/diff/8001/chrome/common/extensions/d... chrome/common/extensions/docs/templates/intros/location.html:5: <h2 id="manifest">Manifest</h2> On 2013/07/23 20:37:29, Jeffrey Yasskin wrote: > Since there's nothing for someone to do in the manifest other than specify the > API permission, I think we should omit this section. It'll be mentioned in the > "Permissions" section of the header table anyway. > > ... Woah, I found a new file. You should update > docs/templates/json/intro_tables.json to make sure chrome.location gets a > Permissions section. This really needs better documentation... Done. https://codereview.chromium.org/19763008/diff/8001/chrome/common/extensions/d... File chrome/common/extensions/docs/templates/public/extensions/location.html (right): https://codereview.chromium.org/19763008/diff/8001/chrome/common/extensions/d... chrome/common/extensions/docs/templates/public/extensions/location.html:1: {{+partials.standard_extensions_api api:apis.location intro:intros.location}} On 2013/07/23 20:37:29, Jeffrey Yasskin wrote: > It looks like chrome.location is enabled for apps too, so you need to put the a > similar file in public/apps/location.html. Looks like that should contain: > > {{+partials.standard_apps_api api:apis.location intro:intros.location}} Done.
On 2013/07/24 19:36:45, vadimt wrote: > I've synced to latest, but git cl upload throws an exception. So, I've uploaded > with --bypass-hooks. > Can this be fixed? Output: > > D:\src\Chromium1\src>git cl upload > Using 50% similarity for rename/copy detection. Override with --similarity. > Loaded authentication cookies from > c:/Utils/depot_tools/.codereview_upload_cookies > Running presubmit upload checks ... > > Traceback (most recent call last): > File "docs\server2\integration_test.py", line 23, in <module> > from local_renderer import LocalRenderer > File > "D:\src\Chromium1\src\chrome\common\extensions\docs\server2\local_renderer.py", > line 9, in <module> > from server_instance import ServerInstance > File > "D:\src\Chromium1\src\chrome\common\extensions\docs\server2\server_instance.py", > line 12, in <module> > from manifest_data_source import ManifestDataSource > File > "D:\src\Chromium1\src\chrome\common\extensions\docs\server2\manifest_data_source.py", > line 5, in <module> > from collections import OrderedDict > ImportError: cannot import name OrderedDict > > ** Presubmit ERRORS ** > IntegrationTest failed! > > Presubmit checks took 3.8s to calculate. > > > Running preview.py results in: > D:\src\Chromium1\src\chrome\common\extensions\docs>python server2/preview.py > Traceback (most recent call last): > File "server2/preview.py", line 40, in <module> > from local_renderer import LocalRenderer > File > "D:\src\Chromium1\src\chrome\common\extensions\docs\server2\local_renderer.py", > line 9, in <module> > from server_instance import ServerInstance > File > "D:\src\Chromium1\src\chrome\common\extensions\docs\server2\server_instance.py", > line 12, in <module> > from manifest_data_source import ManifestDataSource > File > "D:\src\Chromium1\src\chrome\common\extensions\docs\server2\manifest_data_source.py", > line 5, in <module> > from collections import OrderedDict > ImportError: cannot import name OrderedDict Hey vadlmt, what version of Python are you using? OrderedDict is part of the python standard library and became available in version 2.7, so if you are running 2.6 or lower, it isn't available (thus an import error).
we need to import OrderedDict from json_parse not from collections.
On 2013/07/24 20:01:11, kalman wrote: > we need to import OrderedDict from json_parse not from collections. Ben fixed this in r213515 (https://codereview.chromium.org/20142002, not in lkgr yet), so could you either sync past there or install Python 2.7 and try preview.py again?
Thanks everyone! I've uploaded an updated version, and I like how the result looks on the web server. The only issue is that links $ref chrome.location.watchLocation and $ref chrome.location.clearWatch don't work. And I get presubmit warnings: ERROR:root:$ref chrome.location.watchLocation could not be resolved in namespace location. ERROR:root:$ref chrome.location.clearWatch could not be resolved in namespace location. What I observe is non-working links on the page: http://localhost:8000/extensions/location.html#type-watchLocation The link that would work is: http://localhost:8000/extensions/location.html#method-watchLocation So, how to fix this?
mkwst@, please provide OWNER's approval for all files in CL
+awatson https://codereview.chromium.org/19763008/diff/32001/chrome/common/extensions/... File chrome/common/extensions/api/location.idl (right): https://codereview.chromium.org/19763008/diff/32001/chrome/common/extensions/... chrome/common/extensions/api/location.idl:7: // the hosting machine. i preferred the old description https://codereview.chromium.org/19763008/diff/32001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/json/intro_tables.json (right): https://codereview.chromium.org/19763008/diff/32001/chrome/common/extensions/... chrome/common/extensions/docs/templates/json/intro_tables.json:158: } don't need this anymore
https://codereview.chromium.org/19763008/diff/32001/chrome/common/extensions/... File chrome/common/extensions/api/location.idl (right): https://codereview.chromium.org/19763008/diff/32001/chrome/common/extensions/... chrome/common/extensions/api/location.idl:7: // the hosting machine. On 2013/07/27 01:51:49, kalman wrote: > i preferred the old description That was a pretty head-scratchy description for the user :) The user probably wants to see something more digestible and actionable, like: Use the chrome.alarms API to schedule code to run periodically or at a specified time in the future. Also, it creates a false impression that the API is literally a wrapper around that API, which is not true, despite the fact that they share 99% of implementation. https://codereview.chromium.org/19763008/diff/32001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/json/intro_tables.json (right): https://codereview.chromium.org/19763008/diff/32001/chrome/common/extensions/... chrome/common/extensions/docs/templates/json/intro_tables.json:158: } On 2013/07/27 01:51:49, kalman wrote: > don't need this anymore Done.
lgtm after the below comments, but please wait 'til Monday to let Ben or Andy chime in if they want to. https://codereview.chromium.org/19763008/diff/32001/chrome/common/extensions/... File chrome/common/extensions/api/location.idl (right): https://codereview.chromium.org/19763008/diff/32001/chrome/common/extensions/... chrome/common/extensions/api/location.idl:7: // the hosting machine. On 2013/07/27 02:08:55, vadimt wrote: > On 2013/07/27 01:51:49, kalman wrote: > > i preferred the old description > > That was a pretty head-scratchy description for the user :) > > The user probably wants to see something more digestible and actionable, like: > > Use the chrome.alarms API to schedule code to run periodically or at a specified > time in the future. > > Also, it creates a false impression that the API is literally a wrapper around > that API, which is not true, despite the fact that they share 99% of > implementation. Can we combine them? I agree that your new description is much better for new users, but I think experienced HTML5 folks will wonder how it's different from the geolocation API they already know, and why we invented our own. What if you add a sentence "This is a version of the <a href="http://dev.w3.org/geo/api/spec-source.html">HTML Geolocation API</a> that is compatible with event pages." ? https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/intros/location.html (right): https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/location.html:1: <p id="classSummary"> Does this id do anything visible in the UI? Would it make more sense to head this section with an <h2 id="classSummary"> (or "overview" like some other APIs)?
https://codereview.chromium.org/19763008/diff/32001/chrome/common/extensions/... File chrome/common/extensions/api/location.idl (right): https://codereview.chromium.org/19763008/diff/32001/chrome/common/extensions/... chrome/common/extensions/api/location.idl:7: // the hosting machine. On 2013/07/27 03:00:29, Jeffrey Yasskin wrote: > On 2013/07/27 02:08:55, vadimt wrote: > > On 2013/07/27 01:51:49, kalman wrote: > > > i preferred the old description > > > > That was a pretty head-scratchy description for the user :) > > > > The user probably wants to see something more digestible and actionable, like: > > > > Use the chrome.alarms API to schedule code to run periodically or at a > specified > > time in the future. > > > > Also, it creates a false impression that the API is literally a wrapper around > > that API, which is not true, despite the fact that they share 99% of > > implementation. > > Can we combine them? I agree that your new description is much better for new > users, but I think experienced HTML5 folks will wonder how it's different from > the geolocation API they already know, and why we invented our own. What if you > add a sentence "This is a version of the <a > href="http://dev.w3.org/geo/api/spec-source.html">HTML Geolocation API</a> that > is compatible with event pages." ? Yes exactly. This was going to be my next comment. You need to make this clear somewhere. I like it in the description; I definitely want to see it in the intro section.
https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... File chrome/common/extensions/api/location.idl (right): https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... chrome/common/extensions/api/location.idl:6: // Use the <code>chrome.location</code> API to retrieve geographical location of Suggestion: "geographical" --> "the geographic" https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... chrome/common/extensions/api/location.idl:7: // the hosting machine. Suggestion: "hosting" --> "host" You might also mention something about location watches here, since you jump right into location watches in intros/location.html. https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/intros/location.html (right): https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/location.html:2: To create a location watch, call $ref:location.watchLocation passing in Before you explain how to create a location watch, it would be helpful to provide a little context: What is a location watch and when would you use one? https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/location.html:8: You'll also need to add a listener Suggestion: "You'll also" --> "You also" https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/location.html:9: to its <code>onChanged</code> event. What does "its" refer to here? https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/location.html:12: computer changes until you call $ref:location.clearWatch. Suggestion: "the geographical location of your computer" --> "the geographic location of the host machine"
https://codereview.chromium.org/19763008/diff/32001/chrome/common/extensions/... File chrome/common/extensions/api/location.idl (right): https://codereview.chromium.org/19763008/diff/32001/chrome/common/extensions/... chrome/common/extensions/api/location.idl:7: // the hosting machine. On 2013/07/27 03:00:29, Jeffrey Yasskin wrote: > On 2013/07/27 02:08:55, vadimt wrote: > > On 2013/07/27 01:51:49, kalman wrote: > > > i preferred the old description > > > > That was a pretty head-scratchy description for the user :) > > > > The user probably wants to see something more digestible and actionable, like: > > > > Use the chrome.alarms API to schedule code to run periodically or at a > specified > > time in the future. > > > > Also, it creates a false impression that the API is literally a wrapper around > > that API, which is not true, despite the fact that they share 99% of > > implementation. > > Can we combine them? I agree that your new description is much better for new > users, but I think experienced HTML5 folks will wonder how it's different from > the geolocation API they already know, and why we invented our own. What if you > add a sentence "This is a version of the <a > href="http://dev.w3.org/geo/api/spec-source.html">HTML Geolocation API</a> that > is compatible with event pages." ? Done. Probably, a similar comment should be done for chrome.alarms API? https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... File chrome/common/extensions/api/location.idl (right): https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... chrome/common/extensions/api/location.idl:6: // Use the <code>chrome.location</code> API to retrieve geographical location of On 2013/07/30 00:56:06, Andy wrote: > Suggestion: "geographical" --> "the geographic" Done. https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... chrome/common/extensions/api/location.idl:7: // the hosting machine. On 2013/07/30 00:56:06, Andy wrote: > Suggestion: "hosting" --> "host" > > You might also mention something about location watches here, since you jump > right into location watches in intros/location.html. Done. For watches, changed HTML. https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/intros/location.html (right): https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/location.html:1: <p id="classSummary"> On 2013/07/27 03:00:29, Jeffrey Yasskin wrote: > Does this id do anything visible in the UI? Would it make more sense to head > this section with an <h2 id="classSummary"> (or "overview" like some other > APIs)? Done. https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/location.html:2: To create a location watch, call $ref:location.watchLocation passing in On 2013/07/30 00:56:06, Andy wrote: > Before you explain how to create a location watch, it would be helpful to > provide a little context: What is a location watch and when would you use one? Done. https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/location.html:8: You'll also need to add a listener On 2013/07/30 00:56:06, Andy wrote: > Suggestion: "You'll also" --> "You also" Done. https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/location.html:9: to its <code>onChanged</code> event. On 2013/07/30 00:56:06, Andy wrote: > What does "its" refer to here? Done. https://codereview.chromium.org/19763008/diff/39001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/location.html:12: computer changes until you call $ref:location.clearWatch. On 2013/07/30 00:56:06, Andy wrote: > Suggestion: "the geographical location of your computer" --> "the geographic > location of the host machine" Done.
LGTM, with a few more minor suggestions. https://codereview.chromium.org/19763008/diff/66001/chrome/common/extensions/... File chrome/common/extensions/api/location.idl (right): https://codereview.chromium.org/19763008/diff/66001/chrome/common/extensions/... chrome/common/extensions/api/location.idl:7: // of the host machine. This is a version of the <a "This" --> "This API" https://codereview.chromium.org/19763008/diff/66001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/intros/location.html (right): https://codereview.chromium.org/19763008/diff/66001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/location.html:3: To start retrieving the location, you need to create a location watch by "the location" --> "location information" https://codereview.chromium.org/19763008/diff/66001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/location.html:11: to $ref:location.onLocationUpdate event. "to" --> "for the" https://codereview.chromium.org/19763008/diff/66001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/location.html:12: Chrome will fire this event after you create your location watch. After this, It's not clear what "this" refers to in "After this". One idea: "Chrome will fire this event after you create your location watch, and it will keep firing..."
https://codereview.chromium.org/19763008/diff/66001/chrome/common/extensions/... File chrome/common/extensions/api/location.idl (right): https://codereview.chromium.org/19763008/diff/66001/chrome/common/extensions/... chrome/common/extensions/api/location.idl:7: // of the host machine. This is a version of the <a On 2013/07/30 20:26:14, Andy wrote: > "This" --> "This API" Done. https://codereview.chromium.org/19763008/diff/66001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/intros/location.html (right): https://codereview.chromium.org/19763008/diff/66001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/location.html:3: To start retrieving the location, you need to create a location watch by On 2013/07/30 20:26:14, Andy wrote: > "the location" --> "location information" Done. https://codereview.chromium.org/19763008/diff/66001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/location.html:11: to $ref:location.onLocationUpdate event. On 2013/07/30 20:26:14, Andy wrote: > "to" --> "for the" Done. https://codereview.chromium.org/19763008/diff/66001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/location.html:12: Chrome will fire this event after you create your location watch. After this, On 2013/07/30 20:26:14, Andy wrote: > It's not clear what "this" refers to in "After this". One idea: > > "Chrome will fire this event after you create your location watch, and it will > keep firing..." Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vadimt@chromium.org/19763008/71001
Message was sent while issue was closed.
Change committed as 214927 |