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

Issue 1417033010: Adding <keygen> Content Setting (Blink) (Closed)

Created:
5 years, 1 month ago by svaldez
Modified:
4 years, 11 months ago
CC:
blink-reviews, blink-reviews-api_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, darin-cc_chromium.org, dglazkov+blink, gavinp+loader_chromium.org, jam, Nate Chapin, jochen+watch_chromium.org, loading-reviews_chromium.org, mlamouri+watch-test-runner_chromium.org, tyoshino+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@keygen_core
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Adding <keygen> Content Setting (Blink) Adding the KEYGEN hooks into Blink to detect <keygen> usage. BUG=514767 Committed: https://crrev.com/d5d97d05af70d8a8003769e22c46dbee17a3251c Cr-Commit-Position: refs/heads/master@{#367650}

Patch Set 1 #

Total comments: 11

Patch Set 2 : Using AllowKeygen IPC. #

Total comments: 2

Patch Set 3 : Fixing comment. #

Patch Set 4 : Fixing profile_io_data use in chrome_render_message_filter #

Total comments: 9

Patch Set 5 : Clean up code. #

Total comments: 2

Patch Set 6 : Simplify Keygen check. #

Patch Set 7 : Rebase? #

Total comments: 7

Patch Set 8 : Simplifying messages. #

Total comments: 8

Patch Set 9 : More fixes. #

Patch Set 10 : Remove CRMF #

Patch Set 11 : Adding origin URL. #

Patch Set 12 : Fixing missing frame. #

Patch Set 13 : Fixing ContentBlocked calls. #

Patch Set 14 : Rebase. #

Patch Set 15 : Fix failures. #

Patch Set 16 : Fix typo. #

Patch Set 17 : Fixing nullptr. #

