[webkit-reviews] review denied: [Bug 45982] Web Inspector: FileSystem integration : [Attachment 69873] patch

[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 69873: patch
https://bugs.webkit.org/attachment.cgi?id=69873&action=review

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

> WebKit/chromium/src/ChromiumBridge.cpp:345
> +    webKitClient()->fileUtilities()->openInFolder(path);

"revealOSFolder" or "revealFolderInOS"? Otherwise I might think that something
is opening in the folder...

> WebKit/chromium/ChangeLog:5
> +	   Web Inspector: FileSystem integration

How come Joe did not mention - please add more information here.

>> WebCore/platform/AsyncFileSystem.h:122
>> +	virtual String root() const { return m_platformRootPath; }
> 
> virtual const String& root() const { return m_platformRootPath; }
> 
> does this need to be virtual?

@Kinuko Yasuda: I am usually opposed to returning const references to internals
(see abarth's recent message). Is there a big performance win in the case?

> WebCore/inspector/InspectorFileSystemAgent.h:51
> +    void getPersistentFileSystem();

Given that this returns nothing, "get" prefix does no seem right. "request" or
"get*Async" would look better.

> WebCore/inspector/InspectorFileSystemAgent.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
AND ANY

Here and in other new files copyright notices, please use Google's header, not
Apple's header with Google as a company name above.

> WebCore/inspector/InspectorFileSystemAgent.cpp:89
> +    InspectorFileSystemAgent* m_agent;

There is a chance inspector is already closed/destroyed by the time you get the
callback. You should store RefPtr here. I can see that you make sure that
InspectorFileSystemAgent behaves reasonably (does noop) after the call to
::stop which is right.

> WebCore/inspector/InspectorFileSystemAgent.cpp:130
> +	   didGetFileSystem(root, type);

It'd be great if we could make these did* methods private. It might be tricky
due to the call from the async callback and might require moving its
declaration to the private: section in InspectorFileSystem.h, but it would make
our life safer.

> WebCore/inspector/InspectorFileSystemAgent.cpp:144
> +    if (m_inspectorController) {

Nit: replace nested conditions with guard expressions to avoid nesting:
if (!m_inspectorController)
    return;

> WebCore/inspector/Inspector.idl:204
> +	   [handler=FileSystem] void getPersistentFileSystem();

Should we pass the root type notion to the frontend instead of having 2 methods
per each action? getFileSystem(FileSystemType) ? Also, this method is not
getting FileSystem, it is getting the RootFolderPath, right? Given that the
domain already stands for "FileSystem", we can call it
getRootPath(FileSystemType).

> WebCore/inspector/Inspector.idl:205
> +	   [handler=FileSystem] void getTempFileSystem();

No abbreviations in WebKit please.

> WebCore/inspector/front-end/FileSystemView.js:36
> +    this.tabbedPane = new WebInspector.TabbedPane(this.element);

Is there a reason this should be public (not have "_" prefix) ? Could you
attach screenshot please?

> WebCore/inspector/front-end/FileSystemView.js:38
> +    this.persistentFileSystemElement = document.createElement("div");

ditto

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

I don't see modifier localizedStrings.js file in this change set.

> WebCore/inspector/front-end/FileSystemView.js:42
> +    this.tempFileSystemElement = document.createElement("div");

ditto

> WebCore/inspector/front-end/FileSystemView.js:54
> +	    WebInspector.FileSystem.openPersistentFileSystemFolder();

Not sure you need so much delegation, inline this.

> WebCore/inspector/front-end/FileSystemView.js:59
> +	   WebInspector.FileSystem.openTempFileSystemFolder();

ditto

> WebCore/inspector/front-end/FileSystemView.js:115
> +	   fileSystemElement.removeChildren();

Screenshot please!

> WebCore/inspector/front-end/FileSystemView.js:137
> +		   this.enableButton.addEventListener("click",
this._persistentFileSystemSelected.bind(this), false);

You can even bind this straight to
InspectorBackend.openPersistentFileSystemFolder

> WebCore/inspector/front-end/StoragePanel.js:674
> +    this._subtitle = "";

Third constructor parameter already assigned fileSystemDomain to mainTitle.
Nuke it.

> WebCore/inspector/front-end/StoragePanel.js:675
> +    this._mainTitle = this._fileSystemDomain;

Second constructor parameter already assigned fileSystemDomain to mainTitle.
Nuke it.

> WebCore/inspector/front-end/StoragePanel.js:676
> +    this.refreshTitles();

Titles are already up-to-date.

> WebCore/inspector/front-end/StoragePanel.js:682
> +	   WebInspector.panels.storage.showFileSystem(this,
this._fileSystemDomain);

Are we subclassing for the sake of "select" only? Rest seems to be generic
implementation. Can we get away with listener instead?

> WebCore/inspector/front-end/StoragePanel.js:685
> +    get mainTitle()

This is no different form the default behavior you already inherited. Nuke it.

> WebCore/inspector/front-end/StoragePanel.js:690
> +    set mainTitle(x)

ditto

> WebCore/inspector/front-end/StoragePanel.js:696
> +    get subtitle()

ditto

> WebCore/inspector/front-end/StoragePanel.js:701
> +    set subtitle(x)

ditto

> WebCore/inspector/front-end/DOMAgent.js:486
> +WebInspector.FileSystem = {}

FileSystem is not related to DOMAgent, should not belong here. Move to
FileSystemView.js

> WebCore/inspector/front-end/inspector.js:1407
> +WebInspector.didGetPersistentFileSystem = function(root)

Move these to FileSystemView.js please.

> WebCore/inspector/InspectorController.h:201
> +    InspectorFileSystemAgent* fileSystemAgent() { return
m_fileSystemAgent.get(); }

Strictly speaking, you don't need getters here - generator makes code that has
access to backend's privates.