Bug 113306 - Web Inspector: [Elements] Syntax-highlight the "Edit as HTML" editor
Summary: Web Inspector: [Elements] Syntax-highlight the "Edit as HTML" editor
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on: 113798
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-26 06:35 PDT by Alexander Pavlov (apavlov)
Modified: 2014-03-10 16:15 PDT (History)
11 users (show)

See Also:


Attachments
Patch (11.57 KB, patch)
2013-03-26 07:20 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-07 for chromium-linux-x86_64 (1.29 MB, application/zip)
2013-03-26 08:11 PDT, WebKit Review Bot
no flags Details
Patch (13.38 KB, patch)
2013-03-27 05:26 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (11.97 KB, patch)
2013-03-27 08:05 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (12.47 KB, patch)
2013-03-27 09:52 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (17.20 KB, patch)
2013-03-28 06:57 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch for landing (17.21 KB, patch)
2013-03-28 07:50 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2013-03-26 06:35:58 PDT
Currently, it can be very hard to sort out the spaghetti HTML markup in the editor. CodeMirror should work well for this.
Comment 1 Alexander Pavlov (apavlov) 2013-03-26 07:20:18 PDT
Created attachment 195081 [details]
Patch
Comment 2 WebKit Review Bot 2013-03-26 08:11:00 PDT
Comment on attachment 195081 [details]
Patch

Attachment 195081 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17144875

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/cache/subresource-failover-to-network.html
http/tests/inspector/change-iframe-src.html
http/tests/inspector/compiler-source-mapping-debug.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/extensions-useragent.html
http/tests/inspector/extensions-iframe-eval.html
http/tests/inspector-enabled/dynamic-scripts.html
http/tests/inspector/stylesheet-source-mapping.html
http/tests/inspector-enabled/console-exception-while-no-inspector.html
http/tests/inspector/console-cd-completions.html
http/tests/inspector/compiler-script-mapping.html
http/tests/inspector/modify-cross-domain-rule.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
Comment 3 WebKit Review Bot 2013-03-26 08:11:04 PDT
Created attachment 195088 [details]
Archive of layout-test-results from gce-cr-linux-07 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment 4 Pavel Feldman 2013-03-26 09:37:34 PDT
Comment on attachment 195081 [details]
Patch

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

> Source/WebCore/inspector/front-end/ElementsPanel.js:31
> +importScript("cm/codemirror.js");

codemirror is a part of a different module, it should be only inlined in CodeMirrorTextEditor.

> Source/WebCore/inspector/front-end/ElementsPanel.js:49
> +    this.registerRequiredCSS("cm/cmdevtools.css");

These should be loaded lazily.
Comment 5 Alexander Pavlov (apavlov) 2013-03-27 05:26:50 PDT
Created attachment 195289 [details]
Patch
Comment 6 Alexander Pavlov (apavlov) 2013-03-27 08:05:54 PDT
Created attachment 195325 [details]
Patch
Comment 7 Vsevolod Vlasov 2013-03-27 08:38:33 PDT
Comment on attachment 195325 [details]
Patch

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

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:569
> +        var treeElement = this.selectedTreeElement && this.selectedTreeElement.representedObject === node ? this.selectedTreeElement : this.getCachedTreeElement(node);

This asks for assert instead! :)

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:-1483
> -        if (this._htmlEditElement && WebInspector.isBeingEdited(this._htmlEditElement))

Let's keep _htmlEditElement here so that it is true that we need _codeMirror for tests only

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1532
> +        // this._codeMirror is needed for tests.

I think it's needed for commit as well, so this comment is not correct.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1555
> +            if (!this._codeMirror)

We could remove blur listener instead.
Comment 8 Vsevolod Vlasov 2013-03-27 08:45:19 PDT
Comment on attachment 195325 [details]
Patch

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

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1509
> +            if (event.keyIdentifier === "U+001B") {

Please add a comment explaining which key is that.
Comment 9 Pavel Feldman 2013-03-27 08:59:52 PDT
Comment on attachment 195325 [details]
Patch

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

> Source/WebCore/inspector/front-end/cm/cmdevtools.css:123
> +#elements-content .CodeMirror {

This should be a part of elementsPanel.css
Comment 10 Alexander Pavlov (apavlov) 2013-03-27 09:52:15 PDT
Created attachment 195345 [details]
Patch

The changes to the patch warrant a fresh review
Comment 11 Pavel Feldman 2013-03-28 05:23:42 PDT
Comment on attachment 195345 [details]
Patch

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

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:600
> +            var event = document.createEvent("Event");

We never use generic event dispatching.

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:-1542
> -        this._editing = WebInspector.startEditing(this._htmlEditElement, config);

I would still use this controller because it is useful and allows separation between your logic and underlying editor.
Comment 12 Alexander Pavlov (apavlov) 2013-03-28 06:57:58 PDT
Created attachment 195563 [details]
Patch
Comment 13 Vsevolod Vlasov 2013-03-28 07:48:18 PDT
Comment on attachment 195563 [details]
Patch

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

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1483
> +        if (this._htmlEditElement)

this._editing

> Source/WebCore/inspector/front-end/ElementsTreeOutline.js:1546
> +        config.setMultiline(initialValue, { name: "xml", htmlMode: true }, "web-inspector-html", true, true);

setMultilineOptions
Comment 14 Alexander Pavlov (apavlov) 2013-03-28 07:50:00 PDT
Created attachment 195566 [details]
Patch for landing
Comment 15 WebKit Review Bot 2013-03-28 08:13:23 PDT
Comment on attachment 195566 [details]
Patch for landing

Clearing flags on attachment: 195566

Committed r147117: <http://trac.webkit.org/changeset/147117>
Comment 16 WebKit Review Bot 2013-03-28 08:13:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 WebKit Review Bot 2013-04-02 09:32:21 PDT
Re-opened since this is blocked by bug 113798