Record first run startup metrics.

Record first run startup metrics.

Also fixing first run code to only call startup_metric_utils::SetNonBrowserUIDisplayed() if the dialog is actually displayed (which it isn't in many situations).

NOTRY=True (patchset 6 already passed all the try bots, simply updating a string in patchset 7)
BUG=237933, 219419
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198707

[gab, 2013-05-03 21:29:37]

Hey Jeremy, please take a look.

Thanks!
Gab

[jeremy, 2013-05-05 12:35:55]

https://codereview.chromium.org/14946003/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/14946003/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main.cc:1762:
"Startup.BrowserMessageLoopStartTime",
I don't think you need to add a new histogram here, add one to the
"Startup.BrowserMessageLoopStartTimeFromMainEntry" instead.

LoopStartTime and LoopStartTimeFromMainEntry basically measure the same thing
(the former being the old version which is more noisy since it measures from the
OS' notion of process startup so we also measure binary load time)

https://codereview.chromium.org/14946003/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main.h (right):

https://codereview.chromium.org/14946003/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main.h:219: //
Startup.FirstRun.BrowserMessageLoopStartTime instead if |is_first_run|).
2 histogram names seems confusing here, how about changing this comment to read:
Record time from process startup to present time in an UMA histogram.
|is_first_run| - is the current launch part of a first run.

https://codereview.chromium.org/14946003/diff/1/chrome/browser/first_run/firs...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/14946003/diff/1/chrome/browser/first_run/firs...
chrome/browser/first_run/first_run.cc:746: // recording erroneous startup
metrics.
I don't think you need to update the comment - the original if wasn't clear but
the function call is self documenting IMHO.

https://codereview.chromium.org/14946003/diff/1/chrome/browser/first_run/firs...
chrome/browser/first_run/first_run.cc:748: ShowFirstRunDialog(profile)) {
Please phrase as a nested if rather than compound logic with side effects.

https://codereview.chromium.org/14946003/diff/1/chrome/browser/first_run/firs...
File chrome/browser/first_run/first_run_dialog.h (right):

https://codereview.chromium.org/14946003/diff/1/chrome/browser/first_run/firs...
chrome/browser/first_run/first_run_dialog.h:14: bool ShowFirstRunDialog(Profile*
profile);
Rather than changing the prototype, can you #ifdef the calling code
appropriately and remove the dummy function body for this function?

https://codereview.chromium.org/14946003/diff/1/chrome/common/startup_metric_...
File chrome/common/startup_metric_utils.cc (right):

https://codereview.chromium.org/14946003/diff/1/chrome/common/startup_metric_...
chrome/common/startup_metric_utils.cc:100:
"Startup.FirstRun.BrowserMessageLoopStartTimeFromMainEntry_Exact" :
This histogram is only used for perf tests and is never sent to the server - I
don't think we need to add a new variant for first run...

https://codereview.chromium.org/14946003/diff/1/chrome/common/startup_metric_...
File chrome/common/startup_metric_utils.h (right):

https://codereview.chromium.org/14946003/diff/1/chrome/common/startup_metric_...
chrome/common/startup_metric_utils.h:35: // startup stats (split into different
stats based on |is_first_run|).
Don't think the comment needs updating.

[gab, 2013-05-06 16:19:53]

Comments addressed, PTAL.

Thanks!
Gab

https://codereview.chromium.org/14946003/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/14946003/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main.cc:1762:
"Startup.BrowserMessageLoopStartTime",
On 2013/05/05 12:35:56, jeremy wrote:
> I don't think you need to add a new histogram here, add one to the
> "Startup.BrowserMessageLoopStartTimeFromMainEntry" instead.
> 
> LoopStartTime and LoopStartTimeFromMainEntry basically measure the same thing
> (the former being the old version which is more noisy since it measures from
the
> OS' notion of process startup so we also measure binary load time)

Ok, let's not record this one on first run though then since that would add even
more noise to this stat.

https://codereview.chromium.org/14946003/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main.h (right):

https://codereview.chromium.org/14946003/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main.h:219: //
Startup.FirstRun.BrowserMessageLoopStartTime instead if |is_first_run|).
On 2013/05/05 12:35:56, jeremy wrote:
> 2 histogram names seems confusing here, how about changing this comment to
read:
> Record time from process startup to present time in an UMA histogram.
> |is_first_run| - is the current launch part of a first run.

