Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
732943002:
New Getting started example for extensions (Closed)
|
Created:
6 years, 1 month ago by robwu Modified:
5 years, 11 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. |
DescriptionNew Getting started example for extensions
Replaces the outdated Kittens tutorial with a new getting started tutorial that uses the Google Image search API.
BUG=383385
NOTRY=true
R=kalman@chromium.org,mkearney@chromium.org
Committed: https://crrev.com/093cfa5a4f1ca3aaa1343fe5c8b3279ca5dff2ff
Cr-Commit-Position: refs/heads/master@{#311485}
Patch Set 1 #
Total comments: 15
Patch Set 2 : Review comments #2 #Patch Set 3 : Update tutorial #
Total comments: 4
Patch Set 4 : verify response #
Total comments: 25
Patch Set 5 : review comments #21 #Messages
Total messages: 29 (2 generated)
The previous sample was too basic, and was basically a web page embedded in an extension, so I've created a new example. The new example shows: - How to use the asynchronous extension API (one of the FAQ on Stack Overflow). - How to query the current tab's state. - How to perform a cross-origin HTTP request. - How to display the result in the popup. If the sample code looks okay, then I will update the tutorial itself (chrome/common/extensions/docs/templates/articles/getstarted.html).
https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/examples/tutorials/getstarted/popup.html (right): https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.html:7: font-family: "Segoe UI", "Lucida Grande", Tahoma, sans-serif; You shouldn't need fonts, we set these automatically now. Same for scaling, unless you really want 100% font size for some reason. https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.html:15: } I thikn better would be to define a rule [hidden] { display: none !important; } then explicitly set the img to be 'hidden', rather than magical 'src' matching properties. https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js (right): https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js:32: // manifest.json. You should be able to rely on activeTab to get the URL. No need to specify the tabs permission. https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js:64: // Parse and process the response from Google Image Search. Does it reply with any sizing info? It'd be nice to put the correct size on the img. https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js:104: }); You could do just document.addEventListener('DOMContentLoaded', main);
Processed your feedback. If the code looks fine, I will update the accompanying documentation page. https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/examples/tutorials/getstarted/popup.html (right): https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.html:7: font-family: "Segoe UI", "Lucida Grande", Tahoma, sans-serif; On 2014/11/17 22:02:57, kalman wrote: > You shouldn't need fonts, we set these automatically now. Really? Also for the extension popup? > > Same for scaling, unless you really want 100% font size for some reason. After a recent update, the fonts became much smaller than expected for one of my extensions. Choosing an explicit font size ensures that the extension continues to look as intended. https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.html:15: } On 2014/11/17 22:02:57, kalman wrote: > I thikn better would be to define a rule > > [hidden] { > display: none !important; > } > > then explicitly set the img to be 'hidden', rather than magical 'src' matching > properties. Okay, I've added hidden to the <img>. Removed this declaration (and did not put [hidden] back because it already hides by default). https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js (right): https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js:32: // manifest.json. On 2014/11/17 22:02:57, kalman wrote: > You should be able to rely on activeTab to get the URL. No need to specify the > tabs permission. Done. https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js:64: // Parse and process the response from Google Image Search. On 2014/11/17 22:02:57, kalman wrote: > Does it reply with any sizing info? It'd be nice to put the correct size on the > img. Yes, the size is available. I have not used it though, because the purpose of the tutorial is to show how to develop an extension using a third-party API, not teaching the specifics of a certain external web service. If it's worth to mention, I can put a suggestion ("exercise for the reader") in the tutorial page. https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js:104: }); On 2014/11/17 22:02:57, kalman wrote: > You could do just > > document.addEventListener('DOMContentLoaded', main); Done.
lgtm but let's quickly continue the img size question https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/examples/tutorials/getstarted/popup.html (right): https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.html:7: font-family: "Segoe UI", "Lucida Grande", Tahoma, sans-serif; On 2014/11/17 22:26:11, robwu wrote: > On 2014/11/17 22:02:57, kalman wrote: > > You shouldn't need fonts, we set these automatically now. > > Really? Also for the extension popup? Yes, we always set fonts. The options page stuff additionally sets some form styling. > > > > > Same for scaling, unless you really want 100% font size for some reason. > > After a recent update, the fonts became much smaller than expected for one of my > extensions. Choosing an explicit font size ensures that the extension continues > to look as intended. Yes, probably that font change. It sets the size to whatever the "platform default" is (usually 75% apparently). https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js (right): https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js:64: // Parse and process the response from Google Image Search. On 2014/11/17 22:26:11, robwu wrote: > On 2014/11/17 22:02:57, kalman wrote: > > Does it reply with any sizing info? It'd be nice to put the correct size on > the > > img. > > Yes, the size is available. I have not used it though, because the purpose of > the tutorial is to show how to develop an extension using a third-party API, not > teaching the specifics of a certain external web service. If it's worth to > mention, I can put a suggestion ("exercise for the reader") in the tutorial > page. The practical effect would be to avoid unnecessary popup resizing. Do you notice a different setting an explicit size once the image is fetched?
https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js (right): https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js:64: // Parse and process the response from Google Image Search. On 2014/11/17 23:14:03, kalman wrote: > The practical effect would be to avoid unnecessary popup resizing. Do you notice > a different setting an explicit size once the image is fetched? What did you mean by that? Whether I can observe that the page resizes after the image is fetched? If so, yes. But that will happens regardless of whether I set the image size now or later. If I set it upfront, then the page resizes in this callback; if I don't set the size, the page will resize once the image is loaded.
https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js (right): https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js:64: // Parse and process the response from Google Image Search. On 2014/11/17 23:18:54, robwu wrote: > On 2014/11/17 23:14:03, kalman wrote: > > The practical effect would be to avoid unnecessary popup resizing. Do you > notice > > a different setting an explicit size once the image is fetched? > > What did you mean by that? Whether I can observe that the page resizes after the > image is fetched? If so, yes. But that will happens regardless of whether I set > the image size now or later. If I set it upfront, then the page resizes in this > callback; if I don't set the size, the page will resize once the image is > loaded. An advantage of setting it up-front is that there's less pop-in. The popup will resize immediately (and if you wanted you could show a spinner or something) and then load later. I think it's good practice to avoid pop-in as much as possible, so code along those lines + comment explaining it would be good (for a first extension).
https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... File chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js (right): https://codereview.chromium.org/732943002/diff/1/chrome/common/extensions/doc... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js:64: // Parse and process the response from Google Image Search. On 2014/11/17 23:24:05, kalman wrote: > On 2014/11/17 23:18:54, robwu wrote: > > On 2014/11/17 23:14:03, kalman wrote: > > > The practical effect would be to avoid unnecessary popup resizing. Do you > > notice > > > a different setting an explicit size once the image is fetched? > > > > What did you mean by that? Whether I can observe that the page resizes after > the > > image is fetched? If so, yes. But that will happens regardless of whether I > set > > the image size now or later. If I set it upfront, then the page resizes in > this > > callback; if I don't set the size, the page will resize once the image is > > loaded. > > An advantage of setting it up-front is that there's less pop-in. The popup will > resize immediately (and if you wanted you could show a spinner or something) and > then load later. > > I think it's good practice to avoid pop-in as much as possible, so code along > those lines + comment explaining it would be good (for a first extension). Okay, sounds plausible. I will submit a change tomorrow.
Updated the tutorial! I know that there was a preview URL on the appserver, but I cannot recall the exact details. The page at http://www.chromium.org/developers/design-documents/extensions/how-the-extens... is extremely outdated. Could you fix that page when you have time, Ben?
On 2014/11/18 16:01:43, robwu wrote: > Updated the tutorial! > > I know that there was a preview URL on the appserver, but I cannot recall the > exact details. The page at > http://www.chromium.org/developers/design-documents/extensions/how-the-extens... > is extremely outdated. Could you fix that page when you have time, Ben? Yikes. Filed crbug.com/434360.
kalman@chromium.org changed reviewers: + mkearney@chromium.org
lgtm, but Meggin any chance you could go over the article change? https://codereview.chromium.org/732943002/diff/40001/chrome/common/extensions... File chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js (right): https://codereview.chromium.org/732943002/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js:80: callback(imageUrl, width, height); You might want to verify imageUrl, and catch width/height being NaN and show an error, rather than relying on them being correct.
https://codereview.chromium.org/732943002/diff/40001/chrome/common/extensions... File chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js (right): https://codereview.chromium.org/732943002/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js:80: callback(imageUrl, width, height); On 2014/11/18 16:09:37, kalman wrote: > You might want to verify imageUrl, and catch width/height being NaN and show an > error, rather than relying on them being correct. In principle I agree with verifying external data, but because the API is frozen, I don't think that it's necessary to verify that the width/height/url is correct. Here's the reference: https://developers.google.com/image-search/v1/jsondevguide#json_reference.
https://codereview.chromium.org/732943002/diff/40001/chrome/common/extensions... File chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js (right): https://codereview.chromium.org/732943002/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js:80: callback(imageUrl, width, height); On 2014/11/18 16:13:15, robwu wrote: > On 2014/11/18 16:09:37, kalman wrote: > > You might want to verify imageUrl, and catch width/height being NaN and show > an > > error, rather than relying on them being correct. > > In principle I agree with verifying external data, but because the API is > frozen, I don't think that it's necessary to verify that the width/height/url is > correct. > Here's the reference: > https://developers.google.com/image-search/v1/jsondevguide#json_reference. Frozen APIs are well and good, but bugs and issues can happen all over the place. Data validation is good, especially when it's sourced externally.
https://codereview.chromium.org/732943002/diff/40001/chrome/common/extensions... File chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js (right): https://codereview.chromium.org/732943002/diff/40001/chrome/common/extensions... chrome/common/extensions/docs/examples/tutorials/getstarted/popup.js:80: callback(imageUrl, width, height); On 2014/11/18 16:15:20, kalman wrote: > On 2014/11/18 16:13:15, robwu wrote: > > On 2014/11/18 16:09:37, kalman wrote: > > > You might want to verify imageUrl, and catch width/height being NaN and show > > an > > > error, rather than relying on them being correct. > > > > In principle I agree with verifying external data, but because the API is > > frozen, I don't think that it's necessary to verify that the width/height/url > is > > correct. > > Here's the reference: > > https://developers.google.com/image-search/v1/jsondevguide#json_reference. > > Frozen APIs are well and good, but bugs and issues can happen all over the > place. Data validation is good, especially when it's sourced externally. Done.
Ping mkearney@ for review.
On 2014/11/26 17:51:10, robwu wrote: > Ping mkearney@ for review. Ping mkearney@
Hey, guys I've added this as a todo task for this week. Is there urgency to have it done early in the week? Meggin On Tue, Dec 2, 2014 at 10:51 AM, <rob@robwu.nl> wrote: > On 2014/11/26 17:51:10, robwu wrote: > >> Ping mkearney@ for review. >> > > Ping mkearney@ > > https://codereview.chromium.org/732943002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/12/02 19:58:04, chromium-reviews wrote: > Hey, guys > > I've added this as a todo task for this week. > > Is there urgency to have it done early in the week? > > Meggin I'd rate it medium urgency. The example from the previous tutorial has been non-functional for a few months already, so a few days more or less won't make a huge difference.
Meggin, when can you review this CL?
On 2014/12/09 19:16:48, robwu wrote: > Meggin, when can you review this CL? Meggin, pretty please? :)
https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... File chrome/common/extensions/docs/templates/articles/getstarted.html (right): https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:10: image from Google using the current page's URL as search term. "as search term" should be "as a search term". https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:41: the <a href="manifest">documentation of manifest.json</a>. Instead of "documentation of manifest.json", replace with doc's actual title: "Manifest File Format documentation". https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:45: For our example, we will declare a <a href="browserAction">browser action</a>, Replace "For our example" with "In our example's manifest". https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:63: "permissions": [ Add comments in the sample code that call out the above-mentioned items in the manifest. https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:77: Is there a reason you dropped line-by-line breakdown of the manifest? I find it helpful for beginners. https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:151: alt="The menu's icon is three horizontal bars.">. and Remove unnecessary period after "> and before and. It's been lingering there for awhile! https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:192: <br> Remove this <br>. https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:193: For example, let's set a tooltip on the browser action button. According to End paragraph after "For example, let's set a tooltip on the browser action button." And then start a new paragraph with "According to...". https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:195: <code>default_title</code> key in the manifest file. So open Remove 'So' and start sentence with 'Open'. https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:196: <code>manifest.json</code>, and add this property to <code>browser_action</code>. Change 'this property' to 'the default_title to the'. https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:214: see the previous changes in action, the extension has to be reloaded. This can The last two sentences in this paragraph are wordy and a bit buggy. Also, I don't see a Reload button for the entire extensions page. Can we make this very simple? How about: Visit the extensions page (go to chrome://extensions or select Tools>Extensions under the Chrome menu)and click Reload under your extension.
https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... File chrome/common/extensions/docs/templates/articles/getstarted.html (right): https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:10: image from Google using the current page's URL as search term. On 2015/01/07 22:30:23, mkearney1 wrote: > "as search term" should be "as a search term". Done. https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:41: the <a href="manifest">documentation of manifest.json</a>. On 2015/01/07 22:30:23, mkearney1 wrote: > Instead of "documentation of manifest.json", replace with doc's actual title: > "Manifest File Format documentation". Done. https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:45: For our example, we will declare a <a href="browserAction">browser action</a>, On 2015/01/07 22:30:23, mkearney1 wrote: > Replace "For our example" with "In our example's manifest". Done. https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:63: "permissions": [ On 2015/01/07 22:30:23, mkearney1 wrote: > Add comments in the sample code that call out the above-mentioned items in the > manifest. Done. https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:77: On 2015/01/07 22:30:23, mkearney1 wrote: > Is there a reason you dropped line-by-line breakdown of the manifest? I find it > helpful for beginners. name/description/version are obvious. The first thing that the user sees when they load the extension is - name, description and version. The meaning of browser_action is also obvious, because the paragraph before this code section announces that the tutorial is going to show a browser action, and the section after the manifest file explains the meaning of the keys within browser_action. The permissions section and the specific values are also explained in the paragraph before this code snippet. The only thing that is not explicitly explained any more is "manifest_version". If users are really curious about this field, they can click on the link to the manifest reference that was just shown before the previous paragraph. https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:151: alt="The menu's icon is three horizontal bars.">. and On 2015/01/07 22:30:24, mkearney1 wrote: > Remove unnecessary period after "> and before and. It's been lingering there for > awhile! Done. https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:192: <br> On 2015/01/07 22:30:23, mkearney1 wrote: > Remove this <br>. Done. https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:193: For example, let's set a tooltip on the browser action button. According to On 2015/01/07 22:30:23, mkearney1 wrote: > End paragraph after "For example, let's set a tooltip on the browser action > button." And then start a new paragraph with "According to...". Done. https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:195: <code>default_title</code> key in the manifest file. So open On 2015/01/07 22:30:23, mkearney1 wrote: > Remove 'So' and start sentence with 'Open'. Done. https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:196: <code>manifest.json</code>, and add this property to <code>browser_action</code>. On 2015/01/07 22:30:23, mkearney1 wrote: > Change 'this property' to 'the default_title to the'. Done. https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:214: see the previous changes in action, the extension has to be reloaded. This can On 2015/01/07 22:30:23, mkearney1 wrote: > The last two sentences in this paragraph are wordy and a bit buggy. Also, I > don't see a Reload button for the entire extensions page. At the left of the omnibox. > Can we make this very simple? > > How about: Visit the extensions page (go to chrome://extensions or select > Tools>Extensions under the Chrome menu)and click Reload under your extension. I've made this paragraph a bit more concise: https://codereview.chromium.org/732943002/diff2/60001:80001/chrome/common/ext...
lgtm... with one more fix. I'm not seeing comments added to the sample manifest-- maybe they didn't get uploaded in the patch? https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... File chrome/common/extensions/docs/templates/articles/getstarted.html (right): https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:63: "permissions": [ I still don't see the comments. Weird. On 2015/01/07 22:30:23, mkearney1 wrote: > Add comments in the sample code that call out the above-mentioned items in the > manifest.
https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... File chrome/common/extensions/docs/templates/articles/getstarted.html (right): https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:63: "permissions": [ On 2015/01/08 23:00:34, mkearney1 wrote: > I still don't see the comments. Weird. > On 2015/01/07 22:30:23, mkearney1 wrote: > > Add comments in the sample code that call out the above-mentioned items in the > > manifest. > I added a comment to popup.html that explained the relation between popup.html and the manifest file. I'm a bit hesitant with adding comments to manifest.json, because although Chrome loads such JSON files just fine, the Chrome Web Store used to reject manifest.json if it is not strict JSON (as defined by json.org), which means that comments are not supported (https://crbug.com/172863#c9). I don't know whether this restriction still exists, but since comments are not a part of standard JSON, I'm not too keen on adding it to the JSON file.
https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... File chrome/common/extensions/docs/templates/articles/getstarted.html (right): https://codereview.chromium.org/732943002/diff/60001/chrome/common/extensions... chrome/common/extensions/docs/templates/articles/getstarted.html:63: "permissions": [ On 2015/01/08 23:09:03, robwu wrote: > On 2015/01/08 23:00:34, mkearney1 wrote: > > I still don't see the comments. Weird. > > On 2015/01/07 22:30:23, mkearney1 wrote: > > > Add comments in the sample code that call out the above-mentioned items in > the > > > manifest. > > > > I added a comment to popup.html that explained the relation between popup.html > and the manifest file. > > I'm a bit hesitant with adding comments to manifest.json, because although > Chrome loads such JSON files just fine, the Chrome Web Store used to reject > manifest.json if it is not strict JSON (as defined by http://json.org), which means > that comments are not supported (https://crbug.com/172863#c9). I don't know > whether this restriction still exists, but since comments are not a part of > standard JSON, I'm not too keen on adding it to the JSON file. I just tested again (14 jan 2015), and the Chrome Web Store still rejects extensions with comments in manifest.json (// and /* */).
The CQ bit was checked by rob@robwu.nl
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/732943002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/093cfa5a4f1ca3aaa1343fe5c8b3279ca5dff2ff Cr-Commit-Position: refs/heads/master@{#311485} |
