Change tabs + sessions permission warning

Change tabs + sessions and history + sessions permission warnings

The permission warning when both "tabs" and "sessions" present becomes 
"Access your browsing history on all your signed-in devices".

The permission warning when both "history" and "sessions" present becomes
"Read and modify your browsing history on all your signed-in devices".

The warning for tabs + history + sessions is the same as history + sessions.

R=kalman@chromium.org,meacer@chromium.org
BUG=123266, 377607
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276054

[wjywbs, 2014-06-06 23:33:39]

PTAL, thanks.

[not at google - send to devlin, 2014-06-06 23:45:06]

i'll let mustafa review the real code since he's being doing the work there
recently

https://codereview.chromium.org/323633003/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/323633003/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:4311: Access your browsing history on all
devices you have signed in
it looks like we do use the term "signed-in devices" throughout here, so perhaps
this should be "Access your browsing history on all your signed-in devices"

[wjywbs, 2014-06-06 23:52:23]

https://codereview.chromium.org/323633003/diff/20001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/323633003/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:4311: Access your browsing history on all
devices you have signed in
On 2014/06/06 23:45:05, kalman wrote:
> it looks like we do use the term "signed-in devices" throughout here, so
perhaps
> this should be "Access your browsing history on all your signed-in devices"

Done. Can you please update the doc also?

[not at google - send to devlin, 2014-06-06 23:56:36]

doc has been updated.

unrelated question: are you willing to publish (open source) your extension
which uses this API?

[meacer, 2014-06-06 23:56:44]

> The warning for tabs + sessions + history is the same as tabs + history. 

Not sure I understand this.

Warning for tabs + history suppresses the warning for tabs, so you'd either
have:

- warnings for history (tabs+history combined) and sessions (2 warnings)
- or warnings for (tabs+sessions combined) and history (2 warnings again)

depending on whether the suppression or combining of tabs+sessions take place.
You'll probably have to add a new warning string for (sessions + history) as
well, since history is "Read and modify" rather 
than read only and then show that for tabs + sessions + history.

[wjywbs, 2014-06-07 00:10:40]

On 2014/06/06 23:56:44, Mustafa Emre Acer wrote:
> Warning for tabs + history suppresses the warning for tabs, so you'd either
> have:
> 
> - warnings for history (tabs+history combined) and sessions (2 warnings)
> - or warnings for (tabs+sessions combined) and history (2 warnings again)

The tabs permission message is already removed after detecting the tabs+history
case, and "sessions" itself does not have a permission message. The result is:

- warnings for history (tabs+history combined) (1 warning)

Maybe we should also detect history+sessions, so "history+sessions" and
"history+tab+sessions" produce "Read and modify your browsing history on all
your signed-in devices"? This may produce a misunderstanding that the extension
can modify the history on other devices.

[meacer, 2014-06-07 00:18:00]

On 2014/06/07 00:10:40, wjywbs wrote:
> On 2014/06/06 23:56:44, Mustafa Emre Acer wrote:
> > Warning for tabs + history suppresses the warning for tabs, so you'd either
> > have:
> > 
> > - warnings for history (tabs+history combined) and sessions (2 warnings)
> > - or warnings for (tabs+sessions combined) and history (2 warnings again)
> 
> The tabs permission message is already removed after detecting the
tabs+history
> case, and "sessions" itself does not have a permission message. The result is:
> 
> - warnings for history (tabs+history combined) (1 warning)
> 
> Maybe we should also detect history+sessions, so "history+sessions" and
> "history+tab+sessions" produce "Read and modify your browsing history on all
> your signed-in devices"? This may produce a misunderstanding that the
extension
> can modify the history on other devices.

Ah, I forgot sessions didn't have a warning. In that case yes, we should detect
history+sessions. The string you are suggesting sounds good to me.

[wjywbs, 2014-06-07 00:45:41]