Done.

https://codereview.chromium.org/14946003/diff/1/chrome/browser/first_run/firs...
File chrome/browser/first_run/first_run.cc (right):

https://codereview.chromium.org/14946003/diff/1/chrome/browser/first_run/firs...
chrome/browser/first_run/first_run.cc:746: // recording erroneous startup
metrics.
On 2013/05/05 12:35:56, jeremy wrote:
> I don't think you need to update the comment - the original if wasn't clear
but
> the function call is self documenting IMHO.

Done.

https://codereview.chromium.org/14946003/diff/1/chrome/browser/first_run/firs...
chrome/browser/first_run/first_run.cc:748: ShowFirstRunDialog(profile)) {
On 2013/05/05 12:35:56, jeremy wrote:
> Please phrase as a nested if rather than compound logic with side effects.

Done.

https://codereview.chromium.org/14946003/diff/1/chrome/browser/first_run/firs...
File chrome/browser/first_run/first_run_dialog.h (right):

https://codereview.chromium.org/14946003/diff/1/chrome/browser/first_run/firs...
chrome/browser/first_run/first_run_dialog.h:14: bool ShowFirstRunDialog(Profile*
profile);
On 2013/05/05 12:35:56, jeremy wrote:
> Rather than changing the prototype, can you #ifdef the calling code
> appropriately and remove the dummy function body for this function?

Removed the views implementation and moved the call to first_run_posix.cc. This
also makes it more explicit that it's broken for Linux+Aura
(http://crbug.com/234637) -- and also easier to fix since adding a views
implementation will no longer trigger this to show on Windows where it shouldn't
show (I think the EULA replaces this on Windows).

Still modified the interface to return a bool to specify whether the dialog was
shown though as there are many cases where it is not shown and this will help us
get first run stats on Linux/Mac which we wouldn't get if we just blindly assume
that calling this method resulted in a dialog being shown.

https://codereview.chromium.org/14946003/diff/1/chrome/common/startup_metric_...
File chrome/common/startup_metric_utils.cc (right):

https://codereview.chromium.org/14946003/diff/1/chrome/common/startup_metric_...
chrome/common/startup_metric_utils.cc:91:
"Startup.BrowserMessageLoopStartTimeFromMainEntry",
Also split this into two explicit calls since these macros are meant to be used
with constant histogram values I think.

https://codereview.chromium.org/14946003/diff/1/chrome/common/startup_metric_...
chrome/common/startup_metric_utils.cc:100:
"Startup.FirstRun.BrowserMessageLoopStartTimeFromMainEntry_Exact" :
On 2013/05/05 12:35:56, jeremy wrote:
> This histogram is only used for perf tests and is never sent to the server - I
> don't think we need to add a new variant for first run...

Done.

https://codereview.chromium.org/14946003/diff/1/chrome/common/startup_metric_...
File chrome/common/startup_metric_utils.h (right):

https://codereview.chromium.org/14946003/diff/1/chrome/common/startup_metric_...
chrome/common/startup_metric_utils.h:35: // startup stats (split into different
stats based on |is_first_run|).
On 2013/05/05 12:35:56, jeremy wrote:
> Don't think the comment needs updating.

Done (still added what is_first_run is, keeping the same format as in
chrome_browser_main.h).

[jeremy, 2013-05-06 17:59:38]

https://codereview.chromium.org/14946003/diff/40001/chrome/browser/ui/gtk/fir...
File chrome/browser/ui/gtk/first_run_dialog.cc (right):

https://codereview.chromium.org/14946003/diff/40001/chrome/browser/ui/gtk/fir...
chrome/browser/ui/gtk/first_run_dialog.cc:89: return;  // Nothing to do
Does this compile?

https://codereview.chromium.org/14946003/diff/40001/chrome/common/startup_met...
File chrome/common/startup_metric_utils.cc (right):

https://codereview.chromium.org/14946003/diff/40001/chrome/common/startup_met...
chrome/common/startup_metric_utils.cc:88: if (is_first_run) {
Can you make this one call and stick the histogram name in a char* to pass in?

[gab, 2013-05-06 18:44:51]

PTAL.

Ilya, please look at the discussion in startup_metric_utils.cc

Thanks,
Gab

https://codereview.chromium.org/14946003/diff/40001/chrome/browser/ui/gtk/fir...
File chrome/browser/ui/gtk/first_run_dialog.cc (right):

https://codereview.chromium.org/14946003/diff/40001/chrome/browser/ui/gtk/fir...
chrome/browser/ui/gtk/first_run_dialog.cc:89: return;  // Nothing to do
On 2013/05/06 17:59:38, jeremy wrote:
> Does this compile?

Whoops, you're right :) -- I'm on Windows! Cleaned up the interface some more
since |show_reporting_dialog| was already handling the early return and the
|response| was never used.

https://codereview.chromium.org/14946003/diff/40001/chrome/common/startup_met...
File chrome/common/startup_metric_utils.cc (right):

https://codereview.chromium.org/14946003/diff/40001/chrome/common/startup_met...
chrome/common/startup_metric_utils.cc:88: if (is_first_run) {
On 2013/05/06 17:59:38, jeremy wrote:
> Can you make this one call and stick the histogram name in a char* to pass in?

As explained to me by isherman@ (see [1]): the macros use a static local
variable and thus must have a constant string as their first parameter.

In that CL where we wanted to dynamically add a suffix which led to [2].

Perhaps it would be fine here to have 2 constant strings (i.e. this is not as
bad dynamically generated suffixed strings), but I'm not sure...

[1]
https://chromiumcodereview.appspot.com/14246040/diff/13006/chrome/browser/imp...
[2]
https://chromiumcodereview.appspot.com/14246040/diff/42009/chrome/browser/imp...

[jeremy, 2013-05-06 19:09:38]

LGTM for code surrounding startup UMAs.

[gab, 2013-05-06 19:29:29]

Thanks,

@Nico for OWNER

Thanks!
Gab

[Nico, 2013-05-06 21:15:08]

https://codereview.chromium.org/14946003/diff/55001/chrome/browser/first_run/...
File chrome/browser/first_run/first_run.cc (left):

https://codereview.chromium.org/14946003/diff/55001/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:753: }
nit: This looks like it could've been moved in a separate CL?

https://codereview.chromium.org/14946003/diff/55001/chrome/browser/ui/gtk/fir...
File chrome/browser/ui/gtk/first_run_dialog.cc (right):

https://codereview.chromium.org/14946003/diff/55001/chrome/browser/ui/gtk/fir...
chrome/browser/ui/gtk/first_run_dialog.cc:99: }
Is this right? Previously the dialog would call OnResponseDialog() immediately,
which would call first_run::SetShouldShowWelcomePage() and
first_run::CreateSentinel() – is this no longer necessary?

[gab, 2013-05-06 21:28:35]

Nico, PTAL, see replies below.

Thanks!
Gab

https://codereview.chromium.org/14946003/diff/55001/chrome/browser/first_run/...
File chrome/browser/first_run/first_run.cc (left):

https://codereview.chromium.org/14946003/diff/55001/chrome/browser/first_run/...
chrome/browser/first_run/first_run.cc:753: }
On 2013/05/06 21:15:09, Nico wrote:
> nit: This looks like it could've been moved in a separate CL?

It could, but this code needed to change as part of this CL already and jeremy
asked me to move it to the appropriate section (see [1]) instead of relying on
the views impl returning false which makes sense to me. Happy to do it in a
separate CL if you feel strongly.

[1]
https://codereview.chromium.org/14946003/diff/1/chrome/browser/first_run/firs...

https://codereview.chromium.org/14946003/diff/55001/chrome/browser/ui/gtk/fir...
File chrome/browser/ui/gtk/first_run_dialog.cc (right):

https://codereview.chromium.org/14946003/diff/55001/chrome/browser/ui/gtk/fir...
chrome/browser/ui/gtk/first_run_dialog.cc:99: }
On 2013/05/06 21:15:09, Nico wrote:
> Is this right? Previously the dialog would call OnResponseDialog()
immediately,
> which would call first_run::SetShouldShowWelcomePage() and
> first_run::CreateSentinel() – is this no longer necessary?

