Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(4)

Issue 2788823002: Add the Mojo implementation of authenticator.mojom's MakeCredential.

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 3 weeks ago by kpaulhamus
Modified:
18 hours, 7 minutes ago
CC:
Aaron Boodman, abarth-chromium, aczeskis, blink-reviews, blundell+watchlist_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, droger+watchlist_chromium.org, haraken, qsr+mojo_chromium.org, sdefresne+watchlist_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

This change adds the Mojo implementation of authenticator.mojom's MakeCredential. The remainder of the implementation to call U2F.register() is in a follow-up CL. BUG=664630

Patch Set 1 #

Patch Set 2 : Added webauth browser feature. Corrected optional mojom struct params. Blink rename. #

Total comments: 1

Patch Set 3 : rebase #

Patch Set 4 : Stripping out unnecessary comments #

Total comments: 1

Patch Set 5 : Removed dispose method #

Total comments: 9

Patch Set 6 : Rebase for blink rename. Updated with latest callback and mojo binding changes. Removed dependency … #

Patch Set 7 : Adding OWNERS for Source/modules/webauth #

Patch Set 8 : Rebasing to relocate WebRuntimeFeatures #

Total comments: 11

Patch Set 9 : Addressing comments #

Total comments: 22

Patch Set 10 : Addressing mkwst comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+512 lines, -89 lines) Patch
M chrome/browser/BUILD.gn View 1 2 3 4 5 6 7 2 chunks +9 lines, -0 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 8 9 3 chunks +17 lines, -0 lines 0 comments Download
A chrome/browser/webauth/DEPS View 1 chunk +3 lines, -0 lines 0 comments Download
A chrome/browser/webauth/authenticator_web_contents_manager.h View 1 2 3 4 5 6 7 8 9 1 chunk +40 lines, -0 lines 0 comments Download
A chrome/browser/webauth/authenticator_web_contents_manager.cc View 1 2 3 4 5 6 7 8 9 1 chunk +38 lines, -0 lines 0 comments Download
M components/webauth/BUILD.gn View 1 2 3 4 5 1 chunk +16 lines, -1 line 0 comments Download
A components/webauth/DEPS View 1 1 chunk +3 lines, -0 lines 0 comments Download
M components/webauth/authenticator.mojom View 1 2 3 4 5 5 chunks +17 lines, -8 lines 0 comments Download
A components/webauth/authenticator_impl.h View 1 2 3 4 5 6 7 8 1 chunk +65 lines, -0 lines 0 comments Download
A components/webauth/authenticator_impl.cc View 1 2 3 4 5 6 7 8 9 1 chunk +147 lines, -0 lines 0 comments Download
M content/child/runtime_features.cc View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 lines 0 comments Download
M content/public/app/mojo/content_browser_manifest.json View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M content/public/common/content_features.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/content_features.cc View 1 2 3 4 5 6 7 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/BUILD.gn View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/webauth/BUILD.gn View 1 2 3 4 5 1 chunk +1 line, -2 lines 0 comments Download
A third_party/WebKit/Source/modules/webauth/OWNERS View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/webauth/WebAuthentication.h View 1 2 3 4 5 2 chunks +10 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp View 1 2 3 4 5 6 7 8 9 8 chunks +126 lines, -67 lines 0 comments Download
M third_party/WebKit/Source/platform/exported/WebRuntimeFeatures.cpp View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
M third_party/WebKit/public/platform/WebRuntimeFeatures.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 37 (10 generated)
Zhiqiang Zhang (Inactive)
https://codereview.chromium.org/2788823002/diff/20001/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2788823002/diff/20001/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp#newcode209 third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:209: cleanup(); Actually, the issue is unrelated to mojo. I ...
2 months, 1 week ago (2017-04-11 19:42:50 UTC) #3
kpaulhamus
On 2017/04/11 19:42:50, Zhiqiang Zhang wrote: > https://codereview.chromium.org/2788823002/diff/20001/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp > File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): > > https://codereview.chromium.org/2788823002/diff/20001/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp#newcode209 ...
2 months, 1 week ago (2017-04-12 19:15:05 UTC) #5
kpaulhamus
@foolip, can you look at anything under third_party/WebKit/Source/modules? @dcheng, can you review components/webauth, and WebRuntimeFeatures? ...
2 months, 1 week ago (2017-04-12 19:26:11 UTC) #7
kpaulhamus
On 2017/04/12 19:26:11, kpaulhamus wrote: > @foolip, can you look at anything under third_party/WebKit/Source/modules? > ...
2 months, 1 week ago (2017-04-12 19:27:31 UTC) #8
haraken
https://codereview.chromium.org/2788823002/diff/60001/third_party/WebKit/Source/modules/webauth/WebAuthentication.h File third_party/WebKit/Source/modules/webauth/WebAuthentication.h (right): https://codereview.chromium.org/2788823002/diff/60001/third_party/WebKit/Source/modules/webauth/WebAuthentication.h#newcode32 third_party/WebKit/Source/modules/webauth/WebAuthentication.h:32: USING_PRE_FINALIZER(WebAuthentication, Dispose); What is it for? The Dispose method ...
2 months, 1 week ago (2017-04-13 01:59:40 UTC) #10
kpaulhamus
On 2017/04/13 01:59:40, haraken wrote: > https://codereview.chromium.org/2788823002/diff/60001/third_party/WebKit/Source/modules/webauth/WebAuthentication.h > File third_party/WebKit/Source/modules/webauth/WebAuthentication.h (right): > > https://codereview.chromium.org/2788823002/diff/60001/third_party/WebKit/Source/modules/webauth/WebAuthentication.h#newcode32 > ...
2 months, 1 week ago (2017-04-13 16:56:11 UTC) #11
kpaulhamus
2 months, 1 week ago (2017-04-13 17:03:41 UTC) #12
jochen
https://codereview.chromium.org/2788823002/diff/80001/components/webauth/authenticator_impl.h File components/webauth/authenticator_impl.h (right): https://codereview.chromium.org/2788823002/diff/80001/components/webauth/authenticator_impl.h#newcode27 components/webauth/authenticator_impl.h:27: AuthenticatorImpl(content::RenderFrameHost* render_frame_host); nit. explicit. should this ctor be private? ...
1 month, 4 weeks ago (2017-04-25 15:08:26 UTC) #14
kpaulhamus
https://codereview.chromium.org/2788823002/diff/80001/components/webauth/authenticator_impl.h File components/webauth/authenticator_impl.h (right): https://codereview.chromium.org/2788823002/diff/80001/components/webauth/authenticator_impl.h#newcode27 components/webauth/authenticator_impl.h:27: AuthenticatorImpl(content::RenderFrameHost* render_frame_host); On 2017/04/25 15:08:26, jochen wrote: > nit. ...
1 month, 1 week ago (2017-05-11 20:09:57 UTC) #15
foolip
I'm not really familiar with webauthn, but I see that Source/modules/webauth/OWNERS does not exist? kpaulhamus@, ...
1 month, 1 week ago (2017-05-12 07:55:51 UTC) #16
foolip
Took a very cursory look at things that interested me, will do a proper review ...
1 month, 1 week ago (2017-05-12 08:00:07 UTC) #17
jochen
+engedy, so he can get familiar with the code as well
1 month, 1 week ago (2017-05-15 05:31:09 UTC) #19
kpaulhamus
On 2017/05/15 05:31:09, jochen wrote: > +engedy, so he can get familiar with the code ...
4 weeks ago (2017-05-24 20:54:04 UTC) #21
kpaulhamus
https://codereview.chromium.org/2788823002/diff/80001/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2788823002/diff/80001/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp#newcode75 third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:75: // Step 1 of https://w3c.github.io/webauthn/#makeCredential On 2017/05/12 08:00:06, foolip ...
4 weeks ago (2017-05-24 21:08:44 UTC) #23
jochen
my main concern is that this is implementing the old API (before merging with the ...
3 weeks, 2 days ago (2017-05-30 12:03:03 UTC) #24
kpaulhamus
On 2017/05/30 12:03:03, jochen wrote: > my main concern is that this is implementing the ...
3 weeks ago (2017-05-31 20:55:55 UTC) #25
kpaulhamus
https://codereview.chromium.org/2788823002/diff/140001/chrome/browser/chrome_content_browser_client.cc File chrome/browser/chrome_content_browser_client.cc (left): https://codereview.chromium.org/2788823002/diff/140001/chrome/browser/chrome_content_browser_client.cc#oldcode1693 chrome/browser/chrome_content_browser_client.cc:1693: const service_manager::BindSourceInfo& source_info, On 2017/05/30 12:03:02, jochen wrote: > ...
3 weeks ago (2017-05-31 20:56:06 UTC) #26
jochen
I don't mind landing as is. mkwst/engedy, wdyt?
2 weeks, 6 days ago (2017-06-02 11:19:38 UTC) #27
Mike West (OOO until 19th)
Sorry for the delayed response. I think engedy@ was waiting on me to respond, while ...
2 weeks, 1 day ago (2017-06-07 06:37:57 UTC) #28
kpaulhamus
On 2017/06/07 06:37:57, Mike West wrote: > Sorry for the delayed response. I think engedy@ ...
2 weeks ago (2017-06-08 05:02:35 UTC) #29
kpaulhamus
On 2017/06/07 06:37:57, Mike West wrote: > Sorry for the delayed response. I think engedy@ ...
2 weeks ago (2017-06-08 05:02:38 UTC) #30
Mike West (OOO until 19th)
On 2017/06/08 at 05:02:38, kpaulhamus wrote: > On 2017/06/07 06:37:57, Mike West wrote: > > ...
1 week, 1 day ago (2017-06-13 21:11:23 UTC) #32
kpaulhamus
Thanks for taking a look! I'm in the process of adding authenticator_impl_unittest. I'm still learning ...
1 week ago (2017-06-15 01:27:14 UTC) #33
kpaulhamus
I should clarify... I definitely understand the dilemma, and I'm totally willing to add tests. ...
1 week ago (2017-06-15 16:38:11 UTC) #34
Mike West (OOO until 19th)
Thursday was a holiday here, and I guess everyone in MUC ended up out on ...
3 days, 6 hours ago (2017-06-19 12:19:23 UTC) #35
kpaulhamus
On 2017/06/19 12:19:23, Mike West (OOO until 19th) wrote: > Thursday was a holiday here, ...
2 days, 19 hours ago (2017-06-19 23:34:41 UTC) #36
jam
18 hours, 7 minutes ago (2017-06-22 00:31:44 UTC) #37
the mojom should be in blink, any reason why it's not?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld cb946e318