|
|
Created:
7 years, 11 months ago by Gaurav Modified:
7 years, 11 months ago CC:
chromium-reviews, Aaron Boodman, arv (Not doing code reviews), tfarina, chromium-apps-reviews_chromium.org, Evan Stade Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionThis CL adds more api's to developer_private to enable / disable extensions,
load unpacked, uninstall, allow file access etc.
It also adds the related UI code to display the apps.
There are few API's which are still to be implemented like
Pack-extension, allow-incognito etc.
I will add them in the following CL. Also, browser tests
to follow in the follow up CL.
More APIs for AppsDebuggerPrivate.
The AppsDebugger UI code is in 12021003
BUG=149036
TBR=sky@chromium.org
TBR=estade@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177648
Patch Set 1 #Patch Set 2 : removed unused files #Patch Set 3 : Fixed CSP by overriding in manifest #Patch Set 4 : Renamed "extension" to "item" #
Total comments: 23
Patch Set 5 : Addressed comments. Simplified DeveloperPrivateAPI class #Patch Set 6 : . #
Total comments: 22
Patch Set 7 : Addressed comments. #
Total comments: 27
Patch Set 8 : Spliting the UI code in separate CL #Patch Set 9 : Addressed comments #Patch Set 10 : fixing compile on win #Messages
Total messages: 15 (0 generated)
https://codereview.chromium.org/11734031/diff/6001/chrome/browser/extensions/... File chrome/browser/extensions/api/developer_private/developer_private_api.h (right): https://codereview.chromium.org/11734031/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.h:50: class DeveloperPrivateAPI : public ProfileKeyedService, I think it would simplify things to just have the parts related to listening for notifications (eg. the NotificationObserver interface) in this class, and not add a bunch of other unrelated stuff to it to implement various API methods. For example, why not just directly do the work that DeveloperPrivateAPI::AllowFileAccess does inside the implementation of DeveloperPrivateAllowFileAccessFunction::RunImpl? Similarly for EnableItem, LoadUnpacked, etc. https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... File chrome/browser/resources/apps_debugger/js/main_scripts.js (right): https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/js/main_scripts.js:14: //<include src="main.js"/> Looks like these are all commented out - did you mean to still include this file? https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... File chrome/browser/resources/apps_debugger/main.html (right): https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/main.html:19: <!--<script src="../shared/js/cr.js"></script> This srcipt include is commented out - did you mean to remove the line entirely? https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/main.html:56: <span> </span> Is this empty span here just for spacing purposes? https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/main.html:65: <span></span> same question here https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... File chrome/browser/resources/apps_debugger/manifest.json (right): https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/manifest.json:12: "default-src 'self' chrome-extension-resource:; script-src 'self' chrome-extension-resource:; object-src 'self'; connect-src *; style-src 'unsafe-inline' *; img-src *; media-src *;" Because the developerPrivate API has a lot of power, I think we need to be really cautious about opening up the default CSP restrictions. Getting this wrong means a seemingly innocuous extension or app could gain privilege escalation via the developer tool. Opening up script to chrome-extension-resource: and allowing inline scripts are particularly troublesome. I had thought we just needed to open up the ability to source images from chrome-extension-resource. https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... File chrome/common/extensions/api/developer_private.idl (right): https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... chrome/common/extensions/api/developer_private.idl:43: boolean enabled_incognito; does "enabled_incognito" mean "this extension is currently enabled to run in incognito"? If so maybe "incogntio_enabled" or "enabled_in_incognito" would be better names. https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... chrome/common/extensions/api/developer_private.idl:47: boolean incognito_can_be_enabled; nit: maybe this would be better named as "allow_incognito", to match with allow_activity and allow_file_access above? https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... chrome/common/extensions/api/developer_private.idl:95: boolean isAllow, nit: rename "isAllow" to either "isAllowed" or (probably better) just "allow" https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... chrome/common/extensions/api/developer_private.idl:106: boolean isEnable, same naming nit here for isEnable https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... chrome/common/extensions/api/developer_private.idl:113: static void getStrings(GetStringsCallback callback); We need to improve the documentation a little here, because there isn't quite enough information to figure out what this does without looking at the implementation. Which translated strings? What are the named properties to expect in the object passed to the GetStringsCallback? How does this relate (if at all) to what the chrome.i18n API does? (http://developer.chrome.com/extensions/i18n.html)
Thanks Antony. Inline response to comments. With update with fixes. On 2013/01/08 22:07:17, Antony Sargent wrote: > https://codereview.chromium.org/11734031/diff/6001/chrome/browser/extensions/... > File chrome/browser/extensions/api/developer_private/developer_private_api.h > (right): > > https://codereview.chromium.org/11734031/diff/6001/chrome/browser/extensions/... > chrome/browser/extensions/api/developer_private/developer_private_api.h:50: > class DeveloperPrivateAPI : public ProfileKeyedService, > I think it would simplify things to just have the parts related to listening for > notifications (eg. the NotificationObserver interface) in this class, and not > add a bunch of other unrelated stuff to it to implement various API methods. > > For example, why not just directly do the work that > DeveloperPrivateAPI::AllowFileAccess does inside the implementation of > DeveloperPrivateAllowFileAccessFunction::RunImpl? > > Similarly for EnableItem, LoadUnpacked, etc. > > https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... > File chrome/browser/resources/apps_debugger/js/main_scripts.js (right): > > https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... > chrome/browser/resources/apps_debugger/js/main_scripts.js:14: //<include > src="main.js"/> > Looks like these are all commented out - did you mean to still include this > file? > > https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... > File chrome/browser/resources/apps_debugger/main.html (right): > > https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... > chrome/browser/resources/apps_debugger/main.html:19: <!--<script > src="../shared/js/cr.js"></script> > This srcipt include is commented out - did you mean to remove the line entirely? > > https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... > chrome/browser/resources/apps_debugger/main.html:56: <span> </span> > Is this empty span here just for spacing purposes? > > https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... > chrome/browser/resources/apps_debugger/main.html:65: <span></span> > same question here > > https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... > File chrome/browser/resources/apps_debugger/manifest.json (right): > > https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... > chrome/browser/resources/apps_debugger/manifest.json:12: "default-src 'self' > chrome-extension-resource:; script-src 'self' chrome-extension-resource:; > object-src 'self'; connect-src *; style-src 'unsafe-inline' *; img-src *; > media-src *;" > Because the developerPrivate API has a lot of power, I think we need to be > really cautious about opening up the default CSP restrictions. Getting this > wrong means a seemingly innocuous extension or app could gain privilege > escalation via the developer tool. > > Opening up script to chrome-extension-resource: and allowing inline scripts are > particularly troublesome. I had thought we just needed to open up the ability to > source images from chrome-extension-resource. > > https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... > File chrome/common/extensions/api/developer_private.idl (right): > > https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... > chrome/common/extensions/api/developer_private.idl:43: boolean > enabled_incognito; > does "enabled_incognito" mean "this extension is currently enabled to run in > incognito"? If so maybe "incogntio_enabled" or "enabled_in_incognito" would be > better names. > > https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... > chrome/common/extensions/api/developer_private.idl:47: boolean > incognito_can_be_enabled; > nit: maybe this would be better named as "allow_incognito", to match with > allow_activity and allow_file_access above? > > https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... > chrome/common/extensions/api/developer_private.idl:95: boolean isAllow, > nit: rename "isAllow" to either "isAllowed" or (probably better) just "allow" > > https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... > chrome/common/extensions/api/developer_private.idl:106: boolean isEnable, > same naming nit here for isEnable > > https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... > chrome/common/extensions/api/developer_private.idl:113: static void > getStrings(GetStringsCallback callback); > We need to improve the documentation a little here, because there isn't quite > enough information to figure out what this does without looking at the > implementation. Which translated strings? What are the named properties to > expect in the object passed to the GetStringsCallback? > > How does this relate (if at all) to what the chrome.i18n API does? > (http://developer.chrome.com/extensions/i18n.html)
https://codereview.chromium.org/11734031/diff/6001/chrome/browser/extensions/... File chrome/browser/extensions/api/developer_private/developer_private_api.h (right): https://codereview.chromium.org/11734031/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.h:50: class DeveloperPrivateAPI : public ProfileKeyedService, On 2013/01/08 22:07:17, Antony Sargent wrote: > I think it would simplify things to just have the parts related to listening for > notifications (eg. the NotificationObserver interface) in this class, and not > add a bunch of other unrelated stuff to it to implement various API methods. > > For example, why not just directly do the work that > DeveloperPrivateAPI::AllowFileAccess does inside the implementation of > DeveloperPrivateAllowFileAccessFunction::RunImpl? > > Similarly for EnableItem, LoadUnpacked, etc. > Done. https://codereview.chromium.org/11734031/diff/6001/chrome/browser/extensions/... chrome/browser/extensions/api/developer_private/developer_private_api.h:50: class DeveloperPrivateAPI : public ProfileKeyedService, Still left the LoadUnpacked as it saves the last_unpacked_directory, and DeveloperPrivateAPI is the only persistent class here. On 2013/01/08 22:07:17, Antony Sargent wrote: > I think it would simplify things to just have the parts related to listening for > notifications (eg. the NotificationObserver interface) in this class, and not > add a bunch of other unrelated stuff to it to implement various API methods. > > For example, why not just directly do the work that > DeveloperPrivateAPI::AllowFileAccess does inside the implementation of > DeveloperPrivateAllowFileAccessFunction::RunImpl? > > Similarly for EnableItem, LoadUnpacked, etc. > https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... File chrome/browser/resources/apps_debugger/js/main_scripts.js (right): https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/js/main_scripts.js:14: //<include src="main.js"/> Directly including the files doesn't work. I am not totally sure how it works. We include these files here as commented which later gets flattened into the html source. Wallpaper_manager does similar thing. http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/resources/c... On 2013/01/08 22:07:17, Antony Sargent wrote: > Looks like these are all commented out - did you mean to still include this > file? https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... File chrome/browser/resources/apps_debugger/main.html (right): https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/main.html:19: <!--<script src="../shared/js/cr.js"></script> The files in main_scripts needs to be added in comments here. I will add a documentation here for it. On 2013/01/08 22:07:17, Antony Sargent wrote: > This srcipt include is commented out - did you mean to remove the line entirely? https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/main.html:56: <span> </span> This is for spacing purposes. On 2013/01/08 22:07:17, Antony Sargent wrote: > Is this empty span here just for spacing purposes? https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/main.html:65: <span></span> Spacing purpose. Is it ok to use an empty span for space or there is a better way ? Thanks On 2013/01/08 22:07:17, Antony Sargent wrote: > same question here https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... File chrome/browser/resources/apps_debugger/manifest.json (right): https://codereview.chromium.org/11734031/diff/6001/chrome/browser/resources/a... chrome/browser/resources/apps_debugger/manifest.json:12: "default-src 'self' chrome-extension-resource:; script-src 'self' chrome-extension-resource:; object-src 'self'; connect-src *; style-src 'unsafe-inline' *; img-src *; media-src *;" I think overriding script-src is not needed. I will double check and remove. Thanks On 2013/01/08 22:07:17, Antony Sargent wrote: > Because the developerPrivate API has a lot of power, I think we need to be > really cautious about opening up the default CSP restrictions. Getting this > wrong means a seemingly innocuous extension or app could gain privilege > escalation via the developer tool. > > Opening up script to chrome-extension-resource: and allowing inline scripts are > particularly troublesome. I had thought we just needed to open up the ability to > source images from chrome-extension-resource. https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... File chrome/common/extensions/api/developer_private.idl (right): https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... chrome/common/extensions/api/developer_private.idl:43: boolean enabled_incognito; On 2013/01/08 22:07:17, Antony Sargent wrote: > does "enabled_incognito" mean "this extension is currently enabled to run in > incognito"? If so maybe "incogntio_enabled" or "enabled_in_incognito" would be > better names. Done. https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... chrome/common/extensions/api/developer_private.idl:47: boolean incognito_can_be_enabled; On 2013/01/08 22:07:17, Antony Sargent wrote: > nit: maybe this would be better named as "allow_incognito", to match with > allow_activity and allow_file_access above? Done. https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... chrome/common/extensions/api/developer_private.idl:95: boolean isAllow, On 2013/01/08 22:07:17, Antony Sargent wrote: > nit: rename "isAllow" to either "isAllowed" or (probably better) just "allow" Done. https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... chrome/common/extensions/api/developer_private.idl:106: boolean isEnable, On 2013/01/08 22:07:17, Antony Sargent wrote: > same naming nit here for isEnable Done. https://codereview.chromium.org/11734031/diff/6001/chrome/common/extensions/a... chrome/common/extensions/api/developer_private.idl:113: static void getStrings(GetStringsCallback callback); On 2013/01/08 22:07:17, Antony Sargent wrote: > We need to improve the documentation a little here, because there isn't quite > enough information to figure out what this does without looking at the > implementation. Which translated strings? What are the named properties to > expect in the object passed to the GetStringsCallback? > > How does this relate (if at all) to what the chrome.i18n API does? > (http://developer.chrome.com/extensions/i18n.html) > Done.
Sorry to pile on so many additional change requests! At the end of the day we'll have a lean, mean, machine though. I haven't made it all the way through all the javascript files - if possible, it might make sense to split those off into a separate CL since this one it probably already a little too big. https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:613: new EntryPicker(this, shell_window->web_contents(), picker_type, It's a little unusual to have objects begin doing "actual" work from within a constructor, rather than having something like a Show() or Run() method. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Doing_Work_in_... Looking at the implementation of the EntryPicker, I think it doesn't technically violate the style guide. Were you following a pattern you saw in other UI dialog code? https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:639: dict->SetString(id, l10n_util::GetStringUTF16(idr)) We often resist the temptation to use macros just to shorten code, if you could achieve a similar result with something like an inlined or static method. However, one idea for an improvement here would require a macro - it could make it easier to see where a given string is defined if we used the stringified version of the resource id as the key in this dictionary. For example: #define SET_STRING(resourceid)\ dict->SetString(#resourceid, l10n_util::GetStringUTF16(resourceid)); ... SET_STRING(IDS_EXTENSIONS_INSPECT_VIEWS) ... Then in the html file you'd have: <span i18n-content="IDS_EXTENSIONS_INSPECT_VIEWS"></span> I glanced briefly at some of the other html files in the chrome/browser/resources directory and most don't seem to use this pattern, although the chrome/browser/resources/file_manager/ comes closest to it with a slightly different macro used in FileDialogStringsFunction::RunImpl, where they elide the namespace name from the key. Maybe you should ask the opinion of some of the people in the chrome/browser/resources/OWNERS file why we don't already use the pattern I describe above for translated strings. (You'll need someone on there to be a reviewer for this CL to ok the new directory anyway). https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... File chrome/browser/extensions/api/developer_private/developer_private_api.h (right): https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.h:51: public ExtensionUninstallDialog::Delegate, Seeing this reminds me of my comment on the previous revision of the CL, that we want to push as much stuff as possible out of this 'global' DeveloperPrivateAPI object into specific *ExtensionFunction implementation objects, with helper objects/static methods as needed to reduce code duplication. I think the way to think about it should be this: -DeveloperPrivateAPI is a profile keyed service, so it lives longer than any single API method call. It should only contain state and have methods needed to operate outside of the context of any API method call. In concrete terms, I think that means keep the Observe method and perhaps the last_unpacked_directory_ member variable. -Any state/helper methods that are only used during one particular method API call and not needed afterwards should be pushed into the *ExtensionFunction implementation object (with helpers as noted above) https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.h:60: void AddItemsInfo(const ExtensionSet& items, here too - this should be moved out of this global object https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.h:100: void GetShellWindowPagesForExtensionProfile( nit: would GetShellWindows be a better name? https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.h:121: std::string extension_id_prompting_; Assuming we do need this (see my question in developer_private.idl), I'm worried that it could get out of sync if we get multiple calls in succession without the user making a selection in the dialog. I see you have some code to prevent this right now, but the pattern seems like it could be prone to breaking with subsequent changes by people who aren't aware of the pitfall. Can we instead push this onto the *ExtensionFunction instance corresponding to an incoming API call? https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.h:137: content::RenderViewHost* deleting_render_view_host_; I may have missed it, but do you ever set this to NULL once you're done with the iteration over the RenderViewHosts? If not then it seems like you could be holding on to a stale pointer. (In fact, are you sure it's even safe to hold on to it beyond the duration of the callback to Observe for NOTIFICATION_RENDER_VIEW_HOST_DELETED / NOTIFICATION_BACKGROUND_CONTENTS_DELETED notifications? Don't those fire just before the RenderViewHost object gets deleted?) https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.h:237: friend EntryPicker; I'd like to avoid having EntryPicker be a friend class here. Two ways to do that would be: -Define an interface in EntryPicker that DeveloperPrivateChooseEntryFunction can implement or -Have EntryPicker take a base::Callback that it will call back with a non-empty FilePath if the user selected a file, and an empty one if the dialog was cancelled. https://codereview.chromium.org/11734031/diff/16018/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/manifest.json (right): https://codereview.chromium.org/11734031/diff/16018/chrome/browser/resources/... chrome/browser/resources/apps_debugger/manifest.json:12: "default-src 'self' chrome-extension-resource:; object-src 'self'; connect-src *; style-src 'unsafe-inline' *; img-src *; media-src *;" Can you see if we can tighten this further to: "default-src 'self'; img-src 'self' chrome-extension-resource:" ? https://codereview.chromium.org/11734031/diff/16018/chrome/common/extensions/... File chrome/common/extensions/api/developer_private.idl (right): https://codereview.chromium.org/11734031/diff/16018/chrome/common/extensions/... chrome/common/extensions/api/developer_private.idl:102: static void uninstall(DOMString itemId, BooleanCallback callback); Is there some reason you can't just use chrome.management.uninstall? https://codereview.chromium.org/11734031/diff/16018/chrome/common/extensions/... chrome/common/extensions/api/developer_private.idl:113: // strings used in apps_debugger app. Can you add to the end of the sentence here something like "as a dictionary mapping a string identifier to the translated string to use in the UI."
Small comment on CL metadata. The title should be economical, and it should summarize the description. I think the Git standard is 70 characters, though that's just advisory in our case. So "This CL" is a waste; eliminate. And don't end the title with a comma, or use it to continue a thought into the description.
Also, if you want me to review this, please add miket (the other "Mike Tsao" isn't my Chromium account).
Thanks for pointing that out, It took the first line of description as the title by default. I will be careful next time. On 2013/01/16 21:42:36, miket wrote: > Also, if you want me to review this, please add miket (the other "Mike Tsao" > isn't my Chromium account).
https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:613: new EntryPicker(this, shell_window->web_contents(), picker_type, Yes, I saw similar pattern in the file_system API code for the EntryPicker class. On 2013/01/15 23:19:50, Antony Sargent wrote: > It's a little unusual to have objects begin doing "actual" work from within a > constructor, rather than having something like a Show() or Run() method. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Doing_Work_in_... > > Looking at the implementation of the EntryPicker, I think it doesn't technically > violate the style guide. Were you following a pattern you saw in other UI dialog > code? https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:639: dict->SetString(id, l10n_util::GetStringUTF16(idr)) This looks to be a great idea, and make the readability much easier. Adding more people for opinion. Thanks On 2013/01/15 23:19:50, Antony Sargent wrote: > We often resist the temptation to use macros just to shorten code, if you could > achieve a similar result with something like an inlined or static method. > > However, one idea for an improvement here would require a macro - it could make > it easier to see where a given string is defined if we used the stringified > version of the resource id as the key in this dictionary. For example: > > #define SET_STRING(resourceid)\ > dict->SetString(#resourceid, l10n_util::GetStringUTF16(resourceid)); > > > ... > SET_STRING(IDS_EXTENSIONS_INSPECT_VIEWS) > ... > > > > Then in the html file you'd have: > > <span i18n-content="IDS_EXTENSIONS_INSPECT_VIEWS"></span> > > > I glanced briefly at some of the other html files in the > chrome/browser/resources directory and most don't seem to use this pattern, > although the chrome/browser/resources/file_manager/ comes closest to it with a > slightly different macro used in FileDialogStringsFunction::RunImpl, where they > elide the namespace name from the key. > > Maybe you should ask the opinion of some of the people in the > chrome/browser/resources/OWNERS file why we don't already use the pattern I > describe above for translated strings. (You'll need someone on there to be a > reviewer for this CL to ok the new directory anyway). https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... File chrome/browser/extensions/api/developer_private/developer_private_api.h (right): https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.h:51: public ExtensionUninstallDialog::Delegate, Removed ExtensionUninstallDialog, as uninstall is already defined in management API. On 2013/01/15 23:19:50, Antony Sargent wrote: > Seeing this reminds me of my comment on the previous revision of the CL, that we > want to push as much stuff as possible out of this 'global' DeveloperPrivateAPI > object into specific *ExtensionFunction implementation objects, with helper > objects/static methods as needed to reduce code duplication. > > I think the way to think about it should be this: > > -DeveloperPrivateAPI is a profile keyed service, so it lives longer than any > single API method call. It should only contain state and have methods needed to > operate outside of the context of any API method call. In concrete terms, I > think that means keep the Observe method and perhaps the > last_unpacked_directory_ member variable. > > -Any state/helper methods that are only used during one particular method API > call and not needed afterwards should be pushed into the *ExtensionFunction > implementation object (with helpers as noted above) > > > https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.h:60: void AddItemsInfo(const ExtensionSet& items, On 2013/01/15 23:19:50, Antony Sargent wrote: > here too - this should be moved out of this global object > Done. https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.h:100: void GetShellWindowPagesForExtensionProfile( There are other methods like GetShellWindowsForApp etc. To avoid confusion, this looks ok to me. On 2013/01/15 23:19:50, Antony Sargent wrote: > nit: would GetShellWindows be a better name? https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.h:121: std::string extension_id_prompting_; No more needed. Removed. On 2013/01/15 23:19:50, Antony Sargent wrote: > Assuming we do need this (see my question in developer_private.idl), I'm worried > that it could get out of sync if we get multiple calls in succession without the > user making a selection in the dialog. I see you have some code to prevent this > right now, but the pattern seems like it could be prone to breaking with > subsequent changes by people who aren't aware of the pitfall. > > Can we instead push this onto the *ExtensionFunction instance corresponding to > an incoming API call? https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.h:237: friend EntryPicker; Added an interface client and removed it as friend. On 2013/01/15 23:19:50, Antony Sargent wrote: > I'd like to avoid having EntryPicker be a friend class here. Two ways to do that > would be: > > -Define an interface in EntryPicker that DeveloperPrivateChooseEntryFunction can > implement > > or > > -Have EntryPicker take a base::Callback that it will call back with a non-empty > FilePath if the user selected a file, and an empty one if the dialog was > cancelled. > https://codereview.chromium.org/11734031/diff/16018/chrome/browser/resources/... File chrome/browser/resources/apps_debugger/manifest.json (right): https://codereview.chromium.org/11734031/diff/16018/chrome/browser/resources/... chrome/browser/resources/apps_debugger/manifest.json:12: "default-src 'self' chrome-extension-resource:; object-src 'self'; connect-src *; style-src 'unsafe-inline' *; img-src *; media-src *;" made it more strict " "default-src 'self'; style-src 'unsafe-inline' *; img-src *;" On 2013/01/15 23:19:50, Antony Sargent wrote: > Can you see if we can tighten this further to: > > "default-src 'self'; img-src 'self' chrome-extension-resource:" > > ? https://codereview.chromium.org/11734031/diff/16018/chrome/common/extensions/... File chrome/common/extensions/api/developer_private.idl (right): https://codereview.chromium.org/11734031/diff/16018/chrome/common/extensions/... chrome/common/extensions/api/developer_private.idl:102: static void uninstall(DOMString itemId, BooleanCallback callback); Yes. Removed. On 2013/01/15 23:19:50, Antony Sargent wrote: > Is there some reason you can't just use chrome.management.uninstall? > https://codereview.chromium.org/11734031/diff/16018/chrome/common/extensions/... chrome/common/extensions/api/developer_private.idl:113: // strings used in apps_debugger app. On 2013/01/15 23:19:50, Antony Sargent wrote: > Can you add to the end of the sentence here something like "as a dictionary > mapping a string identifier to the translated string to use in the UI." Done.
https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/11734031/diff/16018/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:639: dict->SetString(id, l10n_util::GetStringUTF16(idr)) cc @estade : Any Ideas on this. On 2013/01/17 02:04:11, Gaurav wrote: > This looks to be a great idea, and make the readability much easier. > > Adding more people for opinion. Thanks > On 2013/01/15 23:19:50, Antony Sargent wrote: > > We often resist the temptation to use macros just to shorten code, if you > could > > achieve a similar result with something like an inlined or static method. > > > > However, one idea for an improvement here would require a macro - it could > make > > it easier to see where a given string is defined if we used the stringified > > version of the resource id as the key in this dictionary. For example: > > > > #define SET_STRING(resourceid)\ > > dict->SetString(#resourceid, l10n_util::GetStringUTF16(resourceid)); > > > > > > ... > > SET_STRING(IDS_EXTENSIONS_INSPECT_VIEWS) > > ... > > > > > > > > Then in the html file you'd have: > > > > <span i18n-content="IDS_EXTENSIONS_INSPECT_VIEWS"></span> > > > > > > I glanced briefly at some of the other html files in the > > chrome/browser/resources directory and most don't seem to use this pattern, > > although the chrome/browser/resources/file_manager/ comes closest to it with a > > slightly different macro used in FileDialogStringsFunction::RunImpl, where > they > > elide the namespace name from the key. > > > > Maybe you should ask the opinion of some of the people in the > > chrome/browser/resources/OWNERS file why we don't already use the pattern I > > describe above for translated strings. (You'll need someone on there to be a > > reviewer for this CL to ok the new directory anyway). >
LGTM with a few things to fix before submitting https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:78: break; If you're going to be doing something here in a subsequent CL, it's probably fine to just leave this in, but right now it looks you could just skip having this object observing events altogether since you aren't doing anything with them. =) https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:229: DeveloperPrivateGetItemsInfoFunction::GetShellWindowPagesForExtensionProfile( Clearly this is a sign that "GetShellWindowPagesForExtensionProfile" is too long a name, because this doesn't all fit on one line. =) Assuming you still don't want to shorten it, one style nit - I think we usually prefer having the linebreak at the "::" instead of after the "void". E.g. void DeveloperPrivateGetItemsInfoFunction:: GetShellWindowPagesForExtensionProfile(... https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:259: view(new developer::ItemInspectView()); style nit: line break should be at the "(", so: linked_ptr<developer::ItemInspectView> view( new developer::ItemInspectView()); http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:275: DeveloperPrivateGetItemsInfoFunction::GetInspectablePagesForExtension( same style nit here on line break at the :: https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:351: bool allow = false; (Security paranoia) Please add the following call here: EXTENSION_FUNCTION_VALIDATE(user_gesture_); This ensures that this function will only do its work when a user actually clicked on something (our infrastructure code already takes care of detecting when a user just clicked on a button or typed something, and puts the right value into the user_gesture_ flag we inherit from ExtensionFunction). https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:395: bool DeveloperPrivateEnableFunction::RunImpl() { The implementation of this function looks pretty similar to the implementation of the chrome.management.setEnabled method (ManagementSetEnabledFunction function in chrome/browser/extensions/api/management/management_api.cc ). It looks like the only real difference is that you do the RequirementsChecker stuff here. Should the same checks be done in ManagementSetEnabledFunction? Or are they only relevant in developer mode? Could we instead have a chrome.developerPrivate.checkRequirements method, and then assuming that returns true, the developer UI app would then call chrome.management.setEnabled? Or is there an opportunity to share some of the underlying implementation of these? I think it's ok to go with this as-is for now, but please create a bug to follow up on this issue. https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:438: AsWeakPtr(), extension_id)); Are you sure this works? If you don't AddRef yourself before calling this, I think the fact that we return below means this object will get deleted because the ref count will go to 0, so I don't see how we'll ever end up getting our DeveloperPrivateEnableFunction::OnRequirementsChecked function called. (I don't think the SupportsWeakPtr stuff automatically calls AddRef for us). https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:518: AddRef(); can you add a comment here about where this is balanced with a Release? E.g. AddRef(); // Balanced in FileSelected/FileSelectionCanceled. https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:523: bool DeveloperPrivateChooseEntryFunction::ShowPicker( nit: remove extra space after "bool " https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... File chrome/browser/extensions/api/developer_private/entry_picker.cc (right): https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/entry_picker.cc:22: FilePath* g_path_to_be_picked_for_test; nit: initialize to NULL (I forget if the compiler already does this for us with local statics, but it's better to be explicit) https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... File chrome/browser/extensions/api/developer_private/entry_picker.h (right): https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/entry_picker.h:28: EntryPicker(EntryPickerClient* function, nit: "client" would be a better name than "function" https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/entry_picker.h:52: EntryPickerClient* function_; same nit here: "client_" instead of "function_" https://codereview.chromium.org/11734031/diff/38001/chrome/common/extensions/... File chrome/common/extensions/api/developer_private.idl (right): https://codereview.chromium.org/11734031/diff/38001/chrome/common/extensions/... chrome/common/extensions/api/developer_private.idl:93: // Enable / Disable file access for a given |itemId| nit: end comment with a period. (same thing elsewhere)
https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:78: break; I will adding an event listener to the API, so would leave like this in this CL. On 2013/01/17 23:46:26, Antony Sargent wrote: > If you're going to be doing something here in a subsequent CL, it's probably > fine to just leave this in, but right now it looks you could just skip having > this object observing events altogether since you aren't doing anything with > them. =) https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:229: DeveloperPrivateGetItemsInfoFunction::GetShellWindowPagesForExtensionProfile( On 2013/01/17 23:46:26, Antony Sargent wrote: > Clearly this is a sign that "GetShellWindowPagesForExtensionProfile" is too long > a name, because this doesn't all fit on one line. =) > > > Assuming you still don't want to shorten it, one style nit - I think we usually > prefer having the linebreak at the "::" instead of after the "void". E.g. > > void DeveloperPrivateGetItemsInfoFunction:: > GetShellWindowPagesForExtensionProfile(... > > > Done. https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:259: view(new developer::ItemInspectView()); On 2013/01/17 23:46:26, Antony Sargent wrote: > style nit: line break should be at the "(", so: > > linked_ptr<developer::ItemInspectView> view( > new developer::ItemInspectView()); > > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi... > Done. https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:275: DeveloperPrivateGetItemsInfoFunction::GetInspectablePagesForExtension( On 2013/01/17 23:46:26, Antony Sargent wrote: > same style nit here on line break at the :: Done. https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:351: bool allow = false; On 2013/01/17 23:46:26, Antony Sargent wrote: > (Security paranoia) > > Please add the following call here: > > EXTENSION_FUNCTION_VALIDATE(user_gesture_); > > This ensures that this function will only do its work when a user actually > clicked on something (our infrastructure code already takes care of detecting > when a user just clicked on a button or typed something, and puts the right > value into the user_gesture_ flag we inherit from ExtensionFunction). Done. https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:395: bool DeveloperPrivateEnableFunction::RunImpl() { Creating a bug to track this. On 2013/01/17 23:46:26, Antony Sargent wrote: > The implementation of this function looks pretty similar to the implementation > of the chrome.management.setEnabled method (ManagementSetEnabledFunction > function in chrome/browser/extensions/api/management/management_api.cc ). > > It looks like the only real difference is that you do the RequirementsChecker > stuff here. Should the same checks be done in ManagementSetEnabledFunction? Or > are they only relevant in developer mode? Could we instead have a > chrome.developerPrivate.checkRequirements method, and then assuming that returns > true, the developer UI app would then call chrome.management.setEnabled? > > Or is there an opportunity to share some of the underlying implementation of > these? > > I think it's ok to go with this as-is for now, but please create a bug to follow > up on this issue. > https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:395: bool DeveloperPrivateEnableFunction::RunImpl() { Creating a bug to track this. On 2013/01/17 23:46:26, Antony Sargent wrote: > The implementation of this function looks pretty similar to the implementation > of the chrome.management.setEnabled method (ManagementSetEnabledFunction > function in chrome/browser/extensions/api/management/management_api.cc ). > > It looks like the only real difference is that you do the RequirementsChecker > stuff here. Should the same checks be done in ManagementSetEnabledFunction? Or > are they only relevant in developer mode? Could we instead have a > chrome.developerPrivate.checkRequirements method, and then assuming that returns > true, the developer UI app would then call chrome.management.setEnabled? > > Or is there an opportunity to share some of the underlying implementation of > these? > > I think it's ok to go with this as-is for now, but please create a bug to follow > up on this issue. > https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:438: AsWeakPtr(), extension_id)); It wouldn't have worked. I added the ref counting. Thanks On 2013/01/17 23:46:26, Antony Sargent wrote: > Are you sure this works? If you don't AddRef yourself before calling this, I > think the fact that we return below means this object will get deleted because > the ref count will go to 0, so I don't see how we'll ever end up getting our > DeveloperPrivateEnableFunction::OnRequirementsChecked function called. > > (I don't think the SupportsWeakPtr stuff automatically calls AddRef for us). https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:518: AddRef(); On 2013/01/17 23:46:26, Antony Sargent wrote: > can you add a comment here about where this is balanced with a Release? E.g. > > AddRef(); // Balanced in FileSelected/FileSelectionCanceled. Done. https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/developer_private_api.cc:523: bool DeveloperPrivateChooseEntryFunction::ShowPicker( On 2013/01/17 23:46:26, Antony Sargent wrote: > nit: remove extra space after "bool " Done. https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... File chrome/browser/extensions/api/developer_private/entry_picker.cc (right): https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/entry_picker.cc:22: FilePath* g_path_to_be_picked_for_test; On 2013/01/17 23:46:26, Antony Sargent wrote: > nit: initialize to NULL (I forget if the compiler already does this for us with > local statics, but it's better to be explicit) Done. https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... File chrome/browser/extensions/api/developer_private/entry_picker.h (right): https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/entry_picker.h:28: EntryPicker(EntryPickerClient* function, On 2013/01/17 23:46:26, Antony Sargent wrote: > nit: "client" would be a better name than "function" Done. https://codereview.chromium.org/11734031/diff/38001/chrome/browser/extensions... chrome/browser/extensions/api/developer_private/entry_picker.h:52: EntryPickerClient* function_; On 2013/01/17 23:46:26, Antony Sargent wrote: > same nit here: "client_" instead of "function_" Done. https://codereview.chromium.org/11734031/diff/38001/chrome/common/extensions/... File chrome/common/extensions/api/developer_private.idl (right): https://codereview.chromium.org/11734031/diff/38001/chrome/common/extensions/... chrome/common/extensions/api/developer_private.idl:93: // Enable / Disable file access for a given |itemId| On 2013/01/17 23:46:26, Antony Sargent wrote: > nit: end comment with a period. (same thing elsewhere) Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grv@chromium.org/11734031/47002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/grv@chromium.org/11734031/50004
Message was sent while issue was closed.
Change committed as 177648 |