Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
236113002:
Suppress tabs permission warning if there is already a browsingHistory warning. (Closed)
|
Created:
6 years, 8 months ago by meacer Modified:
6 years, 8 months ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionSuppress tabs permission warning if there is already a browsingHistory warning.
BUG=360148
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=263972
Patch Set 1 #
Total comments: 5
Patch Set 2 : Fix comments #Patch Set 3 : . #
Total comments: 2
Patch Set 4 : Refactor #
Total comments: 3
Patch Set 5 : Fix tests #
Messages
Total messages: 17 (0 generated)
Devlin, can you take a look? Thanks.
https://codereview.chromium.org/236113002/diff/1/chrome/common/extensions/per... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/236113002/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:122: PermissionMessages::iterator override_bookmarks_ui = This code can be deleted now, yes? (It looks like it's doing the same thing as the call on line 130.) https://codereview.chromium.org/236113002/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:134: PermissionMessage::kBrowsingHistory, I think I'd rather have BrowsingHistory suppressed. I think "Access your tabs and browsing activity" is scarier than "Read and modify your browsing history", and we should always show the scariest. :) https://codereview.chromium.org/236113002/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:267: std::set<PermissionMessage>::iterator write_directory_message = More similar hacks that could use your function.
https://codereview.chromium.org/236113002/diff/1/chrome/common/extensions/per... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/236113002/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:122: PermissionMessages::iterator override_bookmarks_ui = On 2014/04/11 23:18:56, D Cronin wrote: > This code can be deleted now, yes? (It looks like it's doing the same thing as > the call on line 130.) Oh, looks like you caught this one while I was reviewing. Nevermind.
On 2014/04/11 23:19:58, D Cronin wrote: > https://codereview.chromium.org/236113002/diff/1/chrome/common/extensions/per... > File chrome/common/extensions/permissions/chrome_permission_message_provider.cc > (right): > > https://codereview.chromium.org/236113002/diff/1/chrome/common/extensions/per... > chrome/common/extensions/permissions/chrome_permission_message_provider.cc:122: > PermissionMessages::iterator override_bookmarks_ui = > On 2014/04/11 23:18:56, D Cronin wrote: > > This code can be deleted now, yes? (It looks like it's doing the same thing as > > the call on line 130.) > > Oh, looks like you caught this one while I was reviewing. Nevermind. Right, I hit send a bit too early :)
https://codereview.chromium.org/236113002/diff/1/chrome/common/extensions/per... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/236113002/diff/1/chrome/common/extensions/per... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:134: PermissionMessage::kBrowsingHistory, On 2014/04/11 23:18:56, D Cronin wrote: > I think I'd rather have BrowsingHistory suppressed. I think "Access your tabs > and browsing activity" is scarier than "Read and modify your browsing history", > and we should always show the scariest. :) I actually think the scary one is browsing history because it encapsulates current browsing activity. "History" is also more familiar to users than "activity", so I'd like to keep the word history.
On 2014/04/11 23:49:53, Mustafa Emre Acer wrote: > https://codereview.chromium.org/236113002/diff/1/chrome/common/extensions/per... > File chrome/common/extensions/permissions/chrome_permission_message_provider.cc > (right): > > https://codereview.chromium.org/236113002/diff/1/chrome/common/extensions/per... > chrome/common/extensions/permissions/chrome_permission_message_provider.cc:134: > PermissionMessage::kBrowsingHistory, > On 2014/04/11 23:18:56, D Cronin wrote: > > I think I'd rather have BrowsingHistory suppressed. I think "Access your tabs > > and browsing activity" is scarier than "Read and modify your browsing > history", > > and we should always show the scariest. :) > > I actually think the scary one is browsing history because it encapsulates > current browsing activity. "History" is also more familiar to users than > "activity", so I'd like to keep the word history. Hmm... my hang up is that "Read and modify your browsing history" doesn't tell me that it can do anything that would actively interfere with my current session, like reorder tabs, redirect tabs, close tabs visiting certain urls, etc. George Santayana quotes aside, my impression as a user is that history is in the past, and can't affect what's going on now. But maybe I'm in the minority?
On 2014/04/12 00:01:04, D Cronin wrote: > On 2014/04/11 23:49:53, Mustafa Emre Acer wrote: > > > https://codereview.chromium.org/236113002/diff/1/chrome/common/extensions/per... > > File > chrome/common/extensions/permissions/chrome_permission_message_provider.cc > > (right): > > > > > https://codereview.chromium.org/236113002/diff/1/chrome/common/extensions/per... > > > chrome/common/extensions/permissions/chrome_permission_message_provider.cc:134: > > PermissionMessage::kBrowsingHistory, > > On 2014/04/11 23:18:56, D Cronin wrote: > > > I think I'd rather have BrowsingHistory suppressed. I think "Access your > tabs > > > and browsing activity" is scarier than "Read and modify your browsing > > history", > > > and we should always show the scariest. :) > > > > I actually think the scary one is browsing history because it encapsulates > > current browsing activity. "History" is also more familiar to users than > > "activity", so I'd like to keep the word history. > > Hmm... my hang up is that "Read and modify your browsing history" doesn't tell > me that it can do anything that would actively interfere with my current > session, like reorder tabs, redirect tabs, close tabs visiting certain urls, > etc. George Santayana quotes aside, my impression as a user is that history is > in the past, and can't affect what's going on now. But maybe I'm in the > minority? Actually tabs permission only gives additional access to urls and favicons. Extensions can already reorder, open or close tabs without tabs permission. So in a way it acts like history permission (list of urls you visited or are currently visiting).
On 2014/04/12 00:15:17, Mustafa Emre Acer wrote: > On 2014/04/12 00:01:04, D Cronin wrote: > > On 2014/04/11 23:49:53, Mustafa Emre Acer wrote: > > > > > > https://codereview.chromium.org/236113002/diff/1/chrome/common/extensions/per... > > > File > > chrome/common/extensions/permissions/chrome_permission_message_provider.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/236113002/diff/1/chrome/common/extensions/per... > > > > > > chrome/common/extensions/permissions/chrome_permission_message_provider.cc:134: > > > PermissionMessage::kBrowsingHistory, > > > On 2014/04/11 23:18:56, D Cronin wrote: > > > > I think I'd rather have BrowsingHistory suppressed. I think "Access your > > tabs > > > > and browsing activity" is scarier than "Read and modify your browsing > > > history", > > > > and we should always show the scariest. :) > > > > > > I actually think the scary one is browsing history because it encapsulates > > > current browsing activity. "History" is also more familiar to users than > > > "activity", so I'd like to keep the word history. > > > > Hmm... my hang up is that "Read and modify your browsing history" doesn't tell > > me that it can do anything that would actively interfere with my current > > session, like reorder tabs, redirect tabs, close tabs visiting certain urls, > > etc. George Santayana quotes aside, my impression as a user is that history > is > > in the past, and can't affect what's going on now. But maybe I'm in the > > minority? > > Actually tabs permission only gives additional access to urls and favicons. > Extensions can already reorder, open or close tabs without tabs permission. So > in a way it acts like history permission (list of urls you visited or are > currently visiting). Ah, right you are. Last I had checked, only things like create tab were allowed without permissions. In that case, just using the history warning sgtm.
https://codereview.chromium.org/236113002/diff/20001/chrome/common/extensions... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/236113002/diff/20001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:275: messages.erase( Could we also move this one (and maybe the file system ones above) into the block ~line 125? It'd be nice to have these type of suppressions in as few places as possible, to make it more readable.
https://codereview.chromium.org/236113002/diff/20001/chrome/common/extensions... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/236113002/diff/20001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:275: messages.erase( On 2014/04/14 22:27:43, D Cronin wrote: > Could we also move this one (and maybe the file system ones above) into the > block ~line 125? It'd be nice to have these type of suppressions in as few > places as possible, to make it more readable. This automatically merged into the SuppressMessage.. block in patchset #4 after refactoring.
https://codereview.chromium.org/236113002/diff/40001/chrome/common/extensions... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/236113002/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:68: typename T::iterator FindMessageByID(T& messages, int id) { Isn't this almost exactly the same as std::find()? Can we get rid of this function (especially since we only call it in one place)? (Sorry I didn't see this before.)
https://codereview.chromium.org/236113002/diff/40001/chrome/common/extensions... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/236113002/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:68: typename T::iterator FindMessageByID(T& messages, int id) { On 2014/04/15 15:09:16, D Cronin wrote: > Isn't this almost exactly the same as std::find()? Can we get rid of this > function (especially since we only call it in one place)? (Sorry I didn't see > this before.) Using std::find with id field would require PermissionMessage to overload the == operator which isn't the case, so I don't think that would work. It's called twice in SuppressMessage so unfortunately I can't inline it either.
LGTM. Sorry for making this one take so long. https://codereview.chromium.org/236113002/diff/40001/chrome/common/extensions... File chrome/common/extensions/permissions/chrome_permission_message_provider.cc (right): https://codereview.chromium.org/236113002/diff/40001/chrome/common/extensions... chrome/common/extensions/permissions/chrome_permission_message_provider.cc:68: typename T::iterator FindMessageByID(T& messages, int id) { On 2014/04/15 16:46:16, Mustafa Emre Acer wrote: > On 2014/04/15 15:09:16, D Cronin wrote: > > Isn't this almost exactly the same as std::find()? Can we get rid of this > > function (especially since we only call it in one place)? (Sorry I didn't see > > this before.) > > Using std::find with id field would require PermissionMessage to overload the == > operator which isn't the case, so I don't think that would work. It's called > twice in SuppressMessage so unfortunately I can't inline it either. Oh, shoot, missed that... and std::find_if would be overkill here. My bad - should have noticed that.
On 2014/04/15 16:52:45, D Cronin wrote: > LGTM. Sorry for making this one take so long. > > https://codereview.chromium.org/236113002/diff/40001/chrome/common/extensions... > File chrome/common/extensions/permissions/chrome_permission_message_provider.cc > (right): > > https://codereview.chromium.org/236113002/diff/40001/chrome/common/extensions... > chrome/common/extensions/permissions/chrome_permission_message_provider.cc:68: > typename T::iterator FindMessageByID(T& messages, int id) { > On 2014/04/15 16:46:16, Mustafa Emre Acer wrote: > > On 2014/04/15 15:09:16, D Cronin wrote: > > > Isn't this almost exactly the same as std::find()? Can we get rid of this > > > function (especially since we only call it in one place)? (Sorry I didn't > see > > > this before.) > > > > Using std::find with id field would require PermissionMessage to overload the > == > > operator which isn't the case, so I don't think that would work. It's called > > twice in SuppressMessage so unfortunately I can't inline it either. > > Oh, shoot, missed that... and std::find_if would be overkill here. My bad - > should have noticed that. No worries :) I'm happy with the refactoring and the current patch. Thanks Devlin!
The CQ bit was checked by meacer@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/meacer@chromium.org/236113002/60001
Message was sent while issue was closed.
Change committed as 263972 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
