Created method to save histogram data for future...

Created method to save histogram data for future
syncing of sessions.

Created a method to track certain session modifications
in order to determine what we want to track when
syncing sessions
BUG=30519
TEST=NONE

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=52252

[jerrica, 2010-07-08 21:02:21]

Hey Tim,

Will you please review this for me?

Thanks,

Jerrica

[jerrica, 2010-07-08 23:42:37]

Hi Tim,

Please review the changes I have made.

Thanks,

Jerrica

[tim (not reviewing), 2010-07-09 00:02:59]

http://codereview.chromium.org/2930002/diff/5001/6001
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/2930002/diff/5001/6001#newcode568
chrome/browser/sessions/session_service.cc:568: if (changed->type ==
NavigationType::EXISTING_PAGE)
I think we should group these together into an || and just call it
NavEntryCommitted.

http://codereview.chromium.org/2930002/diff/5001/6001#newcode1337
chrome/browser/sessions/session_service.cc:1337:
UMA_HISTOGRAM_CUSTOM_TIMES("SessionAltered.For:" + type + "Period",
We should leave this as SessionRestore since that's the existing category on the
dashboard for session stuff.  Also we shouldn't rename the existing histogram.

http://codereview.chromium.org/2930002/diff/5001/6001#newcode1338
chrome/browser/sessions/session_service.cc:1338: delta,
this all needs to be indented two more

http://codereview.chromium.org/2930002/diff/5001/6001#newcode1344
chrome/browser/sessions/session_service.cc:1344:
UMA_HISTOGRAM_CUSTOM_TIMES("SessionAltered.For:" + type + "LongPeriod",
indent off

http://codereview.chromium.org/2930002/diff/5001/6001#newcode1351
chrome/browser/sessions/session_service.cc:1351: last_altered_time_ =
base::TimeTicks::Now();
nit- last_updated_time

http://codereview.chromium.org/2930002/diff/5001/6002
File chrome/browser/sessions/session_service.h (right):

http://codereview.chromium.org/2930002/diff/5001/6002#newcode388
chrome/browser/sessions/session_service.h:388: // want to observe and call
Save() after in the future.  In addition, this is
we don't want to call save here (it gets called anyway).

http://codereview.chromium.org/2930002/diff/5001/6002#newcode392
chrome/browser/sessions/session_service.h:392: void
RecordAlteredSessionHistogramData(std::string type);
Altered? I'd probably go for RecordSessionUpdate* instead

[jerrica, 2010-07-09 00:13:47]

Hello Tim,

Please review these changes.

Thanks,

Jerrica

http://codereview.chromium.org/2930002/diff/5001/6001
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/2930002/diff/5001/6001#newcode568
chrome/browser/sessions/session_service.cc:568: if (changed->type ==
NavigationType::EXISTING_PAGE)
On 2010/07/09 00:03:00, timsteele wrote:
> I think we should group these together into an || and just call it
> NavEntryCommitted.

Done.

http://codereview.chromium.org/2930002/diff/5001/6001#newcode1337
chrome/browser/sessions/session_service.cc:1337:
UMA_HISTOGRAM_CUSTOM_TIMES("SessionAltered.For:" + type + "Period",
On 2010/07/09 00:03:00, timsteele wrote:
> We should leave this as SessionRestore since that's the existing category on
the
> dashboard for session stuff.  Also we shouldn't rename the existing histogram.

Done.

http://codereview.chromium.org/2930002/diff/5001/6001#newcode1338
chrome/browser/sessions/session_service.cc:1338: delta,
On 2010/07/09 00:03:00, timsteele wrote:
> this all needs to be indented two more

Done.

http://codereview.chromium.org/2930002/diff/5001/6001#newcode1344
chrome/browser/sessions/session_service.cc:1344:
UMA_HISTOGRAM_CUSTOM_TIMES("SessionAltered.For:" + type + "LongPeriod",
On 2010/07/09 00:03:00, timsteele wrote:
> indent off

Done.

http://codereview.chromium.org/2930002/diff/5001/6001#newcode1351
chrome/browser/sessions/session_service.cc:1351: last_altered_time_ =
base::TimeTicks::Now();
On 2010/07/09 00:03:00, timsteele wrote:
> nit- last_updated_time

Done.

[tim (not reviewing), 2010-07-09 02:27:50]

http://codereview.chromium.org/2930002/diff/10001/11002
File chrome/browser/sessions/session_service.h (right):

http://codereview.chromium.org/2930002/diff/10001/11002#newcode388
chrome/browser/sessions/session_service.h:388: // want to observe and call
Save() after in the future.  In addition, this is
still need to tweak this comment, don't want to call Save() after these in the
future.

[tim (not reviewing), 2010-07-09 02:28:18]

http://codereview.chromium.org/2930002/diff/10001/11001
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/2930002/diff/10001/11001#newcode1331
chrome/browser/sessions/session_service.cc:1331: void
SessionService::RecordAlteredSessionHistogramData(std::string type) {
doesn't look like the rename happened

[jerrica, 2010-07-09 16:58:30]

Hey Tim,

I had an oops moment.  I was making the changes and then got distracted with the
Session Model Associator stuff before I actually made the changes in the second
file.  My bad.  Hope you find everything in order now.

Thanks,

Jerrica

http://codereview.chromium.org/2930002/diff/5001/6002
File chrome/browser/sessions/session_service.h (right):

http://codereview.chromium.org/2930002/diff/5001/6002#newcode388
chrome/browser/sessions/session_service.h:388: // want to observe and call
Save() after in the future.  In addition, this is
On 2010/07/09 00:03:00, timsteele wrote:
> we don't want to call save here (it gets called anyway).

Done.

http://codereview.chromium.org/2930002/diff/5001/6002#newcode392
chrome/browser/sessions/session_service.h:392: void
RecordAlteredSessionHistogramData(std::string type);
On 2010/07/09 00:03:00, timsteele wrote:
> Altered? I'd probably go for RecordSessionUpdate* instead

Done.

[tim (not reviewing), 2010-07-09 17:47:43]

Something is wrong with the way you're calling the histogram macro.  Click on
one of the failing try bot runs (e.g. the red linux box at the bottom of the
patch set), and scroll to the bottom to see a "Check failed:" message.  I'd set
a breakpoint in histogram.cc @ 842 just before stepping into the CUSTOM_TIMES
call and see what the |name| parameter is and why the .find operation is pulling
out a histogram with a different name.

[jerrica, 2010-07-12 21:12:13]

Hey Tim,

I changed the implementation.  Will you please review the changes?

Thanks,

Jerrica

[jerrica, 2010-07-12 23:22:29]

Hi Tim,

I added another histogram.  Will you please review the changes?

Thanks,
Jerrica

[tim (not reviewing), 2010-07-13 00:06:12]

http://codereview.chromium.org/2930002/diff/6004/21001
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/2930002/diff/6004/21001#newcode1346
chrome/browser/sessions/session_service.cc:1346: bool longer = false;
nit - I'd call this use_long_period or something instead

http://codereview.chromium.org/2930002/diff/24001/25001#newcode1346
chrome/browser/sessions/session_service.cc:1346: bool longer = false;
nit - long_save_period

http://codereview.chromium.org/2930002/diff/24001/25001#newcode1368
chrome/browser/sessions/session_service.cc:1368: (*last_updated_time) =
base::TimeTicks::Now();
nit - NOTREACHED() << "..." is more common

http://codereview.chromium.org/2930002/diff/24001/25001#newcode1415
chrome/browser/sessions/session_service.cc:1415:
base::TimeDelta::FromMilliseconds(2500),
this and below should be indented two spaces less.  same for each Record*
method.

http://codereview.chromium.org/2930002/diff/24001/25001#newcode1419
chrome/browser/sessions/session_service.cc:1419: std::string
long_name_("SessionRestore.NavEntryCommittedLongPeriod");
we should use a constant for this now that it's all over the place

http://codereview.chromium.org/2930002/diff/24001/25002
File chrome/browser/sessions/session_service.h (right):

http://codereview.chromium.org/2930002/diff/24001/25002#newcode181
chrome/browser/sessions/session_service.h:181: static base::TimeTicks
last_updated_nav_entry_commit_time_;
these all need to be in the private section.
Also, why would we want static here?

[jerrica, 2010-07-13 01:15:22]

Hello Tim,

I made some changes.  Will you please review them.

Thanks,

Jerrica

http://codereview.chromium.org/2930002/diff/6004/21001
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/2930002/diff/6004/21001#newcode1346
chrome/browser/sessions/session_service.cc:1346: bool longer = false;
On 2010/07/13 00:06:12, timsteele wrote:
> nit - I'd call this use_long_period or something instead

Done.

http://codereview.chromium.org/2930002/diff/24001/25001#newcode1346
chrome/browser/sessions/session_service.cc:1346: bool longer = false;
On 2010/07/13 00:06:12, timsteele wrote:
> nit - long_save_period

Done.

http://codereview.chromium.org/2930002/diff/24001/25001#newcode1368
chrome/browser/sessions/session_service.cc:1368: (*last_updated_time) =
base::TimeTicks::Now();
On 2010/07/13 00:06:12, timsteele wrote:
> nit - NOTREACHED() << "..." is more common

Done.

http://codereview.chromium.org/2930002/diff/24001/25001#newcode1415
chrome/browser/sessions/session_service.cc:1415:
base::TimeDelta::FromMilliseconds(2500),
On 2010/07/13 00:06:12, timsteele wrote:
> this and below should be indented two spaces less.  same for each Record*
> method.

Done.

http://codereview.chromium.org/2930002/diff/24001/25001#newcode1419
chrome/browser/sessions/session_service.cc:1419: std::string
long_name_("SessionRestore.NavEntryCommittedLongPeriod");
On 2010/07/13 00:06:12, timsteele wrote:
> we should use a constant for this now that it's all over the place

Done.

http://codereview.chromium.org/2930002/diff/24001/25002
File chrome/browser/sessions/session_service.h (right):

http://codereview.chromium.org/2930002/diff/24001/25002#newcode181
chrome/browser/sessions/session_service.h:181: static base::TimeTicks
last_updated_nav_entry_commit_time_;
On 2010/07/13 00:06:12, timsteele wrote:
> these all need to be in the private section.
> Also, why would we want static here?

Done.

[tim (not reviewing), 2010-07-13 16:33:53]

After you fix the stuff mentioned below, this LGTM!

http://codereview.chromium.org/2930002/diff/32001/33001
File chrome/browser/sessions/session_service.cc (right):

http://codereview.chromium.org/2930002/diff/32001/33001#newcode533
chrome/browser/sessions/session_service.cc:533: &last_updated_tab_closed_time_);
indent two more spaces

http://codereview.chromium.org/2930002/diff/32001/33001#newcode551
chrome/browser/sessions/session_service.cc:551:
&last_updated_nav_list_pruned_time_);
indent two more spaces

http://codereview.chromium.org/2930002/diff/32001/33001#newcode578
chrome/browser/sessions/session_service.cc:578:
&last_updated_nav_entry_commit_time_);
if an 'if' statement is more than one line, it needs braces

http://codereview.chromium.org/2930002/diff/32001/33001#newcode1343
chrome/browser/sessions/session_service.cc:1343: if
(!(*last_updated_time).is_null()) {
nit "if (!last_updated_time->is_null())"

http://codereview.chromium.org/2930002/diff/32001/33001#newcode1369
chrome/browser/sessions/session_service.cc:1369: NOTREACHED() << "Bad type sent
to RecordSessionUpdateHistogramData";
nit - indent one more space