Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
27335003:
Add docs for chrome.wallpaper API (Closed)
|
Created:
7 years, 2 months ago by bshe Modified:
7 years, 1 month ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd docs for chrome.wallpaper API
BUG=176183
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235758
Patch Set 1 #
Total comments: 6
Patch Set 2 : reviews #
Total comments: 4
Patch Set 3 : #Patch Set 4 : #
Total comments: 12
Patch Set 5 : #
Total comments: 1
Patch Set 6 : #
Total comments: 1
Patch Set 7 : remove host permission #Patch Set 8 : rebase #
Total comments: 8
Patch Set 9 : more reviews #Patch Set 10 : new templkate syntax #
Messages
Total messages: 17 (0 generated)
Hi Mike. Do you mind to take a look at this CL? I try to follow the example of adding API doc. But I am not sure if I misunderstand anything here. According to the doc, it seems I should be able to access the doc from: https://chrome-apps-doc.appspot.com/_patch/27335003/extensions/wallpaper.html But somehow this link above doesn't work. Thanks! Biao
LGTM but please resolve the doc-server issue before committing so you can verify the markup is working. https://codereview.chromium.org/27335003/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/templates/intros/wallpaper.html (right): https://codereview.chromium.org/27335003/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/intros/wallpaper.html:4: url property of setWallpaper, you must also whitelist the domain of your url in Because this is documentation, we should be more careful about style. "URL" is capitalized. https://codereview.chromium.org/27335003/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/intros/wallpaper.html:5: permission. "in the manifest permissions." https://codereview.chromium.org/27335003/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/intros/wallpaper.html:27: 'url': 'http://example.com/aFile.jpg', This is horribly exacting, but our Chrome style guidelines likely wouldn't have permitted this file name. I know it's just a sample, but a_file.jpg is more consistent.
Thanks for review! https://codereview.chromium.org/27335003/diff/1/chrome/common/extensions/docs... File chrome/common/extensions/docs/templates/intros/wallpaper.html (right): https://codereview.chromium.org/27335003/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/intros/wallpaper.html:4: url property of setWallpaper, you must also whitelist the domain of your url in On 2013/10/15 18:17:33, miket wrote: > Because this is documentation, we should be more careful about style. "URL" is > capitalized. Done. https://codereview.chromium.org/27335003/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/intros/wallpaper.html:5: permission. On 2013/10/15 18:17:33, miket wrote: > "in the manifest permissions." Done. https://codereview.chromium.org/27335003/diff/1/chrome/common/extensions/docs... chrome/common/extensions/docs/templates/intros/wallpaper.html:27: 'url': 'http://example.com/aFile.jpg', On 2013/10/15 18:17:33, miket wrote: > This is horribly exacting, but our Chrome style guidelines likely wouldn't have > permitted this file name. I know it's just a sample, but a_file.jpg is more > consistent. Done.
why is this API apps-only? Seems fine for extensions. https://codereview.chromium.org/27335003/diff/10001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/intros/wallpaper.html (right): https://codereview.chromium.org/27335003/diff/10001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/wallpaper.html:6: For example:</p> This text should go in permissions.json: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... see pattern there. as discussed you need to - un-nodoc wallpaper.setWallpaper - add a description to the schema. https://codereview.chromium.org/27335003/diff/10001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/wallpaper.html:22: </p> This paragraph isn't much use. You can make it better: "For example, to set the wallpaper as the image at <code>http://example.com/a_file.png", .....
Thanks for review. As for extension, I added you to the comment in the wallpaper api design doc. I assumed that we are fine with just enable it for app. But if you disagree, we can definitely continue discussion in the design doc. https://codereview.chromium.org/27335003/diff/10001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/intros/wallpaper.html (right): https://codereview.chromium.org/27335003/diff/10001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/wallpaper.html:6: For example:</p> On 2013/10/16 21:17:27, kalman wrote: > This text should go in permissions.json: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte... > > see pattern there. > > as discussed you need to > - un-nodoc wallpaper.setWallpaper > - add a description to the schema. Done. https://codereview.chromium.org/27335003/diff/10001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/wallpaper.html:22: </p> On 2013/10/16 21:17:27, kalman wrote: > This paragraph isn't much use. You can make it better: > > "For example, to set the wallpaper as the image at > <code>http://example.com/a_file.png", ..... Done.
yes let's have that discussion separately about the extensions vs apps. https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... File chrome/common/extensions/api/wallpaper.json (right): https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... chrome/common/extensions/api/wallpaper.json:17: "description": "Sets wallpaper to the image at 'url' or 'wallpaperData' with the specified layout", <code>url</code> and <code>wallpaperData</code> https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... chrome/common/extensions/api/wallpaper.json:57: "description": "The jpeg encoded wallpaper thumbnail(128x60)." could you explain this (128x60) thing? Does it means it's automatically scaled? that it needs to be these dimensions? also space between thumbnail and the parens. https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/intros/wallpaper.html (right): https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/wallpaper.html:5: <a href="declare_permissions.html">host permissions</a> for the URL in the move this into permissions/wallpaper.html https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/wallpaper.html:33: }, function() { console.error(chrome.runtime.lastError.message); }); why are you printing out the error? is this function likely to throw an error? I assume not. https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/json/permissions.json (right): https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... chrome/common/extensions/docs/templates/json/permissions.json:57: "partial": "permissions/wallpaper.html", you need to actually add this file. it should contain the stuff in wallpaper.html pertaining to the permission. https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... chrome/common/extensions/docs/templates/json/permissions.json:58: "platforms": ["app"] you don't need the platform, docserver can figure it out from the features file.
https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... File chrome/common/extensions/api/wallpaper.json (right): https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... chrome/common/extensions/api/wallpaper.json:17: "description": "Sets wallpaper to the image at 'url' or 'wallpaperData' with the specified layout", On 2013/10/17 18:18:20, kalman wrote: > <code>url</code> and <code>wallpaperData</code> Done. https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... chrome/common/extensions/api/wallpaper.json:57: "description": "The jpeg encoded wallpaper thumbnail(128x60)." On 2013/10/17 18:18:20, kalman wrote: > could you explain this (128x60) thing? Does it means it's automatically scaled? > that it needs to be these dimensions? > > also space between thumbnail and the parens. Done. https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/intros/wallpaper.html (right): https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/wallpaper.html:5: <a href="declare_permissions.html">host permissions</a> for the URL in the It looks like the permissions/wallpaper.html should only describe the permission itself? I added the file and described the wallpaper permission in that file. On 2013/10/17 18:18:20, kalman wrote: > move this into permissions/wallpaper.html https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/wallpaper.html:33: }, function() { console.error(chrome.runtime.lastError.message); }); If the XMLRequest failed (user forgot to specify host permission) or the url is not an image, it could throw errors. I put is here just to show that we can get error message from chrome.runtime.lastError. But it is probably not necessary here in the doc? On 2013/10/17 18:18:20, kalman wrote: > why are you printing out the error? is this function likely to throw an error? I > assume not. https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/json/permissions.json (right): https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... chrome/common/extensions/docs/templates/json/permissions.json:57: "partial": "permissions/wallpaper.html", On 2013/10/17 18:18:20, kalman wrote: > you need to actually add this file. it should contain the stuff in > wallpaper.html pertaining to the permission. Done. https://codereview.chromium.org/27335003/diff/40001/chrome/common/extensions/... chrome/common/extensions/docs/templates/json/permissions.json:58: "platforms": ["app"] On 2013/10/17 18:18:20, kalman wrote: > you don't need the platform, docserver can figure it out from the features file. Done. https://codereview.chromium.org/27335003/diff/51001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/private/permissions/wallpaper.html (right): https://codereview.chromium.org/27335003/diff/51001/chrome/common/extensions/... chrome/common/extensions/docs/templates/private/permissions/wallpaper.html:1: Required if the app uses the <a href="wallpaper.html">chrome.wallpaper</a> API to change ChromeOS wallpaper. This format is based on permissions/web_request_blocking.html I am not sure where is this html file being used though. I thought it might be in the declare_permissions.html. But I was wrong. Do you know what's the link that I can check for this file?
Ah damn it I pointed you at the wrong file :\ sorry! It should have been intro_tables.json not permissions.json.
On 2013/10/17 20:41:46, kalman wrote: > Ah damn it I pointed you at the wrong file :\ sorry! It should have been > intro_tables.json not permissions.json. Aha. I see. Yep. Move it to intro_tables.json add the "host permission" link to the api intro table.
Why does this require host permissions? You can just go <img src=...>. https://codereview.chromium.org/27335003/diff/60001/chrome/common/extensions/... File chrome/common/extensions/docs/templates/intros/wallpaper.html (right): https://codereview.chromium.org/27335003/diff/60001/chrome/common/extensions/... chrome/common/extensions/docs/templates/intros/wallpaper.html:4: 'url' property of setWallpaper, you must also specify "you must declare..." can be left out, the intro table should cover it. "if you use the url property..." well firstly why does that require host permissions? And secondly, you can put that text in intro_tables.json.
On 2013/10/18 00:18:38, kalman wrote: > Why does this require host permissions? You can just go <img src=...>. > > https://codereview.chromium.org/27335003/diff/60001/chrome/common/extensions/... > File chrome/common/extensions/docs/templates/intros/wallpaper.html (right): > > https://codereview.chromium.org/27335003/diff/60001/chrome/common/extensions/... > chrome/common/extensions/docs/templates/intros/wallpaper.html:4: 'url' property > of setWallpaper, you must also specify > "you must declare..." can be left out, the intro table should cover it. > > "if you use the url property..." well firstly why does that require host > permissions? And secondly, you can put that text in intro_tables.json. Hi guys. I have landed two CLs to remove the dependency of host permission: 44563002 and 44183003 I have updated the doc. Would you mind to take another look?
lgtm https://codereview.chromium.org/27335003/diff/178001/chrome/common/extensions... File chrome/common/extensions/api/wallpaper.json (right): https://codereview.chromium.org/27335003/diff/178001/chrome/common/extensions... chrome/common/extensions/api/wallpaper.json:17: "description": "Sets wallpaper to the image at <code>url</code> or <code>wallpaperData</code> with the specified layout", I may be contradicting myself now, but for referring to other properties use <em>url</em> not <code>url</code> etc. https://codereview.chromium.org/27335003/diff/178001/chrome/common/extensions... File chrome/common/extensions/docs/templates/intros/wallpaper.html (right): https://codereview.chromium.org/27335003/diff/178001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/wallpaper.html:23: <pre> maybe data-filename="background.js" here? https://codereview.chromium.org/27335003/diff/178001/chrome/common/extensions... File chrome/common/extensions/docs/templates/json/intro_tables.json (right): https://codereview.chromium.org/27335003/diff/178001/chrome/common/extensions... chrome/common/extensions/docs/templates/json/intro_tables.json:280: }, you don't need to change this file; the server can figure this out https://codereview.chromium.org/27335003/diff/178001/chrome/common/extensions... File chrome/common/extensions/docs/templates/json/permissions.json (right): https://codereview.chromium.org/27335003/diff/178001/chrome/common/extensions... chrome/common/extensions/docs/templates/json/permissions.json:58: "partial": "permissions/wallpaper.html" there isn't any page at "permissions/wallpaper.html" right? Unless you're actually documenting the permission (which you're not; you're documenting the API) you shouldn't need to change this file either.
Done. Thanks for review. https://codereview.chromium.org/27335003/diff/178001/chrome/common/extensions... File chrome/common/extensions/api/wallpaper.json (right): https://codereview.chromium.org/27335003/diff/178001/chrome/common/extensions... chrome/common/extensions/api/wallpaper.json:17: "description": "Sets wallpaper to the image at <code>url</code> or <code>wallpaperData</code> with the specified layout", On 2013/11/14 21:55:55, kalman wrote: > I may be contradicting myself now, but for referring to other properties use > <em>url</em> not <code>url</code> etc. Done. https://codereview.chromium.org/27335003/diff/178001/chrome/common/extensions... File chrome/common/extensions/docs/templates/intros/wallpaper.html (right): https://codereview.chromium.org/27335003/diff/178001/chrome/common/extensions... chrome/common/extensions/docs/templates/intros/wallpaper.html:23: <pre> Not sure if this should belong to background.js, other file can also call this function too. On 2013/11/14 21:55:55, kalman wrote: > maybe data-filename="background.js" here? https://codereview.chromium.org/27335003/diff/178001/chrome/common/extensions... File chrome/common/extensions/docs/templates/json/intro_tables.json (right): https://codereview.chromium.org/27335003/diff/178001/chrome/common/extensions... chrome/common/extensions/docs/templates/json/intro_tables.json:280: }, On 2013/11/14 21:55:55, kalman wrote: > you don't need to change this file; the server can figure this out Done. https://codereview.chromium.org/27335003/diff/178001/chrome/common/extensions... File chrome/common/extensions/docs/templates/json/permissions.json (right): https://codereview.chromium.org/27335003/diff/178001/chrome/common/extensions... chrome/common/extensions/docs/templates/json/permissions.json:58: "partial": "permissions/wallpaper.html" On 2013/11/14 21:55:55, kalman wrote: > there isn't any page at "permissions/wallpaper.html" right? Unless you're > actually documenting the permission (which you're not; you're documenting the > API) you shouldn't need to change this file either. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bshe@chromium.org/27335003/318001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bshe@chromium.org/27335003/428001
Message was sent while issue was closed.
Change committed as 235758 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
