Bug 155629 – Web Inspector: Adding a new console message shouldn't modify DOM when the console log is hidden

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Steps:
1. Open about:blank
2. Open Web Inspector console
3. Enter the following code:
   setInterval(function() {console.log(Math.random())}, 1)
4. Open Elements tab

Expected:
No repaints in the console prompt area.
Web Inspector UI remains responsive.

Actual:
Console prompt gets repainted every millisecond.
Web Inspector UI quickly becomes unresponsive.

[Radar WebKit Bug Importer]

<rdar://problem/25235470>

[Nikita Vasilyev]

Created attachment 274389 [details]
WIP

Known issues:
1. Clicking on the dashboard error/warning/log icons for
   the first time doesn't filter out console messages.
2. The unread messages dot in the console scope bar selector is broken.

[Nikita Vasilyev]

Created attachment 274390 [details]
[Animated GIF] With the patch applied

No repaints around console prompt area.

[Nikita Vasilyev]

Created attachment 274642 [details]
WIP

> Known issues:
> 1. Clicking on the dashboard error/warning/log icons for
>    the first time doesn't filter out console messages.
> 2. The unread messages dot in the console scope bar selector is broken.

These are fixed.

Known issue:
- Repeat counter sometimes doesn't update.

[Nikita Vasilyev]

Created attachment 277111 [details]
WIP

This WIP doesn't have any known bugs.
It also fixes bug 156484.

[Joseph Pecoraro]

Comment on attachment 277111 [details]
WIP

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

> Source/WebInspectorUI/UserInterface/Models/ConsoleCommand.js:26
> +WebInspector.ConsoleCommand = class ConsoleMessage extends WebInspector.Object

Nit: class name here is wrong.

[Nikita Vasilyev]

(In reply to comment #5)
> Created attachment 277111 [details]
> WIP
> 
> This WIP doesn't have any known bugs.
> It also fixes bug 156484.

Known issues:
Repeat counts don't update if the console message hasn't been rendered.

Steps:
1. Open Elements tab, hide split console.
2. Click on <button onclick="console.log(1)">click</button> twice.
3. Open console tab.

Also, WebInspector.JavaScriptLogViewController#appendImmediateExecutionWithResult
still modifies DOM right away.

I'm working on it.

[Nikita Vasilyev]

Created attachment 277728 [details]
Patch

[Timothy Hatcher]

Comment on attachment 277728 [details]
Patch

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

Looks good, with one comment.

> Source/WebInspectorUI/UserInterface/Controllers/JavaScriptLogViewController.js:269
> +        if (WebInspector.consoleContentView.visible)
> +            requestAnimationFrame(() => this.renderPendingMessages());

You should only need to do this one. As-is it will request multiple animation frames if there are multiple messages appended. You should keep the animation frame identifier around and not request again if we already have a pending request.

[Nikita Vasilyev]

Created attachment 277831 [details]
Patch

[Timothy Hatcher]

Comment on attachment 277831 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Controllers/JavaScriptLogViewController.js:273
> +    renderPendingMessages()

You should cancel and clear _scheduledRenderIdentifier here, otherwise the call in shown() will still let the rAF fire once.

> Source/WebInspectorUI/UserInterface/Controllers/JavaScriptLogViewController.js:300
> +        let renderOnRequestAnimationFrame = function() {
> +            this._scheduledRenderIdentifier = 0;
> +            this.renderPendingMessages();
> +        }.bind(this);

If you clear in renderPendingMessages() you would not need to do this here.

[Timothy Hatcher]

*** Bug 152221 has been marked as a duplicate of this bug. ***

[Nikita Vasilyev]

Created attachment 277914 [details]
Patch

[Nikita Vasilyev]

*** Bug 157186 has been marked as a duplicate of this bug. ***

[Nikita Vasilyev]

Comment on attachment 277914 [details]
Patch

[Error] TypeError: undefined is not an object (evaluating 'lastMessageView.message.type')
	renderPendingMessages (JavaScriptLogViewController.js:286)

[Nikita Vasilyev]

Created attachment 277926 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 277926 [details]
Patch

Clearing flags on attachment: 277926

Committed r200337: <http://trac.webkit.org/changeset/200337>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 277926 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:127
> +        this._logViewController.renderPendingMessages();

This should probably call super.shown().

[WebKit Commit Bot]

Re-opened since this is blocked by bug 157294

[Nikita Vasilyev]

(In reply to comment #20)
> Re-opened since this is blocked by bug 157294

Steps:
1. Open Elements tab, keep the split console *closed*
2. Type 1+1 in the console prompt
3. Press Enter

Web Inspector freezes

https://github.com/WebKit/webkit/blob/eeda037140553acaab606ed51ed69af023582116/Source/WebInspectorUI/UserInterface/Controllers/JavaScriptLogViewController.js#L273

renderPendingMessages calls _didRenderConsoleMessageView,
which opens the split console,
which triggers WebInspector.LogContentView#shown,
which calls renderPendingMessages again.

[Nikita Vasilyev]

Created attachment 278023 [details]
Patch

[Timothy Hatcher]

Comment on attachment 278023 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:128
> +    shown()
> +    {
> +        this._logViewController.renderPendingMessages();
> +    }

This should call super.shown() to be future proof.

[Nikita Vasilyev]

(In reply to comment #23)
> Comment on attachment 278023 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=278023&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/LogContentView.js:128
> > +    shown()
> > +    {
> > +        this._logViewController.renderPendingMessages();
> > +    }
> 
> This should call super.shown() to be future proof.

Yes! I saw Joe's comment but forgot about it.

[Nikita Vasilyev]

Created attachment 278024 [details]
Patch

Added super.shown().

[WebKit Commit Bot]

Comment on attachment 278024 [details]
Patch

Clearing flags on attachment: 278024

Committed r200401: <http://trac.webkit.org/changeset/200401>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.