Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1008973002:
[Extensions UI] Update extensions page to use api methods for permissions prompt (Closed)
|
Created:
5 years, 9 months ago by Devlin Modified:
5 years, 9 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, arv+watch_chromium.org, extensions-reviews_chromium.org, tsergeant Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Extensions UI] Update extensions page to use api methods for permissions prompt
Update the chrome://extensions page to use the developerPrivate api in order
to show the permissions prompt.
Also convert the function to a UIThreadExtensionFunction, and update it to
(if possible) use the new app info dialog.
BUG=461039
Committed: https://crrev.com/927c9f4297938a4435ed7926dfa547f630890b9d
Cr-Commit-Position: refs/heads/master@{#320801}
Patch Set 1 : #
Total comments: 18
Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 8
Patch Set 4 : #Messages
Total messages: 22 (6 generated)
Patchset #1 (id:1) has been deleted
rdevlin.cronin@chromium.org changed reviewers: + kalman@chromium.org
easy one, and minus another 100 lines of duplicate code.
kalman@chromium.org changed reviewers: + sashab@chromium.org
"easy one" is tempting fate. https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:572: EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &target_extension_id_)); Use the generated Params struct, don't pull out of |args_| directly. https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:588: AppInfoLaunchSource::NUM_LAUNCH_SOURCES); FYI +sashab are you using this UMA? Is it useful? https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:591: // Display the dialog at a size similar to the app list. We should delete this comment, it's not actually true. https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:594: GetAppInfoNativeDialogSize(), FYI +sashab but somebody should look into why the dialogue is so large. It doesn't need to be if it's invoked from chrome://extensions - it looks pretty weird actually. A hackity fix here would be to vary the size depending on whether the source was webui or the app list. perhaps that would be a reasonable enough TODO. https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:601: prompt_.reset(new ExtensionInstallPrompt(web_contents)); These details should really not be part of this API. A nice way to architect all of this would be to pull this into a new file in the developer_private API directory to be responsible for how to show the "permissions dialogue" - with just a base::Bind interface. how nice would that be. https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:627: DeveloperPrivateReloadFunction::~DeveloperPrivateReloadFunction() {} Get in line, buddy!
sashab@chromium.org changed reviewers: + benwells@chromium.org
+benwells for comment, +tsergeant for fyi. https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:588: AppInfoLaunchSource::NUM_LAUNCH_SOURCES); On 2015/03/13 21:41:38, kalman wrote: > FYI +sashab are you using this UMA? Is it useful? No, but we might use it in the future. +benwells who might know more about its usefulness. https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:594: GetAppInfoNativeDialogSize(), On 2015/03/13 21:41:38, kalman wrote: > FYI +sashab > > but somebody should look into why the dialogue is so large. It doesn't need to > be if it's invoked from chrome://extensions - it looks pretty weird actually. A > hackity fix here would be to vary the size depending on whether the source was > webui or the app list. perhaps that would be a reasonable enough TODO. If the extension has a lot of permissions, the extra space makes sense. And yes - the dialog needs to be a different size depending on where it's launched from. Also remember that with the new-style app launcher, the app info dialog launched from the app list will be a different size, and we definitely don't want that size when opening from the extensions page.
https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:594: GetAppInfoNativeDialogSize(), On 2015/03/13 21:59:26, sasha_b wrote: > On 2015/03/13 21:41:38, kalman wrote: > > FYI +sashab > > > > but somebody should look into why the dialogue is so large. It doesn't need to > > be if it's invoked from chrome://extensions - it looks pretty weird actually. > A > > hackity fix here would be to vary the size depending on whether the source was > > webui or the app list. perhaps that would be a reasonable enough TODO. > > If the extension has a lot of permissions, the extra space makes sense. And yes > - the dialog needs to be a different size depending on where it's launched from. > Also remember that with the new-style app launcher, the app info dialog launched > from the app list will be a different size, and we definitely don't want that > size when opening from the extensions page. Ok, it seems like the API should be a min/max size, not an absolute size, and then let the dialogue resize itself in those bounds (or add a scrollbar if necessary).
https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:594: GetAppInfoNativeDialogSize(), On 2015/03/13 22:02:20, kalman wrote: > On 2015/03/13 21:59:26, sasha_b wrote: > > On 2015/03/13 21:41:38, kalman wrote: > > > FYI +sashab > > > > > > but somebody should look into why the dialogue is so large. It doesn't need > to > > > be if it's invoked from chrome://extensions - it looks pretty weird > actually. > > A > > > hackity fix here would be to vary the size depending on whether the source > was > > > webui or the app list. perhaps that would be a reasonable enough TODO. > > > > If the extension has a lot of permissions, the extra space makes sense. And > yes > > - the dialog needs to be a different size depending on where it's launched > from. > > Also remember that with the new-style app launcher, the app info dialog > launched > > from the app list will be a different size, and we definitely don't want that > > size when opening from the extensions page. > > Ok, it seems like the API should be a min/max size, not an absolute size, and > then let the dialogue resize itself in those bounds (or add a scrollbar if > necessary). Yup, that SGTM. Although I'm pretty sure UI was keen on keeping the 'A4 paper'-shaped general sizing, might have to check with them what is/isn't OK for a dialog with the header/footer style we've designed.
https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:594: GetAppInfoNativeDialogSize(), On 2015/03/13 22:07:51, sasha_b wrote: > On 2015/03/13 22:02:20, kalman wrote: > > On 2015/03/13 21:59:26, sasha_b wrote: > > > On 2015/03/13 21:41:38, kalman wrote: > > > > FYI +sashab > > > > > > > > but somebody should look into why the dialogue is so large. It doesn't > need > > to > > > > be if it's invoked from chrome://extensions - it looks pretty weird > > actually. > > > A > > > > hackity fix here would be to vary the size depending on whether the source > > was > > > > webui or the app list. perhaps that would be a reasonable enough TODO. > > > > > > If the extension has a lot of permissions, the extra space makes sense. And > > yes > > > - the dialog needs to be a different size depending on where it's launched > > from. > > > Also remember that with the new-style app launcher, the app info dialog > > launched > > > from the app list will be a different size, and we definitely don't want > that > > > size when opening from the extensions page. > > > > Ok, it seems like the API should be a min/max size, not an absolute size, and > > then let the dialogue resize itself in those bounds (or add a scrollbar if > > necessary). > > Yup, that SGTM. Although I'm pretty sure UI was keen on keeping the 'A4 > paper'-shaped general sizing, might have to check with them what is/isn't OK for > a dialog with the header/footer style we've designed. Probably not A4, but maybe Letter :-)
https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:588: AppInfoLaunchSource::NUM_LAUNCH_SOURCES); On 2015/03/13 21:59:26, sasha_b wrote: > On 2015/03/13 21:41:38, kalman wrote: > > FYI +sashab are you using this UMA? Is it useful? > > No, but we might use it in the future. +benwells who might know more about its > usefulness. I think we should keep it. It is interesting data to get. Is there a reason to remove it?
https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:588: AppInfoLaunchSource::NUM_LAUNCH_SOURCES); On 2015/03/13 22:15:11, benwells wrote: > On 2015/03/13 21:59:26, sasha_b wrote: > > On 2015/03/13 21:41:38, kalman wrote: > > > FYI +sashab are you using this UMA? Is it useful? > > > > No, but we might use it in the future. +benwells who might know more about its > > usefulness. > > I think we should keep it. It is interesting data to get. > > Is there a reason to remove it? Not if it's interesting data.
https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:572: EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &target_extension_id_)); On 2015/03/13 21:41:38, kalman wrote: > Use the generated Params struct, don't pull out of |args_| directly. Done. https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:588: AppInfoLaunchSource::NUM_LAUNCH_SOURCES); On 2015/03/13 22:17:26, kalman wrote: > On 2015/03/13 22:15:11, benwells wrote: > > On 2015/03/13 21:59:26, sasha_b wrote: > > > On 2015/03/13 21:41:38, kalman wrote: > > > > FYI +sashab are you using this UMA? Is it useful? > > > > > > No, but we might use it in the future. +benwells who might know more about > its > > > usefulness. > > > > I think we should keep it. It is interesting data to get. > > > > Is there a reason to remove it? > > Not if it's interesting data. Concensus: Leaving in for now. https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:591: // Display the dialog at a size similar to the app list. On 2015/03/13 21:41:38, kalman wrote: > We should delete this comment, it's not actually true. Done. https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:601: prompt_.reset(new ExtensionInstallPrompt(web_contents)); On 2015/03/13 21:41:38, kalman wrote: > These details should really not be part of this API. A nice way to architect all > of this would be to pull this into a new file in the developer_private API > directory to be responsible for how to show the "permissions dialogue" - with > just a base::Bind interface. how nice would that be. Sure, why not. https://codereview.chromium.org/1008973002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:627: DeveloperPrivateReloadFunction::~DeveloperPrivateReloadFunction() {} On 2015/03/13 21:41:38, kalman wrote: > Get in line, buddy! Done.
https://codereview.chromium.org/1008973002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/1008973002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:588: show_permissions_dialog_helper_.reset(new ShowPermissionsDialogHelper()); Is this a leak? - This class holds onto a (the) reference to show_permissions_dialog_helper_. - show_permissions_dialog_helper_ holds onto a reference to this because it holds a reference to the callback that you create here, which has a reference to this. I had in mind a static Show method: ShowPermissionsDialogHelper::Show(web_contents, ...); with everything else private. ShowPermissionsDialogHelper would then know to delete itself after calling the callback, releasing the refcount. Or, you could remember to call show_permissions_dialog_helper_.reset() in Finish, but that's not an obvious interface. https://codereview.chromium.org/1008973002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.cc (right): https://codereview.chromium.org/1008973002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.cc:52: base::Unretained(this))); Would it be too neat to just pass |on_complete| into here directly?
https://codereview.chromium.org/1008973002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/1008973002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:588: show_permissions_dialog_helper_.reset(new ShowPermissionsDialogHelper()); On 2015/03/13 23:52:31, kalman wrote: > Is this a leak? > - This class holds onto a (the) reference to show_permissions_dialog_helper_. > - show_permissions_dialog_helper_ holds onto a reference to this because it > holds a reference to the callback that you create here, which has a reference to > this. > > I had in mind a static Show method: > > ShowPermissionsDialogHelper::Show(web_contents, ...); > > with everything else private. ShowPermissionsDialogHelper would then know to > delete itself after calling the callback, releasing the refcount. > > Or, you could remember to call show_permissions_dialog_helper_.reset() in > Finish, but that's not an obvious interface. Done. https://codereview.chromium.org/1008973002/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.cc (right): https://codereview.chromium.org/1008973002/diff/40001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.cc:52: base::Unretained(this))); On 2015/03/13 23:52:31, kalman wrote: > Would it be too neat to just pass |on_complete| into here directly? Nope, just neat enough. :) Done.
lgtm https://codereview.chromium.org/1008973002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/1008973002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:592: extension() == nullptr, // From extensions page if no calling extension. There's a method IsWebUI() or something, use that - safer check (uses the context type rather than the presence of the extension). https://codereview.chromium.org/1008973002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.cc (right): https://codereview.chromium.org/1008973002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.cc:47: // The Unretained() is safe here because this object's parent should never I don't see an Unretained() https://codereview.chromium.org/1008973002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.h (right): https://codereview.chromium.org/1008973002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.h:28: content::BrowserContext* browser_context, I don't think it's necessarily worth taking action on this comment, by anyway: I personally prefer to order my parameters from most to least scope. Possibly because of functional languages (and now base::Bind) when functions are typically passed though some state, then Run()-d with a specific parameter. In this case, put the browsercontext before the webcontents. That said, there's a GetBrowserContext() method on WebContents if you want to avoid this altogether. https://codereview.chromium.org/1008973002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.h:30: bool from_extensions_page, This flag is actually a bit confusing. I don't know whether "extensions page" in this context means "a page in an extension" or "chrome://extensions" -- the latter, I presume? But from_webui might be better.
https://codereview.chromium.org/1008973002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/developer_private_api.cc (right): https://codereview.chromium.org/1008973002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/developer_private_api.cc:592: extension() == nullptr, // From extensions page if no calling extension. On 2015/03/16 17:39:55, kalman wrote: > There's a method IsWebUI() or something, use that - safer check (uses the > context type rather than the presence of the extension). Done. https://codereview.chromium.org/1008973002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.cc (right): https://codereview.chromium.org/1008973002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.cc:47: // The Unretained() is safe here because this object's parent should never On 2015/03/16 17:39:55, kalman wrote: > I don't see an Unretained() Done. https://codereview.chromium.org/1008973002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.h (right): https://codereview.chromium.org/1008973002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.h:28: content::BrowserContext* browser_context, On 2015/03/16 17:39:55, kalman wrote: > I don't think it's necessarily worth taking action on this comment, by anyway: > > I personally prefer to order my parameters from most to least scope. Possibly > because of functional languages (and now base::Bind) when functions are > typically passed though some state, then Run()-d with a specific parameter. In > this case, put the browsercontext before the webcontents. > > That said, there's a GetBrowserContext() method on WebContents if you want to > avoid this altogether. I figured best to avoid the GetBrowserContext() on the web_contents on the crazy offchance that they are different (one incognito?). Not that that can happen yet, but figured might as well be prepared. Happy to reorder, though - done. https://codereview.chromium.org/1008973002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/developer_private/show_permissions_dialog_helper.h:30: bool from_extensions_page, On 2015/03/16 17:39:55, kalman wrote: > This flag is actually a bit confusing. I don't know whether "extensions page" in > this context means "a page in an extension" or "chrome://extensions" -- the > latter, I presume? But from_webui might be better. The reason I said from_extensions_page was to match the AppInfoLaunchSource enum below. But it's probably safe to say that the only webui calling this will be the chrome extensions page, at least for now.
The CQ bit was checked by rdevlin.cronin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1008973002/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1008973002/80001
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/927c9f4297938a4435ed7926dfa547f630890b9d Cr-Commit-Position: refs/heads/master@{#320801} |
