Summary: | Web Inspector: create a CodeMirrorEditingController superclass | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||||
Component: | Web Inspector | Assignee: | Antoine Quint <graouts> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, graouts, joepeck, timothy, webkit-bug-importer | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 119686 | ||||||||||
Attachments: |
|
Description
Antoine Quint
2014-02-20 02:37:52 PST
Created attachment 224743 [details]
Patch
Comment on attachment 224743 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224743&action=review > Source/WebInspectorUI/UserInterface/CodeMirrorEditingController.js:106 > + get cssClassName() > + { > + return ""; > + }, Should this have a // Implemented by subclasses. comment? > Source/WebInspectorUI/UserInterface/Geometry.js:104 > + for (var i = 1; i < rects.length; ++i) for (... of ...) > Source/WebInspectorUI/UserInterface/HoverMenu.js:36 > + this._svg = this._element.appendChild(document.createElementNS("http://www.w3.org/2000/svg", "svg")); _svg is pretty generic. Can this have a better name? _outlineElement? > Source/WebInspectorUI/UserInterface/HoverMenu.js:166 > + _drawTwoNonOverlappingLines: function(rects) Crazy town! But I like it. > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:1312 > + if (marker.type === WebInspector.TextMarker.Type.Color) { > + var color = this._editingController.value; > + if (!color || !color.valid) { Most of this code is now generic (editableMarker), but this is specific to color. Does this not apply to all editable markers? (In reply to comment #3) > (From update of attachment 224743 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=224743&action=review > > > Source/WebInspectorUI/UserInterface/CodeMirrorEditingController.js:106 > > + get cssClassName() > > + { > > + return ""; > > + }, > > Should this have a // Implemented by subclasses. comment? Good point, I'll add it. > > Source/WebInspectorUI/UserInterface/Geometry.js:104 > > + for (var i = 1; i < rects.length; ++i) > > for (... of ...) We start at i = 1 in this case. > > Source/WebInspectorUI/UserInterface/HoverMenu.js:36 > > + this._svg = this._element.appendChild(document.createElementNS("http://www.w3.org/2000/svg", "svg")); > > _svg is pretty generic. Can this have a better name? _outlineElement? Sure! > > Source/WebInspectorUI/UserInterface/HoverMenu.js:166 > > + _drawTwoNonOverlappingLines: function(rects) > > Crazy town! But I like it. This matches DataDetectors UI on OS X and generally looks pretty good. > > Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:1312 > > + if (marker.type === WebInspector.TextMarker.Type.Color) { > > + var color = this._editingController.value; > > + if (!color || !color.valid) { > > Most of this code is now generic (editableMarker), but this is specific to color. Does this not apply to all editable markers? I suppose we ought to make it so that a CodeMirrorEditingController value ought to implement a .valid getter to check that its value is valid so that this code could be generic. I'll rework that in an updated patch if that sounds like a good idea to you. Comment on attachment 224743 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=224743&action=review >>> Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js:1312 >>> + if (!color || !color.valid) { >> >> Most of this code is now generic (editableMarker), but this is specific to color. Does this not apply to all editable markers? > > I suppose we ought to make it so that a CodeMirrorEditingController value ought to implement a .valid getter to check that its value is valid so that this code could be generic. I'll rework that in an updated patch if that sounds like a good idea to you. This is fine as-is. We can change it later of other editors need it. Created attachment 224761 [details]
Patch for landing
Comment on attachment 224761 [details] Patch for landing Rejecting attachment 224761 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 224761, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: nk #1 succeeded at 445 (offset 1 line). patching file Source/WebInspectorUI/UserInterface/Main.js patching file Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.css patching file Source/WebInspectorUI/UserInterface/SourceCodeTextEditor.js patching file Source/WebInspectorUI/UserInterface/TextEditor.js patching file Source/WebInspectorUI/UserInterface/TextMarker.js Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: http://webkit-queues.appspot.com/results/4763882363027456 Created attachment 224766 [details]
Patch for landing
Comment on attachment 224766 [details] Patch for landing Clearing flags on attachment: 224766 Committed r164436: <http://trac.webkit.org/changeset/164436> All reviewed patches have been landed. Closing bug. |