Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
152143003:
[ProtocolHandlers] Add a permission bubble client for RPH requests. (Closed)
|
Created:
6 years, 10 months ago by Greg Billock Modified:
6 years, 10 months ago CC:
chromium-reviews, benwells Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[ProtocolHandlers] Add a permission bubble client for RPH requests.
R=benwells@chromium.org
BUG=332115
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251222
Patch Set 1 #
Total comments: 7
Patch Set 2 : Add bubble request creation, rebase #Patch Set 3 : Move infobar/bubble call to browser.cc #
Total comments: 9
Patch Set 4 : tweaks #
Messages
Total messages: 21 (0 generated)
passing to koz to review ...
https://codereview.chromium.org/152143003/diff/1/chrome/browser/custom_handle... File chrome/browser/custom_handlers/register_protocol_handler_permission_delegate.h (right): https://codereview.chromium.org/152143003/diff/1/chrome/browser/custom_handle... chrome/browser/custom_handlers/register_protocol_handler_permission_delegate.h:13: class RegisterProtocolHandlerPermissionDelegate Could you add a class comment describing when this bubble gets displayed? Also, is this class referred to anywhere? I can't see any references to it in the rest of this CL. https://codereview.chromium.org/152143003/diff/1/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/152143003/diff/1/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1625: registry->RequestPermission(web_contents, handler); Better to not add RequestPermission() to ProtocolHandlerRegistry, because it doesn't handle view level concerns like that. I think you should inline that function here.
https://codereview.chromium.org/152143003/diff/1/chrome/browser/custom_handle... File chrome/browser/custom_handlers/register_protocol_handler_permission_delegate.h (right): https://codereview.chromium.org/152143003/diff/1/chrome/browser/custom_handle... chrome/browser/custom_handlers/register_protocol_handler_permission_delegate.h:13: class RegisterProtocolHandlerPermissionDelegate On 2014/02/06 03:03:26, koz wrote: > Could you add a class comment describing when this bubble gets displayed? > > Also, is this class referred to anywhere? I can't see any references to it in > the rest of this CL. Yeah, I didn't get the actual call site included. Added this comment and fixed that. https://codereview.chromium.org/152143003/diff/1/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/152143003/diff/1/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1625: registry->RequestPermission(web_contents, handler); On 2014/02/06 03:03:26, koz wrote: > Better to not add RequestPermission() to ProtocolHandlerRegistry, because it > doesn't handle view level concerns like that. I think you should inline that > function here. This is intended to contain the bubble/infobar split within the protocol handler registry rather than making it in browser.cc. Have a look: if you want I can move the whole thing here.
https://codereview.chromium.org/152143003/diff/1/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/152143003/diff/1/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1625: registry->RequestPermission(web_contents, handler); On 2014/02/06 22:23:49, Greg Billock wrote: > On 2014/02/06 03:03:26, koz wrote: > > Better to not add RequestPermission() to ProtocolHandlerRegistry, because it > > doesn't handle view level concerns like that. I think you should inline that > > function here. > > This is intended to contain the bubble/infobar split within the protocol handler > registry rather than making it in browser.cc. Have a look: if you want I can > move the whole thing here. Yes please. I think it would make more sense to have this logic in browser.cc. ProtocolHandlerRegistry is currently completely ignorant of UI concepts like WebContents and the difference between when it's appropriate to show an infobar or a bubble, whereas browser.cc is a view class and deals with this kind of thing already - the class creates infobars in many other situations.
https://codereview.chromium.org/152143003/diff/1/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/152143003/diff/1/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1625: registry->RequestPermission(web_contents, handler); On 2014/02/06 23:34:10, koz wrote: > On 2014/02/06 22:23:49, Greg Billock wrote: > > On 2014/02/06 03:03:26, koz wrote: > > > Better to not add RequestPermission() to ProtocolHandlerRegistry, because it > > > doesn't handle view level concerns like that. I think you should inline that > > > function here. > > > > This is intended to contain the bubble/infobar split within the protocol > handler > > registry rather than making it in browser.cc. Have a look: if you want I can > > move the whole thing here. > > Yes please. I think it would make more sense to have this logic in browser.cc. > ProtocolHandlerRegistry is currently completely ignorant of UI concepts like > WebContents and the difference between when it's appropriate to show an infobar > or a bubble, whereas browser.cc is a view class and deals with this kind of > thing already - the class creates infobars in many other situations. Sounds good. The context is that I've been doing several other conversions, and by and large the feature-specific coordinator class is what triggers the infobar in these cases (webrtc, midi, quota, desktop notifications, multiple downloads, geolocation all have this setup), so that's the expectation I'm seeing. That said, I don't think it's that big a thing.
lgtm https://codereview.chromium.org/152143003/diff/1/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/152143003/diff/1/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:1625: registry->RequestPermission(web_contents, handler); On 2014/02/10 23:12:51, Greg Billock wrote: > On 2014/02/06 23:34:10, koz wrote: > > On 2014/02/06 22:23:49, Greg Billock wrote: > > > On 2014/02/06 03:03:26, koz wrote: > > > > Better to not add RequestPermission() to ProtocolHandlerRegistry, because > it > > > > doesn't handle view level concerns like that. I think you should inline > that > > > > function here. > > > > > > This is intended to contain the bubble/infobar split within the protocol > > handler > > > registry rather than making it in browser.cc. Have a look: if you want I can > > > move the whole thing here. > > > > Yes please. I think it would make more sense to have this logic in browser.cc. > > ProtocolHandlerRegistry is currently completely ignorant of UI concepts like > > WebContents and the difference between when it's appropriate to show an > infobar > > or a bubble, whereas browser.cc is a view class and deals with this kind of > > thing already - the class creates infobars in many other situations. > > Sounds good. > > The context is that I've been doing several other conversions, and by and large > the feature-specific coordinator class is what triggers the infobar in these > cases (webrtc, midi, quota, desktop notifications, multiple downloads, > geolocation all have this setup), so that's the expectation I'm seeing. That > said, I don't think it's that big a thing. Yeah, it's probably not, but I'd be afraid of more UI things slipping into ProtocolHandlerRegistry. Thanks!
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/152143003/130001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
On 2014/02/11 16:47:01, I haz the power (commit-bot) wrote: > Retried try job too often on chromium_presubmit for step(s) presubmit > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p... +thestig for chrome owners +pkasting for browser.cc owners
https://codereview.chromium.org/152143003/diff/130001/chrome/browser/custom_h... File chrome/browser/custom_handlers/register_protocol_handler_permission_request.cc (right): https://codereview.chromium.org/152143003/diff/130001/chrome/browser/custom_h... chrome/browser/custom_handlers/register_protocol_handler_permission_request.cc:40: l10n_util::GetStringFUTF16(IDS_REGISTER_PROTOCOL_HANDLER_CONFIRM, Since both the if and else case take the same parameters, you can do: int id = old_handler.IsEmpty() ? IDS_FOO : IDS_BAR; return l10n_util::GetStringFUTF16(id, ...) https://codereview.chromium.org/152143003/diff/130001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/152143003/diff/130001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1630: PermissionBubbleManager::FromWebContents(web_contents); Reuse the result from line 1628?
LGTM https://codereview.chromium.org/152143003/diff/130001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/152143003/diff/130001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1630: PermissionBubbleManager::FromWebContents(web_contents); On 2014/02/13 21:48:30, Lei Zhang wrote: > Reuse the result from line 1628? SGTM https://codereview.chromium.org/152143003/diff/130001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1631: bubble_manager->AddRequest(new RegisterProtocolHandlerPermissionRequest( Tiny nit: I would probably break before "new" instead https://codereview.chromium.org/152143003/diff/130001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1633: return; Nit: Instead of this return I'd do "else" after the conditional and call Create() inside that. This is safer in cases someone comes along in the future and adds more code to the end of this function.
lgtm++
The CQ bit was checked by gbillock@chromium.org
The CQ bit was unchecked by gbillock@chromium.org
https://codereview.chromium.org/152143003/diff/130001/chrome/browser/custom_h... File chrome/browser/custom_handlers/register_protocol_handler_permission_request.cc (right): https://codereview.chromium.org/152143003/diff/130001/chrome/browser/custom_h... chrome/browser/custom_handlers/register_protocol_handler_permission_request.cc:40: l10n_util::GetStringFUTF16(IDS_REGISTER_PROTOCOL_HANDLER_CONFIRM, On 2014/02/13 21:48:30, Lei Zhang wrote: > Since both the if and else case take the same parameters, you can do: > int id = old_handler.IsEmpty() ? IDS_FOO : IDS_BAR; > return l10n_util::GetStringFUTF16(id, ...) mostly the same, yes. the old title is different, and there's a debug check to make sure all params are used. https://codereview.chromium.org/152143003/diff/130001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/152143003/diff/130001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1630: PermissionBubbleManager::FromWebContents(web_contents); On 2014/02/13 21:50:36, Peter Kasting wrote: > On 2014/02/13 21:48:30, Lei Zhang wrote: > > Reuse the result from line 1628? > > SGTM Done. https://codereview.chromium.org/152143003/diff/130001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1631: bubble_manager->AddRequest(new RegisterProtocolHandlerPermissionRequest( On 2014/02/13 21:50:36, Peter Kasting wrote: > Tiny nit: I would probably break before "new" instead Done. https://codereview.chromium.org/152143003/diff/130001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1633: return; On 2014/02/13 21:50:36, Peter Kasting wrote: > Nit: Instead of this return I'd do "else" after the conditional and call > Create() inside that. This is safer in cases someone comes along in the future > and adds more code to the end of this function. Done.
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/152143003/390001
Message was sent while issue was closed.
Change committed as 251222 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
