Bug 138198 - Web Inspector: TextEditor search doesn't work after editing contents
Summary: Web Inspector: TextEditor search doesn't work after editing contents
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-10-29 16:57 PDT by Joseph Pecoraro
Modified: 2014-11-03 17:03 PST (History)
5 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (2.57 KB, patch)
2014-10-29 17:03 PDT, Joseph Pecoraro
timothy: review+
timothy: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (2.22 KB, patch)
2014-11-03 14:43 PST, Joseph Pecoraro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2014-10-29 16:57:59 PDT
* SUMMARY
TextEditor search doesn't work after editing a file. Stops early. Wrong number of results.

* STEPS TO REPRODUCE
1. Inspect <http://bogojoker.com/shell/>
2. Select Stylesheet Resource "print.css"
3. Search for "font" => 10 results
4. Add a blank link at line 29
5. Clear search, and search for "font" again
    => 8 results stopping the count at the edit, expected 10 results.

* NOTES
SourceCodeTextEditor is using its backend search instead of frontend (CodeMirror) search when the editor has edits.
Comment 1 Radar WebKit Bug Importer 2014-10-29 16:58:11 PDT
<rdar://problem/18818999>
Comment 2 Joseph Pecoraro 2014-10-29 17:03:12 PDT
Created attachment 240638 [details]
[PATCH] Proposed Fix
Comment 3 Joseph Pecoraro 2014-10-29 17:04:24 PDT
Heck if we want maybe we should always have CodeMirror perform searching inside of TextEditor. is there a reason we might not want that?
Comment 4 Timothy Hatcher 2014-11-03 11:29:51 PST
Comment on attachment 240638 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:232
> +    get hasEdits()

This is something I would make a function. The rule I follow: if it is a question or action, it is a function. Otherwise, a property.

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:241
> +        var undoableChanges = this._codeMirror.historySize().undo;
> +        if (!undoableChanges)
> +            return false;
> +        if (undoableChanges > 1)
> +            return true;
> +
> +        // Don't claim that we have edits if we have only one change which was pretty-printing.
> +        return !this._formatted;

I wonder if we should use this._codeMirror.isClean() here? Does it give the right result? Maybe we need some well placed markClean() calls to make that work though. (We do a markClean in TextEditor.set string().)
Comment 5 Joseph Pecoraro 2014-11-03 11:54:50 PST
Comment on attachment 240638 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:232
>> +    get hasEdits()
> 
> This is something I would make a function. The rule I follow: if it is a question or action, it is a function. Otherwise, a property.

Agreed, I'll make the change.

>> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:241
>> +        return !this._formatted;
> 
> I wonder if we should use this._codeMirror.isClean() here? Does it give the right result? Maybe we need some well placed markClean() calls to make that work though. (We do a markClean in TextEditor.set string().)

Oh nice, I had forgotten about that. That would certainly be more convenient. I'll give that a try.
Comment 6 Joseph Pecoraro 2014-11-03 14:43:04 PST
Created attachment 240877 [details]
[PATCH] Proposed Fix
Comment 7 Timothy Hatcher 2014-11-03 14:50:13 PST
Comment on attachment 240877 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:818
> +        return !this._codeMirror.isClean();

The previous patch accounted for pretty-printing, does that still need to happen?
Comment 8 Joseph Pecoraro 2014-11-03 16:26:34 PST
Comment on attachment 240877 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/TextEditor.js:818
>> +        return !this._codeMirror.isClean();
> 
> The previous patch accounted for pretty-printing, does that still need to happen?

Turns out we already don't use the customPerformSearch if the content is formatted.

Right now, hasEdits() returns false if the editor has been pretty printed. I think that is fine unless we want to tweak that later.
Comment 9 WebKit Commit Bot 2014-11-03 17:03:37 PST
Comment on attachment 240877 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 240877

Committed r175497: <http://trac.webkit.org/changeset/175497>
Comment 10 WebKit Commit Bot 2014-11-03 17:03:42 PST
All reviewed patches have been landed.  Closing bug.