[Webkit-unassigned] [Bug 45982] Web Inspector: FileSystem integration

Tue Oct 5 23:36:33 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=45982

Pavel Feldman <pfeldman at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #69873|review?                     |review-
               Flag|                            |

--- Comment #6 from Pavel Feldman <pfeldman at chromium.org>  2010-10-05 23:36:32 PST ---
(From update of attachment 69873)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.