Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6710004:
Querying the history service to get the redirect information for urls.... (Closed)
|
Created:
9 years, 9 months ago by kewang Modified:
9 years, 7 months ago Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionQuerying the history service to get the redirect information for urls.
Bug=60831
TEST=unit_tests
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=86401
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #Patch Set 4 : '' #Patch Set 5 : '' #Patch Set 6 : '' #Patch Set 7 : '' #
Total comments: 20
Patch Set 8 : '' #
Total comments: 50
Patch Set 9 : '' #
Total comments: 4
Patch Set 10 : '' #
Total comments: 6
Patch Set 11 : '' #
Messages
Total messages: 15 (0 generated)
http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details.cc (right): http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:236: if (cache_collector_->HasStarted()) { drop the { here http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:286: LOG(INFO) << "url list size:" << urls.size(); let's drop the logging http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:288: AddUrl(urls[i], urls[i+1], "", NULL); I wonder if we should specify a tagname for these redirects. Maybe "redir" ? http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_history.cc (right): http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:67: BrowserThread::UI, FROM_HERE, we are already in the UI thread, no need to post a task here right? http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:108: BrowserThread::PostTask( same here, unless you are posting a task so that we don't hold up the cpu for ourselves too much? In that case we should comment about it http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_history.h (right): http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:28: void StartHistoryCollection(const std::vector<GURL>& urls, comment about the method and args. mention that |callback| will be posted on the IO thread. http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:35: // Returns the rediret urls we get from history service typo: redirect we probably want to return a reference or a pointer to this vector instead of copying the vector here http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:39: HistoryService* history_; maybe expose an accessor instead (GetHistory()) ? http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:56: std::vector<std::vector<GURL> > redirects_urls_; maybe we can create a typedef RedirectChain = std::vector<GURL> ? then this will be clearer that it's a vector of redirect chains
Drive-by with a testing comment. http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_unittest.cc (right): http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:235: void WaitForHistory() { Isn't that TestingProfile::BlockUntilHistoryProcessesPendingRequests? I think using the existing method for it would help avoiding code duplication.
http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details.cc (right): http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:236: if (cache_collector_->HasStarted()) { On 2011/05/13 21:15:26, panayiotis wrote: > drop the { here Done. http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:286: LOG(INFO) << "url list size:" << urls.size(); On 2011/05/13 21:15:26, panayiotis wrote: > let's drop the logging Done. http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:288: AddUrl(urls[i], urls[i+1], "", NULL); On 2011/05/13 21:15:26, panayiotis wrote: > I wonder if we should specify a tagname for these redirects. Maybe "redir" ? Done. http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_history.cc (right): http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:67: BrowserThread::UI, FROM_HERE, On 2011/05/13 21:15:26, panayiotis wrote: > we are already in the UI thread, no need to post a task here right? Yes, absolutely. Moved from old code and didn't improve. Done. http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:108: BrowserThread::PostTask( On 2011/05/13 21:15:26, panayiotis wrote: > same here, unless you are posting a task so that we don't hold up the cpu for > ourselves too much? In that case we should comment about it Done. http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_history.h (right): http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:28: void StartHistoryCollection(const std::vector<GURL>& urls, On 2011/05/13 21:15:26, panayiotis wrote: > comment about the method and args. > > mention that |callback| will be posted on the IO thread. Done. http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:35: // Returns the rediret urls we get from history service On 2011/05/13 21:15:26, panayiotis wrote: > typo: redirect > > we probably want to return a reference or a pointer to this vector instead of > copying the vector here Done. http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:39: HistoryService* history_; On 2011/05/13 21:15:26, panayiotis wrote: > maybe expose an accessor instead (GetHistory()) ? Put a SetHistory() for unittest. http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:56: std::vector<std::vector<GURL> > redirects_urls_; On 2011/05/13 21:15:26, panayiotis wrote: > maybe we can create a typedef RedirectChain = std::vector<GURL> ? > > then this will be clearer that it's a vector of redirect chains Done. http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_unittest.cc (right): http://codereview.chromium.org/6710004/diff/15002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:235: void WaitForHistory() { On 2011/05/14 09:33:49, Paweł Hajdan Jr. wrote: > Isn't that TestingProfile::BlockUntilHistoryProcessesPendingRequests? I think > using the existing method for it would help avoiding code duplication. Good to know this! Fixed.
Looks nice. Let's have Matt have a look. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details.cc (right): http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:287: LOG(INFO) << "--- adding...." << urls[i]; drop the log infos ...
http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details.cc (right): http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:285: for (uint i = 0; i < urls.size()-1; ++i) { size_t http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:286: AddUrl(urls[i], urls[i+1], "redir", NULL); "redir" should be a const. Also, other places just seem to use "" when recording redirects. Is "redir" intended to mean that it came from this history checking step specifically or should the other cases use it too? (be sure to include a comment with the const about what it means) Should it have some sort of special chars or something to avoid name collisions with actual tags? http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details.h (right): http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.h:60: // we send the report. update this comment http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_history.cc (right): http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:49: // History access need to happen in UI thread The HistoryService class comment says it is threadsafe. Maybe you need to be on UI thread to get the history from the profile? (It doesn't seem to have any comments regarding thread safety.) http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:62: for (uint i = 0; i < urls.size(); ++i) size_t (other places too) http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:73: new CancelableRequestConsumer(); This will be leaked. It should be a class member. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:88: if (success && redirect_list->size() > 0) { wrong indentation http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_history.h (right): http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:9: // A class that gets url redirets from the history service typo (redirets) Comments should be complete sentences end with period (applies to some other locations as well). http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:12: #include <vector> blank line between include blocks http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:40: bool HasStarted(); Should be const method. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:43: std::vector<safe_browsing::RedirectChain>* GetCollectedUrls(); Should return a const reference. Should be const method. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:45: // Set the pointer to the history service. Use it in unittest. s/Set/Sets/ (Function comments should be descriptive ("Opens the file") rather than imperative ("Open the file")). Applies to a few other ones too. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_unittest.cc (right): http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:182: profile_.reset(new TestingProfile); It looks like RenderViewHostTestHarness already creates a TestingProfile? http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:183: profile_->CreateHistoryService(false, false); include arg name comments (like false /* delete_file */, false /* no_db */) http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:187: profile_.reset(); Unnecessary, unless there is some reason you need this to be destructed during TearDown instead of when the TestCase is destructed. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:292: // Adds a page to history. Add a comment about the expected order of redirects. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:303: scoped_ptr<TestingProfile> profile_; RenderViewHostTestHarness already has a profile_ member http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:623: // the last item of the redirect chain has to be the final URL I was a bit confused since that's opposite the order the history service returns them in. Maybe add a bit to the comment like "when adding".
I'd like to take another look after the |profile_| issue gets sorted out (good catch by the way!). While looking at the code now, I added some more tiny nits. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_unittest.cc (right): http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:616: // Test getting redirects from history service nit: Dot at the end please. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:618: // Add content to history service nit: Dot at the end. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:623: // the last item of the redirect chain has to be the final URL nit: Start with a capital letter and end with a dot. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:626: // Wait for history service operation finished nit: Dot at the end. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:643: // Let the redirects callbacks complete nit: Dot at the end.
http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details.cc (right): http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:286: AddUrl(urls[i], urls[i+1], "redir", NULL); Actually I take it back, let's not add any tagname, we don't need any. Sorry about that Ke On 2011/05/17 03:17:01, mattm wrote: > "redir" should be a const. Also, other places just seem to use "" when > recording redirects. Is "redir" intended to mean that it came from this history > checking step specifically or should the other cases use it too? (be sure to > include a comment with the const about what it means) > > Should it have some sort of special chars or something to avoid name collisions > with actual tags?
Thanks for the comments. Updated, and please take another look. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details.cc (right): http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:285: for (uint i = 0; i < urls.size()-1; ++i) { On 2011/05/17 03:17:01, mattm wrote: > size_t Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:286: AddUrl(urls[i], urls[i+1], "redir", NULL); On 2011/05/17 03:17:01, mattm wrote: > "redir" should be a const. Also, other places just seem to use "" when > recording redirects. Is "redir" intended to mean that it came from this history > checking step specifically or should the other cases use it too? (be sure to > include a comment with the const about what it means) > > Should it have some sort of special chars or something to avoid name collisions > with actual tags? const added. We are using it for all the redir urls. Applied the tag to other redir urls too. They are all redir urls, just exist in different places. History service contains the redirects except the ones to the final malware ones. That's why we have two different places to get redir. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:286: AddUrl(urls[i], urls[i+1], "redir", NULL); On 2011/05/17 17:19:48, panayiotis wrote: > Actually I take it back, let's not add any tagname, we don't need any. Sorry > about that Ke > > On 2011/05/17 03:17:01, mattm wrote: > > "redir" should be a const. Also, other places just seem to use "" when > > recording redirects. Is "redir" intended to mean that it came from this > history > > checking step specifically or should the other cases use it too? (be sure to > > include a comment with the const about what it means) > > > > Should it have some sort of special chars or something to avoid name > collisions > > with actual tags? > Followed Pano's suggestion and removed it. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.cc:287: LOG(INFO) << "--- adding...." << urls[i]; On 2011/05/16 17:14:55, panayiotis wrote: > drop the log infos ... Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details.h (right): http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details.h:60: // we send the report. On 2011/05/17 03:17:01, mattm wrote: > update this comment Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_history.cc (right): http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:49: // History access need to happen in UI thread On 2011/05/17 03:17:01, mattm wrote: > The HistoryService class comment says it is threadsafe. > > Maybe you need to be on UI thread to get the history from the profile? (It > doesn't seem to have any comments regarding thread safety.) yes, when access history service from profile, it need to be in UI thread, otherwise fail with error msg: FATAL:pref_service.cc(330)] Check failed: CalledOnValidThread(). http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:62: for (uint i = 0; i < urls.size(); ++i) On 2011/05/17 03:17:01, mattm wrote: > size_t (other places too) Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:73: new CancelableRequestConsumer(); On 2011/05/17 03:17:01, mattm wrote: > This will be leaked. It should be a class member. Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:88: if (success && redirect_list->size() > 0) { On 2011/05/17 03:17:01, mattm wrote: > wrong indentation Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_history.h (right): http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:9: // A class that gets url redirets from the history service On 2011/05/17 03:17:01, mattm wrote: > typo (redirets) > > Comments should be complete sentences end with period (applies to some other > locations as well). Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:12: #include <vector> On 2011/05/17 03:17:01, mattm wrote: > blank line between include blocks Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:40: bool HasStarted(); On 2011/05/17 03:17:01, mattm wrote: > Should be const method. Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:43: std::vector<safe_browsing::RedirectChain>* GetCollectedUrls(); On 2011/05/17 03:17:01, mattm wrote: > Should return a const reference. > Should be const method. Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.h:45: // Set the pointer to the history service. Use it in unittest. On 2011/05/17 03:17:01, mattm wrote: > s/Set/Sets/ > > (Function comments should be descriptive ("Opens the file") rather than > imperative ("Open the file")). Applies to a few other ones too. Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_unittest.cc (right): http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:182: profile_.reset(new TestingProfile); On 2011/05/17 03:17:01, mattm wrote: > It looks like RenderViewHostTestHarness already creates a TestingProfile? Yes, and it got reset in SetUp() too. Remove the definition and reset here. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:183: profile_->CreateHistoryService(false, false); On 2011/05/17 03:17:01, mattm wrote: > include arg name comments (like false /* delete_file */, false /* no_db */) Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:187: profile_.reset(); On 2011/05/17 03:17:01, mattm wrote: > Unnecessary, unless there is some reason you need this to be destructed during > TearDown instead of when the TestCase is destructed. Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:292: // Adds a page to history. On 2011/05/17 03:17:01, mattm wrote: > Add a comment about the expected order of redirects. Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:303: scoped_ptr<TestingProfile> profile_; On 2011/05/17 03:17:01, mattm wrote: > RenderViewHostTestHarness already has a profile_ member removed. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:616: // Test getting redirects from history service On 2011/05/17 15:13:54, Paweł Hajdan Jr. wrote: > nit: Dot at the end please. Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:618: // Add content to history service On 2011/05/17 15:13:54, Paweł Hajdan Jr. wrote: > nit: Dot at the end. Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:623: // the last item of the redirect chain has to be the final URL On 2011/05/17 03:17:01, mattm wrote: > I was a bit confused since that's opposite the order the history service returns > them in. Maybe add a bit to the comment like "when adding". yes, history backend has a check: request->redirects.back() == request->url. The order is actually the url redirection order. I modified the AddPageToHistory() to add the ending url there. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:623: // the last item of the redirect chain has to be the final URL On 2011/05/17 15:13:54, Paweł Hajdan Jr. wrote: > nit: Start with a capital letter and end with a dot. Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:626: // Wait for history service operation finished On 2011/05/17 15:13:54, Paweł Hajdan Jr. wrote: > nit: Dot at the end. Done. http://codereview.chromium.org/6710004/diff/22001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:643: // Let the redirects callbacks complete On 2011/05/17 15:13:54, Paweł Hajdan Jr. wrote: > nit: Dot at the end. Done.
Code I commented in the drive-by LGTM.
http://codereview.chromium.org/6710004/diff/26002/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_history.cc (right): http://codereview.chromium.org/6710004/diff/26002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:20: request_consumer_ = new CancelableRequestConsumer(); This will still be leaked. It should be a non-pointer member. http://codereview.chromium.org/6710004/diff/26002/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_unittest.cc (right): http://codereview.chromium.org/6710004/diff/26002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:297: redirects.push_back(url); Arguments that are modified should be passed as pointers, not references. Or you could make it pass by copy.
http://codereview.chromium.org/6710004/diff/26002/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_history.cc (right): http://codereview.chromium.org/6710004/diff/26002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:20: request_consumer_ = new CancelableRequestConsumer(); On 2011/05/18 22:31:47, mattm wrote: > This will still be leaked. It should be a non-pointer member. Done. http://codereview.chromium.org/6710004/diff/26002/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_unittest.cc (right): http://codereview.chromium.org/6710004/diff/26002/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_unittest.cc:297: redirects.push_back(url); On 2011/05/18 22:31:47, mattm wrote: > Arguments that are modified should be passed as pointers, not references. Or > you could make it pass by copy. Done.
Is there anything to do in the relevant browser_test? I guess it just passes? http://codereview.chromium.org/6710004/diff/35001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_history.cc (right): http://codereview.chromium.org/6710004/diff/35001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:29: LOG(INFO) << "Num of urls: " << urls.size(); if you want to keep these logs, maybe we should use DVLOG(1) http://codereview.chromium.org/6710004/diff/35001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:49: // History access from profile need to happen in UI thread "needs to happen" http://codereview.chromium.org/6710004/diff/35001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:51: LOG(INFO) << "Getting access to history service"; same here
Yes, all the unit_tests and browser_tests pass. http://codereview.chromium.org/6710004/diff/35001/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/malware_details_history.cc (right): http://codereview.chromium.org/6710004/diff/35001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:29: LOG(INFO) << "Num of urls: " << urls.size(); On 2011/05/19 17:08:37, panayiotis wrote: > if you want to keep these logs, maybe we should use DVLOG(1) Done. http://codereview.chromium.org/6710004/diff/35001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:49: // History access from profile need to happen in UI thread On 2011/05/19 17:08:37, panayiotis wrote: > "needs to happen" Done. http://codereview.chromium.org/6710004/diff/35001/chrome/browser/safe_browsin... chrome/browser/safe_browsing/malware_details_history.cc:51: LOG(INFO) << "Getting access to history service"; On 2011/05/19 17:08:37, panayiotis wrote: > same here Done.
lgtm On 2011/05/20 03:52:55, kewang wrote: > Yes, all the unit_tests and browser_tests pass. > > http://codereview.chromium.org/6710004/diff/35001/chrome/browser/safe_browsin... > File chrome/browser/safe_browsing/malware_details_history.cc (right): > > http://codereview.chromium.org/6710004/diff/35001/chrome/browser/safe_browsin... > chrome/browser/safe_browsing/malware_details_history.cc:29: LOG(INFO) << "Num of > urls: " << urls.size(); > On 2011/05/19 17:08:37, panayiotis wrote: > > if you want to keep these logs, maybe we should use DVLOG(1) > > Done. > > http://codereview.chromium.org/6710004/diff/35001/chrome/browser/safe_browsin... > chrome/browser/safe_browsing/malware_details_history.cc:49: // History access > from profile need to happen in UI thread > On 2011/05/19 17:08:37, panayiotis wrote: > > "needs to happen" > > Done. > > http://codereview.chromium.org/6710004/diff/35001/chrome/browser/safe_browsin... > chrome/browser/safe_browsing/malware_details_history.cc:51: LOG(INFO) << > "Getting access to history service"; > On 2011/05/19 17:08:37, panayiotis wrote: > > same here > > Done.
lgtm |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
