Bug 111700 – Rename 'KURL::elidedString' and inspector's 'String::trimMiddle' for clarity.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mike West]

In https://bugs.webkit.org/show_bug.cgi?id=111133#c21, Darin suggested that the current name for the process of shortening a URL's string representation was poorly chosen. We should rename it for clarity.

othermaciej suggested "centerEllipsizeToLength" or "centerTruncate".  dydx liked "ellipsize". I'll put something together here, and rename the various callsites.

[Mike West]

For consistency, I'll adjust the 'trimMiddle' method that the Inspector adds to the String prototype as well.

[Mike West]

Created attachment 191958 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 191958 [details]
Patch

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

A few drive by comments below.

I'm unsure if trimming URLs at this level is the right approach. Ideally, Web Inspector would show a shorter URL, but still allow for copying the full one to pasteboard, or possibly expanding inline. Also, an ellipsis in the middle of a 1024 character string is extremely hard to spot, so it could be highlighted in the UI by using a prominent style.

> Source/WebCore/Modules/websockets/WebSocket.cpp:218
> +        scriptExecutionContext()->addConsoleMessage(JSMessageSource, ErrorMessageLevel, "Invalid url for WebSocket " + m_url.stringCenterEllipsizedToLength());

All these call sites look a bit weird without a length argument. But I don't have a better suggestion.

> Source/WebCore/platform/KURL.cpp:1928
> +String KURL::stringCenterEllipsizedToLength(unsigned length) const

Since the result of this function is for viewing only, perhaps we should convert IDNA host names?

> Source/WebCore/platform/KURL.cpp:1933
> +    return string().left(length / 2 - 1) + "..." + string().right(length / 2 - 2);

It would be nice to use an ellipsis (U+2026), not three periods.

[Mike West]

(In reply to comment #3)
> (From update of attachment 191958 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191958&action=review
> 
> A few drive by comments below.

Thanks, much appreciated.

> I'm unsure if trimming URLs at this level is the right approach. Ideally, Web Inspector would show a shorter URL, but still allow for copying the full one to pasteboard, or possibly expanding inline. Also, an ellipsis in the middle of a 1024 character string is extremely hard to spot, so it could be highlighted in the UI by using a prominent style.

It's a little complex. Here's a bit of background for context:

Web Inspector does currently show a shorter, linkified URL for all those longer than ~150 characters. If the URL is shorter than 1k, the full URL is linked, and the short URL is displayed.

Eric noted in bug 111133 that we're potentially doing a _lot_ of copying in order to produce these strings, especially in the case of 'data:' URLs. These URLs especially cause problems when being dumped to the command line or to console.app.

That discussion lead to the patch in 111133 trimming the underlying string before passing it into the inspector.

> > Source/WebCore/Modules/websockets/WebSocket.cpp:218
> > +        scriptExecutionContext()->addConsoleMessage(JSMessageSource, ErrorMessageLevel, "Invalid url for WebSocket " + m_url.stringCenterEllipsizedToLength());
> 
> All these call sites look a bit weird without a length argument. But I don't have a better suggestion.

I considered adding a static property on Console to pass in here, but that seemed like pure overhead. I'd like to avoid boilerplate to the extent possible.

What might make more sense is to move the method to Console, and to do the trimming there. Something like 'Console::centerEllipsizeForConsole(const String&)'. Would that be more clear?
 
> > Source/WebCore/platform/KURL.cpp:1928
> > +String KURL::stringCenterEllipsizedToLength(unsigned length) const
> 
> Since the result of this function is for viewing only, perhaps we should convert IDNA host names?

I've no idea if we do anything useful with those, honestly. If we don't, it would be a good idea to teach the console about them when it goes through linkifying URLs.

> > Source/WebCore/platform/KURL.cpp:1933
> > +    return string().left(length / 2 - 1) + "..." + string().right(length / 2 - 2);
> 
> It would be nice to use an ellipsis (U+2026), not three periods.

I considered that; is it safe to throw unicode out to the console?

[Mike West]

+jochen, eric: Since you reviewed the original patch, perhaps you have opinions about this renaming discussion? :)

[Mike West]

Created attachment 203337 [details]
Patch

[Mike West]

Thanks for the review, Darin. Apologies for the belated rebase/upload. Assuming this passes the bots, I'll land it tonight.

[WebKit Commit Bot]

Comment on attachment 203337 [details]
Patch

Clearing flags on attachment: 203337

Committed r150957: <http://trac.webkit.org/changeset/150957>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.