Bug 145466 – Web Inspector: Activity Viewer does not update on "Clear Log on reload"

• Component: Web Inspector
• Status: RESOLVED FIXED

[Tobias Reiss]

* STEPS TO REPRODUCE
1. Open https://bug-142070-attachments.webkit.org/attachment.cgi?id=247481
2. Go to Console Tab
3. Activity Viewer shows 11 logs
4. Select "Clear Log on Reload"
5. Reload page
6. Activity Viewer shows 22 logs instead of 11 logs

* DESCRIPTION
The Activity Viewer listens on "WebInspector.LogManager.Event.ActiveLogCleared" but the Event isn't dispatched. That is due to a hack in LogContentView that ignores the `Console.clearMessages` call. The hack tries to workaround a general problem between Backend and Frontend logic as described here rdar://problem/13767079.

* HOW TO FIX
1. Fix rdar://problem/13767079 and let the Backend pass a reason (PAGE_NAVIGATION or CLEAR_COMMAND) why the Console got cleared.
2. Don't dispatch "ActiveLogCleared" on "PAGE_NAVIGATION" anymore and let Frontend explicitly maintain page navigation (=reload).
3. Remove "_ignoreDidClearMessages" hack from LogContentView

[Radar WebKit Bug Importer]

<rdar://problem/21164026>

[Tobias Reiss]

Update 1: https://webkit.org/b/145468 is an equivalent of rdar://problem/13767079

Update 2: As noted in 145468, it is not enough to fix the Backend because we need some handling for legacy Backends (iOS 6, 7, 8) that wouldn't see the new Backend changes. Due to that fact I lost my motivation to touch 145468 since we need a solid solution for legacy Backends anyway.

Update 3: HOW TO FIX:
1. Isolate logic within LogManager and remove hacks/logic from all other files
2. Within LogManager: Differentiate "messagesCleared" calls
 a) that happen as a result of a "requested clear" by Frontend (e.g. Button)
 b) that happen on page reload and install "clear/keep-on-page-reload" logic
 c) that happen on frame navigated, console.clear() or clear()
3. Unify "ActiveLogCleared" and "Cleared" events to "Cleared"

For step 2b and 2c it is important to delay the handling and wait until "_mainResourceDidChange" has determined whether a page reload happened or not.

[Tobias Reiss]

Created attachment 253983 [details]
[Patch] For Landing

[Joseph Pecoraro]

> 1. Isolate logic within LogManager and remove hacks/logic from all other
> files
> 2. Within LogManager: Differentiate "messagesCleared" calls
>  a) that happen as a result of a "requested clear" by Frontend (e.g. Button)
>  b) that happen on page reload and install "clear/keep-on-page-reload" logic
>  c) that happen on frame navigated, console.clear() or clear()
> 3. Unify "ActiveLogCleared" and "Cleared" events to "Cleared"
> 
> For step 2b and 2c it is important to delay the handling and wait until
> "_mainResourceDidChange" has determined whether a page reload happened or
> not.

This is excellent. This information should really go in the ChangeLog, so that when someone next works in this area they can see the high level decision making process.

--

Looking at a navigation versus a reload.

    # Navigation

        backend: {"method":"Page.frameStartedLoading",...}
        backend: {"method":"Network.requestWillBeSent",...}
        backend: {"method":"Network.responseReceived",...}
        backend: {"method":"Network.dataReceived",...}
        backend: {"method":"Console.messagesCleared"}
        backend: {"method":"Page.frameNavigated",...}

    # Reload

        frontend: {"method":"Page.reload","id":29}
        backend: {"method":"Page.frameStartedLoading",...}
        backend: {"method":"Network.responseReceived",...}
        backend: {"method":"Network.dataReceived",...}
        backend: {"method":"Console.messagesCleared"}
        backend: {"method":"Page.frameNavigated",...}

So the behavior here is:

    => messagesCleared
      => requested?
        => Cleared
      => was not requested?
        => setTimeout(delayed, 0) to determine if the navigation (and thus the type of navigation) or if no navigation then JS clear()

    => frameNavigated
      => MainResourceDidChange
        => mark if reload

    => timeout trigger
      => we know if it was a reload or something else

This sounds good!

If we decide to make the backend not send messagesCleared during page navigation, then there is nothing that we need to do here. The drawback is that there will be a setTimeout(0) even though it wouldn't strictly be necessary. We may be able to address that.

[Joseph Pecoraro]

> If we decide to make the backend not send messagesCleared during page
> navigation, then there is nothing that we need to do here.

Actually, not true! Comments to be included in the bug.

[Joseph Pecoraro]

Comment on attachment 253983 [details]
[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.

[Tobias Reiss]

Created attachment 254213 [details]
[PATCH] For Landing

[Tobias Reiss]

Comment on attachment 254213 [details]
[PATCH] For Landing

Thank you for the detailed review! I updated the ChangeLog and also copy/pasted the comments you kindly wrote for me.

[WebKit Commit Bot]

Comment on attachment 254213 [details]
[PATCH] For Landing

Rejecting attachment 254213 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Nikita Vasilyev]

This bug looks a lot like "REGRESSION: Web Inspector: Dashboard's resource count does not reset when main resource navigates" https://bugs.webkit.org/show_bug.cgi?id=144553. If they are related and you know how to fix it right away, please feel free to steal it from me :)

[Tobias Reiss]

Created attachment 254218 [details]
[PATCH] For Landing

[WebKit Commit Bot]

Comment on attachment 254218 [details]
[PATCH] For Landing

Clearing flags on attachment: 254218

Committed r185213: <http://trac.webkit.org/changeset/185213>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.