Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2378493002:
DevTools: ConsoleViewMessage breakup _formatMessage() (Closed)
|
Created:
4 years, 2 months ago by luoe Modified:
4 years, 2 months ago Reviewers:
lushnikov CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: ConsoleViewMessage breakup _formatMessage()
This CL splits parts of _formatMessage() into the following functions:
_buildMessage(), _buildAnchor(), and _wrapStackTraceFormatting(). The only
functional change should be that messageElement is now a text node wrapped in a
span, similar to every other messageElement.
Committed: https://crrev.com/879d6c37ada852ab3fcb6302877701fcf7cb864f
Cr-Commit-Position: refs/heads/master@{#422991}
Patch Set 1 #Patch Set 2 : withclass #
Total comments: 2
Patch Set 3 : param2RO #Patch Set 4 : any type #
Total comments: 12
Patch Set 5 : ac-1, tests #Patch Set 6 : back to wrapping anchor with message #
Total comments: 2
Patch Set 7 : ac-2 #Patch Set 8 : rebase master #
Total comments: 8
Patch Set 9 : ac-3 #
Total comments: 2
Patch Set 10 : ac-4 #Messages
Total messages: 25 (10 generated)
Description was changed from ========== DevTools: breakout formatting blocks ========== to ========== DevTools: ConsoleViewMessage breakup _formatMessage() This CL splits parts of _formatMessage() into the following functions: _buildMessage(), _buildAnchor(), and _wrapStackTraceFormatting(). The only functional change should be that messageElement is now a text node wrapped in a span, similar to every other messageElement. ==========
luoe@chromium.org changed reviewers: + lushnikov@chromium.org
https://codereview.chromium.org/2378493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (left): https://codereview.chromium.org/2378493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:211: } This chunk was moved to L:259 https://codereview.chromium.org/2378493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2378493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:798: * @param {!Array.<!WebInspector.RemoteObject>} parameters Note: the old version was incorrect. It actually takes an array of remote objects.
What's the motivation behind the patch? Do you plan to re-use new functions?
On 2016/09/28 16:50:17, lushnikov wrote: > What's the motivation behind the patch? Do you plan to re-use new functions? No re-use planned atm. Splitting these functions is for I plan to move the new functions (_build*) along with other formatting logic from ConsoleViewMessage (CVM) into ConsoleMessageFormatter.js (CMF). At first, I started isolating the console.table() logic (appears in the next CL) by having _formatMessage() call a completely _formatMessageAsTable(). However, logic for building an anchor element is needed in both functions, so I split that out. The only formatting functions that CVM will retain are _formatMessage(), _formatMessageAsTable(), which themselves call CMF._build*() I suppose that introducing _wrapStackTraceFormating() isn't strictly necessary since it isn't reused anywhere else, but it seemed like a good chunk to encapsulate. But since we don't need to use it just yet, I can put the code back into _formatMessage().
On 2016/09/28 17:18:35, luoe wrote: > On 2016/09/28 16:50:17, lushnikov wrote: > > What's the motivation behind the patch? Do you plan to re-use new functions? > > No re-use planned atm. Splitting these functions is for * meant to say: "Splitting these functions is necessary to move functions into CMF"
https://codereview.chromium.org/2378493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2378493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:157: formattedMessage.insertBefore(anchorElement, formattedMessage.firstChild); you don't need insertBefore now https://codereview.chromium.org/2378493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:160: var dumpStackTrace = !!consoleMessage.stackTrace && (consoleMessage.source === WebInspector.ConsoleMessage.MessageSource.Network || consoleMessage.level === WebInspector.ConsoleMessage.MessageLevel.Error || consoleMessage.level === WebInspector.ConsoleMessage.MessageLevel.RevokedError || consoleMessage.type === WebInspector.ConsoleMessage.MessageType.Trace || consoleMessage.level === WebInspector.ConsoleMessage.MessageLevel.Warning); should this code go inside _buildMessage? https://codereview.chromium.org/2378493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:163: var stackTracePreview = WebInspector.DOMPresentationUtils.buildStackTracePreviewContents(target, this._linkifier, consoleMessage.stackTrace); let's move this inside _wrapStackTraceFormatting https://codereview.chromium.org/2378493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:274: _wrapStackTraceFormatting: function(contents, stackTracePreview, autoExpand) lets call it _buildMessageStackTrace: function(consoleMessage) https://codereview.chromium.org/2378493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:357: * @param {*} parameter {!Array.<!WebInspector.RemoteObject|string>} parameters https://codereview.chromium.org/2378493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:365: else if (!target) style: drop "else" if (parameter instanceof WI.RemoteObject) return parameter; if (!target) return WebInspector.RemoteObject.fromLocatObject(parameter); ...
Additional changes: - Removed _formatMessage(), since only a few lines remained there. Logic moved to contentElement(). - buildAnchor(consoleMessage) >> buildMessageAnchor(contents, consoleMessage). It now takes an element, creates an anchor sibling, and wraps both in a span. No functional change, this is identical to the DOM structure before. PTAL https://codereview.chromium.org/2378493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2378493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:157: formattedMessage.insertBefore(anchorElement, formattedMessage.firstChild); On 2016/09/28 20:50:17, lushnikov wrote: > you don't need insertBefore now Done. https://codereview.chromium.org/2378493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:160: var dumpStackTrace = !!consoleMessage.stackTrace && (consoleMessage.source === WebInspector.ConsoleMessage.MessageSource.Network || consoleMessage.level === WebInspector.ConsoleMessage.MessageLevel.Error || consoleMessage.level === WebInspector.ConsoleMessage.MessageLevel.RevokedError || consoleMessage.type === WebInspector.ConsoleMessage.MessageType.Trace || consoleMessage.level === WebInspector.ConsoleMessage.MessageLevel.Warning); On 2016/09/28 20:50:17, lushnikov wrote: > should this code go inside _buildMessage? Done. https://codereview.chromium.org/2378493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:163: var stackTracePreview = WebInspector.DOMPresentationUtils.buildStackTracePreviewContents(target, this._linkifier, consoleMessage.stackTrace); On 2016/09/28 20:50:17, lushnikov wrote: > let's move this inside _wrapStackTraceFormatting Done. https://codereview.chromium.org/2378493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:274: _wrapStackTraceFormatting: function(contents, stackTracePreview, autoExpand) On 2016/09/28 20:50:17, lushnikov wrote: > lets call it _buildMessageStackTrace: function(consoleMessage) Done, with a couple more params: function(contents, consoleMessage, linkifier) https://codereview.chromium.org/2378493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:357: * @param {*} parameter On 2016/09/28 20:50:17, lushnikov wrote: > {!Array.<!WebInspector.RemoteObject|string>} parameters Actually, some parameters are plain objects, so we should do {!WebInspector.RemoteObject|!Object|string} https://codereview.chromium.org/2378493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:365: else if (!target) On 2016/09/28 20:50:17, lushnikov wrote: > style: drop "else" > > if (parameter instanceof WI.RemoteObject) > return parameter; > if (!target) > return WebInspector.RemoteObject.fromLocatObject(parameter); > ... Done.
https://codereview.chromium.org/2378493002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2378493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:216: * @param {!Element} contents let's not have the "contents" element - it makes semantics complicated
buildMessageAnchor() returns the anchorElement, no more wrapping. https://codereview.chromium.org/2378493002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2378493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:216: * @param {!Element} contents On 2016/09/30 21:28:05, lushnikov wrote: > let's not have the "contents" element - it makes semantics complicated Done.
https://codereview.chromium.org/2378493002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2378493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:154: messageElement.textContent = WebInspector.UIString("Console was cleared"); why this change? https://codereview.chromium.org/2378493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:256: _buildMessageStackTrace: function(contents, consoleMessage, linkifier) _buildMessageWithStackTrace: function(consoleMessage, linkifier) https://codereview.chromium.org/2378493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:258: var target = consoleMessage.target(); var messageElement = this._buildMessage(consoleMessage); https://codereview.chromium.org/2378493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:972: var formattedMessage = this._buildMessage(this._message); if (THIS_IS_STACK_MESSAGE) formatterMessage = this._buildMessageWithStack(this._message) else formattedMessage = this._buildMessage(this._message); And in the next patch you'll have a table condition here Will this work?
buildMessageWithStackTrace() no longer takes an element as a parameter! ptal https://codereview.chromium.org/2378493002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2378493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:154: messageElement.textContent = WebInspector.UIString("Console was cleared"); On 2016/10/01 02:30:41, lushnikov wrote: > why this change? Before ".console-info" was being applied to make formattedMessage italic style, which included the anchor element. I think we don't want the anchor style to change in a console.clear() message, only the message content itself should. https://codereview.chromium.org/2378493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:256: _buildMessageStackTrace: function(contents, consoleMessage, linkifier) On 2016/10/01 02:30:41, lushnikov wrote: > _buildMessageWithStackTrace: function(consoleMessage, linkifier) Done. https://codereview.chromium.org/2378493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:258: var target = consoleMessage.target(); On 2016/10/01 02:30:41, lushnikov wrote: > var messageElement = this._buildMessage(consoleMessage); Done. https://codereview.chromium.org/2378493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:972: var formattedMessage = this._buildMessage(this._message); On 2016/10/01 02:30:41, lushnikov wrote: > if (THIS_IS_STACK_MESSAGE) > formatterMessage = this._buildMessageWithStack(this._message) > else > formattedMessage = this._buildMessage(this._message); > > And in the next patch you'll have a table condition here > > Will this work? Done.
lgtm https://codereview.chromium.org/2378493002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2378493002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:257: if (!target) let's move this check up to the "shouldIncludeTrace" boolean in the _contentElement function
https://codereview.chromium.org/2378493002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2378493002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:257: if (!target) On 2016/10/04 00:10:05, lushnikov wrote: > let's move this check up to the "shouldIncludeTrace" boolean in the > _contentElement function Moved next to the shouldIncludeTrace. It turns out that L267's DOMPresentationUtils.buildStackTracePreviewContents() requires a non-null target, so let's make target a parameter of buildMessageWithSTackTrace() itself.
The CQ bit was checked by luoe@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by luoe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2378493002/#ps180001 (title: "ac-4")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== DevTools: ConsoleViewMessage breakup _formatMessage() This CL splits parts of _formatMessage() into the following functions: _buildMessage(), _buildAnchor(), and _wrapStackTraceFormatting(). The only functional change should be that messageElement is now a text node wrapped in a span, similar to every other messageElement. ========== to ========== DevTools: ConsoleViewMessage breakup _formatMessage() This CL splits parts of _formatMessage() into the following functions: _buildMessage(), _buildAnchor(), and _wrapStackTraceFormatting(). The only functional change should be that messageElement is now a text node wrapped in a span, similar to every other messageElement. ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== DevTools: ConsoleViewMessage breakup _formatMessage() This CL splits parts of _formatMessage() into the following functions: _buildMessage(), _buildAnchor(), and _wrapStackTraceFormatting(). The only functional change should be that messageElement is now a text node wrapped in a span, similar to every other messageElement. ========== to ========== DevTools: ConsoleViewMessage breakup _formatMessage() This CL splits parts of _formatMessage() into the following functions: _buildMessage(), _buildAnchor(), and _wrapStackTraceFormatting(). The only functional change should be that messageElement is now a text node wrapped in a span, similar to every other messageElement. Committed: https://crrev.com/879d6c37ada852ab3fcb6302877701fcf7cb864f Cr-Commit-Position: refs/heads/master@{#422991} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/879d6c37ada852ab3fcb6302877701fcf7cb864f Cr-Commit-Position: refs/heads/master@{#422991} |
