Issue 274057: Add histogram for how tab closing time. Did some cleanup along the way.

Issue 274057: Add histogram for how tab closing time. Did some cleanup along the way. (Closed)

Created:
11 years, 2 months ago by jam

Modified:
9 years, 7 months ago

Reviewers:
tony

CC:
chromium-reviews_googlegroups.com, brettw+cc_chromium.org, ben+cc_chromium.org, Erik does not do reviews, pam+watch_chromium.org, Paweł Hajdan Jr., darin (slow to review), tim (not reviewing)

Base URL:
svn://chrome-svn/chrome/trunk/src/

Visibility:
Public.

Description

Add histogram for how tab closing time. Did some cleanup along the way. Moved the is_showing_before_unload_dialog_ stuff from RenderViewHost to TabContents since we need that bit there as well. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=29063

Patch Set 1 : '' #

Total comments: 6

Created: 11 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Stats (+66 lines, -60 lines)			Patch
M	chrome/browser/extensions/extension_host.h	View	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/extensions/extension_host.cc	View	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/renderer_host/render_view_host.h	View	1 chunk	+0 lines, -5 lines	2 comments	Download
M	chrome/browser/renderer_host/render_view_host.cc	View	5 chunks	+3 lines, -12 lines	0 comments	Download
M	chrome/browser/renderer_host/render_view_host_delegate.h	View	1 chunk	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/resources/new_new_tab.html	View	1 chunk	+1 line, -1 line	0 comments	Download
M	chrome/browser/resources/new_new_tab.js	View	1 chunk	+2 lines, -2 lines	0 comments	Download
M	chrome/browser/tab_contents/render_view_host_manager.h	View	1 chunk	+0 lines, -5 lines	0 comments	Download
M	chrome/browser/tab_contents/render_view_host_manager.cc	View	1 chunk	+0 lines, -7 lines	0 comments	Download
M	chrome/browser/tab_contents/tab_contents.h	View	5 chunks	+15 lines, -3 lines	0 comments	Download
M	chrome/browser/tab_contents/tab_contents.cc	View	7 chunks	+30 lines, -9 lines	2 comments	Download
M	chrome/browser/tabs/tab_strip_model.cc	View	2 chunks	+8 lines, -9 lines	2 comments	Download
M	chrome/test/startup/feature_startup_test.cc	View	1 chunk	+3 lines, -3 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

tony

LGTM, just some questions for my own education. http://codereview.chromium.org/274057/diff/3014/41 File chrome/browser/renderer_host/render_view_host.h (left): http://codereview.chromium.org/274057/diff/3014/41#oldcode645 Line 645: ...

11 years, 2 months ago (2009-10-14 23:20:06 UTC) #2

jam

11 years, 2 months ago (2009-10-14 23:31:11 UTC) #3

http://codereview.chromium.org/274057/diff/3014/41
File chrome/browser/renderer_host/render_view_host.h (left):

http://codereview.chromium.org/274057/diff/3014/41#oldcode645
Line 645: bool is_showing_before_unload_dialog_;
On 2009/10/14 23:20:06, tony wrote:
> Nit: Can you explain why this moved in the change description?

Oh, that doesn't fix any bug.  It's just convenient so that the flag is in
tab_contents, which is where I want to do the time related code.  I needed a
flag there to know when a before unload dialog closes, to reset the time.  Added
a comment to the description.

http://codereview.chromium.org/274057/diff/3014/35
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/274057/diff/3014/35#newcode1216
Line 1216: if (tab_close_start_time_.is_null())
On 2009/10/14 23:20:06, tony wrote:
> Is it possible for this to be called twice?

Yes it does, since InternalCloseTabs will get called twice for a tab that has
unload handlers.  I wasn't sure if I should duplicate this comment here, since
it's already in the declaration of that function.

http://codereview.chromium.org/274057/diff/3014/38
File chrome/browser/tabs/tab_strip_model.cc (left):

http://codereview.chromium.org/274057/diff/3014/38#oldcode657
Line 657: if (detached_contents) {
On 2009/10/14 23:20:06, tony wrote:
> Maybe we should DCHECK |detached_contents| above?

I took it out because delegate_->RunUnloadListenerBeforeClosing uses it without
null checking, so if it could have been NULL we'd have seen crashes there.  It
also looks like GetContentsAt has a CHECK there.

Expand Messages | Collapse Messages