[Webkit-unassigned] [Bug 145466] Web Inspector: Activity Viewer does not update on "Clear Log on reload"

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=145466

--- Comment #6 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 253983
  --> https://bugs.webkit.org/attachment.cgi?id=253983
[Patch] For Landing

View in context: https://bugs.webkit.org/attachment.cgi?id=253983&action=review

r=me, but I'd like to see at least an updated ChangeLog.

> Source/WebInspectorUI/UserInterface/Base/Main.js:154
> +    this.clearLogOnReload = new WebInspector.Setting("clear-log-on-reload", true);

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.

> Source/WebInspectorUI/UserInterface/Controllers/LogManager.js:35
> +        this._clearMessagesRequested = false;
> +        this._isPageReload = false;
>          WebInspector.Frame.addEventListener(WebInspector.Frame.Event.MainResourceDidChange, this._mainResourceDidChange, this);

Style: Add a newline between the member variable initialization and the event registration.

> Source/WebInspectorUI/UserInterface/Controllers/LogManager.js:64
> +            // Frontend receives unrequested "cleared console" call from Backend.

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.

> Source/WebInspectorUI/UserInterface/Controllers/LogManager.js:72
> +            // A page reload happened. Only dispatch if "clear on reload" is enabled. Keep log otherwise.

This comment seems superfluous given the variable names.

> Source/WebInspectorUI/UserInterface/Controllers/LogManager.js:77
> +            // A frame navigated, console.clear() or command line clear() happened

Style: Comments are sentences and should end in a period.

> Source/WebInspectorUI/UserInterface/Controllers/LogManager.js:92
> +        this._clearMessagesRequested = true;
>          ConsoleAgent.clearMessages();

Style: Add a newline between these.

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:288
> +        if (WebInspector.clearLogOnReload.value)  {

Style: Extra space by the brace.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150602/d313a749/attachment.html>