Move profile type specific code out of ProfilesPanel class.
Created attachment 189759 [details] Patch
Comment on attachment 189759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189759&action=review > Source/WebCore/inspector/front-end/CPUProfileView.js:678 > + WebInspector.ProfileType.prototype.removeProfile.call(this, profile); Do you really need this call of super class? > Source/WebCore/inspector/front-end/CSSSelectorProfileView.js:-330 > - profilesPanel.setRecordingProfile(WebInspector.CSSSelectorProfileType.TypeId, true); Where is it called now? > Source/WebCore/inspector/front-end/CSSSelectorProfileView.js:-351 > - profilesPanel.setRecordingProfile(WebInspector.CSSSelectorProfileType.TypeId, false); Who notifies the panel about recording state change? > Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:356 > + { What if there is no temp profile? > Source/WebCore/inspector/front-end/ProfilesPanel.js:94 > + get profiles() Please use function instead of getter. > Source/WebCore/inspector/front-end/ProfilesPanel.js:484 > + console.trace(); Remove this. > Source/WebCore/inspector/front-end/ProfilesPanel.js:577 > + profileType.addEventListener(WebInspector.ProfileType.Events.AddProfileHeader, (function(event) { this._addProfileHeader(event.data); }).bind(this)); Please use a method instead of anonymous function. > Source/WebCore/inspector/front-end/ProfilesPanel.js:988 > + return this._getProfileTypesArray().reduce(function(result, type) { return result.concat(type.profiles); }, []); Maybe extract anonymous function and give it a name? > Source/WebCore/inspector/front-end/ProfilesPanel.js:995 > + const visibleView = this.visibleView; I'd rather use a plain old loop. > Source/WebCore/inspector/front-end/ProfilesPanel.js:-1001 > - for (var i = 0; i < this._profiles.length; ++i) { Revert this? > Source/WebCore/inspector/front-end/ProfilesPanel.js:-1148 > - this._launcherView.profileStarted(); Now that this code is removed, how does the launcher view know that profiling has started?
Comment on attachment 189759 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189759&action=review Thanks for the review. Please take another look. >> Source/WebCore/inspector/front-end/CPUProfileView.js:678 >> + WebInspector.ProfileType.prototype.removeProfile.call(this, profile); > > Do you really need this call of super class? Sure. Base ProfileType manages profile headers collection. >> Source/WebCore/inspector/front-end/CSSSelectorProfileView.js:-330 >> - profilesPanel.setRecordingProfile(WebInspector.CSSSelectorProfileType.TypeId, true); > > Where is it called now? in ProfilesPanel.toggleRecordButton >> Source/WebCore/inspector/front-end/CSSSelectorProfileView.js:-351 >> - profilesPanel.setRecordingProfile(WebInspector.CSSSelectorProfileType.TypeId, false); > > Who notifies the panel about recording state change? ProfilesPanel.toggleRecordButton does it itself. >> Source/WebCore/inspector/front-end/NativeMemorySnapshotView.js:356 >> + { > > What if there is no temp profile? This would be an error state. But I can add a check to be on the safe side. >> Source/WebCore/inspector/front-end/ProfilesPanel.js:94 >> + get profiles() > > Please use function instead of getter. done >> Source/WebCore/inspector/front-end/ProfilesPanel.js:484 >> + console.trace(); > > Remove this. done >> Source/WebCore/inspector/front-end/ProfilesPanel.js:577 >> + profileType.addEventListener(WebInspector.ProfileType.Events.AddProfileHeader, (function(event) { this._addProfileHeader(event.data); }).bind(this)); > > Please use a method instead of anonymous function. done >> Source/WebCore/inspector/front-end/ProfilesPanel.js:988 >> + return this._getProfileTypesArray().reduce(function(result, type) { return result.concat(type.profiles); }, []); > > Maybe extract anonymous function and give it a name? turned into a loop. >> Source/WebCore/inspector/front-end/ProfilesPanel.js:995 >> + const visibleView = this.visibleView; > > I'd rather use a plain old loop. ok. >> Source/WebCore/inspector/front-end/ProfilesPanel.js:-1001 >> - for (var i = 0; i < this._profiles.length; ++i) { > > Revert this? done >> Source/WebCore/inspector/front-end/ProfilesPanel.js:-1148 >> - this._launcherView.profileStarted(); > > Now that this code is removed, how does the launcher view know that profiling has started? It is called from ProfilesPanel.toggleRecordButton -> ProfilesPanel.setRecordingProfile
Created attachment 189778 [details] Patch
Comment on attachment 189778 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189778&action=review > Source/WebCore/inspector/front-end/ProfilesPanel.js:575 > + function onAddProfileHeader(event) { style: { on the next line > Source/WebCore/inspector/front-end/ProfilesPanel.js:1007 > + searchableViews.splice(0, 0, this.visibleView); searchableViews.unshift(this.visibleView); BTW, why does visible view have to go first? Maybe just make a swap(0, i) then? > Source/WebCore/inspector/front-end/ProfilesPanel.js:1042 > +// this._getAllProfiles().forEach(function(profile) { profile._profilesTreeElement.searchMatches = 0; }); Remove commented code.
Comment on attachment 189778 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=189778&action=review >> Source/WebCore/inspector/front-end/ProfilesPanel.js:575 >> + function onAddProfileHeader(event) { > > style: { on the next line ok >> Source/WebCore/inspector/front-end/ProfilesPanel.js:1007 >> + searchableViews.splice(0, 0, this.visibleView); > > searchableViews.unshift(this.visibleView); > > BTW, why does visible view have to go first? Maybe just make a swap(0, i) then? It was in the original implementation. Probably because it's more convenient to start searching in the opened view first. Changed to swap. >> Source/WebCore/inspector/front-end/ProfilesPanel.js:1042 >> +// this._getAllProfiles().forEach(function(profile) { profile._profilesTreeElement.searchMatches = 0; }); > > Remove commented code. ok
Created attachment 190016 [details] Patch
Comment on attachment 190016 [details] Patch Clearing flags on attachment: 190016 Committed r143900: <http://trac.webkit.org/changeset/143900>
All reviewed patches have been landed. Closing bug.