The response wasn't used previously.

This method also only creates a FirstRunDialog if |show_reporting_dialog| so
that passing it in the constructor was superfluous.

first_run::CreateSentinel() is not required here, but I realize that this is the
case in other places in the code and I'll fix that in a separate CL instead of
squeezing it in here (added it back to this CL).

[Nico, 2013-05-06 21:31:50]

https://codereview.chromium.org/14946003/diff/55001/chrome/browser/ui/gtk/fir...
File chrome/browser/ui/gtk/first_run_dialog.cc (right):

https://codereview.chromium.org/14946003/diff/55001/chrome/browser/ui/gtk/fir...
chrome/browser/ui/gtk/first_run_dialog.cc:99: }
On 2013/05/06 21:28:36, gab wrote:
> On 2013/05/06 21:15:09, Nico wrote:
> > Is this right? Previously the dialog would call OnResponseDialog()
> immediately,
> > which would call first_run::SetShouldShowWelcomePage() and
> > first_run::CreateSentinel() – is this no longer necessary?
> 
> The response wasn't used previously.

No, but it might've been called for its side effects (such as creating the
sentinel file and setting up the welcome page. Now these side effect aren't
done.

> 
> This method also only creates a FirstRunDialog if |show_reporting_dialog| so
> that passing it in the constructor was superfluous.
> 
> first_run::CreateSentinel() is not required here, but I realize that this is
the
> case in other places in the code and I'll fix that in a separate CL instead of
> squeezing it in here (added it back to this CL).

