Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
214163002:
Added separate required, optional and file permissions to app info dialog (Closed)
|
Created:
6 years, 9 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. |
DescriptionAdded collapsible permissions to the app info dialog
Separated the optional and required permissions in the dialog, and
added retained files as well. Also made the various types of
permissions collapsible, and added a message when the app has no
permissions whatsoever.
BUG=356985, 350746
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266518
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267161
Patch Set 1 #Patch Set 2 : Added collapsible permissions #Patch Set 3 : Made permissions collapsible, required permissions expanded by default #
Total comments: 16
Patch Set 4 : Reverted the RTL special cases, which it turns out Chrome takes care of normally :) #Patch Set 5 : Minor description fixes, dash fix and font size fix #
Total comments: 4
Patch Set 6 : Removed unicode from code #
Total comments: 4
Patch Set 7 : Unittests and minor fixes #Patch Set 8 : Actual unittests this time #
Total comments: 19
Patch Set 9 : Nit fixes + unittest improvements #Patch Set 10 : Attempted tsan fix and some leak fixes #
Messages
Total messages: 36 (0 generated)
This review is for adding the 3 separate scroll areas to the permissions tab. Screenshots on bug. I am also aware that the scroll areas are slightly different widths, which I have filed separately: crbug.com/356986
On 2014/03/27 02:54:38, sasha_b wrote: > This review is for adding the 3 separate scroll areas to the permissions tab. > > Screenshots on bug. I am also aware that the scroll areas are slightly different > widths, which I have filed separately: crbug.com/356986 Why not fix the scroll bar issue now?
I'm working on it now, but I thought I could do this review in parallel (since it's mostly independent). On Thu, Mar 27, 2014 at 2:40 PM, <benwells@chromium.org> wrote: > On 2014/03/27 02:54:38, sasha_b wrote: > >> This review is for adding the 3 separate scroll areas to the permissions >> tab. >> > > Screenshots on bug. I am also aware that the scroll areas are slightly >> > different > >> widths, which I have filed separately: crbug.com/356986 >> > > Why not fix the scroll bar issue now? > > https://codereview.chromium.org/214163002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I know this review is pretty big, but a lot of it is just short functions for getting permissions, etc. The main part is the collapse/expand animation which is mostly a lot of boilerplate. Hopefully the comments/structure help annotate what it's doing a little bit. Screenshots on bug crbug.com/356985.
https://codereview.chromium.org/214163002/diff/40001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/214163002/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:2244: + <message name="IDS_APPLICATION_INFO_REQUIRED_PERMISSIONS_TEXT" desc="Text displayed above the required permissions for the app."> After our discussion about translations, I think I want more descriptive descriptions. desc="Heading for the required permissions of an app (the permissions that the user must grant the app upon installation)." https://codereview.chromium.org/214163002/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:2247: + <message name="IDS_APPLICATION_INFO_OPTIONAL_PERMISSIONS_TEXT" desc="Text displayed above the optional permissions for the app."> desc="Heading for the optional permissions of an app (the permissions that the user may choose to grant the app)." https://codereview.chromium.org/214163002/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:2250: + <message name="IDS_APPLICATION_INFO_RETAINED_FILE_PERMISSIONS_TEXT" desc="Text displayed above the retained files for the app, which are files the app has permanent access to."> Heading for ... https://codereview.chromium.org/214163002/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:2254: + - Does this look better as a colon? It feels more natural ("Required: 2"). If you're using a dash, may as well use an em-dash ("Required — 2"). Also, does this need to be a string? Maybe it can just be a literal in the source code. I don't think we will want to localize it. https://codereview.chromium.org/214163002/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:2256: + <message name="IDS_APPLICATION_INFO_NO_PERMISSIONS_TEXT" desc="Text displayed in the Permissions area of the dialog if the app has no permissions of any kind."> "of the App Info dialog" https://codereview.chromium.org/214163002/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:2257: + This app has no special permissions. I'm not sure about the wording of this. Perhaps "This app requires no special permissions." https://codereview.chromium.org/214163002/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/214163002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:221: // case, format the title as 'number - Title' It's good that you're thinking about this, but have you tested it with an RTL language? I may be wrong, but as far as I can see, you shouldn't have to do anything special for RTL. It should automatically display the string in the "correct" order, and your efforts to deal with it may end up cancelling out. For example, Hebrew. Here I write the string "<Hebrew word for 'Required'> - 3": דרוש - 3 You can see that Chrome automatically places the 3 to the left of the hyphen to the left of the text. We briefly discussed this offline and maybe Views won't handle it this way, because they are actually three separate child views. But I'm still a bit skeptical about manually dealing with RTL at this level, since it's really something that should be dealt with by a text rendering system. Is there any way to make a substring of a label bold? If not, why not just make the entire label bold? https://codereview.chromium.org/214163002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:238: layout->StartRow(0, kSplitColumnSetId); May be same as above.
Also, the CL description is too long. Should be wrapped to 72 chars, and the first line should not continue on to the second (the first line should be a complete sentence <= 72 chars which is equal to the CL description).
See comments. Also changed the size of the headings to be the 'base' font size (instead of the base size + 1): https://chromium.googlecode.com/issues/attachment?aid=3569850008000&name=Coll... https://codereview.chromium.org/214163002/diff/40001/chrome/app/generated_res... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/214163002/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:2244: + <message name="IDS_APPLICATION_INFO_REQUIRED_PERMISSIONS_TEXT" desc="Text displayed above the required permissions for the app."> On 2014/04/16 05:39:16, Matt Giuca wrote: > After our discussion about translations, I think I want more descriptive > descriptions. > > desc="Heading for the required permissions of an app (the permissions that the > user must grant the app upon installation)." Done. https://codereview.chromium.org/214163002/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:2247: + <message name="IDS_APPLICATION_INFO_OPTIONAL_PERMISSIONS_TEXT" desc="Text displayed above the optional permissions for the app."> On 2014/04/16 05:39:16, Matt Giuca wrote: > desc="Heading for the optional permissions of an app (the permissions that the > user may choose to grant the app)." Done. https://codereview.chromium.org/214163002/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:2250: + <message name="IDS_APPLICATION_INFO_RETAINED_FILE_PERMISSIONS_TEXT" desc="Text displayed above the retained files for the app, which are files the app has permanent access to."> On 2014/04/16 05:39:16, Matt Giuca wrote: > Heading for ... Done. https://codereview.chromium.org/214163002/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:2254: + - On 2014/04/16 05:39:16, Matt Giuca wrote: > Does this look better as a colon? It feels more natural ("Required: 2"). If > you're using a dash, may as well use an em-dash ("Required — 2"). > > Also, does this need to be a string? Maybe it can just be a literal in the > source code. I don't think we will want to localize it. The dash was actually an idea from the UI team, but I agree it doesn't need to be localised. Moved out of generated_resources.grd. Em-dash looks a little strange, even though it's typographically correct: https://chromium.googlecode.com/issues/attachment?aid=3569850006000&name=Coll... En-dash actually looks really good, better than a regular dash: https://chromium.googlecode.com/issues/attachment?aid=3569850007000&name=Coll... I think this will all be finalised later in UI review anyway :) But for now I've made it an en-dash, I think it looks a bit more balanced. https://codereview.chromium.org/214163002/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:2256: + <message name="IDS_APPLICATION_INFO_NO_PERMISSIONS_TEXT" desc="Text displayed in the Permissions area of the dialog if the app has no permissions of any kind."> On 2014/04/16 05:39:16, Matt Giuca wrote: > "of the App Info dialog" Done. https://codereview.chromium.org/214163002/diff/40001/chrome/app/generated_res... chrome/app/generated_resources.grd:2257: + This app has no special permissions. On 2014/04/16 05:39:16, Matt Giuca wrote: > I'm not sure about the wording of this. Perhaps "This app requires no special > permissions." Sure, this will go through UI review anyway and the wording will be finalised there. https://codereview.chromium.org/214163002/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/214163002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:221: // case, format the title as 'number - Title' On 2014/04/16 05:39:16, Matt Giuca wrote: > It's good that you're thinking about this, but have you tested it with an RTL > language? I may be wrong, but as far as I can see, you shouldn't have to do > anything special for RTL. It should automatically display the string in the > "correct" order, and your efforts to deal with it may end up cancelling out. > > For example, Hebrew. Here I write the string "<Hebrew word for 'Required'> - 3": > > דרוש - 3 > > You can see that Chrome automatically places the 3 to the left of the hyphen to > the left of the text. > > We briefly discussed this offline and maybe Views won't handle it this way, > because they are actually three separate child views. But I'm still a bit > skeptical about manually dealing with RTL at this level, since it's really > something that should be dealt with by a text rendering system. Is there any way > to make a substring of a label bold? If not, why not just make the entire label > bold? Leaving this as it is, as discussed. RTL is magically handled by chrome. Mmmm magic. I could make the entire label bold, but again I'm basing these off some UI sketches that have the text as bold, and the rest as non-bold. In fact, in the mocks, the number text is a different colour too. So these will eventually have to be separate views anyway. https://codereview.chromium.org/214163002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:238: layout->StartRow(0, kSplitColumnSetId); On 2014/04/16 05:39:16, Matt Giuca wrote: > May be same as above. Done.
lgtm lgtm with maybe going back to a hyphen. https://codereview.chromium.org/214163002/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc (right): https://codereview.chromium.org/214163002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:211: title_view->AddChildView(new views::Label(base::UTF8ToUTF16("–"))); Can you just use a regular hyphen instead of an en-dash? If you do use an en-dash, I don't think it's appropriate to have Unicode characters in the source code (source should be ASCII). Use base::UTF8ToUTF16("\xe2\x80\x93") // En-dash. https://codereview.chromium.org/214163002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:213: new views::Label(base::IntToString16(permission_messages.size()))); Can you just concatenate the last two elements into a single string.
https://codereview.chromium.org/214163002/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc (right): https://codereview.chromium.org/214163002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:211: title_view->AddChildView(new views::Label(base::UTF8ToUTF16("–"))); On 2014/04/16 07:02:55, Matt Giuca wrote: > Can you just use a regular hyphen instead of an en-dash? > > If you do use an en-dash, I don't think it's appropriate to have Unicode > characters in the source code (source should be ASCII). Use > base::UTF8ToUTF16("\xe2\x80\x93") // En-dash. Looking at the screenshots on the bug, the en-dash makes for a better 'dash' (not too long, not too short) and matches the mocks closest. But good suggestion about the unicode source - I've done that now. https://codereview.chromium.org/214163002/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:213: new views::Label(base::IntToString16(permission_messages.size()))); On 2014/04/16 07:02:55, Matt Giuca wrote: > Can you just concatenate the last two elements into a single string. As discussed, the reason I've done it this way is to easily regulate the amount of space between the heading, the dash, and the number. If the UI feels too cluttered, you can just increase this spacing and it will spread it out. I feel as though we are treating the dash like an 'image' and so it belongs in a separate view - we might decide to change the symbol later, or internationalise it, or change it to an image, or so on. The alternative would be to put the dash and the number in a string like ' - 9', but this gives us less fine-grained control over the spacing around the dash (and a single space is quite small, so you would probably want more spaces, leading to a string like ' - 9'). It feels clearer to describe the spacing in terms of pixels, not space characters.
On 2014/04/16 23:26:06, sasha_b wrote: > https://codereview.chromium.org/214163002/diff/80001/chrome/browser/ui/views/... > File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc > (right): > > https://codereview.chromium.org/214163002/diff/80001/chrome/browser/ui/views/... > chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:211: > title_view->AddChildView(new views::Label(base::UTF8ToUTF16("–"))); > On 2014/04/16 07:02:55, Matt Giuca wrote: > > Can you just use a regular hyphen instead of an en-dash? > > > > If you do use an en-dash, I don't think it's appropriate to have Unicode > > characters in the source code (source should be ASCII). Use > > base::UTF8ToUTF16("\xe2\x80\x93") // En-dash. > > Looking at the screenshots on the bug, the en-dash makes for a better 'dash' > (not too long, not too short) and matches the mocks closest. But good suggestion > about the unicode source - I've done that now. > > https://codereview.chromium.org/214163002/diff/80001/chrome/browser/ui/views/... > chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:213: > new views::Label(base::IntToString16(permission_messages.size()))); > On 2014/04/16 07:02:55, Matt Giuca wrote: > > Can you just concatenate the last two elements into a single string. > > As discussed, the reason I've done it this way is to easily regulate the amount > of space between the heading, the dash, and the number. If the UI feels too > cluttered, you can just increase this spacing and it will spread it out. I feel > as though we are treating the dash like an 'image' and so it belongs in a > separate view - we might decide to change the symbol later, or internationalise > it, or change it to an image, or so on. > > The alternative would be to put the dash and the number in a string like ' - 9', > but this gives us less fine-grained control over the spacing around the dash > (and a single space is quite small, so you would probably want more spaces, > leading to a string like ' - 9'). It feels clearer to describe the spacing > in terms of pixels, not space characters. slgtm
benwells please review :)
I just skimmed and only have a couple of nits. Awesome CL. Any chance of getting some testing? In particular some testing to protect us from the extensions system changing would be nice. https://codereview.chromium.org/214163002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc (right): https://codereview.chromium.org/214163002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:70: void AnimateToState(double state); Nit: please name this AnimateToRatio. State to me implies open or closed. https://codereview.chromium.org/214163002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:97: // ExpandableContainerView::DetailsView ---------------------------------------- Nit: Please don't add comments like this, afaik they are not standard at all in chrome code.
I had some problems with 'git cl upload' so hopefully this uploaded correctly. But there are now tests! Yay. I also added a note to the FRIEND_TEST_ALL_PREFIXES pattern because it looks a little strange. Other than that it's mostly test boilerplate, please take a look and let me know what you think :) https://codereview.chromium.org/214163002/diff/100001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc (right): https://codereview.chromium.org/214163002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:70: void AnimateToState(double state); On 2014/04/17 01:45:50, benwells wrote: > Nit: please name this AnimateToRatio. State to me implies open or closed. Done, and added some other renames/comments. https://codereview.chromium.org/214163002/diff/100001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:97: // ExpandableContainerView::DetailsView ---------------------------------------- On 2014/04/17 01:45:50, benwells wrote: > Nit: Please don't add comments like this, afaik they are not standard at all in > chrome code. Copied from where I based this class off, but no worries. Done. https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.h (right): https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.h:34: friend class AppInfoPermissionsTabTest; This is a pretty standard convention; if you code search for 'FRIEND_TEST' you'll see the pattern. 'Git cl presubmit' warns you to use FRIEND_TEST_ALL_PREFIXES instead of FRIEND_TEST, so I've done that here even though I don't need the prefixing feature. I think it's better if the tests describe what they do - it doesn't make sense for them to be named for the method they test, since there's no 1:1 correspondence for these methods and tests.
https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc (right): https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:69: // Animates this to be a height proportional to |state|. Nit: this comment is out of date. https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:77: double state_; Nit: rename to visible_ratio_ https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.h (right): https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.h:11: class AppInfoPermissionsTabTest; You don't need this forward decl. https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.h:34: friend class AppInfoPermissionsTabTest; You don't need the AppInfoPermissionsTabTest friend, just the FRIEND_TEST_ALL_PREFIXES, as the AppInfoPermissionsTabTest base class doesn't need any special access. https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab_unittest.cc (right): https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab_unittest.cc:23: AppInfoPermissionsTabTest() : window(nullptr) {}; AFAIK we don't use nullptr in chrome code. I can't see it used anywhere, it is a c++11 feature. https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab_unittest.cc:77: EXPECT_FALSE(required_permissions->IsEmpty()); Can you also add EXPECT_TRUE(optional_permissions->IsEmpty()) https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab_unittest.cc:83: ASSERT_STREQ("Detect your physical location", Is there a constant you can use instead here? Otherwise these tests will break whenever these strings are changed. https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab_unittest.cc:110: Also EXPECT_TRUE(required->IsEMpty) here too. https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab_unittest.cc:153: base::FilePath path = base::FilePath(FILE_PATH_LITERAL(filename)); I don't think these three variables are used.
I've done all requested fixes. As discussed, I _could_ add a giant test that has required, optional _and_ retained file permissions, but this would just be a giant test that is a copy-and-paste combination of the existing ones - it would be confusing and hard to maintain. So I've left the tests as individual unit tests, which I think still provides good coverage. https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc (right): https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:69: // Animates this to be a height proportional to |state|. On 2014/04/23 00:36:22, benwells wrote: > Nit: this comment is out of date. Done. https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:77: double state_; On 2014/04/23 00:36:22, benwells wrote: > Nit: rename to visible_ratio_ Nice, done. https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.h (right): https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.h:11: class AppInfoPermissionsTabTest; On 2014/04/23 00:36:22, benwells wrote: > You don't need this forward decl. Thanks; done. https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.h:34: friend class AppInfoPermissionsTabTest; On 2014/04/23 00:36:22, benwells wrote: > You don't need the AppInfoPermissionsTabTest friend, just the > FRIEND_TEST_ALL_PREFIXES, as the AppInfoPermissionsTabTest base class doesn't > need any special access. Ahh yup, nice. Done. https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... File chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab_unittest.cc (right): https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab_unittest.cc:23: AppInfoPermissionsTabTest() : window(nullptr) {}; On 2014/04/23 00:36:22, benwells wrote: > AFAIK we don't use nullptr in chrome code. I can't see it used anywhere, it is a > c++11 feature. Removed. Not sure where I saw this. https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab_unittest.cc:77: EXPECT_FALSE(required_permissions->IsEmpty()); On 2014/04/23 00:36:22, benwells wrote: > Can you also add EXPECT_TRUE(optional_permissions->IsEmpty()) Good idea - also added retained files. Changed all tests to check all 3. https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab_unittest.cc:83: ASSERT_STREQ("Detect your physical location", On 2014/04/23 00:36:22, benwells wrote: > Is there a constant you can use instead here? Otherwise these tests will break > whenever these strings are changed. Changed to use the generated strings from the given locale, which is how the messages are being generated anyway. https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab_unittest.cc:110: On 2014/04/23 00:36:22, benwells wrote: > Also EXPECT_TRUE(required->IsEMpty) here too. Done. https://codereview.chromium.org/214163002/diff/140001/chrome/browser/ui/views... chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab_unittest.cc:153: base::FilePath path = base::FilePath(FILE_PATH_LITERAL(filename)); On 2014/04/23 00:36:22, benwells wrote: > I don't think these three variables are used. Oh, nice, yeah these are artifacts from the old way it worked. Removed.
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/214163002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on chromium_presubmit
The CQ bit was checked by sashab@chromium.org
The CQ bit was unchecked by sashab@chromium.org
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/214163002/160001
Message was sent while issue was closed.
Change committed as 266518
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/214163002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium
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/214163002/200001
Message was sent while issue was closed.
Change committed as 267161
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/214163002/220001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc
Hunk #1 FAILED at 4.
Hunk #2 FAILED at 83.
Hunk #3 FAILED at 95.
3 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc.rej
Patch:
chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc
Index: chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc
diff --git
a/chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc
b/chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc
index
e8d6d31b98057b7348963930df79204762210a71..bf5a7791f1bdb7d594734deae136f44e44b13a00
100644
--- a/chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc
+++ b/chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.cc
@@ -4,78 +4,131 @@
#include
"chrome/browser/ui/views/apps/app_info_dialog/app_info_permissions_tab.h"
+#include "apps/saved_files_service.h"
+#include "base/files/file_path.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/utf_string_conversions.h"
#include "extensions/common/extension.h"
+#include "extensions/common/permissions/api_permission.h"
#include "extensions/common/permissions/permission_message_provider.h"
-#include "extensions/common/permissions/permission_set.h"
+#include "extensions/common/permissions/permissions_data.h"
#include "grit/generated_resources.h"
+#include "grit/theme_resources.h"
#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/gfx/animation/animation.h"
+#include "ui/gfx/animation/animation_delegate.h"
+#include "ui/gfx/animation/slide_animation.h"
+#include "ui/gfx/text_constants.h"
+#include "ui/views/controls/button/button.h"
+#include "ui/views/controls/button/image_button.h"
#include "ui/views/controls/label.h"
#include "ui/views/controls/scroll_view.h"
-#include "ui/views/controls/scrollbar/overlay_scroll_bar.h"
+#include "ui/views/layout/box_layout.h"
+#include "ui/views/layout/fill_layout.h"
#include "ui/views/layout/grid_layout.h"
#include "ui/views/layout/layout_constants.h"
-// A scrollable list of permissions for the given app.
-class PermissionsScrollView : public views::ScrollView {
+namespace {
+
+// A view to display a title with an expandable permissions list section.
+class ExpandableContainerView : public views::View,
+ public views::ButtonListener,
+ public gfx::AnimationDelegate {
public:
- PermissionsScrollView(int min_height,
- int max_height,
- const extensions::Extension* app);
+ ExpandableContainerView(
+ views::View* owner,
+ const base::string16& title,
+ const std::vector<base::string16>& permission_messages);
+ virtual ~ExpandableContainerView();
+
+ // views::View:
+ virtual void ChildPreferredSizeChanged(views::View* child) OVERRIDE;
+
+ // views::ButtonListener:
+ virtual void ButtonPressed(views::Button* sender,
+ const ui::Event& event) OVERRIDE;
+
+ // gfx::AnimationDelegate:
+ virtual void AnimationProgressed(const gfx::Animation* animation) OVERRIDE;
+ virtual void AnimationEnded(const gfx::Animation* animation) OVERRIDE;
+
+ // Expand/Collapse the detail section for this ExpandableContainerView.
+ void ToggleDetailLevel();
private:
- virtual ~PermissionsScrollView();
-};
+ // A view which displays the permission messages as a bulleted list.
+ class DetailsView : public views::View {
+ public:
+ explicit DetailsView(std::vector<base::string16> messages);
+ virtual ~DetailsView() {}
-PermissionsScrollView::PermissionsScrollView(int min_height,
- int max_height,
- const extensions::Extension* app)
{
- ClipHeightTo(min_height, max_height);
- SetVerticalScrollBar(new views::OverlayScrollBar(false));
+ // views::View:
+ virtual gfx::Size GetPreferredSize() OVERRIDE;
- views::View* inner_scrollable_view = new views::View();
- this->SetContents(inner_scrollable_view);
+ // Animates this to be a height proportional to |ratio|.
+ void AnimateToRatio(double ratio);
- // Get the permission messages for the app.
- std::vector<base::string16> permission_messages =
- extensions::PermissionMessageProvider::Get()->GetWarningMessages(
- app->GetActivePermissions(), app->GetType());
+ private:
+ // The current state of the animation, as a decimal from 0 to 1 (0 is fully
+ // collapsed, 1 is fully expanded).
+ double visible_ratio_;
- // Create the layout.
- views::GridLayout* layout =
- views::GridLayout::CreatePanel(inner_scrollable_view);
- inner_scrollable_view->SetLayoutManager(layout);
+ DISALLOW_COPY_AND_ASSIGN(DetailsView);
+ };
+
+ // The dialog that owns |this|. It's also an ancestor in the View hierarchy.
+ views::View* owner_;
+
+ // A view for showing |permission_messages|.
+ DetailsView* details_view_;
+
+ gfx::SlideAnimation slide_animation_;
+
+ // The up/down arrow next to the heading (points up/down depending on whether
+ // the details section is expanded).
+ views::ImageButton* arrow_toggle_;
+
+ // Whether the details section is expanded.
+ bool expanded_;
+
+ DISALLOW_COPY_AND_ASSIGN(ExpandableContainerView);
+};
+
+ExpandableContainerView::DetailsView::DetailsView(
+ std::vector<base::string16> messages)
+ : visible_ratio_(0) {
+ views::GridLayout* layout = views::GridLayout::CreatePanel(this);
+ SetLayoutManager(layout);
// Create 2 columns: one for the bullet, one for the bullet text.
- static const int kPermissionBulletsColumnSetId = 1;
- views::ColumnSet* permission_bullets_column_set =
- layout->AddColumnSet(kPermissionBulletsColumnSetId);
- permission_bullets_column_set->AddPaddingColumn(0, 10);
- permission_bullets_column_set->AddColumn(views::GridLayout::LEADING,
- views::GridLayout::LEADING,
- 0,
- views::GridLayout::USE_PREF,
- 0, // no fixed width
- 0);
- permission_bullets_column_set->AddPaddingColumn(0, 5);
- permission_bullets_column_set->AddColumn(views::GridLayout::LEADING,
- views::GridLayout::LEADING,
- 0,
- views::GridLayout::USE_PREF,
- 0, // no fixed width
- 0);
+ static const int kColumnSet = 1;
+ views::ColumnSet* column_set = layout->AddColumnSet(kColumnSet);
+ column_set->AddPaddingColumn(0, 10);
+ column_set->AddColumn(views::GridLayout::LEADING,
+ views::GridLayout::LEADING,
+ 1,
+ views::GridLayout::USE_PREF,
+ 0,
+ 0);
+ column_set->AddPaddingColumn(0, 5);
+ column_set->AddColumn(views::GridLayout::LEADING,
+ views::GridLayout::LEADING,
+ 1,
+ views::GridLayout::USE_PREF,
+ 0,
+ 0);
// Add permissions to scrollable view.
- for (std::vector<base::string16>::const_iterator it =
- permission_messages.begin();
- it != permission_messages.end();
+ for (std::vector<base::string16>::const_iterator it = messages.begin();
+ it != messages.end();
++it) {
views::Label* permission_label = new views::Label(*it);
permission_label->SetMultiLine(true);
permission_label->SetHorizontalAlignment(gfx::ALIGN_LEFT);
- permission_label->SizeToFit(250);
- layout->StartRow(0, kPermissionBulletsColumnSetId);
+ layout->StartRow(0, kColumnSet);
// Extract only the bullet from the IDS_EXTENSION_PERMISSION_LINE text.
layout->AddView(new views::Label(l10n_util::GetStringFUTF16(
IDS_EXTENSION_PERMISSION_LINE, base::string16())));
@@ -83,11 +136,139 @@ PermissionsScrollView::PermissionsScrollView(int
min_height,
// bullet.
layout->AddView(permission_label);
- layout->AddPaddingRow(0, views::kRelatedControlVerticalSpacing);
+ layout->AddPaddingRow(0, views::kRelatedControlSmallVerticalSpacing);
+ }
+}
+
+gfx::Size ExpandableContainerView::DetailsView::GetPreferredSize() {
+ gfx::Size size = views::View::GetPreferredSize();
+ return gfx::Size(size.width(), size.height() * visible_ratio_);
+}
+
+void ExpandableContainerView::DetailsView::AnimateToRatio(double ratio) {
+ visible_ratio_ = ratio;
+ PreferredSizeChanged();
+ SchedulePaint();
+}
+
+ExpandableContainerView::ExpandableContainerView(
+ views::View* owner,
+ const base::string16& title,
+ const std::vector<base::string16>& permission_messages)
+ : owner_(owner),
+ details_view_(NULL),
+ slide_animation_(this),
+ arrow_toggle_(NULL),
+ expanded_(false) {
+ views::GridLayout* layout = new views::GridLayout(this);
+ SetLayoutManager(layout);
+ const int kMainColumnSetId = 0;
+ views::ColumnSet* column_set = layout->AddColumnSet(kMainColumnSetId);
+ column_set->AddColumn(views::GridLayout::LEADING,
+ views::GridLayout::LEADING,
+ 1,
+ views::GridLayout::USE_PREF,
+ 0,
+ 0);
+
+ // A column set that is split in half, to allow for the expand/collapse
button
+ // image to be aligned to the right of the vi…
(message too large)
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