On 2014/06/07 00:18:00, Mustafa Emre Acer wrote:
> Ah, I forgot sessions didn't have a warning. In that case yes, we should
detect
> history+sessions. The string you are suggesting sounds good to me.

Done. By the way, since "topSites" has the same permission message as "history",
topSites+sessions has the same permission message as history+sessions. Thanks.

[meacer, 2014-06-07 01:12:28]

On 2014/06/07 00:45:41, wjywbs wrote:
> On 2014/06/07 00:18:00, Mustafa Emre Acer wrote:
> > Ah, I forgot sessions didn't have a warning. In that case yes, we should
> detect
> > history+sessions. The string you are suggesting sounds good to me.
> 
> Done. By the way, since "topSites" has the same permission message as
"history",
> topSites+sessions has the same permission message as history+sessions. Thanks.

Lgtm. Thanks!

[wjywbs, 2014-06-07 01:45:34]

The CQ bit was checked by wjywbs@gmail.com

[commit-bot: I haz the power, 2014-06-07 01:47:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wjywbs@gmail.com/323633003/60001

[meacer, 2014-06-07 01:53:08]

(I'm not an owner so you still need Ben's lg tm)

[wjywbs, 2014-06-07 01:54:57]

The CQ bit was unchecked by wjywbs@gmail.com

[not at google - send to devlin, 2014-06-09 16:57:31]

https://codereview.chromium.org/323633003/diff/60001/chrome/common/extensions...
File chrome/common/extensions/permissions/chrome_permission_message_provider.cc
(right):

https://codereview.chromium.org/323633003/diff/60001/chrome/common/extensions...
chrome/common/extensions/permissions/chrome_permission_message_provider.cc:101:
GetPermissionMessages(permissions, extension_type);
honestly all this code and its looping is a bit weird and hard to follow.
extensions are unlikely to have >couple of warnings so this optimisation of
gather together bools to look up later is hard to read.

if there were a method bool ContainsWarningMessage(const
std::vector<base::string16>& messages, PermissionMessage::ID){...} which just
did the iteration each time, it would be much easier to read.

(another approach would be to convert to a std::set but that seems like overkill
given the small size of this list).

I don't think this is a cleanup that needs to happen now.

https://codereview.chromium.org/323633003/diff/60001/chrome/common/extensions...
chrome/common/extensions/permissions/chrome_permission_message_provider.cc:167:
id == PermissionMessage::kTabs) {
am I missing something or is this check and the
id==PermissionMessage::kBrowsingHistory check below pointless, because if tabs
is true then that implies PermissionMessage::kTabs and likewise is history is
true then that implies PermissionMessage::kBrowsingHistory?

[wjywbs, 2014-06-09 20:06:28]

PTAL, thanks.

https://codereview.chromium.org/323633003/diff/60001/chrome/common/extensions...
File chrome/common/extensions/permissions/chrome_permission_message_provider.cc
(right):

https://codereview.chromium.org/323633003/diff/60001/chrome/common/extensions...
chrome/common/extensions/permissions/chrome_permission_message_provider.cc:167:
id == PermissionMessage::kTabs) {
On 2014/06/09 16:57:31, kalman wrote:
> am I missing something or is this check and the
> id==PermissionMessage::kBrowsingHistory check below pointless, because if tabs
> is true then that implies PermissionMessage::kTabs and likewise is history is
> true then that implies PermissionMessage::kBrowsingHistory?

You are right. A lot of lines are not necessary. This is in a loop of all
permission messages, and we change the message when the |id| is the kTabs
message.

[not at google - send to devlin, 2014-06-09 20:41:23]

lgtm

[wjywbs, 2014-06-09 20:46:43]

The CQ bit was checked by wjywbs@gmail.com

[commit-bot: I haz the power, 2014-06-09 20:49:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wjywbs@gmail.com/323633003/80001

[commit-bot: I haz the power, 2014-06-10 11:13:50]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-10 16:43:45]

Change committed as 276054