[gab, 2013-05-06 21:40:10]

https://codereview.chromium.org/14946003/diff/55001/chrome/browser/ui/gtk/fir...
File chrome/browser/ui/gtk/first_run_dialog.cc (right):

https://codereview.chromium.org/14946003/diff/55001/chrome/browser/ui/gtk/fir...
chrome/browser/ui/gtk/first_run_dialog.cc:99: }
On 2013/05/06 21:31:50, Nico wrote:
> On 2013/05/06 21:28:36, gab wrote:
> > On 2013/05/06 21:15:09, Nico wrote:
> > > Is this right? Previously the dialog would call OnResponseDialog()
> > immediately,
> > > which would call first_run::SetShouldShowWelcomePage() and
> > > first_run::CreateSentinel() – is this no longer necessary?
> > 
> > The response wasn't used previously.
> 
> No, but it might've been called for its side effects (such as creating the
> sentinel file and setting up the welcome page. Now these side effect aren't
> done.
> 

Hmmm, I only removed the storing of |response| into |response_|
OnResponseDialog() is still called, no? Is there some subtle GTK magic I don't
see here?

> > 
> > This method also only creates a FirstRunDialog if |show_reporting_dialog| so
> > that passing it in the constructor was superfluous.
> > 
> > first_run::CreateSentinel() is not required here, but I realize that this is
> the
> > case in other places in the code and I'll fix that in a separate CL instead
of
> > squeezing it in here (added it back to this CL).
>

[Nico, 2013-05-06 21:55:40]

The gtk code looks wrong, but you're right that it's been that way for a while.
So lgtm, and I cc'd a few people who can decide if we can delete the dead code
in that file, or if we ened to do something else. (But this CL doesn't have to
wait for that decision.)

https://codereview.chromium.org/14946003/diff/55001/chrome/browser/ui/gtk/fir...
File chrome/browser/ui/gtk/first_run_dialog.cc (right):

https://codereview.chromium.org/14946003/diff/55001/chrome/browser/ui/gtk/fir...
chrome/browser/ui/gtk/first_run_dialog.cc:99: }
On 2013/05/06 21:40:10, gab wrote:
> On 2013/05/06 21:31:50, Nico wrote:
> > On 2013/05/06 21:28:36, gab wrote:
> > > On 2013/05/06 21:15:09, Nico wrote:
> > > > Is this right? Previously the dialog would call OnResponseDialog()
> > > immediately,
> > > > which would call first_run::SetShouldShowWelcomePage() and
> > > > first_run::CreateSentinel() – is this no longer necessary?
> > > 
> > > The response wasn't used previously.
> > 
> > No, but it might've been called for its side effects (such as creating the
> > sentinel file and setting up the welcome page. Now these side effect aren't
> > done.
> > 
> 
> Hmmm, I only removed the storing of |response| into |response_|
> OnResponseDialog() is still called, no? Is there some subtle GTK magic I don't
> see here?
> 
> > > 
> > > This method also only creates a FirstRunDialog if |show_reporting_dialog|
so
> > > that passing it in the constructor was superfluous.
> > > 
> > > first_run::CreateSentinel() is not required here, but I realize that this
is
> > the
> > > case in other places in the code and I'll fix that in a separate CL
instead
> of
> > > squeezing it in here (added it back to this CL).
> > 
> 

