Bug 104970 – Web Inspector: Search by selection

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

[johnjbarton]

Browsers provide word-selection upon double click on text. They also provide arbitrary contiguous text selection with the mouse. We can leverage this feature in search by loading the search query with the text selection when ever it is available.

The user experience here is similar to other editors, for example Sublime Text.  For me this feature is the most important one missing from Web Inspector's editor.

Patch to follow.

[johnjbarton]

Created attachment 179365 [details]
Patch

[johnjbarton]

I'll add a test if the patch would be accepted with one.

[Pavel Feldman]

Comment on attachment 179365 [details]
Patch

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

Looks good, style nits inline.

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:246
> +    syncToSelection: function()

Rename to _syncToSelection (make private)

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:249
> +        if (selection.type === "Range" && !selection.isCollapsed)

We typically do if (selection.rangeCount)

[Vsevolod Vlasov]

Comment on attachment 179365 [details]
Patch

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

Pavel, will platform selection always work properly in text editor?

> Source/WebCore/inspector/front-end/AdvancedSearchController.js:250
> +            this._search.value = selection.toString();

selection.toString().replace(/\r?\n.*/, "") to make sure search query does not have any line feeds.

[johnjbarton]

(In reply to comment #3)
> (From update of attachment 179365 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179365&action=review
> 
> Looks good, style nits inline.
> 
> > Source/WebCore/inspector/front-end/AdvancedSearchController.js:246
> > +    syncToSelection: function()
> 
> Rename to _syncToSelection (make private)

Just so I understand:  private-per-file not per-class ? The function is call is across classes.

[Pavel Feldman]

> > Rename to _syncToSelection (make private)
> 
> Just so I understand:  private-per-file not per-class ? The function is call is across classes.

Yes. Private is per class. It is not a compiler annotation, it is rather an indication that you can rename it within file. We also are supposed to follow the file per public class policy.

[johnjbarton]

Created attachment 179518 [details]
Patch

[johnjbarton]

Since the syncToSelection() is across classes I left it unprefixed with _.

The test here is minimal, only the positive single word case.  Multiple line selection works -- puts the first line in the input -- but I don't know how to set the selection to test this.  Random content in the selection buffer should also be tested in principle, but in my experience so far WebInspector already clears selections. Anyway the test file is set up for more cases if we encounter problems.

[Vsevolod Vlasov]

Comment on attachment 179518 [details]
Patch

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

> LayoutTests/inspector/editor/text-editor-selection-to-search.html:21
> +            InspectorTest.showScriptSource("edit-me.js", didShowScriptSource);

Weird indent.

[Vsevolod Vlasov]

(In reply to comment #8)
> Since the syncToSelection() is across classes I left it unprefixed with _.
> 
> The test here is minimal, only the positive single word case.  Multiple line selection works -- puts the first line in the input -- but I don't know how to set the selection to test this. 

You could change from (or to) line number that is passed to the TextRange constructor.

[Vsevolod Vlasov]

Comment on attachment 179518 [details]
Patch

Please fix indent before landing.

[johnjbarton]

Created attachment 179766 [details]
Patch

[johnjbarton]

(In reply to comment #11)
> (From update of attachment 179518 [details])
> Please fix indent before landing.

Done, please submit to commit queue.

[WebKit Review Bot]

Comment on attachment 179766 [details]
Patch

Clearing flags on attachment: 179766

Committed r137928: <http://trac.webkit.org/changeset/137928>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[johnjbarton]

(In reply to comment #4)
> (From update of attachment 179365 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179365&action=review
> 
> Pavel, will platform selection always work properly in text editor?

I know this bug is closed, I'm just storing some observations here regarding your question.

When in the Sources panel on a breakpoint I can select a Scope Variable and hit search, the selected scope variable is the query.  

Yay! This is behavior we want.