Bug 138198 – Web Inspector: TextEditor search doesn't work after editing contents

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Joseph Pecoraro]

* 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.

[Radar WebKit Bug Importer]

<rdar://problem/18818999>

[Joseph Pecoraro]

Created attachment 240638 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Heck if we want maybe we should always have CodeMirror perform searching inside of TextEditor. is there a reason we might not want that?

[Timothy Hatcher]

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().)

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 240877 [details]
[PATCH] Proposed Fix

[Timothy Hatcher]

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?

[Joseph Pecoraro]

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.

[WebKit Commit Bot]

Comment on attachment 240877 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 240877

Committed r175497: <http://trac.webkit.org/changeset/175497>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.