Change topSites API permission warning

Change topSites and tabs API permission warning

The permission warning of the topSites and tabs API becomes
"Read your browsing history".

Combined and Suppressed Warnings:
topSites + history = history
topSites + history + tabs = history

If you add "sessions" on both sides of the above equations,
the equations hold true.

The combined warning for "topSites" and "sessions" is
"Read your browsing history on all your signed-in devices".

R=kalman@chromium.org,meacer@chromium.org,isherman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=277262

[wjywbs, 2014-06-11 00:06:11]

PTAL, thanks. The discussion thread is at
https://groups.google.com/a/chromium.org/forum/#!topic/apps-dev/w-j_Qf8-EGM

[Ilya Sherman, 2014-06-11 00:10:27]

histograms.xml lgtm

[meacer, 2014-06-11 20:03:10]

The code looks good, but a couple of questions for you and kalman.

https://codereview.chromium.org/328943002/diff/1/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/328943002/diff/1/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:4323: +      <message
name="IDS_EXTENSION_PROMPT_WARNING_TOPSITES" desc="Permission string for access
to topSites.">
Nit: Maybe you can rename this to IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ and
IDS_EXTENSION_PROMPT_WARNING_BROWSING_HISTORY to
IDS_EXTENSION_PROMPT_WARNING_HISTORY_WRITE, so that they can be used in the
future for other APIs, rather than being specific to topSites and
browsingHistory APIs.

https://codereview.chromium.org/328943002/diff/1/chrome/common/extensions/per...
File chrome/common/extensions/permissions/chrome_permission_message_provider.cc
(left):

https://codereview.chromium.org/328943002/diff/1/chrome/common/extensions/per...
chrome/common/extensions/permissions/chrome_permission_message_provider.cc:167:
IDS_EXTENSION_PROMPT_WARNING_TABS_AND_SESSIONS));
This is getting a bit complicated. The warning for tabs is "browsing activity",
and for topSites it is "browsing history", but when combined, they both become
"browsing history on all devices".

Question to you and kalman: What do you think about getting rid of "browsing
activity" altogether and use the same warning for both topSites and tabs? ("read
browsing history")

Then there would be four different strings:
Read your browsing history
Read your browsing history on all devices
Read and modify your browsing history
Read and modify your browsing history on all devices

https://codereview.chromium.org/328943002/diff/1/chrome/common/extensions/per...
File chrome/common/extensions/permissions/chrome_permission_message_provider.cc
(right):

https://codereview.chromium.org/328943002/diff/1/chrome/common/extensions/per...
chrome/common/extensions/permissions/chrome_permission_message_provider.cc:101:
PermissionMessage::kTabs);
See my comment below: if we just had "read browsing history" and "read and
modify browsing history" and get rid of "browsing activity", this would be
cleaner: kBrowsingHistoryWrite would suppress kBrowsingHistoryRead.

[wjywbs, 2014-06-11 23:43:18]

https://codereview.chromium.org/328943002/diff/1/chrome/common/extensions/per...
File chrome/common/extensions/permissions/chrome_permission_message_provider.cc
(left):

https://codereview.chromium.org/328943002/diff/1/chrome/common/extensions/per...
chrome/common/extensions/permissions/chrome_permission_message_provider.cc:167:
IDS_EXTENSION_PROMPT_WARNING_TABS_AND_SESSIONS));
On 2014/06/11 20:03:10, Mustafa Emre Acer wrote:
> This is getting a bit complicated. The warning for tabs is "browsing
activity",
> and for topSites it is "browsing history", but when combined, they both become
> "browsing history on all devices".
> 
> Question to you and kalman: What do you think about getting rid of "browsing
> activity" altogether and use the same warning for both topSites and tabs?
("read
> browsing history")
> 
> Then there would be four different strings:
> Read your browsing history
> Read your browsing history on all devices
> Read and modify your browsing history
> Read and modify your browsing history on all devices

The "tabs" API does not have any access to the history. You can collect history
by getting a list of opened URLs and monitoring to tab activities, such as
onClosed, onUpdated, etc. My only concern is that "read browsing history" is not
very accurate.

[meacer, 2014-06-12 00:05:08]

https://codereview.chromium.org/328943002/diff/1/chrome/common/extensions/per...
File chrome/common/extensions/permissions/chrome_permission_message_provider.cc
(left):

https://codereview.chromium.org/328943002/diff/1/chrome/common/extensions/per...
chrome/common/extensions/permissions/chrome_permission_message_provider.cc:167:
IDS_EXTENSION_PROMPT_WARNING_TABS_AND_SESSIONS));
On 2014/06/11 23:43:18, wjywbs wrote:
> On 2014/06/11 20:03:10, Mustafa Emre Acer wrote:
> > This is getting a bit complicated. The warning for tabs is "browsing
> activity",
> > and for topSites it is "browsing history", but when combined, they both
become
> > "browsing history on all devices".
> > 
> > Question to you and kalman: What do you think about getting rid of "browsing
> > activity" altogether and use the same warning for both topSites and tabs?
> ("read
> > browsing history")
> > 
> > Then there would be four different strings:
> > Read your browsing history
> > Read your browsing history on all devices
> > Read and modify your browsing history
> > Read and modify your browsing history on all devices
> 
> The "tabs" API does not have any access to the history. You can collect
history
> by getting a list of opened URLs and monitoring to tab activities, such as
> onClosed, onUpdated, etc. My only concern is that "read browsing history" is
not
> very accurate.

I mean, it kind of does, depending on one's definition of history. If you have
been using the extension for a month, then it has access to your 30 day browsing
history. The distinction between "browsing activity" and "browsing history" is
too subtle that I'm not sure if anyone really understands it.

[wjywbs, 2014-06-13 15:42:47]

I updated the code. It's about 1 week before the M37 branch date now. If the
time is not enough, how about taking the sessions api to stable first, and I can
work on this CL by M38? Thanks.

https://codereview.chromium.org/328943002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/permissions/chrome_api_permissions.cc (right):

https://codereview.chromium.org/328943002/diff/20001/chrome/common/extensions...
chrome/common/extensions/permissions/chrome_api_permissions.cc:132:
PermissionMessage::kBrowsingHistoryRead},
The processes API is probably not related to history. "Browsing activity" could
be better here. My intention was to make the topSites warning accurate. But if
this causes other warnings to become inaccurate, I don't think I should change
the topSites warning anymore.

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

