Summary: | Web Inspector: [Elements] Syntax-highlight the "Edit as HTML" editor | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexander Pavlov (apavlov) <apavlov> | ||||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Alexander Pavlov (apavlov) <apavlov> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | apavlov, bburg, dglazkov, keishi, loislo, pfeldman, pmuellr, vsevik, web-inspector-bugs, webkit.review.bot, yurys | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | All | ||||||||||||||||||
Bug Depends on: | 113798 | ||||||||||||||||||
Bug Blocks: | |||||||||||||||||||
Attachments: |
|
Description
Alexander Pavlov (apavlov)
2013-03-26 06:35:58 PDT
Created attachment 195081 [details]
Patch
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 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 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. Created attachment 195289 [details]
Patch
Created attachment 195325 [details]
Patch
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 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 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 Created attachment 195345 [details]
Patch
The changes to the patch warrant a fresh review
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. Created attachment 195563 [details]
Patch
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 Created attachment 195566 [details]
Patch for landing
Comment on attachment 195566 [details] Patch for landing Clearing flags on attachment: 195566 Committed r147117: <http://trac.webkit.org/changeset/147117> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 113798 |