Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
104523002:
[DevTools] Add power profiler and power overview in timeline panel. (Closed)
|
Created:
7 years ago by Pan Modified:
6 years, 9 months ago CC:
blink-reviews, jamesr, caseq+blink_chromium.org, loislo+blink_chromium.org, eustas+blink_chromium.org, alph+blink_chromium.org, yurys+blink_chromium.org, lushnikov+blink_chromium.org, abarth-chromium, vsevik+blink_chromium.org, pfeldman+blink_chromium.org, dglazkov+blink, paulirish+reviews_chromium.org, apavlov+blink_chromium.org, Inactive, devtools-reviews_chromium.org, arv+blink, aandrey+blink_chromium.org, watchdog-blink-watchlist_google.com Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionThis CL add power profiler in DevTools frontend, and draw power data in PowerOverview of Timeline panel.
BUG=337138
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=169951
Patch Set 1 #
Total comments: 20
Patch Set 2 : #Patch Set 3 : rebase #
Total comments: 12
Patch Set 4 : #
Total comments: 25
Patch Set 5 : #
Total comments: 6
Patch Set 6 : #Patch Set 7 : rebase #Messages
Total messages: 27 (0 generated)
https://codereview.chromium.org/104523002/diff/1/Source/core/inspector/Inspec... File Source/core/inspector/InspectorFrontendHost.idl (right): https://codereview.chromium.org/104523002/diff/1/Source/core/inspector/Inspec... Source/core/inspector/InspectorFrontendHost.idl:61: void checkPowerProfileSupportedStatus(); These should not be defined in the front-end embedder interface, but rather be exposed as a part of the remote debugging protocol. The rationale is that you should query power data from the target device, not necessarily the device that renders the front-end. You could either declare a new domain "Power" in the protocol.json for that or make power data sent along with the timeline records. https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Se... File Source/devtools/front_end/Settings.js (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Se... Source/devtools/front_end/Settings.js:274: this.powerProfile = this._createExperiment("powerProfile", "Enable power profile on timeline"); We now try to create settings locally, where they are used. In timeline in this case. https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Se... File Source/devtools/front_end/SettingsScreen.js (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Se... Source/devtools/front_end/SettingsScreen.js:705: if (experiment.name === "powerProfile") { This seems wrong - generic method should not be concrete experiment-aware. https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Se... Source/devtools/front_end/SettingsScreen.js:713: WebInspector.powerProfiler.checkStatus(); You should allow enabling this experiment unconditionally. https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Ti... File Source/devtools/front_end/TimelineFrameController.js (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Ti... Source/devtools/front_end/TimelineFrameController.js:84: if (record.type === WebInspector.TimelineModel.RecordType.SoC_Package) You should not skip records of any kind. https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Ti... File Source/devtools/front_end/TimelineModel.js (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Ti... Source/devtools/front_end/TimelineModel.js:45: WebInspector.powerProfiler.addEventListener(WebInspector.PowerProfiler.EventTypes.PowerEventRecorded, this._onPowerEventAdded, this); Timeline model should not depend on any of the profilers. Instead, you should tell timeline to collect power data as one of the Timeline.start paarmeters. Then it will be sending this power data as one of the "data" auxiliary properties. https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Ti... File Source/devtools/front_end/TimelineOverviewPane.js (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Ti... Source/devtools/front_end/TimelineOverviewPane.js:1053: WebInspector.TimelinePowerOverview = function(model, windowStartTime, windowEndTime) This change probably needs merging. Could you rebaseline this overview? We will then figure out how to deal with the new mode. https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Ti... File Source/devtools/front_end/TimelinePanel.js (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Ti... Source/devtools/front_end/TimelinePanel.js:156: this._timelineGrid.gridHeaderElement.addStyleClass("with-power"); No hacks like this please. https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/in... File Source/devtools/front_end/inspector.html (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/in... Source/devtools/front_end/inspector.html:87: <script type="text/javascript" src="PowerProfiler.js"></script> I don't think you will need this class. https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/in... File Source/devtools/front_end/inspector.js (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/in... Source/devtools/front_end/inspector.js:389: this.powerProfiler = new WebInspector.PowerProfiler(); Search for the following code and query for power capability behind the experiment instead. PageAgent.canScreencast(WebInspector._initializeCapability.bind(WebInspector, "canScreencast", null)); https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/ti... File Source/devtools/front_end/timelinePanel.css (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/ti... Source/devtools/front_end/timelinePanel.css:477: -webkit-mask-position: -291px -95px; Note that this image has an svg source in Images/src. You should modify the source with Inkscape, rasterise images and optimize them using tools/resources/optimize-png-files.sh https://codereview.chromium.org/104523002/diff/1/Source/devtools/protocol.json File Source/devtools/protocol.json (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/protocol.jso... Source/devtools/protocol.json:4067: "domain": "Power", Oh, ok, so we have this new domain. I think it should be a part of the timeline though.
Pan: What is the status of those CLs? Are they actively worked on?
On 2014/01/16 17:16:31, qsr wrote: > Pan: What is the status of those CLs? Are they actively worked on? yep, I'm working on it, it should be updated next week. thanks Pan
https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Ti... File Source/devtools/front_end/TimelineModel.js (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Ti... Source/devtools/front_end/TimelineModel.js:45: WebInspector.powerProfiler.addEventListener(WebInspector.PowerProfiler.EventTypes.PowerEventRecorded, this._onPowerEventAdded, this); On 2014/01/09 11:38:52, pfeldman wrote: > Timeline model should not depend on any of the profilers. Instead, you should > tell timeline to collect power data as one of the Timeline.start paarmeters. > Then it will be sending this power data as one of the "data" auxiliary > properties. Hi @pfeldman, @qsr, I'm thinking the first solution is to define a Power domain with commands "start/stop" and events "dataAvailable", this will make TimelineModel(or TimelineManager) depend on Power. second solution is, we add a parameter to Timeline.start at both front-end and InspectorTimelineAgent, while, the place to capture paremeter and take action should be content/browser/devtools/DevToolsPowerHandler, InspectorTimelineAgent have to, it is a little weird. I guess there may be a need to collect power data independently (without other timeline events), given that, I prefer first solution a little bit, suggestions? thanks! Pan
What's up with Source/web/InspectorClientImpl.cpp file? https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Po... File Source/devtools/front_end/PowerProfiler.js (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Po... Source/devtools/front_end/PowerProfiler.js:2: * Copyright (C) 2013 Intel Inc. All rights reserved. There's a new much shorter copyright header.
Hi @pfeldman, @alph, The code has been rebased, please review thanks for your help! Pan https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Po... File Source/devtools/front_end/PowerProfiler.js (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Po... Source/devtools/front_end/PowerProfiler.js:2: * Copyright (C) 2013 Intel Inc. All rights reserved. On 2014/02/13 11:59:47, alph wrote: > There's a new much shorter copyright header. thanks, the shorter one introduced :) https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Ti... File Source/devtools/front_end/TimelineFrameController.js (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Ti... Source/devtools/front_end/TimelineFrameController.js:84: if (record.type === WebInspector.TimelineModel.RecordType.SoC_Package) On 2014/01/09 11:38:52, pfeldman wrote: > You should not skip records of any kind. removed this change https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Ti... File Source/devtools/front_end/TimelineOverviewPane.js (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/Ti... Source/devtools/front_end/TimelineOverviewPane.js:1053: WebInspector.TimelinePowerOverview = function(model, windowStartTime, windowEndTime) On 2014/01/09 11:38:52, pfeldman wrote: > This change probably needs merging. Could you rebaseline this overview? We will > then figure out how to deal with the new mode. rebased, thanks! https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/in... File Source/devtools/front_end/inspector.html (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/in... Source/devtools/front_end/inspector.html:87: <script type="text/javascript" src="PowerProfiler.js"></script> On 2014/01/09 11:38:52, pfeldman wrote: > I don't think you will need this class. I tried to remove this, but seems front-end won't show up fully without it. https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/in... File Source/devtools/front_end/inspector.js (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/in... Source/devtools/front_end/inspector.js:389: this.powerProfiler = new WebInspector.PowerProfiler(); On 2014/01/09 11:38:52, pfeldman wrote: > Search for the following code and query for power capability behind the > experiment instead. > > PageAgent.canScreencast(WebInspector._initializeCapability.bind(WebInspector, > "canScreencast", null)); thanks for your guide, done. https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/ti... File Source/devtools/front_end/timelinePanel.css (right): https://codereview.chromium.org/104523002/diff/1/Source/devtools/front_end/ti... Source/devtools/front_end/timelinePanel.css:477: -webkit-mask-position: -291px -95px; On 2014/01/09 11:38:52, pfeldman wrote: > Note that this image has an svg source in Images/src. You should modify the > source with Inkscape, rasterise images and optimize them using > tools/resources/optimize-png-files.sh done, thanks!
Hi @pfeldman, @alph This cl is rebased, could you please review it :) The svg and image changes is not included, I'd like to upload in another issue due to it is easy to conflict with other CLs. thanks Pan
https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... File Source/devtools/front_end/PowerProfiler.js (right): https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... Source/devtools/front_end/PowerProfiler.js:50: this._profiler.dispatchEventToListeners(WebInspector.PowerProfiler.EventTypes.PowerEventRecorded, events[i]); It is better to repack data here instead of renaming timestamp to startTime. I.e. start with a brand new object of a new type and copy the properties you need. Or use the original payload type (It has a type {ProwerAgent.PowerEvent}). But not something in the middle. https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... File Source/devtools/front_end/Settings.js (right): https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... Source/devtools/front_end/Settings.js:292: this.powerTimeline = this._createExperiment("powerProfile", "Enable power mode in Timeline"); Name of the property should match the name of the setting. powerProfiler ? https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... File Source/devtools/front_end/TimelineModel.js (right): https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... Source/devtools/front_end/TimelineModel.js:45: if (WebInspector.experimentsSettings.powerTimeline.isEnabled() && Capabilities.canProfilePower) Timeline model is about timeline events. You should not mix the power events into the timeline event flow. I.e. you should not change this class at all. https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... File Source/devtools/front_end/TimelineOverviewPane.js (right): https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... Source/devtools/front_end/TimelineOverviewPane.js:41: if (WebInspector.experimentsSettings.powerTimeline.isEnabled() && Capabilities.canProfilePower) You should not modify this either. I think it is best to create yet another overview type with the power profile data. Unfortunately, this part of code is currently under massive refactoring. https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... File Source/devtools/front_end/TimelinePowerOverview.js (right): https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... Source/devtools/front_end/TimelinePowerOverview.js:7: * @extends {WebInspector.TimelineOverviewBase} For now, you can add startTimeline and stopTimeline methods into TimelineOverviewBase. That's where you'll start and stop your agent separately from the rest of the timeline. https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... Source/devtools/front_end/TimelinePowerOverview.js:10: WebInspector.TimelinePowerOverview = function(model) Yes, perfect. You should have your own overview type. https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... Source/devtools/front_end/TimelinePowerOverview.js:25: windowChanged: function(startTime, endTime) Please annotate every method. https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... Source/devtools/front_end/TimelinePowerOverview.js:34: this._restorePowerMarker(); Lets think if we can unify this with memory graph or cpu flame chart overview? We seem to be craeting too many different canvases.
https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... File Source/devtools/front_end/TimelinePowerOverview.js (right): https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... Source/devtools/front_end/TimelinePowerOverview.js:34: this._restorePowerMarker(); On 2014/03/11 12:24:02, pfeldman wrote: > Lets think if we can unify this with memory graph or cpu flame chart overview? > We seem to be craeting too many different canvases. Hi @pfeldman, thanks for your help, do you mean it is better to use html node to render the marks(like the point in memory graph) rather than draw them the canvas? Pan
On 2014/03/12 13:11:20, Pan wrote: > https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... > File Source/devtools/front_end/TimelinePowerOverview.js (right): > > https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... > Source/devtools/front_end/TimelinePowerOverview.js:34: > this._restorePowerMarker(); > On 2014/03/11 12:24:02, pfeldman wrote: > > Lets think if we can unify this with memory graph or cpu flame chart overview? > > We seem to be craeting too many different canvases. > > Hi @pfeldman, > thanks for your help, do you mean it is better to use html node to render the > marks(like the point in memory graph) rather than draw them the canvas? > No-no, the canvas is the way to go. I was just saying that we already have too much code rendering generic graphs on canvases and it would be great to reuse one of them. > Pan
On 2014/03/13 05:49:07, pfeldman wrote: > On 2014/03/12 13:11:20, Pan wrote: > > > https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... > > File Source/devtools/front_end/TimelinePowerOverview.js (right): > > > > > https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... > > Source/devtools/front_end/TimelinePowerOverview.js:34: > > this._restorePowerMarker(); > > On 2014/03/11 12:24:02, pfeldman wrote: > > > Lets think if we can unify this with memory graph or cpu flame chart > overview? > > > We seem to be craeting too many different canvases. > > > > Hi @pfeldman, > > thanks for your help, do you mean it is better to use html node to render the > > marks(like the point in memory graph) rather than draw them the canvas? > > > > No-no, the canvas is the way to go. I was just saying that we already have too > much code rendering generic graphs on canvases and it would be great to reuse > one of them. > > > Pan In this CL, drawPowerMark draw a point along power curve, and show a power value; drawEnergyMark show a energy consumption value in selected window, both of them are draw by canvas. But I find both the moving point in memory graph and highlight rect in Flamechart are using node. So I think either adopt node (span, div) here, or keep current canvas way, right? thanks Pan
Reusing memory graph would give you the moving dot, could you try that? We will layer decorate the graph with power specifics.
Hi @pfeldman, Thanks for your help, I abstracted Counter and CounterUIBase in https://codereview.chromium.org/196523006/. Now this CL is based on 196523006. In this one, only power graphs is drawing, the moving dot and power value is provided, while energy consumption is not provided yet, planning to achieve it in next CL. could you please have a look at it? both this one and 196523006. thanks Pan https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... File Source/devtools/front_end/PowerProfiler.js (right): https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... Source/devtools/front_end/PowerProfiler.js:50: this._profiler.dispatchEventToListeners(WebInspector.PowerProfiler.EventTypes.PowerEventRecorded, events[i]); On 2014/03/11 12:24:02, pfeldman wrote: > It is better to repack data here instead of renaming timestamp to startTime. > I.e. start with a brand new object of a new type and copy the properties you > need. Or use the original payload type (It has a type {ProwerAgent.PowerEvent}). > But not something in the middle. Done. https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... File Source/devtools/front_end/Settings.js (right): https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... Source/devtools/front_end/Settings.js:292: this.powerTimeline = this._createExperiment("powerProfile", "Enable power mode in Timeline"); On 2014/03/11 12:24:02, pfeldman wrote: > Name of the property should match the name of the setting. powerProfiler ? Done. https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... File Source/devtools/front_end/TimelinePowerOverview.js (right): https://codereview.chromium.org/104523002/diff/260001/Source/devtools/front_e... Source/devtools/front_end/TimelinePowerOverview.js:7: * @extends {WebInspector.TimelineOverviewBase} On 2014/03/11 12:24:02, pfeldman wrote: > For now, you can add > > startTimeline and stopTimeline methods into TimelineOverviewBase. That's where > you'll start and stop your agent separately from the rest of the timeline. Done.
https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... File Source/devtools/front_end/Target.js (right): https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/Target.js:20: this.powerAgent().canProfilePower(this._initializeCapability.bind(this, "canProfilePower", null)); This should be queried behind the experiment check. https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... File Source/devtools/front_end/TimelineOverviewPane.js (right): https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/TimelineOverviewPane.js:42: this.element.classList.add("power-overview"); You should not modify the overview, not even when the power profiler is there. https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/TimelineOverviewPane.js:160: this._overviewControl.windowChanged(windowTimes.startTime, windowTimes.endTime); Overview control should not be concerned about the time range selected. https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/TimelineOverviewPane.js:323: startTimeline: function() { }, These should be called timelineStarted and timelineStopped https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/TimelineOverviewPane.js:326: windowChanged: function(timeLeft, timeRight) { }, Overview should not be concerned with the selected range change. https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... File Source/devtools/front_end/TimelinePanel.js (right): https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/TimelinePanel.js:302: mode === WebInspector.TimelinePanel.Mode.Power && (!WebInspector.experimentsSettings.powerProfiler.isEnabled() || !Capabilities.canProfilePower)) No need to check for experiment here since you only query for capability behind experiment. https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/TimelinePanel.js:562: this._viewsForMode(WebInspector.TimelinePanel.Mode.Power).overviewView.startTimeline(); this._currentViews.overviewView.timelineStarted(); https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/TimelinePanel.js:571: this._viewsForMode(WebInspector.TimelinePanel.Mode.Power).overviewView.stopTimeline(); ...Stopped https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... File Source/devtools/front_end/TimelinePowerOverview.js (right): https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/TimelinePowerOverview.js:35: * @extends {WebInspector.CounterUIBase} We should figure out whether we want a) power overview, b) power section (like the memory) or both. It seems like both would work best. So we need to have: 1) TimelinePowerOverview (that would be a clone of TimelineMemoryOverview, feel free to reuse code from there). 2) TimelinePowerGraph (that would be a clone of CountersGraph) for the bottom part of the screen. At this point, I don't understand how MemoryStatistics and CountersGraph are separated. Let me try to improve their relationship. https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... File Source/devtools/front_end/timelinePanel.css (right): https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/timelinePanel.css:36: #timeline-overview-panel.power-overview { You should not adjust timeline styles. https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/timelinePanel.css:96: #timeline-overview-grid.power-overview #resources-graphs { ditto https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/timelinePanel.css:829: .timeline-power-marker { ditto
https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... File Source/devtools/front_end/TimelineOverviewPane.js (right): https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/TimelineOverviewPane.js:326: windowChanged: function(timeLeft, timeRight) { }, On 2014/03/24 13:41:25, pfeldman wrote: > Overview should not be concerned with the selected range change. The intention is, I'd like to draw the energy consumption on the panel when users choose a time window in the PowerOverviewPane. is there other approach to achieve that? https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... File Source/devtools/front_end/TimelinePowerOverview.js (right): https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/TimelinePowerOverview.js:35: * @extends {WebInspector.CounterUIBase} On 2014/03/24 13:41:25, pfeldman wrote: > We should figure out whether we want a) power overview, b) power section (like > the memory) or both. It seems like both would work best. > > So we need to have: > > 1) TimelinePowerOverview (that would be a clone of TimelineMemoryOverview, feel > free to reuse code from there). > > 2) TimelinePowerGraph (that would be a clone of CountersGraph) for the bottom > part of the screen. > > At this point, I don't understand how MemoryStatistics and CountersGraph are > separated. Let me try to improve their relationship. Previously, I tried to reuse some code from a memory counters graph, while, to draw TimelinePowerOverview, that's why I extracted a pure CounterUIBase without left "swatch". My original usage definition is: a: A moving dot, and tell instant power value. b: a selected time window, to tell energy consumption during that period. both a and b are supported in the timelineOverviewPanel. Do you mean we need both TimelinePowerOverview and TimelinePowerGraph? If we have both of them, refer to memory mode, I guess you mean: c: TimelinePowerOverview provides the power curves. without instant power value and energy consumption value, d: user choose a time window in TimelinePowerOverview, TimelinePowerGraph(at the same place of memory graph) shows the zoomed in window, and provide energy consumption value in that time window. e: TimelinePowerGraph allows a moving dot to tell the instant power. right? thanks! Pan
Exactly. That way we are most consistent with memory and can aim for the most code reuse.
On 2014/03/25 06:50:06, pfeldman wrote: > Exactly. That way we are most consistent with memory and can aim for the most > code reuse. thanks, let me prepare patches. Pan
Hi @pfeldman, Now,TimelinePowerOverview is just a clone from TimelineMemoryOverview, differences are: 1) memory data from timeline model, power data from its own provider. 2) graph styles. Further step, I think we can extract something like HistogramUI(contains histogram data, and graph styles), make it live in Memory and Power overview. any better solution? thanks Pan https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... File Source/devtools/front_end/Target.js (right): https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/Target.js:20: this.powerAgent().canProfilePower(this._initializeCapability.bind(this, "canProfilePower", null)); On 2014/03/24 13:41:25, pfeldman wrote: > This should be queried behind the experiment check. thanks, done https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... File Source/devtools/front_end/TimelineOverviewPane.js (right): https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/TimelineOverviewPane.js:42: this.element.classList.add("power-overview"); On 2014/03/24 13:41:25, pfeldman wrote: > You should not modify the overview, not even when the power profiler is there. removed. https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/TimelineOverviewPane.js:160: this._overviewControl.windowChanged(windowTimes.startTime, windowTimes.endTime); On 2014/03/24 13:41:25, pfeldman wrote: > Overview control should not be concerned about the time range selected. Done. https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/TimelineOverviewPane.js:323: startTimeline: function() { }, On 2014/03/24 13:41:25, pfeldman wrote: > These should be called timelineStarted and timelineStopped Done. https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/TimelineOverviewPane.js:326: windowChanged: function(timeLeft, timeRight) { }, On 2014/03/24 13:41:25, pfeldman wrote: > Overview should not be concerned with the selected range change. Done. https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... File Source/devtools/front_end/TimelinePanel.js (right): https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/TimelinePanel.js:302: mode === WebInspector.TimelinePanel.Mode.Power && (!WebInspector.experimentsSettings.powerProfiler.isEnabled() || !Capabilities.canProfilePower)) On 2014/03/24 13:41:25, pfeldman wrote: > No need to check for experiment here since you only query for capability behind > experiment. Done. https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/TimelinePanel.js:562: this._viewsForMode(WebInspector.TimelinePanel.Mode.Power).overviewView.startTimeline(); On 2014/03/24 13:41:25, pfeldman wrote: > this._currentViews.overviewView.timelineStarted(); Done. https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/TimelinePanel.js:571: this._viewsForMode(WebInspector.TimelinePanel.Mode.Power).overviewView.stopTimeline(); On 2014/03/24 13:41:25, pfeldman wrote: > ...Stopped Done. https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... File Source/devtools/front_end/timelinePanel.css (right): https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/timelinePanel.css:36: #timeline-overview-panel.power-overview { On 2014/03/24 13:41:25, pfeldman wrote: > You should not adjust timeline styles. removed https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/timelinePanel.css:96: #timeline-overview-grid.power-overview #resources-graphs { On 2014/03/24 13:41:25, pfeldman wrote: > ditto removed https://codereview.chromium.org/104523002/diff/280001/Source/devtools/front_e... Source/devtools/front_end/timelinePanel.css:829: .timeline-power-marker { On 2014/03/24 13:41:25, pfeldman wrote: > ditto removed
lgtm with nits https://codereview.chromium.org/104523002/diff/300001/Source/devtools/front_e... File Source/devtools/front_end/TimelinePanel.js (right): https://codereview.chromium.org/104523002/diff/300001/Source/devtools/front_e... Source/devtools/front_end/TimelinePanel.js:271: views.mainViews = [this._timelineView()]; and you will need another view here that can come in a separate change, right? https://codereview.chromium.org/104523002/diff/300001/Source/devtools/front_e... Source/devtools/front_end/TimelinePanel.js:565: this._viewsForMode(WebInspector.TimelinePanel.Mode.Power).overviewView.timelineStarted(); You should not play favorites here and should send it out to all the overviews available. https://codereview.chromium.org/104523002/diff/300001/Source/devtools/front_e... File Source/devtools/front_end/TimelinePowerOverview.js (right): https://codereview.chromium.org/104523002/diff/300001/Source/devtools/front_e... Source/devtools/front_end/TimelinePowerOverview.js:104: var histogram = new Array(width); Yes, we will want to extract the histogram graph. You can do it in this change or in a follow up. You can call it HistogramGraph.
https://codereview.chromium.org/104523002/diff/300001/Source/devtools/front_e... File Source/devtools/front_end/TimelinePanel.js (right): https://codereview.chromium.org/104523002/diff/300001/Source/devtools/front_e... Source/devtools/front_end/TimelinePanel.js:271: views.mainViews = [this._timelineView()]; On 2014/03/25 13:00:51, pfeldman wrote: > and you will need another view here that can come in a separate change, right? Yes, I'd like it in another change. https://codereview.chromium.org/104523002/diff/300001/Source/devtools/front_e... Source/devtools/front_end/TimelinePanel.js:565: this._viewsForMode(WebInspector.TimelinePanel.Mode.Power).overviewView.timelineStarted(); On 2014/03/25 13:00:51, pfeldman wrote: > You should not play favorites here and should send it out to all the overviews > available. thanks, done https://codereview.chromium.org/104523002/diff/300001/Source/devtools/front_e... File Source/devtools/front_end/TimelinePowerOverview.js (right): https://codereview.chromium.org/104523002/diff/300001/Source/devtools/front_e... Source/devtools/front_end/TimelinePowerOverview.js:104: var histogram = new Array(width); On 2014/03/25 13:00:51, pfeldman wrote: > Yes, we will want to extract the histogram graph. You can do it in this change > or in a follow up. You can call it HistogramGraph. thanks, plan it in another change.
The CQ bit was checked by pan.deng@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pan.deng@intel.com/104523002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for Source/devtools/front_end/Settings.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/devtools/front_end/Settings.js
Hunk #2 FAILED at 289.
1 out of 2 hunks FAILED -- saving rejects to file
Source/devtools/front_end/Settings.js.rej
Patch: Source/devtools/front_end/Settings.js
Index: Source/devtools/front_end/Settings.js
diff --git a/Source/devtools/front_end/Settings.js
b/Source/devtools/front_end/Settings.js
index
558286188556380f9695e7d72bff6edb44f755eb..d20d582361afe7a82586e4550e0141a41fc760b4
100644
--- a/Source/devtools/front_end/Settings.js
+++ b/Source/devtools/front_end/Settings.js
@@ -39,6 +39,7 @@ var Preferences = {
var Capabilities = {
isMainFrontend: false,
+ canProfilePower: false,
}
/**
@@ -288,6 +289,7 @@ WebInspector.ExperimentsSettings =
function(experimentsEnabled)
this.timelineFlameChart = this._createExperiment("timelineFlameChart",
"Enable FlameChart mode in Timeline");
this.heapSnapshotStatistics =
this._createExperiment("heapSnapshotStatistics", "Show memory breakdown
statistics in heap snapshots");
this.timelineNoLiveUpdate = this._createExperiment("timelineNoLiveUpdate",
"Timeline w/o live update");
+ this.powerProfiler = this._createExperiment("powerProfiler", "Enable power
mode in Timeline");
this._cleanUpSetting();
}
The CQ bit was checked by pan.deng@intel.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pan.deng@intel.com/104523002/390001
Message was sent while issue was closed.
Change committed as 169951 |
