Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
55533003:
Add extension permissions for new settings override API. (Closed)
|
Created:
7 years, 1 month ago by MAD Modified:
7 years, 1 month ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd extension permissions for new settings override API.
Needed to create new class, mimicking SimpleAPIPermission, as opposed to inheriting from SetDisjunctionPermission, since we just needed to construct strings dynamically with extension properties.
BUG=306128
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233394
Patch Set 1 : #Patch Set 2 : Trimmed schema/www. and only show one start page. #
Total comments: 34
Patch Set 3 : CR comments 1 #
Total comments: 5
Patch Set 4 : Sync/Rebase to ToT #Patch Set 5 : CR comments 2. #Patch Set 6 : Sync/Rebase to ToT and removed unneeded explicit. #
Total comments: 4
Patch Set 7 : OWNER CR comments addressed. #Patch Set 8 : Added tests. #
Total comments: 4
Patch Set 9 : LGTM CR Comments addressed. #Patch Set 10 : Added missing exclusion in gyp file. #Messages
Total messages: 27 (0 generated)
Can you take a look before I ask for OWNERS approval? Here's the doc describing this feature: https://docs.google.com/a/google.com/document/d/1MOvYs220EuIuluzMzL7mMlS4Ad6a... Thanks! BYE MAD
lg, comments/suggestions below. FWIW, I was not familiar at all with this section of the code, but I familiarized myself with it and dumped my questions/thoughts below :)! Have a great Halloween weekend! Gab https://codereview.chromium.org/55533003/diff/70001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/55533003/diff/70001/chrome/app/generated_reso... chrome/app/generated_resources.grd:4378: + Change your home page to: <ph name="HOME_PAGE">$1<ex>home.page.com/home.html</ex></ph> Should this change go in NOW?! Before the string freeze? https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... File chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc (right): https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:64: // see the scheme, and also remove www. if present. I would quote the "www." here; otherwise the '.' feels like the end of the sentence; had to read this twice.. What about: // Returns a string representing |url| in the UI for a permission // confirmation. This is the |url| without its scheme and "www.". https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:67: std::string result(url.GetContent()); static const char kWwwPrefix[] = "www."; and use that constant and arraysize(kWwwPrefix) instead of hardcoding 4 below. https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:106: PermissionsData::GetInitialAPIPermissions(extension); Feels kind of weird to use a static call to get data that is already in the local |extension| variable... but I guess this is what PermissionsData::GetInitialAPIPermissions() is meant for (as this is all it does...). https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:106: PermissionsData::GetInitialAPIPermissions(extension); This method states this is only non-NULL during extension initialization; a DCHECK here to make sure would be nice? https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:110: CreateManifestURL(info->search_engine->search_url)-> Why doesn't this use GetURLDisplayText()? https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... File chrome/common/extensions/permissions/chrome_api_permissions.cc (right): https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/chrome_api_permissions.cc:354: APIPermissionInfo::kFlagInternal, Why internal? Is that how we make sure extensions with other permissions can't add those? If so, how does an extension actually specify these prefs? Is it the logic in settings_overrides_handler.cc? Perhaps a comment here saying what takes care of handling the internal logic makes sense? https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/chrome_api_permissions.cc:362: { APIPermission::kStartupPages, "startupPages", Other comments suggest this can only be 1 page; so should this be "startupPage" instead? https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... File chrome/common/extensions/permissions/settings_override_permission.cc (right): https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/settings_override_permission.cc:29: PermissionMessages result; Move this line to line 47 since it's not needed before that. https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/settings_override_permission.cc:32: case(kHomePage): { Typical style for case statements is: case foo: ... i.e., () are unnecessary https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/settings_override_permission.cc:53: bool SettingsOverrideAPIPermission::Check( I'm guessing you based most of this implementation on SimpleAPIPermission in api_permission.cc? Probably worth mentioning in the CL description for future reviewers (although they'll probably be more familiar with this than I...). https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/settings_override_permission.cc:73: // added when settigns overrides are used. s/settigns/settings https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... File chrome/common/extensions/permissions/settings_override_permission.h (right): https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/settings_override_permission.h:14: // This class takes care of creating custom permission messages for extensions s/This class takes care.../Takes care... https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/settings_override_permission.h:15: // that overrides settings. s/overrides/override (since extensions is plural) https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/settings_override_permission.h:18: explicit SettingsOverrideAPIPermission( -explicit https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/settings_override_permission.h:19: const APIPermissionInfo* permission, const std::string setting_value); s/const std::string/const std::string& https://codereview.chromium.org/55533003/diff/70001/extensions/common/permiss... File extensions/common/permissions/api_permission.h (right): https://codereview.chromium.org/55533003/diff/70001/extensions/common/permiss... extensions/common/permissions/api_permission.h:131: kStartupPages, Should we group these new permissions together in this list since they're "special"?
Thanks a lot! All addressed/answered... Anything else? BYE MAD https://codereview.chromium.org/55533003/diff/70001/chrome/app/generated_reso... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/55533003/diff/70001/chrome/app/generated_reso... chrome/app/generated_resources.grd:4378: + Change your home page to: <ph name="HOME_PAGE">$1<ex>home.page.com/home.html</ex></ph> On 2013/11/01 22:35:40, gab wrote: > Should this change go in NOW?! Before the string freeze? Maybe... Although it's not critical, it doesn't change the behavior, it's just a hint for the translators. https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... File chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc (right): https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:64: // see the scheme, and also remove www. if present. On 2013/11/01 22:35:40, gab wrote: > I would quote the "www." here; otherwise the '.' feels like the end of the > sentence; had to read this twice.. > > What about: > > // Returns a string representing |url| in the UI for a permission > // confirmation. This is the |url| without its scheme and "www.". Done. https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:67: std::string result(url.GetContent()); On 2013/11/01 22:35:40, gab wrote: > static const char kWwwPrefix[] = "www."; > > and use that constant and arraysize(kWwwPrefix) instead of hardcoding 4 below. Done. https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:106: PermissionsData::GetInitialAPIPermissions(extension); On 2013/11/01 22:35:40, gab wrote: > This method states this is only non-NULL during extension initialization; a > DCHECK here to make sure would be nice? Done. https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:106: PermissionsData::GetInitialAPIPermissions(extension); On 2013/11/01 22:35:40, gab wrote: > Feels kind of weird to use a static call to get data that is already in the > local |extension| variable... but I guess this is what > PermissionsData::GetInitialAPIPermissions() is meant for (as this is all it > does...). It's because it needs to go through private data... https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:110: CreateManifestURL(info->search_engine->search_url)-> On 2013/11/01 22:35:40, gab wrote: > Why doesn't this use GetURLDisplayText()? This was because it doesn't have the same needs, since we don't want to get the whole content, just the origin host. Anyway, this changed now... https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... File chrome/common/extensions/permissions/chrome_api_permissions.cc (right): https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/chrome_api_permissions.cc:354: APIPermissionInfo::kFlagInternal, This is what "internal" actually means, it means we insert these permissions internally, as opposed to read them from the manifest. https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/chrome_api_permissions.cc:362: { APIPermission::kStartupPages, "startupPages", On 2013/11/01 22:35:40, gab wrote: > Other comments suggest this can only be 1 page; so should this be "startupPage" > instead? Yes, but the manifest format calls this property startup_pages and have a list type, so I prefer to keep it like this https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... File chrome/common/extensions/permissions/settings_override_permission.cc (right): https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/settings_override_permission.cc:29: PermissionMessages result; On 2013/11/01 22:35:40, gab wrote: > Move this line to line 47 since it's not needed before that. Done. https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/settings_override_permission.cc:32: case(kHomePage): { On 2013/11/01 22:35:40, gab wrote: > Typical style for case statements is: > > case foo: ... > > i.e., () are unnecessary Done. https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/settings_override_permission.cc:73: // added when settigns overrides are used. On 2013/11/01 22:35:40, gab wrote: > s/settigns/settings Done. https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... File chrome/common/extensions/permissions/settings_override_permission.h (right): https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/settings_override_permission.h:14: // This class takes care of creating custom permission messages for extensions On 2013/11/01 22:35:40, gab wrote: > s/This class takes care.../Takes care... Done. https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/settings_override_permission.h:15: // that overrides settings. On 2013/11/01 22:35:40, gab wrote: > s/overrides/override (since extensions is plural) Done. https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/settings_override_permission.h:18: explicit SettingsOverrideAPIPermission( On 2013/11/01 22:35:40, gab wrote: > -explicit Not needed when there is more than one argument. https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/settings_override_permission.h:19: const APIPermissionInfo* permission, const std::string setting_value); On 2013/11/01 22:35:40, gab wrote: > s/const std::string/const std::string& Done. https://codereview.chromium.org/55533003/diff/70001/extensions/common/permiss... File extensions/common/permissions/api_permission.h (right): https://codereview.chromium.org/55533003/diff/70001/extensions/common/permiss... extensions/common/permissions/api_permission.h:131: kStartupPages, On 2013/11/01 22:35:40, gab wrote: > Should we group these new permissions together in this list since they're > "special"? There are other speciaL permissions, and they are all sorted.
Drive by comments, overall looks great. https://codereview.chromium.org/55533003/diff/150001/chrome/common/extensions... File chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc (right): https://codereview.chromium.org/55533003/diff/150001/chrome/common/extensions... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:109: RemoveWwwPrefix(CreateManifestURL(info->search_engine->search_url)-> Please add a comment or a note in the changelist description explaining why the www. prefix is removed here (and not any other prefix, euch as say http://search.yahoo.com/). https://codereview.chromium.org/55533003/diff/150001/chrome/common/extensions... File chrome/common/extensions/permissions/settings_override_permission.cc (right): https://codereview.chromium.org/55533003/diff/150001/chrome/common/extensions... chrome/common/extensions/permissions/settings_override_permission.cc:59: CHECK(info() == rhs->info()); I see other implementers of APIPermission CHECK on unexpected params, though I couldn't tell why this was a good idea (it wasn't spelled out in api_permission.h). If you've encountered the reasoning, please could you add an explanatory comment.
Thanks... Added a comment for the prefix, but I don't know enough about your other request. https://codereview.chromium.org/55533003/diff/150001/chrome/common/extensions... File chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc (right): https://codereview.chromium.org/55533003/diff/150001/chrome/common/extensions... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:109: RemoveWwwPrefix(CreateManifestURL(info->search_engine->search_url)-> On 2013/11/04 01:11:13, robertshield wrote: > Please add a comment or a note in the changelist description explaining why the > www. prefix is removed here (and not any other prefix, euch as say > http://search.yahoo.com/). Done. https://codereview.chromium.org/55533003/diff/150001/chrome/common/extensions... File chrome/common/extensions/permissions/settings_override_permission.cc (right): https://codereview.chromium.org/55533003/diff/150001/chrome/common/extensions... chrome/common/extensions/permissions/settings_override_permission.cc:59: CHECK(info() == rhs->info()); On 2013/11/04 01:11:13, robertshield wrote: > I see other implementers of APIPermission CHECK on unexpected params, though I > couldn't tell why this was a good idea (it wasn't spelled out in > api_permission.h). If you've encountered the reasoning, please could you add an > explanatory comment. I honestly don't know. I can only guess that these calls are only allowed across compatible permissions, i.e., with the same info. But it's only a guess, I simply blindly copied the code from SimpleAPIPermission. I could ask the OWNERS when the CR gets to that level.
https://codereview.chromium.org/55533003/diff/150001/chrome/common/extensions... File chrome/common/extensions/permissions/settings_override_permission.cc (right): https://codereview.chromium.org/55533003/diff/150001/chrome/common/extensions... chrome/common/extensions/permissions/settings_override_permission.cc:59: CHECK(info() == rhs->info()); On 2013/11/04 02:06:34, MAD wrote: > On 2013/11/04 01:11:13, robertshield wrote: > > I see other implementers of APIPermission CHECK on unexpected params, though I > > couldn't tell why this was a good idea (it wasn't spelled out in > > api_permission.h). If you've encountered the reasoning, please could you add > an > > explanatory comment. > I honestly don't know. I can only guess that these calls are only allowed across > compatible permissions, i.e., with the same info. But it's only a guess, I > simply blindly copied the code from SimpleAPIPermission. I would do the same :) > > I could ask the OWNERS when the CR gets to that level. Sgtm. I was mainly curious.
Adding yoz@ for OWNERS review, and a question from one of the reviewers: Why are we CHECKing that info() is equal between this and rhs in the APIPermission methods? Also, I will take a closer look at all try both failures... Thanks! BYE MAD
https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... File chrome/common/extensions/permissions/settings_override_permission.h (right): https://codereview.chromium.org/55533003/diff/70001/chrome/common/extensions/... chrome/common/extensions/permissions/settings_override_permission.h:18: explicit SettingsOverrideAPIPermission( On 2013/11/03 02:21:20, MAD wrote: > On 2013/11/01 22:35:40, gab wrote: > > -explicit > > Not needed when there is more than one argument. Right, that's why I was saying to remove it...
D'Ho! I thought you wanted me to add explicit... :-) Will remove when I'm back at my desk... BYE MAD... sent from Mobile Answering Device! Le 4 nov. 2013 13:05, <gab@chromium.org> a écrit : > > https://codereview.chromium.org/55533003/diff/70001/ > chrome/common/extensions/permissions/settings_override_permission.h > File chrome/common/extensions/permissions/settings_override_permission.h > (right): > > https://codereview.chromium.org/55533003/diff/70001/ > chrome/common/extensions/permissions/settings_override_ > permission.h#newcode18 > chrome/common/extensions/permissions/settings_override_permission.h:18: > explicit SettingsOverrideAPIPermission( > On 2013/11/03 02:21:20, MAD wrote: > >> On 2013/11/01 22:35:40, gab wrote: >> > -explicit >> > > Not needed when there is more than one argument. >> > > Right, that's why I was saying to remove it... > > https://codereview.chromium.org/55533003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Done, removed explicit and synced/rebased again... Thanks! BYE MAD
Are there any tests for this? It would help to see an example of how this appears in a manifest.
The code to load this from a manifest is already in (although, I heard the last CL to exercise it completely has been reverted). There is an example extension here: http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/s... A few previous CLs related to this: http://src.chromium.org/viewvc/chrome?view=revision&revision=232132 http://src.chromium.org/viewvc/chrome?view=revision&revision=231001 http://src.chromium.org/viewvc/chrome?view=revision&revision=230419 On Mon, Nov 4, 2013 at 3:33 PM, <yoz@chromium.org> wrote: > Are there any tests for this? It would help to see an example of how this > appears in a manifest. > > https://codereview.chromium.org/55533003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
By a test, I mean you can add a unit test that checks that an extension with the appropriate manifest fields has the desired permission warnings. https://codereview.chromium.org/55533003/diff/420001/chrome/common/extensions... File chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc (right): https://codereview.chromium.org/55533003/diff/420001/chrome/common/extensions... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:117: // We only support one startup page. Is this "we only support warning for one startup page"? https://codereview.chromium.org/55533003/diff/420001/chrome/common/extensions... File chrome/common/extensions/permissions/settings_override_permission.cc (right): https://codereview.chromium.org/55533003/diff/420001/chrome/common/extensions... chrome/common/extensions/permissions/settings_override_permission.cc:74: NOTREACHED(); Are these permissions stored in the prefs? If not, how do we remember that the user accepted them the next time Chrome starts? I don't think these can be left unimplemented.
Thanks for the comments, some answers/comments/changes below... As for the tests, I'm adding one inspired by other internal permissions that are created dynamically... An updated patch is coming up... BYE MAD https://codereview.chromium.org/55533003/diff/420001/chrome/common/extensions... File chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc (right): https://codereview.chromium.org/55533003/diff/420001/chrome/common/extensions... chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:117: // We only support one startup page. On 2013/11/04 22:15:50, Yoyo Zhou wrote: > Is this "we only support warning for one startup page"? No, we only support one startup page in the manifest parsing code. https://codereview.chromium.org/55533003/diff/420001/chrome/common/extensions... File chrome/common/extensions/permissions/settings_override_permission.cc (right): https://codereview.chromium.org/55533003/diff/420001/chrome/common/extensions... chrome/common/extensions/permissions/settings_override_permission.cc:74: NOTREACHED(); On 2013/11/04 22:15:50, Yoyo Zhou wrote: > Are these permissions stored in the prefs? If not, how do we remember that the > user accepted them the next time Chrome starts? > > I don't think these can be left unimplemented. No, they are not. These permissions are created dynamically when we parse the manifest and find some declarations. once the extension is installed and enabled, we don't prompt anymore, unless user clicks on the permissions link in the extensions webui... I don't know these are persisted though. Could it be that these are permissions that are only used when extension is installed? We don't get prompt again when we restart Chrome... I could use the same implementation as SimplePIPermission if you want (e.g., if (value) return false; return true;).
On 2013/11/05 01:02:05, MAD wrote: > Thanks for the comments, some answers/comments/changes below... > > As for the tests, I'm adding one inspired by other internal permissions that are > created dynamically... An updated patch is coming up... > > BYE > MAD > > https://codereview.chromium.org/55533003/diff/420001/chrome/common/extensions... > File chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc > (right): > > https://codereview.chromium.org/55533003/diff/420001/chrome/common/extensions... > chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc:117: // > We only support one startup page. > On 2013/11/04 22:15:50, Yoyo Zhou wrote: > > Is this "we only support warning for one startup page"? > > No, we only support one startup page in the manifest parsing code. > > https://codereview.chromium.org/55533003/diff/420001/chrome/common/extensions... > File chrome/common/extensions/permissions/settings_override_permission.cc > (right): > > https://codereview.chromium.org/55533003/diff/420001/chrome/common/extensions... > chrome/common/extensions/permissions/settings_override_permission.cc:74: > NOTREACHED(); > On 2013/11/04 22:15:50, Yoyo Zhou wrote: > > Are these permissions stored in the prefs? If not, how do we remember that the > > user accepted them the next time Chrome starts? > > > > I don't think these can be left unimplemented. > > No, they are not. These permissions are created dynamically when we parse the > manifest and find some declarations. once the extension is installed and > enabled, we don't prompt anymore, unless user clicks on the permissions link in > the extensions webui... I don't know these are persisted though. Could it be > that these are permissions that are only used when extension is installed? We > don't get prompt again when we restart Chrome... > > I could use the same implementation as SimplePIPermission if you want (e.g., if > (value) return false; return true;). Sorry, I think you're right - these are only warned at install time. Can you see what happens if the extension is upgraded to a newer version? Do the same warnings appear again?
If dragging a new version of a locally packed crx with a new version number has the same effect as a server upgrade, the permissions confirmation only show up if we add new permissions... I even tried if I start with all the permissioned properties, then remove one, update with a new version number, and then add back the property that was removed, and the confirmation dialog didn't show up... For the test, I'm having some trouble. I use ExtensionManifestTest::LoadAndExpectSuccess() and then PermissionsData::GetInitialAPIPermissions() fails because initial_required_permissions_ is not set... What am I missing? On Mon, Nov 4, 2013 at 8:55 PM, <yoz@chromium.org> wrote: > On 2013/11/05 01:02:05, MAD wrote: > >> Thanks for the comments, some answers/comments/changes below... >> > > As for the tests, I'm adding one inspired by other internal permissions >> that >> > are > >> created dynamically... An updated patch is coming up... >> > > BYE >> MAD >> > > > https://codereview.chromium.org/55533003/diff/420001/ > chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc > >> File chrome/common/extensions/manifest_handlers/settings_ >> overrides_handler.cc >> (right): >> > > > https://codereview.chromium.org/55533003/diff/420001/ > chrome/common/extensions/manifest_handlers/settings_overrides_handler.cc# > newcode117 > >> chrome/common/extensions/manifest_handlers/settings_ >> overrides_handler.cc:117: >> > // > >> We only support one startup page. >> On 2013/11/04 22:15:50, Yoyo Zhou wrote: >> > Is this "we only support warning for one startup page"? >> > > No, we only support one startup page in the manifest parsing code. >> > > > https://codereview.chromium.org/55533003/diff/420001/ > chrome/common/extensions/permissions/settings_override_permission.cc > >> File chrome/common/extensions/permissions/settings_override_permission.cc >> (right): >> > > > https://codereview.chromium.org/55533003/diff/420001/ > chrome/common/extensions/permissions/settings_override_ > permission.cc#newcode74 > >> chrome/common/extensions/permissions/settings_override_permission.cc:74: >> NOTREACHED(); >> On 2013/11/04 22:15:50, Yoyo Zhou wrote: >> > Are these permissions stored in the prefs? If not, how do we remember >> that >> > the > >> > user accepted them the next time Chrome starts? >> > >> > I don't think these can be left unimplemented. >> > > No, they are not. These permissions are created dynamically when we parse >> the >> manifest and find some declarations. once the extension is installed and >> enabled, we don't prompt anymore, unless user clicks on the permissions >> link >> > in > >> the extensions webui... I don't know these are persisted though. Could it >> be >> that these are permissions that are only used when extension is >> installed? We >> don't get prompt again when we restart Chrome... >> > > I could use the same implementation as SimplePIPermission if you want >> (e.g., >> > if > >> (value) return false; return true;). >> > > Sorry, I think you're right - these are only warned at install time. Can > you see > what happens if the extension is upgraded to a newer version? Do the same > warnings appear again? > > https://codereview.chromium.org/55533003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/11/05 02:30:14, MAD wrote: > If dragging a new version of a locally packed crx with a new version number > has the same effect as a server upgrade, the permissions confirmation only > show up if we add new permissions... > > I even tried if I start with all the permissioned properties, then remove > one, update with a new version number, and then add back the property that > was removed, and the confirmation dialog didn't show up... Ok, I just verified that this would show new permission warnings if there were any, so that should be fine. > For the test, I'm having some trouble. I > use ExtensionManifestTest::LoadAndExpectSuccess() and > then PermissionsData::GetInitialAPIPermissions() fails > because initial_required_permissions_ is not set... What am I missing? I'm not sure because I can't see where it's failing. But it might be better to look at PermissionsTest (permission_set_unittest.cc) for a model; those tests are fairly simple and test what you want to test.
Got it... Succeeded in getting one test to run, I need to write a few more, but it's getting late here in EST, so I'll finish this tomorrow morning... Thanks! On Mon, Nov 4, 2013 at 9:40 PM, <yoz@chromium.org> wrote: > On 2013/11/05 02:30:14, MAD wrote: > >> If dragging a new version of a locally packed crx with a new version >> number >> has the same effect as a server upgrade, the permissions confirmation only >> show up if we add new permissions... >> > > I even tried if I start with all the permissioned properties, then remove >> one, update with a new version number, and then add back the property that >> was removed, and the confirmation dialog didn't show up... >> > > Ok, I just verified that this would show new permission warnings if there > were > any, so that should be fine. > > > For the test, I'm having some trouble. I >> use ExtensionManifestTest::LoadAndExpectSuccess() and >> then PermissionsData::GetInitialAPIPermissions() fails >> because initial_required_permissions_ is not set... What am I missing? >> > > I'm not sure because I can't see where it's failing. But it might be > better to > look at PermissionsTest (permission_set_unittest.cc) for a model; those > tests > are fairly simple and test what you want to test. > > https://codereview.chromium.org/55533003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
OK, tests were added, and some constants were renamed to fit with the new names in the manifest (I had not noticed that change earlier)... Let me know if you want other changes... Thanks! BYE MAD
LGTM https://codereview.chromium.org/55533003/diff/760001/chrome/common/extensions... File chrome/common/extensions/permissions/chrome_api_permissions.cc (right): https://codereview.chromium.org/55533003/diff/760001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_api_permissions.cc:352: { APIPermission::kHomePage, "homepage", I suspect this will be confusing if it's not capitalized as kHomepage. https://codereview.chromium.org/55533003/diff/760001/chrome/common/extensions... File chrome/common/extensions/permissions/settings_override_permission_unittest.cc (right): https://codereview.chromium.org/55533003/diff/760001/chrome/common/extensions... chrome/common/extensions/permissions/settings_override_permission_unittest.cc:68: EXPECT_TRUE(permission_set->HasAPIPermission(APIPermission::kHomePage)); Consider testing the contents of GetWarningMessages to make sure they contain the correct custom messages.
Cool, thanks... Addressed and about to CQ... BYE MAD https://codereview.chromium.org/55533003/diff/760001/chrome/common/extensions... File chrome/common/extensions/permissions/chrome_api_permissions.cc (right): https://codereview.chromium.org/55533003/diff/760001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_api_permissions.cc:352: { APIPermission::kHomePage, "homepage", On 2013/11/05 21:11:06, Yoyo Zhou wrote: > I suspect this will be confusing if it's not capitalized as kHomepage. Done. https://codereview.chromium.org/55533003/diff/760001/chrome/common/extensions... File chrome/common/extensions/permissions/settings_override_permission_unittest.cc (right): https://codereview.chromium.org/55533003/diff/760001/chrome/common/extensions... chrome/common/extensions/permissions/settings_override_permission_unittest.cc:68: EXPECT_TRUE(permission_set->HasAPIPermission(APIPermission::kHomePage)); On 2013/11/05 21:11:06, Yoyo Zhou wrote: > Consider testing the contents of GetWarningMessages to make sure they contain > the correct custom messages. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/55533003/860001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_clang_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/55533003/1100002
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mad@chromium.org/55533003/1100002
Message was sent while issue was closed.
Change committed as 233394 |
