Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(680)

Issue 2702653002: Patch #1 of multiple. Add webauth .mojom and initial usage of makeCredential. (Closed)

Created:
3 years, 10 months ago by kpaulhamus
Modified:
3 years, 6 months ago
CC:
Aaron Boodman, abarth-chromium, aczeskis_google.comchromium-reviews, agrieve+watch_chromium.org, Bernhard Bauer, blink-reviews, darin (slow to review), haraken, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Add webauth makeCredential mojom. Patch #1 of multiple This patch adds the webauth authenticator mojom interface. It has the function makeCredential and necessary structs. The function is used by the WebAuthentication class. BUG=664630 Review-Url: https://codereview.chromium.org/2702653002 Cr-Commit-Position: refs/heads/master@{#471498} Committed: https://chromium.googlesource.com/chromium/src/+/b1f03c340eaaf193e6d4cae7047600c0eddd5ffb

Patch Set 1 #

Patch Set 2 : Small syntax fix #

Patch Set 3 : Updated UseCounter.h #

Total comments: 2

Patch Set 4 : Fixed Java compile errors #

Patch Set 5 : Restructured to only include mojom and usage for makeCredential call #

Total comments: 28

Patch Set 6 : Addressing reviewer comments #

Total comments: 2

Patch Set 7 : Missing comma in DEPS #

Patch Set 8 : Remove counters to put in a later CL - annoying to rebase, downstream CLs, etc #

Patch Set 9 : rebase #

Patch Set 10 : Blink renaming #

Patch Set 11 : Updated based on SharedArrayBuffer changes #

Total comments: 19

Patch Set 12 : Addressing comments #

Patch Set 13 : Addressing comments #

Total comments: 9

Patch Set 14 : Capitalizing mojom interface method #

Total comments: 6

Patch Set 15 : Addressing more comments #

Total comments: 13

Patch Set 16 : Addressing more comments #

Patch Set 17 : working on updating layouttests #

Patch Set 18 : Rebaselining layout tests #

Patch Set 19 : Adding additional comments to mojom structs. #

Total comments: 8

Patch Set 20 : Rebaselining layout tests and fixes nits #

Total comments: 4

Patch Set 21 : Resolving final comments #

Patch Set 22 : Change to ChromeInterfaceRegistrar.java shouldn't be here - left over from splitting of Android imp… #

Patch Set 23 : Add Authenticator to interface provider to correct try job 'service_manager' error. #

Patch Set 24 : IDL methods should be lower-case - was failing layouttests #

Patch Set 25 : Removing obsolete idl-expected file to pass layouttests #

Unified diffs Side-by-side diffs Delta from patch set Stats (+339 lines, -16 lines) Patch
A components/webauth/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +15 lines, -0 lines 0 comments Download
A components/webauth/OWNERS View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
A components/webauth/authenticator.mojom View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +89 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/webauth/idl.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 2 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/modules/webauth/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/webauth/DEPS View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/modules/webauth/WebAuthentication.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +26 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 3 chunks +202 lines, -4 lines 0 comments Download

Messages

