Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22938005:
Add ErrorConsole UI for Extension Install Warnings (Closed)
|
Created:
7 years, 4 months ago by Devlin Modified:
7 years, 3 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, arv+watch_chromium.org, extensions-reviews_chromium.org, not at google - send to devlin, Dan Beam Base URL:
https://chromium.googlesource.com/chromium/src.git@dc_ec_install_warnings Visibility:
Public. |
DescriptionAdd Error Console UI for install warnings to the chrome:extensions page (hidden behind the error console switch).
Snazzy Images: http://imgur.com/a/7QnMo#0 (updated 9/3)
BUG=21734
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221527
Patch Set 1 : #Patch Set 2 : #
Total comments: 21
Patch Set 3 : Rebase to Master #
Total comments: 20
Patch Set 4 : Yoyo's #Patch Set 5 : More robust highlighting + testing #
Total comments: 4
Patch Set 6 : Yoz's, Round II #
Total comments: 4
Patch Set 7 : Yoyo's + temporarily remove *.png for apply issue #
Total comments: 79
Patch Set 8 : Dan's #
Total comments: 32
Patch Set 9 : Dan's, round II #Patch Set 10 : Requested UI Changes #
Total comments: 34
Patch Set 11 : Dan's, III #Patch Set 12 : #Patch Set 13 : Rebase to exclude backend #
Total comments: 6
Patch Set 14 : Ben's #Patch Set 15 : Latest Master (-binary data) #Patch Set 16 : License #Messages
Total messages: 37 (0 generated)
https://codereview.chromium.org/22938005/diff/50001/chrome/browser/extensions... File chrome/browser/extensions/error_console/error_console.cc (right): https://codereview.chromium.org/22938005/diff/50001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console.cc:183: ReportError(scoped_ptr<const ExtensionError>(new ManifestParsingError( Seeing this makes me wonder whether it's right to call these ManifestParsingErrors. After all, they don't prevent the manifest from loading. Thoughts? https://codereview.chromium.org/22938005/diff/50001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console.cc:236: // refresh of chrome:extensions, and we don't want to wipe our history Does this notification get sent when unpacked extensions are reloaded? (The refresh of chrome://extensions only reloads unpacked extensions.) https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right): https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:61: // If the feature is a string, we may need to account for the ending quote. and quoted = true? https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:70: // run off the end. And do we need to worry about comments? Probably you want to use something like JSONParser::EatComments beforehand. https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:104: // |start| can point to a location at which we start our search, and will be This "can" phrasing is confusing. How about: |start| contains a location at which... https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:106: // |end| can point to a location which we should not pass, and will be populated |end| contains a location we should not pass for the beginning of the feature... (FindFeatureEnd ignores *end) https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:114: i < *end && i < manifest.size() && i != std::string::npos; ++i) { What's the npos check for? https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... File chrome/browser/ui/webui/extensions/extension_error_handler.h (right): https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.h:39: // Handle the "requestFileSource" call. Seems like this should have a name pertaining to manifests if it has manifest-specifics. https://codereview.chromium.org/22938005/diff/50001/extensions/browser/extens... File extensions/browser/extension_error.h (right): https://codereview.chromium.org/22938005/diff/50001/extensions/browser/extens... extensions/browser/extension_error.h:142: void DetermineExtensionId(); We (aa and I) had a debate about this a long time ago and decided on "ID" rather than "Id". Is there some reason to change this? https://codereview.chromium.org/22938005/diff/57001/chrome/browser/extensions... File chrome/browser/extensions/error_console/error_console_browsertest.cc (right): https://codereview.chromium.org/22938005/diff/57001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console_browsertest.cc:39: // Verify that all properites of a given |error| are correct. typo: properties https://codereview.chromium.org/22938005/diff/57001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console_browsertest.cc:147: // Load the extension at |path| wait for |expected_errors| errors. nit: missing an and https://codereview.chromium.org/22938005/diff/57001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console_browsertest.cc:148: // Populates |extension| with a pointer to the loaded extension. This could return Extension*. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console_browsertest.cc:198: if (temp_error->manifest_key() == base::UTF8ToUTF16("permissions")) { This is more extensible if you write it like: for (size_t i = 0; ...) { if (errors[i]->key() == ..."permissions") permissions_error = errors[i]; else if (... == ..."fakepermission") unknown_key_error = errors[i]; } ASSERT_TRUE(permissions_error); ASSERT_TRUE(unknown_key_error); https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error.js:1: // Copyright 2013 The Chromium Authors. All rights reserved. P.S. You might want to find a qualified Javascript reviewer for this. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error.js:9: * Returns whether or not a given |link| is associated with an extension. Why use 'link' here when you are referring to URLs? 'url' would be clearer. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error.js:25: function getRelativeLink(link, extensionUrl) { Same here - I would call this getRelativePath(url, extensionUrl) https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... File chrome/browser/resources/extensions/extension_error_overlay.js (right): https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error_overlay.js:61: source = source.replace(/ /g, ' ').replace(/\n/g, '<br/>'); ? https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error_overlay.js:85: // Update the C++ call so this isn't necessary. Is this a TODO? https://codereview.chromium.org/22938005/diff/57001/chrome/browser/ui/webui/e... File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right): https://codereview.chromium.org/22938005/diff/57001/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:260: // If we never found bounds for the feature, then there will be no highlighted It looks like we might highlight everything.
https://codereview.chromium.org/22938005/diff/57001/chrome/browser/extensions... File chrome/browser/extensions/error_console/error_console_browsertest.cc (right): https://codereview.chromium.org/22938005/diff/57001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console_browsertest.cc:39: // Verify that all properites of a given |error| are correct. On 2013/08/16 00:17:26, Yoyo Zhou wrote: > typo: properties Done. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console_browsertest.cc:147: // Load the extension at |path| wait for |expected_errors| errors. On 2013/08/16 00:17:26, Yoyo Zhou wrote: > nit: missing an and Done. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console_browsertest.cc:148: // Populates |extension| with a pointer to the loaded extension. On 2013/08/16 00:17:26, Yoyo Zhou wrote: > This could return Extension*. Unfortunately, it can't. Use of ASSERT* in a function means that it has to have a void return type. If we substitute the ASSERTs for CHECKs, we can return any type - but this kinda goes against testing practice. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console_browsertest.cc:198: if (temp_error->manifest_key() == base::UTF8ToUTF16("permissions")) { On 2013/08/16 00:17:26, Yoyo Zhou wrote: > This is more extensible if you write it like: > for (size_t i = 0; ...) { > if (errors[i]->key() == ..."permissions") > permissions_error = errors[i]; > else if (... == ..."fakepermission") > unknown_key_error = errors[i]; > } > ASSERT_TRUE(permissions_error); > ASSERT_TRUE(unknown_key_error); Done. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error.js:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2013/08/16 00:17:26, Yoyo Zhou wrote: > P.S. You might want to find a qualified Javascript reviewer for this. Yep :) I'm gonna run c/b/r/e and c/b/ui/webui by OWNERS; just making sure that nothing needs to be completely restructured first. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error.js:9: * Returns whether or not a given |link| is associated with an extension. On 2013/08/16 00:17:26, Yoyo Zhou wrote: > Why use 'link' here when you are referring to URLs? 'url' would be clearer. My thinking was that things like "manifest.json" or even "event_bindings" aren't really urls. But I guess "link" isn't any better. Changed. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error.js:25: function getRelativeLink(link, extensionUrl) { On 2013/08/16 00:17:26, Yoyo Zhou wrote: > Same here - I would call this getRelativePath(url, extensionUrl) Done. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... File chrome/browser/resources/extensions/extension_error_overlay.js (right): https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error_overlay.js:61: source = source.replace(/ /g, ' ').replace(/\n/g, '<br/>'); On 2013/08/16 00:17:26, Yoyo Zhou wrote: > ? Huh. I wonder why that worked... oh well. Fixed. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error_overlay.js:85: // Update the C++ call so this isn't necessary. On 2013/08/16 00:17:26, Yoyo Zhou wrote: > Is this a TODO? I was following the pattern of PackExtension/Commands Overlays, but looking into it more, might as well just fix 'em all. =) https://codereview.chromium.org/22849012/ https://codereview.chromium.org/22938005/diff/57001/chrome/browser/ui/webui/e... File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right): https://codereview.chromium.org/22938005/diff/57001/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:260: // If we never found bounds for the feature, then there will be no highlighted On 2013/08/16 00:17:26, Yoyo Zhou wrote: > It looks like we might highlight everything. D'oh. Fixed.
https://codereview.chromium.org/22938005/diff/57001/chrome/browser/extensions... File chrome/browser/extensions/error_console/error_console_browsertest.cc (right): https://codereview.chromium.org/22938005/diff/57001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console_browsertest.cc:39: // Verify that all properites of a given |error| are correct. On 2013/08/16 00:17:26, Yoyo Zhou wrote: > typo: properties Done. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console_browsertest.cc:147: // Load the extension at |path| wait for |expected_errors| errors. On 2013/08/16 00:17:26, Yoyo Zhou wrote: > nit: missing an and Done. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console_browsertest.cc:148: // Populates |extension| with a pointer to the loaded extension. On 2013/08/16 00:17:26, Yoyo Zhou wrote: > This could return Extension*. Unfortunately, it can't. Use of ASSERT* in a function means that it has to have a void return type. If we substitute the ASSERTs for CHECKs, we can return any type - but this kinda goes against testing practice. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console_browsertest.cc:198: if (temp_error->manifest_key() == base::UTF8ToUTF16("permissions")) { On 2013/08/16 00:17:26, Yoyo Zhou wrote: > This is more extensible if you write it like: > for (size_t i = 0; ...) { > if (errors[i]->key() == ..."permissions") > permissions_error = errors[i]; > else if (... == ..."fakepermission") > unknown_key_error = errors[i]; > } > ASSERT_TRUE(permissions_error); > ASSERT_TRUE(unknown_key_error); Done. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error.js:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2013/08/16 00:17:26, Yoyo Zhou wrote: > P.S. You might want to find a qualified Javascript reviewer for this. Yep :) I'm gonna run c/b/r/e and c/b/ui/webui by OWNERS; just making sure that nothing needs to be completely restructured first. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error.js:9: * Returns whether or not a given |link| is associated with an extension. On 2013/08/16 00:17:26, Yoyo Zhou wrote: > Why use 'link' here when you are referring to URLs? 'url' would be clearer. My thinking was that things like "manifest.json" or even "event_bindings" aren't really urls. But I guess "link" isn't any better. Changed. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error.js:25: function getRelativeLink(link, extensionUrl) { On 2013/08/16 00:17:26, Yoyo Zhou wrote: > Same here - I would call this getRelativePath(url, extensionUrl) Done. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... File chrome/browser/resources/extensions/extension_error_overlay.js (right): https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error_overlay.js:61: source = source.replace(/ /g, ' ').replace(/\n/g, '<br/>'); On 2013/08/16 00:17:26, Yoyo Zhou wrote: > ? Huh. I wonder why that worked... oh well. Fixed. https://codereview.chromium.org/22938005/diff/57001/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error_overlay.js:85: // Update the C++ call so this isn't necessary. On 2013/08/16 00:17:26, Yoyo Zhou wrote: > Is this a TODO? I was following the pattern of PackExtension/Commands Overlays, but looking into it more, might as well just fix 'em all. =) https://codereview.chromium.org/22849012/ https://codereview.chromium.org/22938005/diff/57001/chrome/browser/ui/webui/e... File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right): https://codereview.chromium.org/22938005/diff/57001/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:260: // If we never found bounds for the feature, then there will be no highlighted On 2013/08/16 00:17:26, Yoyo Zhou wrote: > It looks like we might highlight everything. D'oh. Fixed.
LGTM
https://codereview.chromium.org/22938005/diff/50001/chrome/browser/extensions... File chrome/browser/extensions/error_console/error_console.cc (right): https://codereview.chromium.org/22938005/diff/50001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console.cc:183: ReportError(scoped_ptr<const ExtensionError>(new ManifestParsingError( On 2013/08/16 00:17:25, Yoyo Zhou wrote: > Seeing this makes me wonder whether it's right to call these > ManifestParsingErrors. After all, they don't prevent the manifest from loading. > Thoughts? I've been wondering that as well... How about a general rename from ManifestParsingError -> ManifestError (this will also keep the door open for if we decide we want to include blocking errors from ExtensionErrorReporter), and JavascriptRuntimeError -> RuntimeError? Unfortunately, the "Error" parts (of these and ExtensionError) are still somewhat misnomers, since they could be warnings or even just logs - but I can't think of a more accurate blanket term. https://codereview.chromium.org/22938005/diff/50001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console.cc:236: // refresh of chrome:extensions, and we don't want to wipe our history On 2013/08/16 00:17:25, Yoyo Zhou wrote: > Does this notification get sent when unpacked extensions are reloaded? (The > refresh of chrome://extensions only reloads unpacked extensions.) Yep, it does. Which means that any time the page was refreshed, all the runtime errors would be wiped. https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right): https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:61: // If the feature is a string, we may need to account for the ending quote. On 2013/08/16 00:17:25, Yoyo Zhou wrote: > and quoted = true? Nope. If we have: ... "permissions": [ "tabs", "history" ], ... ^ The index right now will point to the location right after the feature name ends - in this case, the arrowed quote mark. We want to skip that one, and not have quoted = true, because we don't count the other side. https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:70: // run off the end. On 2013/08/16 00:17:25, Yoyo Zhou wrote: > And do we need to worry about comments? Probably you want to use something like > JSONParser::EatComments beforehand. This, coupled with a few other edge cases I hadn't thought of, made this larger (mostly because I wanted to add tests for it). Mind taking another look? c/b/manifest_highlighter* https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:104: // |start| can point to a location at which we start our search, and will be On 2013/08/16 00:17:25, Yoyo Zhou wrote: > This "can" phrasing is confusing. How about: |start| contains a location at > which... Done. https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:106: // |end| can point to a location which we should not pass, and will be populated On 2013/08/16 00:17:25, Yoyo Zhou wrote: > |end| contains a location we should not pass for the beginning of the feature... > (FindFeatureEnd ignores *end) Done. https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:114: i < *end && i < manifest.size() && i != std::string::npos; ++i) { On 2013/08/16 00:17:25, Yoyo Zhou wrote: > What's the npos check for? Yeah, I suppose that it's redundant with checking that it's less than size, since npos is guaranteed to be huge. Taken out. https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... File chrome/browser/ui/webui/extensions/extension_error_handler.h (right): https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.h:39: // Handle the "requestFileSource" call. On 2013/08/16 00:17:25, Yoyo Zhou wrote: > Seems like this should have a name pertaining to manifests if it has > manifest-specifics. It's in preparation for the more-generic call for either manifest or source files (which is why there's also a file-type key in the call itself, even though only manifest are here right now). I'd put the rest in, but it seemed like this patch was somewhat monstrous already, and didn't wanna overwhelm anyone more than I already was. https://codereview.chromium.org/22938005/diff/50001/extensions/browser/extens... File extensions/browser/extension_error.h (right): https://codereview.chromium.org/22938005/diff/50001/extensions/browser/extens... extensions/browser/extension_error.h:142: void DetermineExtensionId(); On 2013/08/16 00:17:25, Yoyo Zhou wrote: > We (aa and I) had a debate about this a long time ago and decided on "ID" rather > than "Id". Is there some reason to change this? My reasoning was: $ git grep "xtensionId" | wc >> 819 3356 98743 $ git grep "xtensionID" | wc >> 92 393 10591 It seems that "Id" is nearly 10x more common than ID for [e,E]xtensionI[d,D]... but I can always avoid making it 11x, if it's preferable? :)
https://codereview.chromium.org/22938005/diff/50001/chrome/browser/extensions... File chrome/browser/extensions/error_console/error_console.cc (right): https://codereview.chromium.org/22938005/diff/50001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console.cc:183: ReportError(scoped_ptr<const ExtensionError>(new ManifestParsingError( On 2013/08/16 23:56:54, D Cronin wrote: > On 2013/08/16 00:17:25, Yoyo Zhou wrote: > > Seeing this makes me wonder whether it's right to call these > > ManifestParsingErrors. After all, they don't prevent the manifest from > loading. > > Thoughts? > > I've been wondering that as well... > > How about a general rename from ManifestParsingError -> ManifestError (this will > also keep the door open for if we decide we want to include blocking errors from > ExtensionErrorReporter), and JavascriptRuntimeError -> RuntimeError? > > Unfortunately, the "Error" parts (of these and ExtensionError) are still > somewhat misnomers, since they could be warnings or even just logs - but I can't > think of a more accurate blanket term. ManifestError seems fine. If we want to have other kinds of runtime errors, though, we can add them (or rename JavascriptRuntimeError) later. https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... File chrome/browser/ui/webui/extensions/extension_error_handler.h (right): https://codereview.chromium.org/22938005/diff/50001/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.h:39: // Handle the "requestFileSource" call. On 2013/08/16 23:56:54, D Cronin wrote: > On 2013/08/16 00:17:25, Yoyo Zhou wrote: > > Seems like this should have a name pertaining to manifests if it has > > manifest-specifics. > > It's in preparation for the more-generic call for either manifest or source > files (which is why there's also a file-type key in the call itself, even though > only manifest are here right now). I'd put the rest in, but it seemed like this > patch was somewhat monstrous already, and didn't wanna overwhelm anyone more > than I already was. Ok, it just seems incongruous to have this generic-sounding requestFileSource and have the callback be GetManifestFileCallback. https://codereview.chromium.org/22938005/diff/88001/extensions/browser/manife... File extensions/browser/manifest_highlighter.cc (right): https://codereview.chromium.org/22938005/diff/88001/extensions/browser/manife... extensions/browser/manifest_highlighter.cc:85: void ManifestHighlighter::FindBoundsHelper(const std::string& feature, You could also call this FindBoundsEnd https://codereview.chromium.org/22938005/diff/88001/extensions/browser/manife... extensions/browser/manifest_highlighter.cc:109: if (c == '"') This is unfortunate, but we can also have \" inside ""s. Or the next " we find might be inside a comment. These are the perils of writing your own parser...
https://codereview.chromium.org/22938005/diff/50001/chrome/browser/extensions... File chrome/browser/extensions/error_console/error_console.cc (right): https://codereview.chromium.org/22938005/diff/50001/chrome/browser/extensions... chrome/browser/extensions/error_console/error_console.cc:183: ReportError(scoped_ptr<const ExtensionError>(new ManifestParsingError( On 2013/08/19 20:39:46, Yoyo Zhou wrote: > On 2013/08/16 23:56:54, D Cronin wrote: > > On 2013/08/16 00:17:25, Yoyo Zhou wrote: > > > Seeing this makes me wonder whether it's right to call these > > > ManifestParsingErrors. After all, they don't prevent the manifest from > > loading. > > > Thoughts? > > > > I've been wondering that as well... > > > > How about a general rename from ManifestParsingError -> ManifestError (this > will > > also keep the door open for if we decide we want to include blocking errors > from > > ExtensionErrorReporter), and JavascriptRuntimeError -> RuntimeError? > > > > Unfortunately, the "Error" parts (of these and ExtensionError) are still > > somewhat misnomers, since they could be warnings or even just logs - but I > can't > > think of a more accurate blanket term. > > ManifestError seems fine. If we want to have other kinds of runtime errors, > though, we can add them (or rename JavascriptRuntimeError) later. Done. https://codereview.chromium.org/22938005/diff/88001/extensions/browser/manife... File extensions/browser/manifest_highlighter.cc (right): https://codereview.chromium.org/22938005/diff/88001/extensions/browser/manife... extensions/browser/manifest_highlighter.cc:85: void ManifestHighlighter::FindBoundsHelper(const std::string& feature, On 2013/08/19 20:39:46, Yoyo Zhou wrote: > You could also call this FindBoundsEnd Done. https://codereview.chromium.org/22938005/diff/88001/extensions/browser/manife... extensions/browser/manifest_highlighter.cc:109: if (c == '"') On 2013/08/19 20:39:46, Yoyo Zhou wrote: > This is unfortunate, but we can also have \" inside ""s. > Or the next " we find might be inside a comment. > > These are the perils of writing your own parser... Escaped chars now handled. Commented "s aren't really possible, since we only search for quotes while already in a string literal, and strings take precedence over comments (i.e. "/* foo */" is a string literal with /* foo */, rather than an empty string.) Tests added for both.
LGTM https://codereview.chromium.org/22938005/diff/93001/extensions/browser/manife... File extensions/browser/manifest_highlighter.cc (right): https://codereview.chromium.org/22938005/diff/93001/extensions/browser/manife... extensions/browser/manifest_highlighter.cc:23: return index < string.size() ? index : std::string::npos; Should this test use found? https://codereview.chromium.org/22938005/diff/93001/extensions/browser/manife... extensions/browser/manifest_highlighter.cc:139: void ManifestHighlighter::CommentSafeIncrement(size_t* index) { nit: seems like this and FindNextQuote are in the same family, so they should both be in the anon namespace and have the same interface.
https://codereview.chromium.org/22938005/diff/93001/extensions/browser/manife... File extensions/browser/manifest_highlighter.cc (right): https://codereview.chromium.org/22938005/diff/93001/extensions/browser/manife... extensions/browser/manifest_highlighter.cc:23: return index < string.size() ? index : std::string::npos; On 2013/08/19 21:48:51, Yoyo Zhou wrote: > Should this test use found? Done. https://codereview.chromium.org/22938005/diff/93001/extensions/browser/manife... extensions/browser/manifest_highlighter.cc:139: void ManifestHighlighter::CommentSafeIncrement(size_t* index) { On 2013/08/19 21:48:51, Yoyo Zhou wrote: > nit: seems like this and FindNextQuote are in the same family, so they should > both be in the anon namespace and have the same interface. Done.
+ben for c/b/r/extensions
I'm happy to take this review but I also don't know how I ended up in that OWNERS file. Perhaps somebody who knows more about webui should take a look as well. danbeam?
https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error.css (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.css:25: .extension-error-simple-wrapper >:first-child { nit: .extension-error-simple-wrapper > :first-child { https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error.html (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.html:4: * in the LICENSE file. nit: ^ s/ * // https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.html:15: <img></img> ^ this is odd, can you add this via JS instead? https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:21: * @param {url} The url to make relative. @param {string} url The url to make relative. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:22: * @param {extensionUrl} The host for which the url is relative. @param {string} extensionURL The host for which the url is relative. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:36: return $('template-collection-extension-error') nit: operators at end return $('template-collection-extension-error'). querySelector(templateName).cloneNode(true); https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:43: * @return {HTMLElement} The created error element. nit: remove @return https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:43: * @return {HTMLElement} The created error element. * @constructor * @extends {HTMLDivElement} https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:66: 'extension-error-icon-error'; nit: something like this is easier to read, IMO if (this.error_.level == 0) iconNode.className = 'extension-error-icon-log'; else if (this.error_.level == 1) iconNode.className = 'extension-error-icon-warn'; else iconNode.className = 'extension-error-icon-error'; https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:73: // The error message... ^ this seems to be self-evident https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:77: // The error source... ^ and this (so comments probably aren't required) https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:89: * @param {string} url The url to the resource to view. * @private https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:99: * @return {boolean} Whether or not we can view the source for the url. * @private https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:103: url === 'manifest.json'; nit: s/===/==/ IMO https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:109: * @return {HTMLElement} The clickable node to view the source. * @private https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:118: [{ nit: chrome.send('extensionErrorRequestFileSource', [{'extensionId': this.error_.extensionId, ... 'specific': this.error_.manifestSpecific}]); https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:128: } nit: }, https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:130: /** * What an ExtensionErrorList does in prose as a doc comment. * @constructor * @extends {HTMLDivElement} */ https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:142: kMaxErrorsToShow_: 3, /** * @private * @const */ MAX_ERRORS_TO_SHOW_: 3, https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:150: }.bind(this)); }, this); (forEach() takes a thisArg) https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:154: i < this.contents_.children.length; ++i) nit: curlies https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:159: /** @private */ https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:161: var button = this.querySelector('.extension-error-list-show-more') this.querySelector('.extension-error-list-show-more a') https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:165: button.addEventListener('click', function(e) { nit: for (var i = this.MAX_ERRORS_TO_SHOW_; i < this.contents_.children.length; ++i) { this.contents_.children[i].hidden = button.isShowingAll; } var msg = button.isShowingAll ? 'extensionErrorsShowMore' : 'extensionErrorsShowFewer'; button.innerText = loadTimeData.getString(msg); https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error_overlay.html (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.html:6: <div id="extensionErrorOverlay" class="page"> ids-like-this, i.e. extension-error-overlay https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.html:9: <div class="content-area" id="extensionErrorOverlayContent"></div> extension-error-overlay-content https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.html:13: <button id="extensionErrorOverlayDismiss" i18n-content="ok"></button> extension-error-overlay-dismiss https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error_overlay.js (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.js:7: /** @constructor */ https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.js:17: initializePage: function() { does this override anything? https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.js:29: * @param {Event} e The click event. @private https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.js:42: }; ^ er, what's the point of this? https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:317: node.querySelector('.manifest-errors').hidden = false; nit: only query selector once, IMO var errors = node.querySelector('.manifest-errors'); errors.hidden = false; errors.appendChild( new extensions.ExtensionErrorList(extension.manifestErrors)); https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:139: extensionErrorOverlay.initializePage(); nit: extensions.ExtensionErrorOverlay.getInstance().initializePage(); https://codereview.chromium.org/22938005/diff/100002/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:7: #include "base/bind.h" #include "base/location.h" for FROM_HERE https://codereview.chromium.org/22938005/diff/100002/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:91: CHECK(args->GetSize() == 1); CHECK_EQ(1U, args->GetSize()); https://codereview.chromium.org/22938005/diff/100002/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:145: } else { // currently, only manifest file types supported. nit: } else { // Only manifest file type are currently supported. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_error_handler.h (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.h:7: #include <string> #include "base/basictypes.h" https://codereview.chromium.org/22938005/diff/100002/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.h:9: #include "base/strings/string16.h" ^ unused https://codereview.chromium.org/22938005/diff/100002/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.h:14: class DictionaryValue; nit: alpha
All addressed. Thanks for the very thorough review, and sorry my JS is so rusty. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error.css (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.css:25: .extension-error-simple-wrapper >:first-child { On 2013/08/20 21:39:26, Dan Beam wrote: > nit: .extension-error-simple-wrapper > :first-child { Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error.html (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.html:4: * in the LICENSE file. On 2013/08/20 21:39:26, Dan Beam wrote: > nit: ^ s/ * // Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.html:15: <img></img> On 2013/08/20 21:39:26, Dan Beam wrote: > ^ this is odd, can you add this via JS instead? Sure thing. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:21: * @param {url} The url to make relative. On 2013/08/20 21:39:26, Dan Beam wrote: > @param {string} url The url to make relative. D'oh. Fixed. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:22: * @param {extensionUrl} The host for which the url is relative. On 2013/08/20 21:39:26, Dan Beam wrote: > @param {string} extensionURL The host for which the url is relative. Is chrome JS style 'URL' or 'Url'? Kept 'Url' here, since this was the only comment with URL, but lemme know if this is wrong. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:36: return $('template-collection-extension-error') On 2013/08/20 21:39:26, Dan Beam wrote: > nit: operators at end > > return $('template-collection-extension-error'). > querySelector(templateName).cloneNode(true); Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:43: * @return {HTMLElement} The created error element. On 2013/08/20 21:39:26, Dan Beam wrote: > nit: remove @return Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:43: * @return {HTMLElement} The created error element. On 2013/08/20 21:39:26, Dan Beam wrote: > * @constructor > * @extends {HTMLDivElement} Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:66: 'extension-error-icon-error'; On 2013/08/20 21:39:26, Dan Beam wrote: > nit: something like this is easier to read, IMO > > if (this.error_.level == 0) > iconNode.className = 'extension-error-icon-log'; > else if (this.error_.level == 1) > iconNode.className = 'extension-error-icon-warn'; > else > iconNode.className = 'extension-error-icon-error'; Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:73: // The error message... On 2013/08/20 21:39:26, Dan Beam wrote: > ^ this seems to be self-evident Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:77: // The error source... On 2013/08/20 21:39:26, Dan Beam wrote: > ^ and this (so comments probably aren't required) Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:89: * @param {string} url The url to the resource to view. On 2013/08/20 21:39:26, Dan Beam wrote: > * @private Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:99: * @return {boolean} Whether or not we can view the source for the url. On 2013/08/20 21:39:26, Dan Beam wrote: > * @private Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:103: url === 'manifest.json'; On 2013/08/20 21:39:26, Dan Beam wrote: > nit: s/===/==/ IMO Mmkay (it's definitely not done enough for any of the performance benefits of === to kick in). Assuming this preference extends to the whole file, and replaced it in isExtensionUrl() - lemme know if this is wrong. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:109: * @return {HTMLElement} The clickable node to view the source. On 2013/08/20 21:39:26, Dan Beam wrote: > * @private Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:118: [{ On 2013/08/20 21:39:26, Dan Beam wrote: > nit: > > chrome.send('extensionErrorRequestFileSource', > [{'extensionId': this.error_.extensionId, > ... > 'specific': this.error_.manifestSpecific}]); Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:128: } On 2013/08/20 21:39:26, Dan Beam wrote: > nit: }, Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:130: On 2013/08/20 21:39:26, Dan Beam wrote: > /** > * What an ExtensionErrorList does in prose as a doc comment. > * @constructor > * @extends {HTMLDivElement} > */ Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:142: kMaxErrorsToShow_: 3, On 2013/08/20 21:39:26, Dan Beam wrote: > /** > * @private > * @const > */ > MAX_ERRORS_TO_SHOW_: 3, Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:150: }.bind(this)); On 2013/08/20 21:39:26, Dan Beam wrote: > }, this); (forEach() takes a thisArg) Ooh, much cleaner. :) https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:154: i < this.contents_.children.length; ++i) On 2013/08/20 21:39:26, Dan Beam wrote: > nit: curlies Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:159: On 2013/08/20 21:39:26, Dan Beam wrote: > /** @private */ Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:161: var button = this.querySelector('.extension-error-list-show-more') On 2013/08/20 21:39:26, Dan Beam wrote: > this.querySelector('.extension-error-list-show-more a') nifty. Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:165: button.addEventListener('click', function(e) { On 2013/08/20 21:39:26, Dan Beam wrote: > nit: > > for (var i = this.MAX_ERRORS_TO_SHOW_; > i < this.contents_.children.length; ++i) { > this.contents_.children[i].hidden = button.isShowingAll; > } > var msg = button.isShowingAll ? 'extensionErrorsShowMore' : > 'extensionErrorsShowFewer'; > button.innerText = loadTimeData.getString(msg); Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error_overlay.html (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.html:6: <div id="extensionErrorOverlay" class="page"> On 2013/08/20 21:39:26, Dan Beam wrote: > ids-like-this, i.e. extension-error-overlay Following nearby code strikes again. Note to self: fix extension_commands_overlay.html to match in a smaller CL. Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.html:9: <div class="content-area" id="extensionErrorOverlayContent"></div> On 2013/08/20 21:39:26, Dan Beam wrote: > extension-error-overlay-content Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.html:13: <button id="extensionErrorOverlayDismiss" i18n-content="ok"></button> On 2013/08/20 21:39:26, Dan Beam wrote: > extension-error-overlay-dismiss Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error_overlay.js (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.js:7: On 2013/08/20 21:39:26, Dan Beam wrote: > /** @constructor */ Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.js:17: initializePage: function() { On 2013/08/20 21:39:26, Dan Beam wrote: > does this override anything? I don't *believe* so. There's an identically-named initalizePage() method in OptionsPage, but I don't think that these overlays are directly related to that class (since we don't set the __proto__ anywhere). Also, the Kiosk pages (in c/b/r/e/chromeos/) have a method which performs analogously, and is just called initialize(). So, I'm pretty sure these are just common names for ease in extensions.js. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.js:29: * @param {Event} e The click event. On 2013/08/20 21:39:26, Dan Beam wrote: > @private Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.js:42: }; On 2013/08/20 21:39:26, Dan Beam wrote: > ^ er, what's the point of this? To teach me that, no matter how much I look at a CL, there's always gonna be at least one blatant mistake I miss. Sorry :/ Removed. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... File chrome/browser/resources/extensions/extension_list.js (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extension_list.js:317: node.querySelector('.manifest-errors').hidden = false; On 2013/08/20 21:39:26, Dan Beam wrote: > nit: only query selector once, IMO > > var errors = node.querySelector('.manifest-errors'); > errors.hidden = false; > errors.appendChild( > new extensions.ExtensionErrorList(extension.manifestErrors)); Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... File chrome/browser/resources/extensions/extensions.js (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/resources... chrome/browser/resources/extensions/extensions.js:139: extensionErrorOverlay.initializePage(); On 2013/08/20 21:39:26, Dan Beam wrote: > nit: extensions.ExtensionErrorOverlay.getInstance().initializePage(); Done (and fixed neighbors). https://codereview.chromium.org/22938005/diff/100002/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:7: #include "base/bind.h" On 2013/08/20 21:39:26, Dan Beam wrote: > #include "base/location.h" for FROM_HERE Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:91: CHECK(args->GetSize() == 1); On 2013/08/20 21:39:26, Dan Beam wrote: > CHECK_EQ(1U, args->GetSize()); Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:145: } else { // currently, only manifest file types supported. On 2013/08/20 21:39:26, Dan Beam wrote: > nit: > > } else { > // Only manifest file type are currently supported. Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_error_handler.h (right): https://codereview.chromium.org/22938005/diff/100002/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.h:7: On 2013/08/20 21:39:26, Dan Beam wrote: > #include <string> > > #include "base/basictypes.h" Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.h:9: #include "base/strings/string16.h" On 2013/08/20 21:39:26, Dan Beam wrote: > ^ unused Done. https://codereview.chromium.org/22938005/diff/100002/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.h:14: class DictionaryValue; On 2013/08/20 21:39:26, Dan Beam wrote: > nit: alpha Done.
very very close https://chromiumcodereview.appspot.com/22938005/diff/100002/chrome/browser/re... File chrome/browser/resources/extensions/extension_error_overlay.html (right): https://chromiumcodereview.appspot.com/22938005/diff/100002/chrome/browser/re... chrome/browser/resources/extensions/extension_error_overlay.html:6: <div id="extensionErrorOverlay" class="page"> On 2013/08/20 23:06:51, D Cronin wrote: > On 2013/08/20 21:39:26, Dan Beam wrote: > > ids-like-this, i.e. extension-error-overlay > > Following nearby code strikes again. Note to self: fix > extension_commands_overlay.html to match in a smaller CL. > Done. yes and that'd be great (this is spelled out in the style guide but we haven't made mass fixes yet: http://www.chromium.org/developers/web-development-style-guide) https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/res... File chrome/browser/resources/extensions/extension_error.css (right): https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/res... chrome/browser/resources/extensions/extension_error.css:6: cursor: pointer; ^ why is this necessary? https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/res... chrome/browser/resources/extensions/extension_error.css:21: display: -webkit-box; why is this a box with nothing flexing? https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/res... File chrome/browser/resources/extensions/extension_error.js (right): https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/res... chrome/browser/resources/extensions/extension_error.js:108: url == 'manifest.json'; nit: think this fits in one line https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/res... chrome/browser/resources/extensions/extension_error.js:158: * @const nit: forgot @type {number} https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/res... File chrome/browser/resources/extensions/extension_error_overlay.js (right): https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/res... chrome/browser/resources/extensions/extension_error_overlay.js:62: source = source.replace(/ /g, ' ').replace(/\n/g, '<br/>'); s/<br\/>/<br>/, also what about \r or \n\n? https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/res... File chrome/browser/resources/extensions/extensions.css (right): https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/res... chrome/browser/resources/extensions/extensions.css:249: background: pink; lol https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/ui/... File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right): https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/ui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:11: #include "base/strings/utf_string_conversions.h" #include "base/strings/string16.h" (sorry, see below as to why) https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/ui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:43: void AddHighlightedToDictionaryForOverlay(base::DictionaryValue* dict, nit: HighlightDictionary https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/ui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:46: const std::string& after_highlight) { nit: just pass a const ManifestHighlighter& highlighter instead of the public API, piece by piece :P https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/ui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:48: dict->SetString(kBeforeHighlightKey, base::UTF8ToUTF16(before_highlight)); nit: \n https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/ui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:50: dict->SetString(kHighlightKey, base::UTF8ToUTF16(highlight)); nit: \n https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/ui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:100: base::string16 error_message; ^ i lied, you do need to include string16, sorry... https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/ui/... File chrome/browser/ui/webui/extensions/extension_error_handler.h (right): https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/ui/... chrome/browser/ui/webui/extensions/extension_error_handler.h:6: #define CHROME_BROWSER_UI_WEBUI_EXTENSIONS_EXTENSION_ERROR_HANDLER_H_ #include <string> https://chromiumcodereview.appspot.com/22938005/diff/93002/chrome/browser/ui/... chrome/browser/ui/webui/extensions/extension_error_handler.h:23: namespace extensions { nit: \n https://chromiumcodereview.appspot.com/22938005/diff/93002/extensions/browser... File extensions/browser/manifest_highlighter.h (right): https://chromiumcodereview.appspot.com/22938005/diff/93002/extensions/browser... extensions/browser/manifest_highlighter.h:37: std::string GetBeforeFeature() const; nit: \n between basically all these members/methods
https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... File chrome/browser/resources/extensions/extension_error.css (right): https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error.css:6: cursor: pointer; On 2013/08/21 23:41:03, Dan Beam wrote: > ^ why is this necessary? It's for the "View source" links next to the errors. Since they aren't links, per se, they won't have a pointer cursor by default. The other standard options are to use href="#" or href="javascript:void(0)" - are either of these preferable? https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error.css:21: display: -webkit-box; On 2013/08/21 23:41:03, Dan Beam wrote: > why is this a box with nothing flexing? -webkit-box, -webkit-flex, -webkit-inline-box, and -webkit-inline-flex all allow the "View source" link to be shown on the same line as the error message; all other display options force it onto a new line (http://imgur.com/ATWI0TO). Is there a preferable css combination for this effect? https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error.js:108: url == 'manifest.json'; On 2013/08/21 23:41:03, Dan Beam wrote: > nit: think this fits in one line Done. https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error.js:158: * @const On 2013/08/21 23:41:03, Dan Beam wrote: > nit: forgot @type {number} Done. https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... File chrome/browser/resources/extensions/extension_error_overlay.js (right): https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error_overlay.js:62: source = source.replace(/ /g, ' ').replace(/\n/g, '<br/>'); On 2013/08/21 23:41:03, Dan Beam wrote: > s/<br\/>/<br>/, also what about \r or \n\n? \r addressed. What's the issue with \n\n? Isn't desired behavior to just replace it with two <br>s? https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right): https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:11: #include "base/strings/utf_string_conversions.h" On 2013/08/21 23:41:03, Dan Beam wrote: > #include "base/strings/string16.h" (sorry, see below as to why) Done. https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:43: void AddHighlightedToDictionaryForOverlay(base::DictionaryValue* dict, On 2013/08/21 23:41:03, Dan Beam wrote: > nit: HighlightDictionary Done. https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:46: const std::string& after_highlight) { On 2013/08/21 23:41:03, Dan Beam wrote: > nit: just pass a const ManifestHighlighter& highlighter instead of the public > API, piece by piece :P Done. https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:48: dict->SetString(kBeforeHighlightKey, base::UTF8ToUTF16(before_highlight)); On 2013/08/21 23:41:03, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:50: dict->SetString(kHighlightKey, base::UTF8ToUTF16(highlight)); On 2013/08/21 23:41:03, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.cc:100: base::string16 error_message; On 2013/08/21 23:41:03, Dan Beam wrote: > ^ i lied, you do need to include string16, sorry... haha no worries. Done. https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... File chrome/browser/ui/webui/extensions/extension_error_handler.h (right): https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.h:6: #define CHROME_BROWSER_UI_WEBUI_EXTENSIONS_EXTENSION_ERROR_HANDLER_H_ On 2013/08/21 23:41:03, Dan Beam wrote: > > #include <string> Done. https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... chrome/browser/ui/webui/extensions/extension_error_handler.h:23: namespace extensions { On 2013/08/21 23:41:03, Dan Beam wrote: > nit: \n Done. https://codereview.chromium.org/22938005/diff/93002/extensions/browser/manife... File extensions/browser/manifest_highlighter.h (right): https://codereview.chromium.org/22938005/diff/93002/extensions/browser/manife... extensions/browser/manifest_highlighter.h:37: std::string GetBeforeFeature() const; On 2013/08/21 23:41:03, Dan Beam wrote: > nit: \n between basically all these members/methods Done.
On 2013/08/22 18:29:05, D Cronin wrote: > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... > File chrome/browser/resources/extensions/extension_error.css (right): > > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... > chrome/browser/resources/extensions/extension_error.css:6: cursor: pointer; > On 2013/08/21 23:41:03, Dan Beam wrote: > > ^ why is this necessary? > > It's for the "View source" links next to the errors. Since they aren't links, > per se, they won't have a pointer cursor by default. > > The other standard options are to use href="#" or href="javascript:void(0)" - > are either of these preferable? > > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... > chrome/browser/resources/extensions/extension_error.css:21: display: > -webkit-box; > On 2013/08/21 23:41:03, Dan Beam wrote: > > why is this a box with nothing flexing? > > -webkit-box, -webkit-flex, -webkit-inline-box, and -webkit-inline-flex all allow > the "View source" link to be shown on the same line as the error message; all > other display options force it onto a new line (http://imgur.com/ATWI0TO). Is > there a preferable css combination for this effect? > > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... > File chrome/browser/resources/extensions/extension_error.js (right): > > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... > chrome/browser/resources/extensions/extension_error.js:108: url == > 'manifest.json'; > On 2013/08/21 23:41:03, Dan Beam wrote: > > nit: think this fits in one line > > Done. > > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... > chrome/browser/resources/extensions/extension_error.js:158: * @const > On 2013/08/21 23:41:03, Dan Beam wrote: > > nit: forgot @type {number} > > Done. > > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... > File chrome/browser/resources/extensions/extension_error_overlay.js (right): > > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... > chrome/browser/resources/extensions/extension_error_overlay.js:62: source = > source.replace(/ /g, ' ').replace(/\n/g, '<br/>'); > On 2013/08/21 23:41:03, Dan Beam wrote: > > s/<br\/>/<br>/, also what about \r or \n\n? > > \r addressed. What's the issue with \n\n? Isn't desired behavior to just > replace it with two <br>s? > > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... > File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right): > > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... > chrome/browser/ui/webui/extensions/extension_error_handler.cc:11: #include > "base/strings/utf_string_conversions.h" > On 2013/08/21 23:41:03, Dan Beam wrote: > > #include "base/strings/string16.h" (sorry, see below as to why) > > Done. > > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... > chrome/browser/ui/webui/extensions/extension_error_handler.cc:43: void > AddHighlightedToDictionaryForOverlay(base::DictionaryValue* dict, > On 2013/08/21 23:41:03, Dan Beam wrote: > > nit: HighlightDictionary > > Done. > > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... > chrome/browser/ui/webui/extensions/extension_error_handler.cc:46: const > std::string& after_highlight) { > On 2013/08/21 23:41:03, Dan Beam wrote: > > nit: just pass a const ManifestHighlighter& highlighter instead of the public > > API, piece by piece :P > > Done. > > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... > chrome/browser/ui/webui/extensions/extension_error_handler.cc:48: > dict->SetString(kBeforeHighlightKey, base::UTF8ToUTF16(before_highlight)); > On 2013/08/21 23:41:03, Dan Beam wrote: > > nit: \n > > Done. > > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... > chrome/browser/ui/webui/extensions/extension_error_handler.cc:50: > dict->SetString(kHighlightKey, base::UTF8ToUTF16(highlight)); > On 2013/08/21 23:41:03, Dan Beam wrote: > > nit: \n > > Done. > > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... > chrome/browser/ui/webui/extensions/extension_error_handler.cc:100: > base::string16 error_message; > On 2013/08/21 23:41:03, Dan Beam wrote: > > ^ i lied, you do need to include string16, sorry... > > haha no worries. Done. > > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... > File chrome/browser/ui/webui/extensions/extension_error_handler.h (right): > > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... > chrome/browser/ui/webui/extensions/extension_error_handler.h:6: #define > CHROME_BROWSER_UI_WEBUI_EXTENSIONS_EXTENSION_ERROR_HANDLER_H_ > On 2013/08/21 23:41:03, Dan Beam wrote: > > > > #include <string> > > Done. > > https://codereview.chromium.org/22938005/diff/93002/chrome/browser/ui/webui/e... > chrome/browser/ui/webui/extensions/extension_error_handler.h:23: namespace > extensions { > On 2013/08/21 23:41:03, Dan Beam wrote: > > nit: \n > > Done. > > https://codereview.chromium.org/22938005/diff/93002/extensions/browser/manife... > File extensions/browser/manifest_highlighter.h (right): > > https://codereview.chromium.org/22938005/diff/93002/extensions/browser/manife... > extensions/browser/manifest_highlighter.h:37: std::string GetBeforeFeature() > const; > On 2013/08/21 23:41:03, Dan Beam wrote: > > nit: \n between basically all these members/methods > > Done. friendly ping :) Also, the UI folks requested a couple (fairly small) changes, which have been added. Additionally, I added you on an email thread relating to this project (primarily its UI) so that you have a bit more context for what it is, where it's going, etc. Cheers
Remind me please where the entry point to this from the IPC message is.
On 2013/08/23 20:01:53, Cris Neckar wrote: > Remind me please where the entry point to this from the IPC message is. There's no entry point in this one. In short, this project reports both runtime and manifest errors for extensions. Runtime ones needed WebKit support, since JS is run from that side. Manifest errors are exclusively in Chrome, so there is no IPC entry. I'll put a comment on the Runtime side issue for where the entry point is. Cheers :)
dbeam: friendly ping :)
https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... File chrome/browser/resources/extensions/extension_error.css (right): https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error.css:6: cursor: pointer; On 2013/08/22 18:29:06, D Cronin wrote: > On 2013/08/21 23:41:03, Dan Beam wrote: > > ^ why is this necessary? > > It's for the "View source" links next to the errors. Since they aren't links, > per se, they won't have a pointer cursor by default. > > The other standard options are to use href="#" or href="javascript:void(0)" - > are either of these preferable? I think href="#" or <button class="link-button"> are common for this, yes https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error.css:21: display: -webkit-box; On 2013/08/22 18:29:06, D Cronin wrote: > On 2013/08/21 23:41:03, Dan Beam wrote: > > why is this a box with nothing flexing? > > -webkit-box, -webkit-flex, -webkit-inline-box, and -webkit-inline-flex all allow > the "View source" link to be shown on the same line as the error message; all > other display options force it onto a new line (http://imgur.com/ATWI0TO). Is > there a preferable css combination for this effect? white-space: nowrap; if I understand what your preferred UI should look like. https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... File chrome/browser/resources/extensions/extension_error_overlay.js (right): https://codereview.chromium.org/22938005/diff/93002/chrome/browser/resources/... chrome/browser/resources/extensions/extension_error_overlay.js:62: source = source.replace(/ /g, ' ').replace(/\n/g, '<br/>'); On 2013/08/22 18:29:06, D Cronin wrote: > On 2013/08/21 23:41:03, Dan Beam wrote: > > s/<br\/>/<br>/, also what about \r or \n\n? > > \r addressed. What's the issue with \n\n? Isn't desired behavior to just > replace it with two <br>s? oh, sorry, \n\n should be fine because of /g flag, however ...replace(/[\s\t]+/g, ' ').replace(/[\n\r]+/g, '<br>') collapses duplicate whitespace (which may or may not be desired) https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error.css (right): https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.css:38: .extension-error-warn .extension-error-message { hmmm, error-warn and error-error are awkward. extension-notification-errors extension-notification-warnings (though maybe there are other things that mean "notification")? https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error.html (right): https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.html:15: <img class="extension-error-icon"></img> using img with an empty [src] can (possibly) reload the relative URL "", which is the same page (which reloads the same page, etc.). I recommend doing any image creation in JS instead. https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:37: querySelector(templateName).cloneNode(true); maybe: querySelector('.' + templateName) and remove '.' from everywhere else? https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:41: * Creates a new ExtensionError HTMLElement. ^ this is kind of a lacking description. what does this do? maybe, "Used to show a notification when extension errors occur." (or something like that). https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:87: * link to do so. Otherwise, return a plain-text node. ^ is this still returning a text node? https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:89: * a human-friendly description the location (e.g., filename, line). nit: i think it'd be better to just explain what should be passed with @param, not exactly how it's used. @param {string} description A human-friendly description the location (e.g., filename, line). https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:95: if (this.canViewSource_(url)) { nit: no curlies https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:100: node.className = 'extension-error-view-source'; what does setting the classname of a text node do? https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:107: * @param {string} url The url to the resource to view. nit: the url _of_ the resource to view? https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:144: * the ExtensionList in chrome://extensions, if errors are present. nit: this whole paragraph could be shortened to: A variable length list of runtime or manifest errors for a given extension. IMO https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:184: * Initialize the "Show More" button for the error list, if there are more nit: Initialize the "Show More" button for the error list. If there are more... https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error_overlay.css (right): https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.css:6: font-family: monospace; does this take into account the user's monospace font preferences? https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error_overlay.js (right): https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.js:56: $('extension-error-overlay').querySelector( nit: document.querySelector( '#extension-error-overlay .extension-error-overlay-title'); IMO https://codereview.chromium.org/22938005/diff/141001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right): https://codereview.chromium.org/22938005/diff/141001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:40: const char kAfterHighlightKey[] = "afterHighlight"; nit: if it's all the same to you, could you alphabetize these and/or split between *Highlight* and non-*Highlight* keys? https://codereview.chromium.org/22938005/diff/141001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:118: extension_service()->GetExtensionById(extension_id, nit: it's possible L118-119 should be more indented https://codereview.chromium.org/22938005/diff/141001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:143: contents = new std::string; // Owned by GetManifestFileCallback() nit: end comment with . https://codereview.chromium.org/22938005/diff/141001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_error_handler.h (right): https://codereview.chromium.org/22938005/diff/141001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.h:49: // corresponds to the given |key| and |specific| locations. nit: mention ownership model of |dict| and |contents| (it's not apparent from looking at the header if they're in-params [which should be const or taken ownership of] or out-params [which should be overwritten])
https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error.css (right): https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.css:38: .extension-error-warn .extension-error-message { On 2013/08/29 00:24:30, Dan Beam (OOO until 9-23) wrote: > hmmm, error-warn and error-error are awkward. extension-notification-errors > extension-notification-warnings (though maybe there are other things that mean > "notification")? Yeah, I thought error-error was odd, too. But I think it's best to keep everything with the "ExtensionError" name, since that's what we're using everywhere else (C++ side and JS). How about extension-error-severity-info, ...severity-warning, and ...severity-fatal? Keeps extension-error prefix while still providing context and not sounding (quite) as weird? https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error.html (right): https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.html:15: <img class="extension-error-icon"></img> On 2013/08/29 00:24:30, Dan Beam (OOO until 9-23) wrote: > using img with an empty [src] can (possibly) reload the relative URL "", which > is the same page (which reloads the same page, etc.). I recommend doing any > image creation in JS instead. Done. https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error.js (right): https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:37: querySelector(templateName).cloneNode(true); On 2013/08/29 00:24:30, Dan Beam (OOO until 9-23) wrote: > maybe: querySelector('.' + templateName) and remove '.' from everywhere else? Done. https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:41: * Creates a new ExtensionError HTMLElement. On 2013/08/29 00:24:30, Dan Beam (OOO until 9-23) wrote: > ^ this is kind of a lacking description. what does this do? maybe, "Used to > show a notification when extension errors occur." (or something like that). Done. https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:87: * link to do so. Otherwise, return a plain-text node. On 2013/08/29 00:24:30, Dan Beam (OOO until 9-23) wrote: > ^ is this still returning a text node? Yeah - it returns a div with just plain text (and no link) if we can't view the source. I revised the comment to make it a bit more clear. https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:89: * a human-friendly description the location (e.g., filename, line). On 2013/08/29 00:24:30, Dan Beam (OOO until 9-23) wrote: > nit: i think it'd be better to just explain what should be passed with @param, > not exactly how it's used. > > @param {string} description A human-friendly description the location (e.g., > filename, line). Done. https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:95: if (this.canViewSource_(url)) { On 2013/08/29 00:24:30, Dan Beam (OOO until 9-23) wrote: > nit: no curlies Done. https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:100: node.className = 'extension-error-view-source'; On 2013/08/29 00:24:30, Dan Beam (OOO until 9-23) wrote: > what does setting the classname of a text node do? It's so the css can take effect. Right now, there's only one property for the class, but I thought it better to have all style in the css, when possible. https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:107: * @param {string} url The url to the resource to view. On 2013/08/29 00:24:30, Dan Beam (OOO until 9-23) wrote: > nit: the url _of_ the resource to view? Done. https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:144: * the ExtensionList in chrome://extensions, if errors are present. On 2013/08/29 00:24:30, Dan Beam (OOO until 9-23) wrote: > nit: this whole paragraph could be shortened to: > > A variable length list of runtime or manifest errors for a given extension. > > IMO Done. https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error.js:184: * Initialize the "Show More" button for the error list, if there are more On 2013/08/29 00:24:30, Dan Beam (OOO until 9-23) wrote: > nit: Initialize the "Show More" button for the error list. If there are more... Done. https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error_overlay.css (right): https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.css:6: font-family: monospace; On 2013/08/29 00:24:30, Dan Beam (OOO until 9-23) wrote: > does this take into account the user's monospace font preferences? Yep (font changes if I change my font prefs in chrome:settings). https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... File chrome/browser/resources/extensions/extension_error_overlay.js (right): https://codereview.chromium.org/22938005/diff/141001/chrome/browser/resources... chrome/browser/resources/extensions/extension_error_overlay.js:56: $('extension-error-overlay').querySelector( On 2013/08/29 00:24:30, Dan Beam (OOO until 9-23) wrote: > nit: > > document.querySelector( > '#extension-error-overlay .extension-error-overlay-title'); > > IMO Done. https://codereview.chromium.org/22938005/diff/141001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_error_handler.cc (right): https://codereview.chromium.org/22938005/diff/141001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:40: const char kAfterHighlightKey[] = "afterHighlight"; On 2013/08/29 00:24:30, Dan Beam (OOO until 9-23) wrote: > nit: if it's all the same to you, could you alphabetize these and/or split > between *Highlight* and non-*Highlight* keys? Done. https://codereview.chromium.org/22938005/diff/141001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:118: extension_service()->GetExtensionById(extension_id, On 2013/08/29 00:24:30, Dan Beam (OOO until 9-23) wrote: > nit: it's possible L118-119 should be more indented I've seen it both ways, so I think it's OWNERS preference. Changed. :) https://codereview.chromium.org/22938005/diff/141001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.cc:143: contents = new std::string; // Owned by GetManifestFileCallback() On 2013/08/29 00:24:30, Dan Beam (OOO until 9-23) wrote: > nit: end comment with . Done. https://codereview.chromium.org/22938005/diff/141001/chrome/browser/ui/webui/... File chrome/browser/ui/webui/extensions/extension_error_handler.h (right): https://codereview.chromium.org/22938005/diff/141001/chrome/browser/ui/webui/... chrome/browser/ui/webui/extensions/extension_error_handler.h:49: // corresponds to the given |key| and |specific| locations. On 2013/08/29 00:24:30, Dan Beam (OOO until 9-23) wrote: > nit: mention ownership model of |dict| and |contents| (it's not apparent from > looking at the header if they're in-params [which should be const or taken > ownership of] or out-params [which should be overwritten]) Done.
+ finnur for c/b/ui/webui/extensions, c/b/r/extensions Finnur, Dan left for vacation before he could finish the review. Would you mind taking over? (It should be nearly done.)
lgtm on behalf of danbeam who is OOO, I didn't really go over it in a lot of detail. https://codereview.chromium.org/22938005/diff/173001/extensions/browser/exten... File extensions/browser/extension_error.cc (right): https://codereview.chromium.org/22938005/diff/173001/extensions/browser/exten... extensions/browser/extension_error.cc:28: return false; this looks very similar to ExtensionSet::GetExtensionOrAppIDByURL. https://codereview.chromium.org/22938005/diff/173001/extensions/browser/exten... extensions/browser/extension_error.cc:71: return value.Pass(); consider using DictionaryBuilder here. https://codereview.chromium.org/22938005/diff/173001/extensions/browser/manif... File extensions/browser/manifest_highlighter.cc (right): https://codereview.chromium.org/22938005/diff/173001/extensions/browser/manif... extensions/browser/manifest_highlighter.cc:46: *index = i + 1; you might want to add a test with non-ascii characters as well
https://codereview.chromium.org/22938005/diff/173001/extensions/browser/exten... File extensions/browser/extension_error.cc (right): https://codereview.chromium.org/22938005/diff/173001/extensions/browser/exten... extensions/browser/extension_error.cc:28: return false; On 2013/09/03 22:51:46, kalman wrote: > this looks very similar to ExtensionSet::GetExtensionOrAppIDByURL. Whoops. That's old. Gone. https://codereview.chromium.org/22938005/diff/173001/extensions/browser/exten... extensions/browser/extension_error.cc:71: return value.Pass(); On 2013/09/03 22:51:46, kalman wrote: > consider using DictionaryBuilder here. Will do once we have value builder in extensions/. (added a TODO) https://codereview.chromium.org/22938005/diff/173001/extensions/browser/manif... File extensions/browser/manifest_highlighter.cc (right): https://codereview.chromium.org/22938005/diff/173001/extensions/browser/manif... extensions/browser/manifest_highlighter.cc:46: *index = i + 1; On 2013/09/03 22:51:46, kalman wrote: > you might want to add a test with non-ascii characters as well Added to the unittest.
Looks like you've got a green light from Dan via Ben. :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/22938005/1...
On 2013/09/04 12:01:20, Finnur wrote: > Looks like you've got a green light from Dan via Ben. :) Ben's only an OWNER in c/b/r/e, not c/b/ui/webui/e. :(
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
OWNERS LGTM stamp on c/b/ui/webui/e
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/22938005/1...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/22938005/2...
Sorry for I got bad news for ya. Compile failed with a clobber build on ios_dbg_simulator. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/22938005/2...
Retried try job too often on ios_dbg_simulator for step(s) ui_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/22938005/2...
Message was sent while issue was closed.
Change committed as 221527 |
