Bug 125695 - Web Inspector: provide an abstraction for CodeMirror's TextMarker
Summary: Web Inspector: provide an abstraction for CodeMirror's TextMarker
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: Antoine Quint
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-12-13 10:10 PST by Antoine Quint
Modified: 2014-02-18 03:26 PST (History)
6 users (show)

See Also:


Attachments
Patch (16.73 KB, patch)
2013-12-13 10:28 PST, Antoine Quint
no flags Details | Formatted Diff | Diff
Patch for landing (16.90 KB, patch)
2013-12-13 10:54 PST, Antoine Quint
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Quint 2013-12-13 10:10:09 PST
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.
Comment 1 Antoine Quint 2013-12-13 10:28:59 PST
Created attachment 219172 [details]
Patch
Comment 2 Timothy Hatcher 2013-12-13 10:38:03 PST
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.
Comment 3 Antoine Quint 2013-12-13 10:54:15 PST
Created attachment 219174 [details]
Patch for landing
Comment 4 Antoine Quint 2013-12-13 10:55:42 PST
(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.
Comment 5 WebKit Commit Bot 2013-12-13 11:29:42 PST
Comment on attachment 219174 [details]
Patch for landing

Clearing flags on attachment: 219174

Committed r160552: <http://trac.webkit.org/changeset/160552>
Comment 6 WebKit Commit Bot 2013-12-13 11:29:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Joseph Pecoraro 2013-12-13 13:02:53 PST
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?
Comment 8 Joseph Pecoraro 2013-12-13 20:20:40 PST
(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()')
Comment 9 Antoine Quint 2013-12-14 04:06:18 PST
(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.
Comment 10 Antoine Quint 2014-02-18 03:26:25 PST
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.
Comment 11 Radar WebKit Bug Importer 2014-02-18 03:26:44 PST
<rdar://problem/16096034>