Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(212)

Issue 223770043: Fix the tv.b.ui.createSelector() call in record_selection_dialog.html. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 1 month ago by nednguyen
Modified:
9 years, 1 month ago
Reviewers:
dsinclair, nduca
CC:
trace-viewer-review_googlegroups.com
Base URL:
https://github.com/google/trace-viewer.git@master
Visibility:
Public.

Description

Fix the tv.b.ui.createSelector() call in record_selection_dialog.html. Previously, I use tv.b.ui.createSelector(this, this.tracingRecordMode,..) which is non sense as the second parameter is supposed to be the targe property of |this|. I only discover this bug when actually trying to build chromium with the previous patch (https://codereview.appspot.com/221780043/). The error stack is: TypeError: Cannot read property 'selectedValue' of undefined at HTMLDivElement.tracingRecordMode (chrome://tracing/tracing.js:14006:41) at HTMLDivElement.RecordSelectionDialog.decorate (chrome://tracing/tracing.js:13951:21) at HTMLDivElement.f.decorate (chrome://tracing/tracing.js:13187:19) at HTMLDivElement.f (chrome://tracing/tracing.js:13177:18) at showTracingDialog (chrome://tracing/tracing.js:14476:22) This is because when this.tracingRecordMode is evaluated, this.tracingRecorModeSltr_ is not yet defined. The reason that no unittest was able to catch the bug was because all existing tests in record_selection_dialog_test have the non empty value of record_selection_dialog_preset. In this patch, I add a new test that tests RecordSelectionDialog's creation when there is no record_selection_dialog_preset. With this test, the previous patch fails, and pass after applying new changes in this test. BUG=https://github.com/google/trace-viewer/issues/842 R=nduca@chromium.org Committed: https://chromium.googlesource.com/external/trace-viewer/+/9a6f511c6724881f9055d662c1a473daa52f1b00

Patch Set 1 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+9 lines, -5 lines) Patch
M trace_viewer/extras/about_tracing/record_selection_dialog.html View 3 chunks +3 lines, -5 lines 0 comments Download
M trace_viewer/extras/about_tracing/record_selection_dialog_test.html View 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 3
nednguyen
9 years, 1 month ago (2015-03-27 05:03:54 UTC) #1
nduca
lgtm; as always, ned, you amaze me with your bughunting skillz.
9 years, 1 month ago (2015-03-27 17:33:56 UTC) #2
nednguyen
9 years, 1 month ago (2015-03-27 17:52:42 UTC) #3
Message was sent while issue was closed.
Committed patchset #1 (id:10007) manually as
9a6f511c6724881f9055d662c1a473daa52f1b00 (presubmit successful).
Sign in to reply to this message.

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