Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2644753002:
DevTools: untruncate links on copy (Closed)
|
Created:
3 years, 11 months ago by luoe Modified:
3 years, 7 months ago 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/heads/master Project:
chromium Visibility:
Public. |
DescriptionDevTools: untruncate links on copy
During exporti or copying of selected text, links that were truncated will be
replaced with their original, un-trimmed text.
BUG=505177
Review-Url: https://codereview.chromium.org/2644753002
Cr-Commit-Position: refs/heads/master@{#468514}
Committed: https://chromium.googlesource.com/chromium/src/+/9bee9f62d8507206938b746e3987ee2c87d6190b
Patch Set 1 #Patch Set 2 : a #
Total comments: 11
Patch Set 3 : ac #
Total comments: 4
Patch Set 4 : ac #
Total comments: 2
Patch Set 5 : rebase and return to patch set 3 but with anchorElementForTest removed #
Total comments: 4
Patch Set 6 : ac #
Total comments: 8
Patch Set 7 : account for links with complex subtrees #Patch Set 8 : ac #Patch Set 9 : ac! #Patch Set 10 : YAAAY #Patch Set 11 : reset tests with zero width space #
Total comments: 10
Patch Set 12 : ac #Patch Set 13 : simpler #
Total comments: 8
Patch Set 14 : split by regex, always trim hash #Patch Set 15 : short url with hashes test #Patch Set 16 : undo zero-width space, back to ellipsis #
Total comments: 2
Patch Set 17 : ac #Messages
Total messages: 38 (13 generated)
Description was changed from ========== DevTools: untruncate links on copy BUG= ========== to ========== DevTools: untruncate links on copy During exporti or copying of selected text, links that were truncated will be replaced with their original, un-trimmed text. BUG=505177 ==========
luoe@chromium.org changed reviewers: + allada@chromium.org
Please take a look
https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:308: var text = info.originalLinkText.replace(/([a-f0-9]{7})[a-f0-9]{13}[a-f0-9]*/g, '$1\u2026'); This looks very strange to me. I believe it's saying: replace text that is a-z0-9 of length 20 or more with only the first 7 characters followed by '...' I think it'd be more readable to change this regex to: text.replace(/([a-z0-9]{7})[a-f0-9]{13,}/g, '$1\u2026') https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:79: return typeof originalLinkText === 'string' ? originalLinkText : node.textContent; nit: return originalLinkText === null ? node.textContent : originalLinkText (maybe consider if empty strings should look into node.textContent too?) https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CallStackSidebarPane.js (right): https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CallStackSidebarPane.js:174: var text = uiLocation.linkText(true /* skipTrim */); Are we sure we want to show the full data in text? This includes massive urls like data urls or really long urls, which will make possibly massive tooltips. https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js (right): https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:668: var linkText = this.uiSourceCode.displayName(skipTrim); I'm worried about data urls. Can we do a test to see what happens if we have a few hundred 20k data urls get logged and returned by this method?
https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:308: var text = info.originalLinkText.replace(/([a-f0-9]{7})[a-f0-9]{13}[a-f0-9]*/g, '$1\u2026'); On 2017/03/28 01:13:08, allada wrote: > This looks very strange to me. I believe it's saying: > > replace text that is a-z0-9 of length 20 or more with only the first 7 > characters followed by '...' > > I think it'd be more readable to change this regex to: > text.replace(/([a-z0-9]{7})[a-f0-9]{13,}/g, '$1\u2026') Done. https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:79: return typeof originalLinkText === 'string' ? originalLinkText : node.textContent; On 2017/03/28 01:13:08, allada wrote: > nit: return originalLinkText === null ? node.textContent : originalLinkText > > (maybe consider if empty strings should look into node.textContent too?) Done. Empty strings is a good case to consider, but right now there's no reason why they should be different. I'd keep non-nulls consistent. https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CallStackSidebarPane.js (right): https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CallStackSidebarPane.js:174: var text = uiLocation.linkText(true /* skipTrim */); On 2017/03/28 01:13:08, allada wrote: > Are we sure we want to show the full data in text? This includes massive urls > like data urls or really long urls, which will make possibly massive tooltips. I tried it out, and it doesn't look good with a huge wall of tooltip on hover. Done, removed. https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js (right): https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:668: var linkText = this.uiSourceCode.displayName(skipTrim); On 2017/03/28 01:13:08, allada wrote: > I'm worried about data urls. Can we do a test to see what happens if we have a > few hundred 20k data urls get logged and returned by this method? Testing methodology: make 10k logs of the sample text in the console, select all messages, ctrl-C, and compute time spent on _selectedText() In the worst case conditions, a truncated string is 10k characters long (we have a MaxLengthToIgnoreLinkifier that bails out). Sample: data url of length 10k (trimmed to 150 currently) - 483ms before, 788ms after patch Sample: regular text of length 150 - 361ms before, 409ms after patch In the worst case, copying hundreds of MB of text takes a few hundred more ms. The average case does not regress by much.
lgtm https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:79: return typeof originalLinkText === 'string' ? originalLinkText : node.textContent; On 2017/03/31 21:35:21, luoe wrote: > On 2017/03/28 01:13:08, allada wrote: > > nit: return originalLinkText === null ? node.textContent : originalLinkText > > > > (maybe consider if empty strings should look into node.textContent too?) > > Done. > > Empty strings is a good case to consider, but right now there's no reason why > they should be different. I'd keep non-nulls consistent. Acknowledged. https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/sources/CallStackSidebarPane.js (right): https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/sources/CallStackSidebarPane.js:174: var text = uiLocation.linkText(true /* skipTrim */); On 2017/03/31 21:35:21, luoe wrote: > On 2017/03/28 01:13:08, allada wrote: > > Are we sure we want to show the full data in text? This includes massive urls > > like data urls or really long urls, which will make possibly massive tooltips. > > I tried it out, and it doesn't look good with a huge wall of tooltip on hover. > Done, removed. Acknowledged. https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js (right): https://codereview.chromium.org/2644753002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/workspace/UISourceCode.js:668: var linkText = this.uiSourceCode.displayName(skipTrim); On 2017/03/31 21:35:21, luoe wrote: > On 2017/03/28 01:13:08, allada wrote: > > I'm worried about data urls. Can we do a test to see what happens if we have a > > few hundred 20k data urls get logged and returned by this method? > > Testing methodology: make 10k logs of the sample text in the console, select all > messages, ctrl-C, and compute time spent on _selectedText() > > In the worst case conditions, a truncated string is 10k characters long (we have > a MaxLengthToIgnoreLinkifier that bails out). > > Sample: data url of length 10k (trimmed to 150 currently) > - 483ms before, 788ms after patch > > Sample: regular text of length 150 > - 361ms before, 409ms after patch > > In the worst case, copying hundreds of MB of text takes a few hundred more ms. > The average case does not regress by much. Acknowledged.
luoe@chromium.org changed reviewers: + lushnikov@chromium.org
https://codereview.chromium.org/2644753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2644753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:127: this._anchorElementForTest = anchorElement; query it! https://codereview.chromium.org/2644753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2644753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:437: var lineContent = currElement.childTextNodes().map(Console.ConsoleViewport.contentTransform).join(''); Let's come up with a general solution, which will allow us to have text truncation in console messages. How about: 1. A simple utility method to create truncated textNodes: DOMUtils.createTruncatedTextNode(text, maxLength) { textNode = document.createTextNode(); textNode.textContent = text.truncate(text); textNode[UntruncatedTextSymbol] = text; } 2. Only a few places need untruncated text: ConsoleView copying and ConsoleView extraction logic. These two places could explicitly look for UntruncatedTextSymbol on text nodes.
Comments addressed! Please take another look https://codereview.chromium.org/2644753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js (right): https://codereview.chromium.org/2644753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewMessage.js:127: this._anchorElementForTest = anchorElement; On 2017/04/04 01:07:07, lushnikov wrote: > query it! Done. https://codereview.chromium.org/2644753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2644753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:437: var lineContent = currElement.childTextNodes().map(Console.ConsoleViewport.contentTransform).join(''); Done, also with a DOMPresUtils.originalNodeText() to make #2 convenient.
dgozman@chromium.org changed reviewers: + dgozman@chromium.org
https://codereview.chromium.org/2644753002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/DOMPresentationUtils.js (right): https://codereview.chromium.org/2644753002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/DOMPresentationUtils.js:601: node[Components.DOMPresentationUtils._untruncatedTextSymbol] = originalText; This is only used for Console, let's put it there. I doubt we'll support copying text similar to how we do in Console in any other place. Also, this could retain potentially long strings for no reason.
Please take another look https://codereview.chromium.org/2644753002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/components/DOMPresentationUtils.js (right): https://codereview.chromium.org/2644753002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/components/DOMPresentationUtils.js:601: node[Components.DOMPresentationUtils._untruncatedTextSymbol] = originalText; I've returned to the approach in patch set 3 where only console makes use of originalLinkText.
https://codereview.chromium.org/2644753002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2644753002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:78: var originalLinkText = Components.Linkifier.originalLinkText(node.parentElement); let's inline this method to clients https://codereview.chromium.org/2644753002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:472: var originalLinkText = Components.Linkifier.originalLinkText(container.parentElement); this logic belongs to the linkifier who truncates link text in a complicated manner. Let's put it there: Linkifier = class { ... selectionOffsetToOriginalTextOffset { .. } }
Ready for another look https://codereview.chromium.org/2644753002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2644753002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:78: var originalLinkText = Components.Linkifier.originalLinkText(node.parentElement); On 2017/04/08 00:00:31, lushnikov wrote: > let's inline this method to clients Done. https://codereview.chromium.org/2644753002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:472: var originalLinkText = Components.Linkifier.originalLinkText(container.parentElement); On 2017/04/08 00:00:31, lushnikov wrote: > this logic belongs to the linkifier who truncates link text in a complicated > manner. > > Let's put it there: > > Linkifier = class { > ... > > selectionOffsetToOriginalTextOffset { > .. > } > } Done.
https://codereview.chromium.org/2644753002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/2644753002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:424: static originalLinkText(nodes) { let's pass a link here https://codereview.chromium.org/2644753002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:438: static selectionOffsetToOriginalOffset(offset, node) { let's pass link here https://codereview.chromium.org/2644753002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:438: static selectionOffsetToOriginalOffset(offset, node) { selectionOffsetToOriginalTextOffset ? https://codereview.chromium.org/2644753002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:446: if (offset > link.textContent.length >> 1) /2
In order to account for selections over truncated links with nested elements inside them (e.g. highlighted ranges when doing a search), I've updated the logic to check for an enclosing 'devtools-link' instead of just checking the parentElement. Please take another look https://codereview.chromium.org/2644753002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/2644753002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:424: static originalLinkText(nodes) { On 2017/04/11 22:59:34, lushnikov wrote: > let's pass a link here Done. https://codereview.chromium.org/2644753002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:438: static selectionOffsetToOriginalOffset(offset, node) { On 2017/04/11 22:59:34, lushnikov wrote: > let's pass link here Done. https://codereview.chromium.org/2644753002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:438: static selectionOffsetToOriginalOffset(offset, node) { On 2017/04/11 22:59:34, lushnikov wrote: > let's pass link here Done. https://codereview.chromium.org/2644753002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:446: if (offset > link.textContent.length >> 1) On 2017/04/11 22:59:34, lushnikov wrote: > /2 Done.
as per offline discussion, we came up with the following solution: - instead of simply truncating text, we should create multiple textnodes which hold the original text information. Example - today: <a> <T>google.com/a/b/.../c.html</T> </a> The link text is a single textnode <T> with ellipsis in the middle. Example - future: <a> <T>google.com/a/b</T> <T>...</T> <T>c.html</T> </a> The link text consists of three text nodes. The text node which holds ellipsis also stores the truncated text as a symbol on itself. Note: make sure this idea will work with the UI.highlightRangesWithStyleClass
It appears that the range highlighter will interfere when we make an empty text node in the middle. If we make the node in the middle an ellipsis character, users will be able to search for it, which interferes as well. Using a zero-width space character seems to do the trick, thanks einbinder@! Please take another look
https://codereview.chromium.org/2644753002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/2644753002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:424: if (trimHashes) { I don't quite follow what's going on here. I though we need to truncate hash first and then truncate middle if needed. https://codereview.chromium.org/2644753002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:459: static selectionOffsetToOriginalTextOffset(node, offset) { you don't need this method anymore - let's inline https://codereview.chromium.org/2644753002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:471: static originalNodeText(node) { untruncatedNodeText https://codereview.chromium.org/2644753002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/platform/utilities.js (right): https://codereview.chromium.org/2644753002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/platform/utilities.js:173: String.prototype.trimMiddleIndices = function(maxLength) { Let's not introduce this method - it would be hardly ever used by anyone except Linkifier https://codereview.chromium.org/2644753002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/platform/utilities.js:178: if (this.codePointAt(this.length - rightHalf - 1) >= 0x10000) { Wow, something is going on here! Do you know why we need to check for codepoints?
Comments addressed https://codereview.chromium.org/2644753002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/2644753002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:424: if (trimHashes) { On 2017/04/17 17:57:40, lushnikov wrote: > I don't quite follow what's going on here. I though we need to truncate hash > first and then truncate middle if needed. That is correct. I'm trying to calculate the indices of the first and last hashes in the string: `hello [hash of length 20] world [hash of length 20]` I'd like to find indices that let us show: `hello [first 7 chars of 1st hash]...[last 7 chars of hash 2nd hash]` Perhaps I can write it more clearly with Math.max/min's? https://codereview.chromium.org/2644753002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:459: static selectionOffsetToOriginalTextOffset(node, offset) { On 2017/04/17 17:57:40, lushnikov wrote: > you don't need this method anymore - let's inline Done, however inlining this still feels a little sketchy to me, since it is dependent upon Linkifier L446. But console is the only client and there are tests covering this case, so I'm fine with moving it and adding a comment. https://codereview.chromium.org/2644753002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:471: static originalNodeText(node) { On 2017/04/17 17:57:40, lushnikov wrote: > untruncatedNodeText Done. https://codereview.chromium.org/2644753002/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/platform/utilities.js (right): https://codereview.chromium.org/2644753002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/platform/utilities.js:173: String.prototype.trimMiddleIndices = function(maxLength) { On 2017/04/17 17:57:40, lushnikov wrote: > Let's not introduce this method - it would be hardly ever used by anyone except > Linkifier Done. https://codereview.chromium.org/2644753002/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/platform/utilities.js:178: if (this.codePointAt(this.length - rightHalf - 1) >= 0x10000) { On 2017/04/17 17:57:40, lushnikov wrote: > Wow, something is going on here! Do you know why we need to check for > codepoints? Emoji characters have length > 1, and we do not break them up!
The latest patch set should hopefully make _setTrimmedText() be easier to read. Please take another look.
overall looks good, but we need to fix the _setTrimmedText function. I just recalled that we have a method called TextUtils.splitStringByRegexes - it might come handy in your case https://codereview.chromium.org/2644753002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/2644753002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:394: Components.Linkifier._setTrimmedText(link, text, false /* trimHashes */, maxLength); can we always trim hashes? https://codereview.chromium.org/2644753002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:430: hiddenIndices[1] = Math.max(hiddenIndices[1], hashIndices[1]); in case of: 1. text.length < maxLength 2. text has a cache-busting in the very end this will truncate link from center up to the very end (where the hash is located). https://codereview.chromium.org/2644753002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:437: link.createTextChild(text.substr(0, leftIndex)); nit: text.substring(0, leftIndex) we try to use substring to avoid confusion between substr and substring https://codereview.chromium.org/2644753002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:440: link.createTextChild(text.substr(rightIndex, text.length - rightIndex)); nit: text.substring(rightIndex)
Finding out about splitByRegexes: https://media.giphy.com/media/26ufdipQqU2lhNA4g/giphy.gif The general purpose regex splitter makes it cleaner now. Thanks for the suggestion, and the patience. Ready for another look https://codereview.chromium.org/2644753002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/components/Linkifier.js (right): https://codereview.chromium.org/2644753002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:394: Components.Linkifier._setTrimmedText(link, text, false /* trimHashes */, maxLength); On 2017/04/22 01:11:48, lushnikov wrote: > can we always trim hashes? I'm all for consistency! Tried it out and it seems fine. https://codereview.chromium.org/2644753002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:430: hiddenIndices[1] = Math.max(hiddenIndices[1], hashIndices[1]); On 2017/04/22 01:11:48, lushnikov wrote: > in case of: > 1. text.length < maxLength > 2. text has a cache-busting in the very end > > this will truncate link from center up to the very end (where the hash is > located). I see, so my "max/min from the center" approach does not work correctly. The new approach using splitByRegexes should address this. I've added a test for the short url with hashes case. https://codereview.chromium.org/2644753002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:437: link.createTextChild(text.substr(0, leftIndex)); On 2017/04/22 01:11:48, lushnikov wrote: > nit: text.substring(0, leftIndex) > > we try to use substring to avoid confusion between substr and substring Done. https://codereview.chromium.org/2644753002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/components/Linkifier.js:440: link.createTextChild(text.substr(rightIndex, text.length - rightIndex)); On 2017/04/22 01:11:48, lushnikov wrote: > nit: text.substring(rightIndex) Done.
luoe@chromium.org changed reviewers: + einbinder@chromium.org
Added a test case to simulate when a text selection ends on an empty element (e.g. expand triangle icon) Switched from the zero-width space character back to the ellipsis. While this latest patch does not support untruncating copy when the "..." is highlighted due to search, it is a rare case and we can fix the UI.highlightRangesWithClass().
lgtm https://codereview.chromium.org/2644753002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2644753002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:463: if (container.textContent.length === 0 && container.nodeType !== Node.TEXT_NODE) { this is unrelated to the patch - let's have this separately!
Thanks for the reviews https://codereview.chromium.org/2644753002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js (right): https://codereview.chromium.org/2644753002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/devtools/front_end/console/ConsoleViewport.js:463: if (container.textContent.length === 0 && container.nodeType !== Node.TEXT_NODE) { On 2017/05/01 22:52:26, lushnikov wrote: > this is unrelated to the patch - let's have this separately! Sounds good, I've made a separate bug with the repro: crbug.com/717286
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 allada@chromium.org, lushnikov@chromium.org Link to the patchset: https://codereview.chromium.org/2644753002/#ps320001 (title: "ac")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1493685710852260,
"parent_rev": "5a6b4429ac4a17ad658d76531b5a25fc01ae1018", "commit_rev":
"9bee9f62d8507206938b746e3987ee2c87d6190b"}
Message was sent while issue was closed.
Description was changed from ========== DevTools: untruncate links on copy During exporti or copying of selected text, links that were truncated will be replaced with their original, un-trimmed text. BUG=505177 ========== to ========== DevTools: untruncate links on copy During exporti or copying of selected text, links that were truncated will be replaced with their original, un-trimmed text. BUG=505177 Review-Url: https://codereview.chromium.org/2644753002 Cr-Commit-Position: refs/heads/master@{#468514} Committed: https://chromium.googlesource.com/chromium/src/+/9bee9f62d8507206938b746e3987... ==========
Message was sent while issue was closed.
Committed patchset #17 (id:320001) as https://chromium.googlesource.com/chromium/src/+/9bee9f62d8507206938b746e3987... |
