Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15573003:
New architecture of the activity logging: Policies for summarization (and compression) (Closed)
|
Created:
7 years, 7 months ago by dbabic Modified:
7 years, 6 months ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionActivity is now logged through policies. Policies are a flexible mechanism for summarization of the log data. Each Policy object encapsulates storage (for instance, could be an SQLite database or just append logs), summarization (for instance, frequency counting of various API calls), and possibly compression.
BUG=238256
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=206785
Patch Set 1 #Patch Set 2 : Fixed compiler errors on different platforms (at least I hope so). #
Total comments: 60
Patch Set 3 : Addressed another batch of Adrienne's comments #Patch Set 4 : Fixed buggy test #Patch Set 5 : More fixes #Patch Set 6 : Yet more fixes #Patch Set 7 : A few more fixes :-( #
Total comments: 47
Patch Set 8 : Addressed Adrienne's new comments and fixed lint warnings. #Patch Set 9 : Fixed a missing OVERRIDE. #Patch Set 10 : Another iteration... #Patch Set 11 : Removed virtual call from destructor #Patch Set 12 : Wrote unit tests #Patch Set 13 : Changed the thread running tests. #Patch Set 14 : Fixing test failures #
Total comments: 2
Patch Set 15 : Addressed Adrienne's final comments. #
Total comments: 21
Patch Set 16 : Addressed Matt's comments. #Patch Set 17 : Replaced stringstream with append, added a TODO. #Patch Set 18 : Merged policies and ActivityLogPrivate. #Patch Set 19 : Merge (partial, commented out redundant code, but haven't removed it). #Patch Set 20 : Merge with master. #
Total comments: 37
Patch Set 21 : Adressed Matt and Adrienne's comments. #
Total comments: 6
Patch Set 22 : Fixed compilation error on Chrome OS, addressed Matt's final comments. #Patch Set 23 : Another Chrome OS fix. #Patch Set 24 : Commented out original cmnd line restoration. #Patch Set 25 : Fixed policy initialization and dispatching of SetBatchModeForTesting." #Patch Set 26 : Fixed a compilation error. #Patch Set 27 : Uncommented the cmnd line restoration. #Patch Set 28 : Fixed SEGV by removing the IsLogEnabled call from the constructor. #Patch Set 29 : Changed the CALL in tests to lowercase. #Patch Set 30 : Fixed browser test. #Messages
Total messages: 45 (0 generated)
Hi, Seems like most tests are passing. I have a few more running, but it looks promising. Would you please be willing to review the diff now? I have the interview workshop starting at 1pm, and it would be great if we could square away the diff by then. I'll have another 30min in the late afternoon. Thanks! Best,
Hey, I'm still going through this, but in the meantime, can you update the issue to have: (1) A more detailed description of the changes in the issue description. (What's the purpose of policies? What role do they play? What did you refactor?) (2) Please update the bug ID -- I don't think it's the right one. https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/activity_log/activity_log.cc (right): https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/activity_log.cc:267: if (!testing_mode_ && Shouldn't this be moved into one of the policies? https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/activity_log.cc:277: CHECK(extension && "Must pass a valid extension ptr."); In general, you probably want DCHECK here. Regarding this specific problem: note that a currently unsolved problem (that luckily only occurs very infrequently) is that sometimes extensions are uninstalled while we're in the middle of logging an activity. Coming up with a way to handle that is on my TODO list, but keep in mind that at present this CHECK/DCHECK may actually fire. https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/activity_log.cc:288: // doesn't require construction of the action object. Regardless, the following code will all be deleted when I implement the API and get rid of the old UI in a subsequent CL.
Hi Domagoj, I'll take another pass after you make these changes. Could you also update the title of the CL to be more meaningful (maybe "Add support for summarization policies to the Activity Log" or something)? I have 9 issues visible in my dashboard so it's helpful to have a unique name. Thanks! Adrienne https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/activity_log/activity_log_policy.cc (right): https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/activity_log_policy.cc:20: CHECK(profile && "Null ptr dereference"); Always use DCHECK -- DCHECK will crash in debug mode, but not on release browsers. With that being said, I'm not sure you need this check. If profile is null the browser has huge problems ;) P.S. More information on CHECK/DCHECK: http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED- https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/activity_log_policy.cc:25: ActionType action_ty, this can be action_type. https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/activity_log_policy.cc:31: Remove blank lines at beginning of methods, for this method and all new methods added. Reference on vertical whitespace: https://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Vertical_Whit... https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/activity_log_policy.cc:34: // Check timestamp and save state if needed Please revert to using the timer that flushes on destruction. What if there simply aren't any more calls (we'd miss the last few)? What if the browser closes before it's time? https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/activity_log/activity_log_policy.h (right): https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/activity_log_policy.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. The style guide was updated very recently -- can you remove the "(c)"? See: http://www.chromium.org/developers/coding-style#TOC-File-headers -- Same goes for all other new files (but no need to change old files) https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/activity_log_policy.h:44: ACTION_ON_SCRIPTS, Is there a reason why you separate this out into more types than we had before? (Not saying it's wrong-- wondering about the rationale. Perhaps the API should be updated to have more types.) https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/activity_log_policy.h:59: UI_KEY_... Please delete the /* ... */ https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/activity_log_policy.h:71: const Extension&, All parameters should be named. This goes for all method signatures in all the new classes. https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/activity_log_policy.h:74: const base::ListValue* /* arguments */, For consistency, switch to the // style of comment after the comma https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/activity_log_policy.h:97: // reproducible. Can you rename to not use "Sync", since that is usually used to refer to syncing with a server? I'd prefer you stick with something to do with batching, but more importantly don't use sync. https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/activity_log_policy.h:104: protected: Why are these protected instead of private? (Is it for inheritance? If so, what about using friend classes?) https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/activity_log_policy.h:106: virtual void DoProcessAction(ActionType, const Extension&, Please take a look at https://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decl... and then redo how you're lining up your arguments (both here and in other new methods). https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/activity_log_policy.h:110: base::Time last_sync_timestamp_; What is this for? Why not implement this with the timer? https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/activity_log_policy.h:112: bool sync_on_request_only_; What does this variable name mean? Is it equivalent to something named batching_on_ ? https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:78: FilePath base_dir = profile->GetPath(); Isn't this redundant with base::FilePath profile_base_path_ ? https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:134: std::stringstream& processed_args) const { Style guide says not to use stringstreams. Can you go back to having a separate return value? https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... File chrome/browser/extensions/activity_log/fullstream_ui_policy.h (right): https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:30: // TODO(felt,dbabic) This is overly specific to FullStreamUIPolicy. I don't understand why this too specific. Can you explain? https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:37: virtual void GetKey(ActivityLogPolicy::KeyType, std::string&) const OVERRIDE; Line too long? https://codereview.chromium.org/15573003/diff/3001/chrome/browser/extensions/... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:80: content::BrowserThread::ID dispatch_thread_; Could the Activity Log specify the thread, instead of leaving it up to the policy? (So, take it in the constructor?) More generally, could the templating be in the parent class so it only has to be done once?
https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... File chrome/browser/extensions/activity_log/activity_log.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log.cc:267: if (!testing_mode_ && On 2013/05/22 19:55:46, felt wrote: > Shouldn't this be moved into one of the policies? Sure, if that's your preference. If you expect that the whitelist might change in the future, and vary from policy to policy, then that would certainly make sense. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log.cc:277: CHECK(extension && "Must pass a valid extension ptr."); On 2013/05/22 19:55:46, felt wrote: > In general, you probably want DCHECK here. > > Regarding this specific problem: note that a currently unsolved problem (that > luckily only occurs very infrequently) is that sometimes extensions are > uninstalled while we're in the middle of logging an activity. Coming up with a > way to handle that is on my TODO list, but keep in mind that at present this > CHECK/DCHECK may actually fire. Infrequent or not, the old ActivityLog code will crash if that happens. Â I will add an explicit check at the beginning of all Log* member functions. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log.cc:288: // doesn't require construction of the action object. On 2013/05/22 19:55:46, felt wrote: > Regardless, the following code will all be deleted when I implement the API and > get rid of the old UI in a subsequent CL. Ok. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... File chrome/browser/extensions/activity_log/activity_log_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.cc:20: CHECK(profile && "Null ptr dereference"); On 2013/05/22 21:17:59, felt wrote: > Always use DCHECK -- DCHECK will crash in debug mode, but not on release > browsers. > > With that being said, I'm not sure you need this check. If profile is null the > browser has huge problems ;) > > P.S. More information on CHECK/DCHECK: > http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED- It's usually good to check for things that would crash the browser anyways with CHECK, as at least it produces some trace of what happened. If you don't mind, I'd leave it here, even though profile is never expected to be NULL. That way, I document the invariants and effectively provide a contract "if invariants hold, my code works". https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.cc:25: ActionType action_ty, On 2013/05/22 21:17:59, felt wrote: > this can be action_type. Done. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.cc:31: On 2013/05/22 21:17:59, felt wrote: > Remove blank lines at beginning of methods, for this method and all new methods > added. Reference on vertical whitespace: > https://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Vertical_Whit... Done. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.cc:34: // Check timestamp and save state if needed On 2013/05/22 21:17:59, felt wrote: > Please revert to using the timer that flushes on destruction. What if there > simply aren't any more calls (we'd miss the last few)? What if the browser > closes before it's time? Could you suggest the class I should use? https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... File chrome/browser/extensions/activity_log/activity_log_policy.h (right): https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.h:1: // Copyright (c) 2013 The Chromium Authors. All rights reserved. On 2013/05/22 21:17:59, felt wrote: > The style guide was updated very recently -- can you remove the "(c)"? See: > http://www.chromium.org/developers/coding-style#TOC-File-headers -- Same goes > for all other new files (but no need to change old files) Done. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.h:44: ACTION_ON_SCRIPTS, On 2013/05/22 21:17:59, felt wrote: > Is there a reason why you separate this out into more types than we had before? > (Not saying it's wrong-- wondering about the rationale. Perhaps the API should > be updated to have more types.) Done. You are probably referring to the ON_SCRIPTS one? I used it in some older versions of the code, but forgot to remove it. All others seem to be required. The first two are required for LogAPIAction and LogEventAction. LogBlockedAction uses the third, LogDOMAction the fourth, LogWebRequestAction the fifth. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.h:59: UI_KEY_... On 2013/05/22 21:17:59, felt wrote: > Please delete the /* ... */ Done. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.h:71: const Extension&, On 2013/05/22 21:17:59, felt wrote: > All parameters should be named. This goes for all method signatures in all the > new classes. Done. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.h:74: const base::ListValue* /* arguments */, On 2013/05/22 21:17:59, felt wrote: > For consistency, switch to the // style of comment after the comma Done. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.h:97: // reproducible. On 2013/05/22 21:17:59, felt wrote: > Can you rename to not use "Sync", since that is usually used to refer to syncing > with a server? I'd prefer you stick with something to do with batching, but more > importantly don't use sync. Done. I feel that calling it "batching" is a bit misleading, as batching has many overloaded meanings. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.h:104: protected: On 2013/05/22 21:17:59, felt wrote: > Why are these protected instead of private? (Is it for inheritance? If so, what > about using friend classes?) Done. Good question for DoProcessAction, I removed that from the protected API. The fields need to be protected for inheritance. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.h:106: virtual void DoProcessAction(ActionType, const Extension&, On 2013/05/22 21:17:59, felt wrote: > Please take a look at > https://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decl... > and then redo how you're lining up your arguments (both here and in other new > methods). Done. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.h:110: base::Time last_sync_timestamp_; On 2013/05/22 21:17:59, felt wrote: > What is this for? Why not implement this with the timer? That's the periodic state saving feature Ulfar suggested. About the timer, please see my comments in the cc file. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.h:112: bool sync_on_request_only_; On 2013/05/22 21:17:59, felt wrote: > What does this variable name mean? Is it equivalent to something named > batching_on_ ? Yes. I've renamed it now: _sync_ -> _save_state_ https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:78: FilePath base_dir = profile->GetPath(); On 2013/05/22 21:17:59, felt wrote: > Isn't this redundant with base::FilePath profile_base_path_ ? Done. Good catch! https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:134: std::stringstream& processed_args) const { On 2013/05/22 21:17:59, felt wrote: > Style guide says not to use stringstreams. Can you go back to having a separate > return value? String concatenation, as implemented in MakeArgList, is terribly inefficient, more precisely, quadratic in the size of the string, in the worst case. Stringstreams are the standard way of doing such things, but if guide says no stringstreams, what is the standard way people do this at Google (or in Chrome, if this is a Chrome-specific guideline)? https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... File chrome/browser/extensions/activity_log/fullstream_ui_policy.h (right): https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:30: // TODO(felt,dbabic) This is overly specific to FullStreamUIPolicy. On 2013/05/22 21:17:59, felt wrote: > I don't understand why this too specific. Can you explain? It assumes that the callback can return a sorted vector of actions. Some policies might not do that. For instance, imagine a trivial policy that just counts the frequency of certain actions within some time period, this call would be meaningless, as it couldn't return anything useful. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:37: virtual void GetKey(ActivityLogPolicy::KeyType, std::string&) const OVERRIDE; On 2013/05/22 21:17:59, felt wrote: > Line too long? Done. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:80: content::BrowserThread::ID dispatch_thread_; On 2013/05/22 21:17:59, felt wrote: > Could the Activity Log specify the thread, instead of leaving it up to the > policy? (So, take it in the constructor?) More generally, could the templating > be in the parent class so it only has to be done once? Sure, it could be easily passed as a param. Sorry, didn't quite understand how would you template the parent class and what would be the benefit of that?
Could you make the changes to the issue description & upload your new patchset? https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... File chrome/browser/extensions/activity_log/activity_log.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log.cc:267: if (!testing_mode_ && Yes, I expect this to change. Please move it into the policy that redacts args. On 2013/05/23 01:35:04, dbabic wrote: > On 2013/05/22 19:55:46, felt wrote: > > Shouldn't this be moved into one of the policies? > > Sure, if that's your preference. If you expect that the whitelist might change > in the future, and vary from policy to policy, then that would certainly make > sense. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log.cc:277: CHECK(extension && "Must pass a valid extension ptr."); Yup, just letting you know it's a known bug. Please still either use DCHECK as per the style guide *or* add a quick fix (have it simply return if it's null). On 2013/05/23 01:35:04, dbabic wrote: > On 2013/05/22 19:55:46, felt wrote: > > In general, you probably want DCHECK here. > > > > Regarding this specific problem: note that a currently unsolved problem (that > > luckily only occurs very infrequently) is that sometimes extensions are > > uninstalled while we're in the middle of logging an activity. Coming up with a > > way to handle that is on my TODO list, but keep in mind that at present this > > CHECK/DCHECK may actually fire. > > Infrequent or not, the old ActivityLog code will crash if that happens. I will > add an explicit check at the beginning of all Log* member functions. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... File chrome/browser/extensions/activity_log/activity_log_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.cc:20: CHECK(profile && "Null ptr dereference"); Other reviewers have heckled me for this in the past -- not wanting extra checks. Leave it for now but be aware you might still be asked to remove it by another reviewer. On 2013/05/23 01:35:04, dbabic wrote: > On 2013/05/22 21:17:59, felt wrote: > > Always use DCHECK -- DCHECK will crash in debug mode, but not on release > > browsers. > > > > With that being said, I'm not sure you need this check. If profile is null the > > browser has huge problems ;) > > > > P.S. More information on CHECK/DCHECK: > > > http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED- > > It's usually good to check for things that would crash the browser anyways with > CHECK, as at least it produces some trace of what happened. If you don't mind, > I'd leave it here, even though profile is never expected to be NULL. That way, > I document the invariants and effectively provide a contract "if invariants > hold, my code works". https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.cc:34: // Check timestamp and save state if needed Yes, copy how it was implemented in activity_database.cc: with a base::RepeatingTimer<ActivityDatabase> timer_. See everything pertaining to ActivityDatabase::RecordBatchedActions and timer_; also check out the destructor. On 2013/05/23 01:35:04, dbabic wrote: > On 2013/05/22 21:17:59, felt wrote: > > Please revert to using the timer that flushes on destruction. What if there > > simply aren't any more calls (we'd miss the last few)? What if the browser > > closes before it's time? > > Could you suggest the class I should use? https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... File chrome/browser/extensions/activity_log/activity_log_policy.h (right): https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.h:110: base::Time last_sync_timestamp_; I think there is some confusion -- Ulfar wasn't suggesting a new feature, he was describing the timer I'd already implemented. On 2013/05/23 01:35:04, dbabic wrote: > On 2013/05/22 21:17:59, felt wrote: > > What is this for? Why not implement this with the timer? > > That's the periodic state saving feature Ulfar suggested. About the timer, > please see my comments in the cc file. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:134: std::stringstream& processed_args) const { Why is MakeArgList quadratic in the size of the string? Maybe I'm being dense-- but it looks to me like each value is touched twice (except for the first one, which has three operations on it). On 2013/05/23 01:35:04, dbabic wrote: > On 2013/05/22 21:17:59, felt wrote: > > Style guide says not to use stringstreams. Can you go back to having a > separate > > return value? > > String concatenation, as implemented in MakeArgList, is terribly inefficient, > more precisely, quadratic in the size of the string, in the worst case. > Stringstreams are the standard way of doing such things, but if guide says no > stringstreams, what is the standard way people do this at Google (or in Chrome, > if this is a Chrome-specific guideline)? https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... File chrome/browser/extensions/activity_log/fullstream_ui_policy.h (right): https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:30: // TODO(felt,dbabic) This is overly specific to FullStreamUIPolicy. Ahhh, OK. Can you add this to the comment so that I remember? On 2013/05/23 01:35:04, dbabic wrote: > On 2013/05/22 21:17:59, felt wrote: > > I don't understand why this too specific. Can you explain? > > It assumes that the callback can return a sorted vector of actions. Some > policies might not do that. For instance, imagine a trivial policy that just > counts the frequency of certain actions within some time period, this call would > be meaningless, as it couldn't return anything useful. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:80: content::BrowserThread::ID dispatch_thread_; Sorry to be unclear: by "the templating" I meant the code for ScheduleAndForget, so that it doesn't have to be re-implemented in all of the policies. On 2013/05/23 01:35:04, dbabic wrote: > On 2013/05/22 21:17:59, felt wrote: > > Could the Activity Log specify the thread, instead of leaving it up to the > > policy? (So, take it in the constructor?) More generally, could the templating > > be in the parent class so it only has to be done once? > > Sure, it could be easily passed as a param. Sorry, didn't quite understand how > would you template the parent class and what would be the benefit of that? https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy.h (right): https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.h:12: class StreamWithoutArgsUIPolicy : public FullStreamUIPolicy { Is there a .cc file that goes with this to implement ProcessArguments?
Hi, I think I've addressed all your comments. It took me a while to debug all the issues with compiling and testing on various platforms. Cheers, https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... File chrome/browser/extensions/activity_log/activity_log.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log.cc:267: if (!testing_mode_ && On 2013/05/23 04:20:01, felt wrote: > Yes, I expect this to change. Please move it into the policy that redacts args. > > On 2013/05/23 01:35:04, dbabic wrote: > > On 2013/05/22 19:55:46, felt wrote: > > > Shouldn't this be moved into one of the policies? > > > > Sure, if that's your preference. If you expect that the whitelist might > change > > in the future, and vary from policy to policy, then that would certainly make > > sense. > Done. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log.cc:277: CHECK(extension && "Must pass a valid extension ptr."); On 2013/05/23 04:20:01, felt wrote: > Yup, just letting you know it's a known bug. Please still either use DCHECK as > per the style guide *or* add a quick fix (have it simply return if it's null). > > On 2013/05/23 01:35:04, dbabic wrote: > > On 2013/05/22 19:55:46, felt wrote: > > > In general, you probably want DCHECK here. > > > > > > Regarding this specific problem: note that a currently unsolved problem > (that > > > luckily only occurs very infrequently) is that sometimes extensions are > > > uninstalled while we're in the middle of logging an activity. Coming up with > a > > > way to handle that is on my TODO list, but keep in mind that at present this > > > CHECK/DCHECK may actually fire. > > > > Infrequent or not, the old ActivityLog code will crash if that happens. > I will > > add an explicit check at the beginning of all Log* member functions. > Done. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... File chrome/browser/extensions/activity_log/activity_log_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.cc:34: // Check timestamp and save state if needed On 2013/05/23 04:20:01, felt wrote: > Yes, copy how it was implemented in activity_database.cc: with a > base::RepeatingTimer<ActivityDatabase> timer_. See everything pertaining to > ActivityDatabase::RecordBatchedActions and timer_; also check out the > destructor. > > On 2013/05/23 01:35:04, dbabic wrote: > > On 2013/05/22 21:17:59, felt wrote: > > > Please revert to using the timer that flushes on destruction. What if there > > > simply aren't any more calls (we'd miss the last few)? What if the browser > > > closes before it's time? > > > > Could you suggest the class I should use? > Done. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... File chrome/browser/extensions/activity_log/activity_log_policy.h (right): https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/activity_log_policy.h:110: base::Time last_sync_timestamp_; On 2013/05/23 04:20:01, felt wrote: > I think there is some confusion -- Ulfar wasn't suggesting a new feature, he was > describing the timer I'd already implemented. > > On 2013/05/23 01:35:04, dbabic wrote: > > On 2013/05/22 21:17:59, felt wrote: > > > What is this for? Why not implement this with the timer? > > > > That's the periodic state saving feature Ulfar suggested. About the timer, > > please see my comments in the cc file. > Ok. Done. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... File chrome/browser/extensions/activity_log/fullstream_ui_policy.h (right): https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:30: // TODO(felt,dbabic) This is overly specific to FullStreamUIPolicy. On 2013/05/23 04:20:01, felt wrote: > Ahhh, OK. Can you add this to the comment so that I remember? > > On 2013/05/23 01:35:04, dbabic wrote: > > On 2013/05/22 21:17:59, felt wrote: > > > I don't understand why this too specific. Can you explain? > > > > It assumes that the callback can return a sorted vector of actions. Some > > policies might not do that. For instance, imagine a trivial policy that just > > counts the frequency of certain actions within some time period, this call > would > > be meaningless, as it couldn't return anything useful. > Done. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:80: content::BrowserThread::ID dispatch_thread_; On 2013/05/23 04:20:01, felt wrote: > Sorry to be unclear: by "the templating" I meant the code for ScheduleAndForget, > so that it doesn't have to be re-implemented in all of the policies. > > On 2013/05/23 01:35:04, dbabic wrote: > > On 2013/05/22 21:17:59, felt wrote: > > > Could the Activity Log specify the thread, instead of leaving it up to the > > > policy? (So, take it in the constructor?) More generally, could the > templating > > > be in the parent class so it only has to be done once? > > > > Sure, it could be easily passed as a param. Sorry, didn't quite understand > how > > would you template the parent class and what would be the benefit of that? > Done. https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy.h (right): https://chromiumcodereview.appspot.com/15573003/diff/3001/chrome/browser/exte... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.h:12: class StreamWithoutArgsUIPolicy : public FullStreamUIPolicy { On 2013/05/23 04:20:01, felt wrote: > Is there a .cc file that goes with this to implement ProcessArguments? Done. Now there is. There wasn't a need for it before.
Hi Domagoj, a few more comments. Please ping me when you've uploaded the new policy tests so I know to look. https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/activity_log.cc (right): https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_log.cc:132: switch (policy_type_) { Where is policy_type_ set? https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_log.cc:171: if (!extension) Note that these checks help with but still don't solve the TOCTOU problem. Can you add a TODO here so I can add a real fix in another version? (In the future: I'll get rid of the dependency on having the extension object, and just take the extension ID as a parameter.) https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_log.cc:309: Delete the extra vertical spaces at the beginning of methods (both here and elsewhere) https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/activity_log.h (right): https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_log.h:128: static void SetDefaultPolicy(ActivityLogPolicy::PolicyType policy_type) { Can you add a comment describing what this is used for? https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/activity_log_policy.cc (right): https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_log_policy.cc:24: &ActivityLogPolicy::SaveState); OK, I was confused. I thought you were going to move more of the ActivityDatabase functionality into the ActivityLogPolicy [sub]classes. Given that the ActivityDatabase class ended up unchanged, you can get rid of all of the timer_ and SaveState logic in the policy [sub]classes. (Since the ActivityDatabase already takes care of it.) All that needs to be done: the FullStreamUIPolicy needs to call db_->SetBatchModeForTesting(false) to explicitly disable it (since we don't want to batch writes when experimenting). All other policies can just ignore it, they will magically get batched writes as long as they use the ActivityDatabase class. https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/activity_log_policy.h (right): https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_log_policy.h:39: class ActivityLogPolicy { Can you please specify that all of these methods can be called on the UI thread, because all DB operations are dispatched via the ActivityDatabase. https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_log_policy.h:96: // of the FullStreamUIPolicy. We should refractor it to use the above "refractor" -> "refactor" https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:96: Callback<void(scoped_ptr<std::vector<scoped_refptr<Action> > >)>& callback) Is there enough room to indent the callback another 4 so we can see it's continued from previous line? (I can't tell in the code review dashboard) https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:110: key = "fsuip.reason"; Can you make these k constants? Since they're only used within the class, they can be in an anonymous namespace in this file. https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:130: void FullStreamUIPolicy::ProcessArguments( Try checking out base/strings classes to see if you can find anything to replace the stringstream: https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/&q=pa... https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:183: dummy /* TODO(dbabic,felt) Drop in the next revision */); use // for consistency, here and in the other parts of the switch statement P.S. Petr might have found a use for extras, so we might not want to drop them. (Keep the comment though so we remember to discuss it.) https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:265: } // End of the extensions namespace "namespace extensions" https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/fullstream_ui_policy.h (right): https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:19: public: "public:" should be indented 1 space, and all the method signatures should be indented 2 spaces. (This goes for all the other files too.) https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:20: no vspace between "public" and comment https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:66: // We initialize this on the same thread as the ActivityLog, but then "ActivityLog and policy, but then" https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc (right): https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:17: for (int i = 0; i < APIAction::kSizeAlwaysLog; i++) { ++i (I often do it the other way around out of habit-- so if you copied my code here, sorry for inheriting my bad habit) https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:42: // unless testing is enabled. Change comment to match what's happening here https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:52: } // End of the extensions namespace "namespace extensions"
Another thing I missed: If you look on the right hand side of the patchset, there is a column named "Lint". They might look like question marks. Click on each of them and you will see the lint errors for each file. There are a number (out-of-order includes, etc.). Please fix those too. On 2013/05/24 18:43:37, felt wrote: > Hi Domagoj, a few more comments. Please ping me when you've uploaded the new > policy tests so I know to look. > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > File chrome/browser/extensions/activity_log/activity_log.cc (right): > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/activity_log.cc:132: switch > (policy_type_) { > Where is policy_type_ set? > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/activity_log.cc:171: if (!extension) > Note that these checks help with but still don't solve the TOCTOU problem. Can > you add a TODO here so I can add a real fix in another version? (In the future: > I'll get rid of the dependency on having the extension object, and just take the > extension ID as a parameter.) > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/activity_log.cc:309: > Delete the extra vertical spaces at the beginning of methods (both here and > elsewhere) > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > File chrome/browser/extensions/activity_log/activity_log.h (right): > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/activity_log.h:128: static void > SetDefaultPolicy(ActivityLogPolicy::PolicyType policy_type) { > Can you add a comment describing what this is used for? > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > File chrome/browser/extensions/activity_log/activity_log_policy.cc (right): > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/activity_log_policy.cc:24: > &ActivityLogPolicy::SaveState); > OK, I was confused. I thought you were going to move more of the > ActivityDatabase functionality into the ActivityLogPolicy [sub]classes. Given > that the ActivityDatabase class ended up unchanged, you can get rid of all of > the timer_ and SaveState logic in the policy [sub]classes. (Since the > ActivityDatabase already takes care of it.) > > All that needs to be done: the FullStreamUIPolicy needs to call > db_->SetBatchModeForTesting(false) to explicitly disable it (since we don't want > to batch writes when experimenting). All other policies can just ignore it, they > will magically get batched writes as long as they use the ActivityDatabase > class. > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > File chrome/browser/extensions/activity_log/activity_log_policy.h (right): > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/activity_log_policy.h:39: class > ActivityLogPolicy { > Can you please specify that all of these methods can be called on the UI thread, > because all DB operations are dispatched via the ActivityDatabase. > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/activity_log_policy.h:96: // of the > FullStreamUIPolicy. We should refractor it to use the above > "refractor" -> "refactor" > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:96: > Callback<void(scoped_ptr<std::vector<scoped_refptr<Action> > >)>& callback) > Is there enough room to indent the callback another 4 so we can see it's > continued from previous line? (I can't tell in the code review dashboard) > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:110: key = > "fsuip.reason"; > Can you make these k constants? Since they're only used within the class, they > can be in an anonymous namespace in this file. > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:130: void > FullStreamUIPolicy::ProcessArguments( > Try checking out base/strings classes to see if you can find anything to replace > the stringstream: > https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/&q=pa... > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:183: dummy /* > TODO(dbabic,felt) Drop in the next revision */); > use // for consistency, here and in the other parts of the switch statement > > P.S. Petr might have found a use for extras, so we might not want to drop them. > (Keep the comment though so we remember to discuss it.) > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:265: } // End of > the extensions namespace > "namespace extensions" > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > File chrome/browser/extensions/activity_log/fullstream_ui_policy.h (right): > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/fullstream_ui_policy.h:19: public: > "public:" should be indented 1 space, and all the method signatures should be > indented 2 spaces. (This goes for all the other files too.) > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/fullstream_ui_policy.h:20: > no vspace between "public" and comment > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/fullstream_ui_policy.h:66: // We > initialize this on the same thread as the ActivityLog, but then > "ActivityLog and policy, but then" > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > File chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc (right): > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:17: for (int i > = 0; i < APIAction::kSizeAlwaysLog; i++) { > ++i > (I often do it the other way around out of habit-- so if you copied my code > here, sorry for inheriting my bad habit) > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:42: // unless > testing is enabled. > Change comment to match what's happening here > > https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... > chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:52: } // End > of the extensions namespace > "namespace extensions"
https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... File chrome/browser/extensions/activity_log/activity_log.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/activity_log.cc:132: switch (policy_type_) { On 2013/05/24 18:43:38, felt wrote: > Where is policy_type_ set? There's a static constructor at the end of this file. Since ActivityLog is a singleton, that should be fine. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/activity_log.cc:171: if (!extension) On 2013/05/24 18:43:38, felt wrote: > Note that these checks help with but still don't solve the TOCTOU problem. Can > you add a TODO here so I can add a real fix in another version? (In the future: > I'll get rid of the dependency on having the extension object, and just take the > extension ID as a parameter.) Sorry, I didn't understand why this doesn't fix the problem. Are you saying that the extension object can get deallocated in between the check (line 171) and use (say, line 187)? Taking extension ID would be a great fix. I'll add a comment anyways. Done. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/activity_log.cc:309: On 2013/05/24 18:43:38, felt wrote: > Delete the extra vertical spaces at the beginning of methods (both here and > elsewhere) Done. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... File chrome/browser/extensions/activity_log/activity_log.h (right): https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/activity_log.h:128: static void SetDefaultPolicy(ActivityLogPolicy::PolicyType policy_type) { On 2013/05/24 18:43:38, felt wrote: > Can you add a comment describing what this is used for? Done. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... File chrome/browser/extensions/activity_log/activity_log_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/activity_log_policy.cc:24: &ActivityLogPolicy::SaveState); On 2013/05/24 18:43:38, felt wrote: > OK, I was confused. I thought you were going to move more of the > ActivityDatabase functionality into the ActivityLogPolicy [sub]classes. Given > that the ActivityDatabase class ended up unchanged, you can get rid of all of > the timer_ and SaveState logic in the policy [sub]classes. (Since the > ActivityDatabase already takes care of it.) > > All that needs to be done: the FullStreamUIPolicy needs to call > db_->SetBatchModeForTesting(false) to explicitly disable it (since we don't want > to batch writes when experimenting). All other policies can just ignore it, they > will magically get batched writes as long as they use the ActivityDatabase > class. I think we still need to implement this in ActivityLogPolicy as well. Besides saving the database, we also need to save the in-memory state to the database, which should also happen periodically. My suggestion is to: (1) leave the timer, as it's necessary for syncing memory and database, and (2) add db_->SetBatchModeForTesting(false) to SetSaveStateOnRequestOnly(). Done. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... File chrome/browser/extensions/activity_log/activity_log_policy.h (right): https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/activity_log_policy.h:39: class ActivityLogPolicy { On 2013/05/24 18:43:38, felt wrote: > Can you please specify that all of these methods can be called on the UI thread, > because all DB operations are dispatched via the ActivityDatabase. Done. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/activity_log_policy.h:96: // of the FullStreamUIPolicy. We should refractor it to use the above On 2013/05/24 18:43:38, felt wrote: > "refractor" -> "refactor" Done. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:96: Callback<void(scoped_ptr<std::vector<scoped_refptr<Action> > >)>& callback) On 2013/05/24 18:43:38, felt wrote: > Is there enough room to indent the callback another 4 so we can see it's > continued from previous line? (I can't tell in the code review dashboard) Done (coppied your formatting). https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:110: key = "fsuip.reason"; On 2013/05/24 18:43:38, felt wrote: > Can you make these k constants? Since they're only used within the class, they > can be in an anonymous namespace in this file. Done. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:130: void FullStreamUIPolicy::ProcessArguments( On 2013/05/24 18:43:38, felt wrote: > Try checking out base/strings classes to see if you can find anything to replace > the stringstream: > https://code.google.com/p/chromium/codesearch#chromium/src/base/strings/&q=pa... I checked all the classes and macros, there doesn't seem to be anything for efficient string concatenation. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:183: dummy /* TODO(dbabic,felt) Drop in the next revision */); On 2013/05/24 18:43:38, felt wrote: > use // for consistency, here and in the other parts of the switch statement > > P.S. Petr might have found a use for extras, so we might not want to drop them. > (Keep the comment though so we remember to discuss it.) Done. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:265: } // End of the extensions namespace On 2013/05/24 18:43:38, felt wrote: > "namespace extensions" Done. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:17: for (int i = 0; i < APIAction::kSizeAlwaysLog; i++) { On 2013/05/24 18:43:38, felt wrote: > ++i > (I often do it the other way around out of habit-- so if you copied my code > here, sorry for inheriting my bad habit) Why? Preincrement is used for iterators (in most cases), because it avoids calling a destructor and a copy constructor, but for basic data types, it doesn't matter whether it's post- or pre-increment. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:42: // unless testing is enabled. On 2013/05/24 18:43:38, felt wrote: > Change comment to match what's happening here Done. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:52: } // End of the extensions namespace On 2013/05/24 18:43:38, felt wrote: > "namespace extensions" Done.
I think you missed the comments in fullstream_ui_policy.h, please make those changes as well as address the ones below. https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/activity_log.cc (right): https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_log.cc:132: switch (policy_type_) { Ah, didn't see it. Cool. On 2013/05/28 21:11:49, dbabic wrote: > On 2013/05/24 18:43:38, felt wrote: > > Where is policy_type_ set? > > There's a static constructor at the end of this file. Since ActivityLog is a > singleton, that should be fine. https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_log.cc:171: if (!extension) Yup, if the extension is uninstalled, the extension object might disappear, and that can happen at any time. Another CL I have out will fix this (the only reason we have the extension object instead of ID is because Eric Dingle relies on the extension object for the UI that I'm about to deprecate). On 2013/05/28 21:11:49, dbabic wrote: > On 2013/05/24 18:43:38, felt wrote: > > Note that these checks help with but still don't solve the TOCTOU problem. Can > > you add a TODO here so I can add a real fix in another version? (In the > future: > > I'll get rid of the dependency on having the extension object, and just take > the > > extension ID as a parameter.) > > Sorry, I didn't understand why this doesn't fix the problem. Are you saying > that the extension object can get deallocated in between the check (line 171) > and use (say, line 187)? Taking extension ID would be a great fix. > > I'll add a comment anyways. Done. https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/activity_log_policy.cc (right): https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_log_policy.cc:24: &ActivityLogPolicy::SaveState); Huh? The ActivityDatabase batching code isn't saving the database. What it does is queue up all write operations to the database, and then does them all at once every 2 minutes. (So it's "saving in-memory state to the database.") On 2013/05/28 21:11:49, dbabic wrote: > On 2013/05/24 18:43:38, felt wrote: > > OK, I was confused. I thought you were going to move more of the > > ActivityDatabase functionality into the ActivityLogPolicy [sub]classes. Given > > that the ActivityDatabase class ended up unchanged, you can get rid of all of > > the timer_ and SaveState logic in the policy [sub]classes. (Since the > > ActivityDatabase already takes care of it.) > > > > All that needs to be done: the FullStreamUIPolicy needs to call > > db_->SetBatchModeForTesting(false) to explicitly disable it (since we don't > want > > to batch writes when experimenting). All other policies can just ignore it, > they > > will magically get batched writes as long as they use the ActivityDatabase > > class. > > I think we still need to implement this in ActivityLogPolicy as well. Besides > saving the database, we also need to save the in-memory state to the database, > which should also happen periodically. My suggestion is to: (1) leave the > timer, as it's necessary for syncing memory and database, and (2) add > db_->SetBatchModeForTesting(false) to SetSaveStateOnRequestOnly(). > > Done. https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc (right): https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:17: for (int i = 0; i < APIAction::kSizeAlwaysLog; i++) { Because that's what the style guide says to do. Please make this change. On 2013/05/28 21:11:49, dbabic wrote: > On 2013/05/24 18:43:38, felt wrote: > > ++i > > (I often do it the other way around out of habit-- so if you copied my code > > here, sorry for inheriting my bad habit) > > Why? Preincrement is used for iterators (in most cases), because it avoids > calling a destructor and a copy constructor, but for basic data types, it > doesn't matter whether it's post- or pre-increment.
https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... File chrome/browser/extensions/activity_log/activity_log_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/activity_log_policy.cc:24: &ActivityLogPolicy::SaveState); On 2013/05/29 03:41:15, felt wrote: > Huh? The ActivityDatabase batching code isn't saving the database. What it does > is queue up all write operations to the database, and then does them all at once > every 2 minutes. (So it's "saving in-memory state to the database.") > > On 2013/05/28 21:11:49, dbabic wrote: > > On 2013/05/24 18:43:38, felt wrote: > > > OK, I was confused. I thought you were going to move more of the > > > ActivityDatabase functionality into the ActivityLogPolicy [sub]classes. > Given > > > that the ActivityDatabase class ended up unchanged, you can get rid of all > of > > > the timer_ and SaveState logic in the policy [sub]classes. (Since the > > > ActivityDatabase already takes care of it.) > > > > > > All that needs to be done: the FullStreamUIPolicy needs to call > > > db_->SetBatchModeForTesting(false) to explicitly disable it (since we don't > > want > > > to batch writes when experimenting). All other policies can just ignore it, > > they > > > will magically get batched writes as long as they use the ActivityDatabase > > > class. > > > > I think we still need to implement this in ActivityLogPolicy as well. Besides > > saving the database, we also need to save the in-memory state to the database, > > which should also happen periodically. My suggestion is to: (1) leave the > > timer, as it's necessary for syncing memory and database, and (2) add > > db_->SetBatchModeForTesting(false) to SetSaveStateOnRequestOnly(). > > > > Done. > Note that the current policies don't implement SaveState(), but batching is completely delegated to the database, because everything is logged (i.e., sent to the database). Future policies might/will be more selective and build more complex models in memory, without necessarily immediately writing the database immediately. Thus, there will be a need to periodically save in-memory state (or sync it with the database). That's why I feel it makes sense to leave this. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... File chrome/browser/extensions/activity_log/fullstream_ui_policy.h (right): https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:19: public: On 2013/05/24 18:43:38, felt wrote: > "public:" should be indented 1 space, and all the method signatures should be > indented 2 spaces. (This goes for all the other files too.) Done. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:20: On 2013/05/24 18:43:38, felt wrote: > no vspace between "public" and comment Done. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:66: // We initialize this on the same thread as the ActivityLog, but then On 2013/05/24 18:43:38, felt wrote: > "ActivityLog and policy, but then" Done. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:17: for (int i = 0; i < APIAction::kSizeAlwaysLog; i++) { On 2013/05/29 03:41:15, felt wrote: > Because that's what the style guide says to do. Please make this change. > > On 2013/05/28 21:11:49, dbabic wrote: > > On 2013/05/24 18:43:38, felt wrote: > > > ++i > > > (I often do it the other way around out of habit-- so if you copied my code > > > here, sorry for inheriting my bad habit) > > > > Why? Preincrement is used for iterators (in most cases), because it avoids > > calling a destructor and a copy constructor, but for basic data types, it > > doesn't matter whether it's post- or pre-increment. > Since this is a simple data type (not an iterator), pre/post-increment is equally efficient, and the style guide explicitly allows both: "For simple scalar (non-object) values there is no reason to prefer one form and we allow either.".
One last change, other than the unit tests. Please e-mail me when the unit tests are uploaded so I can check them out. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... File chrome/browser/extensions/activity_log/activity_log_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/activity_log_policy.cc:24: &ActivityLogPolicy::SaveState); OK, so there will be two simultaneous timers going in some cases? I suppose that's fine. You should think about the threading issues relating to having a timer_. The timer is not threadsafe, so you need to ensure that it both starts and ends on the same thread. You will now need to make sure that is the case for the ActivityLogPolicy class. I *think* that the ActivityLog constructor and destructor will now be called on the same thread (and henceforth also the ActivityLogPolicy constructor and destructor), but perhaps you could add some DCHECKs to the ActivityLogPolicy constructor and destructors to make sure this is true. On 2013/05/29 18:41:40, dbabic wrote: > On 2013/05/29 03:41:15, felt wrote: > > Huh? The ActivityDatabase batching code isn't saving the database. What it > does > > is queue up all write operations to the database, and then does them all at > once > > every 2 minutes. (So it's "saving in-memory state to the database.") > > > > On 2013/05/28 21:11:49, dbabic wrote: > > > On 2013/05/24 18:43:38, felt wrote: > > > > OK, I was confused. I thought you were going to move more of the > > > > ActivityDatabase functionality into the ActivityLogPolicy [sub]classes. > > Given > > > > that the ActivityDatabase class ended up unchanged, you can get rid of all > > of > > > > the timer_ and SaveState logic in the policy [sub]classes. (Since the > > > > ActivityDatabase already takes care of it.) > > > > > > > > All that needs to be done: the FullStreamUIPolicy needs to call > > > > db_->SetBatchModeForTesting(false) to explicitly disable it (since we > don't > > > want > > > > to batch writes when experimenting). All other policies can just ignore > it, > > > they > > > > will magically get batched writes as long as they use the ActivityDatabase > > > > class. > > > > > > I think we still need to implement this in ActivityLogPolicy as well. > Besides > > > saving the database, we also need to save the in-memory state to the > database, > > > which should also happen periodically. My suggestion is to: (1) leave the > > > timer, as it's necessary for syncing memory and database, and (2) add > > > db_->SetBatchModeForTesting(false) to SetSaveStateOnRequestOnly(). > > > > > > Done. > > > > Note that the current policies don't implement SaveState(), but batching is > completely delegated to the database, because everything is logged (i.e., sent > to the database). > > Future policies might/will be more selective and build more complex models in > memory, without necessarily immediately writing the database immediately. Thus, > there will be a need to periodically save in-memory state (or sync it with the > database). That's why I feel it makes sense to leave this. https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:17: for (int i = 0; i < APIAction::kSizeAlwaysLog; i++) { Whoops, you're right, misremembered. :) On 2013/05/29 18:41:40, dbabic wrote: > On 2013/05/29 03:41:15, felt wrote: > > Because that's what the style guide says to do. Please make this change. > > > > On 2013/05/28 21:11:49, dbabic wrote: > > > On 2013/05/24 18:43:38, felt wrote: > > > > ++i > > > > (I often do it the other way around out of habit-- so if you copied my > code > > > > here, sorry for inheriting my bad habit) > > > > > > Why? Preincrement is used for iterators (in most cases), because it avoids > > > calling a destructor and a copy constructor, but for basic data types, it > > > doesn't matter whether it's post- or pre-increment. > > > > Since this is a simple data type (not an iterator), pre/post-increment is > equally efficient, and the style guide explicitly allows both: "For simple > scalar (non-object) values there is no reason to prefer one form and we allow > either.".
https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... File chrome/browser/extensions/activity_log/activity_log_policy.cc (right): https://codereview.chromium.org/15573003/diff/30001/chrome/browser/extensions... chrome/browser/extensions/activity_log/activity_log_policy.cc:24: &ActivityLogPolicy::SaveState); Another thing that came to mind: when shutting down the browser, state will likely need to be saved since the last time the timer ran. The destructor should stop the timer and then save state. On 2013/05/29 19:12:29, felt wrote: > OK, so there will be two simultaneous timers going in some cases? I suppose > that's fine. > > You should think about the threading issues relating to having a timer_. The > timer is not threadsafe, so you need to ensure that it both starts and ends on > the same thread. You will now need to make sure that is the case for the > ActivityLogPolicy class. I *think* that the ActivityLog constructor and > destructor will now be called on the same thread (and henceforth also the > ActivityLogPolicy constructor and destructor), but perhaps you could add some > DCHECKs to the ActivityLogPolicy constructor and destructors to make sure this > is true. > > On 2013/05/29 18:41:40, dbabic wrote: > > On 2013/05/29 03:41:15, felt wrote: > > > Huh? The ActivityDatabase batching code isn't saving the database. What it > > does > > > is queue up all write operations to the database, and then does them all at > > once > > > every 2 minutes. (So it's "saving in-memory state to the database.") > > > > > > On 2013/05/28 21:11:49, dbabic wrote: > > > > On 2013/05/24 18:43:38, felt wrote: > > > > > OK, I was confused. I thought you were going to move more of the > > > > > ActivityDatabase functionality into the ActivityLogPolicy [sub]classes. > > > Given > > > > > that the ActivityDatabase class ended up unchanged, you can get rid of > all > > > of > > > > > the timer_ and SaveState logic in the policy [sub]classes. (Since the > > > > > ActivityDatabase already takes care of it.) > > > > > > > > > > All that needs to be done: the FullStreamUIPolicy needs to call > > > > > db_->SetBatchModeForTesting(false) to explicitly disable it (since we > > don't > > > > want > > > > > to batch writes when experimenting). All other policies can just ignore > > it, > > > > they > > > > > will magically get batched writes as long as they use the > ActivityDatabase > > > > > class. > > > > > > > > I think we still need to implement this in ActivityLogPolicy as well. > > Besides > > > > saving the database, we also need to save the in-memory state to the > > database, > > > > which should also happen periodically. My suggestion is to: (1) leave the > > > > timer, as it's necessary for syncing memory and database, and (2) add > > > > db_->SetBatchModeForTesting(false) to SetSaveStateOnRequestOnly(). > > > > > > > > Done. > > > > > > > Note that the current policies don't implement SaveState(), but batching is > > completely delegated to the database, because everything is logged (i.e., sent > > to the database). > > > > Future policies might/will be more selective and build more complex models in > > memory, without necessarily immediately writing the database immediately. > Thus, > > there will be a need to periodically save in-memory state (or sync it with the > > database). That's why I feel it makes sense to leave this. >
Please let me know whether we have converged on this. Thanks. Will start working on tests... Thanks, https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... File chrome/browser/extensions/activity_log/activity_log_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/activity_log_policy.cc:24: &ActivityLogPolicy::SaveState); On 2013/05/29 19:22:35, felt wrote: > Another thing that came to mind: when shutting down the browser, state will > likely need to be saved since the last time the timer ran. The destructor should > stop the timer and then save state. > > On 2013/05/29 19:12:29, felt wrote: > > OK, so there will be two simultaneous timers going in some cases? I suppose > > that's fine. > > > > You should think about the threading issues relating to having a timer_. The > > timer is not threadsafe, so you need to ensure that it both starts and ends on > > the same thread. You will now need to make sure that is the case for the > > ActivityLogPolicy class. I *think* that the ActivityLog constructor and > > destructor will now be called on the same thread (and henceforth also the > > ActivityLogPolicy constructor and destructor), but perhaps you could add some > > DCHECKs to the ActivityLogPolicy constructor and destructors to make sure this > > is true. > > > > On 2013/05/29 18:41:40, dbabic wrote: > > > On 2013/05/29 03:41:15, felt wrote: > > > > Huh? The ActivityDatabase batching code isn't saving the database. What it > > > does > > > > is queue up all write operations to the database, and then does them all > at > > > once > > > > every 2 minutes. (So it's "saving in-memory state to the database.") > > > > > > > > On 2013/05/28 21:11:49, dbabic wrote: > > > > > On 2013/05/24 18:43:38, felt wrote: > > > > > > OK, I was confused. I thought you were going to move more of the > > > > > > ActivityDatabase functionality into the ActivityLogPolicy > [sub]classes. > > > > Given > > > > > > that the ActivityDatabase class ended up unchanged, you can get rid of > > all > > > > of > > > > > > the timer_ and SaveState logic in the policy [sub]classes. (Since the > > > > > > ActivityDatabase already takes care of it.) > > > > > > > > > > > > All that needs to be done: the FullStreamUIPolicy needs to call > > > > > > db_->SetBatchModeForTesting(false) to explicitly disable it (since we > > > don't > > > > > want > > > > > > to batch writes when experimenting). All other policies can just > ignore > > > it, > > > > > they > > > > > > will magically get batched writes as long as they use the > > ActivityDatabase > > > > > > class. > > > > > > > > > > I think we still need to implement this in ActivityLogPolicy as well. > > > Besides > > > > > saving the database, we also need to save the in-memory state to the > > > database, > > > > > which should also happen periodically. My suggestion is to: (1) leave > the > > > > > timer, as it's necessary for syncing memory and database, and (2) add > > > > > db_->SetBatchModeForTesting(false) to SetSaveStateOnRequestOnly(). > > > > > > > > > > Done. > > > > > > > > > > Note that the current policies don't implement SaveState(), but batching is > > > completely delegated to the database, because everything is logged (i.e., > sent > > > to the database). > > > > > > Future policies might/will be more selective and build more complex models > in > > > memory, without necessarily immediately writing the database immediately. > > Thus, > > > there will be a need to periodically save in-memory state (or sync it with > the > > > database). That's why I feel it makes sense to leave this. > > > You are bringing up an interesting issue here. Technically, we shouldn't call any virtual member functions from constructors/destructors. I will remove the SaveState() call from the destructor bellow, as it's effectively a NOP, and replace that with stopping the timer. I can still call SaveState() in one of the subclasses, but since the existing policies don't implement SaveState(), that wouldn't be useful either. I see no reason why constructors and destructors would be called by different threads. I expected that SaveState() would need to be thread safe, see the comment in activity_log_policy.h. If you think any additional DCHECKs are needed, please let me know.
https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... File chrome/browser/extensions/activity_log/activity_log_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/30001/chrome/browser/ext... chrome/browser/extensions/activity_log/activity_log_policy.cc:24: &ActivityLogPolicy::SaveState); Sounds good. In the past, there was an incident where the ActivityLog was being destructed on a different thread because of pending IPC messages that kept it alive longer than the thread it was initialized on. I changed how the ActivityDatabase worked and it was no longer a problem. But, there is a chance that this can happen again. Up to you whether to add DCHECKs but be aware of it as a potential issue if there are any failures in the future. On 2013/05/29 20:12:26, dbabic wrote: > On 2013/05/29 19:22:35, felt wrote: > > Another thing that came to mind: when shutting down the browser, state will > > likely need to be saved since the last time the timer ran. The destructor > should > > stop the timer and then save state. > > > > On 2013/05/29 19:12:29, felt wrote: > > > OK, so there will be two simultaneous timers going in some cases? I suppose > > > that's fine. > > > > > > You should think about the threading issues relating to having a timer_. The > > > timer is not threadsafe, so you need to ensure that it both starts and ends > on > > > the same thread. You will now need to make sure that is the case for the > > > ActivityLogPolicy class. I *think* that the ActivityLog constructor and > > > destructor will now be called on the same thread (and henceforth also the > > > ActivityLogPolicy constructor and destructor), but perhaps you could add > some > > > DCHECKs to the ActivityLogPolicy constructor and destructors to make sure > this > > > is true. > > > > > > On 2013/05/29 18:41:40, dbabic wrote: > > > > On 2013/05/29 03:41:15, felt wrote: > > > > > Huh? The ActivityDatabase batching code isn't saving the database. What > it > > > > does > > > > > is queue up all write operations to the database, and then does them all > > at > > > > once > > > > > every 2 minutes. (So it's "saving in-memory state to the database.") > > > > > > > > > > On 2013/05/28 21:11:49, dbabic wrote: > > > > > > On 2013/05/24 18:43:38, felt wrote: > > > > > > > OK, I was confused. I thought you were going to move more of the > > > > > > > ActivityDatabase functionality into the ActivityLogPolicy > > [sub]classes. > > > > > Given > > > > > > > that the ActivityDatabase class ended up unchanged, you can get rid > of > > > all > > > > > of > > > > > > > the timer_ and SaveState logic in the policy [sub]classes. (Since > the > > > > > > > ActivityDatabase already takes care of it.) > > > > > > > > > > > > > > All that needs to be done: the FullStreamUIPolicy needs to call > > > > > > > db_->SetBatchModeForTesting(false) to explicitly disable it (since > we > > > > don't > > > > > > want > > > > > > > to batch writes when experimenting). All other policies can just > > ignore > > > > it, > > > > > > they > > > > > > > will magically get batched writes as long as they use the > > > ActivityDatabase > > > > > > > class. > > > > > > > > > > > > I think we still need to implement this in ActivityLogPolicy as well. > > > > Besides > > > > > > saving the database, we also need to save the in-memory state to the > > > > database, > > > > > > which should also happen periodically. My suggestion is to: (1) leave > > the > > > > > > timer, as it's necessary for syncing memory and database, and (2) add > > > > > > db_->SetBatchModeForTesting(false) to SetSaveStateOnRequestOnly(). > > > > > > > > > > > > Done. > > > > > > > > > > > > > Note that the current policies don't implement SaveState(), but batching > is > > > > completely delegated to the database, because everything is logged (i.e., > > sent > > > > to the database). > > > > > > > > Future policies might/will be more selective and build more complex models > > in > > > > memory, without necessarily immediately writing the database immediately. > > > Thus, > > > > there will be a need to periodically save in-memory state (or sync it with > > the > > > > database). That's why I feel it makes sense to leave this. > > > > > > > You are bringing up an interesting issue here. Technically, we shouldn't call > any virtual member functions from constructors/destructors. I will remove the > SaveState() call from the destructor bellow, as it's effectively a NOP, and > replace that with stopping the timer. I can still call SaveState() in one of > the subclasses, but since the existing policies don't implement SaveState(), > that wouldn't be useful either. > > I see no reason why constructors and destructors would be called by different > threads. I expected that SaveState() would need to be thread safe, see the > comment in activity_log_policy.h. If you think any additional DCHECKs are > needed, please let me know.
Hi, I've written the tests. It took me a bit more than I expected (sorry!), because I was wrestling with "patch apply" failures, with sync, and with threads usage. Cheers,
OK, done with my review for now. Please have mpcomplete@chromium review, and ask him for suggestions about what to use to replace the stringstream usage. FYI, this will need to be rebased because I've begun committing the API-related patches today. Please ping me when Matt gives the LGTM and I can help resolve the conflicts. https://chromiumcodereview.appspot.com/15573003/diff/83001/chrome/browser/ext... File chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/83001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc:86: BrowserThread::UI); nit: indent to match profile_, or put profile_ on the next line with BrowserThread::UI; same in other places
On 2013/05/30 03:36:34, felt wrote: > OK, done with my review for now. Please have mpcomplete@chromium review, and ask > him for suggestions about what to use to replace the stringstream usage. > > FYI, this will need to be rebased because I've begun committing the API-related > patches today. Please ping me when Matt gives the LGTM and I can help resolve > the conflicts. > > https://chromiumcodereview.appspot.com/15573003/diff/83001/chrome/browser/ext... > File chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc > (right): > > https://chromiumcodereview.appspot.com/15573003/diff/83001/chrome/browser/ext... > chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc:86: > BrowserThread::UI); > nit: indent to match profile_, or put profile_ on the next line with > BrowserThread::UI; same in other places
One quick note: the BUG= field needs to be a chrome bug, not a buganizer bug. I know this is confusing since our team primarily uses buganizer. Can you please change the bug number to be 238256? That points to an external chrome bug.
Matt, Would you please be willing to review this diff? I would also like to hear your thoughts about the usage of std::stringstream: We need to concatenate some strings, but using the '+' operator in a loop is very inefficient. It's quadratic in the size of the string (as one needs to walk over partial results over and over again), calls system calls to (de)allocate memory, and a bunch of con/de-structors. The standard way of doing string concatenation efficiently is using stringstreams. However, Adrienne is worried whether stringstreams are allowed. We are not using stringstreams for any IO, so I'm hoping we are fine, but if not, could you please suggest how to do efficient string concatenation not using stringstreams? Thanks! -- Domagoj https://chromiumcodereview.appspot.com/15573003/diff/83001/chrome/browser/ext... File chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/83001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc:86: BrowserThread::UI); On 2013/05/30 03:36:34, felt wrote: > nit: indent to match profile_, or put profile_ on the next line with > BrowserThread::UI; same in other places Done.
Matt, Please ignore the "patch apply" failures in the Patch Set 15, which differs only in indentation from Patch Set 14, where the majority of tests are passing and the failures don't seem to be related to the diff. Thanks. Best, -- Domagoj
https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:164: processed_args << arg; I doubt the efficiency of stringstream vs string matters much here. - It's not called that frequently. - Serializing a JSON value is likely more intensive than concatenating a string. - std::string append is amortized linear time, not quadratic. Most implementations will double in size each time they reach capacity. https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... File chrome/browser/extensions/activity_log/fullstream_ui_policy.h (right): https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:19: class FullStreamUIPolicy : public ActivityLogPolicy { Add a class comment https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:15: arg_whitelist_api_.insert(std::string(APIAction::kAlwaysLog[i])); the explicit call to std::string should be unnecessary https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:26: nit: rm newline https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy.h (right): https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.h:15: class StreamWithoutArgsUIPolicy : public FullStreamUIPolicy { Add a class comment. https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy_unittest.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/stream_noargs_ui_policy_unittest.cc:1: // Copyright $YEAR The Chromium Authors. All rights reserved. ?
Matt, Thank you for quick response. I've addressed all your comments, except for the one related to stringstreams. Please check out my comment there and let me know how would you like to proceed on that. Thanks! Best, -- Domagoj https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:164: processed_args << arg; On 2013/05/30 18:55:32, Matt Perry wrote: > I doubt the efficiency of stringstream vs string matters much here. > - It's not called that frequently. > - Serializing a JSON value is likely more intensive than concatenating a string. > - std::string append is amortized linear time, not quadratic. Most > implementations will double in size each time they reach capacity. I checked the GCC 4.9 source, and string::append doesn't seem to be amortized linear time there. Namely, the call to _M_clone (called from reserve, called from append) allocates each time exactly size_of_old_string + size_of_new_string memory. Feel free to check it yourself (./libstdc++-v3/include/bits/basic_string.tcc). Thus, unless I'm missing something, running append in a loop will be quadratic. Now, whether that matters is a completely different question. At this point, I don't know how many arguments we might have (Adrienne might know better). Would you still prefer to go with string::append? I guess it's an efficiency-vs-conformance question. https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... File chrome/browser/extensions/activity_log/fullstream_ui_policy.h (right): https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.h:19: class FullStreamUIPolicy : public ActivityLogPolicy { On 2013/05/30 18:55:32, Matt Perry wrote: > Add a class comment Done. https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:15: arg_whitelist_api_.insert(std::string(APIAction::kAlwaysLog[i])); On 2013/05/30 18:55:32, Matt Perry wrote: > the explicit call to std::string should be unnecessary Done. https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:26: On 2013/05/30 18:55:32, Matt Perry wrote: > nit: rm newline Done. https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy.h (right): https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.h:15: class StreamWithoutArgsUIPolicy : public FullStreamUIPolicy { On 2013/05/30 18:55:32, Matt Perry wrote: > Add a class comment. Done. https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy_unittest.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/stream_noargs_ui_policy_unittest.cc:1: // Copyright $YEAR The Chromium Authors. All rights reserved. On 2013/05/30 18:55:32, Matt Perry wrote: > ? Sorry, I took http://www.chromium.org/developers/coding-style too literally and expected that git will replace $YEAR. Done.
https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:164: processed_args << arg; On 2013/05/30 21:51:25, dbabic wrote: > On 2013/05/30 18:55:32, Matt Perry wrote: > > I doubt the efficiency of stringstream vs string matters much here. > > - It's not called that frequently. > > - Serializing a JSON value is likely more intensive than concatenating a > string. > > - std::string append is amortized linear time, not quadratic. Most > > implementations will double in size each time they reach capacity. > > I checked the GCC 4.9 source, and string::append doesn't seem to be amortized > linear time there. Namely, the call to _M_clone (called from reserve, called > from append) allocates each time exactly size_of_old_string + size_of_new_string > memory. Feel free to check it yourself > (./libstdc++-v3/include/bits/basic_string.tcc). Thus, unless I'm missing > something, running append in a loop will be quadratic. That's surprising - the SGI docs [ http://www.sgi.com/tech/stl/basic_string.html ] claim that string concat is O(N), and they're generally trustworthy. > Now, whether that matters is a completely different question. At this point, I > don't know how many arguments we might have (Adrienne might know better). Would > you still prefer to go with string::append? I guess it's an > efficiency-vs-conformance question. I still think it doesn't matter in this case. It's not something that will happen 100s of times per second, so performance is unlikely to matter. https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:165: } On second thought - can I ask why you don't just run SerializeAndOmitBinaryValues over args directly? Why serialize each argument independently?
https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:164: processed_args << arg; On 2013/05/30 22:00:27, Matt Perry wrote: > On 2013/05/30 21:51:25, dbabic wrote: > > On 2013/05/30 18:55:32, Matt Perry wrote: > > > I doubt the efficiency of stringstream vs string matters much here. > > > - It's not called that frequently. > > > - Serializing a JSON value is likely more intensive than concatenating a > > string. > > > - std::string append is amortized linear time, not quadratic. Most > > > implementations will double in size each time they reach capacity. > > > > I checked the GCC 4.9 source, and string::append doesn't seem to be amortized > > linear time there. Namely, the call to _M_clone (called from reserve, called > > from append) allocates each time exactly size_of_old_string + > size_of_new_string > > memory. Feel free to check it yourself > > (./libstdc++-v3/include/bits/basic_string.tcc). Thus, unless I'm missing > > something, running append in a loop will be quadratic. > > That's surprising - the SGI docs [ http://www.sgi.com/tech/stl/basic_string.html > ] claim that string concat is O(N), and they're generally trustworthy. > > > Now, whether that matters is a completely different question. At this point, > I > > don't know how many arguments we might have (Adrienne might know better). > Would > > you still prefer to go with string::append? I guess it's an > > efficiency-vs-conformance question. > > I still think it doesn't matter in this case. It's not something that will > happen 100s of times per second, so performance is unlikely to matter. I believe that SGI docs are correct, it's O(N) for a single append, but we are doing appends in a loop. All args will be logged only in Navitron. In deployment, all args will be logged only for DOM and whitelisted (privacy-irrelevant) actions. I don't know how many of those will get executed per second. With that info, do you still prefer string::apply? https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:165: } On 2013/05/30 22:00:27, Matt Perry wrote: > On second thought - can I ask why you don't just run > SerializeAndOmitBinaryValues over args directly? Why serialize each argument > independently? I don't know the answer to that. I copied the previously existing code and made it a bit more efficient by changing the implementation to stringstream. If you don't mind, I'll add a TODO for that --- since my code is out of sync with the master (from this morning), I can't run tests and would prefer to avoid all non-trivial code changes until I can run "git cl try" again.
https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:164: processed_args << arg; On 2013/05/30 22:15:45, dbabic wrote: > On 2013/05/30 22:00:27, Matt Perry wrote: > > On 2013/05/30 21:51:25, dbabic wrote: > > > On 2013/05/30 18:55:32, Matt Perry wrote: > > > > I doubt the efficiency of stringstream vs string matters much here. > > > > - It's not called that frequently. > > > > - Serializing a JSON value is likely more intensive than concatenating a > > > string. > > > > - std::string append is amortized linear time, not quadratic. Most > > > > implementations will double in size each time they reach capacity. > > > > > > I checked the GCC 4.9 source, and string::append doesn't seem to be > amortized > > > linear time there. Namely, the call to _M_clone (called from reserve, > called > > > from append) allocates each time exactly size_of_old_string + > > size_of_new_string > > > memory. Feel free to check it yourself > > > (./libstdc++-v3/include/bits/basic_string.tcc). Thus, unless I'm missing > > > something, running append in a loop will be quadratic. > > > > That's surprising - the SGI docs [ > http://www.sgi.com/tech/stl/basic_string.html > > ] claim that string concat is O(N), and they're generally trustworthy. > > > > > Now, whether that matters is a completely different question. At this > point, > > I > > > don't know how many arguments we might have (Adrienne might know better). > > Would > > > you still prefer to go with string::append? I guess it's an > > > efficiency-vs-conformance question. > > > > I still think it doesn't matter in this case. It's not something that will > > happen 100s of times per second, so performance is unlikely to matter. > > I believe that SGI docs are correct, it's O(N) for a single append, but we are > doing appends in a loop. Then it depends on what N refers to. If N is the length of the appended data, then the full loop is still O(length of final string). > All args will be logged only in Navitron. In deployment, all args will be > logged only for DOM and whitelisted (privacy-irrelevant) actions. I don't know > how many of those will get executed per second. > > With that info, do you still prefer string::apply? += or append, yes. https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:165: } On 2013/05/30 22:15:45, dbabic wrote: > On 2013/05/30 22:00:27, Matt Perry wrote: > > On second thought - can I ask why you don't just run > > SerializeAndOmitBinaryValues over args directly? Why serialize each argument > > independently? > > I don't know the answer to that. I copied the previously existing code and made > it a bit more efficient by changing the implementation to stringstream. If you > don't mind, I'll add a TODO for that --- since my code is out of sync with the > master (from this morning), I can't run tests and would prefer to avoid all > non-trivial code changes until I can run "git cl try" again. Sounds good. It looks like the JSONSerializer will add additional [] to surround the arguments. Not sure if that's a dealbreaker for you.
Addressed both (sstream -> append, TODO). Would you be willing to approve now? Thanks. -- Domagoj https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:164: processed_args << arg; On 2013/05/30 22:46:21, Matt Perry wrote: > On 2013/05/30 22:15:45, dbabic wrote: > > On 2013/05/30 22:00:27, Matt Perry wrote: > > > On 2013/05/30 21:51:25, dbabic wrote: > > > > On 2013/05/30 18:55:32, Matt Perry wrote: > > > > > I doubt the efficiency of stringstream vs string matters much here. > > > > > - It's not called that frequently. > > > > > - Serializing a JSON value is likely more intensive than concatenating a > > > > string. > > > > > - std::string append is amortized linear time, not quadratic. Most > > > > > implementations will double in size each time they reach capacity. > > > > > > > > I checked the GCC 4.9 source, and string::append doesn't seem to be > > amortized > > > > linear time there. Namely, the call to _M_clone (called from reserve, > > called > > > > from append) allocates each time exactly size_of_old_string + > > > size_of_new_string > > > > memory. Feel free to check it yourself > > > > (./libstdc++-v3/include/bits/basic_string.tcc). Thus, unless I'm missing > > > > something, running append in a loop will be quadratic. > > > > > > That's surprising - the SGI docs [ > > http://www.sgi.com/tech/stl/basic_string.html > > > ] claim that string concat is O(N), and they're generally trustworthy. > > > > > > > Now, whether that matters is a completely different question. At this > > point, > > > I > > > > don't know how many arguments we might have (Adrienne might know better). > > > Would > > > > you still prefer to go with string::append? I guess it's an > > > > efficiency-vs-conformance question. > > > > > > I still think it doesn't matter in this case. It's not something that will > > > happen 100s of times per second, so performance is unlikely to matter. > > > > I believe that SGI docs are correct, it's O(N) for a single append, but we are > > doing appends in a loop. > > Then it depends on what N refers to. If N is the length of the appended data, > then the full loop is still O(length of final string). > > > All args will be logged only in Navitron. In deployment, all args will be > > logged only for DOM and whitelisted (privacy-irrelevant) actions. I don't > know > > how many of those will get executed per second. > > > > With that info, do you still prefer string::apply? > > += or append, yes. Done. https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:165: } On 2013/05/30 22:46:21, Matt Perry wrote: > On 2013/05/30 22:15:45, dbabic wrote: > > On 2013/05/30 22:00:27, Matt Perry wrote: > > > On second thought - can I ask why you don't just run > > > SerializeAndOmitBinaryValues over args directly? Why serialize each argument > > > independently? > > > > I don't know the answer to that. I copied the previously existing code and > made > > it a bit more efficient by changing the implementation to stringstream. If > you > > don't mind, I'll add a TODO for that --- since my code is out of sync with the > > master (from this morning), I can't run tests and would prefer to avoid all > > non-trivial code changes until I can run "git cl try" again. > > Sounds good. It looks like the JSONSerializer will add additional [] to surround > the arguments. Not sure if that's a dealbreaker for you. Done.
https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://chromiumcodereview.appspot.com/15573003/diff/94001/chrome/browser/ext... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:164: processed_args << arg; On 2013/05/30 22:54:41, dbabic wrote: > > Then it depends on what N refers to. If N is the length of the appended data, > > then the full loop is still O(length of final string). Just noticed this... At least the way it's implemented in GCC, N has to refer to the size of the length of the concatenated string, as the prefix is reallocated and copied on each append. Thus, complexity, at least in GCC, has to be quadratic. Anyways, I've changed it to string::append.
Hi, As we discussed, I waited until Adrienne's recent patches landed, and then merged the policy stuff with the master. Would you please be willing to review? As far as I can tell, the test failures are not related to the changes. Thanks, -- Domagoj
https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/activity_log.cc (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.cc:250: // */ merge junk https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/activity_log_policy.h (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log_policy.h:86: const base::DictionaryValue* details) = 0; // details - return the string instead of passing it as an out-param. - pass GURL by const ref. if there is no GURL, just pass an empty GURL object. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log_policy.h:115: virtual void GetKey(KeyType key_id, std::string* key_string) const; - return the string instead of passing it as an out-param. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:30: return; else branch is unnecessary https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy_unittest.cc (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/stream_noargs_ui_policy_unittest.cc:99: delete policy; use a scoped_ptr, or even just allocate it on the stack.
Hi Domagoj, here's my first batch of comments. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/activity_log.cc (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.cc:172: // that require them. This code was being used to control whether we dispatched anything to the ActivityDatabase. So either add checks in each of the methods in this file and don't invoke the policy if has_threads_ is false, *or* you need to copy this into the policies and have each of the policies not write if has_threads_ is false. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.cc:288: NULL); Note that we've started using the extra fields so they shouldn't just be thrown away (for example, Petr's recent patch.) Goes for all action types. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.cc:362: // doesn't require construction of the action object. This isn't going to happen: we need to make action objects in order to use the various helper classes for writing to the DB, converting to ExtensionActivities, etc. Please remove TODO. If it's something you feel strongly about, you can file a bug and we can discuss it in a thread. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/activity_log.h (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.h:145: // be needed to rename the argument. Does this need to be distinct from selecting the policy via SetDefaultPolicy? (I thought one policy was with args, another policy was without args?) https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.h:194: // TODO(felt) These two flags could use a comment. Is there a reason why you reordered these? I liked having them near the InstallTracker_, since they work together.
Hi Domagoj, a few more requests. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:55: dispatch_thread_ = BrowserThread::UI; Hey, during my last bug scrub I got rid of this dispatch_thread_ stuff. Now, we don't dispatch at all if either the IO, DB, or FILE threads are missing. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:65: ScheduleAndForget(db_, &ActivityDatabase::Close); If the IO, DB, ir FILE threads are missing you need to delete db_; instead of calling ::Close. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:84: db_->SetBatchModeForTesting(false); Hey, I just noticed that you're doing this from the UI thread. Either dispatch this asynchronously on the DB thread or make sure that the bool inside SetBatchModeForTesting is threadsafe (add a Lock to the class and an AutoLock in the method scope). https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:148: // TODO(dbabic,felt) Drop the dummy string in the next revision What's the dummy string? Is that the extra? Can you drop this TODO and rename extras, since we've started finding use cases for having extras. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc:36: file_thread_(BrowserThread::FILE, base::MessageLoop::current()) {} Hey, there were a bunch of changes made to the activity_log_unittest.cc. Can you please check out the new version of those unittests and also update this one accordingly? For example we now have a thread bundle, reset the command line, etc. Same for the other unittest.
https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/activity_log.cc (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.cc:172: // that require them. On 2013/06/13 18:52:02, felt wrote: > This code was being used to control whether we dispatched anything to the > ActivityDatabase. So either add checks in each of the methods in this file and > don't invoke the policy if has_threads_ is false, *or* you need to copy this > into the policies and have each of the policies not write if has_threads_ is > false. If has_threads_ is false, IsLogEnabled() will return false, and therefore SetDefaultPolicy will not create any policy. Does this address your concern? https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.cc:250: // */ On 2013/06/13 18:23:23, Matt Perry wrote: > merge junk Done. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.cc:288: NULL); On 2013/06/13 18:52:02, felt wrote: > Note that we've started using the extra fields so they shouldn't just be thrown > away (for example, Petr's recent patch.) Goes for all action types. Done. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.cc:362: // doesn't require construction of the action object. On 2013/06/13 18:52:02, felt wrote: > This isn't going to happen: we need to make action objects in order to use the > various helper classes for writing to the DB, converting to ExtensionActivities, > etc. Please remove TODO. If it's something you feel strongly about, you can file > a bug and we can discuss it in a thread. Done. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/activity_log.h (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.h:145: // be needed to rename the argument. On 2013/06/13 18:52:02, felt wrote: > Does this need to be distinct from selecting the policy via SetDefaultPolicy? (I > thought one policy was with args, another policy was without args?) We could have many different policies in the future, so I'm guessing that this member function will become redundant. "log_arguments" might not be applicable to all policies. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.h:194: // TODO(felt) These two flags could use a comment. On 2013/06/13 18:52:02, felt wrote: > Is there a reason why you reordered these? I liked having them near the > InstallTracker_, since they work together. Efficiency. With the old ordering, the compiler will pad bool to a 32- or 64-bit boundary. With the current ordering, all bools will be in a single word. I know this is a singleton object, so it doesn't matter that much here. Still, it's a good practice and style. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/activity_log_policy.h (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log_policy.h:86: const base::DictionaryValue* details) = 0; // details On 2013/06/13 18:23:23, Matt Perry wrote: > - return the string instead of passing it as an out-param. > - pass GURL by const ref. if there is no GURL, just pass an empty GURL object. Done. Notice that ProcessAction doesn't really return anything, to there's nothing to do for the first part of the comment. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log_policy.h:115: virtual void GetKey(KeyType key_id, std::string* key_string) const; On 2013/06/13 18:23:23, Matt Perry wrote: > - return the string instead of passing it as an out-param. Done. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:55: dispatch_thread_ = BrowserThread::UI; On 2013/06/13 20:24:16, felt wrote: > Hey, during my last bug scrub I got rid of this dispatch_thread_ stuff. Now, we > don't dispatch at all if either the IO, DB, or FILE threads are missing. Done. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:65: ScheduleAndForget(db_, &ActivityDatabase::Close); On 2013/06/13 20:24:16, felt wrote: > If the IO, DB, ir FILE threads are missing you need to delete db_; instead of > calling ::Close. Done. If the DB thread is missing, the policy will never get created in the first place. I've added a check. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:84: db_->SetBatchModeForTesting(false); On 2013/06/13 20:24:16, felt wrote: > Hey, I just noticed that you're doing this from the UI thread. Either dispatch > this asynchronously on the DB thread or make sure that the bool inside > SetBatchModeForTesting is threadsafe (add a Lock to the class and an AutoLock in > the method scope). This should all be running on the DB thread. If there's no DB thread, the policy object won't get created. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:148: // TODO(dbabic,felt) Drop the dummy string in the next revision On 2013/06/13 20:24:16, felt wrote: > What's the dummy string? Is that the extra? Can you drop this TODO and rename > extras, since we've started finding use cases for having extras. Done. It's now the extra string. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/fullstream_ui_policy_unittest.cc:36: file_thread_(BrowserThread::FILE, base::MessageLoop::current()) {} On 2013/06/13 20:24:16, felt wrote: > Hey, there were a bunch of changes made to the activity_log_unittest.cc. Can you > please check out the new version of those unittests and also update this one > accordingly? For example we now have a thread bundle, reset the command line, > etc. Same for the other unittest. Done. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/stream_noargs_ui_policy.cc:30: return; On 2013/06/13 18:23:23, Matt Perry wrote: > else branch is unnecessary Done. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy_unittest.cc (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/stream_noargs_ui_policy_unittest.cc:99: delete policy; On 2013/06/13 18:23:23, Matt Perry wrote: > use a scoped_ptr, or even just allocate it on the stack. Done.
mostly LGTM, just a few style nits https://codereview.chromium.org/15573003/diff/132001/chrome/browser/extension... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://codereview.chromium.org/15573003/diff/132001/chrome/browser/extension... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:98: std::string* processed_args) const { Sorry, this is the method I was thinking of when I said to return the processed_args string. https://codereview.chromium.org/15573003/diff/132001/chrome/browser/extension... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:138: details->GetString(key, &extra); nit: could just do details->GetString(GetKey(PARAM_KEY_EXTRA), &extra); https://codereview.chromium.org/15573003/diff/132001/chrome/browser/extension... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy_unittest.cc (right): https://codereview.chromium.org/15573003/diff/132001/chrome/browser/extension... chrome/browser/extensions/activity_log/stream_noargs_ui_policy_unittest.cc:119: scoped_ptr<GURL> gurl(new GURL("http://www.google.com")); make this a stack var, no need to allocate on the heap
https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/activity_log.cc (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.cc:172: // that require them. Hmm... originally I had meant for IsLogEnabled to be separate from the has_threads_ issue, so that we could still try sending events over to the log or printing them to the console. I added a temporary hack that sets it to false in IsLogEnabled with no comment (my bad!) when firefighting earlier this week. But now that I'm thinking about it, maybe it does make sense to leave it in -- since hopefully this would only come up during testing, anyway. On 2013/06/13 23:10:08, dbabic wrote: > On 2013/06/13 18:52:02, felt wrote: > > This code was being used to control whether we dispatched anything to the > > ActivityDatabase. So either add checks in each of the methods in this file and > > don't invoke the policy if has_threads_ is false, *or* you need to copy this > > into the policies and have each of the policies not write if has_threads_ is > > false. > > If has_threads_ is false, IsLogEnabled() will return false, and therefore > SetDefaultPolicy will not create any policy. Does this address your concern? https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/activity_log.h (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.h:145: // be needed to rename the argument. I think we agree: I'm suggesting we get rid of SetArgumentLoggingForTesting and just pick the policy we want in unit tests or when certain flags are set. On 2013/06/13 23:10:08, dbabic wrote: > On 2013/06/13 18:52:02, felt wrote: > > Does this need to be distinct from selecting the policy via SetDefaultPolicy? > (I > > thought one policy was with args, another policy was without args?) > > We could have many different policies in the future, so I'm guessing that this > member function will become redundant. "log_arguments" might not be applicable > to all policies. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.h:194: // TODO(felt) These two flags could use a comment. OK, but now your desired comment isn't going to be able to explain these together in a group when I write it. :) On 2013/06/13 23:10:08, dbabic wrote: > On 2013/06/13 18:52:02, felt wrote: > > Is there a reason why you reordered these? I liked having them near the > > InstallTracker_, since they work together. > > Efficiency. With the old ordering, the compiler will pad bool to a 32- or > 64-bit boundary. With the current ordering, all bools will be in a single word. > I know this is a singleton object, so it doesn't matter that much here. Still, > it's a good practice and style.
https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/activity_log.cc (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.cc:172: // that require them. On 2013/06/13 23:23:37, felt wrote: > Hmm... originally I had meant for IsLogEnabled to be separate from the > has_threads_ issue, so that we could still try sending events over to the log or > printing them to the console. I added a temporary hack that sets it to false in > IsLogEnabled with no comment (my bad!) when firefighting earlier this week. But > now that I'm thinking about it, maybe it does make sense to leave it in -- since > hopefully this would only come up during testing, anyway. > > On 2013/06/13 23:10:08, dbabic wrote: > > On 2013/06/13 18:52:02, felt wrote: > > > This code was being used to control whether we dispatched anything to the > > > ActivityDatabase. So either add checks in each of the methods in this file > and > > > don't invoke the policy if has_threads_ is false, *or* you need to copy this > > > into the policies and have each of the policies not write if has_threads_ is > > > false. > > > > If has_threads_ is false, IsLogEnabled() will return false, and therefore > > SetDefaultPolicy will not create any policy. Does this address your concern? > Ok. https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/activity_log.h (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.h:145: // be needed to rename the argument. On 2013/06/13 23:23:37, felt wrote: > I think we agree: I'm suggesting we get rid of SetArgumentLoggingForTesting and > just pick the policy we want in unit tests or when certain flags are set. > > On 2013/06/13 23:10:08, dbabic wrote: > > On 2013/06/13 18:52:02, felt wrote: > > > Does this need to be distinct from selecting the policy via > SetDefaultPolicy? > > (I > > > thought one policy was with args, another policy was without args?) > > > > We could have many different policies in the future, so I'm guessing that this > > member function will become redundant. "log_arguments" might not be > applicable > > to all policies. > Yes, that would be a good solution, IMO. Would you like to have that in this patch, or leave it for some later one? https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/activity_log.h:194: // TODO(felt) These two flags could use a comment. On 2013/06/13 23:23:37, felt wrote: > OK, but now your desired comment isn't going to be able to explain these > together in a group when I write it. :) > > On 2013/06/13 23:10:08, dbabic wrote: > > On 2013/06/13 18:52:02, felt wrote: > > > Is there a reason why you reordered these? I liked having them near the > > > InstallTracker_, since they work together. > > > > Efficiency. With the old ordering, the compiler will pad bool to a 32- or > > 64-bit boundary. With the current ordering, all bools will be in a single > word. > > I know this is a singleton object, so it doesn't matter that much here. > Still, > > it's a good practice and style. > It shouldn't be too difficult to write two comments, or refer in one comment to another field... https://codereview.chromium.org/15573003/diff/132001/chrome/browser/extension... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://codereview.chromium.org/15573003/diff/132001/chrome/browser/extension... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:98: std::string* processed_args) const { On 2013/06/13 23:16:14, Matt Perry wrote: > Sorry, this is the method I was thinking of when I said to return the > processed_args string. Done. https://codereview.chromium.org/15573003/diff/132001/chrome/browser/extension... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:138: details->GetString(key, &extra); On 2013/06/13 23:16:14, Matt Perry wrote: > nit: could just do details->GetString(GetKey(PARAM_KEY_EXTRA), &extra); Done. https://codereview.chromium.org/15573003/diff/132001/chrome/browser/extension... File chrome/browser/extensions/activity_log/stream_noargs_ui_policy_unittest.cc (right): https://codereview.chromium.org/15573003/diff/132001/chrome/browser/extension... chrome/browser/extensions/activity_log/stream_noargs_ui_policy_unittest.cc:119: scoped_ptr<GURL> gurl(new GURL("http://www.google.com")); On 2013/06/13 23:16:14, Matt Perry wrote: > make this a stack var, no need to allocate on the heap Done.
https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right): https://codereview.chromium.org/15573003/diff/119001/chrome/browser/extension... chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:84: db_->SetBatchModeForTesting(false); I'm confused. Where is SetSaveStateOnRequestOnly dispatched on the DB thread? It looks like the UI thread to me? On 2013/06/13 23:10:08, dbabic wrote: > On 2013/06/13 20:24:16, felt wrote: > > Hey, I just noticed that you're doing this from the UI thread. Either dispatch > > this asynchronously on the DB thread or make sure that the bool inside > > SetBatchModeForTesting is threadsafe (add a Lock to the class and an AutoLock > in > > the method scope). > > This should all be running on the DB thread. If there's no DB thread, the > policy object won't get created.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dbabic@chromium.org/15573003/165001
Retried try job too often on linux_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dbabic@chromium.org/15573003/185001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dbabic@chromium.org/15573003/176003
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dbabic@chromium.org/15573003/205001
Message was sent while issue was closed.
Change committed as 206785 |
