Bug 108947 - Web Inspector: show whitespace characters in DTE
Summary: Web Inspector: show whitespace characters in DTE
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Andrey Lushnikov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-05 08:06 PST by Andrey Lushnikov
Modified: 2013-02-09 01:59 PST (History)
13 users (show)

See Also:


Attachments
Patch (17.33 KB, patch)
2013-02-05 08:44 PST, Andrey Lushnikov
no flags Details | Formatted Diff | Diff
Patch (17.22 KB, patch)
2013-02-06 02:09 PST, Andrey Lushnikov
no flags Details | Formatted Diff | Diff
Patch (17.82 KB, patch)
2013-02-06 04:32 PST, Andrey Lushnikov
no flags Details | Formatted Diff | Diff
Patch (17.78 KB, patch)
2013-02-06 07:48 PST, Andrey Lushnikov
no flags Details | Formatted Diff | Diff
Patch (17.79 KB, patch)
2013-02-07 04:07 PST, Andrey Lushnikov
no flags Details | Formatted Diff | Diff
Patch (17.48 KB, patch)
2013-02-07 07:06 PST, Andrey Lushnikov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andrey Lushnikov 2013-02-05 08:06:58 PST
Make an option to show whitespace characters in DefaultTextEditor.
Comment 1 Andrey Lushnikov 2013-02-05 08:44:52 PST
Created attachment 186632 [details]
Patch
Comment 2 Andrey Adaikin 2013-02-05 09:25:27 PST
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)?
Comment 3 WebKit Review Bot 2013-02-05 09:28:22 PST
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
Comment 4 Build Bot 2013-02-05 09:31:41 PST
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
Comment 5 Pavel Feldman 2013-02-05 10:04:04 PST
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.
Comment 6 Build Bot 2013-02-05 10:16:30 PST
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
Comment 7 Alexander Pavlov (apavlov) 2013-02-05 12:02:24 PST
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
Comment 8 Andrey Lushnikov 2013-02-06 02:05:24 PST
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
Comment 9 Andrey Lushnikov 2013-02-06 02:09:54 PST
Created attachment 186797 [details]
Patch
Comment 10 WebKit Review Bot 2013-02-06 03:13:10 PST
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
Comment 11 Pavel Feldman 2013-02-06 04:05:29 PST
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.
Comment 12 Andrey Lushnikov 2013-02-06 04:32:04 PST
Created attachment 186825 [details]
Patch
Comment 13 WebKit Review Bot 2013-02-06 06:46:51 PST
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
Comment 14 Andrey Lushnikov 2013-02-06 07:48:46 PST
Created attachment 186855 [details]
Patch
Comment 15 Build Bot 2013-02-06 10:30:58 PST
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
Comment 16 Build Bot 2013-02-06 11:18:23 PST
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
Comment 17 Build Bot 2013-02-06 19:02:39 PST
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
Comment 18 Andrey Lushnikov 2013-02-07 04:07:38 PST
Created attachment 187054 [details]
Patch
Comment 19 Build Bot 2013-02-07 05:29:37 PST
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
Comment 20 Build Bot 2013-02-07 06:28:45 PST
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
Comment 21 Andrey Lushnikov 2013-02-07 07:06:02 PST
Created attachment 187100 [details]
Patch
Comment 22 WebKit Review Bot 2013-02-09 01:59:47 PST
Comment on attachment 187100 [details]
Patch

Clearing flags on attachment: 187100

Committed r142351: <http://trac.webkit.org/changeset/142351>
Comment 23 WebKit Review Bot 2013-02-09 01:59:52 PST
All reviewed patches have been landed.  Closing bug.