Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
18878009:
Add functions to clean URLs from the activity log (Closed)
|
Created:
7 years, 5 months ago by karenlees Modified:
7 years, 3 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, browser-components-watch_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdds functions to clean URLs from the activity log DB.
BUG=253367
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=220075
Patch Set 1 : Functions to clean URLs from the activity log #Patch Set 2 : Fix some comments #
Total comments: 19
Patch Set 3 : Changes after refactoring. #Patch Set 4 : Remove some logging #Patch Set 5 : no changes - seeing if the diff view works better #Patch Set 6 : Added counting policy #
Total comments: 24
Patch Set 7 : Updates for review comments #Patch Set 8 : minor cleanup - should have been in last patchw #Patch Set 9 : Merge with 16703004 #Patch Set 10 : Fix formatting diffs from merge #Patch Set 11 : Revert history file changes - will do them on a new CL #
Total comments: 5
Patch Set 12 : minor fixes #Patch Set 13 : Return after errors #
Total comments: 5
Patch Set 14 : Update tests to check fields directly #Patch Set 15 : Change EXPECT_EQ back to ASSERT_EQ #Patch Set 16 : use const_iterator #Messages
Total messages: 36 (0 generated)
This doesn't have the changes to the history code in it. I updated
history_service.cc and tested the deletions manually but I wasn't allowed to
upload the changes:
"** Presubmit ERRORS **
You added one or more #includes that violate checkdeps rules.
chrome/browser/history/history_service.cc
Illegal include: "chrome/browser/extensions/activity_log/activity_log.h"
Because of "-chrome/browser" from chrome/browser/history's include_rules."
So, for now can you review the activity log changes and I'll ask the history
team which files I should edit.
Hi Karen, a comment about structure below. https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/activity_database.h (right): https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_database.h:65: static const char* kURLFields[]; This constant should probably be moved into the ActivityPolicy classes, since each one might have a slightly different database layout. Check out the new Delegate class here inside the ActivityDatabase. (This has changed since we originally talked about this -- the Delegate class didn't exist at that point in time.) Perhaps the removal methods could take more parameters: the database name and the columns to clear, rather having constants in the ActivityDatabase. Another option this could be a method on the Delegate class, actually implemented inside the policy classes. That might be more confusing than adding more parameters, since it would be like... ActivityPolicy->RemoveURLs calls ActivityDatabase->RemoveURLs calls ActivityPolicy->RemoveURLsInternal or something. Michael, do you have thoughts/a preference? https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_database.h:178: static std::string ExtractTLD(const GURL& gurl); Maybe it could be a static method on the activity_actions.h class?
This is looking good overall. Some of my comments are stylistic suggestions which you don't have to follow if you don't agree. --Michael Vrable https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/activity_database.cc (right): https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_database.cc:267: std::string ActivityDatabase::ExtractTLD(const GURL& gurl) { I know you just took the name from the DOMAction code, but the term "TLD" here doesn't seem right to me. In a URL like http://www.google.com/, the top-level domain (TLD) would be .com; a term like "origin" or "host" should be used to refer to "www.google.com". I was thinking of using just a single url column (instead of separate host and path columns) in the database after refactoring, in which case this method might not be needed? So the name might not matter much. https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_database.cc:284: set_fields_str = base::StringPrintf("%s,%s=''", set_fields_str.c_str(), Personal preference: I would use NULL rather than '' to represent a cleared URL, but the two are nearly equivalent so I'll leave it up to you (unless other reviewers want to comment on NULL vs. empty values in Chrome databases, if there are conventions to follow). https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_database.cc:300: "WHERE url_tld='%s'", You shouldn't insert the URL directly into the query like this, as it can lead to SQL injection vulnerabilties; you should use a placeholder ("?") and then use BindString to set the value. Inserting column names or text you know is safe is fine. (It might be safe depending on whether special characters like ' are always escaped in the URL...but much better to be obviously safe.) https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/activity_database.h (right): https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_database.h:65: static const char* kURLFields[]; On 2013/07/17 01:55:40, felt wrote: > This constant should probably be moved into the ActivityPolicy classes, since > each one might have a slightly different database layout. Check out the new > Delegate class here inside the ActivityDatabase. (This has changed since we > originally talked about this -- the Delegate class didn't exist at that point in > time.) > > Perhaps the removal methods could take more parameters: the database name and > the columns to clear, rather having constants in the ActivityDatabase. > > Another option this could be a method on the Delegate class, actually > implemented inside the policy classes. That might be more confusing than adding > more parameters, since it would be like... ActivityPolicy->RemoveURLs calls > ActivityDatabase->RemoveURLs calls ActivityPolicy->RemoveURLsInternal or > something. > > Michael, do you have thoughts/a preference? If the history clearing code is in the ActivityDatabase, even if it takes various parameters, its flexibility is still limited (imagine a policy that is performing URL clustering or something similar). Long-term I think it would make sense to have the code in the ActivityPolicy (your second option). One other approach would be to have a general method in the ActivityDatabase to run (on the database thread) a callback that is passed the database handle. The end result would still be a call chain like ActivityPolicy::RemoveURLs -> ActivityDatabase::RemoveURLs -> ActivityPolicy::RemoveURLsInternal, except that the code in ActivityDatabase wouldn't be tied to clearing history at all and could be re-used for other purposes (if needed). I'm fine with this code as-is, however, and can move the database code into the policy as needed when I'm migrating the other policy-specific code in ActivityDatabase. https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_database.h:178: static std::string ExtractTLD(const GURL& gurl); On 2013/07/17 01:55:40, felt wrote: > Maybe it could be a static method on the activity_actions.h class? That would be a good location for it. https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/activity_log.cc (left): https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_log.cc:149: break; Deleting these lines looks like an accidental edit--revert? https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/activity_log_policy.h (right): https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_log_policy.h:112: virtual void RemoveURLs(const std::vector<GURL>& gurls) = 0; You could simplify the code by just providing a single method that takes a vector, and not the RemoveURL method. It could be fine to keep the multiple methods on ActivityLog to simplify callers' lives, but reduce the number of methods that needed to be threaded from ActivityLog -> ActivityLogPolicy -> ActivityDatabase https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/fullstream_ui_policy.h (right): https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:49: virtual void RemoveURLs(const std::vector<GURL>& gurls); Mark these methods as OVERRIDE https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy.h (right): https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.h:21: // Clean the relevant URL data stored for this policy. StreamWithoutArgsUIPolicy inherits from FullStreamUIPolicy. Any reason to override the definitions from there rather than just re-using that code implicitly?
No hurry to review these as it won't get in until M31 anyway. I think I did everything in the previous comments. I wasn't sure if there was an particular ordering to add methods so you might prefer me to move them to a different position in the files. I've added the history file to the CL to show what I was initially trying to do. I'll get touch with the history team to ask them were they prefer me to add the changes as the DEPs file tells me multiple times not to do what I'm doing. https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/activity_database.cc (right): https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_database.cc:267: std::string ActivityDatabase::ExtractTLD(const GURL& gurl) { On 2013/07/17 16:41:23, mvrable wrote: > I know you just took the name from the DOMAction code, but the term "TLD" here > doesn't seem right to me. In a URL like http://www.google.com/, the top-level > domain (TLD) would be .com; a term like "origin" or "host" should be used to > refer to "www.google.com". > > I was thinking of using just a single url column (instead of separate host and > path columns) in the database after refactoring, in which case this method might > not be needed? So the name might not matter much. No longer need this method. https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_database.cc:267: std::string ActivityDatabase::ExtractTLD(const GURL& gurl) { On 2013/07/17 16:41:23, mvrable wrote: > I know you just took the name from the DOMAction code, but the term "TLD" here > doesn't seem right to me. In a URL like http://www.google.com/, the top-level > domain (TLD) would be .com; a term like "origin" or "host" should be used to > refer to "www.google.com". > > I was thinking of using just a single url column (instead of separate host and > path columns) in the database after refactoring, in which case this method might > not be needed? So the name might not matter much. Method no longer needed, removed. https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_database.cc:284: set_fields_str = base::StringPrintf("%s,%s=''", set_fields_str.c_str(), On 2013/07/17 16:41:23, mvrable wrote: > Personal preference: I would use NULL rather than '' to represent a cleared URL, > but the two are nearly equivalent so I'll leave it up to you (unless other > reviewers want to comment on NULL vs. empty values in Chrome databases, if there > are conventions to follow). I like NULL, updated. https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_database.cc:300: "WHERE url_tld='%s'", On 2013/07/17 16:41:23, mvrable wrote: > You shouldn't insert the URL directly into the query like this, as it can lead > to SQL injection vulnerabilties; you should use a placeholder ("?") and then use > BindString to set the value. Inserting column names or text you know is safe is > fine. > > (It might be safe depending on whether special characters like ' are always > escaped in the URL...but much better to be obviously safe.) I like safe, updated. https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/activity_log.cc (left): https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_log.cc:149: break; On 2013/07/17 16:41:23, mvrable wrote: > Deleting these lines looks like an accidental edit--revert? Done. https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/activity_log_policy.h (right): https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_log_policy.h:112: virtual void RemoveURLs(const std::vector<GURL>& gurls) = 0; On 2013/07/17 16:41:23, mvrable wrote: > You could simplify the code by just providing a single method that takes a > vector, and not the RemoveURL method. It could be fine to keep the multiple > methods on ActivityLog to simplify callers' lives, but reduce the number of > methods that needed to be threaded from ActivityLog -> ActivityLogPolicy -> > ActivityDatabase Done. https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/fullstream_ui_policy.h (right): https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:49: virtual void RemoveURLs(const std::vector<GURL>& gurls); On 2013/07/17 16:41:23, mvrable wrote: > Mark these methods as OVERRIDE Done. https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy.h (right): https://codereview.chromium.org/18878009/diff/13001/chrome/browser/extensions... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.h:21: // Clean the relevant URL data stored for this policy. On 2013/07/17 16:41:23, mvrable wrote: > StreamWithoutArgsUIPolicy inherits from FullStreamUIPolicy. Any reason to > override the definitions from there rather than just re-using that code > implicitly? Done.
This has the counting policy added now. As discussed, I can't get the DeleteSpecificURLs to work. It doesn't seem to match correctly if I have the "WHERE page_url_x=url_id" but it does seem to be retrieving the correct ids, so can't figure out what's up.
Some comments which should help fix the unit tests for the counting policy. I haven't checked the fullstream policy yet. https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/counting_policy.cc (right): https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... chrome/browser/extensions/activity_log/counting_policy.cc:579: "WHERE page_url_x=? OR arg_url_x=?", Use "WHERE page_url_x IS ? OR arg_url_x IS ?" to handle null values in the database properly. https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/counting_policy_unittest.cc (right): https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... chrome/browser/extensions/activity_log/counting_policy_unittest.cc:608: new Action("punky", mock_clock->Now(), When data is read from the database in CheckReadData, values are sorted by time (from most recent to oldest). All Actions that you are inserting have the same time value (mock_clock->Now()), so the order of the values when read back is indeterminate--but the checking code assumes the values come back in a specific order. I'd use a different time value for each action, so you have a dependable ordering. https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... chrome/browser/extensions/activity_log/counting_policy_unittest.cc:667: action->set_page_title("Google"); Missing a action->set_arg_url(GURL("http://www.google2.com")) here? The comparison seems to expect arg_url to be set.
https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:196: Add a activity_database()->AdviseFlush(ActivityDatabase::kFlushImmediately); to the start of this method so that actions queued in memory are covered by the history clearing.
https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/counting_policy.cc (right): https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... chrome/browser/extensions/activity_log/counting_policy.cc:548: // Ensure data is flushed to the database first so that we query over all I'd just update this comment as "query" does not describe what this function is doing; "flush data first so the URL clearing affects queued-up data as well" or something similar. https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... chrome/browser/extensions/activity_log/counting_policy.cc:569: for (uint32_t i = 0; i < restrict_urls.size(); ++i) { size_t instead of uint32_t? https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... chrome/browser/extensions/activity_log/counting_policy.cc:579: "WHERE page_url_x=? OR arg_url_x=?", However, I might still prefer using two separate update statements: one to clear page_url & page_title, and one for arg_url. (It doesn't seem that a match on page_url should cause arg_url to be cleared, and vice versa.) In that case using "=" or "IS" should be equivalent. https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... chrome/browser/extensions/activity_log/counting_policy.cc:600: BrowserThread::PostTask( I think you can use ScheduleAndForget instead of PostTask. https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/counting_policy.h (right): https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... chrome/browser/extensions/activity_log/counting_policy.h:40: virtual void RemoveURLs(const std::vector<GURL>&); virtual void RemoveURLs(const std::vector<GURL>&) OVERRIDE; https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... chrome/browser/extensions/activity_log/counting_policy.h:74: virtual void DoRemoveURLs(const std::vector<GURL>& restrict_urls); Doesn't need to be virtual since the method is private. https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:214: for (uint32_t i = 0; i < restrict_urls.size(); ++i) { size_t https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:267: BrowserThread::PostTask( ScheduleAndForget should be able to do the same as postTask but with slightly simpler code.
https://chromiumcodereview.appspot.com/18878009/diff/64001/chrome/browser/ext... File chrome/browser/extensions/activity_log/counting_policy.cc (right): https://chromiumcodereview.appspot.com/18878009/diff/64001/chrome/browser/ext... chrome/browser/extensions/activity_log/counting_policy.cc:548: // Ensure data is flushed to the database first so that we query over all On 2013/08/26 20:38:50, mvrable wrote: > I'd just update this comment as "query" does not describe what this function is > doing; "flush data first so the URL clearing affects queued-up data as well" or > something similar. Done. Sorry for abusing the term for query for this sql statement. https://chromiumcodereview.appspot.com/18878009/diff/64001/chrome/browser/ext... chrome/browser/extensions/activity_log/counting_policy.cc:569: for (uint32_t i = 0; i < restrict_urls.size(); ++i) { On 2013/08/26 20:38:50, mvrable wrote: > size_t instead of uint32_t? Done. https://chromiumcodereview.appspot.com/18878009/diff/64001/chrome/browser/ext... chrome/browser/extensions/activity_log/counting_policy.cc:569: for (uint32_t i = 0; i < restrict_urls.size(); ++i) { On 2013/08/26 20:38:50, mvrable wrote: > size_t instead of uint32_t? Done. https://chromiumcodereview.appspot.com/18878009/diff/64001/chrome/browser/ext... chrome/browser/extensions/activity_log/counting_policy.cc:579: "WHERE page_url_x=? OR arg_url_x=?", On 2013/08/26 18:14:57, mvrable wrote: > Use "WHERE page_url_x IS ? OR arg_url_x IS ?" to handle null values in the > database properly. Done. Sorry I did try the statements on friday, not sure why I couldn't get it to work without the OR statement (just on page_url). That works now, but I still can't get the full thing to work with the OR statement, even if the IS is used. I'll do in two statements as you suggested as alternative. https://chromiumcodereview.appspot.com/18878009/diff/64001/chrome/browser/ext... chrome/browser/extensions/activity_log/counting_policy.cc:579: "WHERE page_url_x=? OR arg_url_x=?", On 2013/08/26 20:38:50, mvrable wrote: > However, I might still prefer using two separate update statements: one to clear > page_url & page_title, and one for arg_url. (It doesn't seem that a match on > page_url should cause arg_url to be cleared, and vice versa.) In that case > using "=" or "IS" should be equivalent. Yep, doing, see above comment. https://chromiumcodereview.appspot.com/18878009/diff/64001/chrome/browser/ext... File chrome/browser/extensions/activity_log/counting_policy.h (right): https://chromiumcodereview.appspot.com/18878009/diff/64001/chrome/browser/ext... chrome/browser/extensions/activity_log/counting_policy.h:40: virtual void RemoveURLs(const std::vector<GURL>&); On 2013/08/26 20:38:50, mvrable wrote: > virtual void RemoveURLs(const std::vector<GURL>&) OVERRIDE; Done. https://chromiumcodereview.appspot.com/18878009/diff/64001/chrome/browser/ext... chrome/browser/extensions/activity_log/counting_policy.h:74: virtual void DoRemoveURLs(const std::vector<GURL>& restrict_urls); On 2013/08/26 20:38:50, mvrable wrote: > Doesn't need to be virtual since the method is private. Done - but should I make it protected instead, incase this gets subclassed? https://chromiumcodereview.appspot.com/18878009/diff/64001/chrome/browser/ext... File chrome/browser/extensions/activity_log/counting_policy_unittest.cc (right): https://chromiumcodereview.appspot.com/18878009/diff/64001/chrome/browser/ext... chrome/browser/extensions/activity_log/counting_policy_unittest.cc:608: new Action("punky", mock_clock->Now(), On 2013/08/26 18:14:57, mvrable wrote: > When data is read from the database in CheckReadData, values are sorted by time > (from most recent to oldest). All Actions that you are inserting have the same > time value (mock_clock->Now()), so the order of the values when read back is > indeterminate--but the checking code assumes the values come back in a specific > order. > > I'd use a different time value for each action, so you have a dependable > ordering. Done. https://chromiumcodereview.appspot.com/18878009/diff/64001/chrome/browser/ext... chrome/browser/extensions/activity_log/counting_policy_unittest.cc:667: action->set_page_title("Google"); On 2013/08/26 18:14:57, mvrable wrote: > Missing a action->set_arg_url(GURL("http://www.google2.com")) here? The > comparison seems to expect arg_url to be set. This was deliberate to make sure the sql statement still works if the arg_url is missing. If the others passed and this failed I needed to make two calls to handle the null values in the OR statement correctly. Is it possible for there to be an arg_url without a page_url? If so, I'll add a test for that too. https://chromiumcodereview.appspot.com/18878009/diff/64001/chrome/browser/ext... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://chromiumcodereview.appspot.com/18878009/diff/64001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:196: On 2013/08/26 18:22:42, mvrable wrote: > Add a > activity_database()->AdviseFlush(ActivityDatabase::kFlushImmediately); > to the start of this method so that actions queued in memory are covered by the > history clearing. Done. https://chromiumcodereview.appspot.com/18878009/diff/64001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:214: for (uint32_t i = 0; i < restrict_urls.size(); ++i) { On 2013/08/26 20:38:50, mvrable wrote: > size_t Done.
lgtm https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/counting_policy.h (right): https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... chrome/browser/extensions/activity_log/counting_policy.h:74: virtual void DoRemoveURLs(const std::vector<GURL>& restrict_urls); On 2013/08/26 22:58:36, karenlees wrote: > On 2013/08/26 20:38:50, mvrable wrote: > > Doesn't need to be virtual since the method is private. > > Done - but should I make it protected instead, incase this gets subclassed? Personal opinion: it doesn't need to be as we can always go back later and make it protected virtual if we need to subclass. But I don't have a strong feeling on this.
On 2013/08/27 17:17:37, mvrable wrote: > lgtm > > https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... > File chrome/browser/extensions/activity_log/counting_policy.h (right): > > https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/counting_policy.h:74: virtual void > DoRemoveURLs(const std::vector<GURL>& restrict_urls); > On 2013/08/26 22:58:36, karenlees wrote: > > On 2013/08/26 20:38:50, mvrable wrote: > > > Doesn't need to be virtual since the method is private. > > > > Done - but should I make it protected instead, incase this gets subclassed? > > Personal opinion: it doesn't need to be as we can always go back later and make > it protected virtual if we need to subclass. But I don't have a strong feeling > on this. I've merged with the latest changes. Adrienne - as we might also need these functions for the api now, I think it would be better for me to move the history changes to a separate patch. Then we can get these changes submitted today and start using the functions. I can send out the patch for discussion with the history team as soon as this is in. What do you think?
On 2013/08/27 18:35:52, karenlees wrote: > On 2013/08/27 17:17:37, mvrable wrote: > > lgtm > > > > > https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... > > File chrome/browser/extensions/activity_log/counting_policy.h (right): > > > > > https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... > > chrome/browser/extensions/activity_log/counting_policy.h:74: virtual void > > DoRemoveURLs(const std::vector<GURL>& restrict_urls); > > On 2013/08/26 22:58:36, karenlees wrote: > > > On 2013/08/26 20:38:50, mvrable wrote: > > > > Doesn't need to be virtual since the method is private. > > > > > > Done - but should I make it protected instead, incase this gets subclassed? > > > > Personal opinion: it doesn't need to be as we can always go back later and > make > > it protected virtual if we need to subclass. But I don't have a strong > feeling > > on this. > > > I've merged with the latest changes. > > Adrienne - as we might also need these functions for the api now, I think it > would be better for me to move the history changes to a separate patch. Then we > can get these changes submitted today and start using the functions. I can send > out the patch for discussion with the history team as soon as this is in. What > do you think? Hi karen, I'm not 100% sure what you mean. Do you mean: add the deletion functions in this CL, and then add the calls to the functions from history code in a separate CL? If so, that sounds good to me.
On 2013/08/27 18:44:38, felt wrote: > On 2013/08/27 18:35:52, karenlees wrote: > > On 2013/08/27 17:17:37, mvrable wrote: > > > lgtm > > > > > > > > > https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... > > > File chrome/browser/extensions/activity_log/counting_policy.h (right): > > > > > > > > > https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... > > > chrome/browser/extensions/activity_log/counting_policy.h:74: virtual void > > > DoRemoveURLs(const std::vector<GURL>& restrict_urls); > > > On 2013/08/26 22:58:36, karenlees wrote: > > > > On 2013/08/26 20:38:50, mvrable wrote: > > > > > Doesn't need to be virtual since the method is private. > > > > > > > > Done - but should I make it protected instead, incase this gets > subclassed? > > > > > > Personal opinion: it doesn't need to be as we can always go back later and > > make > > > it protected virtual if we need to subclass. But I don't have a strong > > feeling > > > on this. > > > > > > I've merged with the latest changes. > > > > Adrienne - as we might also need these functions for the api now, I think it > > would be better for me to move the history changes to a separate patch. Then > we > > can get these changes submitted today and start using the functions. I can > send > > out the patch for discussion with the history team as soon as this is in. > What > > do you think? > > Hi karen, I'm not 100% sure what you mean. Do you mean: add the deletion > functions in this CL, and then add the calls to the functions from history code > in a separate CL? If so, that sounds good to me. Yep, I basically mean move these files to a different CL: - chrome/browser/history/DEPS - chrome/browser/history/history_service.cc Then I only need permission to submit to the activity_log directory.
On 2013/08/27 19:09:26, karenlees wrote: > On 2013/08/27 18:44:38, felt wrote: > > On 2013/08/27 18:35:52, karenlees wrote: > > > On 2013/08/27 17:17:37, mvrable wrote: > > > > lgtm > > > > > > > > > > > > > > https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... > > > > File chrome/browser/extensions/activity_log/counting_policy.h (right): > > > > > > > > > > > > > > https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... > > > > chrome/browser/extensions/activity_log/counting_policy.h:74: virtual void > > > > DoRemoveURLs(const std::vector<GURL>& restrict_urls); > > > > On 2013/08/26 22:58:36, karenlees wrote: > > > > > On 2013/08/26 20:38:50, mvrable wrote: > > > > > > Doesn't need to be virtual since the method is private. > > > > > > > > > > Done - but should I make it protected instead, incase this gets > > subclassed? > > > > > > > > Personal opinion: it doesn't need to be as we can always go back later and > > > make > > > > it protected virtual if we need to subclass. But I don't have a strong > > > feeling > > > > on this. > > > > > > > > > I've merged with the latest changes. > > > > > > Adrienne - as we might also need these functions for the api now, I think it > > > would be better for me to move the history changes to a separate patch. Then > > we > > > can get these changes submitted today and start using the functions. I can > > send > > > out the patch for discussion with the history team as soon as this is in. > > What > > > do you think? > > > > Hi karen, I'm not 100% sure what you mean. Do you mean: add the deletion > > functions in this CL, and then add the calls to the functions from history > code > > in a separate CL? If so, that sounds good to me. > > Yep, I basically mean move these files to a different CL: > - chrome/browser/history/DEPS > - chrome/browser/history/history_service.cc > > Then I only need permission to submit to the activity_log directory. Sounds good. Ping me when you update the patch (remove the files) and I'll review.
On 2013/08/27 19:10:04, felt wrote: > On 2013/08/27 19:09:26, karenlees wrote: > > On 2013/08/27 18:44:38, felt wrote: > > > On 2013/08/27 18:35:52, karenlees wrote: > > > > On 2013/08/27 17:17:37, mvrable wrote: > > > > > lgtm > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... > > > > > File chrome/browser/extensions/activity_log/counting_policy.h (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... > > > > > chrome/browser/extensions/activity_log/counting_policy.h:74: virtual > void > > > > > DoRemoveURLs(const std::vector<GURL>& restrict_urls); > > > > > On 2013/08/26 22:58:36, karenlees wrote: > > > > > > On 2013/08/26 20:38:50, mvrable wrote: > > > > > > > Doesn't need to be virtual since the method is private. > > > > > > > > > > > > Done - but should I make it protected instead, incase this gets > > > subclassed? > > > > > > > > > > Personal opinion: it doesn't need to be as we can always go back later > and > > > > make > > > > > it protected virtual if we need to subclass. But I don't have a strong > > > > feeling > > > > > on this. > > > > > > > > > > > > I've merged with the latest changes. > > > > > > > > Adrienne - as we might also need these functions for the api now, I think > it > > > > would be better for me to move the history changes to a separate patch. > Then > > > we > > > > can get these changes submitted today and start using the functions. I > can > > > send > > > > out the patch for discussion with the history team as soon as this is in. > > > What > > > > do you think? > > > > > > Hi karen, I'm not 100% sure what you mean. Do you mean: add the deletion > > > functions in this CL, and then add the calls to the functions from history > > code > > > in a separate CL? If so, that sounds good to me. > > > > Yep, I basically mean move these files to a different CL: > > - chrome/browser/history/DEPS > > - chrome/browser/history/history_service.cc > > > > Then I only need permission to submit to the activity_log directory. > > Sounds good. Ping me when you update the patch (remove the files) and I'll > review. Done.
On 2013/08/27 19:26:17, karenlees wrote: > On 2013/08/27 19:10:04, felt wrote: > > On 2013/08/27 19:09:26, karenlees wrote: > > > On 2013/08/27 18:44:38, felt wrote: > > > > On 2013/08/27 18:35:52, karenlees wrote: > > > > > On 2013/08/27 17:17:37, mvrable wrote: > > > > > > lgtm > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... > > > > > > File chrome/browser/extensions/activity_log/counting_policy.h (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/18878009/diff/64001/chrome/browser/extensions... > > > > > > chrome/browser/extensions/activity_log/counting_policy.h:74: virtual > > void > > > > > > DoRemoveURLs(const std::vector<GURL>& restrict_urls); > > > > > > On 2013/08/26 22:58:36, karenlees wrote: > > > > > > > On 2013/08/26 20:38:50, mvrable wrote: > > > > > > > > Doesn't need to be virtual since the method is private. > > > > > > > > > > > > > > Done - but should I make it protected instead, incase this gets > > > > subclassed? > > > > > > > > > > > > Personal opinion: it doesn't need to be as we can always go back later > > and > > > > > make > > > > > > it protected virtual if we need to subclass. But I don't have a > strong > > > > > feeling > > > > > > on this. > > > > > > > > > > > > > > > I've merged with the latest changes. > > > > > > > > > > Adrienne - as we might also need these functions for the api now, I > think > > it > > > > > would be better for me to move the history changes to a separate patch. > > Then > > > > we > > > > > can get these changes submitted today and start using the functions. I > > can > > > > send > > > > > out the patch for discussion with the history team as soon as this is > in. > > > > What > > > > > do you think? > > > > > > > > Hi karen, I'm not 100% sure what you mean. Do you mean: add the deletion > > > > functions in this CL, and then add the calls to the functions from history > > > code > > > > in a separate CL? If so, that sounds good to me. > > > > > > Yep, I basically mean move these files to a different CL: > > > - chrome/browser/history/DEPS > > > - chrome/browser/history/history_service.cc > > > > > > Then I only need permission to submit to the activity_log directory. > > > > Sounds good. Ping me when you update the patch (remove the files) and I'll > > review. > > Done. Btw, I messed up a bit when first I merged your recent changes. I think I have everything back the way it should be but please double check I haven't changed anything I shouldn't have.
Hi Karen, I think the new cleanup method needs to be part of the delegate class in ActivityDatabase. I'm suspending my review since that'll require a little bit of refactoring. https://codereview.chromium.org/18878009/diff/96001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/activity_log.cc (right): https://codereview.chromium.org/18878009/diff/96001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_log.cc:431: if (!IsLogEnabled()) { I don't think the DLOG is needed -- I think just returning would suffice. You also should check if(policy_) https://codereview.chromium.org/18878009/diff/96001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/counting_policy.cc (right): https://codereview.chromium.org/18878009/diff/96001/chrome/browser/extensions... chrome/browser/extensions/activity_log/counting_policy.cc:592: << statement.GetSQLStatement(); I think you need to implement this method via the Delegate (like how the flushing is done) so that the ActivityDatabase can handle failures for you. That's why the other database-failure-sensitive methods return true/false -- false triggers error handling by the AD class.
Hi Adrienne, I thought it had been agreed in previous CL comments not to use the delegate class way. Before I start refactoring it for a third time, it would be great if you and Michael could agree on the correct way to do it. Alternatively, I would be happy for someone with more knowledge of how this code base is changing to patch in my changes and do the refactoring. I realize I should have finished this week's ago and that might have cut down on the need for multiple refactors, so I am sorry that I have been so slow. Please could you let me know the preferred way to proceed. https://codereview.chromium.org/18878009/diff/96001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/activity_log.cc (right): https://codereview.chromium.org/18878009/diff/96001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_log.cc:431: if (!IsLogEnabled()) { On 2013/08/27 19:59:03, felt wrote: > I don't think the DLOG is needed -- I think just returning would suffice. > > You also should check if(policy_) Done.
https://codereview.chromium.org/18878009/diff/96001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/counting_policy.cc (right): https://codereview.chromium.org/18878009/diff/96001/chrome/browser/extensions... chrome/browser/extensions/activity_log/counting_policy.cc:592: << statement.GetSQLStatement(); On 2013/08/27 19:59:03, felt wrote: > I think you need to implement this method via the Delegate (like how the > flushing is done) so that the ActivityDatabase can handle failures for you. > That's why the other database-failure-sensitive methods return true/false -- > false triggers error handling by the AD class. There are currently two styles--flushing goes via the Delegate, but reading is done directly and does not return a status code. Flushing is special because it can be triggered asynchronously by a timer in ActivityDatabase, so it needs to be part of the Delegate interface. I've looked a bit at the code, and I think the database is still notified of failures via DatabaseErrorCallback (which should cause SoftFailureClose to be called). We don't want to continue executing after an error, but perhaps just returning early from DoRemoveURLs will be enough. That is--any time there is an error, rather than just a LOG(ERROR) also include a return. Other cleanup code should still run as part of the DatabaseErrorCallback, without more work needed here. Adrienne, how does this seem?
On 2013/08/27 21:16:34, mvrable wrote: > https://codereview.chromium.org/18878009/diff/96001/chrome/browser/extensions... > File chrome/browser/extensions/activity_log/counting_policy.cc (right): > > https://codereview.chromium.org/18878009/diff/96001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/counting_policy.cc:592: << > statement.GetSQLStatement(); > On 2013/08/27 19:59:03, felt wrote: > > I think you need to implement this method via the Delegate (like how the > > flushing is done) so that the ActivityDatabase can handle failures for you. > > That's why the other database-failure-sensitive methods return true/false -- > > false triggers error handling by the AD class. > > There are currently two styles--flushing goes via the Delegate, but reading is > done directly and does not return a status code. Flushing is special because it > can be triggered asynchronously by a timer in ActivityDatabase, so it needs to > be part of the Delegate interface. > > I've looked a bit at the code, and I think the database is still notified of > failures via DatabaseErrorCallback (which should cause SoftFailureClose to be > called). We don't want to continue executing after an error, but perhaps just > returning early from DoRemoveURLs will be enough. > > That is--any time there is an error, rather than just a LOG(ERROR) also include > a return. Other cleanup code should still run as part of the > DatabaseErrorCallback, without more work needed here. > > Adrienne, how does this seem? Sorry Karen, I lost track of the thread and forgot we'd already discussed this. Can you go with Michael's suggestion, which is to return immediately after a failure?
https://codereview.chromium.org/18878009/diff/96001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/counting_policy.cc (right): https://codereview.chromium.org/18878009/diff/96001/chrome/browser/extensions... chrome/browser/extensions/activity_log/counting_policy.cc:592: << statement.GetSQLStatement(); On 2013/08/27 21:16:34, mvrable wrote: > On 2013/08/27 19:59:03, felt wrote: > > I think you need to implement this method via the Delegate (like how the > > flushing is done) so that the ActivityDatabase can handle failures for you. > > That's why the other database-failure-sensitive methods return true/false -- > > false triggers error handling by the AD class. > > There are currently two styles--flushing goes via the Delegate, but reading is > done directly and does not return a status code. Flushing is special because it > can be triggered asynchronously by a timer in ActivityDatabase, so it needs to > be part of the Delegate interface. > > I've looked a bit at the code, and I think the database is still notified of > failures via DatabaseErrorCallback (which should cause SoftFailureClose to be > called). We don't want to continue executing after an error, but perhaps just > returning early from DoRemoveURLs will be enough. > > That is--any time there is an error, rather than just a LOG(ERROR) also include > a return. Other cleanup code should still run as part of the > DatabaseErrorCallback, without more work needed here. > > Adrienne, how does this seem? Done.
just some nits the comment on testing is meant to apply to the other tests, too. https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... File chrome/browser/extensions/activity_log/counting_policy_unittest.cc (right): https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... chrome/browser/extensions/activity_log/counting_policy_unittest.cc:256: ASSERT_EQ(action_urls_cleared, actions->at(0)->PrintForDebug()); I'm trying to make a rule that new tests should directly check the values rather than rely on PrintForDebug. At some point I have to go back and clean up all the uses of printing strings. could you switch this to the newer style? https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:304: return; is this return in the wrong block?
https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... File chrome/browser/extensions/activity_log/counting_policy_unittest.cc (right): https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... chrome/browser/extensions/activity_log/counting_policy_unittest.cc:256: ASSERT_EQ(action_urls_cleared, actions->at(0)->PrintForDebug()); On 2013/08/27 23:16:55, felt wrote: > I'm trying to make a rule that new tests should directly check the values rather > than rely on PrintForDebug. At some point I have to go back and clean up all the > uses of printing strings. could you switch this to the newer style? Sure, if you already have the new rule can you point me to the code. As I'm not cc'd onto every CL I am struggling to keep up with the changes. Sorry for not knowing about the new way. I just used the same way that the other tests in this file used for consistency. https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:304: return; On 2013/08/27 23:16:55, felt wrote: > is this return in the wrong block? No, it was always supposed to return here as there is nothing more to do when clearing all CLs (in the counting policy there is, so there isn't a return here). As it returned here there was no point putting a return after the LOG(ERROR) as it returned anyway.
https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... File chrome/browser/extensions/activity_log/counting_policy_unittest.cc (right): https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... chrome/browser/extensions/activity_log/counting_policy_unittest.cc:256: ASSERT_EQ(action_urls_cleared, actions->at(0)->PrintForDebug()); On 2013/08/27 23:26:55, karenlees wrote: > On 2013/08/27 23:16:55, felt wrote: > > I'm trying to make a rule that new tests should directly check the values > rather > > than rely on PrintForDebug. At some point I have to go back and clean up all > the > > uses of printing strings. could you switch this to the newer style? > > Sure, if you already have the new rule can you point me to the code. As I'm not > cc'd onto every CL I am struggling to keep up with the changes. Sorry for not > knowing about the new way. I just used the same way that the other tests in > this file used for consistency. Np, I sort of originally imagined I would get to the cleanup sooner so you wouldn't have had to worry about this at all. ;) Basically, I'm asking you to do ASSERT_EQ("", actions->at(0)->page_url()); so that will check it's empty and not just missing from the log statement.
On 2013/08/27 23:32:47, felt wrote: > https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... > File chrome/browser/extensions/activity_log/counting_policy_unittest.cc (right): > > https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... > chrome/browser/extensions/activity_log/counting_policy_unittest.cc:256: > ASSERT_EQ(action_urls_cleared, actions->at(0)->PrintForDebug()); > On 2013/08/27 23:26:55, karenlees wrote: > > On 2013/08/27 23:16:55, felt wrote: > > > I'm trying to make a rule that new tests should directly check the values > > rather > > > than rely on PrintForDebug. At some point I have to go back and clean up all > > the > > > uses of printing strings. could you switch this to the newer style? > > > > Sure, if you already have the new rule can you point me to the code. As I'm > not > > cc'd onto every CL I am struggling to keep up with the changes. Sorry for not > > knowing about the new way. I just used the same way that the other tests in > > this file used for consistency. > > Np, I sort of originally imagined I would get to the cleanup sooner so you > wouldn't have had to worry about this at all. ;) Basically, I'm asking you to do > ASSERT_EQ("", actions->at(0)->page_url()); so that will check it's empty and not > just missing from the log statement. S. what will the new rule actually do? If I change the code to assert and then have to change it again to use the new rule, it all seems a bit pointless. IF I change, should I change all the tests in this file to make it consistent?
On 2013/08/27 23:34:59, karenlees wrote: > On 2013/08/27 23:32:47, felt wrote: > > > https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... > > File chrome/browser/extensions/activity_log/counting_policy_unittest.cc > (right): > > > > > https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... > > chrome/browser/extensions/activity_log/counting_policy_unittest.cc:256: > > ASSERT_EQ(action_urls_cleared, actions->at(0)->PrintForDebug()); > > On 2013/08/27 23:26:55, karenlees wrote: > > > On 2013/08/27 23:16:55, felt wrote: > > > > I'm trying to make a rule that new tests should directly check the values > > > rather > > > > than rely on PrintForDebug. At some point I have to go back and clean up > all > > > the > > > > uses of printing strings. could you switch this to the newer style? > > > > > > Sure, if you already have the new rule can you point me to the code. As I'm > > not > > > cc'd onto every CL I am struggling to keep up with the changes. Sorry for > not > > > knowing about the new way. I just used the same way that the other tests in > > > this file used for consistency. > > > > Np, I sort of originally imagined I would get to the cleanup sooner so you > > wouldn't have had to worry about this at all. ;) Basically, I'm asking you to > do > > ASSERT_EQ("", actions->at(0)->page_url()); so that will check it's empty and > not > > just missing from the log statement. > > S. what will the new rule actually do? If I change the code to assert and then > have to change it again to use the new rule, it all seems a bit pointless. IF I > change, should I change all the tests in this file to make it consistent? Maybe I was unclear. The new "rule" is just me asking everyone who writes new tests to check the actual action values, not the print statement. So instead of comparing to the print string, check the actual action value.
On 2013/08/27 23:42:34, felt wrote: > On 2013/08/27 23:34:59, karenlees wrote: > > On 2013/08/27 23:32:47, felt wrote: > > > > > > https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... > > > File chrome/browser/extensions/activity_log/counting_policy_unittest.cc > > (right): > > > > > > > > > https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... > > > chrome/browser/extensions/activity_log/counting_policy_unittest.cc:256: > > > ASSERT_EQ(action_urls_cleared, actions->at(0)->PrintForDebug()); > > > On 2013/08/27 23:26:55, karenlees wrote: > > > > On 2013/08/27 23:16:55, felt wrote: > > > > > I'm trying to make a rule that new tests should directly check the > values > > > > rather > > > > > than rely on PrintForDebug. At some point I have to go back and clean up > > all > > > > the > > > > > uses of printing strings. could you switch this to the newer style? > > > > > > > > Sure, if you already have the new rule can you point me to the code. As > I'm > > > not > > > > cc'd onto every CL I am struggling to keep up with the changes. Sorry for > > not > > > > knowing about the new way. I just used the same way that the other tests > in > > > > this file used for consistency. > > > > > > Np, I sort of originally imagined I would get to the cleanup sooner so you > > > wouldn't have had to worry about this at all. ;) Basically, I'm asking you > to > > do > > > ASSERT_EQ("", actions->at(0)->page_url()); so that will check it's empty and > > not > > > just missing from the log statement. > > > > S. what will the new rule actually do? If I change the code to assert and > then > > have to change it again to use the new rule, it all seems a bit pointless. IF > I > > change, should I change all the tests in this file to make it consistent? > > Maybe I was unclear. The new "rule" is just me asking everyone who writes new > tests to check the actual action values, not the print statement. So instead of > comparing to the print string, check the actual action value. (So changing to ASSERT_EQ("", actions->at(0)->page_url()); *is* the new style for testing. I think this is less likely to yield bugs, whereas printing can end up hiding bugs like the one you found last week.)
On 2013/08/27 23:43:43, felt wrote: > On 2013/08/27 23:42:34, felt wrote: > > On 2013/08/27 23:34:59, karenlees wrote: > > > On 2013/08/27 23:32:47, felt wrote: > > > > > > > > > > https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... > > > > File chrome/browser/extensions/activity_log/counting_policy_unittest.cc > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... > > > > chrome/browser/extensions/activity_log/counting_policy_unittest.cc:256: > > > > ASSERT_EQ(action_urls_cleared, actions->at(0)->PrintForDebug()); > > > > On 2013/08/27 23:26:55, karenlees wrote: > > > > > On 2013/08/27 23:16:55, felt wrote: > > > > > > I'm trying to make a rule that new tests should directly check the > > values > > > > > rather > > > > > > than rely on PrintForDebug. At some point I have to go back and clean > up > > > all > > > > > the > > > > > > uses of printing strings. could you switch this to the newer style? > > > > > > > > > > Sure, if you already have the new rule can you point me to the code. As > > I'm > > > > not > > > > > cc'd onto every CL I am struggling to keep up with the changes. Sorry > for > > > not > > > > > knowing about the new way. I just used the same way that the other > tests > > in > > > > > this file used for consistency. > > > > > > > > Np, I sort of originally imagined I would get to the cleanup sooner so you > > > > wouldn't have had to worry about this at all. ;) Basically, I'm asking you > > to > > > do > > > > ASSERT_EQ("", actions->at(0)->page_url()); so that will check it's empty > and > > > not > > > > just missing from the log statement. > > > > > > S. what will the new rule actually do? If I change the code to assert and > > then > > > have to change it again to use the new rule, it all seems a bit pointless. > IF > > I > > > change, should I change all the tests in this file to make it consistent? > > > > Maybe I was unclear. The new "rule" is just me asking everyone who writes new > > tests to check the actual action values, not the print statement. So instead > of > > comparing to the print string, check the actual action value. > > (So changing to ASSERT_EQ("", actions->at(0)->page_url()); *is* the new style > for testing. I think this is less likely to yield bugs, whereas printing can end > up hiding bugs like the one you found last week.) The problem with the bug I found last week is that no arg urls were added to actions, so they were never tested. The printing wouldn't make much difference to that. Anyway, sorry for reading your emails while in a meeting and getting confused. I have now added checks as you requested and written a helper function to check everything. That will force *all* the fields to be checked so nothing should be forgotten about in the tests. I have added the same function to both tests - so if there is a better place to put it where it can be used by both I can move it. If it would be helpful I could write another CL to clean up the remaining unittests as I like cleaning tests.
lgtm
On 2013/08/28 02:07:36, karenlees wrote: > On 2013/08/27 23:43:43, felt wrote: > > On 2013/08/27 23:42:34, felt wrote: > > > On 2013/08/27 23:34:59, karenlees wrote: > > > > On 2013/08/27 23:32:47, felt wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... > > > > > File chrome/browser/extensions/activity_log/counting_policy_unittest.cc > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... > > > > > chrome/browser/extensions/activity_log/counting_policy_unittest.cc:256: > > > > > ASSERT_EQ(action_urls_cleared, actions->at(0)->PrintForDebug()); > > > > > On 2013/08/27 23:26:55, karenlees wrote: > > > > > > On 2013/08/27 23:16:55, felt wrote: > > > > > > > I'm trying to make a rule that new tests should directly check the > > > values > > > > > > rather > > > > > > > than rely on PrintForDebug. At some point I have to go back and > clean > > up > > > > all > > > > > > the > > > > > > > uses of printing strings. could you switch this to the newer style? > > > > > > > > > > > > Sure, if you already have the new rule can you point me to the code. > As > > > I'm > > > > > not > > > > > > cc'd onto every CL I am struggling to keep up with the changes. Sorry > > for > > > > not > > > > > > knowing about the new way. I just used the same way that the other > > tests > > > in > > > > > > this file used for consistency. > > > > > > > > > > Np, I sort of originally imagined I would get to the cleanup sooner so > you > > > > > wouldn't have had to worry about this at all. ;) Basically, I'm asking > you > > > to > > > > do > > > > > ASSERT_EQ("", actions->at(0)->page_url()); so that will check it's empty > > and > > > > not > > > > > just missing from the log statement. > > > > > > > > S. what will the new rule actually do? If I change the code to assert and > > > then > > > > have to change it again to use the new rule, it all seems a bit pointless. > > > IF > > > I > > > > change, should I change all the tests in this file to make it consistent? > > > > > > Maybe I was unclear. The new "rule" is just me asking everyone who writes > new > > > tests to check the actual action values, not the print statement. So instead > > of > > > comparing to the print string, check the actual action value. > > > > (So changing to ASSERT_EQ("", actions->at(0)->page_url()); *is* the new style > > for testing. I think this is less likely to yield bugs, whereas printing can > end > > up hiding bugs like the one you found last week.) > > The problem with the bug I found last week is that no arg urls were added to > actions, so they were never tested. The printing wouldn't make much difference > to that. > > Anyway, sorry for reading your emails while in a meeting and getting confused. I > have now added checks as you requested and written a helper function to check > everything. That will force *all* the fields to be checked so nothing should be > forgotten about in the tests. I have added the same function to both tests - so > if there is a better place to put it where it can be used by both I can move it. > If it would be helpful I could write another CL to clean up the remaining > unittests as I like cleaning tests. Looks great! If you are interested in cleaning up the other tests, there is a bug: https://code.google.com/p/chromium/issues/detail?id=255728 :)
On 2013/08/28 05:19:26, felt wrote: > On 2013/08/28 02:07:36, karenlees wrote: > > On 2013/08/27 23:43:43, felt wrote: > > > On 2013/08/27 23:42:34, felt wrote: > > > > On 2013/08/27 23:34:59, karenlees wrote: > > > > > On 2013/08/27 23:32:47, felt wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... > > > > > > File > chrome/browser/extensions/activity_log/counting_policy_unittest.cc > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/18878009/diff/109001/chrome/browser/extension... > > > > > > > chrome/browser/extensions/activity_log/counting_policy_unittest.cc:256: > > > > > > ASSERT_EQ(action_urls_cleared, actions->at(0)->PrintForDebug()); > > > > > > On 2013/08/27 23:26:55, karenlees wrote: > > > > > > > On 2013/08/27 23:16:55, felt wrote: > > > > > > > > I'm trying to make a rule that new tests should directly check the > > > > values > > > > > > > rather > > > > > > > > than rely on PrintForDebug. At some point I have to go back and > > clean > > > up > > > > > all > > > > > > > the > > > > > > > > uses of printing strings. could you switch this to the newer > style? > > > > > > > > > > > > > > Sure, if you already have the new rule can you point me to the code. > > As > > > > I'm > > > > > > not > > > > > > > cc'd onto every CL I am struggling to keep up with the changes. > Sorry > > > for > > > > > not > > > > > > > knowing about the new way. I just used the same way that the other > > > tests > > > > in > > > > > > > this file used for consistency. > > > > > > > > > > > > Np, I sort of originally imagined I would get to the cleanup sooner so > > you > > > > > > wouldn't have had to worry about this at all. ;) Basically, I'm asking > > you > > > > to > > > > > do > > > > > > ASSERT_EQ("", actions->at(0)->page_url()); so that will check it's > empty > > > and > > > > > not > > > > > > just missing from the log statement. > > > > > > > > > > S. what will the new rule actually do? If I change the code to assert > and > > > > then > > > > > have to change it again to use the new rule, it all seems a bit > pointless. > > > > > IF > > > > I > > > > > change, should I change all the tests in this file to make it > consistent? > > > > > > > > Maybe I was unclear. The new "rule" is just me asking everyone who writes > > new > > > > tests to check the actual action values, not the print statement. So > instead > > > of > > > > comparing to the print string, check the actual action value. > > > > > > (So changing to ASSERT_EQ("", actions->at(0)->page_url()); *is* the new > style > > > for testing. I think this is less likely to yield bugs, whereas printing can > > end > > > up hiding bugs like the one you found last week.) > > > > The problem with the bug I found last week is that no arg urls were added to > > actions, so they were never tested. The printing wouldn't make much difference > > to that. > > > > Anyway, sorry for reading your emails while in a meeting and getting confused. > I > > have now added checks as you requested and written a helper function to check > > everything. That will force *all* the fields to be checked so nothing should > be > > forgotten about in the tests. I have added the same function to both tests - > so > > if there is a better place to put it where it can be used by both I can move > it. > > If it would be helpful I could write another CL to clean up the remaining > > unittests as I like cleaning tests. > > Looks great! If you are interested in cleaning up the other tests, there is a > bug: https://code.google.com/p/chromium/issues/detail?id=255728 :) Sure, I'll take a look when I've got the CL's I've promised you this week finished :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/karenlees@chromium.org/18878009/123001
Sorry for I got bad news for ya. Compile failed with a clobber build on android_dbg. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
On 2013/08/28 16:02:02, I haz the power (commit-bot) wrote: > Sorry for I got bad news for ya. > Compile failed with a clobber build on android_dbg. > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... > Your code is likely broken or HEAD is junk. Please ensure your > code is not broken then alert the build sheriffs. > Look at the try server FAQ for more details. I need to make an iterator a const_iterator. Good point, not sure why it wasn't caught when building on linux, I guess the android compiler is more strict. Rebuilding and testing locally with const_iterator. Will upload fix shortly. Error was: ../../chrome/browser/extensions/activity_log/activity_log.cc:443:58: error: conversion from 'std::set<GURL>::const_iterator {aka std::priv::_Rb_tree_iterator<GURL, std::priv::_ConstSetTraitsT<GURL> >}' to non-scalar type 'std::set<GURL>::iterator {aka std::priv::_Rb_tree_iterator<GURL, std::priv::_SetTraitsT<GURL> >}' requested
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/karenlees@chromium.org/18878009/146001
Message was sent while issue was closed.
Change committed as 220075 |
