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

Issue 7976039: Change PhishingDOMFeatureExtractor to cache the WebDocument rather than a WebFrame pointer. (Closed)

Created:
9 years, 3 months ago by Brian Ryner
Modified:
9 years, 3 months ago
CC:
chromium-reviews, Paweł Hajdan Jr., darin-cc_chromium.org, brettw-cc_chromium.org, Garrett Casto
Visibility:
Public.

Description

Change PhishingDOMFeatureExtractor to cache the WebDocument rather than a WebFrame pointer. WebFrames are not guaranteed to remain valid across calls to ExtractFeaturesWithTmeout. BUG=97148 TEST=PhishingDOMFeatureExtractorTest.SubframeRemoval Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=102541

Patch Set 1 #

Total comments: 14

Patch Set 2 : address mattm's and darin's review comments #

Total comments: 2

Patch Set 3 : Address Noe's review comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+122 lines, -25 lines) Patch
M chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h View 1 3 chunks +10 lines, -5 lines 0 comments Download
M chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc View 1 2 5 chunks +36 lines, -20 lines 0 comments Download
M chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc View 1 3 chunks +76 lines, -0 lines 0 comments Download

Messages

Total messages: 16 (0 generated)
Brian Ryner
9 years, 3 months ago (2011-09-21 23:51:24 UTC) #1
mattm
http://codereview.chromium.org/7976039/diff/1/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc (right): http://codereview.chromium.org/7976039/diff/1/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc#newcode400 chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc:400: if (frame) { Does this mean that if a ...
9 years, 3 months ago (2011-09-22 02:31:38 UTC) #2
Brian Ryner
http://codereview.chromium.org/7976039/diff/1/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc (right): http://codereview.chromium.org/7976039/diff/1/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc#newcode400 chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc:400: if (frame) { On 2011/09/22 02:31:38, mattm wrote: > ...
9 years, 3 months ago (2011-09-22 02:42:45 UTC) #3
mattm
http://codereview.chromium.org/7976039/diff/1/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc (right): http://codereview.chromium.org/7976039/diff/1/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc#newcode400 chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc:400: if (frame) { On 2011/09/22 02:42:45, Brian Ryner wrote: ...
9 years, 3 months ago (2011-09-22 03:08:23 UTC) #4
Brian Ryner
http://codereview.chromium.org/7976039/diff/1/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc (right): http://codereview.chromium.org/7976039/diff/1/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc#newcode400 chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc:400: if (frame) { On 2011/09/22 03:08:23, mattm wrote: > ...
9 years, 3 months ago (2011-09-22 05:09:56 UTC) #5
noelutz
lgtm
9 years, 3 months ago (2011-09-22 16:45:50 UTC) #6
darin (slow to review)
http://codereview.chromium.org/7976039/diff/1/chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc File chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc (right): http://codereview.chromium.org/7976039/diff/1/chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc#newcode39 chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:39: method_factory_.NewRunnableMethod( nit: use base::Bind instead. http://codereview.chromium.org/7976039/diff/1/chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc#newcode46 chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc:46: ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {} ...
9 years, 3 months ago (2011-09-22 17:43:51 UTC) #7
noelutz
http://codereview.chromium.org/7976039/diff/1/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h (right): http://codereview.chromium.org/7976039/diff/1/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h#newcode135 chrome/renderer/safe_browsing/phishing_dom_feature_extractor.h:135: scoped_ptr<WebKit::WebDocument> cur_document_; On 2011/09/22 05:09:56, Brian Ryner wrote: > ...
9 years, 3 months ago (2011-09-22 21:36:20 UTC) #8
Brian Ryner
Please take another look.
9 years, 3 months ago (2011-09-23 00:53:16 UTC) #9
noelutz
http://codereview.chromium.org/7976039/diff/8001/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc (right): http://codereview.chromium.org/7976039/diff/8001/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc#newcode138 chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc:138: cur_document_ = web_view->mainFrame()->document(); Alternatively, you could also only set ...
9 years, 3 months ago (2011-09-23 01:13:15 UTC) #10
Brian Ryner
Please take another look. http://codereview.chromium.org/7976039/diff/8001/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc File chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc (right): http://codereview.chromium.org/7976039/diff/8001/chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc#newcode138 chrome/renderer/safe_browsing/phishing_dom_feature_extractor.cc:138: cur_document_ = web_view->mainFrame()->document(); On 2011/09/23 ...
9 years, 3 months ago (2011-09-23 03:24:53 UTC) #11
noelutz
lgtm Splendid. thanks, noé.
9 years, 3 months ago (2011-09-23 04:07:54 UTC) #12
darin (slow to review)
OK,LGTM
9 years, 3 months ago (2011-09-23 04:37:53 UTC) #13
mattm
lgtm
9 years, 3 months ago (2011-09-23 04:52:26 UTC) #14
commit-bot: I haz the power
CQ is trying tha patch. Follow status at https://chromium-status.appspot.com/cq/bryner@chromium.org/7976039/11002
9 years, 3 months ago (2011-09-23 16:57:49 UTC) #15
commit-bot: I haz the power
9 years, 3 months ago (2011-09-23 18:56:37 UTC) #16
Change committed as 102541

Powered by Google App Engine
This is Rietveld 408576698