Total messages: 86 (46 generated)
yzshen1
https://codereview.chromium.org/2702653002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/webauth/AuthenticatorFactory.java File chrome/android/java/src/org/chromium/chrome/browser/webauth/AuthenticatorFactory.java (right): https://codereview.chromium.org/2702653002/diff/40001/chrome/android/java/src/org/chromium/chrome/browser/webauth/AuthenticatorFactory.java#newcode28 chrome/android/java/src/org/chromium/chrome/browser/webauth/AuthenticatorFactory.java:28: return null(); remove ()?
3 years, 10 months ago (2017-02-17 22:42:23 UTC) #10
kpaulhamus
Hi, can you review the following? Elliott - simple changes in Source/modules/ Bernhard - addition ...
3 years, 10 months ago (2017-02-21 17:37:47 UTC) #16
dcheng
Can we break this CL up and land mojom changes incrementally as we implement them ...
3 years, 10 months ago (2017-02-21 23:47:11 UTC) #17
kpaulhamus
On 2017/02/21 23:47:11, dcheng wrote: > Can we break this CL up and land mojom ...
3 years, 10 months ago (2017-02-22 00:25:10 UTC) #18
kpaulhamus
On 2017/02/22 00:25:10, kpaulhamus wrote: > On 2017/02/21 23:47:11, dcheng wrote: > > Can we ...
3 years, 10 months ago (2017-02-23 21:20:37 UTC) #19
dcheng
https://codereview.chromium.org/2702653002/diff/80001/components/webauth/authenticator.mojom File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/80001/components/webauth/authenticator.mojom#newcode9 components/webauth/authenticator.mojom:9: array<uint8> clientData; Nit: hacker_case instead of camelCase for variable ...
3 years, 9 months ago (2017-02-28 07:25:56 UTC) #23
ikilpatrick
I just had a quick pass.. https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2702653002/diff/80001/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp#newcode50 third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:50: ScopedCredentialType convertScopedCredentialType(const WTF::String& ...
3 years, 9 months ago (2017-02-28 19:18:18 UTC) #24
kpaulhamus
https://codereview.chromium.org/2702653002/diff/80001/components/webauth/authenticator.mojom File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/80001/components/webauth/authenticator.mojom#newcode9 components/webauth/authenticator.mojom:9: array<uint8> clientData; On 2017/02/28 07:25:56, dcheng wrote: > Nit: ...
3 years, 9 months ago (2017-03-01 01:56:35 UTC) #25
dcheng
https://codereview.chromium.org/2702653002/diff/100001/third_party/WebKit/Source/modules/webauth/DEPS File third_party/WebKit/Source/modules/webauth/DEPS (right): https://codereview.chromium.org/2702653002/diff/100001/third_party/WebKit/Source/modules/webauth/DEPS#newcode2 third_party/WebKit/Source/modules/webauth/DEPS:2: "+components/webauth" The presubmit is complaining because there's a missing ...
3 years, 9 months ago (2017-03-01 03:55:08 UTC) #26
kpaulhamus
There are some dependent CLs for the typemapping and others that show how the mojom ...
3 years, 9 months ago (2017-03-09 19:16:55 UTC) #28
dcheng
https://codereview.chromium.org/2702653002/diff/200001/components/webauth/authenticator.mojom File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/200001/components/webauth/authenticator.mojom#newcode8 components/webauth/authenticator.mojom:8: struct ScopedCredentialInfo { Nit: please add some more comments ...
3 years, 8 months ago (2017-04-20 13:04:44 UTC) #30
kpaulhamus
https://codereview.chromium.org/2702653002/diff/200001/components/webauth/authenticator.mojom File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/200001/components/webauth/authenticator.mojom#newcode8 components/webauth/authenticator.mojom:8: struct ScopedCredentialInfo { On 2017/04/20 13:04:44, dcheng (OOO through ...
3 years, 8 months ago (2017-04-22 01:04:45 UTC) #31
dcheng
https://codereview.chromium.org/2702653002/diff/200001/components/webauth/authenticator.mojom File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/200001/components/webauth/authenticator.mojom#newcode8 components/webauth/authenticator.mojom:8: struct ScopedCredentialInfo { On 2017/04/22 01:04:44, kpaulhamus wrote: > ...
3 years, 8 months ago (2017-04-24 12:25:26 UTC) #32
kpaulhamus
https://codereview.chromium.org/2702653002/diff/240001/components/webauth/authenticator.mojom File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/240001/components/webauth/authenticator.mojom#newcode54 components/webauth/authenticator.mojom:54: array<uint8> id; On 2017/04/24 12:25:26, dcheng (OOO through May ...
3 years, 8 months ago (2017-04-24 17:48:34 UTC) #33
ikilpatrick
https://codereview.chromium.org/2702653002/diff/260001/components/webauth/BUILD.gn File components/webauth/BUILD.gn (right): https://codereview.chromium.org/2702653002/diff/260001/components/webauth/BUILD.gn#newcode1 components/webauth/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights reserved. ...
3 years, 8 months ago (2017-04-24 22:11:44 UTC) #36
kpaulhamus
https://codereview.chromium.org/2702653002/diff/260001/components/webauth/BUILD.gn File components/webauth/BUILD.gn (right): https://codereview.chromium.org/2702653002/diff/260001/components/webauth/BUILD.gn#newcode1 components/webauth/BUILD.gn:1: # Copyright 2016 The Chromium Authors. All rights reserved. ...
3 years, 8 months ago (2017-04-25 00:03:08 UTC) #37
dcheng
https://codereview.chromium.org/2702653002/diff/240001/components/webauth/authenticator.mojom File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/240001/components/webauth/authenticator.mojom#newcode54 components/webauth/authenticator.mojom:54: array<uint8> id; On 2017/04/24 17:48:34, kpaulhamus wrote: > On ...
3 years, 8 months ago (2017-04-25 12:52:15 UTC) #38
kpaulhamus
Also had to add layouttest updates due to blink naming changes. https://codereview.chromium.org/2702653002/diff/280001/components/webauth/authenticator.mojom File components/webauth/authenticator.mojom (right): ...
3 years, 7 months ago (2017-05-03 17:00:46 UTC) #39
dcheng
(Sorry for the review delay, mostly caught up now) https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2702653002/diff/280001/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp#newcode70 third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:70: ...
3 years, 7 months ago (2017-05-04 07:22:35 UTC) #40
kpaulhamus
https://codereview.chromium.org/2702653002/diff/360001/components/webauth/authenticator.mojom File components/webauth/authenticator.mojom (right): https://codereview.chromium.org/2702653002/diff/360001/components/webauth/authenticator.mojom#newcode58 components/webauth/authenticator.mojom:58: // A 255-byte blob representing a credential key handle ...
3 years, 7 months ago (2017-05-05 01:32:36 UTC) #41
dcheng
mojo lgtm but... It would help code reviewers greatly to address as many comments as ...
3 years, 7 months ago (2017-05-08 21:32:47 UTC) #42
kpaulhamus
On 2017/05/08 21:32:47, dcheng wrote: > mojo lgtm but... > > It would help code ...
3 years, 7 months ago (2017-05-09 20:42:04 UTC) #43
ikilpatrick
lgtm % comments. https://codereview.chromium.org/2702653002/diff/380001/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2702653002/diff/380001/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp#newcode180 third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:180: authenticator_.reset(); nothing is needed here except ...
3 years, 7 months ago (2017-05-09 20:48:58 UTC) #45
jochen (gone - plz use gerrit)
rubberstamp lgtm
3 years, 7 months ago (2017-05-10 08:25:01 UTC) #46
kpaulhamus
https://codereview.chromium.org/2702653002/diff/380001/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp File third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp (right): https://codereview.chromium.org/2702653002/diff/380001/third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp#newcode180 third_party/WebKit/Source/modules/webauth/WebAuthentication.cpp:180: authenticator_.reset(); On 2017/05/09 20:48:58, ikilpatrick wrote: > nothing is ...
3 years, 7 months ago (2017-05-10 17:58:29 UTC) #48
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2702653002/420001
3 years, 7 months ago (2017-05-10 21:18:58 UTC) #54
dcheng
On 2017/05/09 20:42:04, kpaulhamus wrote: > On 2017/05/08 21:32:47, dcheng wrote: > > mojo lgtm ...
3 years, 7 months ago (2017-05-10 21:31:37 UTC) #55
commit-bot: I haz the power
Exhausted attempt retry quota accross all builders (set as global_retry_quota=2 in cq.cfg of this project)
3 years, 7 months ago (2017-05-10 22:45:12 UTC) #57
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2702653002/440001
3 years, 7 months ago (2017-05-11 18:37:23 UTC) #66
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/451280)
3 years, 7 months ago (2017-05-11 19:51:33 UTC) #68
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2702653002/480001
3 years, 7 months ago (2017-05-12 15:33:29 UTC) #71
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/443091)
3 years, 7 months ago (2017-05-12 17:56:23 UTC) #73
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2702653002/480001
3 years, 7 months ago (2017-05-12 18:05:18 UTC) #75
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/443315)
3 years, 7 months ago (2017-05-12 18:18:53 UTC) #77
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2702653002/480001
3 years, 7 months ago (2017-05-12 18:26:33 UTC) #79
commit-bot: I haz the power
Committed patchset #25 (id:480001) as https://chromium.googlesource.com/chromium/src/+/b1f03c340eaaf193e6d4cae7047600c0eddd5ffb
3 years, 7 months ago (2017-05-13 00:01:16 UTC) #82
jam
why was a new component added for this mojom file? Code in chrome/browser or content/browser ...
3 years, 6 months ago (2017-06-13 04:39:05 UTC) #83
kpaulhamus
On 2017/06/13 04:39:05, jam (ooo 6-14 - 6-20) wrote: > why was a new component ...
3 years, 6 months ago (2017-06-13 16:31:51 UTC) #84
jam
ping On Mon, Jun 12, 2017 at 9:39 PM, <jam@chromium.org> wrote: > why was a ...
3 years, 6 months ago (2017-06-22 00:30:28 UTC) #85
jam
3 years, 6 months ago (2017-06-22 00:33:30 UTC) #86
Message was sent while issue was closed.
On 2017/06/13 16:31:51, kpaulhamus wrote:
> On 2017/06/13 04:39:05, jam (ooo 6-14 - 6-20) wrote:
> > why was a new component added for this mojom file? Code in chrome/browser or
> > content/browser can include mojom's declared in blink, which this is.
> > 
> > Can you please move this to blink?
> 
> Hi John, I was under the impression that the mojom and implementation should
> live in components, and one reason that comes to mind is because I will be
> writing both C++ and Java implementations of the mojom (although I can't tell
> you exactly where  I heard that, so maybe I'm mistaken). I've been referencing
> payments as an example. In any case, could you take a look at
> https://codereview.chromium.org/2788823002/ as well and make sure things are
in
> the right place? Should the mojom implementation code in that CL move
somewhere
> else as well?

(ah just noticed this, for some reason I didn't get an email)

Note payments moved to blink already, there was no need for that to be in a
component.

Java/C++ doesn't affect where the mojom is.

Can you move this to webkit please?

Powered by Google App Engine
This is Rietveld 408576698