Ah, I didn't see tge early return if (!show_reporting_dialog) slightly higher
up. What threw me off is that FirstRunDialog::ShowReportingDialog() checks if
(!show_reporting_dialog) too, but due to the early exit that branch will never
be taken. +mnissler who wrote the lengthy-ish comment about this block of dead
code, +jeanluc which added the early exit here in
https://codereview.chromium.org/3847006 and +msw who touched it somewhat
recently (but doesn't really look all that related:
https://codereview.chromium.org/9285002)

[commit-bot: I haz the power, 2013-05-06 22:02:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/14946003/66001

[Ilya Sherman, 2013-05-06 23:46:24]

https://chromiumcodereview.appspot.com/14946003/diff/40001/chrome/common/star...
File chrome/common/startup_metric_utils.cc (right):

https://chromiumcodereview.appspot.com/14946003/diff/40001/chrome/common/star...
chrome/common/startup_metric_utils.cc:88: if (is_first_run) {
On 2013/05/06 18:44:51, gab wrote:
> On 2013/05/06 17:59:38, jeremy wrote:
> > Can you make this one call and stick the histogram name in a char* to pass
in?
> 
> As explained to me by isherman@ (see [1]): the macros use a static local
> variable and thus must have a constant string as their first parameter.
> 
> In that CL where we wanted to dynamically add a suffix which led to [2].
> 
> Perhaps it would be fine here to have 2 constant strings (i.e. this is not as
> bad dynamically generated suffixed strings), but I'm not sure...
> 
> [1]
>
https://chromiumcodereview.appspot.com/14246040/diff/13006/chrome/browser/imp...
> [2]
>
https://chromiumcodereview.appspot.com/14246040/diff/42009/chrome/browser/imp...

Specifically, the requirement is that string needs to be a runtime constant. 
But, it's simplest not to try to worry about what strings are guaranteed to be
runtime constants vs. not and just do what you've done here.

https://chromiumcodereview.appspot.com/14946003/diff/40001/chrome/common/star...
chrome/common/startup_metric_utils.cc:90:
"Startup.FirstRun.BrowserMessageLoopStartTimeFromMainEntry",
nit: You might want to name this
"Startup.BrowserMessageLoopStartTimeFromMainEntry.FirstRun" instead, so that the
two histograms are visually grouped on the dashboard (where they get sorted
alphabetically).

[gab, 2013-05-07 02:59:03]

https://chromiumcodereview.appspot.com/14946003/diff/40001/chrome/common/star...
File chrome/common/startup_metric_utils.cc (right):

https://chromiumcodereview.appspot.com/14946003/diff/40001/chrome/common/star...
chrome/common/startup_metric_utils.cc:88: if (is_first_run) {
On 2013/05/06 23:46:24, Ilya Sherman wrote:
> On 2013/05/06 18:44:51, gab wrote:
> > On 2013/05/06 17:59:38, jeremy wrote:
> > > Can you make this one call and stick the histogram name in a char* to pass
> in?
> > 
> > As explained to me by isherman@ (see [1]): the macros use a static local
> > variable and thus must have a constant string as their first parameter.
> > 
> > In that CL where we wanted to dynamically add a suffix which led to [2].
> > 
> > Perhaps it would be fine here to have 2 constant strings (i.e. this is not
as
> > bad dynamically generated suffixed strings), but I'm not sure...
> > 
> > [1]
> >
>
https://chromiumcodereview.appspot.com/14246040/diff/13006/chrome/browser/imp...
> > [2]
> >
>
https://chromiumcodereview.appspot.com/14246040/diff/42009/chrome/browser/imp...
> 
> Specifically, the requirement is that string needs to be a runtime constant. 
> But, it's simplest not to try to worry about what strings are guaranteed to be
> runtime constants vs. not and just do what you've done here.

I see, thanks for the feedback, will keep it as is.

https://chromiumcodereview.appspot.com/14946003/diff/40001/chrome/common/star...
chrome/common/startup_metric_utils.cc:90:
"Startup.FirstRun.BrowserMessageLoopStartTimeFromMainEntry",
On 2013/05/06 23:46:24, Ilya Sherman wrote:
> nit: You might want to name this
> "Startup.BrowserMessageLoopStartTimeFromMainEntry.FirstRun" instead, so that
the
> two histograms are visually grouped on the dashboard (where they get sorted
> alphabetically).

Good point, I initially had more than one Startup.FirstRun stat (see patchset 1)
which is why I originally put it that way (i.e., so that all Startup.FirstRun
stats would be together), but now that it's only one, it will be more
discoverable if it immediately follows the existing stat.

[gab, 2013-05-07 02:59:21]

Thanks for the feedback Ilya!

[commit-bot: I haz the power, 2013-05-07 02:59:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/14946003/84001

[commit-bot: I haz the power, 2013-05-07 03:15:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/14946003/84001

[Mattias Nissler (ping if slow), 2013-05-07 08:27:51]

https://codereview.chromium.org/14946003/diff/55001/chrome/browser/ui/gtk/fir...
File chrome/browser/ui/gtk/first_run_dialog.cc (right):

https://codereview.chromium.org/14946003/diff/55001/chrome/browser/ui/gtk/fir...
chrome/browser/ui/gtk/first_run_dialog.cc:99: }
On 2013/05/06 21:55:40, Nico wrote:
> On 2013/05/06 21:40:10, gab wrote:
> > On 2013/05/06 21:31:50, Nico wrote:
> > > On 2013/05/06 21:28:36, gab wrote:
> > > > On 2013/05/06 21:15:09, Nico wrote:
> > > > > Is this right? Previously the dialog would call OnResponseDialog()
> > > > immediately,
> > > > > which would call first_run::SetShouldShowWelcomePage() and
> > > > > first_run::CreateSentinel() – is this no longer necessary?
> > > > 
> > > > The response wasn't used previously.
> > > 
> > > No, but it might've been called for its side effects (such as creating the
> > > sentinel file and setting up the welcome page. Now these side effect
aren't
> > > done.
> > > 
> > 
> > Hmmm, I only removed the storing of |response| into |response_|
> > OnResponseDialog() is still called, no? Is there some subtle GTK magic I
don't
> > see here?
> > 
> > > > 
> > > > This method also only creates a FirstRunDialog if
|show_reporting_dialog|
> so
> > > > that passing it in the constructor was superfluous.
> > > > 
> > > > first_run::CreateSentinel() is not required here, but I realize that
this
> is
> > > the
> > > > case in other places in the code and I'll fix that in a separate CL
> instead
> > of
> > > > squeezing it in here (added it back to this CL).
> > > 
> > 
> 
> Ah, I didn't see tge early return if (!show_reporting_dialog) slightly higher
> up. What threw me off is that FirstRunDialog::ShowReportingDialog() checks if
> (!show_reporting_dialog) too, but due to the early exit that branch will never
> be taken. +mnissler who wrote the lengthy-ish comment about this block of dead
> code, +jeanluc which added the early exit here in
> <crxref style="background-image:
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">https://codereview.chromium.org/3847006</crxref>
and +msw who touched it somewhat
> recently (but doesn't really look all that related:
> <crxref style="background-image:
url(chrome-extension://mamkkcngfoeihgpkjdkmmfajlamhifpl/res/type/codereview.png);">https://codereview.chromium.org/9285002</crxref>)

I suspect I wrote that comment back in a time when the check in line 126 wasn't
dead code (or at least I hope so). The new code looks fine from my side.

[commit-bot: I haz the power, 2013-05-07 13:04:29]

List of reviewers changed. mnissler@chromium.org did a drive-by without
LGTM'ing!

[Mattias Nissler (ping if slow), 2013-05-07 13:07:20]

On 2013/05/07 13:04:29, I haz the power (commit-bot) wrote:
> List of reviewers changed. mailto:mnissler@chromium.org did a drive-by without
> LGTM'ing!

Sorry, LGTM.

[commit-bot: I haz the power, 2013-05-07 13:07:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/14946003/84001

[commit-bot: I haz the power, 2013-05-07 13:14:50]

Change committed as 198707