Extensions: Switch to new permission message system, part I

Extensions: Switch to new permission message system, part I

- Update lots of tests to check that the old and new system match.
- In the process, fix two small bugs in the new system.
- Rename PermissionsData::GetPermissionMessageStrings and GetPermissionMessageDetailsStrings to "*Legacy*", to match the corresponding methods in PermissionMessageProvider.

BUG=398257
TBR=asargent
for a trivial rename in ephemeral_app_launcher.cc

Committed: https://crrev.com/f91b57ec449cf77cec413328b7fc6fc3f48e6fe6
Cr-Commit-Position: refs/heads/master@{#321550}

[Marc Treib, 2015-03-09 14:08:02]

Part 1 of "flip the switch" - PTAL!
(I'll add OWNERS once you've done a sanity check :)

https://codereview.chromium.org/980353003/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/980353003/diff/40001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:1021:
extension->permissions_data()->GetLegacyPermissionMessages();
Unfortunately, we can't just switch to the new system here, since the old IDs
are used in a histogram. We'll have to break that if we want to actually remove
the old stuff :-(

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

https://codereview.chromium.org/980353003/diff/40001/chrome/common/extensions...
chrome/common/extensions/permissions/chrome_permission_message_provider.cc:207:
CoalesceWarningMessages(permissions,
Before, we'd coalesce the warnings, but not their details. That resulted in
their indices not matching up anymore.

https://codereview.chromium.org/980353003/diff/40001/chrome/common/extensions...
chrome/common/extensions/permissions/chrome_permission_message_provider.cc:272:
if (permission_ids != NULL && extension_type != Manifest::TYPE_PLATFORM_APP)
Platform apps never show the "all urls" warning in the old system; this makes
the new system do the same.

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

https://codereview.chromium.org/980353003/diff/40001/chrome/common/extensions...
chrome/common/extensions/permissions/chrome_permission_message_rules.cc:268:
IDS_EXTENSION_PROMPT_WARNING_SOCKET_HOSTS_IN_DOMAIN,
Changed the order here because some tests in permission_set_unittest check that
domain warnings come before host warnings.

[Marc Treib, 2015-03-09 16:07:49]

treib@chromium.org changed reviewers:
+ sashab@chromium.org

[Marc Treib, 2015-03-09 16:07:50]

+sashab - it helps to actually add the reviewer :D

[sashab, 2015-03-09 20:05:50]

This generally looks awesome, but out of scaredness, may I make a suggestion -
why don't we, instead of switching in one go, add a
--enable-new-permission-messages flag which uses the new system (so keep both
systems in place)? That would make it easier to launch, and easy to switch back
if anything goes wrong. It also means that these "part N"-style CLs can all land
safely independent of branch points.

I still like the "Legacy" class renames, so keep those :)

[sashab, 2015-03-09 20:07:31]

Also... I'm actually in MTV this week, so might not get a chance to look at it
soon :-) yoz@ was actually reviewing my initial work for this, so if he's
available, might be good to get his opinion on this stuff as well.

[Marc Treib, 2015-03-10 12:08:41]

treib@chromium.org changed reviewers:
+ yoz@chromium.org

[Marc Treib, 2015-03-10 12:08:41]

+yoz, then. PTAL!

[Marc Treib, 2015-03-10 12:09:54]

On 2015/03/09 20:05:50, sasha_b wrote:
> This generally looks awesome, but out of scaredness, may I make a suggestion -
> why don't we, instead of switching in one go, add a
> --enable-new-permission-messages flag which uses the new system (so keep both
> systems in place)? That would make it easier to launch, and easy to switch
back
> if anything goes wrong. It also means that these "part N"-style CLs can all
land
> safely independent of branch points.

Hm.. a flag is probably a good idea in general, but checking it at every call
site seems super-fragile and awkward. I guess we could check the flag in
GetCoalescedPermissionMessages, and call the legacy implementation if it's not
set (and package the results up in a CoalescedPermissionMessages). WDYT?

[Yoyo Zhou, 2015-03-11 01:33:23]

https://chromiumcodereview.appspot.com/980353003/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/980353003/diff/40001/chrome/browser/ex...
chrome/browser/extensions/extension_service.cc:1021:
extension->permissions_data()->GetLegacyPermissionMessages();
On 2015/03/09 14:08:02, Marc Treib wrote:
> Unfortunately, we can't just switch to the new system here, since the old IDs
> are used in a histogram. We'll have to break that if we want to actually
remove
> the old stuff :-(

Can we introduce a new histogram? I imagine we are still interested in
collecting this data, but if we're measuring something different, we need a new
histogram. Of course, I don't think we want to deprecate the old one before the
new one is ready.

https://chromiumcodereview.appspot.com/980353003/diff/40001/chrome/common/ext...
File chrome/common/extensions/permissions/chrome_permission_message_provider.cc
(right):

https://chromiumcodereview.appspot.com/980353003/diff/40001/chrome/common/ext...
chrome/common/extensions/permissions/chrome_permission_message_provider.cc:207:
CoalesceWarningMessages(permissions,
On 2015/03/09 14:08:02, Marc Treib wrote:
> Before, we'd coalesce the warnings, but not their details. That resulted in
> their indices not matching up anymore.

Is there a test for this case?

https://chromiumcodereview.appspot.com/980353003/diff/40001/extensions/common...
File extensions/common/permissions/permission_message_test_util.cc (right):

https://chromiumcodereview.appspot.com/980353003/diff/40001/extensions/common...
extensions/common/permissions/permission_message_test_util.cc:112:
EXPECT_TRUE(false) << "Expected details for \"" << message << "\" to be "
EXPECT_TRUE(false) can also be written as ADD_FAILURE()

https://chromiumcodereview.appspot.com/980353003/diff/40001/extensions/common...
extensions/common/permissions/permission_message_test_util.cc:311: << "Expected
" << expected_messages.size() << " messages "
Since messages can get rather long, it might be better to have these separated
from each other by newlines.

https://chromiumcodereview.appspot.com/980353003/diff/40001/extensions/common...
File extensions/common/permissions/permissions_data.h (right):

https://chromiumcodereview.appspot.com/980353003/diff/40001/extensions/common...
extensions/common/permissions/permissions_data.h:74: // Returns true if we
should skip the permissons warning for the extension
nit: permissions

https://chromiumcodereview.appspot.com/980353003/diff/40001/extensions/common...
extensions/common/permissions/permissions_data.h:138: PermissionMessages
GetLegacyPermissionMessages() const;
Here's my thinking about flags. Ideally, we have some APIs like
GetPermissionMessages that we can hide the implementation change behind. I'm not
sure if works out that cleanly, but it would be best if GetPermissionMessages
and friends check the flag and, if it is set, use the new system.

If that doesn't work, I'm okay with your proposal to create a
CoalescedPermissionMessages legacy-style if the flag is set to 'legacy'. That
does mean that this change is not a no-op without the flag, though, since we
have switched to using CoalescedPermissionMessages and potentially introduced
bugs where we did so, etc. I think our test coverage is good enough to take that
risk.

[Marc Treib, 2015-03-11 13:08:52]

https://chromiumcodereview.appspot.com/980353003/diff/40001/chrome/browser/ex...
File chrome/browser/extensions/extension_service.cc (right):

https://chromiumcodereview.appspot.com/980353003/diff/40001/chrome/browser/ex...
chrome/browser/extensions/extension_service.cc:1021:
extension->permissions_data()->GetLegacyPermissionMessages();
On 2015/03/11 01:33:23, Yoyo Zhou wrote:
> On 2015/03/09 14:08:02, Marc Treib wrote:
> > Unfortunately, we can't just switch to the new system here, since the old
IDs
> > are used in a histogram. We'll have to break that if we want to actually
> remove
> > the old stuff :-(
> 
> Can we introduce a new histogram? I imagine we are still interested in
> collecting this data, but if we're measuring something different, we need a
new
> histogram. Of course, I don't think we want to deprecate the old one before
the
> new one is ready.

Yes, I think we'll introduce a new histogram to (eventually) replace this one.
I'd like to add the new histogram in a separate CL though; this one is already
way too large.

https://chromiumcodereview.appspot.com/980353003/diff/40001/chrome/common/ext...
File chrome/common/extensions/permissions/chrome_permission_message_provider.cc
(right):

https://chromiumcodereview.appspot.com/980353003/diff/40001/chrome/common/ext...
chrome/common/extensions/permissions/chrome_permission_message_provider.cc:207:
CoalesceWarningMessages(permissions,
On 2015/03/11 01:33:23, Yoyo Zhou wrote:
> On 2015/03/09 14:08:02, Marc Treib wrote:
> > Before, we'd coalesce the warnings, but not their details. That resulted in
> > their indices not matching up anymore.
> 
> Is there a test for this case?

Not yet. I'll pull this part out into a separate CL, and add a test.

https://chromiumcodereview.appspot.com/980353003/diff/40001/extensions/common...
File extensions/common/permissions/permission_message_test_util.cc (right):

https://chromiumcodereview.appspot.com/980353003/diff/40001/extensions/common...
extensions/common/permissions/permission_message_test_util.cc:112:
EXPECT_TRUE(false) << "Expected details for \"" << message << "\" to be "
On 2015/03/11 01:33:23, Yoyo Zhou wrote:
> EXPECT_TRUE(false) can also be written as ADD_FAILURE()

Ah, nice! Thanks, done.

https://chromiumcodereview.appspot.com/980353003/diff/40001/extensions/common...
extensions/common/permissions/permission_message_test_util.cc:311: << "Expected
" << expected_messages.size() << " messages "
On 2015/03/11 01:33:23, Yoyo Zhou wrote:
> Since messages can get rather long, it might be better to have these separated
> from each other by newlines.

Done.

https://chromiumcodereview.appspot.com/980353003/diff/40001/extensions/common...
File extensions/common/permissions/permissions_data.h (right):

https://chromiumcodereview.appspot.com/980353003/diff/40001/extensions/common...
extensions/common/permissions/permissions_data.h:74: // Returns true if we
should skip the permissons warning for the extension
On 2015/03/11 01:33:23, Yoyo Zhou wrote:
> nit: permissions

Done.

https://chromiumcodereview.appspot.com/980353003/diff/40001/extensions/common...
extensions/common/permissions/permissions_data.h:138: PermissionMessages
GetLegacyPermissionMessages() const;
On 2015/03/11 01:33:23, Yoyo Zhou wrote:
> Here's my thinking about flags. Ideally, we have some APIs like
> GetPermissionMessages that we can hide the implementation change behind. I'm
not
> sure if works out that cleanly, but it would be best if GetPermissionMessages
> and friends check the flag and, if it is set, use the new system.
> 
> If that doesn't work, I'm okay with your proposal to create a
> CoalescedPermissionMessages legacy-style if the flag is set to 'legacy'. That
> does mean that this change is not a no-op without the flag, though, since we
> have switched to using CoalescedPermissionMessages and potentially introduced
> bugs where we did so, etc. I think our test coverage is good enough to take
that
> risk.

Alright. I'll do that and ping you again.

[Marc Treib, 2015-03-16 15:00:13]

PTAL again! This CL updates lots of tests to check both the old and the new
system.

[Yoyo Zhou, 2015-03-17 00:02:05]

mostly LGTM

https://codereview.chromium.org/980353003/diff/60001/extensions/common/permis...
File extensions/common/permissions/permissions_data.h (right):

https://codereview.chromium.org/980353003/diff/60001/extensions/common/permis...
extensions/common/permissions/permissions_data.h:136: // Deprecated; use
GetCoalescedPermissionMessages() instead.
Why was this removed? I think it helps to say what to use instead.

https://codereview.chromium.org/980353003/diff/140001/extensions/common/permi...
File extensions/common/permissions/permission_message_test_util.cc (right):

https://codereview.chromium.org/980353003/diff/140001/extensions/common/permi...
extensions/common/permissions/permission_message_test_util.cc:117: const
PermissionMessageString& message,
Call this expected_message

https://codereview.chromium.org/980353003/diff/140001/extensions/common/permi...
extensions/common/permissions/permission_message_test_util.cc:156:
testing::AssertionResult VerifyHasPermissionMessageImpl(
I notice the order of arguments here is different from CheckThatSubmessagesMatch
- the convention is usually that expected comes before actual. I would also
check that the name of this function actually makes sense. This is more like
checking that messages match.

https://codereview.chromium.org/980353003/diff/140001/extensions/common/permi...
extensions/common/permissions/permission_message_test_util.cc:205:
testing::AssertionResult VerifyPermissionMessagesWithSubmessagesImpl(
ditto here

[Marc Treib, 2015-03-17 10:00:27]

https://codereview.chromium.org/980353003/diff/60001/extensions/common/permis...
File extensions/common/permissions/permissions_data.h (right):

https://codereview.chromium.org/980353003/diff/60001/extensions/common/permis...
extensions/common/permissions/permissions_data.h:136: // Deprecated; use
GetCoalescedPermissionMessages() instead.
On 2015/03/17 00:02:05, Yoyo Zhou wrote:
> Why was this removed? I think it helps to say what to use instead.

Ah, the CL's confusing here, sorry about that.
The existing GetPermissionMessageStrings was renamed to
GetLegacyPermissionMessageStrings, and that one still has the deprecation
comment.
This new GetPermissionMessageStrings is the new interim API which calls either
the old or the new system, so this is what's actually supposed to be used. (My
next CL will switch all callers of the Legacy methods over to this one.)

I've updated the deprecation comments on the other methods to refer to this
method as the one to be used.

https://codereview.chromium.org/980353003/diff/140001/extensions/common/permi...
File extensions/common/permissions/permission_message_test_util.cc (right):

https://codereview.chromium.org/980353003/diff/140001/extensions/common/permi...
extensions/common/permissions/permission_message_test_util.cc:117: const
PermissionMessageString& message,
On 2015/03/17 00:02:05, Yoyo Zhou wrote:
> Call this expected_message

Done.

https://codereview.chromium.org/980353003/diff/140001/extensions/common/permi...
extensions/common/permissions/permission_message_test_util.cc:156:
testing::AssertionResult VerifyHasPermissionMessageImpl(
On 2015/03/17 00:02:05, Yoyo Zhou wrote:
> I notice the order of arguments here is different from
CheckThatSubmessagesMatch
> - the convention is usually that expected comes before actual. I would also
> check that the name of this function actually makes sense. This is more like
> checking that messages match.

Argument order fixed.
I think the name does make sense though: This checks for existence of the
(single) expected_message within the (multiple) actual_messages. Always open for
better suggestions, though :)

https://codereview.chromium.org/980353003/diff/140001/extensions/common/permi...
extensions/common/permissions/permission_message_test_util.cc:205:
testing::AssertionResult VerifyPermissionMessagesWithSubmessagesImpl(
On 2015/03/17 00:02:05, Yoyo Zhou wrote:
> ditto here

Done.

[Marc Treib, 2015-03-20 10:26:48]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-03-20 10:26:49]

The patchset sent to the CQ was uploaded after l-g-t-m from yoz@chromium.org
Link to the patchset: https://codereview.chromium.org/980353003/#ps160001
(title: "review")

[commit-bot: I haz the power, 2015-03-20 10:27:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/980353003/160001

[commit-bot: I haz the power, 2015-03-20 11:45:09]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-03-20 11:45:44]

Patchset 9 (id:??) landed as
https://crrev.com/f91b57ec449cf77cec413328b7fc6fc3f48e6fe6
Cr-Commit-Position: refs/heads/master@{#321550}