Extensions: Switch to new permission message system, part IV

Extensions: Switch to new permission message system, part IV

Flip the Switch!

Also, re-order a rule in the new system to fix a test.
Also, cleanup the test.

BUG=398257, 478634
Committed: https://crrev.com/f94fcdc47b9f0c1db393ea6b276e6af3a9d4587d
Cr-Commit-Position: refs/heads/master@{#327020}

[Marc Treib, 2015-04-24 12:46:02]

treib@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Marc Treib, 2015-04-24 12:46:02]

PTAL! (See https://code.google.com/p/chromium/issues/detail?id=478634#c8 re
launch!)

https://codereview.chromium.org/1106613005/diff/1/chrome/common/extensions/pe...
File chrome/common/extensions/permissions/permission_set_unittest.cc (left):

https://codereview.chromium.org/1106613005/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:1169:
EXPECT_EQ(combined_index, IndexOf(warnings, "Use your microphone"));
This check was failing because the order of warnings changed from old to new
system. I've updated the rules for the new system so that it matches the old
one, but afaik we don't particularly care about the order of messages anywhere
else, so maybe we want to just remove this check?
(The rule reordering makes sense either way, because it moves related rules
together.)

[Devlin, 2015-04-24 15:48:52]

lgtm.

Out of curiosity, do we have any real way of knowing if this isn't working,
outside of our unit tests?  It seems like if a message doesn't show up on an
extension, it will be hard for users to detect....

https://codereview.chromium.org/1106613005/diff/1/chrome/common/extensions/pe...
File chrome/common/extensions/permissions/permission_set_unittest.cc (left):

https://codereview.chromium.org/1106613005/diff/1/chrome/common/extensions/pe...
chrome/common/extensions/permissions/permission_set_unittest.cc:1169:
EXPECT_EQ(combined_index, IndexOf(warnings, "Use your microphone"));
On 2015/04/24 12:46:02, Marc Treib wrote:
> This check was failing because the order of warnings changed from old to new
> system. I've updated the rules for the new system so that it matches the old
> one, but afaik we don't particularly care about the order of messages anywhere
> else, so maybe we want to just remove this check?
> (The rule reordering makes sense either way, because it moves related rules
> together.)

In general, we try to show the "scariest" permissions first, I think.  So
"access and change all your data on all websites" should come before, e.g., "add
bookmarks" (or whatever that message is).  In this particular case, I don't know
what the right order is, but I think it should be consistent (audio, video,
audio and video at about the same rank).

(So no action required, reorder is good.)

https://codereview.chromium.org/1106613005/diff/1/extensions/common/permissio...
File extensions/common/permissions/permission_message_provider.cc (right):

https://codereview.chromium.org/1106613005/diff/1/extensions/common/permissio...
extensions/common/permissions/permission_message_provider.cc:23: if (group_name
== "OldSystem")
nit: I think the old return style is fine, as long as we're not planning on
adding any weird half-on states :)

return group_name != "OldSystem";

[Marc Treib, 2015-04-27 08:22:08]

No, unfortunately we don't really have a good way of knowing if anything doesn't
work. I'm hoping that if anything major is missing, someone will notice...

https://codereview.chromium.org/1106613005/diff/1/extensions/common/permissio...
File extensions/common/permissions/permission_message_provider.cc (right):

https://codereview.chromium.org/1106613005/diff/1/extensions/common/permissio...
extensions/common/permissions/permission_message_provider.cc:23: if (group_name
== "OldSystem")
On 2015/04/24 15:48:52, Devlin wrote:
> nit: I think the old return style is fine, as long as we're not planning on
> adding any weird half-on states :)
> 
> return group_name != "OldSystem";

Done.

[Marc Treib, 2015-04-27 08:22:54]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-04-27 08:22:54]

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

[commit-bot: I haz the power, 2015-04-27 08:23:00]

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

[commit-bot: I haz the power, 2015-04-27 09:30:55]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-04-27 09:31:45]

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