Modify experimental identity flow to display scope descriptions and details.

Modify experimental identity flow to display scope descriptions and details.

Only desktop Linux should be affected for now.

largely based on patch by jstritar

BUG=130206
TEST=manual

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

[Evan Stade, 2012-06-22 00:46:56]

this is mostly just jstritar's patch unbitrotted with a bare bones GTK
implementation for the initial prompt (no re-prompt).

http://codereview.chromium.org/10630021/diff/1/chrome/browser/extensions/api/...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

http://codereview.chromium.org/10630021/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/identity/identity_api.cc:60:
OAuth2MintTokenFlow::MODE_MINT_TOKEN_NO_FORCE));
so, correct the parts that are wrong, but this is what I figure:

In cases where the user is logged in, but regardless of whether they've ever
seen an OAuth2 permissions dialog, using FORCE here silently records the grant
and mints a token without prompting the user. Obviously we want to prompt the
user, so we want NO_FORCE on all platforms, but only after we have implemented
[re]prompting the user for the grant.

http://codereview.chromium.org/10630021/diff/1/chrome/browser/extensions/api/...
chrome/browser/extensions/api/identity/identity_api.cc:96: // TODO(estade): need
to prompt the user for scope permissions.
It's not clear to me we'd want to prompt automatically. This could lead to some
annoying behavior (e.g. a background script tries to grab a token, but is
blocked, and the user is interrupted while they are trying to do something
else). I'm more inclined to make the runtime prompting be available via a
different API call that can only be triggered by a user action.

http://codereview.chromium.org/10630021/diff/1/chrome/browser/ui/gtk/extensio...
File chrome/browser/ui/gtk/extensions/extension_install_dialog_gtk.cc (right):

http://codereview.chromium.org/10630021/diff/1/chrome/browser/ui/gtk/extensio...
chrome/browser/ui/gtk/extensions/extension_install_dialog_gtk.cc:250: "The app
wants these scopes:");
obviously this should be localized. The design doc didn't give a definitive
string. Any opinions? Should we ping Glen?

[Evan Stade, 2012-06-22 23:00:54]

ping on this. If it's important for i/o I'd like to get your feedback sooner
rather than later.

[jstritar, 2012-06-25 15:10:35]

