Bug 125695 – Web Inspector: provide an abstraction for CodeMirror's TextMarker

• Component: Web Inspector
• Status: RESOLVED FIXED

[Antoine Quint]

When we introduced support for editing colors in SourceCodeTextEditor, we started using CodeMirror's TextMarker type directly. This class should have no knowledge of CodeMirror though, so we should provide an abstraction for this type.

[Antoine Quint]

Created attachment 219172 [details]
Patch

[Timothy Hatcher]

Comment on attachment 219172 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/CodeMirrorColorEditingController.js:90
> +        this._range = new WebInspector.TextRange(this._range.startLine, this._range.startColumn, this._range.endLine, this._range.startColumn + text.length);

Shouldn't this take into account any "\n" in text when making the endLine and endColumn? Or just assert no "\n" is allowed in the text and use startLine for endLine.

[Antoine Quint]

Created attachment 219174 [details]
Patch for landing

[Antoine Quint]

(In reply to comment #2)
> (From update of attachment 219172 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219172&action=review
> 
> > Source/WebInspectorUI/UserInterface/CodeMirrorColorEditingController.js:90
> > +        this._range = new WebInspector.TextRange(this._range.startLine, this._range.startColumn, this._range.endLine, this._range.startColumn + text.length);
> 
> Shouldn't this take into account any "\n" in text when making the endLine and endColumn? Or just assert no "\n" is allowed in the text and use startLine for endLine.

For the record, this was fixed in the commit after discussion on IRC.

[WebKit Commit Bot]

Comment on attachment 219174 [details]
Patch for landing

Clearing flags on attachment: 219174

Committed r160552: <http://trac.webkit.org/changeset/160552>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Could this have caused:

file:///Volumes/Data/Build/Debug/WebInspectorUI.framework/Resources/External/CodeMirror/codemirror.js:1446:39: JS ERROR: TypeError: undefined is not a function (evaluating 'marker.find()')
file:///Volumes/Data/Build/Debug/WebInspectorUI.framework/Resources/Color.js:97:133: JS ERROR: TypeError: undefined is not a function (evaluating 'WebInspector.Color.Nicknames[nickname].concat(1)')

No marker.find?

[Joseph Pecoraro]

(In reply to comment #7)
> Could this have caused:
> 
> file:///Volumes/Data/Build/Debug/WebInspectorUI.framework/Resources/External/CodeMirror/codemirror.js:1446:39: JS ERROR: TypeError: undefined is not a function (evaluating 'marker.find()')
> file:///Volumes/Data/Build/Debug/WebInspectorUI.framework/Resources/Color.js:97:133: JS ERROR: TypeError: undefined is not a function (evaluating 'WebInspector.Color.Nicknames[nickname].concat(1)')
> 
> No marker.find?

Follow-up fix on:
<https://webkit.org/b/125724> Web Inspector: Exception: TypeError: undefined is not a function (evaluating 'marker.find()')

[Antoine Quint]

(In reply to comment #8)
> (In reply to comment #7)
> > Could this have caused:
> > 
> > file:///Volumes/Data/Build/Debug/WebInspectorUI.framework/Resources/External/CodeMirror/codemirror.js:1446:39: JS ERROR: TypeError: undefined is not a function (evaluating 'marker.find()')
> > file:///Volumes/Data/Build/Debug/WebInspectorUI.framework/Resources/Color.js:97:133: JS ERROR: TypeError: undefined is not a function (evaluating 'WebInspector.Color.Nicknames[nickname].concat(1)')
> > 
> > No marker.find?
> 
> Follow-up fix on:
> <https://webkit.org/b/125724> Web Inspector: Exception: TypeError: undefined is not a function (evaluating 'marker.find()')

There shouldn't be a marker.find(), there's a "range" property on WebInspector.TextMarker.

[Antoine Quint]

This patch introduced another regression: https://bugs.webkit.org/show_bug.cgi?id=128965, Web Inspector: editing a color in the Styles sidebar using the color picker only works once for a given color.

[Radar WebKit Bug Importer]

<rdar://problem/16096034>