Summary: | Web Inspector: Snapshots should be cleared at some point | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||
Component: | Web Inspector | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Attachments: |
|
Description
Joseph Pecoraro
2016-05-19 12:01:33 PDT
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. |