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)
Created attachment 87549 [details] patch
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.
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).
Created attachment 87716 [details] patch - removed documentWide parameter to querySelector[All]() - moved document request logic from DOMAgent.js to the client side
Manually committed as r82663: http://trac.webkit.org/changeset/82663