Patch Set 18 : Rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+42 lines, -1 line) Patch
M chrome/browser/content_settings/tab_specific_content_settings.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/content_settings/tab_specific_content_settings.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +11 lines, -0 lines 0 comments Download
M chrome/renderer/content_settings_observer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +2 lines, -1 line 0 comments Download
M chrome/renderer/content_settings_observer.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +7 lines, -0 lines 0 comments Download
M components/content_settings/content/common/content_settings_messages.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameLoaderClient.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/web/FrameLoaderClientImpl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +6 lines, -0 lines 0 comments Download
M third_party/WebKit/public/web/WebContentSettingsClient.h View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 62 (16 generated)
svaldez
5 years, 1 month ago (2015-11-03 15:38:32 UTC) #2
raymes
Not sure who is the best person to review this part (it's not me:). jochen ...
5 years, 1 month ago (2015-11-05 02:07:47 UTC) #3
Bernhard Bauer
https://codereview.chromium.org/1417033010/diff/1/chrome/renderer/content_settings_observer.cc File chrome/renderer/content_settings_observer.cc (right): https://codereview.chromium.org/1417033010/diff/1/chrome/renderer/content_settings_observer.cc#newcode366 chrome/renderer/content_settings_observer.cc:366: // allowing all keygen, it's quicker this way. This ...
5 years, 1 month ago (2015-11-05 11:12:13 UTC) #5
svaldez
https://codereview.chromium.org/1417033010/diff/1/chrome/renderer/content_settings_observer.cc File chrome/renderer/content_settings_observer.cc (right): https://codereview.chromium.org/1417033010/diff/1/chrome/renderer/content_settings_observer.cc#newcode366 chrome/renderer/content_settings_observer.cc:366: // allowing all keygen, it's quicker this way. On ...
5 years, 1 month ago (2015-11-05 16:35:20 UTC) #6
Bernhard Bauer
https://codereview.chromium.org/1417033010/diff/1/content/browser/renderer_host/render_message_filter.cc File content/browser/renderer_host/render_message_filter.cc (right): https://codereview.chromium.org/1417033010/diff/1/content/browser/renderer_host/render_message_filter.cc#newcode583 content/browser/renderer_host/render_message_filter.cc:583: if (!GetContentClient()->browser()->AllowKeygen(url, resource_context_)) { On 2015/11/05 16:35:20, svaldez wrote: ...
5 years, 1 month ago (2015-11-05 16:45:19 UTC) #7
svaldez
On 2015/11/05 16:45:19, Bernhard Bauer wrote: > https://codereview.chromium.org/1417033010/diff/1/content/browser/renderer_host/render_message_filter.cc > File content/browser/renderer_host/render_message_filter.cc (right): > > https://codereview.chromium.org/1417033010/diff/1/content/browser/renderer_host/render_message_filter.cc#newcode583 ...
5 years, 1 month ago (2015-11-05 16:59:29 UTC) #8
Bernhard Bauer
LGTM w/ a nit, but I don't own much (if anything?) here. On 2015/11/05 16:59:29, ...
5 years, 1 month ago (2015-11-05 17:17:34 UTC) #9
svaldez
https://codereview.chromium.org/1417033010/diff/20001/chrome/renderer/content_settings_observer.h File chrome/renderer/content_settings_observer.h (right): https://codereview.chromium.org/1417033010/diff/20001/chrome/renderer/content_settings_observer.h#newcode45 chrome/renderer/content_settings_observer.h:45: // |AllowScriptFromSource()|, and |AllowKeygen()|. |content_setting_rules| On 2015/11/05 17:17:33, Bernhard ...
5 years, 1 month ago (2015-11-05 17:40:12 UTC) #10
jochen (gone - plz use gerrit)
with the test runner changes, it should be possible to add a layout test, no? ...
5 years, 1 month ago (2015-11-07 05:53:29 UTC) #11
svaldez
https://codereview.chromium.org/1417033010/diff/60001/chrome/renderer/content_settings_observer.cc File chrome/renderer/content_settings_observer.cc (right): https://codereview.chromium.org/1417033010/diff/60001/chrome/renderer/content_settings_observer.cc#newcode359 chrome/renderer/content_settings_observer.cc:359: if (it != cached_keygen_permissions_.end()) On 2015/11/07 05:53:29, jochen (slow ...
5 years, 1 month ago (2015-11-10 15:23:42 UTC) #12
jochen (gone - plz use gerrit)
https://codereview.chromium.org/1417033010/diff/60001/third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp File third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp (right): https://codereview.chromium.org/1417033010/diff/60001/third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp#newcode66 third_party/WebKit/Source/core/html/HTMLKeygenElement.cpp:66: document().frame()->loader().client()->didNotAllowKeygen(); On 2015/11/10 at 15:23:41, svaldez wrote: > On ...
5 years, 1 month ago (2015-11-10 19:54:45 UTC) #13
Ryan Sleevi
On 2015/11/10 19:54:45, jochen (slow - traveling) wrote: > yes, but I don't understand why ...
5 years, 1 month ago (2015-11-10 21:49:11 UTC) #14
jochen (gone - plz use gerrit)
so you're saying that the actual blocking happens somewhere else (outside of blink?)
5 years, 1 month ago (2015-11-10 21:54:00 UTC) #15
Ryan Sleevi
On 2015/11/10 21:54:00, jochen (slow - traveling) wrote: > so you're saying that the actual ...
5 years, 1 month ago (2015-11-10 22:28:02 UTC) #16
jochen (gone - plz use gerrit)
+esprehn for some advice how we'd best ping chrome about the fact that a given ...
5 years, 1 month ago (2015-11-11 22:14:52 UTC) #18
svaldez
On 2015/11/11 22:14:52, jochen wrote: > +esprehn for some advice how we'd best ping chrome ...
5 years, 1 month ago (2015-11-17 21:54:04 UTC) #19
jochen (gone - plz use gerrit)
On 2015/11/17 at 21:54:04, svaldez wrote: > On 2015/11/11 22:14:52, jochen wrote: > > +esprehn ...
5 years, 1 month ago (2015-11-18 16:16:09 UTC) #20
svaldez
On 2015/11/18 16:16:09, jochen wrote: > On 2015/11/17 at 21:54:04, svaldez wrote: > > On ...
5 years, 1 month ago (2015-11-18 16:23:45 UTC) #21
Bernhard Bauer
On 2015/11/18 16:23:45, svaldez wrote: > On 2015/11/18 16:16:09, jochen wrote: > > On 2015/11/17 ...
5 years, 1 month ago (2015-11-18 16:44:18 UTC) #22
jochen (gone - plz use gerrit)
On 2015/11/18 at 16:44:18, bauerb wrote: > On 2015/11/18 16:23:45, svaldez wrote: > > On ...
5 years, 1 month ago (2015-11-18 16:48:12 UTC) #23
svaldez
Rebasing made the patchsets weird, but the diff from 5 to 7 should show the ...
5 years, 1 month ago (2015-11-23 20:13:50 UTC) #24
jochen (gone - plz use gerrit)
https://codereview.chromium.org/1417033010/diff/120001/chrome/browser/renderer_host/chrome_render_message_filter.cc File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/1417033010/diff/120001/chrome/browser/renderer_host/chrome_render_message_filter.cc#newcode185 chrome/browser/renderer_host/chrome_render_message_filter.cc:185: *allowed = content_settings->GetContentSetting( instead of getting the content settings ...
5 years ago (2015-11-24 13:05:08 UTC) #25
svaldez
https://codereview.chromium.org/1417033010/diff/120001/chrome/browser/renderer_host/chrome_render_message_filter.cc File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/1417033010/diff/120001/chrome/browser/renderer_host/chrome_render_message_filter.cc#newcode185 chrome/browser/renderer_host/chrome_render_message_filter.cc:185: *allowed = content_settings->GetContentSetting( On 2015/11/24 13:05:08, jochen wrote: > ...
5 years ago (2015-11-24 17:29:04 UTC) #26
jochen (gone - plz use gerrit)
https://codereview.chromium.org/1417033010/diff/140001/chrome/browser/renderer_host/chrome_render_message_filter.h File chrome/browser/renderer_host/chrome_render_message_filter.h (right): https://codereview.chromium.org/1417033010/diff/140001/chrome/browser/renderer_host/chrome_render_message_filter.h#newcode18 chrome/browser/renderer_host/chrome_render_message_filter.h:18: class ProfileIOData; changes to this file are no longer ...
5 years ago (2015-11-25 08:54:35 UTC) #27
svaldez
https://codereview.chromium.org/1417033010/diff/140001/chrome/browser/renderer_host/chrome_render_message_filter.h File chrome/browser/renderer_host/chrome_render_message_filter.h (right): https://codereview.chromium.org/1417033010/diff/140001/chrome/browser/renderer_host/chrome_render_message_filter.h#newcode18 chrome/browser/renderer_host/chrome_render_message_filter.h:18: class ProfileIOData; On 2015/11/25 08:54:34, jochen wrote: > changes ...
5 years ago (2015-11-25 14:56:55 UTC) #28
jochen (gone - plz use gerrit)
lgtm if rsleevi and/or felt confirms that we actually want to show the top-level URL ...
5 years ago (2015-11-25 15:01:00 UTC) #29
svaldez
On 2015/11/25 15:01:00, jochen wrote: > lgtm if rsleevi and/or felt confirms that we actually ...
5 years ago (2015-11-25 15:39:05 UTC) #30
jochen (gone - plz use gerrit)
On 2015/11/25 at 15:39:05, svaldez wrote: > On 2015/11/25 15:01:00, jochen wrote: > > lgtm ...
5 years ago (2015-11-26 13:25:30 UTC) #31
jochen (gone - plz use gerrit)
On 2015/11/25 at 15:39:05, svaldez wrote: > On 2015/11/25 15:01:00, jochen wrote: > > lgtm ...
5 years ago (2015-11-26 13:25:30 UTC) #32
svaldez
On 2015/11/26 13:25:30, jochen wrote: > On 2015/11/25 at 15:39:05, svaldez wrote: > > On ...
5 years ago (2015-11-30 17:26:13 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417033010/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417033010/220001
5 years ago (2015-11-30 21:18:01 UTC) #36
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/123263)
5 years ago (2015-11-30 21:46:15 UTC) #38
svaldez
Modification to content_settings_messages needs a security review.
5 years ago (2015-11-30 22:00:27 UTC) #40
svaldez
On 2015/11/30 22:00:27, svaldez wrote: > Modification to content_settings_messages needs a security review. Updates to ...
5 years ago (2015-12-01 20:05:43 UTC) #41
Will Harris
security lgtm for the messages you could consider adding a RAPPOR in TabSpecificContentSettings::OnDidUseKeygen with the ...
5 years ago (2015-12-01 22:01:59 UTC) #42
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417033010/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417033010/240001
5 years ago (2015-12-01 22:09:01 UTC) #45
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/142401)
5 years ago (2015-12-01 22:53:13 UTC) #47
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417033010/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417033010/240001
5 years ago (2015-12-09 22:27:35 UTC) #49
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/147348)
5 years ago (2015-12-10 00:22:17 UTC) #51
svaldez
So I ended up having to move the didUseKeygen call back out of the constructor ...
5 years ago (2015-12-11 16:20:37 UTC) #52
jochen (gone - plz use gerrit)
On 2015/12/11 at 16:20:37, svaldez wrote: > So I ended up having to move the ...
5 years ago (2015-12-14 12:08:41 UTC) #53
svaldez
On 2015/12/14 12:08:41, jochen wrote: > On 2015/12/11 at 16:20:37, svaldez wrote: > > So ...
5 years ago (2015-12-14 14:56:03 UTC) #54
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417033010/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417033010/320001
4 years, 11 months ago (2016-01-05 19:14:58 UTC) #56
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417033010/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417033010/340001
4 years, 11 months ago (2016-01-05 19:43:26 UTC) #59
commit-bot: I haz the power
Committed patchset #18 (id:340001)
4 years, 11 months ago (2016-01-05 21:22:51 UTC) #60
commit-bot: I haz the power
4 years, 11 months ago (2016-01-05 21:23:41 UTC) #62
Message was sent while issue was closed.
Patchset 18 (id:??) landed as
https://crrev.com/d5d97d05af70d8a8003769e22c46dbee17a3251c
Cr-Commit-Position: refs/heads/master@{#367650}

Powered by Google App Engine
This is Rietveld 408576698