[Offline Pages] Create a event/activity logger

[Offline Pages] Create a event/activity logger

This is used by both the offline page model and the request coordinator.

BUG=609570
Committed: https://crrev.com/70608500dc1a8019024023710ed4267a90069e43
Cr-Commit-Position: refs/heads/master@{#402156}

[chili, 2016-06-23 05:46:44]

Description was changed from

==========
[Offline Pages] Create a event/activity logger

This is used by both the offline page model and the request coordinator.

BUG=609570
==========

to

==========
[Offline Pages] Create a event/activity logger

This is used by both the offline page model and the request coordinator.

BUG=609570
==========

[chili, 2016-06-23 05:46:44]

chili@chromium.org changed reviewers:
+ dewittj@chromium.org, fgorski@chromium.org, petewil@google.com

[fgorski, 2016-06-23 16:34:07]

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:77: void
SetIsLogging(bool should_log);
I started review in the middle, so you might see a more surprising comment
later, but I think you could expose a method: GetOfflineLogger here. and not
copy it's interface on the consumers.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator_event_logger.h
(right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger.h:17: const
std::string& id);
Which ID is this referring to?
Please document and perhaps rename.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
File
components/offline_pages/background/request_coordinator_event_logger_unittest.cc
(right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:9:
namespace offline_internals {
I only noticed that now, but why did we go with a different namespace than
offline_pages?

If this is specific to this file, please fix it. I don't think we need a second
namespace for our stuff. (I think we only did that on Android).

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:11:
namespace {
I don't think we need an anonymous namespace here.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:13:
class RequestCoordinatorEventLoggerTest : public ::testing::Test {
Since the fixture is empty you can use TEST() macro instead of TEST_F()
https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?rcl=...

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:46:
for (int i = 0; i < 21; i++) {
I am guessing your buffer size is 20.
Could you please document that better. Perhaps a const instead of a magic
number?

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:51:
EXPECT_EQ(20, log.size());
ditto

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:205: // Logger to facilitate
recording of events.
? I am not sure I follow.
Did you mean to add a logger here?
Is there any reason to duplicate the logging interface?

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_event_logger.h (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_event_logger.h:17: const
std::string& id);
please document which ID it is.
If it is part of the client id, please consider moving it next to namespace. if
offline ID it can stay

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_event_logger.h:19: // Records that a
page with |id| has been deleted.
same: document ID please.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_event_logger_unittest.cc
(right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_event_logger_unittest.cc:9:
namespace offline_internals {
same comments apply to this file as to the other one.

(namespace, TEST(), magic number). Please apply here.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:683: void
OfflinePageModelImpl::GetLogs(std::vector<std::string>& storage_vector) {
I think a cleaner way would be to expose GetOfflineLogger method.
Please think about that.

[dewittj, 2016-06-23 22:19:03]

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_event_logger.cc (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_event_logger.cc:35: std::time_t current_time =
std::time(nullptr);
This is to low-leve.  I think in Chrome we normally use base::Time.  See:

base/time/time.h
base/i18n/time_formatting.h

https://cs.chromium.org/chromium/src/chromeos/dbus/update_engine_client.h?typ...
confirms my hypothesis, not much there.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_event_logger_unittest.cc
(right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_event_logger_unittest.cc:46: for
(int i = 0; i < 21; i++) {
please document maximum size in code like

const int kMaxLogSize = 20;
for (int i = 0; i < kMaxLogSize + 1; ++i) {
...
}

Also, small C++ nit: Many Chromies prefer that you write preincrement rather
than postincrement operators in for loops.

[Pete Williamson, 2016-06-24 00:38:31]

petewil@chromium.org changed reviewers:
+ petewil@chromium.org

[Pete Williamson, 2016-06-24 00:38:32]

Overall, I'm very impressed on how fast you went from being brand new to chrome
to turning out good quality code!

The formatting looks really good, including details like two spaces between "}"
and "// namespace", and after the endif on the include guards.

I have a lot of small comments and suggestions, but nothing really serious.

Because you have come up to speed so well, I'm not afraid to be nit-picky, so
there are some nits in the comments.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:154: void
RequestCoordinator::SetIsLogging(bool should_log) {
I don't see any calls to record logs (for example RecordSavePageRequestUpdated).
 It might be good to add a sample, so I can have a working example to start from
to add more where we want them.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
File
components/offline_pages/background/request_coordinator_event_logger_unittest.cc
(right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:20:
OfflinePageModelEventLogger logger;
Did you mean RequestCoordinatorEventLogger instead of
OfflinePageModelEventLogger?  (I see it in other functions in this file too,
maybe a cut and paste didn't get fully customized).

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:24:
logger.RecordSavePageRequestUpdated("last_n", "pending", "foo");
It might be nice to make last_n, pending, and foo into string constants at the
top of this file.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:27:
EXPECT_EQ(1, log.size());
Can we also test that the content of the logs are what we expect?  Something
like:

EXPECT_EQ("2010:06:21:1230:45.0071 - last_n - pending - foo",
log[0].getString());

Doing it in one test should be enough, I don't see a need to do it for every
test.

If it is tough to get the time string to match what you expect, I'd be OK with
just checking the part after the time string.

Alternately, you could use a base::Clock interface to get your time from, and
have a test clock that outputs a time you expect on the test so you can test
against an exact string.  I think FGorski may have made a similar suggestion
elsewhere in the changelist.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:46:
for (int i = 0; i < 21; i++) {
On 2016/06/23 16:34:06, fgorski wrote:
> I am guessing your buffer size is 20.
> Could you please document that better. Perhaps a const instead of a magic
> number?

Maybe you could expose MAX_LOGGED_ACTIVITY_COUNT in offline_event_logger.h
(instead of in a private namespace) and use that here?

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_event_logger.cc (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_event_logger.cc:12: const size_t
MAX_LOGGED_ACTIVITY_COUNT = 20;
Reading your code for the first time, I'm not sure what constitutes an activity
here. A comment might help.  For instance, does it represent a high level user
action, an individual function call, any "log" statement, or something else?  If
I understand that, I will have a better idea if 20 is a good number.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_event_logger.h (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_event_logger.h:16: class OfflineEventLogger {
A class header comment with usage notes would be nice here.

For instance, maybe SetLogging() and GetLogs() is called by
chrome://chrome-offlining.
Maybe how to put logs into the logger from the code being logged, and some
guidance about what to log. (Do we manually call to log interesting events?  Are
all function calls automatically traced into the log?)
Maybe some hint of how much we can log before the log cycles over.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_event_logger.h:37: std::deque<std::string>
activities_;
I like your choice of a deque here, it fits perfectly!

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_event_logger_unittest.cc
(right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_event_logger_unittest.cc:27:
EXPECT_EQ(1, log.size());
It would be nice to check the contents of the log string also if you can (I made
the same comment in the Request Coordinator file).

[chili, 2016-06-24 02:45:52]

Thanks for all the comments :)

PTAL

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.cc:154: void
RequestCoordinator::SetIsLogging(bool should_log) {
On 2016/06/24 00:38:31, Pete Williamson wrote:
> I don't see any calls to record logs (for example
RecordSavePageRequestUpdated).
>  It might be good to add a sample, so I can have a working example to start
from
> to add more where we want them.

You don't see any logs because I don't actually know where would be the best
place to record them.  I tentatively put something in the OfflinerDoneCallback
for now. Let me know if you think there's a better place for it.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator.h:77: void
SetIsLogging(bool should_log);
On 2016/06/23 16:34:06, fgorski wrote:
> I started review in the middle, so you might see a more surprising comment
> later, but I think you could expose a method: GetOfflineLogger here. and not
> copy it's interface on the consumers.

done

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
File components/offline_pages/background/request_coordinator_event_logger.h
(right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger.h:17: const
std::string& id);
On 2016/06/23 16:34:06, fgorski wrote:
> Which ID is this referring to?
> Please document and perhaps rename.

Done.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
File
components/offline_pages/background/request_coordinator_event_logger_unittest.cc
(right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:9:
namespace offline_internals {
On 2016/06/23 16:34:06, fgorski wrote:
> I only noticed that now, but why did we go with a different namespace than
> offline_pages?
> 
> If this is specific to this file, please fix it. I don't think we need a
second
> namespace for our stuff. (I think we only did that on Android).

Oops. brainfart.  Fixed.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:11:
namespace {
On 2016/06/23 16:34:06, fgorski wrote:
> I don't think we need an anonymous namespace here.

Removed. A couple tests I was looking at had this.  Are there some rules
surrounding use of this?

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:13:
class RequestCoordinatorEventLoggerTest : public ::testing::Test {
On 2016/06/23 16:34:06, fgorski wrote:
> Since the fixture is empty you can use TEST() macro instead of TEST_F()
>
https://cs.chromium.org/chromium/src/testing/gtest/include/gtest/gtest.h?rcl=...

Done.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:20:
OfflinePageModelEventLogger logger;
On 2016/06/24 00:38:32, Pete Williamson wrote:
> Did you mean RequestCoordinatorEventLogger instead of
> OfflinePageModelEventLogger?  (I see it in other functions in this file too,
> maybe a cut and paste didn't get fully customized).

haha good catch
oops

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:24:
logger.RecordSavePageRequestUpdated("last_n", "pending", "foo");
On 2016/06/24 00:38:32, Pete Williamson wrote:
> It might be nice to make last_n, pending, and foo into string constants at the
> top of this file.

Done.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:27:
EXPECT_EQ(1, log.size());
On 2016/06/24 00:38:32, Pete Williamson wrote:
> Can we also test that the content of the logs are what we expect?  Something
> like:
> 
> EXPECT_EQ("2010:06:21:1230:45.0071 - last_n - pending - foo",
> log[0].getString());
> 
> Doing it in one test should be enough, I don't see a need to do it for every
> test.
> 
> If it is tough to get the time string to match what you expect, I'd be OK with
> just checking the part after the time string.
> 
> Alternately, you could use a base::Clock interface to get your time from, and
> have a test clock that outputs a time you expect on the test so you can test
> against an exact string.  I think FGorski may have made a similar suggestion
> elsewhere in the changelist.

As explained in the other comment, I ultimately didn't end up testing for the
exact string because I didn't think test update should be necessary if the
logging string changes for any reason (better wording or typo/etc).  I'm happy
to add it in if you think otherwise.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:46:
for (int i = 0; i < 21; i++) {
On 2016/06/24 00:38:31, Pete Williamson wrote:
> On 2016/06/23 16:34:06, fgorski wrote:
> > I am guessing your buffer size is 20.
> > Could you please document that better. Perhaps a const instead of a magic
> > number?
> 
> Maybe you could expose MAX_LOGGED_ACTIVITY_COUNT in offline_event_logger.h
> (instead of in a private namespace) and use that here?

Done.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:51:
EXPECT_EQ(20, log.size());
On 2016/06/23 16:34:06, fgorski wrote:
> ditto

Done.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_event_logger.cc (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_event_logger.cc:12: const size_t
MAX_LOGGED_ACTIVITY_COUNT = 20;
On 2016/06/24 00:38:32, Pete Williamson wrote:
> Reading your code for the first time, I'm not sure what constitutes an
activity
> here. A comment might help.  For instance, does it represent a high level user
> action, an individual function call, any "log" statement, or something else? 
If
> I understand that, I will have a better idea if 20 is a good number.

Added comment. Let me know if it's not sufficient

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_event_logger.cc:35: std::time_t current_time =
std::time(nullptr);
On 2016/06/23 22:19:02, dewittj wrote:
> This is to low-leve.  I think in Chrome we normally use base::Time.  See:
> 
> base/time/time.h
> base/i18n/time_formatting.h
> 
>
https://cs.chromium.org/chromium/src/chromeos/dbus/update_engine_client.h?typ...
> confirms my hypothesis, not much there.

I looked at time_formatting.  Unfortunately, I want my time in a style such that
sorting lexicographically would also end up sorting chronologically, hence the
format by strftime.  I couldn't find something similarly customizable in
base/time

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_event_logger.h (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_event_logger.h:16: class OfflineEventLogger {
On 2016/06/24 00:38:32, Pete Williamson wrote:
> A class header comment with usage notes would be nice here.
> 
> For instance, maybe SetLogging() and GetLogs() is called by
> chrome://chrome-offlining.
> Maybe how to put logs into the logger from the code being logged, and some
> guidance about what to log. (Do we manually call to log interesting events? 
Are
> all function calls automatically traced into the log?)
> Maybe some hint of how much we can log before the log cycles over.

Done.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_event_logger.h:37: std::deque<std::string>
activities_;
On 2016/06/24 00:38:32, Pete Williamson wrote:
> I like your choice of a deque here, it fits perfectly!

Acknowledged. ^_^

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model.h:205: // Logger to facilitate
recording of events.
On 2016/06/23 16:34:06, fgorski wrote:
> ? I am not sure I follow.
> Did you mean to add a logger here?
> Is there any reason to duplicate the logging interface?

I had a Logger here with all its implementations so child classes don't have to
copy.  I was advised against that, and didn't clear it up all the way.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_event_logger.h (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_event_logger.h:17: const
std::string& id);
On 2016/06/23 16:34:07, fgorski wrote:
> please document which ID it is.
> If it is part of the client id, please consider moving it next to namespace.
if
> offline ID it can stay

Done.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_event_logger.h:19: // Records that a
page with |id| has been deleted.
On 2016/06/23 16:34:07, fgorski wrote:
> same: document ID please.

Done.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_event_logger_unittest.cc
(right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_event_logger_unittest.cc:9:
namespace offline_internals {
On 2016/06/23 16:34:07, fgorski wrote:
> same comments apply to this file as to the other one.
> 
> (namespace, TEST(), magic number). Please apply here.

Done.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_event_logger_unittest.cc:27:
EXPECT_EQ(1, log.size());
On 2016/06/24 00:38:32, Pete Williamson wrote:
> It would be nice to check the contents of the log string also if you can (I
made
> the same comment in the Request Coordinator file).

I had this debate with myself.  I ended up not checking the contents of the log
string because I think a change in how the log string is formatted shouldn't
result in a change of the test.  However, I do think the test as is feels kind
of empty, so let me know what you think :)

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_event_logger_unittest.cc:46: for
(int i = 0; i < 21; i++) {
On 2016/06/23 22:19:02, dewittj wrote:
> please document maximum size in code like
> 
> const int kMaxLogSize = 20;
> for (int i = 0; i < kMaxLogSize + 1; ++i) {
> ...
> }
> 
> Also, small C++ nit: Many Chromies prefer that you write preincrement rather
> than postincrement operators in for loops.

:(     .... but i like postincrement >"<

Changed

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:683: void
OfflinePageModelImpl::GetLogs(std::vector<std::string>& storage_vector) {
On 2016/06/23 16:34:07, fgorski wrote:
> I think a cleaner way would be to expose GetOfflineLogger method.
> Please think about that.

My original thought was that then people can add a bunch of activities to the
logger.  However, that was before I made a bunch of decisions so that the
assumption is no longer true.  Changed.

[Pete Williamson, 2016-06-24 16:52:58]

Mostly looks good, two issues left.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
File
components/offline_pages/background/request_coordinator_event_logger_unittest.cc
(right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:27:
EXPECT_EQ(1, log.size());
On 2016/06/24 02:45:51, chili wrote:
> On 2016/06/24 00:38:32, Pete Williamson wrote:
> > Can we also test that the content of the logs are what we expect?  Something
> > like:
> > 
> > EXPECT_EQ("2010:06:21:1230:45.0071 - last_n - pending - foo",
> > log[0].getString());
> > 
> > Doing it in one test should be enough, I don't see a need to do it for every
> > test.
> > 
> > If it is tough to get the time string to match what you expect, I'd be OK
with
> > just checking the part after the time string.
> > 
> > Alternately, you could use a base::Clock interface to get your time from,
and
> > have a test clock that outputs a time you expect on the test so you can test
> > against an exact string.  I think FGorski may have made a similar suggestion
> > elsewhere in the changelist.
> 
> As explained in the other comment, I ultimately didn't end up testing for the
> exact string because I didn't think test update should be necessary if the
> logging string changes for any reason (better wording or typo/etc).  I'm happy
> to add it in if you think otherwise.

OK, I understand that the test will break if we change the log string format,
but that just means we need to fix the test, and that's expected to fix tests
when we change code.  Let's add the test.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_event_logger.cc (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_event_logger.cc:12: const size_t
MAX_LOGGED_ACTIVITY_COUNT = 20;
On 2016/06/24 02:45:51, chili wrote:
> On 2016/06/24 00:38:32, Pete Williamson wrote:
> > Reading your code for the first time, I'm not sure what constitutes an
> activity
> > here. A comment might help.  For instance, does it represent a high level
user
> > action, an individual function call, any "log" statement, or something else?

> If
> > I understand that, I will have a better idea if 20 is a good number.
> 
> Added comment. Let me know if it's not sufficient

OK, thanks for the comment, 20 feels too small to me.  How about 50?

[fgorski, 2016-06-24 18:17:34]

Looks even better than the last time. A few comments, mostly nits.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_event_logger.cc (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_event_logger.cc:35: std::time_t current_time =
std::time(nullptr);
On 2016/06/24 02:45:51, chili wrote:
> On 2016/06/23 22:19:02, dewittj wrote:
> > This is to low-leve.  I think in Chrome we normally use base::Time.  See:
> > 
> > base/time/time.h
> > base/i18n/time_formatting.h
> > 
> >
>
https://cs.chromium.org/chromium/src/chromeos/dbus/update_engine_client.h?typ...
> > confirms my hypothesis, not much there.
> 
> I looked at time_formatting.  Unfortunately, I want my time in a style such
that
> sorting lexicographically would also end up sorting chronologically, hence the
> format by strftime.  I couldn't find something similarly customizable in
> base/time 

I think the point is to get away from time_t per comment in base/time/time.h
From/ToTimeT method.
(https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=454)

I see 2 possible solutions (hacky):
1. Use locale that prints the date in the form you want, e.g. pl_PL and use
TimeFormatShortDate (but there us a risk you are switching the whole chrome to
that locale, hence I would not do that) 
2. Do it in more than a single line by appending year, month, day, ... to a
string. After all this is a diagnostic printing mechanism.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:81: bool is_busy() {
productivity tip (that is with reviewer's productivity in mind):
for larger changes like this one try to separate the fixing from rebasing (that
order is preferred). It is cleaner when doing incremental review.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
File
components/offline_pages/background/request_coordinator_event_logger_unittest.cc
(right):

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:17:
} // namespace
nit: we use two spaces after }

}  //

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:54:
} // namespace offline_internals
please update to }  // (two spaces)
and fix the namespace name

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
File components/offline_pages/offline_event_logger.h (right):

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_event_logger.h:18: // Maximum number of
recorded Logs to keep track of at any moment.
I think you might want to put the documentation above the const definition and
remove the line below.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_event_logger.h:22: // call RecordActivity to
write into the log. SetIsLogging, GetLogs, and
|SetIsLogging|, ...

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_event_logger.h:27: // or when an offlined page
has been accessed/saved)
nit: please put period at the end of a sentence.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_event_logger.h:29: // This log only keeps track
of the last 20 events.
last |kMaxLogCount| events.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model_event_logger_unittest.cc
(right):

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_event_logger_unittest.cc:46: } //
namespace
}  //

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_event_logger_unittest.cc:48: } //
namespace offline_internals
}  // namespace offline_pages

[dewittj, 2016-06-24 18:32:49]

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:160: "Saved",
If this is the only place this is called, I'd probably just remove the "Saved"
parameter and embed it into the log message.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:161:
std::to_string(request.request_id()));
Seems like you could just pass the request_id here and do the string conversion
over in the logger.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
File components/offline_pages/offline_event_logger.h (right):

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_event_logger.h:43: void
GetLogs(std::vector<std::string>& records);
nit: use a pointer, non-const pass by reference is uncommon in Chrome.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model_event_logger_unittest.cc
(right):

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_event_logger_unittest.cc:11:
TEST(OfflinePageModelEventLoggerTest, RecordsWhenLoggingIsOn) {
These tests don't actually test the OfflinePageModelEventLogger; instead they
are probably better suited for the OfflineEventLogger directly.

The tests here should probably ensure that log.size() increments when you call
the OfflinePageModelEventLogger-specific functions.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:972:
offline_event_logger_.RecordStoreReloadError();
Is there any more detail that could be passed here? I imagine that this would be
a very interesting log event for someone debugging.

[chili, 2016-06-24 20:30:11]

Thanks for all the comments :)

I *think* I addressed all of them. Please bump comment again if I accidentally
missed yours

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
File
components/offline_pages/background/request_coordinator_event_logger_unittest.cc
(right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/ba...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:27:
EXPECT_EQ(1, log.size());
On 2016/06/24 16:52:58, Pete Williamson wrote:
> On 2016/06/24 02:45:51, chili wrote:
> > On 2016/06/24 00:38:32, Pete Williamson wrote:
> > > Can we also test that the content of the logs are what we expect? 
Something
> > > like:
> > > 
> > > EXPECT_EQ("2010:06:21:1230:45.0071 - last_n - pending - foo",
> > > log[0].getString());
> > > 
> > > Doing it in one test should be enough, I don't see a need to do it for
every
> > > test.
> > > 
> > > If it is tough to get the time string to match what you expect, I'd be OK
> with
> > > just checking the part after the time string.
> > > 
> > > Alternately, you could use a base::Clock interface to get your time from,
> and
> > > have a test clock that outputs a time you expect on the test so you can
test
> > > against an exact string.  I think FGorski may have made a similar
suggestion
> > > elsewhere in the changelist.
> > 
> > As explained in the other comment, I ultimately didn't end up testing for
the
> > exact string because I didn't think test update should be necessary if the
> > logging string changes for any reason (better wording or typo/etc).  I'm
happy
> > to add it in if you think otherwise.
> 
> OK, I understand that the test will break if we change the log string format,
> but that just means we need to fix the test, and that's expected to fix tests
> when we change code.  Let's add the test.

Added the test.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_event_logger.cc (right):

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_event_logger.cc:12: const size_t
MAX_LOGGED_ACTIVITY_COUNT = 20;
On 2016/06/24 16:52:58, Pete Williamson wrote:
> On 2016/06/24 02:45:51, chili wrote:
> > On 2016/06/24 00:38:32, Pete Williamson wrote:
> > > Reading your code for the first time, I'm not sure what constitutes an
> > activity
> > > here. A comment might help.  For instance, does it represent a high level
> user
> > > action, an individual function call, any "log" statement, or something
else?
> 
> > If
> > > I understand that, I will have a better idea if 20 is a good number.
> > 
> > Added comment. Let me know if it's not sufficient
> 
> OK, thanks for the comment, 20 feels too small to me.  How about 50?

Done.

https://codereview.chromium.org/2089413002/diff/1/components/offline_pages/of...
components/offline_pages/offline_event_logger.cc:35: std::time_t current_time =
std::time(nullptr);
On 2016/06/24 18:17:33, fgorski wrote:
> On 2016/06/24 02:45:51, chili wrote:
> > On 2016/06/23 22:19:02, dewittj wrote:
> > > This is to low-leve.  I think in Chrome we normally use base::Time.  See:
> > > 
> > > base/time/time.h
> > > base/i18n/time_formatting.h
> > > 
> > >
> >
>
https://cs.chromium.org/chromium/src/chromeos/dbus/update_engine_client.h?typ...
> > > confirms my hypothesis, not much there.
> > 
> > I looked at time_formatting.  Unfortunately, I want my time in a style such
> that
> > sorting lexicographically would also end up sorting chronologically, hence
the
> > format by strftime.  I couldn't find something similarly customizable in
> > base/time 
> 
> I think the point is to get away from time_t per comment in base/time/time.h
> From/ToTimeT method.
> (https://cs.chromium.org/chromium/src/base/time/time.h?rcl=0&l=454)
> 
> I see 2 possible solutions (hacky):
> 1. Use locale that prints the date in the form you want, e.g. pl_PL and use
> TimeFormatShortDate (but there us a risk you are switching the whole chrome to
> that locale, hence I would not do that) 
> 2. Do it in more than a single line by appending year, month, day, ... to a
> string. After all this is a diagnostic printing mechanism.

hmm... how does this look now?

A combination of using Time::Explode and StringPrintf

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.cc (right):

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:160: "Saved",
On 2016/06/24 18:32:49, dewittj wrote:
> If this is the only place this is called, I'd probably just remove the "Saved"
> parameter and embed it into the log message.

Ideally (probably in another CL), we can move the Status -> string method in
offline_internals_ui to here and call that to convert the string.  For a start,
though, I think the status parameter has a high chance of being very useful as
pete updates and completes the request coordinator.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.cc:161:
std::to_string(request.request_id()));
On 2016/06/24 18:32:49, dewittj wrote:
> Seems like you could just pass the request_id here and do the string
conversion
> over in the logger.

Done.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:81: bool is_busy() {
On 2016/06/24 18:17:33, fgorski wrote:
> productivity tip (that is with reviewer's productivity in mind):
> for larger changes like this one try to separate the fixing from rebasing
(that
> order is preferred). It is cleaner when doing incremental review.

Ack.

Just to make sure, you mean do git cl upload with the fix and then do git
rebase-update?  or do you mean 2 separate CLs?  (As far as I remember, I didn't
fix anything in this CL...)

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
File
components/offline_pages/background/request_coordinator_event_logger_unittest.cc
(right):

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:17:
} // namespace
On 2016/06/24 18:17:33, fgorski wrote:
> nit: we use two spaces after }
> 
> }  // 

Done.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator_event_logger_unittest.cc:54:
} // namespace offline_internals
On 2016/06/24 18:17:33, fgorski wrote:
> please update to }  // (two spaces)
> and fix the namespace name

Done.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
File components/offline_pages/offline_event_logger.h (right):

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_event_logger.h:18: // Maximum number of
recorded Logs to keep track of at any moment.
On 2016/06/24 18:17:33, fgorski wrote:
> I think you might want to put the documentation above the const definition and
> remove the line below.

Oops. I forgot to remove when I was debugging a different error.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_event_logger.h:22: // call RecordActivity to
write into the log. SetIsLogging, GetLogs, and
On 2016/06/24 18:17:33, fgorski wrote:
> |SetIsLogging|, ...

Done.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_event_logger.h:27: // or when an offlined page
has been accessed/saved)
On 2016/06/24 18:17:34, fgorski wrote:
> nit: please put period at the end of a sentence.

Done.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_event_logger.h:29: // This log only keeps track
of the last 20 events.
On 2016/06/24 18:17:33, fgorski wrote:
> last |kMaxLogCount| events.

Done.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_event_logger.h:43: void
GetLogs(std::vector<std::string>& records);
On 2016/06/24 18:32:49, dewittj wrote:
> nit: use a pointer, non-const pass by reference is uncommon in Chrome.

Done.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model_event_logger_unittest.cc
(right):

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_event_logger_unittest.cc:11:
TEST(OfflinePageModelEventLoggerTest, RecordsWhenLoggingIsOn) {
On 2016/06/24 18:32:49, dewittj wrote:
> These tests don't actually test the OfflinePageModelEventLogger; instead they
> are probably better suited for the OfflineEventLogger directly.
> 
> The tests here should probably ensure that log.size() increments when you call
> the OfflinePageModelEventLogger-specific functions.

Done.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_event_logger_unittest.cc:46: } //
namespace
On 2016/06/24 18:17:34, fgorski wrote:
> }  //

Done.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_event_logger_unittest.cc:48: } //
namespace offline_internals
On 2016/06/24 18:17:34, fgorski wrote:
> }  // namespace offline_pages

Done.

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:972:
offline_event_logger_.RecordStoreReloadError();
On 2016/06/24 18:32:49, dewittj wrote:
> Is there any more detail that could be passed here? I imagine that this would
be
> a very interesting log event for someone debugging.

I'm only going off the UMA histogram recording here.  I'm not sure myself what
kind of details would be useful to pass to the logs...

[Pete Williamson, 2016-06-24 20:51:02]

lgtm

[fgorski, 2016-06-24 21:00:51]

lgtm

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
File components/offline_pages/background/request_coordinator.h (right):

https://codereview.chromium.org/2089413002/diff/20001/components/offline_page...
components/offline_pages/background/request_coordinator.h:81: bool is_busy() {
On 2016/06/24 20:30:10, chili wrote:
> On 2016/06/24 18:17:33, fgorski wrote:
> > productivity tip (that is with reviewer's productivity in mind):
> > for larger changes like this one try to separate the fixing from rebasing
> (that
> > order is preferred). It is cleaner when doing incremental review.
> 
> Ack.
> 
> Just to make sure, you mean do git cl upload with the fix and then do git
> rebase-update?  or do you mean 2 separate CLs?  (As far as I remember, I
didn't
> fix anything in this CL...)

Fix first
Upload
rebase-update
upload again (even if you didn't have to change stuff --- if the files didn't
change there will be nothing to upload, but otherwise, there should be and that
marks the difference )

[dewittj, 2016-06-24 21:16:41]

lgtm

https://codereview.chromium.org/2089413002/diff/60001/components/offline_page...
File components/offline_pages/offline_event_logger.cc (right):

https://codereview.chromium.org/2089413002/diff/60001/components/offline_page...
components/offline_pages/offline_event_logger.cc:28: void
OfflineEventLogger::RecordActivity(const std::string& activity) {
any chance for OfflineEventLoggerTest?

https://codereview.chromium.org/2089413002/diff/60001/components/offline_page...
components/offline_pages/offline_event_logger.cc:50: void
OfflineEventLogger::GetLogs(std::vector<std::string>* records) {
nit: add DCHECK(records)

[chili, 2016-06-24 21:28:25]

chili@chromium.org changed reviewers:
- petewil@google.com

[chili, 2016-06-24 21:28:26]

Thanks everyone :)

https://codereview.chromium.org/2089413002/diff/60001/components/offline_page...
File components/offline_pages/offline_event_logger.cc (right):

https://codereview.chromium.org/2089413002/diff/60001/components/offline_page...
components/offline_pages/offline_event_logger.cc:28: void
OfflineEventLogger::RecordActivity(const std::string& activity) {
On 2016/06/24 21:16:41, dewittj wrote:
> any chance for OfflineEventLoggerTest?

I feel like this method (and class) is adequately tested via the
OfflinePageModelEventLogger and RequestCoordinatorEventLogger...

https://codereview.chromium.org/2089413002/diff/60001/components/offline_page...
components/offline_pages/offline_event_logger.cc:50: void
OfflineEventLogger::GetLogs(std::vector<std::string>* records) {
On 2016/06/24 21:16:41, dewittj wrote:
> nit: add DCHECK(records)

Done.

[chili, 2016-06-26 20:17:18]

The CQ bit was checked by chili@chromium.org

[chili, 2016-06-26 20:17:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
dewittj@chromium.org, petewil@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2089413002/#ps240001
(title: "rebase-update")

[commit-bot: I haz the power, 2016-06-26 20:17:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[chili, 2016-06-27 11:55:08]

The CQ bit was checked by chili@chromium.org

[chili, 2016-06-27 11:55:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
dewittj@chromium.org, petewil@chromium.org, fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/2089413002/#ps280001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-27 11:55:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 12:46:32]

Description was changed from

==========
[Offline Pages] Create a event/activity logger

This is used by both the offline page model and the request coordinator.

BUG=609570
==========

to

==========
[Offline Pages] Create a event/activity logger

This is used by both the offline page model and the request coordinator.

BUG=609570
==========

[commit-bot: I haz the power, 2016-06-27 12:46:33]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-06-27 12:48:36]

Description was changed from

==========
[Offline Pages] Create a event/activity logger

This is used by both the offline page model and the request coordinator.

BUG=609570
==========

to

==========
[Offline Pages] Create a event/activity logger

This is used by both the offline page model and the request coordinator.

BUG=609570

Committed: https://crrev.com/70608500dc1a8019024023710ed4267a90069e43
Cr-Commit-Position: refs/heads/master@{#402156}
==========

[commit-bot: I haz the power, 2016-06-27 12:48:37]

Patchset 15 (id:??) landed as
https://crrev.com/70608500dc1a8019024023710ed4267a90069e43
Cr-Commit-Position: refs/heads/master@{#402156}