Bug 157907 - Web Inspector: Snapshots should be cleared at some point
Summary: Web Inspector: Snapshots should be cleared at some point
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-05-19 12:01 PDT by Joseph Pecoraro
Modified: 2016-06-23 11:53 PDT (History)
8 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (50.31 KB, patch)
2016-06-21 16:51 PDT, Joseph Pecoraro
timothy: review+
Details | Formatted Diff | Diff
[IMAGE] Invalidated Snapshots (110.23 KB, image/png)
2016-06-21 16:52 PDT, Joseph Pecoraro
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-05-19 12:01:33 PDT
* 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.
Comment 1 Radar WebKit Bug Importer 2016-05-19 12:04:16 PDT
<rdar://problem/26373610>
Comment 2 Joseph Pecoraro 2016-06-21 16:51:48 PDT
Created attachment 281793 [details]
[PATCH] Proposed Fix
Comment 3 Joseph Pecoraro 2016-06-21 16:52:14 PDT
Created attachment 281794 [details]
[IMAGE] Invalidated Snapshots
Comment 4 WebKit Commit Bot 2016-06-21 16:52:50 PDT
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 5 Timothy Hatcher 2016-06-23 10:27:18 PDT
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 6 Brian Burg 2016-06-23 10:55:48 PDT
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 7 Joseph Pecoraro 2016-06-23 11:23:00 PDT
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.
Comment 8 Joseph Pecoraro 2016-06-23 11:53:50 PDT
<http://trac.webkit.org/changeset/202383>