* SUMMARY Snapshots should be cleared at some point. Unfortunately right now we continue to preserve previous TimelineRecordings, so in effect we never clear any snapshots ever. On both the frontend and the backend. Maybe page navigations should do something (on the frontend and backend) and basically finalize the snapshots as all Dead or something.
<rdar://problem/26373610>
Created attachment 281793 [details] [PATCH] Proposed Fix
Created attachment 281794 [details] [IMAGE] Invalidated Snapshots
Attachment 281793 [details] did not pass style-queue: ERROR: Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.h:54: "override" is redundant since function is already declared as "final" [readability/inheritance] [4] ERROR: Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.h:55: "override" is redundant since function is already declared as "final" [readability/inheritance] [4] ERROR: Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.h:56: "override" is redundant since function is already declared as "final" [readability/inheritance] [4] ERROR: Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.h:57: "override" is redundant since function is already declared as "final" [readability/inheritance] [4] ERROR: Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.h:58: The parameter name "functionDetails" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.h:58: The parameter name "objectPreview" adds no information, so it should be removed. [readability/parameter_name] [5] ERROR: Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.h:58: "override" is redundant since function is already declared as "final" [readability/inheritance] [4] ERROR: Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.h:59: "override" is redundant since function is already declared as "final" [readability/inheritance] [4] Total errors found: 8 in 27 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 281793 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=281793&action=review > Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.h:59 > + void gc(ErrorString&) override final; > + void snapshot(ErrorString&, double* timestamp, String* snapshotData) override final; > + void startTracking(ErrorString&) override final; > + void stopTracking(ErrorString&) override final; > + void getPreview(ErrorString&, int heapObjectId, Inspector::Protocol::OptOutput<String>* resultString, RefPtr<Inspector::Protocol::Debugger::FunctionDetails>& functionDetails, RefPtr<Inspector::Protocol::Runtime::ObjectPreview>& objectPreview) override final; > + void getRemoteObject(ErrorString&, int heapObjectId, const String* optionalObjectGroup, RefPtr<Inspector::Protocol::Runtime::RemoteObject>& result) override final; Just final.
Comment on attachment 281793 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=281793&action=review Code change looks good. I am a little concerned that this policy will be too aggressive for multi-navigation recordings, but I guess we can wait and see if this turns out to be a problem in practice. (I think it would be more intuitive if we drop snapshots when the recording becomes non-active.) > Source/WebCore/inspector/PageHeapAgent.cpp:56 > +} // namespace WebCore Trailing newline? > Source/WebInspectorUI/UserInterface/Proxies/HeapSnapshotWorkerProxy.js:103 > + this.clearSnapshots(function() { arrow function? > Source/WebInspectorUI/UserInterface/Proxies/HeapSnapshotWorkerProxy.js:117 > + // Error. Comment here does not add anything.
Comment on attachment 281793 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=281793&action=review >> Source/WebCore/inspector/PageHeapAgent.cpp:56 >> +} // namespace WebCore > > Trailing newline? FWIW: There is a trailing newline, and the diff tool would mention if there wasn't a trailing newline in its own line. >> Source/WebInspectorUI/UserInterface/Proxies/HeapSnapshotWorkerProxy.js:103 >> + this.clearSnapshots(function() { > > arrow function? I've been waffling on whether or not to use an arrow function for a simple function that doesn't use `this`. But maybe we should just start using it in more places like inline anonymous functions.
<http://trac.webkit.org/changeset/202383>