Re: Start implementing an auth flow for platform apps to be able to do auth (issue 10178020)

mih...@chromium.org

unread,

May 14, 2012, 4:59:39 PM5/14/12

to mun...@chromium.org, j...@chromium.org, a...@chromium.org, chromium...@chromium.org, a...@chromium.org, mihaip...@chromium.org

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

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/extensions/api/identity/identity_api.cc#newcode89
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/extensions/web_auth_flow_window.cc
File chrome/browser/ui/extensions/web_auth_flow_window.cc (right):

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ui/extensions/web_auth_flow_window.cc#newcode12
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/extensions/web_auth_flow_window.h
File chrome/browser/ui/extensions/web_auth_flow_window.h (right):

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ui/extensions/web_auth_flow_window.h#newcode18
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/views/extensions/web_auth_flow_window_views.h
File chrome/browser/ui/views/extensions/web_auth_flow_window_views.h
(right):

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ui/views/extensions/web_auth_flow_window_views.h#newcode10
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/extensions/api_test/identity/test.js
File chrome/test/data/extensions/api_test/identity/test.js (right):

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/test/data/extensions/api_test/identity/test.js#newcode15
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.

https://chromiumcodereview.appspot.com/10178020/

mun...@chromium.org

unread,

May 14, 2012, 8:55:20 PM5/14/12

to j...@chromium.org, mih...@chromium.org, a...@chromium.org, chromium...@chromium.org, a...@chromium.org, mihaip...@chromium.org

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

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/extensions/api/identity/identity_api.cc#newcode89
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/extensions/web_auth_flow_window.cc
File chrome/browser/ui/extensions/web_auth_flow_window.cc (right):

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ui/extensions/web_auth_flow_window.cc#newcode12
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/extensions/web_auth_flow_window.h
File chrome/browser/ui/extensions/web_auth_flow_window.h (right):

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/browser/ui/extensions/web_auth_flow_window.h#newcode18
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/extensions/api_test/identity/test.js
File chrome/test/data/extensions/api_test/identity/test.js (right):

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/test/data/extensions/api_test/identity/test.js#newcode15
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?

https://chromiumcodereview.appspot.com/10178020/

mun...@chromium.org

unread,

May 15, 2012, 2:20:02 PM5/15/12

to j...@chromium.org, mih...@chromium.org, a...@chromium.org, chromium...@chromium.org, a...@chromium.org, mihaip...@chromium.org

Done with everything. PTAL.

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

https://chromiumcodereview.appspot.com/10178020/diff/39005/chrome/test/data/extensions/api_test/identity/test.js#newcode15
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.

https://chromiumcodereview.appspot.com/10178020/

mih...@chromium.org

unread,

May 15, 2012, 7:48:30 PM5/15/12

to mun...@chromium.org, j...@chromium.org, a...@chromium.org, chromium...@chromium.org, a...@chromium.org, mihaip...@chromium.org

LGTM

https://chromiumcodereview.appspot.com/10178020/

sco...@chromium.org

unread,

May 16, 2012, 4:27:07 PM5/16/12

to mun...@chromium.org, j...@chromium.org, mih...@chromium.org, a...@chromium.org, chromium...@chromium.org, a...@chromium.org, mihaip...@chromium.org

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.

http://codereview.chromium.org/10178020/

commi...@chromium.org

unread,

May 16, 2012, 5:43:30 PM5/16/12

to mun...@chromium.org, j...@chromium.org, mih...@chromium.org, a...@chromium.org, chromium...@chromium.org, a...@chromium.org, mihaip...@chromium.org

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

https://chromiumcodereview.appspot.com/10178020/

commi...@chromium.org

unread,

May 16, 2012, 5:43:50 PM5/16/12

to mun...@chromium.org, j...@chromium.org, mih...@chromium.org, a...@chromium.org, chromium...@chromium.org, a...@chromium.org, mihaip...@chromium.org

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.

https://chromiumcodereview.appspot.com/10178020/

mun...@chromium.org

unread,

May 16, 2012, 7:12:11 PM5/16/12

to j...@chromium.org, mih...@chromium.org, a...@chromium.org, s...@chromium.org, chromium...@chromium.org, a...@chromium.org, mihaip...@chromium.org

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

http://codereview.chromium.org/10178020/

s...@chromium.org

unread,

May 16, 2012, 7:17:11 PM5/16/12

to mun...@chromium.org, j...@chromium.org, mih...@chromium.org, a...@chromium.org, chromium...@chromium.org, a...@chromium.org, mihaip...@chromium.org

mun...@chromium.org

unread,

May 16, 2012, 7:33:44 PM5/16/12

to j...@chromium.org, mih...@chromium.org, a...@chromium.org, s...@chromium.org, chromium...@chromium.org, a...@chromium.org, mihaip...@chromium.org

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

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

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/extensions/web_auth_flow_window.cc#newcode32
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/extensions/web_auth_flow_window.h
File chrome/browser/ui/extensions/web_auth_flow_window.h (right):

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/extensions/web_auth_flow_window.h#newcode43
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/extensions/web_auth_flow_window.h#newcode47
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/extensions/web_auth_flow_window_views.cc
File chrome/browser/ui/views/extensions/web_auth_flow_window_views.cc
(right):

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/views/extensions/web_auth_flow_window_views.cc#newcode26
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/extensions/web_auth_flow_window_views.cc#newcode26
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/extensions/web_auth_flow_window_views.cc#newcode40
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/extensions/web_auth_flow_window_views.h
File chrome/browser/ui/views/extensions/web_auth_flow_window_views.h
(right):

http://codereview.chromium.org/10178020/diff/46033/chrome/browser/ui/views/extensions/web_auth_flow_window_views.h#newcode27
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.

http://codereview.chromium.org/10178020/

s...@chromium.org

unread,

May 16, 2012, 11:39:07 PM5/16/12

to mun...@chromium.org, j...@chromium.org, mih...@chromium.org, a...@chromium.org, chromium...@chromium.org, a...@chromium.org, mihaip...@chromium.org

LGTM

http://codereview.chromium.org/10178020/

Reply all

Reply to author

Forward