Bug 166634 - Web Inspector: convert some DOMTreeManager methods to support promises and unify related test code
Summary: Web Inspector: convert some DOMTreeManager methods to support promises and un...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords:
Depends on: 161358 166523
Blocks:
  Show dependency treegraph
 
Reported: 2017-01-01 19:51 PST by BJ Burg
Modified: 2017-02-02 20:40 PST (History)
8 users (show)

See Also:


Attachments
Proposed Fix (depends on 166523, 161358) (147.04 KB, patch)
2017-01-01 22:05 PST, BJ Burg
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (1.02 MB, application/zip)
2017-01-01 23:22 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (1.67 MB, application/zip)
2017-01-01 23:35 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (2.15 MB, application/zip)
2017-01-02 00:35 PST, Build Bot
no flags Details
[PATCH] Rebased (148.39 KB, patch)
2017-01-05 14:39 PST, BJ Burg
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix v2 (153.60 KB, patch)
2017-01-05 15:27 PST, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2017-01-01 19:51:45 PST
Splitting from another bug, this got way out of hand.
Comment 1 BJ Burg 2017-01-01 22:05:23 PST
Created attachment 297877 [details]
Proposed Fix (depends on 166523, 161358)
Comment 2 Build Bot 2017-01-01 23:22:55 PST
Comment on attachment 297877 [details]
Proposed Fix (depends on 166523, 161358)

Attachment 297877 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2817256

New failing tests:
inspector/dom/customElementState.html
inspector/dom/domutilities-path-dump.html
inspector/dom/highlight-shape-outside-margin.html
inspector/dom/pseudo-element-dynamic.html
inspector/dom/domutilities-csspath.html
inspector/dom/getOuterHTML.html
inspector/dom/pseudo-element-static.html
http/tests/inspector/console/cross-domain-inspected-node-access.html
inspector/dom/hideHighlight.html
inspector/dom/shadowRootType.html
inspector/console/addInspectedNode.html
inspector/css/shadow-scoped-style.html
inspector/console/command-line-api.html
inspector/css/stylesheet-with-mutations.html
inspector/unit-tests/dom-test-helpers.html
inspector/dom/domutilities-xpath.html
inspector/dom/setOuterHTML.html
inspector/dom/csp-hash.html
inspector/dom/csp-big5-hash.html
inspector/dom/highlightNode.html
inspector/css/generate-css-rule-string.html
inspector/dom/template-content.html
Comment 3 Build Bot 2017-01-01 23:22:58 PST
Created attachment 297878 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 4 Build Bot 2017-01-01 23:35:10 PST
Comment on attachment 297877 [details]
Proposed Fix (depends on 166523, 161358)

Attachment 297877 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2817267

New failing tests:
inspector/css/generate-css-rule-string.html
inspector/css/stylesheet-with-mutations.html
inspector/dom/domutilities-path-dump.html
inspector/dom/shadowRootType.html
http/tests/inspector/console/cross-domain-inspected-node-access.html
inspector/unit-tests/dom-test-helpers.html
inspector/dom/customElementState.html
inspector/dom/highlight-shape-outside-margin.html
inspector/dom/domutilities-xpath.html
inspector/dom/setOuterHTML.html
inspector/dom/domutilities-csspath.html
inspector/dom/template-content.html
inspector/dom/hideHighlight.html
inspector/dom/getOuterHTML.html
inspector/dom/csp-hash.html
inspector/dom/pseudo-element-dynamic.html
inspector/dom/csp-big5-hash.html
inspector/dom/highlightNode.html
inspector/css/shadow-scoped-style.html
inspector/dom/pseudo-element-static.html
inspector/console/command-line-api.html
Comment 5 Build Bot 2017-01-01 23:35:14 PST
Created attachment 297879 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 6 Build Bot 2017-01-02 00:35:46 PST
Comment on attachment 297877 [details]
Proposed Fix (depends on 166523, 161358)

Attachment 297877 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2817341

New failing tests:
inspector/dom/customElementState.html
inspector/dom/domutilities-path-dump.html
inspector/dom/highlight-shape-outside-margin.html
inspector/dom/pseudo-element-dynamic.html
inspector/dom/domutilities-csspath.html
inspector/dom/getOuterHTML.html
inspector/dom/pseudo-element-static.html
http/tests/inspector/console/cross-domain-inspected-node-access.html
inspector/dom/hideHighlight.html
inspector/dom/shadowRootType.html
inspector/console/addInspectedNode.html
inspector/css/shadow-scoped-style.html
inspector/console/command-line-api.html
inspector/css/stylesheet-with-mutations.html
inspector/unit-tests/dom-test-helpers.html
inspector/dom/domutilities-xpath.html
inspector/dom/setOuterHTML.html
inspector/dom/csp-hash.html
inspector/dom/csp-big5-hash.html
inspector/dom/highlightNode.html
inspector/css/generate-css-rule-string.html
inspector/dom/template-content.html
Comment 7 Build Bot 2017-01-02 00:35:50 PST
Created attachment 297881 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 8 Yusuke Suzuki 2017-01-02 02:09:11 PST
What do you think of using `async` and `await` for the DOM tests?
Comment 9 BJ Burg 2017-01-03 09:58:48 PST
(In reply to comment #8)
> What do you think of using `async` and `await` for the DOM tests?

I am somewhat sheepish on using them everywhere because I don't like adding try/catch blocks. They might be appropriate in a few smaller cases, let me know if you have a few cases in mind!
Comment 10 BJ Burg 2017-01-05 14:39:19 PST
Created attachment 298137 [details]
[PATCH] Rebased
Comment 11 BJ Burg 2017-01-05 15:27:02 PST
Created attachment 298144 [details]
[PATCH] Proposed Fix v2
Comment 12 Matt Baker 2017-01-05 15:56:34 PST
Comment on attachment 298144 [details]
[PATCH] Proposed Fix v2

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

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:82
> +        // FIXME: convert clients to chain on this method instead of using a callback.

Please file a bug and add a FIXME.

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:118
> +    querySelector(nodeId, selectors, callback)

Are querySelector and querySelectorAll still in use? I didn't find any references.

> Source/WebInspectorUI/UserInterface/Controllers/DOMTreeManager.js:232
> +        if (payload && "nodeId" in payload) {

Braces aren't needed around one-line branches.
Comment 13 BJ Burg 2017-02-02 20:40:16 PST
Comment on attachment 298144 [details]
[PATCH] Proposed Fix v2

Clearing r? as we aren't comfortable replacing more callbacks with promises until our promise debugging tools and test debugging are better.