[webkit-reviews] review denied: [Bug 45982] Web Inspector: FileSystem integration : [Attachment 70385] patch3 - corrected minor indents.

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied 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 70385: patch3 - corrected minor indents.
https://bugs.webkit.org/attachment.cgi?id=70385&action=review

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

Looks good. Few things to fix in code and UI before we land it!

> WebCore/inspector/InspectorFileSystemAgent.cpp:129
> +    AsyncFileSystem::Type asyncFileSystemType =
static_cast<AsyncFileSystem::Type>(type);

This really should be handled outside, but that is the generator's problem, I
understand.

> WebCore/inspector/InspectorFileSystemAgent.cpp:162
> +    m_frontend->didGetFileSystemPath("Error while retrieving root",
static_cast<unsigned int>(type));

Nit: This is not exactly a 'path' you are passing in here. You should add a
separate 'success' / 'errorMessage' out parameter to report about errors.

> WebCore/inspector/front-end/FileSystemView.js:49
> +    this._tabbedPane.appendTab("persistent",
WebInspector.UIString("Persistent File System"),
this._persistentFileSystemElement, this._selectFileSystemTab.bind(this, true));


Still no localizedStrings.js file changed.

> WebCore/inspector/front-end/FileSystemView.js:88
> +	  
WebInspector.FileSystem.getFileSystemPathAsync(this.fileSystemType.PERSISTENT);


Nit: given that these are always called next to each other, should we return
array of objects?