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

Issue 2832813002: Drop SiteBoundCredential in favor of CredentialUserData (Closed)

Created:
3 years, 8 months ago by jdoerrie
Modified:
3 years, 8 months ago
CC:
blink-reviews, chromium-reviews, haraken, mac-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Drop SiteBoundCredential in favor of CredentialUserData This change implements dropping the SiteBoundCredential interface in favor of the CredentialUserData mixin. This change was introduced in the Credential Manager API specification with commit a43865bd8aa9842dbc8d8e688d5668c087677eac (https://github.com/w3c/webappsec-credential-management/commit/a43865bd8aa9842dbc8d8e688d5668c087677eac). Intent to Deprecate and Remove: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/X-5zjSOzLC4 R=vasilii@chromium.org,mkwst@chromium.org,jochen@chromium.org BUG=714615 Review-Url: https://codereview.chromium.org/2832813002 Cr-Commit-Position: refs/heads/master@{#467305} Committed: https://chromium.googlesource.com/chromium/src/+/c95e21cadda8ce43f68dc87d1c65d3ae39ab7850

Patch Set 1 #

Total comments: 11

Patch Set 2 : Nits #

Total comments: 5

Patch Set 3 : HTTPS Rename #

Unified diffs Side-by-side diffs Delta from patch set Stats (+66 lines, -254 lines) Patch
A + third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.https.html View 1 2 3 chunks +10 lines, -5 lines 0 comments Download
A + third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.https-expected.txt View 1 2 2 chunks +4 lines, -4 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl.html View 1 1 chunk +0 lines, -86 lines 0 comments Download
D third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl-expected.txt View 1 1 chunk +0 lines, -50 lines 0 comments Download
M third_party/WebKit/LayoutTests/platform/mac/virtual/stable/webexposed/global-interface-listing-expected.txt View 1 3 chunks +6 lines, -7 lines 0 comments Download
M third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt View 1 3 chunks +6 lines, -7 lines 0 comments Download
M third_party/WebKit/LayoutTests/virtual/service-worker-navigation-preload-disabled/webexposed/global-interface-listing-expected.txt View 1 3 chunks +6 lines, -7 lines 0 comments Download
M third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt View 1 3 chunks +6 lines, -7 lines 0 comments Download
M third_party/WebKit/Source/modules/credentialmanager/BUILD.gn View 2 chunks +2 lines, -2 lines 0 comments Download
A + third_party/WebKit/Source/modules/credentialmanager/CredentialUserData.h View 2 chunks +6 lines, -6 lines 0 comments Download
A + third_party/WebKit/Source/modules/credentialmanager/CredentialUserData.cpp View 1 chunk +2 lines, -3 lines 0 comments Download
A + third_party/WebKit/Source/modules/credentialmanager/CredentialUserData.idl View 1 1 chunk +4 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/modules/credentialmanager/FederatedCredential.h View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/credentialmanager/FederatedCredential.cpp View 1 chunk +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/credentialmanager/FederatedCredential.idl View 1 chunk +2 lines, -1 line 0 comments Download
M third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.h View 2 chunks +2 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.cpp View 1 3 chunks +3 lines, -3 lines 0 comments Download
M third_party/WebKit/Source/modules/credentialmanager/PasswordCredential.idl View 1 chunk +2 lines, -1 line 0 comments Download
D third_party/WebKit/Source/modules/credentialmanager/SiteBoundCredential.h View 1 chunk +0 lines, -29 lines 0 comments Download
D third_party/WebKit/Source/modules/credentialmanager/SiteBoundCredential.cpp View 1 chunk +0 lines, -13 lines 0 comments Download
D third_party/WebKit/Source/modules/credentialmanager/SiteBoundCredential.idl View 1 chunk +0 lines, -13 lines 0 comments Download
M third_party/WebKit/Source/modules/modules_idl_files.gni View 1 1 chunk +1 line, -1 line 0 comments Download

Dependent Patchsets:

Messages

Total messages: 31 (13 generated)
jdoerrie
Dear all, please review. This is my first adventure in this part of the code-base, ...
3 years, 8 months ago (2017-04-20 12:58:02 UTC) #3
vasilii
https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt File third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt (right): https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt#newcode4685 third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt:4685: getter name Doesn't it have setters for the same. ...
3 years, 8 months ago (2017-04-20 14:21:31 UTC) #4
vasilii
Also you can email blink-dev with an "Intent to deprecate and remove: `SiteBoundCredential`". See https://www.chromium.org/blink/removing-features
3 years, 8 months ago (2017-04-20 14:24:46 UTC) #5
jdoerrie
Thanks for the comments so far, I will follow up with an email to blink-team ...
3 years, 8 months ago (2017-04-21 08:14:57 UTC) #8
vasilii
https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/Source/modules/credentialmanager/CredentialUserData.idl File third_party/WebKit/Source/modules/credentialmanager/CredentialUserData.idl (right): https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/Source/modules/credentialmanager/CredentialUserData.idl#newcode9 third_party/WebKit/Source/modules/credentialmanager/CredentialUserData.idl:9: SecureContext, On 2017/04/21 08:14:57, jdoerrie wrote: > On 2017/04/20 ...
3 years, 8 months ago (2017-04-21 08:42:59 UTC) #9
jochen (gone - plz use gerrit)
this requires an entry on chromestatus.com and an intent to deprecate email (https://www.chromium.org/blink#TOC-Web-Platform-Changes:-Process) I'd also ...
3 years, 8 months ago (2017-04-21 08:58:13 UTC) #10
Mike West
https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl.html File third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl.html (right): https://codereview.chromium.org/2832813002/diff/1/third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl.html#newcode18 third_party/WebKit/LayoutTests/http/tests/credentialmanager/idl.html:18: USVString iconURL; Either in this patch or a subsequent ...
3 years, 8 months ago (2017-04-24 07:34:08 UTC) #11
jdoerrie
Dear all, please take another look. I updated the referenced bug to be the OWP ...
3 years, 8 months ago (2017-04-24 15:05:45 UTC) #13
vasilii
https://codereview.chromium.org/2832813002/diff/20001/third_party/WebKit/LayoutTests/external/wpt/credential-management/idl-expected.txt File third_party/WebKit/LayoutTests/external/wpt/credential-management/idl-expected.txt (right): https://codereview.chromium.org/2832813002/diff/20001/third_party/WebKit/LayoutTests/external/wpt/credential-management/idl-expected.txt#newcode45 third_party/WebKit/LayoutTests/external/wpt/credential-management/idl-expected.txt:45: PASS FederatedCredential interface: new FederatedCredential({ id: "id", provider: "https://example.com", ...
3 years, 8 months ago (2017-04-24 16:46:26 UTC) #14
Mike West
https://codereview.chromium.org/2832813002/diff/20001/third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.html File third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.html (right): https://codereview.chromium.org/2832813002/diff/20001/third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.html#newcode1 third_party/WebKit/LayoutTests/external/wpt/credential-management/idl.html:1: <!DOCTYPE html> Rename this file to `idl.https.html`; otherwise it's ...
3 years, 8 months ago (2017-04-25 07:39:20 UTC) #15
Mike West
Also, nit: Please link to the intent thread in the CL description. :)
3 years, 8 months ago (2017-04-25 07:39:55 UTC) #16
jdoerrie
Next Round :) I also added the intent thread to the description. https://codereview.chromium.org/2832813002/diff/20001/third_party/WebKit/LayoutTests/external/wpt/credential-management/idl-expected.txt File third_party/WebKit/LayoutTests/external/wpt/credential-management/idl-expected.txt ...
3 years, 8 months ago (2017-04-25 09:43:38 UTC) #20
Mike West
Code change LGTM, but note that you'll still need jochen@, et al. to stamp the ...
3 years, 8 months ago (2017-04-25 11:10:28 UTC) #21
vasilii
lgtm
3 years, 8 months ago (2017-04-25 11:31:13 UTC) #24
jochen (gone - plz use gerrit)
(waiting for the intent thread to be resolved)
3 years, 8 months ago (2017-04-25 15:15:59 UTC) #25
jochen (gone - plz use gerrit)
lgtm
3 years, 8 months ago (2017-04-26 10:07:23 UTC) #26
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/2832813002/40001
3 years, 8 months ago (2017-04-26 10:58:39 UTC) #28
commit-bot: I haz the power
3 years, 8 months ago (2017-04-26 12:35:14 UTC) #31
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/c95e21cadda8ce43f68dc87d1c65...

Powered by Google App Engine
This is Rietveld 408576698