Bug 108947 – Web Inspector: show whitespace characters in DTE

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Lushnikov]

Make an option to show whitespace characters in DefaultTextEditor.

[Andrey Lushnikov]

Created attachment 186632 [details]
Patch

[Andrey Adaikin]

Isn't it getting too heavy having everything in one file?
Do we want to extract a "core editor"?
Will this new functionality automatically work in other editor impls (code mirror)?

[WebKit Review Bot]

Comment on attachment 186632 [details]
Patch

Attachment 186632 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16379671

New failing tests:
inspector/editor/text-editor-show-whitespaces.html

[Build Bot]

Comment on attachment 186632 [details]
Patch

Attachment 186632 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16369823

New failing tests:
inspector/editor/text-editor-show-whitespaces.html

[Pavel Feldman]

Comment on attachment 186632 [details]
Patch

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

> Source/WebCore/English.lproj/localizedStrings.js:393
> +localizedStrings["Show whitespaces"] = "Show whitespaces";

Show whitespace

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1411
> +        if (!visibleTo) {

no {} around single line block

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2022
> +            var cssClass = ranges[i].token ? "webkit-" + ranges[i].token : "";

Will this work for tab character? Note that it can be of different size.

[Build Bot]

Comment on attachment 186632 [details]
Patch

Attachment 186632 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16366882

New failing tests:
inspector/editor/text-editor-show-whitespaces.html

[Alexander Pavlov (apavlov)]

Comment on attachment 186632 [details]
Patch

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1395
> +WebInspector.TextEditorMainPanel._ConsequtiveSpaces = {

_Consecutive...

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2022
>> +            var cssClass = ranges[i].token ? "webkit-" + ranges[i].token : "";
> 
> Will this work for tab character? Note that it can be of different size.

Tabs are also typically rendered differently (usually using the RIGHT ARROW character, perhaps ࢐)

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2027
> +                for(var whiteSpaceLength = 16; whiteSpaceLength > 0; whiteSpaceLength >>= 1)

missing whitespace after "for"

> Source/WebCore/inspector/front-end/Settings.js:113
> +    this.showWhitespacesInEditor = this.createSetting("showWhitespacesInEditor", false);

showWhitespaceInEditor

[Andrey Lushnikov]

Comment on attachment 186632 [details]
Patch

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

>> Source/WebCore/English.lproj/localizedStrings.js:393
>> +localizedStrings["Show whitespaces"] = "Show whitespaces";
> 
> Show whitespace

fixed everywhere

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1395
>> +WebInspector.TextEditorMainPanel._ConsequtiveSpaces = {
> 
> _Consecutive...

renamed into _ConsecutiveWhitespaceChars

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1411
>> +        if (!visibleTo) {
> 
> no {} around single line block

fixed

>>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2022
>>> +            var cssClass = ranges[i].token ? "webkit-" + ranges[i].token : "";
>> 
>> Will this work for tab character? Note that it can be of different size.
> 
> Tabs are also typically rendered differently (usually using the RIGHT ARROW character, perhaps ࢐)

no, this won't work for tab chars. Tabs have to be separated into a separate token "tabs" by lexer, which explicitly mean changes in re2js grammar. I believe this change is massive enough to worth a separate patch.

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2027
>> +                for(var whiteSpaceLength = 16; whiteSpaceLength > 0; whiteSpaceLength >>= 1)
> 
> missing whitespace after "for"

fixed

>> Source/WebCore/inspector/front-end/Settings.js:113
>> +    this.showWhitespacesInEditor = this.createSetting("showWhitespacesInEditor", false);
> 
> showWhitespaceInEditor

fixed

[Andrey Lushnikov]

Created attachment 186797 [details]
Patch

[WebKit Review Bot]

Comment on attachment 186797 [details]
Patch

Attachment 186797 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16392170

New failing tests:
inspector/editor/text-editor-show-whitespace.html

[Pavel Feldman]

Comment on attachment 186797 [details]
Patch

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2022
> +            var cssClass = ranges[i].token ? "webkit-" + ranges[i].token : "";

You should probably extract a method: as is, it has more logic than the rest of range rendering.

[Andrey Lushnikov]

Created attachment 186825 [details]
Patch

[WebKit Review Bot]

Comment on attachment 186825 [details]
Patch

Rejecting attachment 186825 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 186825, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
fuzz 2 (offset -1 lines).
patching file Source/WebCore/inspector/front-end/inspectorSyntaxHighlight.css
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/inspector/editor/text-editor-show-whitespace-expected.txt
patching file LayoutTests/inspector/editor/text-editor-show-whitespace.html

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Pavel Feldman']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16390296

[Andrey Lushnikov]

Created attachment 186855 [details]
Patch

[Build Bot]

Comment on attachment 186855 [details]
Patch

Attachment 186855 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16401204

New failing tests:
inspector/editor/text-editor-show-whitespace.html

[Build Bot]

Comment on attachment 186855 [details]
Patch

Attachment 186855 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16400215

New failing tests:
inspector/editor/text-editor-show-whitespace.html

[Build Bot]

Comment on attachment 186855 [details]
Patch

Attachment 186855 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16405014

New failing tests:
inspector/editor/text-editor-show-whitespace.html
fast/js/array-sort-modifying-tostring.html

[Andrey Lushnikov]

Created attachment 187054 [details]
Patch

[Build Bot]

Comment on attachment 187054 [details]
Patch

Attachment 187054 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16445139

New failing tests:
http/tests/cache/cached-main-resource.html
inspector/editor/text-editor-show-whitespace.html

[Build Bot]

Comment on attachment 187054 [details]
Patch

Attachment 187054 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16443141

New failing tests:
inspector/editor/text-editor-show-whitespace.html

[Andrey Lushnikov]

Created attachment 187100 [details]
Patch

[WebKit Review Bot]

Comment on attachment 187100 [details]
Patch

Clearing flags on attachment: 187100

Committed r142351: <http://trac.webkit.org/changeset/142351>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.