Extensions: Prepare switch to new permission message system

Extensions: Prepare switch to new permission message system

PermissionMessageProvider:
- add GetPermissionMessageStrings which calls either the old or the new system, based on a field trial
- rename GetWarningMessages/GetWarningMessagesDetails to *Legacy*
- replace GetPermissionMessages by GetLegacyPermissionMessageIDs -> remove the old PermissionMessages type from the interface

Callers have *not* been updated to use the new GetPermissionMessageStrings yet.

TBRing trivial change in ephemeral_app_launcher.cc
TBR=asargent@chromium.org

BUG=398257
Committed: https://crrev.com/b566ee83484dd1ef53e25dd24bbd6fe02d80daec
Cr-Commit-Position: refs/heads/master@{#320702}

[Marc Treib, 2015-03-12 15:19:50]

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

[Marc Treib, 2015-03-12 15:19:51]

yoz: PTAL!

sashab: FYI, or feel free to take a look as well :)

[sashab, 2015-03-12 16:05:30]

Ahh, I like this MUCH better. Thanks for adding the common interface. Will try
to look at this in more detail later :-)

[Yoyo Zhou, 2015-03-12 20:55:58]

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

https://codereview.chromium.org/1006453002/diff/20001/chrome/common/extension...
chrome/common/extensions/permissions/chrome_permission_message_provider.cc:30:
base::FieldTrialList::FindFullName("PermissionMessageSystem");
You haven't defined this field trial yet (see field_trial.h documentation). IMO
you should do so in this patch, because otherwise we're asking about a
nonexistent field trial. The trial can start out with 100% of users not using
the new system.

https://codereview.chromium.org/1006453002/diff/20001/chrome/common/extension...
File chrome/common/extensions/permissions/chrome_permission_message_provider.h
(right):

https://codereview.chromium.org/1006453002/diff/20001/chrome/common/extension...
chrome/common/extensions/permissions/chrome_permission_message_provider.h:28: //
PermissionMessageProvider implementation.
Add some comments here about the legacy-ness of various methods here. If someone
wants to add some new code using permissions, how do they know what interface to
use, while we have both the old and new systems?

