Bug 105523

Summary: Web Inspector: highlight occurences of word in DefaultTextEditor
Product: WebKit Reporter: Andrey Lushnikov <lushnikov>
Component: Web Inspector (Deprecated)Assignee: Andrey Lushnikov <lushnikov>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, buildbot, dglazkov, keishi, loislo, pfeldman, pmuellr, rniwa, vsevik, web-inspector-bugs, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
word-highlight-screenshot
none
Patch
none
feature demonstration
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Andrey Lushnikov 2012-12-20 03:14:57 PST
It would be convenient to highlight all occurrences of some identifier in a javascript code when user selects one of them (via text selection). 
This is done per-word in a Sublime Text 2 (i.e. it highlights word occurrences in text)
Comment 1 Andrey Lushnikov 2012-12-20 03:26:51 PST
Created attachment 180314 [details]
Patch
Comment 2 Pavel Feldman 2012-12-20 04:22:19 PST
Comment on attachment 180314 [details]
Patch

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1344
> +    function checkSelectedIdentifier(event) {

This should be a prototype method that you bind and pass into listeners.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1346
> +        if (selection.isCollapsed)

We already have selection stored in a variable in mainPanel

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1353
> +        // The following conditions will:

WebKit strongly advises not using comments. We only use them when absolutely necessary.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1369
> +            if (className === "webkit-javascript-ident") {

Do not use "javascript" word in editor. What about css variables?

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1375
> +    this.element.addEventListener("mousemove", checkSelectedIdentifier.bind(this), false);

"click"

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1377
> +    this.element.addEventListener("keyup", checkSelectedIdentifier.bind(this), false);

we already track text mutations, why bothering about the keyboard events?

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1391
> +        var elements = document.querySelectorAll(".webkit-javascript-ident");

Just repaintAll

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:1401
> +            var elements = document.querySelectorAll(".webkit-selected-identifier");

ditto

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:2151
> +        if (span.className === "webkit-javascript-ident" && this._selectedIdentifier && this._selectedIdentifier === content)

javascript word again...
Comment 3 Andrey Lushnikov 2013-01-15 07:07:41 PST
Clarifying title: highlighting not only identifier occurrences, but any word occurrences.
Comment 4 Andrey Lushnikov 2013-01-15 07:15:50 PST
Created attachment 182761 [details]
Patch
Comment 5 Pavel Feldman 2013-01-15 07:28:08 PST
Comment on attachment 182761 [details]
Patch

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

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:3046
> +            return this._deselect();

Although I would love to act alike, we don't do that.

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:3068
> +    _select: function(selectedWord)

this naming is misleading - it does not mutate selection.
Comment 6 Andrey Lushnikov 2013-01-15 07:43:32 PST
Created attachment 182771 [details]
Patch
Comment 7 WebKit Review Bot 2013-01-15 08:28:36 PST
Comment on attachment 182771 [details]
Patch

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

New failing tests:
inspector/editor/text-editor-highlight-token.html
Comment 8 Build Bot 2013-01-15 09:10:37 PST
Comment on attachment 182771 [details]
Patch

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

New failing tests:
inspector/editor/text-editor-highlight-token.html
Comment 9 Build Bot 2013-01-16 00:44:50 PST
Comment on attachment 182771 [details]
Patch

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

New failing tests:
inspector/editor/text-editor-highlight-token.html
Comment 10 Andrey Lushnikov 2013-01-16 02:50:00 PST
Created attachment 182948 [details]
Patch
Comment 11 Build Bot 2013-01-16 03:17:02 PST
Comment on attachment 182948 [details]
Patch

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

New failing tests:
inspector/editor/text-editor-highlight-token.html
Comment 12 Andrey Lushnikov 2013-01-16 04:24:08 PST
Created attachment 182958 [details]
word-highlight-screenshot

What's happening when you select some word
Comment 13 Andrey Lushnikov 2013-01-16 07:36:47 PST
Created attachment 182979 [details]
Patch
Comment 14 Andrey Lushnikov 2013-01-16 07:39:34 PST
Created attachment 182981 [details]
feature demonstration
Comment 15 Pavel Feldman 2013-01-16 08:50:34 PST
Comment on attachment 182979 [details]
Patch

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

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

if (!this._muteSelectionListener)

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:3055
> +        if (selectedText == this._selectedWord)

===

> Source/WebCore/inspector/front-end/DefaultTextEditor.js:3077
> +        this._mainPanel.highlightRegex(this._activeRegex, "text-editor-token-highlight")

this._muteSelectionListener = true;
this._mainPanel.highlightRegex(this._activeRegex, "text-editor-token-highlight");
delete this._muteSelectionListener;
Comment 16 Andrey Lushnikov 2013-01-21 02:45:44 PST
Comment on attachment 182979 [details]
Patch

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

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:3043
>> +        if (!range) {
> 
> if (!this._muteSelectionListener)

_muteSelectionListener doesn't work here (see below).

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:3055
>> +        if (selectedText == this._selectedWord)
> 
> ===

my bad; fixed.

>> Source/WebCore/inspector/front-end/DefaultTextEditor.js:3077
>> +        this._mainPanel.highlightRegex(this._activeRegex, "text-editor-token-highlight")
> 
> this._muteSelectionListener = true;
> this._mainPanel.highlightRegex(this._activeRegex, "text-editor-token-highlight");
> delete this._muteSelectionListener;

this doesn't work because events happen on the next scheduled frame.
Comment 17 Andrey Lushnikov 2013-01-23 07:50:16 PST
Created attachment 184235 [details]
Patch
Comment 18 Pavel Feldman 2013-01-23 08:00:14 PST
Comment on attachment 184235 [details]
Patch

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

> LayoutTests/inspector/editor/text-editor-highlight-token.html:35
> +        setTimeout(step2);

why setTimeout ?
Comment 19 Andrey Lushnikov 2013-01-23 08:39:34 PST
Comment on attachment 184235 [details]
Patch

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

>> LayoutTests/inspector/editor/text-editor-highlight-token.html:35
>> +        setTimeout(step2);
> 
> why setTimeout ?

That's because setSelection triggers "document.selectionChange" listeners which gonna be executed on next frame (and token highlighter is one of those listeners).
Comment 20 Build Bot 2013-01-23 19:06:25 PST
Comment on attachment 184235 [details]
Patch

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

New failing tests:
svg/as-image/img-preserveAspectRatio-support-2.html
Comment 21 Andrey Lushnikov 2013-01-24 00:55:24 PST
Created attachment 184431 [details]
Patch
Comment 22 Pavel Feldman 2013-01-24 04:46:58 PST
Comment on attachment 184431 [details]
Patch

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

> LayoutTests/inspector/editor/text-editor-highlight-token.html:45
> +    setTimeout(step1);

Bump it above step1. You should use selection listener here.
Comment 23 Andrey Lushnikov 2013-01-24 07:20:19 PST
Created attachment 184487 [details]
Patch
Comment 24 WebKit Review Bot 2013-01-24 08:12:23 PST
Comment on attachment 184487 [details]
Patch

Rejecting attachment 184487 [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-01', 'apply-attachment', '--no-update', '--non-interactive', 184487, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
atch: **** Can't create file /tmp/ppQlOiba : No space left on device
fatal: pathspec 'LayoutTests/inspector/editor/text-editor-highlight-token-expected.txt' did not match any files
Failed to git add LayoutTests/inspector/editor/text-editor-highlight-token-expected.txt. at /mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply line 448.

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

Full output: http://queues.webkit.org/results/16097229
Comment 25 Andrey Lushnikov 2013-01-24 08:16:16 PST
Created attachment 184503 [details]
Patch
Comment 26 Andrey Lushnikov 2013-01-24 08:16:48 PST
Comment on attachment 184503 [details]
Patch

re-submitted last patch
Comment 27 WebKit Review Bot 2013-01-24 08:48:46 PST
Comment on attachment 184503 [details]
Patch

Rejecting attachment 184503 [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-01', 'apply-attachment', '--no-update', '--non-interactive', 184503, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
atch: **** Can't create file /tmp/pp2s7yc3 : No space left on device
fatal: pathspec 'LayoutTests/inspector/editor/text-editor-highlight-token-expected.txt' did not match any files
Failed to git add LayoutTests/inspector/editor/text-editor-highlight-token-expected.txt. at /mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply line 448.

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

Full output: http://queues.webkit.org/results/16080734
Comment 28 Andrey Lushnikov 2013-01-25 01:30:04 PST
Created attachment 184696 [details]
Patch
Comment 29 WebKit Review Bot 2013-01-25 03:16:34 PST
Comment on attachment 184696 [details]
Patch

Rejecting attachment 184696 [details] from commit-queue.

New failing tests:
fast/frames/parser-append-subframe-count.html
Full output: http://queues.webkit.org/results/16121280
Comment 30 Andrey Lushnikov 2013-01-25 06:39:53 PST
Created attachment 184741 [details]
Patch
Comment 31 WebKit Review Bot 2013-01-25 08:23:30 PST
Comment on attachment 184741 [details]
Patch

Clearing flags on attachment: 184741

Committed r140828: <http://trac.webkit.org/changeset/140828>
Comment 32 WebKit Review Bot 2013-01-25 08:23:36 PST
All reviewed patches have been landed.  Closing bug.