Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(7)

Issue 3013013002: [pinpoint] Change refactor. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 week ago by dtu
Modified:
5 days, 18 hours ago
Reviewers:
perezju, shatch, sullivan
CC:
catapult-reviews_chromium.org, perf-dashboard-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
catapult
Visibility:
Public.

Description

This patch is trying to accomplish two major objectives: * Make Change.deps (now Change.commits) ordered. In the case of multiple DEPS rolls, e.g. clank@1 chromium@2 v8@3, we need them in order to know that v8@3 is the one to display in the UI. * Simplify and correct the Change.Midpoint() logic. It was incorrect in the case of change_a == chromium@1, change_b == chromium@1 v8@4. It was giving no midpoint, when we expected chromium@1 v8@2. Hopefully the logic is easier to follow now. In addition, I've tacked on some other things that were on my wishlist: * Rename Dep to Commit. * Split change.py into separate modules for Change, Commit, and Patch. * Make the Change.Midpoint() unit tests much easier to grok and write. Review-Url: https://chromiumcodereview.appspot.com/3013013002 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/5638c631c865e13614c71c6b68bc5a1f69806742

Patch Set 1 #

Patch Set 2 : . #

Total comments: 21

Patch Set 3 : Comments #

Patch Set 4 : Comment #

Patch Set 5 : UI #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+874 lines, -812 lines) Patch
M dashboard/dashboard/pinpoint/elements/change-info.html View 1 2 3 4 4 chunks +19 lines, -32 lines 0 comments Download
M dashboard/dashboard/pinpoint/handlers/isolate_test.py View 7 chunks +8 lines, -8 lines 0 comments Download
M dashboard/dashboard/pinpoint/handlers/new.py View 1 chunk +4 lines, -4 lines 0 comments Download
D dashboard/dashboard/pinpoint/models/change.py View 1 chunk +0 lines, -334 lines 0 comments Download
A dashboard/dashboard/pinpoint/models/change/__init__.py View 1 chunk +8 lines, -0 lines 0 comments Download
A dashboard/dashboard/pinpoint/models/change/change.py View 1 2 3 1 chunk +212 lines, -0 lines 4 comments Download
A dashboard/dashboard/pinpoint/models/change/change_test.py View 1 2 1 chunk +209 lines, -0 lines 0 comments Download
A dashboard/dashboard/pinpoint/models/change/commit.py View 1 2 1 chunk +166 lines, -0 lines 0 comments Download
A dashboard/dashboard/pinpoint/models/change/commit_test.py View 1 2 1 chunk +154 lines, -0 lines 0 comments Download
A dashboard/dashboard/pinpoint/models/change/patch.py View 1 chunk +26 lines, -0 lines 0 comments Download
A dashboard/dashboard/pinpoint/models/change/patch_test.py View 1 chunk +39 lines, -0 lines 0 comments Download
D dashboard/dashboard/pinpoint/models/change_test.py View 1 chunk +0 lines, -405 lines 0 comments Download
M dashboard/dashboard/pinpoint/models/isolate_test.py View 1 chunk +2 lines, -2 lines 0 comments Download
M dashboard/dashboard/pinpoint/models/job.py View 1 2 2 chunks +14 lines, -15 lines 0 comments Download
M dashboard/dashboard/pinpoint/models/quest/find_isolate_test.py View 8 chunks +13 lines, -12 lines 0 comments Download
Commit queue not available (can’t edit this change).

Messages

Total messages: 15 (6 generated)
dtu
Making `deps` ordered is a breaking, incompatible change. I've tacked on a bunch of other ...
1 week ago (2017-09-12 08:03:16 UTC) #4
perezju
Added a few comments for now; will probably have a second pass after any changes ...
1 week ago (2017-09-12 10:59:55 UTC) #5
dtu
https://codereview.chromium.org/3013013002/diff/20001/dashboard/dashboard/pinpoint/models/change/change.py File dashboard/dashboard/pinpoint/models/change/change.py (right): https://codereview.chromium.org/3013013002/diff/20001/dashboard/dashboard/pinpoint/models/change/change.py#newcode37 dashboard/dashboard/pinpoint/models/change/change.py:37: def id_string(self): On 2017/09/12 10:59:54, perezju wrote: > add ...
6 days, 13 hours ago (2017-09-12 22:31:16 UTC) #6
shatch
lgtm once perezu is happy with it
6 days, 10 hours ago (2017-09-13 00:53:37 UTC) #7
perezju
I'm going to rs-lgtm this mostly so you that can land the refactor and don't ...
5 days, 22 hours ago (2017-09-13 12:52:24 UTC) #8
dtu
https://codereview.chromium.org/3013013002/diff/20001/dashboard/dashboard/pinpoint/models/change/change.py File dashboard/dashboard/pinpoint/models/change/change.py (right): https://codereview.chromium.org/3013013002/diff/20001/dashboard/dashboard/pinpoint/models/change/change.py#newcode139 dashboard/dashboard/pinpoint/models/change/change.py:139: for commit_a in commits_a: On 2017/09/13 12:52:23, perezju wrote: ...
5 days, 20 hours ago (2017-09-13 15:49:26 UTC) #9
dtu
For another complex case where you would need to modify the list as you're iterating ...
5 days, 19 hours ago (2017-09-13 15:56:28 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/3013013002/80001
5 days, 18 hours ago (2017-09-13 16:59:27 UTC) #12
commit-bot: I haz the power
5 days, 18 hours ago (2017-09-13 17:33:27 UTC) #15
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld b40b6558b