Long URLs in error messages should be shortened .... (10MB of data).... XVpLTMiPjwvbGk+Ci0tPgoKIAo=' because the document's frame is sandboxed and the 'allow-scripts' permission is not set. I'm sure we have lots of these. :( I suspect this is affecting performance of some of our data: url tests (including perf tests).
We're already visually shortening the messages in the console. I don't see any reason that we couldn't shorten the actual string when it's over some arbitrarily large limit. We'd lose the ability to click through to see the page that generated the error, but for this specific case that's probably not a terrible loss. 1. Can you point me at a test result that's currently shockingly large? 2. What's the most performant mechanism to shorten a string? :)
Oh, the log that I'm seeing is not on the console, it's to stdout/stderr. :) I see this when running DRT manually.
Right, we don't shorten the string at all, we just look for URLs and elide them when they're over some length (~150 characters, I think). It's perfectly reasonable to do that at the string level for some higher limit (1k?). I'll throw up a patch as soon as I remember how to export methods from KURL for use in WebCore. :/
(In reply to comment #3) > I'll throw up a patch as soon as I remember how to export methods from KURL for use in WebCore. :/ Scratch that. I'll upload a patch as soon as I realize that I'm an idiot who can't properly read linker errors. :)
I think we need to do a somewhat careful audit of all the times we're copying (see bug 111134) or converting urls to strings. URLs can literally be megabytes in length in WebKit. :)
(I'm not suggesting we need to do such as part of this bug. More just complaining in public.)
Created attachment 190916 [details] Patch
(In reply to comment #7) > Created an attachment (id=190916) [details] > Patch Feedback welcome, Eric and Pavel. If this seems like a workable approach, I'll go through the rest of the console messages that contain URLs and add them to the patch. I'm a bit worried about the substring and concat approach in terms of performance. I'd appreciate alternative suggestions.
Comment on attachment 190916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=190916&action=review > Source/WebCore/bindings/ScriptControllerBase.cpp:45 > + m_frame->document()->addConsoleMessage(HTMLMessageSource, ErrorMessageLevel, "Blocked script execution in '" + m_frame->document()->url().elidedString() + "' because the document's frame is sandboxed and the 'allow-scripts' permission is not set."); I wonder if we should be ASSERTing in addConsoleMessage for certain error levels that we're not above a certain size threshold.
Going to leave this for abarth or pfeldman to review. I'm a bit too tired to be a good reviewer right now. :)
(In reply to comment #9) > (From update of attachment 190916 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=190916&action=review > > > Source/WebCore/bindings/ScriptControllerBase.cpp:45 > > + m_frame->document()->addConsoleMessage(HTMLMessageSource, ErrorMessageLevel, "Blocked script execution in '" + m_frame->document()->url().elidedString() + "' because the document's frame is sandboxed and the 'allow-scripts' permission is not set."); > > I wonder if we should be ASSERTing in addConsoleMessage for certain error levels that we're not above a certain size threshold. That probably makes sense for some reasonably high amount. 10k? 100k? Pavel will certainly have opinions here. :)
Jochen expressed some doubt about putting this on KURL. Adding him in case his doubt grows into an r-.
Comment on attachment 190916 [details] Patch LGTM, but I'll give jochen and others a chance to comment.
Comment on attachment 190916 [details] Patch ok
(In reply to comment #12) > Jochen expressed some doubt about putting this on KURL. Adding him in case his doubt grows into an r-. I thought you were talking about a case where the inspector is passed the URL. But you're passing a string, so you'll have to do the conversion earlier, so it's ok
(In reply to comment #15) > (In reply to comment #12) > > Jochen expressed some doubt about putting this on KURL. Adding him in case his doubt grows into an r-. > > I thought you were talking about a case where the inspector is passed the URL. But you're passing a string, so you'll have to do the conversion earlier, so it's ok Cool. I'll add a few of the more obvious cases of copying full, potentially long URLs and land this on Monday. We can take a closer look at the non-obvious bits in bug 111134.
Created attachment 191115 [details] Patch
Comment on attachment 191115 [details] Patch Clearing r?, carrying over Jochen's r+. I'll wait until Monday to land this.
Comment on attachment 191115 [details] Patch Clearing flags on attachment: 191115 Committed r144607: <http://trac.webkit.org/changeset/144607>
All reviewed patches have been landed. Closing bug.
Comment on attachment 191115 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=191115&action=review > Source/WebCore/ChangeLog:28 > + * platform/KURL.cpp: > + (WebCore::KURL::elidedString): Added. > + * platform/KURL.h: > + An exciting new method that gives you the same result as string() > + for URLs less than 1k long, and elides the middle of URLs longer > + than 1k by replacing everything but the first and last 0.5k with > + "...". The name “elided string” does not seem good for this function. It’s nice that it’s a short name. But the string is not “elided”, only the middle of the string is.
(In reply to comment #21) > (From update of attachment 191115 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=191115&action=review > > > Source/WebCore/ChangeLog:28 > > + * platform/KURL.cpp: > > + (WebCore::KURL::elidedString): Added. > > + * platform/KURL.h: > > + An exciting new method that gives you the same result as string() > > + for URLs less than 1k long, and elides the middle of URLs longer > > + than 1k by replacing everything but the first and last 0.5k with > > + "...". > > The name “elided string” does not seem good for this function. > > It’s nice that it’s a short name. But the string is not “elided”, only the middle of the string is. Hrm. I thought the name fit well conceptually, but I'm happy to change it for clarity. "ellipsisedString"? I'm not sure "ellipsised" is a word. :) "stringTrimmedTo1k"? It looks like DevTools has a 'trimMiddle' function[1], perhaps following that convention would be reasonable? [1]: http://trac.webkit.org/browser/trunk/Source/WebCore/inspector/front-end/utilities.js#L117
Split out renaming into https://bugs.webkit.org/show_bug.cgi?id=111700.