Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11794034:
Adds functionality to pack an extension / app from the app. (Closed)
|
Created:
7 years, 11 months ago by Gaurav Modified:
7 years, 10 months ago CC:
chromium-reviews, Aaron Boodman, arv (Not doing code reviews), chromium-apps-reviews_chromium.org, asargent_no_longer_on_chrome, miket_OOO Base URL:
http://git.chromium.org/chromium/src.git@bacha_lo Visibility:
Public. |
DescriptionAdds functionality to pack an extension / app from the developer_app.
BUG=149036
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183156
Patch Set 1 #
Total comments: 44
Patch Set 2 : Addressed comments #Patch Set 3 : . #
Total comments: 26
Patch Set 4 : Rebased and Addressed comments. #
Total comments: 12
Patch Set 5 : Addressed comments #
Total comments: 32
Patch Set 6 : comments #
Total comments: 13
Patch Set 7 : comments and rebase #Patch Set 8 : rebase #Patch Set 9 : . #Messages
Total messages: 19 (0 generated)
https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:600: bool DeveloperPrivateChooseEntryFunction::ShowPicker( nit: remove the extra space on this line between bool and DeveloperPrivateChooseEntryFunction https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/developer_private/developer_private_api.h (right): https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.h:263: : public DeveloperPrivateChooseEntryFunction { I'm a little wary of using inheritance of another concrete ExtensionFunction. Two better options would be: a) Have a common superclass that inherits from AsyncExtensionFunction with a concrete ShowPicker method but pure virtual FileSelected/FileSelectionCanceled methods. Then both DeveloperPrivateChooseEntryFunction and DeveloperPrivateChooseItemFunction would derive from this class. b) Have a ShowPicker method definied in the .cc file here (either static, or in an anonymous namespace) that gets necessary dependencies passed in as input parameters, and takes a base::Callback as well which it will use to report success or failure. https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... File chrome/common/extensions/api/developer_private.idl (right): https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/developer_private.idl:137: // |file_type| : Required file type. What are the legal values for this? Does this mean the file's extension, e.g. ".json"? https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/developer_private.idl:139: static void chooseItem(DOMString select_type, Nit: maybe "choosePath" would be a better name for this method? Also, we should use an enum instead of a DOMString for |select_type|. (see some of the other .idl files in this directory for examples) https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/developer_private.idl:145: static void packItem(DOMString item_path, nit: I assume you use the term 'item' here and elsewhere because this could be either an app or an extension, right? It would be nice if we could come up with something better than 'item' in such cases, because it's a pretty vague term that could mean anything. So maybe "packDirectory" would be a better name for this method? And just "path" or "directory_path" instead of "item_path"?
https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:588: ui::SelectFileDialog::SELECT_FOLDER; This local constant definition doesn't really make the code any clearer, unless I'm missing your intent. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:592: return The newline after this return shouldn't be necessary. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:644: crx_file, pem_file))); Similar comment as earlier in this file: why create the temp variable? Why not just assign the UTF16ToUTF8 result directly to response.message? Given the already well-named StandardSuccessMessage method, it's already sufficiently clear what you're doing. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:689: response.message = l10n_util::GetStringUTF8( These inner braces aren't needed. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:700: response.message = l10n_util::GetStringUTF8( Did you mean to set response.status in this case? https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:731: type = ui::SelectFileDialog::SELECT_OPEN_FILE; No braces https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:753: DeveloperPrivateAPI::Get(profile())->getLastUnpackedDirectory(); That's a weirdly capitalized method (probably not yours) https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:755: ShowPicker(type, Don't wrap after the return. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:763: std::string result(std::string(UTF16ToUTF8(path.LossyDisplayName()))); Similar to above (temp variable doesn't add anything), though the method's already so short it's not a big deal. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/developer_private/developer_private_api.h (right): https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.h:263: : public DeveloperPrivateChooseEntryFunction { I agree with Antony's suggestion. This kind of inheritance seems to have a short shelf life. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.h:288: extensions::ExtensionCreator::ErrorType) OVERRIDE; Even if you don't need it, please include a parameter name. https://codereview.chromium.org/11794034/diff/1/chrome/browser/resources/apps... File chrome/browser/resources/apps_debugger/js/pack_item_overlay.js (right): https://codereview.chromium.org/11794034/diff/1/chrome/browser/resources/apps... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:57: } else if (response.status == 'ERROR') { Braces optional here https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... File chrome/common/extensions/api/developer_private.idl (right): https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/developer_private.idl:78: this newline isn't needed (follow style of others in this file) https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/developer_private.idl:83: DOMString item_path; The prevailing style in the IDL is itemPath, pemPath, etc. They get fixed for C++ style during generation. https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/developer_private.idl:116: // Enable / Disable file access for a given |item_id| Hmmmm. These changes seem to go in the wrong style direction. Were they necessary for some reason? https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/developer_private.idl:118: boolean is_allow, isAllowed sounds more natural (isEnabled below, too) https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/developer_private.idl:137: // |file_type| : Required file type. On 2013/01/09 00:48:05, Antony Sargent wrote: > What are the legal values for this? Does this mean the file's extension, e.g. > ".json"? An example would be good if a full enumeration isn't practical.
This CL is on top of 11734031 which is yet to be checked-in. There are few updates reflected in this CL which are part of that. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:588: ui::SelectFileDialog::SELECT_FOLDER; On 2013/01/09 18:52:41, miket wrote: > This local constant definition doesn't really make the code any clearer, unless > I'm missing your intent. Done. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:592: return On 2013/01/09 18:52:41, miket wrote: > The newline after this return shouldn't be necessary. Done. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:600: bool DeveloperPrivateChooseEntryFunction::ShowPicker( On 2013/01/09 00:48:05, Antony Sargent wrote: > nit: remove the extra space on this line between bool and > DeveloperPrivateChooseEntryFunction Done. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:644: crx_file, pem_file))); On 2013/01/09 18:52:41, miket wrote: > Similar comment as earlier in this file: why create the temp variable? Why not > just assign the UTF16ToUTF8 result directly to response.message? Given the > already well-named StandardSuccessMessage method, it's already sufficiently > clear what you're doing. Done. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:689: response.message = l10n_util::GetStringUTF8( On 2013/01/09 18:52:41, miket wrote: > These inner braces aren't needed. Done. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:700: response.message = l10n_util::GetStringUTF8( yes. Fixed now. On 2013/01/09 18:52:41, miket wrote: > Did you mean to set response.status in this case? https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:731: type = ui::SelectFileDialog::SELECT_OPEN_FILE; On 2013/01/09 18:52:41, miket wrote: > No braces Done. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:753: DeveloperPrivateAPI::Get(profile())->getLastUnpackedDirectory(); On 2013/01/09 18:52:41, miket wrote: > That's a weirdly capitalized method (probably not yours) Done. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:755: ShowPicker(type, On 2013/01/09 18:52:41, miket wrote: > Don't wrap after the return. Done. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.cc:763: std::string result(std::string(UTF16ToUTF8(path.LossyDisplayName()))); On 2013/01/09 18:52:41, miket wrote: > Similar to above (temp variable doesn't add anything), though the method's > already so short it's not a big deal. Done. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... File chrome/browser/extensions/api/developer_private/developer_private_api.h (right): https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.h:263: : public DeveloperPrivateChooseEntryFunction { On 2013/01/09 18:52:41, miket wrote: > I agree with Antony's suggestion. This kind of inheritance seems to have a short > shelf life. Done. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.h:263: : public DeveloperPrivateChooseEntryFunction { On 2013/01/09 00:48:05, Antony Sargent wrote: > I'm a little wary of using inheritance of another concrete ExtensionFunction. > Two better options would be: > > a) Have a common superclass that inherits from AsyncExtensionFunction with a > concrete ShowPicker method but pure virtual FileSelected/FileSelectionCanceled > methods. Then both DeveloperPrivateChooseEntryFunction and > DeveloperPrivateChooseItemFunction would derive from this class. > > b) Have a ShowPicker method definied in the .cc file here (either static, or in > an anonymous namespace) that gets necessary dependencies passed in as input > parameters, and takes a base::Callback as well which it will use to report > success or failure. > Done. https://codereview.chromium.org/11794034/diff/1/chrome/browser/extensions/api... chrome/browser/extensions/api/developer_private/developer_private_api.h:288: extensions::ExtensionCreator::ErrorType) OVERRIDE; On 2013/01/09 18:52:41, miket wrote: > Even if you don't need it, please include a parameter name. Done. https://codereview.chromium.org/11794034/diff/1/chrome/browser/resources/apps... File chrome/browser/resources/apps_debugger/js/pack_item_overlay.js (right): https://codereview.chromium.org/11794034/diff/1/chrome/browser/resources/apps... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:57: } else if (response.status == 'ERROR') { On 2013/01/09 18:52:41, miket wrote: > Braces optional here Done. https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... File chrome/common/extensions/api/developer_private.idl (right): https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/developer_private.idl:78: On 2013/01/09 18:52:41, miket wrote: > this newline isn't needed (follow style of others in this file) Done. https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/developer_private.idl:83: DOMString item_path; I changed the naming from camel case to underscores, as enum generation had some issues with it. Should I replace all the variables in the IDL to caml case ? Thanks. On 2013/01/09 18:52:41, miket wrote: > The prevailing style in the IDL is itemPath, pemPath, etc. They get fixed for > C++ style during generation. https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/developer_private.idl:116: // Enable / Disable file access for a given |item_id| Did them to be consistent with enums. Should I revert them ? On 2013/01/09 18:52:41, miket wrote: > Hmmmm. These changes seem to go in the wrong style direction. Were they > necessary for some reason? https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/developer_private.idl:118: boolean is_allow, On 2013/01/09 18:52:41, miket wrote: > isAllowed sounds more natural (isEnabled below, too) Done. https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/developer_private.idl:137: // |file_type| : Required file type. On 2013/01/09 18:52:41, miket wrote: > On 2013/01/09 00:48:05, Antony Sargent wrote: > > What are the legal values for this? Does this mean the file's extension, e.g. > > ".json"? > > An example would be good if a full enumeration isn't practical. Done. https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/developer_private.idl:137: // |file_type| : Required file type. On 2013/01/09 00:48:05, Antony Sargent wrote: > What are the legal values for this? Does this mean the file's extension, e.g. > ".json"? Done. https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/developer_private.idl:139: static void chooseItem(DOMString select_type, On 2013/01/09 00:48:05, Antony Sargent wrote: > Nit: maybe "choosePath" would be a better name for this method? > > Also, we should use an enum instead of a DOMString for |select_type|. (see some > of the other .idl files in this directory for examples) Done. https://codereview.chromium.org/11794034/diff/1/chrome/common/extensions/api/... chrome/common/extensions/api/developer_private.idl:145: static void packItem(DOMString item_path, On 2013/01/09 00:48:05, Antony Sargent wrote: > nit: I assume you use the term 'item' here and elsewhere because this could be > either an app or an extension, right? It would be nice if we could come up with > something better than 'item' in such cases, because it's a pretty vague term > that could mean anything. > > So maybe "packDirectory" would be a better name for this method? And just "path" > or "directory_path" instead of "item_path"? > Done.
@dbeam files under chrome/browser/resources @asargent: I have updated the diff addressing the comments. Can you have a look at extensions/api/developer_private and developer_private.idl. Thanks
https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/css/pack_item_overlay.css (right): https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/css/pack_item_overlay.css:11: text-align: right; nit: text-align: end; works in RTL (if you want it to flip) https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/js/pack_item_overlay.js (right): https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:11: function PackItemOverlay() { nit: don't think you need this \n https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:37: * @param {Event} e The click event. @private https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:45: * @param {Event} e The click event. @private https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:53: nit: jsdoc https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:89: * @param {String} message The message to show to the user. nit: @param {string} https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:92: alertOverlay.setValues( nit: this is a little scary looking, can you do one of two things here: 1) annotate each arg that's unclear, i.e. '' /* someParamName */ 2) change this into an object, i.e. alertOverlay.setValues({ someMessage: str('someMessage'), }); https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:106: * the alert overlay and return to showing the PackItemOverlay. nit: missing @param https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:123: * user confirms the action. nit: missing @param https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:127: AppsDebugger.showOverlay(null); nit: can you make a central function and re-use it, i.e. var hideOverlay = function() { AppsDebugger.showOverlay(null); }; (named for what it does, I'm just guessing...) https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:149: PackItemOverlay: PackItemOverlay opt nit: I prefer , at the end https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/main.html (right): https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/main.html:32: <h1> Apps / Extensions Debugger. </h1> nit: no \s around text, also why is this not translated? https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/pack_item_overlay.html (right): https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/pack_item_overlay.html:15: id="itemPrivateKey" type="text"> nit: it looks like you could unwrap this a little bit
https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/css/pack_item_overlay.css (right): https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/css/pack_item_overlay.css:11: text-align: right; On 2013/02/07 01:54:44, Dan Beam wrote: > nit: text-align: end; works in RTL (if you want it to flip) Done. https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/js/pack_item_overlay.js (right): https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:11: function PackItemOverlay() { On 2013/02/07 01:54:44, Dan Beam wrote: > nit: don't think you need this \n Done. https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:37: * @param {Event} e The click event. On 2013/02/07 01:54:44, Dan Beam wrote: > @private Done. https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:45: * @param {Event} e The click event. On 2013/02/07 01:54:44, Dan Beam wrote: > @private Done. https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:53: On 2013/02/07 01:54:44, Dan Beam wrote: > nit: jsdoc Done. https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:53: On 2013/02/07 01:54:44, Dan Beam wrote: > nit: jsdoc Done. https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:89: * @param {String} message The message to show to the user. On 2013/02/07 01:54:44, Dan Beam wrote: > nit: @param {string} Done. https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:92: alertOverlay.setValues( On 2013/02/07 01:54:44, Dan Beam wrote: > nit: this is a little scary looking, can you do one of two things here: > > 1) annotate each arg that's unclear, i.e. > > '' /* someParamName */ > > 2) change this into an object, i.e. > > alertOverlay.setValues({ > someMessage: str('someMessage'), > }); Done. https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:106: * the alert overlay and return to showing the PackItemOverlay. On 2013/02/07 01:54:44, Dan Beam wrote: > nit: missing @param Done. https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:127: AppsDebugger.showOverlay(null); On 2013/02/07 01:54:44, Dan Beam wrote: > nit: can you make a central function and re-use it, i.e. > > var hideOverlay = function() { > AppsDebugger.showOverlay(null); > }; > > (named for what it does, I'm just guessing...) Done. https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:149: PackItemOverlay: PackItemOverlay On 2013/02/07 01:54:44, Dan Beam wrote: > opt nit: I prefer , at the end Done. https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/main.html (right): https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/main.html:32: <h1> Apps / Extensions Debugger. </h1> On 2013/02/07 01:54:44, Dan Beam wrote: > nit: no \s around text, also why is this not translated? Done. https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/pack_item_overlay.html (right): https://codereview.chromium.org/11794034/diff/15001/chrome/browser/resources/... chrome/browser/resources/apps_debugger/pack_item_overlay.html:15: id="itemPrivateKey" type="text"> On 2013/02/07 01:54:44, Dan Beam wrote: > nit: it looks like you could unwrap this a little bit Done.
chrome/{common,browser}/extensions LGTM
https://codereview.chromium.org/11794034/diff/26001/chrome/browser/extensions...
File chrome/browser/extensions/api/developer_private/developer_private_api.cc
(right):
https://codereview.chromium.org/11794034/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:520:
ShowPicker(
style nit: collapse the first 2 lines here:
int result = ShowPicker(
ui::SelectFileDialog::SELECT_FOLDER,
...
https://codereview.chromium.org/11794034/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:530:
style nit: no empty space at beginning of functions
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Vertical_White...
"... resist starting functions with a blank line..."
https://codereview.chromium.org/11794034/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:637: //
Pack job will release the function instance.
nit: this comment is slightly misleading - better to say something like
"balanced in OnPackSuccess/OnPackFailure"
https://codereview.chromium.org/11794034/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/developer_private/developer_private_api.cc:687:
ShowPicker(
nit: collapse first 2 lines, e.g.
int result = ShowPicker(
type,
...
https://codereview.chromium.org/11794034/diff/26001/chrome/browser/extensions...
File chrome/browser/extensions/api/developer_private/developer_private_api.h
(right):
https://codereview.chromium.org/11794034/diff/26001/chrome/browser/extensions...
chrome/browser/extensions/api/developer_private/developer_private_api.h:270:
std::string key_path_str_;
naming suggestion: consider omitting the "_str" part from these names since it's
clear from their types that they are strings.
https://codereview.chromium.org/11794034/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/11794034/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:520: ShowPicker( On 2013/02/09 00:38:45, Antony Sargent wrote: > style nit: collapse the first 2 lines here: > > int result = ShowPicker( > ui::SelectFileDialog::SELECT_FOLDER, > ... > > Done. https://codereview.chromium.org/11794034/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:530: On 2013/02/09 00:38:45, Antony Sargent wrote: > style nit: no empty space at beginning of functions > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Vertical_White... > > > "... resist starting functions with a blank line..." Done. https://codereview.chromium.org/11794034/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:637: // Pack job will release the function instance. On 2013/02/09 00:38:45, Antony Sargent wrote: > nit: this comment is slightly misleading - better to say something like > "balanced in OnPackSuccess/OnPackFailure" Done. https://codereview.chromium.org/11794034/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:637: // Pack job will release the function instance. On 2013/02/09 00:38:45, Antony Sargent wrote: > nit: this comment is slightly misleading - better to say something like > "balanced in OnPackSuccess/OnPackFailure" Done. https://codereview.chromium.org/11794034/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:687: ShowPicker( On 2013/02/09 00:38:45, Antony Sargent wrote: > nit: collapse first 2 lines, e.g. > > int result = ShowPicker( > type, > ... Done. https://codereview.chromium.org/11794034/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/api/developer_private/developer_private_api.h (right): https://codereview.chromium.org/11794034/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.h:270: std::string key_path_str_; Added str as when they are used they might be confused to be "FilePath" instead of strings. On 2013/02/09 00:38:45, Antony Sargent wrote: > naming suggestion: consider omitting the "_str" part from these names since it's > clear from their types that they are strings.
https://codereview.chromium.org/11794034/diff/26001/chrome/browser/extensions... File chrome/browser/extensions/api/developer_private/developer_private_api.h (right): https://codereview.chromium.org/11794034/diff/26001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.h:270: std::string key_path_str_; On 2013/02/11 20:01:55, Gaurav wrote: > Added str as when they are used they might be confused to be "FilePath" instead > of strings. > On 2013/02/09 00:38:45, Antony Sargent wrote: > > naming suggestion: consider omitting the "_str" part from these names since > it's > > clear from their types that they are strings. > Yep, I assumed that's why you had that suffix. If I recall correctly though you used "_path" on names of FilePath variables in places where you had both a std::string and a FilePath, and that seemed sufficient to me to disambiguate them. Anyhow, no big deal either way.
https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... File chrome/browser/resources/apps_debugger/css/items.css (right): https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... chrome/browser/resources/apps_debugger/css/items.css:135: #overlay .page:not(.showing) { aren't the overlays already being marked with [hidden], which makes them display: none;? https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... File chrome/browser/resources/apps_debugger/js/items.js (right): https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... chrome/browser/resources/apps_debugger/js/items.js:126: packItemOverlay.initializePage(); nit: indent off https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... chrome/browser/resources/apps_debugger/js/items.js:127: nit: remove \n https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... chrome/browser/resources/apps_debugger/js/items.js:163: return document.querySelector('#overlay .page.showing'); why did you change this? https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... File chrome/browser/resources/apps_debugger/js/items_list.js (right): https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... chrome/browser/resources/apps_debugger/js/items_list.js:3: // found in the LICENSE file.. remove second . https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... File chrome/browser/resources/apps_debugger/js/pack_item_overlay.js (right): https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:5: cr.define('appsDevtool', function() { nit: apps_dev_tool or apps_devtool https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:17: * Initialize the page. can you add to this comment or simply remove it? https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:76: $('itemRootDir').value = filePath; this should be item-root-dir (instead of itemRootDir) https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:87: $('itemPrivateKey').value = filePath; and item-private-key https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:93: AppsDevTool.showOverlay(null); same as handleDismiss_, combine https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:109: AppsDevTool.showOverlay($('alertOverlay')); alert-overlay https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:135: var closeAlert = function() { why do you need this? this is the same as just using `hideOverlay` directly. https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:152: closeAlert()); this should not have calling parens(), have you tried this code? https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... File chrome/browser/resources/apps_debugger/main.html (right): https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... chrome/browser/resources/apps_debugger/main.html:15: <link rel="stylesheet" href="../../../../ui/webui/resources/css/alert_overlay.css"> nit: 80 col wrap https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... File chrome/browser/resources/apps_debugger/pack_item_overlay.html (right): https://chromiumcodereview.appspot.com/11794034/diff/26003/chrome/browser/res... chrome/browser/resources/apps_debugger/pack_item_overlay.html:8: <input class="pack-item-text-area" id="itemRootDir" type="text"> Element IDs use dash-form, http://dev.chromium.org/developers/web-development-style-guide#TOC-Body
https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/css/items.css (right): https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/css/items.css:135: #overlay .page:not(.showing) { There are two types of overlay which are both hidden initially. Their visibility is triggered by setting "showing" in items.js On 2013/02/11 23:44:24, Dan Beam wrote: > aren't the overlays already being marked with [hidden], which makes them > display: none;? https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/js/items.js (right): https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/items.js:126: packItemOverlay.initializePage(); On 2013/02/11 23:44:24, Dan Beam wrote: > nit: indent off Done. https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/items.js:127: On 2013/02/11 23:44:24, Dan Beam wrote: > nit: remove \n Done. https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/items.js:163: return document.querySelector('#overlay .page.showing'); this.ownerDocument returns null. On 2013/02/11 23:44:24, Dan Beam wrote: > why did you change this? https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/js/items_list.js (right): https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/items_list.js:3: // found in the LICENSE file.. On 2013/02/11 23:44:24, Dan Beam wrote: > remove second . Done. https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/js/pack_item_overlay.js (right): https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:5: cr.define('appsDevtool', function() { On 2013/02/11 23:44:24, Dan Beam wrote: > nit: apps_dev_tool or apps_devtool Done. https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:17: * Initialize the page. On 2013/02/11 23:44:24, Dan Beam wrote: > can you add to this comment or simply remove it? Done. https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:76: $('itemRootDir').value = filePath; On 2013/02/11 23:44:24, Dan Beam wrote: > this should be item-root-dir (instead of itemRootDir) Done. https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:87: $('itemPrivateKey').value = filePath; On 2013/02/11 23:44:24, Dan Beam wrote: > and item-private-key Done. https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:93: AppsDevTool.showOverlay(null); On 2013/02/11 23:44:24, Dan Beam wrote: > same as handleDismiss_, combine Done. https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:109: AppsDevTool.showOverlay($('alertOverlay')); The id "alertOverlay" is declared in a shared file resources/html/alert_overlay.html. On 2013/02/11 23:44:24, Dan Beam wrote: > alert-overlay https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:135: var closeAlert = function() { removed On 2013/02/11 23:44:24, Dan Beam wrote: > why do you need this? this is the same as just using `hideOverlay` directly. https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:152: closeAlert()); On 2013/02/11 23:44:24, Dan Beam wrote: > this should not have calling parens(), have you tried this code? Done. https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/main.html (right): https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/main.html:15: <link rel="stylesheet" href="../../../../ui/webui/resources/css/alert_overlay.css"> Wrapping it in two lines doesn't load the css. On 2013/02/11 23:44:24, Dan Beam wrote: > nit: 80 col wrap https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/pack_item_overlay.html (right): https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/pack_item_overlay.html:8: <input class="pack-item-text-area" id="itemRootDir" type="text"> On 2013/02/11 23:44:24, Dan Beam wrote: > Element IDs use dash-form, > http://dev.chromium.org/developers/web-development-style-guide#TOC-Body Done.
https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/js/items.js (right): https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/items.js:163: return document.querySelector('#overlay .page.showing'); On 2013/02/12 19:33:17, Gaurav wrote: > this.ownerDocument returns null. > > On 2013/02/11 23:44:24, Dan Beam wrote: > > why did you change this? > ah, 'cuz it's static, OK https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/main.html (right): https://codereview.chromium.org/11794034/diff/26003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/main.html:15: <link rel="stylesheet" href="../../../../ui/webui/resources/css/alert_overlay.css"> On 2013/02/12 19:33:17, Gaurav wrote: > Wrapping it in two lines doesn't load the css. > On 2013/02/11 23:44:24, Dan Beam wrote: > > nit: 80 col wrap > Hmm, that's strange, works for me - http://jsfiddle.net/wAXLx/ https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/js/items.js (right): https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/items.js:126: packItemOverlay.initializePage(); nit: apps_dev_tool.PackItemOverlay.getInstance().initializePage(); https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/js/pack_item_overlay.js (right): https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:123: * user confirms the action. @param {response} https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/main.html (right): https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/main.html:15: <link rel="stylesheet" href="../../../../ui/webui/resources/css/alert_overlay.css"> nit: 80 col wrap https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/main.html:40: <h1> Apps / Extensions Debugger</h1> nit: remove \n, where is this translated? https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/main.html:42: <div class="page" id="extension-settings"> nit: remove \n https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/pack_item_overlay.html (right): https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/pack_item_overlay.html:9: <button id="browseItemDir" nit: IDs should be browse-item-dir
please fix the nits, then lgtm
https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/js/items.js (right): https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/items.js:126: packItemOverlay.initializePage(); On 2013/02/14 21:10:40, Dan Beam wrote: > nit: > apps_dev_tool.PackItemOverlay.getInstance().initializePage(); Done. https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/js/pack_item_overlay.js (right): https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/js/pack_item_overlay.js:123: * user confirms the action. On 2013/02/14 21:10:40, Dan Beam wrote: > @param {response} Done. https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/main.html (right): https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/main.html:15: <link rel="stylesheet" href="../../../../ui/webui/resources/css/alert_overlay.css"> On 2013/02/14 21:10:40, Dan Beam wrote: > nit: 80 col wrap Not working with col wrap. https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/main.html:40: <h1> Apps / Extensions Debugger</h1> On 2013/02/14 21:10:40, Dan Beam wrote: > nit: remove \n, where is this translated? Done. https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/main.html:42: <div class="page" id="extension-settings"> On 2013/02/14 21:10:40, Dan Beam wrote: > nit: remove \n Done. https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/pack_item_overlay.html (right): https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/pack_item_overlay.html:9: <button id="browseItemDir" On 2013/02/14 21:10:40, Dan Beam wrote: > nit: IDs should be browse-item-dir Done.
slgtm https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/main.html (right): https://codereview.chromium.org/11794034/diff/31003/chrome/browser/resources/... chrome/browser/resources/apps_debugger/main.html:15: <link rel="stylesheet" href="../../../../ui/webui/resources/css/alert_overlay.css"> On 2013/02/15 17:54:24, Gaurav wrote: > On 2013/02/14 21:10:40, Dan Beam wrote: > > nit: 80 col wrap > Not working with col wrap. We found out this was the issue - https://code.google.com/p/chromium/issues/detail?id=176593
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grv@chromium.org/11794034/45001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grv@chromium.org/11794034/52002
Message was sent while issue was closed.
Change committed as 183156 |