https://codereview.chromium.org/328943002/diff/20001/chrome/common/extensions...
chrome/common/extensions/permissions/chrome_permission_message_provider.cc:97:
PermissionMessage::kBrowsingHistoryRead);
Granting all hosts permission will also allow "processes", "tabs", "topSites"
and "webNavigation".

[meacer, 2014-06-13 17:28:36]

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

https://codereview.chromium.org/328943002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:4309: Read and modify your browsing history
on all your signed-in devices
Nit: "Read" vs. "Access" sounds inconsistent. Can we change write warnings to
"Access and modify your browsing history..."

https://codereview.chromium.org/328943002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/permissions/permission_set_unittest.cc (right):

https://codereview.chromium.org/328943002/diff/20001/chrome/common/extensions...
chrome/common/extensions/permissions/permission_set_unittest.cc:896:
api_permissions.insert(APIPermission::kTopSites);
Maybe you can simply check kTab and kTopSites (and processes etc) have the same
warning?

https://codereview.chromium.org/328943002/diff/20001/chrome/common/extensions...
chrome/common/extensions/permissions/permission_set_unittest.cc:1083: new
PermissionSet(api_permissions, ManifestPermissionSet(),
Same as above.

[not at google - send to devlin, 2014-06-13 17:29:35]

On 2014/06/13 15:42:47, wjywbs wrote:
> I updated the code. It's about 1 week before the M37 branch date now. If the
> time is not enough, how about taking the sessions api to stable first, and I
can
> work on this CL by M38? Thanks.
> 

FYI this API is likely good to go for stable in M37.

[wjywbs, 2014-06-13 18:02:53]

PTAL, thanks.

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

https://codereview.chromium.org/328943002/diff/20001/chrome/app/generated_res...
chrome/app/generated_resources.grd:4318: +        Access your browsing history
I changed it to "Read".

https://codereview.chromium.org/328943002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/permissions/permission_set_unittest.cc (right):

https://codereview.chromium.org/328943002/diff/20001/chrome/common/extensions...
chrome/common/extensions/permissions/permission_set_unittest.cc:896:
api_permissions.insert(APIPermission::kTopSites);
On 2014/06/13 17:28:36, Mustafa Emre Acer wrote:
> Maybe you can simply check kTab and kTopSites (and processes etc) have the
same
> warning?

Done.

https://codereview.chromium.org/328943002/diff/20001/chrome/common/extensions...
chrome/common/extensions/permissions/permission_set_unittest.cc:1083: new
PermissionSet(api_permissions, ManifestPermissionSet(),
On 2014/06/13 17:28:36, Mustafa Emre Acer wrote:
> Same as above.

Done.

[meacer, 2014-06-13 19:59:21]

Lgtm (you'll need Ben's lgtm too)

https://codereview.chromium.org/328943002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/permissions/permission_set_unittest.cc (right):

https://codereview.chromium.org/328943002/diff/40001/chrome/common/extensions...
chrome/common/extensions/permissions/permission_set_unittest.cc:835:
EXPECT_EQ(PermissionMessage::kBrowsingHistoryWrite, messages[0].id());
This case doesn't seem necessary anymore, now that you are testing for history +
tab + topsites + processes + webnavigation below.

https://codereview.chromium.org/328943002/diff/40001/chrome/common/extensions...
chrome/common/extensions/permissions/permission_set_unittest.cc:1047:
api_permissions.insert(APIPermission::kHistory);
Can you put kHistory at the top? (since it's the one that suppresses others)

[not at google - send to devlin, 2014-06-13 20:21:56]

rubber stamp lgtm

[wjywbs, 2014-06-13 20:50:10]

PTAL. This change will not pass chromium_presubmit because the enums are renamed
instead of adding new ones at bottom. Thanks.

https://codereview.chromium.org/328943002/diff/40001/chrome/common/extensions...
File chrome/common/extensions/permissions/permission_set_unittest.cc (right):

https://codereview.chromium.org/328943002/diff/40001/chrome/common/extensions...
chrome/common/extensions/permissions/permission_set_unittest.cc:835:
EXPECT_EQ(PermissionMessage::kBrowsingHistoryWrite, messages[0].id());
On 2014/06/13 19:59:20, Mustafa Emre Acer wrote:
> This case doesn't seem necessary anymore, now that you are testing for history
+
> tab + topsites + processes + webnavigation below.

Done.

https://codereview.chromium.org/328943002/diff/40001/chrome/common/extensions...
chrome/common/extensions/permissions/permission_set_unittest.cc:1047:
api_permissions.insert(APIPermission::kHistory);
On 2014/06/13 19:59:20, Mustafa Emre Acer wrote:
> Can you put kHistory at the top? (since it's the one that suppresses others)

Done.

[meacer, 2014-06-13 21:03:01]

On 2014/06/13 20:50:10, wjywbs wrote:
> PTAL. This change will not pass chromium_presubmit because the enums are
renamed
> instead of adding new ones at bottom. Thanks.
> 
>
https://codereview.chromium.org/328943002/diff/40001/chrome/common/extensions...
> File chrome/common/extensions/permissions/permission_set_unittest.cc (right):
> 
>
https://codereview.chromium.org/328943002/diff/40001/chrome/common/extensions...
> chrome/common/extensions/permissions/permission_set_unittest.cc:835:
> EXPECT_EQ(PermissionMessage::kBrowsingHistoryWrite, messages[0].id());
> On 2014/06/13 19:59:20, Mustafa Emre Acer wrote:
> > This case doesn't seem necessary anymore, now that you are testing for
history
> +
> > tab + topsites + processes + webnavigation below.
> 
> Done.
> 
>
https://codereview.chromium.org/328943002/diff/40001/chrome/common/extensions...
> chrome/common/extensions/permissions/permission_set_unittest.cc:1047:
> api_permissions.insert(APIPermission::kHistory);
> On 2014/06/13 19:59:20, Mustafa Emre Acer wrote:
> > Can you put kHistory at the top? (since it's the one that suppresses others)
> 
> Done.

I guess you can keep IDS_EXTENSION_PROMPT_WARNING_BROWSING_HISTORY and
IDS_EXTENSION_PROMPT_WARNING_TABS then :/

[wjywbs, 2014-06-13 21:05:57]

On 2014/06/13 21:03:01, Mustafa Emre Acer wrote:
> I guess you can keep IDS_EXTENSION_PROMPT_WARNING_BROWSING_HISTORY and
> IDS_EXTENSION_PROMPT_WARNING_TABS then :/

Sorry, I meant kBrowsingHistory and kTabs in permission_message.h. I plan to run
the try jobs, let the chromium_presubmit fail and set NOTRY=true when all other
jobs are green. Is this ok?

[meacer, 2014-06-13 21:12:03]

On 2014/06/13 21:05:57, wjywbs wrote:
> On 2014/06/13 21:03:01, Mustafa Emre Acer wrote:
> > I guess you can keep IDS_EXTENSION_PROMPT_WARNING_BROWSING_HISTORY and
> > IDS_EXTENSION_PROMPT_WARNING_TABS then :/
> 
> Sorry, I meant kBrowsingHistory and kTabs in permission_message.h. I plan to
run
> the try jobs, let the chromium_presubmit fail and set NOTRY=true when all
other
> jobs are green. Is this ok?

Ben pointed that the enums are referenced from multiple UMA histograms. I don't
know if anyone is actively monitoring those histograms, but we might want to
hold off renaming for now and go with original enums. Does that sound OK?

Sorry for the trouble.

[wjywbs, 2014-06-13 22:29:22]

PTAL, thanks.

[meacer, 2014-06-13 23:54:28]

On 2014/06/13 22:29:22, wjywbs wrote:
> PTAL, thanks.

Lgtm, thanks again.

[wjywbs, 2014-06-14 01:53:06]

The CQ bit was checked by wjywbs@gmail.com

[commit-bot: I haz the power, 2014-06-14 01:54:53]

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

[commit-bot: I haz the power, 2014-06-14 20:15:25]

Change committed as 277262