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

Issue 2713233002: Update ThreatDOMDetails to be able to collect non-resource HTML Elements based on their attributes. (Closed)

Created:
3 years, 10 months ago by lpz
Modified:
3 years, 9 months ago
CC:
chromium-reviews, grt+watch_chromium.org, vakh (use Gerrit instead)
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Update ThreatDOMDetails to be able to collect non-resource HTML Elements based on their attributes, and make sure they can be rolled up into the DOM in ThreatDetails. BUG=674618 Review-Url: https://codereview.chromium.org/2713233002 Cr-Commit-Position: refs/heads/master@{#453994} Committed: https://chromium.googlesource.com/chromium/src/+/d9d61ec69bdeb53703521c67c4292e148201f084

Patch Set 1 #

Total comments: 24

Patch Set 2 : Address comments #

Total comments: 16

Patch Set 3 : Address comments (in particular change map to vector<struct>) #

Patch Set 4 : Sync #

Unified diffs Side-by-side diffs Delta from patch set Stats (+376 lines, -94 lines) Patch
M chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc View 7 chunks +45 lines, -15 lines 0 comments Download
M chrome/browser/safe_browsing/threat_details.cc View 1 2 1 chunk +5 lines, -2 lines 0 comments Download
M chrome/browser/safe_browsing/threat_details_unittest.cc View 5 chunks +72 lines, -36 lines 0 comments Download
M chrome/renderer/safe_browsing/threat_dom_details.h View 1 2 2 chunks +21 lines, -0 lines 0 comments Download
M chrome/renderer/safe_browsing/threat_dom_details.cc View 1 2 4 chunks +151 lines, -27 lines 0 comments Download
M chrome/renderer/safe_browsing/threat_dom_details_browsertest.cc View 1 2 3 chunks +78 lines, -13 lines 0 comments Download
M chrome/test/data/safe_browsing/malware2.html View 1 chunk +4 lines, -1 line 0 comments Download

Messages

Total messages: 31 (19 generated)
lpz
3 years, 10 months ago (2017-02-24 18:09:24 UTC) #4
vakh (use Gerrit instead)
Didn't look at the tests in too much detail. LGTM overall. https://codereview.chromium.org/2713233002/diff/1/chrome/renderer/safe_browsing/threat_dom_details.cc File chrome/renderer/safe_browsing/threat_dom_details.cc (right): ...
3 years, 10 months ago (2017-02-24 19:08:38 UTC) #7
Jialiu Lin
lgtm https://codereview.chromium.org/2713233002/diff/1/chrome/renderer/safe_browsing/threat_dom_details.cc File chrome/renderer/safe_browsing/threat_dom_details.cc (right): https://codereview.chromium.org/2713233002/diff/1/chrome/renderer/safe_browsing/threat_dom_details.cc#newcode35 chrome/renderer/safe_browsing/threat_dom_details.cc:35: // This Feature specifies which HTML Elements to ...
3 years, 10 months ago (2017-02-25 00:46:18 UTC) #10
vakh (use Gerrit instead)
https://codereview.chromium.org/2713233002/diff/1/chrome/renderer/safe_browsing/threat_dom_details.cc File chrome/renderer/safe_browsing/threat_dom_details.cc (right): https://codereview.chromium.org/2713233002/diff/1/chrome/renderer/safe_browsing/threat_dom_details.cc#newcode73 chrome/renderer/safe_browsing/threat_dom_details.cc:73: for (size_t i = 0; i < split.size(); i ...
3 years, 10 months ago (2017-02-25 00:48:07 UTC) #11
lpz
Thank you for the thorough feedback! https://codereview.chromium.org/2713233002/diff/1/chrome/renderer/safe_browsing/threat_dom_details.cc File chrome/renderer/safe_browsing/threat_dom_details.cc (right): https://codereview.chromium.org/2713233002/diff/1/chrome/renderer/safe_browsing/threat_dom_details.cc#newcode35 chrome/renderer/safe_browsing/threat_dom_details.cc:35: // This Feature ...
3 years, 9 months ago (2017-02-27 15:46:44 UTC) #14
vakh (use Gerrit instead)
lgtm https://codereview.chromium.org/2713233002/diff/1/chrome/renderer/safe_browsing/threat_dom_details.cc File chrome/renderer/safe_browsing/threat_dom_details.cc (right): https://codereview.chromium.org/2713233002/diff/1/chrome/renderer/safe_browsing/threat_dom_details.cc#newcode51 chrome/renderer/safe_browsing/threat_dom_details.cc:51: std::map<std::string, std::unordered_set<std::string>>; On 2017/02/27 15:46:43, lpz wrote: > ...
3 years, 9 months ago (2017-02-27 17:24:36 UTC) #17
Nathan Parker
lgtm mostly nits and some questions. https://codereview.chromium.org/2713233002/diff/20001/chrome/browser/safe_browsing/threat_details.cc File chrome/browser/safe_browsing/threat_details.cc (right): https://codereview.chromium.org/2713233002/diff/20001/chrome/browser/safe_browsing/threat_details.cc#newcode503 chrome/browser/safe_browsing/threat_details.cc:503: if (!node.tag_name.empty()) { ...
3 years, 9 months ago (2017-02-27 23:00:36 UTC) #18
lpz
https://codereview.chromium.org/2713233002/diff/1/chrome/renderer/safe_browsing/threat_dom_details.cc File chrome/renderer/safe_browsing/threat_dom_details.cc (right): https://codereview.chromium.org/2713233002/diff/1/chrome/renderer/safe_browsing/threat_dom_details.cc#newcode51 chrome/renderer/safe_browsing/threat_dom_details.cc:51: std::map<std::string, std::unordered_set<std::string>>; On 2017/02/27 17:24:35, vakh (Varun Khaneja) wrote: ...
3 years, 9 months ago (2017-02-28 22:53:28 UTC) #21
vakh (use Gerrit instead)
lgtm https://codereview.chromium.org/2713233002/diff/1/chrome/renderer/safe_browsing/threat_dom_details.cc File chrome/renderer/safe_browsing/threat_dom_details.cc (right): https://codereview.chromium.org/2713233002/diff/1/chrome/renderer/safe_browsing/threat_dom_details.cc#newcode51 chrome/renderer/safe_browsing/threat_dom_details.cc:51: std::map<std::string, std::unordered_set<std::string>>; On 2017/02/28 22:53:28, lpz wrote: > ...
3 years, 9 months ago (2017-02-28 23:00:04 UTC) #22
Nathan Parker
lgtm https://codereview.chromium.org/2713233002/diff/20001/chrome/renderer/safe_browsing/threat_dom_details.cc File chrome/renderer/safe_browsing/threat_dom_details.cc (right): https://codereview.chromium.org/2713233002/diff/20001/chrome/renderer/safe_browsing/threat_dom_details.cc#newcode37 chrome/renderer/safe_browsing/threat_dom_details.cc:37: // list of pairs. For example: "tag1,id,tag1,height,tag2,foo" - ...
3 years, 9 months ago (2017-02-28 23:27:10 UTC) #23
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/2713233002/60001
3 years, 9 months ago (2017-03-01 17:44:21 UTC) #28
commit-bot: I haz the power
3 years, 9 months ago (2017-03-01 19:20:14 UTC) #31
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/d9d61ec69bdeb53703521c67c429...

Powered by Google App Engine
This is Rietveld 408576698