Bug 150876 – Web Inspector: Pretty print falsely triggers on some JS that wasn't minified

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Created attachment 264763 [details]
[Animated GIF] Bug

https://codemirror.net/lib/codemirror.js is falsely detected as minified.

[Radar WebKit Bug Importer]

<rdar://problem/23387755>

[Joseph Pecoraro]

There is a line that is 1957 characters long:

    var extendingChars = /..../

We pretty print any file that has a line with more than 500 characters:

    WebInspector.SourceCodeTextEditor.AutoFormatMinimumLineLength = 500;

Would you want to change this heuristic?

[Timothy Hatcher]

I mentioned to Nikita that we might want to change the heuristic to look at average line length, so a single long line won't trigger this.

[Nikita Vasilyev]

I looked at minified JS and CSS on:
- https://cdnjs.com/libraries
- https://developers.google.com/speed/libraries/

The majority of them have the very first line longer than 500 characters.

Some have a license comment at the top:

jQuery, 1 line license:
https://ajax.googleapis.com/ajax/libs/jquery/2.1.4/jquery.min.js
/*! jQuery v2.1.4 | (c) 2005, 2015 jQuery Foundation, Inc. | jquery.org/license */


Angular, 5 lines license:
https://ajax.googleapis.com/ajax/libs/angularjs/1.3.15/angular.min.js
/*
 AngularJS v1.3.15
 (c) 2010-2014 Google, Inc. http://angularjs.org
 License: MIT
*/


React, 12 lines license:
https://cdnjs.cloudflare.com/ajax/libs/react/0.14.2/react.min.js
/**
 * React v0.14.2
 *
 * Copyright 2013-2015, Facebook, Inc.
 * All rights reserved.
 *
 * This source code is licensed under the BSD-style license found in the
 * LICENSE file in the root directory of this source tree. An additional grant
 * of patent rights can be found in the PATENTS file in the same directory.
 *
 */


In case of codemirror.js, only the 8298th line is long. 8298th! There are 8297 lines before it that are less than 500 characters long, but we loop over 8298 lines before we exit.

I think, we should only check the first 50 lines. If those are <500 chars, it's very unlikely that this file is minified.

[Nikita Vasilyev]

Created attachment 264817 [details]
Patch

[Joseph Pecoraro]

By this criteria, inspecting the inspector when minified would not pretty print it, as its first 51 lines are below the longest line being ~274 characters:

