Bug 157907 – Web Inspector: Snapshots should be cleared at some point

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* 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.

[Radar WebKit Bug Importer]

<rdar://problem/26373610>

[Joseph Pecoraro]

Created attachment 281793 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 281794 [details]
[IMAGE] Invalidated Snapshots

[WebKit Commit Bot]

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.

[Timothy Hatcher]

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.

[Blaze Burg]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

<http://trac.webkit.org/changeset/202383>