Start implementing an auth flow for platform apps to be able to do auth

BUG=124493
Implementation of a new API, launchAuthFlow, that allows the apps
to perform any auth protocol with any provider.

- Add the API boilerplate (patches from Jon's CL)
- Create a new ExtensionAuthFlow class that encapsulates the
  WebContents / WebView / Widget code to perform the auth flow
- Use ExtensionAuthFlow in the API implementation.

Note that we currently allow two URL formats for redirect back
from the provider:
1. chrome-extension://<extension-id>/
2. https://<extension-id>.ext.chromium.org/

We would ideally want just 1, but providers don't currently allow such a scheme.
So we allow 2 as a workaround and if/when providers add support for 1, we can
remove 2.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=138039

[sky, 2012-04-27 18:27:38]

https://chromiumcodereview.appspot.com/10178020/diff/1/chrome/browser/extensi...
File chrome/browser/extensions/api/identity/extension_auth_flow.h (right):

https://chromiumcodereview.appspot.com/10178020/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/identity/extension_auth_flow.h:75:
scoped_ptr<views::WebView> web_view_;
Views are normally owned by their parent. So, as long as you only create this
when showing in a widget you don't need a scoped_ptr. Alternatively you can use
a scoped_ptr but invoke set_parent_owned(false).

https://chromiumcodereview.appspot.com/10178020/diff/1/chrome/browser/extensi...
chrome/browser/extensions/api/identity/extension_auth_flow.h:76:
scoped_ptr<views::Widget> widget_;
If you want to own the widget like this you need to create it with the InitParam
WIDGET_OWNS_NATIVE_WIDGET.

[Munjal (Google), 2012-05-02 18:03:21]

John, can you review the usage of WebContents / WebContentsDelegate / Widget /
WidgetDelegate parts in extension_auth_flow.h/.cc?

Please note that I am starting to write unit tests now and hence they are
missing from the patch. But I don't intend to commit that way. I still wanted to
send this out to get feedback on the whole approach.

Couple of details I haven't fully figured out:
- Best position for the popup that shows up. Currently it does not appear to be
centered on screen or in Chrome window.
- Detecting gesture when API is called. We probably don't want to show a popup
when API is called without a gesture.

I plan to tackle that in separate patches.

[Munjal (Google), 2012-05-02 18:03:59]

Also, I already tried this with Google and Facebook OAuth2 endpoints and both
work great.

[jam, 2012-05-03 17:17:51]

On 2012/05/02 18:03:21, munjal wrote:
> John, can you review the usage of WebContents / WebContentsDelegate / Widget /
> WidgetDelegate parts in extension_auth_flow.h/.cc?

I quickly looked through the usage of WC/WCD (I'm not that familiar with Widget)
and it looked fine. There are plenty of examples of WC usage in Chrome (see
http://code.google.com/p/chromium/source/search?q=file%3Achrome+WebContents%3...),
so if you based it off these that should be fine.

[Munjal (Google), 2012-05-03 19:32:42]

Mihai, can you please take a look?

On Thu, May 3, 2012 at 10:17 AM, <jam@chromium.org> wrote:

> On 2012/05/02 18:03:21, munjal wrote:
>
>> John, can you review the usage of WebContents / WebContentsDelegate /
>> Widget /
>> WidgetDelegate parts in extension_auth_flow.h/.cc?
>>
>
> I quickly looked through the usage of WC/WCD (I'm not that familiar with
> Widget)
> and it looked fine. There are plenty of examples of WC usage in Chrome (see
> http://code.google.com/p/**chromium/source/search?q=file%**
> 3Achrome+WebContents%3A%**3ACreate&origq=file%3Achrome+**
>
WebContents%3A%3ACreate&btnG=**Search+Trunk<http://code.google.com/p/chromium/source/search?q=file%3Achrome+WebContents%3A%3ACreate&origq=file%3Achrome+WebContents%3A%3ACreate&btnG=Search+Trunk>
> ),
> so if you based it off these that should be fine.
>
>
http://codereview.chromium.**org/10178020/<http://codereview.chromium.org/101...
>

[Mihai Parparita -not on Chrome, 2012-05-04 22:37:18]

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
File chrome/browser/extensions/api/identity/extension_auth_flow.cc (right):

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.cc:28:
"https://%s.ext.chromium.org/";
I would prefer not use chromium.org, since it's still a valid domain. How about
chromiumapp.org?

Also (in a separate CL), there should be an API method for generating these
callback URLs, so that app authors don't have to be aware of the exact formats.
Something like chrome.identity.generateCallbackUrl with an optional "generate
HTTPS URL" boolean param.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.cc:30: static void
PopulateValidRedirectUrlPrefixes(
Not sure I see the benefit of making this into a static function (vs. a private
one that doesn't need to be passed in extension_id_  and valid_prefixes_).

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.cc:51:
extension_id_(extension_id),
The extension_id_ member is otherwise unused, you could just pass in
extension_id to PopulateValidRedirectUrlPrefixes.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.cc:99: // we are done
with the flow.
I don't think this is quite true. If I'm not signed into a service, I might go
through several pages in the flow window before ending up at the callback URL.

You can perhaps let the client of this class (and thus the app developer) pass
in a set of URLPatterns that shouldn't trigger response, but should let the flow
keep going. That way, if I'm doing a Facebook flow, I would say that
https://facebook.com/* is OK, but if the user somehow clicks on a non-Facebook
link, then the flow should be aborted.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.cc:104: result_ =
url;
I don't think result_ should be a member, instead you can just pass the result
into ReturnResult as a parameter.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.cc:119:
widget_->CenterWindow(gfx::Size(1024, 768));
TODO about making this configurable?

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.cc:120:
widget_->Show();
So we only show the window once the initial URL has loaded? It seems like we'd
want some kind of progress indicator as soon as the API is called.

This can be a TODO, but consider something like showing the window straightaway,
filled with a "Loading.." message, and swapping that with the WebView once it
loads.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
File chrome/browser/extensions/api/identity/extension_auth_flow.h (right):

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.h:13: #include
"content/public/browser/web_contents.h"
Seems like you can just forward-declare WebContents, since you only use pointers
to it in this file.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.h:16: #include
"ui/views/controls/webview/webview.h"
Ditto for WebView and Widget.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.h:16: #include
"ui/views/controls/webview/webview.h"
Also, this is views-only code, so you'll need to have some mechanisms for
supporting this in a cross-platform way.

It's fine for this CL to only implement the flow on Windows, but I believe the
code as is will not even build on Linux or Mac.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.h:29: // Given a
provider URL, load the URL and perform usual web navigation
To make this clearer, perhaps mention that the provider URL needs to indicate to
the provider a specific style of callback URL, so that this flow can intercept
it.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.h:33: class
ExtensionAuthFlow : public content::WebContentsDelegate,
"ExtensionAuthFlow" isn't that descriptive. GenericDelegatedAuthFlow perhaps?
(it's already in the extensions namespace, so need for that in the name).

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.cc:11: #include
"chrome/browser/tab_contents/tab_util.h"
All of these #includes that got added seem to be unused.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.cc:102: AddRef();
Add comments about this being balanced in OnAuthFlowCompleted|Failed, and
vice-versa.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.cc:106: void
LaunchAuthFlowFunction::OnAuthFlowCompleted(
Nit: The token minting delegate uses the names On*Succes/Failure, but this flow
uses On*Completed/Failed. Consistency seems like a good idea.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.h:20: class WebContents;
This is unused.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/test/data/e...
File chrome/test/data/extensions/api_test/identity/test.js (right):

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/test/data/e...
chrome/test/data/extensions/api_test/identity/test.js:17: {url:
"https://some/url"},
This test won't actually work right now, right?

You can probably use the redirect helper handlers in the test server
(http://code.google.com/searchframe#OAMlx_jo-ck/src/net/tools/testserver/tests...)
to implement it though.

[Avi (use Gerrit), 2012-05-07 23:31:12]

My advice:

- File a bug for this feature.
- Put the link to the spec as well as the explanatory text into the bug. Link
the bug here as a BUG= link.
- Write up a spec specifying the UI that you want to implement that will achieve
your goals. Email the spec to chrome-ui-leads@ and iterate. Get approval for the
UI you want.
- Find UI experts on the platforms who can help you design what you want.

That last step is where I'll be useful; I can't be of much help before that.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
File chrome/browser/extensions/api/identity/extension_auth_flow.cc (right):

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.cc:118: widget_ =
Widget::CreateWindow(this);
This is just a floating window, popup-like without being a popup?

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
File chrome/browser/extensions/api/identity/extension_auth_flow.h (right):

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.h:78: Delegate*
delegate_;
What is the ownership story here? Make sure to use scoped_ptr as appropriate.

[Munjal (Google), 2012-05-08 19:26:35]

Thanks for a thorough review.

Addressed most of the comments, except one thing: use of views makes this
Windows/ChromeOS specific. Still figuring out details on how to address that.
But wanted to publish replies so far.

Avi, I added the bug number. Jordan is going to add screenshots to the bug soon.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
File chrome/browser/extensions/api/identity/extension_auth_flow.cc (right):

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.cc:28:
"https://%s.ext.chromium.org/";
On 2012/05/04 22:37:18, Mihai Parparita wrote:
> I would prefer not use http://chromium.org, since it's still a valid domain.
How about
> chromiumapp.org?
> 
> Also (in a separate CL), there should be an API method for generating these
> callback URLs, so that app authors don't have to be aware of the exact
formats.
> Something like chrome.identity.generateCallbackUrl with an optional "generate
> HTTPS URL" boolean param.

Done.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.cc:30: static void
PopulateValidRedirectUrlPrefixes(
On 2012/05/04 22:37:18, Mihai Parparita wrote:
> Not sure I see the benefit of making this into a static function (vs. a
private
> one that doesn't need to be passed in extension_id_  and valid_prefixes_).

Done.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.cc:51:
extension_id_(extension_id),
On 2012/05/04 22:37:18, Mihai Parparita wrote:
> The extension_id_ member is otherwise unused, you could just pass in
> extension_id to PopulateValidRedirectUrlPrefixes.

Done.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.cc:51:
extension_id_(extension_id),
On 2012/05/04 22:37:18, Mihai Parparita wrote:
> The extension_id_ member is otherwise unused, you could just pass in
> extension_id to PopulateValidRedirectUrlPrefixes.

Done.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.cc:99: // we are done
with the flow.
On 2012/05/04 22:37:18, Mihai Parparita wrote:
> I don't think this is quite true. If I'm not signed into a service, I might go
> through several pages in the flow window before ending up at the callback URL.
> 
> You can perhaps let the client of this class (and thus the app developer) pass
> in a set of URLPatterns that shouldn't trigger response, but should let the
flow
> keep going. That way, if I'm doing a Facebook flow, I would say that
> https://facebook.com/* is OK, but if the user somehow clicks on a non-Facebook
> link, then the flow should be aborted.

I put in some thought into this but not enough to decide what to do exactly
here.

The way I have it here, it works fine if the user is not logged in etc. as long
as the URLs being loaed are in the same domain as the provider. I already tested
it with Google and Facebook.

But what you said is probably useful to do in cases where the app is
communicating with its own server to do auth and then the app server can
delegate it to another provider to do th real sign in. In fact, this is what
Google tells mobile app developers to do auth. So I changed the logis to as
follows:
- Get done with the flow when either the user closes the window OR
- Redirect to a valid extension redirect URL happens.

I think the complexity of letting the developer specify a list of URLs is
probably not worth it.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.cc:104: result_ =
url;
On 2012/05/04 22:37:18, Mihai Parparita wrote:
> I don't think result_ should be a member, instead you can just pass the result
> into ReturnResult as a parameter.

Done.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.cc:118: widget_ =
Widget::CreateWindow(this);
On 2012/05/07 23:31:13, Avi wrote:
> This is just a floating window, popup-like without being a popup?

I am not aware of what is the difference between a floating window and a popup.
Regardless of terminology, this seems to produce the UI we need (on Windows).

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.cc:119:
widget_->CenterWindow(gfx::Size(1024, 768));
On 2012/05/04 22:37:18, Mihai Parparita wrote:
> TODO about making this configurable?

Done.
I suppose you meant the size of the window.
By configurable do you mean the app can specify it?

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.cc:120:
widget_->Show();
On 2012/05/04 22:37:18, Mihai Parparita wrote:
> So we only show the window once the initial URL has loaded? It seems like we'd
> want some kind of progress indicator as soon as the API is called.
> 
> This can be a TODO, but consider something like showing the window
straightaway,
> filled with a "Loading.." message, and swapping that with the WebView once it
> loads.

Yes, this is actually intentional. We will not show any UI if the auth flow
results in a silent redirect back to the app (in cases when the user is signed
in and the approval is already done). If we show "Loading" UI, then in such
cases the Loading window will come up and go away, which can be very jarring.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
File chrome/browser/extensions/api/identity/extension_auth_flow.h (right):

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.h:13: #include
"content/public/browser/web_contents.h"
On 2012/05/04 22:37:18, Mihai Parparita wrote:
> Seems like you can just forward-declare WebContents, since you only use
pointers
> to it in this file.

Done. I started out using scoped_ptr (which I think requires full definition).
Don't need scoped_ptr anymore. Good catch.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.h:16: #include
"ui/views/controls/webview/webview.h"
On 2012/05/04 22:37:18, Mihai Parparita wrote:
> Ditto for WebView and Widget.

Done.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.h:16: #include
"ui/views/controls/webview/webview.h"
On 2012/05/04 22:37:18, Mihai Parparita wrote:
> Also, this is views-only code, so you'll need to have some mechanisms for
> supporting this in a cross-platform way.
> 
> It's fine for this CL to only implement the flow on Windows, but I believe the
> code as is will not even build on Linux or Mac.

I didn't realize I am using Windows / ChromeOS specific UI classes. Do you have
an example of some apps V2 code that does this correctly that I can look at?

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.h:29: // Given a
provider URL, load the URL and perform usual web navigation
On 2012/05/04 22:37:18, Mihai Parparita wrote:
> To make this clearer, perhaps mention that the provider URL needs to indicate
to
> the provider a specific style of callback URL, so that this flow can intercept
> it.

Actually I added a TODo to add a link to the design doc since I don't want to
repeat everything in the design doc here.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.h:33: class
ExtensionAuthFlow : public content::WebContentsDelegate,
On 2012/05/04 22:37:18, Mihai Parparita wrote:
> "ExtensionAuthFlow" isn't that descriptive. GenericDelegatedAuthFlow perhaps?
> (it's already in the extensions namespace, so need for that in the name).

How about WebAuthFlow?

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.h:78: Delegate*
delegate_;
On 2012/05/07 23:31:13, Avi wrote:
> What is the ownership story here? Make sure to use scoped_ptr as appropriate.

Caller owns it. Added comment to the constructor (where the caller passes in the
delegate).

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.cc:102: AddRef();
On 2012/05/04 22:37:18, Mihai Parparita wrote:
> Add comments about this being balanced in OnAuthFlowCompleted|Failed, and
> vice-versa.

Done.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.cc:106: void
LaunchAuthFlowFunction::OnAuthFlowCompleted(
On 2012/05/04 22:37:18, Mihai Parparita wrote:
> Nit: The token minting delegate uses the names On*Succes/Failure, but this
flow
> uses On*Completed/Failed. Consistency seems like a good idea.

Done.

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.h (right):

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.h:20: class WebContents;
On 2012/05/04 22:37:18, Mihai Parparita wrote:
> This is unused.

Done.

[Avi (use Gerrit), 2012-05-08 20:03:55]

On 2012/05/08 19:26:35, munjal wrote:
> Still figuring out details on how to address that.
> But wanted to publish replies so far.

I think you misunderstand me. Thanks for addressing the issues, but please stop
work on this. This is new UI. You need UI approval. There's no point in working
further on UI before you get approval for the UI approach you're taking.

[Avi (use Gerrit), 2012-05-08 21:23:52]

I was waaay too harsh here; I'm sorry. I didn't fully understand the context
here that this is in an area where the UI is evolving, and I'm no longer worried
about shipping it as-is.

As we discussed, ShellWindow is a good model to work from. Let me know if you
have specific Mac questions that I can help you with.

It was good to meet you, Munjal; good luck.

[Mihai Parparita -not on Chrome, 2012-05-08 21:33:30]

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
File chrome/browser/extensions/api/identity/extension_auth_flow.h (right):

https://chromiumcodereview.appspot.com/10178020/diff/26003/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.h:33: class
ExtensionAuthFlow : public content::WebContentsDelegate,
On 2012/05/08 19:26:35, munjal wrote:
> How about WebAuthFlow?

Sounds good.

https://chromiumcodereview.appspot.com/10178020/diff/34002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/extension_auth_flow.h (right):

https://chromiumcodereview.appspot.com/10178020/diff/34002/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.h:12: #include
"base/memory/scoped_ptr.h"
You can remove this.

https://chromiumcodereview.appspot.com/10178020/diff/34002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://chromiumcodereview.appspot.com/10178020/diff/34002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.cc:16: using
content::WebContents;
These two using statements can be removed.

[Munjal (Google), 2012-05-11 18:53:32]

Ok finally I am done with tests and making sure it compiles on non views
platforms.

Note that I ended up creating the window abstraction since putting ifdefs in the
WebAuthFlow class was very ugly since the view classes were used a lot there.

Also, for now I don't have linux/mac implementations of the window abstraction.
On those platforms I return NULL for CreateWindow and handle that in WebAuthFlow

https://chromiumcodereview.appspot.com/10178020/diff/34002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/extension_auth_flow.h (right):

https://chromiumcodereview.appspot.com/10178020/diff/34002/chrome/browser/ext...
chrome/browser/extensions/api/identity/extension_auth_flow.h:12: #include
"base/memory/scoped_ptr.h"
On 2012/05/08 21:33:30, Mihai Parparita wrote:
> You can remove this.

Done.

https://chromiumcodereview.appspot.com/10178020/diff/34002/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://chromiumcodereview.appspot.com/10178020/diff/34002/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.cc:16: using
content::WebContents;
On 2012/05/08 21:33:30, Mihai Parparita wrote:
> These two using statements can be removed.

Done.

[Mihai Parparita -not on Chrome, 2012-05-14 20:59:39]

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.cc:89: AddRef();  //
Balanced in OnAuthFlowSuccess/Failed.
Nit: "Failure" instead of "Failed"

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ui/...
File chrome/browser/ui/extensions/web_auth_flow_window.cc (right):

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ui/...
chrome/browser/ui/extensions/web_auth_flow_window.cc:12: WebAuthFlowWindow*
WebAuthFlowWindow::Create(
If all this does is call WebAuthFlowWindow::CreateWebAuthFlowWindow, then you
don't need to separate the two, you can just have WebAuthFlowWindow::Create be
the function that's implemented per-platform.

The reason why ShellWindow has the separation is because ::Create does some
massaging of parameters before calling ::CreateShellWindow

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ui/...
File chrome/browser/ui/extensions/web_auth_flow_window.h (right):

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ui/...
chrome/browser/ui/extensions/web_auth_flow_window.h:18: class WebAuthFlowWindow
{
Add a brief class-level comment indicating that this is an abstract base class
that has per-platform implementations (which are otherwise hidden).

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ui/...
File chrome/browser/ui/views/extensions/web_auth_flow_window_views.h (right):

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ui/...
chrome/browser/ui/views/extensions/web_auth_flow_window_views.h:10: #include
"ui/gfx/rect.h"
This is unused.

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/test/data/e...
File chrome/test/data/extensions/api_test/identity/test.js (right):

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/test/data/e...
chrome/test/data/extensions/api_test/identity/test.js:15: function
launchAuthFlow() {
Seems like you need to update the C++ side of this test to set up the right mock
flow.

[Munjal (Google), 2012-05-15 00:55:20]

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ext...
File chrome/browser/extensions/api/identity/identity_api.cc (right):

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ext...
chrome/browser/extensions/api/identity/identity_api.cc:89: AddRef();  //
Balanced in OnAuthFlowSuccess/Failed.
On 2012/05/14 20:59:39, Mihai Parparita wrote:
> Nit: "Failure" instead of "Failed"

Done.

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ui/...
File chrome/browser/ui/extensions/web_auth_flow_window.cc (right):

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ui/...
chrome/browser/ui/extensions/web_auth_flow_window.cc:12: WebAuthFlowWindow*
WebAuthFlowWindow::Create(
On 2012/05/14 20:59:39, Mihai Parparita wrote:
> If all this does is call WebAuthFlowWindow::CreateWebAuthFlowWindow, then you
> don't need to separate the two, you can just have WebAuthFlowWindow::Create be
> the function that's implemented per-platform.
> 
> The reason why ShellWindow has the separation is because ::Create does some
> massaging of parameters before calling ::CreateShellWindow

Done.

Yeah, thought about it when I did this but at the time I kept it thinking we
might have some non-trivial logic in future. But no point ni doing what we don't
need now. We can always add it later. So removed the redundant method.

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ui/...
File chrome/browser/ui/extensions/web_auth_flow_window.h (right):

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ui/...
chrome/browser/ui/extensions/web_auth_flow_window.h:18: class WebAuthFlowWindow
{
On 2012/05/14 20:59:39, Mihai Parparita wrote:
> Add a brief class-level comment indicating that this is an abstract base class
> that has per-platform implementations (which are otherwise hidden).

Done.

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/test/data/e...
File chrome/test/data/extensions/api_test/identity/test.js (right):

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/test/data/e...
chrome/test/data/extensions/api_test/identity/test.js:15: function
launchAuthFlow() {
On 2012/05/14 20:59:39, Mihai Parparita wrote:
> Seems like you need to update the C++ side of this test to set up the right
mock
> flow.

Oh yes, completely forgot about this file (got it from Jon's  patch).

I have not done this before. Can you point me to where the C++ side of this test
lives?

[Munjal (Google), 2012-05-15 18:20:02]

Done with everything. PTAL.

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/test/data/e...
File chrome/test/data/extensions/api_test/identity/test.js (right):

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/test/data/e...
chrome/test/data/extensions/api_test/identity/test.js:15: function
launchAuthFlow() {
On 2012/05/14 20:59:39, Mihai Parparita wrote:
> Seems like you need to update the C++ side of this test to set up the right
mock
> flow.

Done.

[Mihai Parparita -not on Chrome, 2012-05-15 23:48:30]

LGTM

[scottmg, 2012-05-16 20:27:07]

Regarding the compile error, the header is "friend class ..." so it's probably
fwd declaring another class in that scope. Just remove the 'class' I expect.

[commit-bot: I haz the power, 2012-05-16 21:43:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/munjal@chromium.org/10178020/46033

[commit-bot: I haz the power, 2012-05-16 21:43:49]

Presubmit check for 10178020-46033 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/ui

Presubmit checks took 2.7s to calculate.

[Munjal (Google), 2012-05-16 23:12:11]

Scott, can you take a look at stuff under chrome/browser/ui?

[sky, 2012-05-16 23:17:10]

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/web_auth_flow_window.cc (right):

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/web_auth_flow_window.cc:32:
WebAuthFlowWindow::~WebAuthFlowWindow() {
method order should match header.

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/web_auth_flow_window.h (right):

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/web_auth_flow_window.h:25: };
Add a protected virtual destructor to make it clear WebAuthFlowWindow doesn't
own the Delegate.

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/web_auth_flow_window.h:43: explicit
WebAuthFlowWindow(Delegate* delegate,
no explicit

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/web_auth_flow_window.h:47: Delegate* delegate_;
Style guide says no protected members.

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/web_auth_flow_window_views.cc (right):

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/web_auth_flow_window_views.cc:26:
WebAuthFlowWindowViews::~WebAuthFlowWindowViews() {
Make order match header.

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/web_auth_flow_window_views.cc:40:
CHECK(web_view_);
Use DCHECK for these.

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/web_auth_flow_window_views.h (right):

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/web_auth_flow_window_views.h:27: explicit
WebAuthFlowWindowViews(
no explicit

[Munjal (Google), 2012-05-16 23:33:43]

Scott, thanks for the quick review. All done. PTAL.

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/web_auth_flow_window.cc (right):

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/web_auth_flow_window.cc:32:
WebAuthFlowWindow::~WebAuthFlowWindow() {
On 2012/05/16 23:17:11, sky wrote:
> method order should match header.

Done.

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/web_auth_flow_window.h (right):

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/web_auth_flow_window.h:43: explicit
WebAuthFlowWindow(Delegate* delegate,
On 2012/05/16 23:17:11, sky wrote:
> no explicit

Done.

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/web_auth_flow_window.h:47: Delegate* delegate_;
On 2012/05/16 23:17:11, sky wrote:
> Style guide says no protected members.

Done.

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/web_auth_flow_window_views.cc (right):

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/web_auth_flow_window_views.cc:26:
WebAuthFlowWindowViews::~WebAuthFlowWindowViews() {
On 2012/05/16 23:17:11, sky wrote:
> Make order match header.

Done.

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/web_auth_flow_window_views.cc:26:
WebAuthFlowWindowViews::~WebAuthFlowWindowViews() {
On 2012/05/16 23:17:11, sky wrote:
> Make order match header.

Done.

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/web_auth_flow_window_views.cc:40:
CHECK(web_view_);
On 2012/05/16 23:17:11, sky wrote:
> Use DCHECK for these.

Done.

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/views/ex...
File chrome/browser/ui/views/extensions/web_auth_flow_window_views.h (right):

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/views/ex...
chrome/browser/ui/views/extensions/web_auth_flow_window_views.h:27: explicit
WebAuthFlowWindowViews(
On 2012/05/16 23:17:11, sky wrote:
> no explicit

Done.

[sky, 2012-05-17 03:39:07]

LGTM