[webkit-reviews] review granted: [Bug 45982] Web Inspector: FileSystem integration : [Attachment 72216] patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has granted Kavita Kanetkar
<kkanetkar at chromium.org>'s request for review:
Bug 45982: Web Inspector: FileSystem integration
https://bugs.webkit.org/show_bug.cgi?id=45982

Attachment 72216: patch
https://bugs.webkit.org/attachment.cgi?id=72216&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=72216&action=review

> WebCore/inspector/InspectorFileSystemAgent.cpp:128
> +    for (Frame* frame = mainFrame; frame; frame =
frame->tree()->traverseNext()) {

nit: you should pass mainFrame into the traverseNext

> WebCore/inspector/front-end/inspector.js:55
> +    fileSystemOrigins: {},

I really don't like polluting our global namespace with public maps like this.
I am currently getting rid of the nastiest one, the WebInspector.resources. It
is painful to see the new ones coming. They belong to the corresponding panels.
I'd like to see this fixed in the follow up change.

> WebCore/inspector/front-end/inspector.js:1296
> +		   // This should match the SecurityOrigin::toString(). FIXME:
Add a test for this.

I'd like to see the test in the follow up change please.