Bug 57466 – Web Inspector: provide front-end wrappers for DOMAgent.querySelector[All]() that take care of fetching the document

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

[Andrey Kosyakov]

Requiring document to be set on front-end before calls to querySelector[All]() is counter-intuitive.
This is a better fix for bug 57349 (with some cosmetic drive-by fixes)

[Andrey Kosyakov]

Created attachment 87549 [details]
patch

[Pavel Feldman]

Comment on attachment 87549 [details]
patch

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

> Source/WebCore/inspector/front-end/AuditRules.js:675
> +                console.error("Failed to get image style");

When does this happen?

> Source/WebCore/inspector/front-end/AuditRules.js:731
> +                console.error("Failed to get styles");

ditto

> Source/WebCore/inspector/front-end/DOMAgent.js:363
> +            requestArguments.push(callbackWrapper);

do you want to do this only in the success branch of onDocumentAvailable?

> Source/WebCore/inspector/front-end/DOMAgent.js:495
> +            DOMAgent.querySelector(nodeId || this._document.id, selectors, documentWide, wrappedCallback);

So 0 is still valid? I thought we agreed on not allowing it here.

[Andrey Kosyakov]

Comment on attachment 87549 [details]
patch

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

>> Source/WebCore/inspector/front-end/AuditRules.js:675
>> +                console.error("Failed to get image style");
> 
> When does this happen?

This used to happen because we were getting 0 ids from querySelectorAll(), because document wasn't pushed to front-end. I actually removed it now, this is only supposed to happen in case something seriously went wrong in other places, so let's crash instead.

>> Source/WebCore/inspector/front-end/AuditRules.js:731
>> +                console.error("Failed to get styles");
> 
> ditto

Now this is a bit different, this happens only in case we receive error from getStylesAsync() (from what I can see, this is only in case we supplied invalid node id -- yet, if there's a possibility of a function to return error, we shouldn't be swallowing it or crashing on it, hence I just added logging).

>> Source/WebCore/inspector/front-end/DOMAgent.js:363
>> +            requestArguments.push(callbackWrapper);
> 
> do you want to do this only in the success branch of onDocumentAvailable?

I tried it, but didn't quite like it -- we need to extract callback from the argument list anyway, and I think that having this push together with callback extraction logic makes things a bit more readable.

>> Source/WebCore/inspector/front-end/DOMAgent.js:495
>> +            DOMAgent.querySelector(nodeId || this._document.id, selectors, documentWide, wrappedCallback);
> 
> So 0 is still valid? I thought we agreed on not allowing it here.

Yup, now the document request logic moves to client (and I'm also dropping documentWide parameter altogether, as discussed).

[Andrey Kosyakov]

Created attachment 87716 [details]
patch

- removed documentWide parameter to querySelector[All]()
- moved document request logic from DOMAgent.js to the client side

[Andrey Kosyakov]

Manually committed as r82663: http://trac.webkit.org/changeset/82663