> /*
>  * Copyright (C) 2007-2014 Apple Inc. All rights reserved.
>  * Copyright (C) 2008 Matt Lilek. All rights reserved.
>  * Copyright (C) 2008-2009 Anthony Ricaud <[e-mail]>
>  * Copyright (C) 2009-2010 Joseph Pecoraro. All rights reserved.
>  * Copyright (C) 2009-2011 Google Inc. All rights reserved.
>  * Copyright (C) 2009 280 North Inc. All Rights Reserved.
>  * Copyright (C) 2010 Nikita Vasilyev. All rights reserved.
>  * Copyright (C) 2011 Brian Grinstead All rights reserved.
>  * Copyright (C) 2013 Matt Holden <[e-mail]>
>  * Copyright (C) 2013 Samsung Electronics. All rights reserved.
>  * Copyright (C) 2013 Seokju Kwon ([e-mail])
>  * Copyright (C) 2013 Adobe Systems Inc. All rights reserved.
>  * Copyright (C) 2013-2014 University of Washington. All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>  * are met:
>  * 1. Redistributions of source code must retain the above copyright
>  *    notice, this list of conditions and the following disclaimer.
>  * 2. Redistributions in binary form must reproduce the above copyright
>  *    notice, this list of conditions and the following disclaimer in the
>  *    documentation and/or other materials provided with the distribution.
>  *
>  * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
>  * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
>  * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
>  * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
>  * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
>  * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>  * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
>  * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
>  * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
>  * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
>  * THE POSSIBILITY OF SUCH DAMAGE.
>  */
> const WebInspector={};if(!window.Symbol){window.Symbol=function(string)
> {return string;}}
> if(!window.InspectorFrontendHost){WebInspector.InspectorFrontendHostStub=function()
> {};WebInspector.InspectorFrontendHostStub.prototype={ initializeWebSocket:function(url)
> {var socket=new WebSocket(url);socket.addEventListener("open",socketReady.bind(this));function socketReady()
> {this._socket=socket;this._socket.addEventListener("message",function(message){InspectorBackend.dispatch(message.data);});this._socket.addEventListener("error",function(error){console.error(error);});this._sendPendingMessagesToBackendIfNeeded();}},bringToFront:function()
> {this._windowVisible=true;},closeWindow:function()
> {this._windowVisible=false;},requestSetDockSide:function(side)
> {InspectorFrontendAPI.setDockSide(side);},setAttachedWindowHeight:function(height)
> {},setAttachedWindowWidth:function(width)
> {},setToolbarHeight:function(width)
> {},startWindowDrag:function()
> {},moveWindowBy:function(x,y)
> {},loaded:function()
> {},localizedStringsURL:function()

[Nikita Vasilyev]

The minification we use is rather unorthodox.

We place an opening curly brace on a new line for function declarations, which is an uncommon JS coding style.
Combined with jsmin.py not stripping that line break, we have such a strange minified JS:

> const WebInspector={};if(!window.Symbol){window.Symbol=function(string)
> {return string;}}
> if(!window.InspectorFrontendHost){WebInspector.InspectorFrontendHostStub=function()
> {};WebInspector.InspectorFrontendHostStub.prototype={ initializeWebSocket:function(url)

Google Closure compiler, Uglify JS and Babel would all strip that line break before "{".

[Nikita Vasilyev]

What if we use whitespace to non-whitespace ratio for this heuristic?

Demo: http://jsbin.com/guxoye/edit?output

It properly detects minified and unminified jQuery, React, Angular, and even Web Inspector's Main.js.

[Timothy Hatcher]

I like that idea. Would you add a character limit to avoid walking the whole resource? Maybe we should exclude comments in the calculation too, so license headers don't count.

[Nikita Vasilyev]

(In reply to comment #9)
> I like that idea. Would you add a character limit to avoid walking the whole
> resource?

It looks at first 5000 characters in the worse case and it exits earlier if whitespace/non-whitespace ratio drops below 5%.

(You can click on "JavaScript" tab on http://jsbin.com/guxoye/edit?output to show the code)

> Maybe we should exclude comments in the calculation too, so
> license headers don't count.

And strings. I'd rather not do any parsing until we find a case when the current heuristic fails.

[Blaze Burg]

Comment on attachment 264817 [details]
Patch

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

I thought we decided to look at whitespace ratio?

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:393
> +            var lineCount = 0;

nit; let

[Nikita Vasilyev]

Created attachment 264875 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 264875 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:402
> +        for (let i = 0, size = Math.min(5000, content.length); i < size; i++) {

This is a case where const variables would really help. They can be local to this function.

    const autoFormatMaxCharactersToCheck = 5000;
    const autoFormatWhitespaceRatio = 0.1; // 10%

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:404
> +            if (char === " " || char === "\n" || char === "\t")

I wonder if tabs should count as more than 1. It is tough to say. In our files where we have 4 spaces for leading whitespace, if we replaced those with tabs we would drop our whitespace count by 75%. I wonder if that might push some files over the edge.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:412
> +            if (i >= 500) {
> +                ratio = whiteSpaceCount / i;
> +                if (ratio < 0.05)
> +                    return true;
>              }
>          }

Seems you could just do this at the end. Instead of doing it 4500 times for an unminified file.

> Source/WebInspectorUI/UserInterface/Views/SourceCodeTextEditor.js:414
> +        return ratio < 0.1;

In the loop this is 5% but at the end it is 10%. I'm not sure the "in the loop" is helping for a mere max 5000 characters.

[WebKit Commit Bot]

Comment on attachment 264875 [details]
Patch

Clearing flags on attachment: 264875

Committed r192076: <http://trac.webkit.org/changeset/192076>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nikita Vasilyev]

Created attachment 287131 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 287131 [details]
Patch

Whoops, this should've be attached to bug 161159.