Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(31)

Issue 8672009: Fix the merging of threadnames with multiple digits on about:profiler. (Closed)

Created:
9 years, 1 month ago by eroman
Modified:
9 years, 1 month ago
CC:
chromium-reviews, arv (Not doing code reviews)
Visibility:
Public.

Description

Fix the merging of threadnames with multiple digits on about:profiler. ... for real this time! Problem is the first (.*) is greedy, so it gobbles up all but the last digit. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=111398

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -1 line) Patch
M chrome/browser/resources/profiler.js View 1 chunk +1 line, -1 line 1 comment Download

Messages

Total messages: 3 (0 generated)
eroman
9 years, 1 month ago (2011-11-23 03:22:57 UTC) #1
jar (doing other things)
See comment/suggestion below. If you really liked it the way it is, then LGTM. If ...
9 years, 1 month ago (2011-11-23 16:45:48 UTC) #2
eroman
9 years, 1 month ago (2011-11-23 18:52:47 UTC) #3
Thanks, you are absolutely right.

Grr, I can't seem to get my regular expressions straight!

Ok, I will test this some more before committing with your fix.

On Wed, Nov 23, 2011 at 8:45 AM, <jar@chromium.org> wrote:

> See comment/suggestion below.  If you really liked it the way it is, then
> LGTM.
> If you change it then LGTM ;-)
>
>
> http://codereview.chromium.**org/8672009/diff/1/chrome/**
>
browser/resources/profiler.js<http://codereview.chromium.org/8672009/diff/1/chrome/browser/resources/profiler.js>
> File chrome/browser/resources/**profiler.js (right):
>
> http://codereview.chromium.**org/8672009/diff/1/chrome/**
>
browser/resources/profiler.js#**newcode881<http://codereview.chromium.org/8672009/diff/1/chrome/browser/resources/profiler.js#newcode881>
> chrome/browser/resources/**profiler.js:881: var m =
>
> /^(.*)[^\d](\d+)$/.exec(value)**;
> I think you discard the last character of the non-numeric prefix.  I
> guess you might assume it is a separator, like a "-" or a space.... but
> if you processed:
> Thread23
>
> you'd end up with
> Threa*
>
> (after you concatenation in line 883).
>
> I think it might be better to say:
>
> var m = /^(.*[^\d])(\d+)$/.exec(value)**;
>
> With the paren pushed after the non-digit single-match.
>
>
http://codereview.chromium.**org/8672009/<http://codereview.chromium.org/8672...
>

Powered by Google App Engine
This is Rietveld 408576698