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

Issue 8342069: Add unit tests for about:tracing TimelineModel. (Closed)

Created:
9 years, 2 months ago by nduca
Modified:
9 years, 2 months ago
CC:
chromium-reviews, arv (Not doing code reviews)
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Add unit tests for about:tracing TimelineModel. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106842

Patch Set 1 #

Patch Set 2 : Try using webui style #

Patch Set 3 : Focus on timeline_model test for now. #

Patch Set 4 : Just use jstest for now. #

Total comments: 11

Patch Set 5 : . #

Unified diffs Side-by-side diffs Delta from patch set Stats (+343 lines, -17 lines) Patch
M chrome/browser/resources/tracing/timeline_model.js View 1 2 3 4 12 chunks +45 lines, -17 lines 0 comments Download
A chrome/browser/resources/tracing/timeline_model_test.html View 1 2 3 4 1 chunk +298 lines, -0 lines 0 comments Download

Messages

Total messages: 5 (0 generated)
nduca
Quick one. Talked to scr@ about getting these tests running as part of bots. Will ...
9 years, 2 months ago (2011-10-21 05:37:52 UTC) #1
jbates
On 2011/10/21 05:37:52, nduca wrote: > Quick one. Talked to scr@ about getting these tests ...
9 years, 2 months ago (2011-10-21 16:52:21 UTC) #2
arv (Not doing code reviews)
http://codereview.chromium.org/8342069/diff/5001/chrome/browser/resources/tracing/timeline_model.js File chrome/browser/resources/tracing/timeline_model.js (right): http://codereview.chromium.org/8342069/diff/5001/chrome/browser/resources/tracing/timeline_model.js#newcode157 chrome/browser/resources/tracing/timeline_model.js:157: for (var p in this.threads) Missing {} http://codereview.chromium.org/8342069/diff/5001/chrome/browser/resources/tracing/timeline_model.js#newcode174 chrome/browser/resources/tracing/timeline_model.js:174: ...
9 years, 2 months ago (2011-10-21 17:09:18 UTC) #3
nduca
http://codereview.chromium.org/8342069/diff/5001/chrome/browser/resources/tracing/timeline_model.js File chrome/browser/resources/tracing/timeline_model.js (right): http://codereview.chromium.org/8342069/diff/5001/chrome/browser/resources/tracing/timeline_model.js#newcode157 chrome/browser/resources/tracing/timeline_model.js:157: for (var p in this.threads) On 2011/10/21 17:09:18, arv ...
9 years, 2 months ago (2011-10-21 19:50:40 UTC) #4
arv (Not doing code reviews)
9 years, 2 months ago (2011-10-21 23:43:53 UTC) #5
lgtm

http://codereview.chromium.org/8342069/diff/5001/chrome/browser/resources/tra...
File chrome/browser/resources/tracing/timeline_model.js (right):

http://codereview.chromium.org/8342069/diff/5001/chrome/browser/resources/tra...
chrome/browser/resources/tracing/timeline_model.js:174: function
getStringHash(name) {
On 2011/10/21 19:50:40, nduca wrote:
> On 2011/10/21 17:09:18, arv wrote:
> > This looks like it might be overkill for choosing colors.
> 
> Do you have any suggestions? The goal is to get a hash for a string that is
> fairly random but also stable across computers --- this way "that red task"
> shows up as red on your and my computer. Tracing has been using this one for a
> few months and it seems good-ish.

I don't have any better idea. Are the strings short? If not maybe limit the
number of chars you go through.

Powered by Google App Engine
This is Rietveld 408576698