This is great, thanks! Can you take a screenshot of the prompt in action?

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/a...
chrome/browser/extensions/api/identity/identity_api.cc:85: // Existing grant was
revoked, so we got info back instead.
Are you sure OnIssueAdviceSuccess is called when the grants are revoked? vs
OnMintTokenFailure?

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_prompt.cc (right):

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.cc:415: prompt_type_ ==
BUNDLE_INSTALL_PROMPT) {
We should probably use the fallback behavior for the INLINE_INSTALL_PROMPT too,
since it has a custom UI.

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_prompt.h (right):

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.h:244: void
FetchOAuthIssuesIfNeeded();
Nice, glad you moved this in here... seems like the right place.

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_ui_default.h (right):

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_ui_default.h:15: class
ExtensionInstallUIDefault : public ExtensionInstallUI {
If you're familiar with this class, can you add a short class-level comment? I
think this was refactored since I worked on the project.

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/p...
File chrome/browser/extensions/permissions_updater.cc (right):

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/p...
chrome/browser/extensions/permissions_updater.cc:88:
extension->GetActivePermissions();
I guess this section is kind of awkward since GetActivePermissions is returning
the scopes before they've actually been granted, but seems okay to me.

[Evan Stade, 2012-06-25 20:31:42]

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/a...
chrome/browser/extensions/api/identity/identity_api.cc:85: // Existing grant was
revoked, so we got info back instead.
On 2012/06/25 15:10:35, jstritar wrote:
> Are you sure OnIssueAdviceSuccess is called when the grants are revoked? vs
> OnMintTokenFailure?

from my testing, that seems to be the case (although unintuitive). Presumably
this is done so that a new prompt can be shown with the given issues.

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_prompt.cc (right):

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.cc:415: prompt_type_ ==
BUNDLE_INSTALL_PROMPT) {
On 2012/06/25 15:10:35, jstritar wrote:
> We should probably use the fallback behavior for the INLINE_INSTALL_PROMPT
too,
> since it has a custom UI.

Done.

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_ui_default.h (right):

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_ui_default.h:15: class
ExtensionInstallUIDefault : public ExtensionInstallUI {
On 2012/06/25 15:10:35, jstritar wrote:
> If you're familiar with this class, can you add a short class-level comment? I
> think this was refactored since I worked on the project.

yes it was (it was the biggest source of sync conflicts), and no I'm not
particularly familiar with it.

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/p...
File chrome/browser/extensions/permissions_updater.cc (right):

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/p...
chrome/browser/extensions/permissions_updater.cc:88:
extension->GetActivePermissions();
On 2012/06/25 15:10:35, jstritar wrote:
> I guess this section is kind of awkward since GetActivePermissions is
returning
> the scopes before they've actually been granted, but seems okay to me.

yes, as I understand it "active" means something like "requested".

[Munjal (Google), 2012-06-25 20:40:09]

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/a...
chrome/browser/extensions/api/identity/identity_api.cc:85: // Existing grant was
revoked, so we got info back instead.
On 2012/06/25 15:10:35, jstritar wrote:
> Are you sure OnIssueAdviceSuccess is called when the grants are revoked? vs
> OnMintTokenFailure?

I think this is fine. If the grant is revoked out of band then the servers will
return "issue adivce" for the scopes in the request in NO_FORCE mode.

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_prompt.h (right):

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.h:244: void
FetchOAuthIssuesIfNeeded();
Nit: Can we name this FetchOAuthIssueAdviceIfNeeded?

"Issues" is a misnomer here. What we are fetching are messages we should show to
the user to get the user's consent. That is called "issue advice" by the oauth
team.

http://codereview.chromium.org/10630021/diff/5001/chrome/common/net/gaia/oaut...
File chrome/common/net/gaia/oauth2_mint_token_flow.h (right):

http://codereview.chromium.org/10630021/diff/5001/chrome/common/net/gaia/oaut...
chrome/common/net/gaia/oauth2_mint_token_flow.h:58: public
base::RefCounted<OAuth2MintTokenFlow> {
Is there anyway we can avoid making this a ref counted class (assuming it forces
consumers of this class to always use scoped_refptr)?

[Evan Stade, 2012-06-26 00:56:03]

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_install_prompt.h (right):

http://codereview.chromium.org/10630021/diff/5001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_install_prompt.h:244: void
FetchOAuthIssuesIfNeeded();
On 2012/06/25 20:40:09, munjal wrote:
> Nit: Can we name this FetchOAuthIssueAdviceIfNeeded?
> 
> "Issues" is a misnomer here. What we are fetching are messages we should show
to
> the user to get the user's consent. That is called "issue advice" by the oauth
> team.

changed here and elsewhere.

http://codereview.chromium.org/10630021/diff/5001/chrome/common/net/gaia/oaut...
File chrome/common/net/gaia/oauth2_mint_token_flow.h (right):

http://codereview.chromium.org/10630021/diff/5001/chrome/common/net/gaia/oaut...
chrome/common/net/gaia/oauth2_mint_token_flow.h:58: public
base::RefCounted<OAuth2MintTokenFlow> {
On 2012/06/25 20:40:09, munjal wrote:
> Is there anyway we can avoid making this a ref counted class (assuming it
forces
> consumers of this class to always use scoped_refptr)?

changed to use weak ptrs.

http://codereview.chromium.org/10630021/diff/11002/chrome/browser/ui/gtk/exte...
File chrome/browser/ui/gtk/extensions/extension_install_dialog_gtk.cc (right):

http://codereview.chromium.org/10630021/diff/11002/chrome/browser/ui/gtk/exte...
chrome/browser/ui/gtk/extensions/extension_install_dialog_gtk.cc:255: //
TODO(estade): display the issue details under zippies.
as this TODO points out, it doesn't yet actually display the details, just the
top level descriptions. I would post a screenshot, but I'm currently getting
Connection failed (101) errors.

[Munjal (Google), 2012-06-26 05:10:17]

lgtm

[jstritar, 2012-06-26 17:57:30]

LGTM. Can you add a test to verify the grants are only recorded if the issues
are fetched?

[Evan Stade, 2012-06-26 22:06:52]

I added the requested test. I also changed the new UI + flow to be hidden behind
a flag so that the behavior wouldn't differ between platforms.

[commit-bot: I haz the power, 2012-06-27 05:00:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/10630021/24001

[commit-bot: I haz the power, 2012-06-27 06:32:22]

Failed to apply patch for chrome/browser/extensions/extension_prefs.cc:
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/extension_prefs.cc
Hunk #1 FAILED at 930.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/extensions/extension_prefs.cc.rej