<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Activity Viewer does not update on "Clear Log on reload""
href="https://bugs.webkit.org/show_bug.cgi?id=145466#c6">Comment # 6</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Web Inspector: Activity Viewer does not update on "Clear Log on reload""
href="https://bugs.webkit.org/show_bug.cgi?id=145466">bug 145466</a>
from <span class="vcard"><a class="email" href="mailto:joepeck@webkit.org" title="Joseph Pecoraro <joepeck@webkit.org>"> <span class="fn">Joseph Pecoraro</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=253983&action=diff" name="attach_253983" title="[Patch] For Landing">attachment 253983</a> <a href="attachment.cgi?id=253983&action=edit" title="[Patch] For Landing">[details]</a></span>
[Patch] For Landing
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=253983&action=review">https://bugs.webkit.org/attachment.cgi?id=253983&action=review</a>
r=me, but I'd like to see at least an updated ChangeLog.
<span class="quote">> Source/WebInspectorUI/UserInterface/Base/Main.js:154
> + this.clearLogOnReload = new WebInspector.Setting("clear-log-on-reload", true);</span >
If there were multiple LogContentViews, it may have made more sense to keep this Setting per-LogContentView.
However, given the current implementation, this may make more sense on the single LogManager instance instead of WebInspector. Since LogManager seems to make decisions based on this setting or not.
<span class="quote">> Source/WebInspectorUI/UserInterface/Controllers/LogManager.js:35
> + this._clearMessagesRequested = false;
> + this._isPageReload = false;
> WebInspector.Frame.addEventListener(WebInspector.Frame.Event.MainResourceDidChange, this._mainResourceDidChange, this);</span >
Style: Add a newline between the member variable initialization and the event registration.
<span class="quote">> Source/WebInspectorUI/UserInterface/Controllers/LogManager.js:64
> + // Frontend receives unrequested "cleared console" call from Backend.</span >
All the other comments really aren't that important (and could be removed). But here is where a comment would be most important, because the reason "why" we are delaying is unclear. Something like:
// Received an unrequested clear console event.
// This could be for a navigation or other reasons (like console.clear()).
// If this was a reload, we may not want to dispatch WebInspector.LogManager.Event.Cleared.
// To detect if this is a reload we wait a turn and check if there was a main resource change reload.
If we then change the backend to no longer send a clear event on navigation we could:
(1) Add a COMPATIBILITY comment here.
(2) Have LogManager behave differently and keep this legacy path.
<span class="quote">> Source/WebInspectorUI/UserInterface/Controllers/LogManager.js:72
> + // A page reload happened. Only dispatch if "clear on reload" is enabled. Keep log otherwise.</span >
This comment seems superfluous given the variable names.
<span class="quote">> Source/WebInspectorUI/UserInterface/Controllers/LogManager.js:77
> + // A frame navigated, console.clear() or command line clear() happened</span >
Style: Comments are sentences and should end in a period.
<span class="quote">> Source/WebInspectorUI/UserInterface/Controllers/LogManager.js:92
> + this._clearMessagesRequested = true;
> ConsoleAgent.clearMessages();</span >
Style: Add a newline between these.
<span class="quote">> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:288
> + if (WebInspector.clearLogOnReload.value) {</span >
Style: Extra space by the brace.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>