Summary: | Web Inspector: Adding a new console message shouldn't modify DOM when the console log is hidden | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Nikita Vasilyev <nvasilyev> | ||||||||||||||||||||||
Component: | Web Inspector | Assignee: | Nikita Vasilyev <nvasilyev> | ||||||||||||||||||||||
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 | ||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=157186 | ||||||||||||||||||||||||
Bug Depends on: | 157294 | ||||||||||||||||||||||||
Bug Blocks: | 136984, 142011 | ||||||||||||||||||||||||
Attachments: |
|
Description
Nikita Vasilyev
2016-03-18 00:34:20 PDT
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.
Created attachment 274390 [details]
[Animated GIF] With the patch applied
No repaints around console prompt area.
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. Created attachment 277111 [details] WIP This WIP doesn't have any known bugs. It also fixes bug 156484. 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. (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. Created attachment 277728 [details]
Patch
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. Created attachment 277831 [details]
Patch
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. *** Bug 152221 has been marked as a duplicate of this bug. *** Created attachment 277914 [details]
Patch
*** Bug 157186 has been marked as a duplicate of this bug. *** Comment on attachment 277914 [details]
Patch
[Error] TypeError: undefined is not an object (evaluating 'lastMessageView.message.type')
renderPendingMessages (JavaScriptLogViewController.js:286)
Created attachment 277926 [details]
Patch
Comment on attachment 277926 [details] Patch Clearing flags on attachment: 277926 Committed r200337: <http://trac.webkit.org/changeset/200337> All reviewed patches have been landed. Closing bug. 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(). Re-opened since this is blocked by bug 157294 (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. Created attachment 278023 [details]
Patch
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. (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. Created attachment 278024 [details]
Patch
Added super.shown().
Comment on attachment 278024 [details] Patch Clearing flags on attachment: 278024 Committed r200401: <http://trac.webkit.org/changeset/200401> All reviewed patches have been landed. Closing bug. |