(Even though your next CL will probably be to switch to using the intermediate
state, it'll help to have documentation here.)

[Marc Treib, 2015-03-13 10:43:09]

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

https://codereview.chromium.org/1006453002/diff/20001/chrome/common/extension...
chrome/common/extensions/permissions/chrome_permission_message_provider.cc:30:
base::FieldTrialList::FindFullName("PermissionMessageSystem");
On 2015/03/12 20:55:57, Yoyo Zhou wrote:
> You haven't defined this field trial yet (see field_trial.h documentation).
IMO
> you should do so in this patch, because otherwise we're asking about a
> nonexistent field trial. The trial can start out with 100% of users not using
> the new system.

My intention was to enable this through Finch. In that case, I think we mustn't
define the field trial in Chrome (see go/finch-guide).

https://codereview.chromium.org/1006453002/diff/20001/chrome/common/extension...
File chrome/common/extensions/permissions/chrome_permission_message_provider.h
(right):

https://codereview.chromium.org/1006453002/diff/20001/chrome/common/extension...
chrome/common/extensions/permissions/chrome_permission_message_provider.h:28: //
PermissionMessageProvider implementation.
On 2015/03/12 20:55:57, Yoyo Zhou wrote:
> Add some comments here about the legacy-ness of various methods here. If
someone
> wants to add some new code using permissions, how do they know what interface
to
> use, while we have both the old and new systems?
> 
> (Even though your next CL will probably be to switch to using the intermediate
> state, it'll help to have documentation here.)

There are already comments to that effect in permission_message_provider.h. I've
added a pointer to those and a short tl;dr here.

[Yoyo Zhou, 2015-03-13 18:08:13]

LGTM, for me at least.

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

https://codereview.chromium.org/1006453002/diff/20001/chrome/common/extension...
chrome/common/extensions/permissions/chrome_permission_message_provider.cc:30:
base::FieldTrialList::FindFullName("PermissionMessageSystem");
On 2015/03/13 10:43:09, Marc Treib wrote:
> On 2015/03/12 20:55:57, Yoyo Zhou wrote:
> > You haven't defined this field trial yet (see field_trial.h documentation).
> IMO
> > you should do so in this patch, because otherwise we're asking about a
> > nonexistent field trial. The trial can start out with 100% of users not
using
> > the new system.
> 
> My intention was to enable this through Finch. In that case, I think we
mustn't
> define the field trial in Chrome (see go/finch-guide).

Ok.

https://codereview.chromium.org/1006453002/diff/20001/chrome/common/extension...
File chrome/common/extensions/permissions/chrome_permission_message_provider.h
(right):

https://codereview.chromium.org/1006453002/diff/20001/chrome/common/extension...
chrome/common/extensions/permissions/chrome_permission_message_provider.h:28: //
PermissionMessageProvider implementation.
On 2015/03/13 10:43:09, Marc Treib wrote:
> On 2015/03/12 20:55:57, Yoyo Zhou wrote:
> > Add some comments here about the legacy-ness of various methods here. If
> someone
> > wants to add some new code using permissions, how do they know what
interface
> to
> > use, while we have both the old and new systems?
> > 
> > (Even though your next CL will probably be to switch to using the
intermediate
> > state, it'll help to have documentation here.)
> 
> There are already comments to that effect in permission_message_provider.h.
I've
> added a pointer to those and a short tl;dr here.

Ah, sorry; don't know how I missed that.

[sashab, 2015-03-13 18:26:21]

Had a proper look, and LGTM, awesome job. Great work with the transition class
too, that was really clever :)

Can't wait to see this start to get cleaned up.. :-) Maybe you should use a
common string somewhere so you remember all the places that need cleaning? Haha

https://codereview.chromium.org/1006453002/diff/40001/chrome/common/extension...
File chrome/common/extensions/permissions/chrome_permission_message_provider.h
(right):

https://codereview.chromium.org/1006453002/diff/40001/chrome/common/extension...
chrome/common/extensions/permissions/chrome_permission_message_provider.h:31: //
*Coalesced* methods.
Maybe TODO: Make the legacy/coalesced ones private?

https://codereview.chromium.org/1006453002/diff/40001/extensions/common/permi...
File extensions/common/permissions/permission_message_provider.h (right):

https://codereview.chromium.org/1006453002/diff/40001/extensions/common/permi...
extensions/common/permissions/permission_message_provider.h:21: // TODO(treib):
Remove this once we've switched to the new system.
This was an ingenious idea. Great job.

[Marc Treib, 2015-03-16 09:11:04]

Yay! Thanks for the feedback!

There should be TODO(treib)s everywhere I've added to-be-cleaned code. If I've
missed some place, do point me to it!

https://codereview.chromium.org/1006453002/diff/40001/chrome/common/extension...
File chrome/common/extensions/permissions/chrome_permission_message_provider.h
(right):

https://codereview.chromium.org/1006453002/diff/40001/chrome/common/extension...
chrome/common/extensions/permissions/chrome_permission_message_provider.h:31: //
*Coalesced* methods.
On 2015/03/13 18:26:20, sasha_b wrote:
> Maybe TODO: Make the legacy/coalesced ones private?

Good idea! I'll do that once I've switched all callers to the intermediate API.

[Marc Treib, 2015-03-16 09:12:55]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-03-16 09:12:55]

The patchset sent to the CQ was uploaded after l-g-t-m from yoz@chromium.org,
sashab@chromium.org
Link to the patchset: https://codereview.chromium.org/1006453002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2015-03-16 09:13:03]

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

[commit-bot: I haz the power, 2015-03-16 10:20:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-03-16 10:21:54]

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