Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
278773002:
Add 'Revoke File Permissions' button to App Info Dialog (Closed)
|
Created:
6 years, 7 months ago by sashab Modified:
6 years, 7 months ago CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd 'Revoke File Permissions' button to App Info Dialog
Allowed revoking of file permissions for apps that required file permissions.
This button appears in the 'expanded' section of the app info dialog under
'Files'.
BUG=371691
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271340
Patch Set 1 #Patch Set 2 : Updated button text #
Total comments: 23
Patch Set 3 : Small fixes #
Total comments: 8
Patch Set 4 : Fixes from review + slightly simplified layout #
Messages
Total messages: 12 (0 generated)
https://codereview.chromium.org/278773002/diff/20001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/278773002/diff/20001/chrome/app/generated_res... chrome/app/generated_resources.grd:2261: + <message name="IDS_APPLICATION_INFO_REVOKE_RETAINED_FILE_PERMISSIONS_BUTTON_TEXT" desc="Text displayed in the button to remove the app's file permissions. This no longer allows the app to access the files."> Awkwardly written. ("This no longer..." implies that you have changed the behaviour.) Try: "After pressing this button, the app no longer has access to the files." https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc (right): https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:49: views::View* revoke_button); I think the type of this should be views::Button*. Even if you aren't specifically using Button methods, it is still good to constrain the type to the most specific one that makes sense (i.e., LabelButton is too restrictive since it doesn't really matter for the purposes of this view whether it is a LabelButton or some other kind of button, but it should definitely be a Button). https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:72: views::View* revoke_button); As above; this should be views::Button*. https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:72: views::View* revoke_button); // |revoke_button| may be NULL. https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:108: const std::vector<base::string16> messages, Umm, should this be a reference? (I see that you're fixing this from a previous CL. Const without a reference isn't really necessary, but it probably should be a const &.) https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:111: // views::GridLayout* layout = views::GridLayout::CreatePanel(this); Was this accidentally left in? Please remove either this line or the next. https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:386: IDS_APPLICATION_INFO_REVOKE_RETAINED_FILE_PERMISSIONS_BUTTON_TEXT)); nit: Line too long. Can you run git cl format on the whole CL? https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:419: if (sender == revoke_file_permissions_button_) { nit: Don't need curlies for one-line if-then-else. https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:430: if (apps::AppRestoreService::Get(profile_)->IsAppRestorable(app_->id())) { nit: Don't need curlies. https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:438: return true; Why do you need this at all if it just returns true? https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.h (right): https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.h:56: virtual void RevokeFilePermissions(); Why are these virtual?
Thanks, sorry about all the small mistakes; this wasn't such a good quality CL. I've fixed it up a lot now - please review. https://codereview.chromium.org/278773002/diff/20001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/278773002/diff/20001/chrome/app/generated_res... chrome/app/generated_resources.grd:2261: + <message name="IDS_APPLICATION_INFO_REVOKE_RETAINED_FILE_PERMISSIONS_BUTTON_TEXT" desc="Text displayed in the button to remove the app's file permissions. This no longer allows the app to access the files."> On 2014/05/09 05:36:47, Matt Giuca wrote: > Awkwardly written. ("This no longer..." implies that you have changed the > behaviour.) Try: "After pressing this button, the app no longer has access to > the files." Nice, thanks. Done. https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc (right): https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:49: views::View* revoke_button); On 2014/05/09 05:36:47, Matt Giuca wrote: > I think the type of this should be views::Button*. Even if you aren't > specifically using Button methods, it is still good to constrain the type to the > most specific one that makes sense (i.e., LabelButton is too restrictive since > it doesn't really matter for the purposes of this view whether it is a > LabelButton or some other kind of button, but it should definitely be a Button). Yup, I agree with this. Done. https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:72: views::View* revoke_button); On 2014/05/09 05:36:47, Matt Giuca wrote: > // |revoke_button| may be NULL. Done. https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:72: views::View* revoke_button); On 2014/05/09 05:36:47, Matt Giuca wrote: > As above; this should be views::Button*. Done. https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:108: const std::vector<base::string16> messages, On 2014/05/09 05:36:47, Matt Giuca wrote: > Umm, should this be a reference? (I see that you're fixing this from a previous > CL. Const without a reference isn't really necessary, but it probably should be > a const &.) Yes, this is a fix from a previous CL. And it should have been a reference. Thanks. https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:111: // views::GridLayout* layout = views::GridLayout::CreatePanel(this); On 2014/05/09 05:36:47, Matt Giuca wrote: > Was this accidentally left in? Please remove either this line or the next. Oops, yup. Removed. https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:386: IDS_APPLICATION_INFO_REVOKE_RETAINED_FILE_PERMISSIONS_BUTTON_TEXT)); On 2014/05/09 05:36:47, Matt Giuca wrote: > nit: Line too long. > > Can you run git cl format on the whole CL? Weird, I did run the formatter. Will do it again. https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:419: if (sender == revoke_file_permissions_button_) { On 2014/05/09 05:36:47, Matt Giuca wrote: > nit: Don't need curlies for one-line if-then-else. Done. https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:430: if (apps::AppRestoreService::Get(profile_)->IsAppRestorable(app_->id())) { On 2014/05/09 05:36:47, Matt Giuca wrote: > nit: Don't need curlies. Done. https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:438: return true; On 2014/05/09 05:36:47, Matt Giuca wrote: > Why do you need this at all if it just returns true? Originally I was investigating whether there was any reason why a user would not be able to revoke an app's permissions. Turns out there isn't one. Function removed. https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.h (right): https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.h:56: virtual void RevokeFilePermissions(); On 2014/05/09 05:36:47, Matt Giuca wrote: > Why are these virtual? I don't think anything anything will extend this class, so there's no need for them to be virtual. Removed.
lgtm You'll need someone else to review app_info_dialog changes. (Maybe you should get yourself added as the owner of that directory.) https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.h (right): https://codereview.chromium.org/278773002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.h:56: virtual void RevokeFilePermissions(); You shouldn't make methods virtual just because "something might extend this class" or even "something does extend this class". A method should only be virtual if you *need* to override it. Every virtual method increases code complexity: 1. It's bad for performance; every virtual method call has to do an extra pointer lookup, and defeats optimizations like inlining. But more importantly, 2. It increases the potential amount of behaviour that can happen when you call the method. When I call a non-virtual method, I just have to consider what that method does. When I call a virtual method, I don't really know what I am calling and I have to consider all of the possible behaviour. You should treat virtual as a tool you use when you need it, not as a modifier to use whenever it might make sense (like const).
benwells@chromium.org: Please review changes in the app_info_dialog directory
https://codereview.chromium.org/278773002/diff/40001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/278773002/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:2256: + Revoke file access Don't we already have a string that is used in the other dialog? https://codereview.chromium.org/278773002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc (right): https://codereview.chromium.org/278773002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:49: views::Button* revoke_button); Nit: As far as this view is concerned, this could be any button. That is, it doesn't need any knowledge of what the button does, just that there is a button, so I think you should just name it button. https://codereview.chromium.org/278773002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:144: layout->AddPaddingRow(100.0f, views::kPanelVertMargin); What is the 100 here mean? Might be good to comment it. Also, how come we are adding a row even when there is no button? Is that something unrelated that you're getting in with this change? https://codereview.chromium.org/278773002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:425: if (apps::AppRestoreService::Get(profile_)->IsAppRestorable(app_->id())) This is rather counterintuitive. It would be nicer to have AppLoadService::RestartApplicationIfRunning, whihc I think is what the logic is. Can you add a TODO on me, and a bug assigned to me, to do that? I'm fiddling in that code now.
https://codereview.chromium.org/278773002/diff/40001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/278773002/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:2256: + Revoke file access On 2014/05/12 04:54:03, benwells wrote: > Don't we already have a string that is used in the other dialog? Yes, IDS_EXTENSION_PROMPT_PERMISSIONS_CLEAR_RETAINED_FILES_BUTTON. I considered re-using that string but I feel it belongs in the permissions prompt whereas this one belongs in the info dialog. I think keeping them distinct is good - we might change the layout of this dialog such that this button needs to say something slightly different (it's just a coincidence that they both say the same thing right now). https://codereview.chromium.org/278773002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc (right): https://codereview.chromium.org/278773002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:49: views::Button* revoke_button); On 2014/05/12 04:54:03, benwells wrote: > Nit: As far as this view is concerned, this could be any button. That is, it > doesn't need any knowledge of what the button does, just that there is a button, > so I think you should just name it button. Yup, good point. This is leftover from when all the revoke logic was in the ExpandableContainerView. Done. Although, you could argue that this could just be a View since all I'm doing is adding it to the end of the expandable bit (in other words, you can pass any arbitrary view and it will append it to the end of the container), but I think this would be too much of a generalisation (why should you be allowed to pass any arbitrary view and add it to the end of the expandable container?). I think keeping it as a views::Button and naming it button is a good balance of specificity/generality. If I wanted to make it more general, all it would take is a view and a title, and the view would be the collapsible part. But that's for another time :) https://codereview.chromium.org/278773002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:144: layout->AddPaddingRow(100.0f, views::kPanelVertMargin); On 2014/05/12 04:54:03, benwells wrote: > What is the 100 here mean? Might be good to comment it. Also, how come we are > adding a row even when there is no button? Is that something unrelated that > you're getting in with this change? 1. The 100.0 is the 'resize percent', to make the row go across all columns. This is actually a mistake on my part, and is not needed - all padding rows essentially cover all columns (I saw how this function is called in other dialogs) so I changed this to 0 correctly. 2. The change from views::GridLayout::CreatePanel() on line 101 to new views::GridLayout() means all spacing around this view was removed. The original insets (from the definition of CreatePanel) were: +------------+ _ | | _ kPanelVertMargin | +------+ | | | | | | +------+ | _ | | _ kPanelVertMargin +------------+ | | | | kButtonHEdgeMarginNew, I don't want to apply these insets to the whole grid because this insets the button as well (I want the button to be able to stretch to the edge of the view, whilst the bulleted list is inset). Something I did actually change is the horizontal padding (kButtonHEdgeMarginNew). I changed it to kPanelHorizMargin because this isn't a 'new style' dialog view (kButtonHEdgeMarginNew is too much padding - it's for the outermost view in new-style dialogs, and has already been applied 'outside' this view, so applying it twice looks weird). Without this function all these insets are 0, so I need to add kPanelVertMargin either side and kPanelHorizMargin on the top and bottom. This line adds the topmost padding, then there's actually a line missing to add the bottom padding. I've simplified it a bit now by putting the common insets into 'SetInsets' of the layout. Hopefully this makes more sense now - the way I've done it is a bit complicated, but the only other ways I can think of doing it are more complicated. https://codereview.chromium.org/278773002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:425: if (apps::AppRestoreService::Get(profile_)->IsAppRestorable(app_->id())) On 2014/05/12 04:54:03, benwells wrote: > This is rather counterintuitive. It would be nicer to have > AppLoadService::RestartApplicationIfRunning, whihc I think is what the logic is. > Can you add a TODO on me, and a bug assigned to me, to do that? I'm fiddling in > that code now. Done.
lgtm
The CQ bit was checked by sashab@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sashab@chromium.org/278773002/60001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
Message was sent while issue was closed.
Change committed as 271340 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
