Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
360053003:
DevTools: Basic support of multiple targets for CPU profiler (Closed)
|
Created:
6 years, 5 months ago by sergeyv Modified:
6 years, 5 months ago CC:
blink-reviews, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, malch+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, sergeyv+blink_chromium.org, aandrey+blink_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionDevTools: Basic support of multiple targets for CPU profiler
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=177462
Patch Set 1 #
Total comments: 4
Patch Set 2 : Migrate to events #
Total comments: 4
Patch Set 3 : Address vsevik's comments #Patch Set 4 : Fix tests #
Messages
Total messages: 19 (0 generated)
https://chromiumcodereview.appspot.com/360053003/diff/1/Source/devtools/front... File Source/devtools/front_end/profiler/CPUProfileView.js (right): https://chromiumcodereview.appspot.com/360053003/diff/1/Source/devtools/front... Source/devtools/front_end/profiler/CPUProfileView.js:507: target.cpuProfilerModel.setDelegate(this); Let's use event listeners instead of delegate. https://chromiumcodereview.appspot.com/360053003/diff/1/Source/devtools/front... Source/devtools/front_end/profiler/CPUProfileView.js:647: if (!this._profileBeingRecorded || !this._profileBeingRecorded.target()) What will happen if the worker closes while we are recording profile without sending profile stopped event?
https://chromiumcodereview.appspot.com/360053003/diff/1/Source/devtools/front... File Source/devtools/front_end/profiler/CPUProfileView.js (right): https://chromiumcodereview.appspot.com/360053003/diff/1/Source/devtools/front... Source/devtools/front_end/profiler/CPUProfileView.js:507: target.cpuProfilerModel.setDelegate(this); Let's use event listeners instead of delegate. https://chromiumcodereview.appspot.com/360053003/diff/1/Source/devtools/front... Source/devtools/front_end/profiler/CPUProfileView.js:647: if (!this._profileBeingRecorded || !this._profileBeingRecorded.target()) What will happen if the worker closes while we are recording profile without sending profile stopped event?
https://codereview.chromium.org/360053003/diff/1/Source/devtools/front_end/pr... File Source/devtools/front_end/profiler/CPUProfileView.js (right): https://codereview.chromium.org/360053003/diff/1/Source/devtools/front_end/pr... Source/devtools/front_end/profiler/CPUProfileView.js:507: target.cpuProfilerModel.setDelegate(this); On 2014/07/01 08:48:54, vsevik wrote: > Let's use event listeners instead of delegate. Done. https://codereview.chromium.org/360053003/diff/1/Source/devtools/front_end/pr... Source/devtools/front_end/profiler/CPUProfileView.js:647: if (!this._profileBeingRecorded || !this._profileBeingRecorded.target()) On 2014/07/01 08:48:54, vsevik wrote: > What will happen if the worker closes while we are recording profile without > sending profile stopped event? In current behavior - nothing will happen, lets handle this case in the separate patch
lgtm https://chromiumcodereview.appspot.com/360053003/diff/20001/Source/devtools/f... File Source/devtools/front_end/profiler/CPUProfileView.js (right): https://chromiumcodereview.appspot.com/360053003/diff/20001/Source/devtools/f... Source/devtools/front_end/profiler/CPUProfileView.js:577: var data = /** @type {{protocolId: string, scriptLocation: !WebInspector.DebuggerModel.Location, cpuProfile: !ProfilerAgent.CPUProfile, title: (string|undefined)}} */ (event.data); Consider casting each field separately instead. https://chromiumcodereview.appspot.com/360053003/diff/20001/Source/devtools/f... Source/devtools/front_end/profiler/CPUProfileView.js:624: isRecordingProfile: function() I think this method and hence the field is never used.
https://codereview.chromium.org/360053003/diff/20001/Source/devtools/front_en... File Source/devtools/front_end/profiler/CPUProfileView.js (right): https://codereview.chromium.org/360053003/diff/20001/Source/devtools/front_en... Source/devtools/front_end/profiler/CPUProfileView.js:577: var data = /** @type {{protocolId: string, scriptLocation: !WebInspector.DebuggerModel.Location, cpuProfile: !ProfilerAgent.CPUProfile, title: (string|undefined)}} */ (event.data); On 2014/07/02 09:08:49, vsevik wrote: > Consider casting each field separately instead. Done. https://codereview.chromium.org/360053003/diff/20001/Source/devtools/front_en... Source/devtools/front_end/profiler/CPUProfileView.js:624: isRecordingProfile: function() On 2014/07/02 09:08:49, vsevik wrote: > I think this method and hence the field is never used. Done. Method is indeed unused, but the field is used
The CQ bit was checked by sergeyv@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyv@chromium.org/360053003/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_blink_dbg on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...) linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...) mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/13943) win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/15055)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
The CQ bit was checked by sergeyv@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyv@chromium.org/360053003/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/15074)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/15092)
The CQ bit was checked by sergeyv@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sergeyv@chromium.org/360053003/60001
Message was sent while issue was closed.
Change committed as 177462 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
