| Summary: | Web Inspector: replace the Web Inspector editor with CodeMirror | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Pavel Feldman <pfeldman> | ||||||||||||||||||
| Component: | Web Inspector (Deprecated) | Assignee: | Jan Keromnes <janx> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | apavlov, bweinstein, dglazkov, dsam2912, janx, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, vivekgalatage, webkit.review.bot, yurys | ||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||
| OS: | All | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Pavel Feldman
2012-07-31 09:43:09 PDT
+1 I am game for it. Obvious +1 I'd gladly provide support on the codemirror side. Created attachment 157382 [details]
Patch
I integrated CodeMirror2 as a devtools experiment, replacing DefaultTextEditor when the experiment is active. The above patch adds support for: - Open/closing files - Editing files - Search/replacing (1) - Saving files - Setting breakpoints (UI only) - Add/get/removeAttribute for any line (2) Features that are not yet supported by this implementation: - Communication with WebInspector's breakpoint manager - Showing and hiding message bubbles (e.g. error messages in the code) - Displaying popovers (i.e. when stopped on a breakpoint, showing the value for an expression on hover) - Populated context menus Please tell me if you think I forgot an important editor feature in the lists above. Potential performance issues: (1) The search functionality implemented by WebInspector doesn't scale well with CodeMirror on big files (> 10KLOC). It could also be a good idea to use CodeMirror's own search algorithm instead. (2) WebInspector seems to check for the attribute "breakpoint" on every line of a file. The current implementation of getAttribute gets the actual CodeMirror line handle, and I have the impression that accessing it for every line deoptimizes what the viewport gains. Maybe the list of current breakpoints can be provided by the breakpoint manager instead. Comment on attachment 157382 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157382&action=review Good stuff, gives a really good clue on what is missing and what not. > Source/WebCore/WebCore.gypi:6258 > + 'inspector/front-end/css.js', we'll need to fix the build so that we could store cm files under a separate folder (as we do to uglify) > Source/WebCore/WebCore.gypi:6259 > + 'inspector/front-end/javascript.js', Given the size of these, they are likely to slow down the load time. We should probably deploy them separately (as we do with worker scripts) and load them using XHR in case the experiment is turned on. > Source/WebCore/WebCore.vcproj/WebCore.vcproj:75425 > + RelativePath="..\inspector\front-end\codemirror.js" you are missing the mode files. > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:50 > + onGutterClick: function(cm, line) { I guess you should add this later along with the proper breakpoint marker. You should fire corresponding event to embedder, decoration will be added by the sourceframe later. > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:58 > + extraKeys: { "Ctrl-S": this._commitEditing.bind(this) } Also might require a more careful treatment. > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:76 > + this._codeMirror.setOption("mode", mimeType); Nuke this line. > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:89 > + console.log('called setReadOnly', arguments); Remove logging here and below. > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:90 > + this._readOnly = readOnly; Do you really need this state? You can synchronously query option for that. > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:123 > + this._codeMirror.setCursor({line: lineNumber}); should be line: lineNumber, ch: 0 > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:125 > + this._codeMirror.scrollTo(coords.x, coords.y); Doesn't cm do that on its own? If not, lets upstream it. Going through coords for such a simple operation sounds strange. > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:164 > + var mark = this._codeMirror.markText({line:lineNumber,ch:0},{line:lineNumber,ch:line.length},"CodeMirror-searching"); Please format arrays and objects as in WebKit. > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:175 > + console.log('called freeCachedElements', arguments); We should just remove this (call it on willHide in DefaultTextViewer) > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:197 > + console.log('called beginUpdates', arguments); We call these around the calls into decorations. We should make it private and call it within addDecoration and such in the default editor. > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:232 > + this._delegate.beforeTextChanged(); beforeTextChanged expects to be called on the original state of the model. I can look into how we can workaround it. Worst case we would need an additional event. > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:243 > + this._delegate.commitEditing(); We should probably move this to the embedder... > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:256 > + this._codeMirror.setCursor({line:lineNumber, ch:0}); What is the difference between the revealLine and scrollToLine? > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:266 > + return null; This should not be hard to mock. > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:314 > + return this._toRange({line:0, ch:0}, this._codeMirror.posFromIndex(this._codeMirror.getValue().length)); getValue is expensive in codemirror. You probably want something like {line: lineCount - 1, ch: getLine(lineCount - 1).length} > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:369 > + if (handle.attributes !== undefined) delete handle.attributes[name]; You can't compare to undefined in JavaScript (not a good style), you do id (handle.attributes) Also, delete should go on the next line. > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:375 > + if (this._content) { This hack is no longer needed. > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:386 > + _textChanged: function(event) This does not need to be used. Created attachment 157394 [details]
Patch
Comment on attachment 157394 [details] Patch Attachment 157394 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13460463 New failing tests: http/tests/inspector/appcache/appcache-iframe-manifests.html http/tests/inspector/injected-script-for-origin.html http/tests/inspector/extensions-network-redirect.html http/tests/inspector/change-iframe-src.html http/tests/inspector/modify-cross-domain-rule.html http/tests/inspector/resource-har-conversion.html http/tests/inspector/console-resource-errors.html http/tests/inspector/extensions-headers.html http/tests/inspector-enabled/injected-script-discard.html http/tests/inspector/console-cross-origin-iframe-logging.html http/tests/inspector-enabled/contentSecurityPolicy-blocks-setTimeout.html http/tests/inspector/extensions-useragent.html fast/canvas/webgl/shader-precision-format.html http/tests/inspector/console-cd-completions.html http/tests/inspector/compiler-script-mapping.html http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval.html http/tests/inspector/compiler-source-mapping-debug.html http/tests/inspector/web-socket-frame-error.html http/tests/inspector/console-xhr-logging.html http/tests/inspector/console-xhr-logging-async.html http/tests/inspector/console-cd.html http/tests/inspector/resource-har-headers.html http/tests/inspector/network-preflight-options.html http/tests/inspector/inspect-iframe-from-different-domain.html http/tests/inspector-enabled/console-log-before-frame-navigation.html http/tests/inspector-enabled/dom-storage-open.html http/tests/inspector/resource-parameters.html http/tests/inspector-enabled/database-open.html Created attachment 157399 [details]
Archive of layout-test-results from gce-cr-linux-08
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
(In reply to comment #5) > we'll need to fix the build so that we could store cm files under a separate folder (as we do to uglify) > > Given the size of these, they are likely to slow down the load time. We should probably deploy them separately (as we do with worker scripts) and load them using XHR in case the experiment is turned on. Good idea! > I guess you should add this later along with the proper breakpoint marker. You should fire corresponding event to embedder, decoration will be added by the sourceframe later. Removed in patch above. > > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:58 > > + extraKeys: { "Ctrl-S": this._commitEditing.bind(this) } > > Also might require a more careful treatment. I tested it and it seems to work, it has the same behavior as DefaultTextEditor. Did you spot any particular problem? > > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:76 > > + this._codeMirror.setOption("mode", mimeType); > > Nuke this line. It is needed for proper language-specific highlighting. > > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:89 > > + console.log('called setReadOnly', arguments); > > Remove logging here and below. Removed all logging instrumentation in patch above. > > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:90 > > + this._readOnly = readOnly; > > Do you really need this state? You can synchronously query option for that. Indeed the state is not needed, I'll remove it. > > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:123 > > + this._codeMirror.setCursor({line: lineNumber}); > > should be line: lineNumber, ch: 0 `ch` defaults to 0 but if you prefer the explicit version I'll change it. > > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:125 > > + this._codeMirror.scrollTo(coords.x, coords.y); > > Doesn't cm do that on its own? If not, lets upstream it. Going through coords for such a simple operation sounds strange. I think we might have to upstream that one. I'll remove it when it's done. > > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:164 > > + var mark = this._codeMirror.markText({line:lineNumber,ch:0},{line:lineNumber,ch:line.length},"CodeMirror-searching"); > > Please format arrays and objects as in WebKit. Will do. > > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:175 > > + console.log('called freeCachedElements', arguments); > > We should just remove this (call it on willHide in DefaultTextViewer) ditto > > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:197 > > + console.log('called beginUpdates', arguments); > > We call these around the calls into decorations. We should make it private and call it within addDecoration and such in the default editor. ditto > > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:232 > > + this._delegate.beforeTextChanged(); > > beforeTextChanged expects to be called on the original state of the model. I can look into how we can workaround it. Worst case we would need an additional event. I'll wait for your conclusion then. > > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:243 > > + this._delegate.commitEditing(); > > We should probably move this to the embedder... I'm not sure what you mean. This calls back to the SourceFrame to commit a working copy, should it work differently? > > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:256 > > + this._codeMirror.setCursor({line:lineNumber, ch:0}); > > What is the difference between the revealLine and scrollToLine? Not sure anymore, I'll investigate this. > > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:266 > > + return null; > > This should not be hard to mock. I'll implement this in the next patch. > > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:314 > > + return this._toRange({line:0, ch:0}, this._codeMirror.posFromIndex(this._codeMirror.getValue().length)); > > getValue is expensive in codemirror. You probably want something like {line: lineCount - 1, ch: getLine(lineCount - 1).length} I'll change it to your version. > > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:369 > > + if (handle.attributes !== undefined) delete handle.attributes[name]; > > You can't compare to undefined in JavaScript (not a good style), you do id (handle.attributes) > > Also, delete should go on the next line. Ok. > > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:375 > > + if (this._content) { > > This hack is no longer needed. > > > Source/WebCore/inspector/front-end/CodeMirrorTextEditor.js:386 > > + _textChanged: function(event) > > This does not need to be used. Removing the hack. Created attachment 157408 [details]
Patch
Comment on attachment 157408 [details] Patch Attachment 157408 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13470172 New failing tests: http/tests/inspector/appcache/appcache-iframe-manifests.html http/tests/inspector/injected-script-for-origin.html http/tests/inspector/extensions-network-redirect.html http/tests/inspector/change-iframe-src.html http/tests/inspector/modify-cross-domain-rule.html http/tests/inspector/resource-har-conversion.html http/tests/inspector/console-resource-errors.html http/tests/inspector/extensions-headers.html http/tests/inspector-enabled/injected-script-discard.html http/tests/inspector/console-cross-origin-iframe-logging.html http/tests/inspector-enabled/contentSecurityPolicy-blocks-setTimeout.html http/tests/inspector/extensions-useragent.html fast/canvas/webgl/shader-precision-format.html http/tests/inspector/console-cd-completions.html http/tests/inspector/compiler-script-mapping.html http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval.html http/tests/inspector/compiler-source-mapping-debug.html http/tests/inspector/web-socket-frame-error.html http/tests/inspector/console-xhr-logging.html http/tests/inspector/console-xhr-logging-async.html http/tests/inspector/console-cd.html http/tests/inspector/resource-har-headers.html http/tests/inspector/network-preflight-options.html http/tests/inspector/inspect-iframe-from-different-domain.html http/tests/inspector-enabled/console-log-before-frame-navigation.html http/tests/inspector-enabled/dom-storage-open.html http/tests/inspector/resource-parameters.html http/tests/inspector-enabled/database-open.html Created attachment 157412 [details]
Archive of layout-test-results from gce-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 157413 [details]
Patch
Looks like the test bot is failing, all tests pass on my machine. New patch with stripped down functional prototype, without logging instrumentation nor breakpoint UI, and with the changes you asked for in comment #5. Missing features and performance issues described in comment #4 still apply. Still not sure about revealLine and scrollToLine. I'll let you work on deployment and refactoring. Comment on attachment 157413 [details] Patch Attachment 157413 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13464361 New failing tests: http/tests/inspector/appcache/appcache-iframe-manifests.html http/tests/inspector/injected-script-for-origin.html http/tests/inspector/extensions-network-redirect.html http/tests/inspector/change-iframe-src.html http/tests/inspector/modify-cross-domain-rule.html http/tests/inspector/resource-har-conversion.html http/tests/inspector/console-resource-errors.html http/tests/inspector/extensions-headers.html http/tests/inspector-enabled/injected-script-discard.html http/tests/inspector/console-cross-origin-iframe-logging.html http/tests/inspector-enabled/contentSecurityPolicy-blocks-setTimeout.html http/tests/inspector/extensions-useragent.html fast/canvas/webgl/shader-precision-format.html http/tests/inspector/console-cd-completions.html http/tests/inspector/compiler-script-mapping.html http/tests/inspector-enabled/contentSecurityPolicy-blocks-setInterval.html http/tests/inspector/compiler-source-mapping-debug.html http/tests/inspector/web-socket-frame-error.html http/tests/inspector/console-xhr-logging.html http/tests/inspector/console-xhr-logging-async.html http/tests/inspector/console-cd.html http/tests/inspector/resource-har-headers.html http/tests/inspector/network-preflight-options.html http/tests/inspector/inspect-iframe-from-different-domain.html http/tests/inspector-enabled/console-log-before-frame-navigation.html http/tests/inspector-enabled/dom-storage-open.html http/tests/inspector/resource-parameters.html http/tests/inspector-enabled/database-open.html Created attachment 157486 [details]
Archive of layout-test-results from gce-cr-linux-06
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Created attachment 157497 [details]
[Patch] stripped away the code we are unsure of
Comment on attachment 157497 [details] [Patch] stripped away the code we are unsure of Clearing flags on attachment: 157497 Committed r125201: <http://trac.webkit.org/changeset/125201> All reviewed patches have been landed. Closing bug. |