Bug 87724 – Web Inspector: Implement InspectorFileSystemAgent::readDirectory.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Taiju Tsuiki]

Implement InspectorFileSystemAgent::readDirectory for FileSystem support.

This is a patch against 87635 and 87702.
Monolithic version is available on http://webkit.org/b/68203 for overview.

[Taiju Tsuiki]

Created attachment 144506 [details]
Patch

[Kinuko Yasuda]

Comment on attachment 144506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144506&action=review

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:105
> +    virtual void contextDestroyed()

nit: OVERRIDE ?

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:128
> +        , m_url(KURL(), url) { }

m_url(ParsedURLString, url) might be enough?

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:149
> +    virtual bool handleEvent(FileError* error)

ditto.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:220
> +    m_directoryReader = reader.get();

May worth adding a comment why we don't need reference here?

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:233
> +{

nit: ASSERT(entries) ?

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:246
> +    reportResult(static_cast<FileError::ErrorCode>(0), m_entries);

nit: I slightly prefer calling this and exit earlier if (!entries->length())

[Taiju Tsuiki]

Created attachment 144540 [details]
Patch

[Taiju Tsuiki]

Comment on attachment 144506 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144506&action=review

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:105
>> +    virtual void contextDestroyed()
> 
> nit: OVERRIDE ?

Done

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:128
>> +        , m_url(KURL(), url) { }
> 
> m_url(ParsedURLString, url) might be enough?

Done

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:149
>> +    virtual bool handleEvent(FileError* error)
> 
> ditto.

Done

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:220
>> +    m_directoryReader = reader.get();
> 
> May worth adding a comment why we don't need reference here?

Hmm, I misunderstood on ownership relation on DirectoryReader and callbacks.
We can use RefPtr safely here.

I thought it causes circular reference.

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:233
>> +{
> 
> nit: ASSERT(entries) ?

Done

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:246
>> +    reportResult(static_cast<FileError::ErrorCode>(0), m_entries);
> 
> nit: I slightly prefer calling this and exit earlier if (!entries->length())

Done

[Yury Semikhatsky]

Comment on attachment 144540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144540&action=review

> Source/WebCore/ChangeLog:7
> +

Can we have a test for readDirectory command?

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:98
> +    class ReadDirectoryEntriesCallback;

Do these classes need to be public?

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:112
> +    void gotFileSystem(DOMFileSystem*);

These methods should be private.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:152
> +            m_readDirectoryTask->reportResult(error->code(), 0);

I don't think didReadDirectory allows entries to be 0 we should probably declare it optional in the protocol or pass an empty array here. BTW I can't find didReadDirectory in Inspector.json, is it already there?

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:172
> +        m_readDirectoryTask->gotFileSystem(fileSystem);

Can we have ReadDirectoryTask implement all the callbacks to avoid this intermediate wrappers that just delegate the calls to the ReadDirectoryTask at the end? All callbacks seem to have handleEvent with different signatures so it should be doable and would require less code.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:286
> +    Frame* frame = m_instrumentingAgents->inspectorPageAgent()->frameForId(frameId);

We use InstrumentingAgents to access enabled agents only in InspectorInstrumentation.h/cpp If InspectorFileSystemAgent depends on another agent you should pass a pointer to the agent into InspectorFileSystemAgent constructor(see for example InspectorDOMAgent which depends on InspectorPageAgent).

Also you should check if a frame with given id exists and report an error if not.

[Taiju Tsuiki]

Comment on attachment 144540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144540&action=review

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:172
>> +        m_readDirectoryTask->gotFileSystem(fileSystem);
> 
> Can we have ReadDirectoryTask implement all the callbacks to avoid this intermediate wrappers that just delegate the calls to the ReadDirectoryTask at the end? All callbacks seem to have handleEvent with different signatures so it should be doable and would require less code.

All of these callback classes are RefCounted and these inheritance are not virtual.
So, I think, we can't inherit more than one *Callback even if they have different signature.

We can use template to avoid duplication. Like this:
  template<typename CallbackBase, typename Handler, typename Signature> class CallbackWrapper;
  CallbackWrapper<EntriesCallback, ReadDirectoryTask, bool (*)(EntryArray*)>::create(this, &ReadDirectoryTask::gotEntries);

Does it look good to you?

[Yury Semikhatsky]

Comment on attachment 144540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144540&action=review

>>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:172
>>> +        m_readDirectoryTask->gotFileSystem(fileSystem);
>> 
>> Can we have ReadDirectoryTask implement all the callbacks to avoid this intermediate wrappers that just delegate the calls to the ReadDirectoryTask at the end? All callbacks seem to have handleEvent with different signatures so it should be doable and would require less code.
> 
> All of these callback classes are RefCounted and these inheritance are not virtual.
> So, I think, we can't inherit more than one *Callback even if they have different signature.
> 
> We can use template to avoid duplication. Like this:
>   template<typename CallbackBase, typename Handler, typename Signature> class CallbackWrapper;
>   CallbackWrapper<EntriesCallback, ReadDirectoryTask, bool (*)(EntryArray*)>::create(this, &ReadDirectoryTask::gotEntries);
> 
> Does it look good to you?

Ok, I missed that problem with RefCounted inheritance, now the the current approach makes sense to me. I don't think templates would help much here.

[Taiju Tsuiki]

Created attachment 145994 [details]
Patch

[Taiju Tsuiki]

Comment on attachment 144540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=144540&action=review

>> Source/WebCore/ChangeLog:7
>> +
> 
> Can we have a test for readDirectory command?

Done.

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:98
>> +    class ReadDirectoryEntriesCallback;
> 
> Do these classes need to be public?

No, It doesn't.
Moved to private section.

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:112
>> +    void gotFileSystem(DOMFileSystem*);
> 
> These methods should be private.

Done.

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:152
>> +            m_readDirectoryTask->reportResult(error->code(), 0);
> 
> I don't think didReadDirectory allows entries to be 0 we should probably declare it optional in the protocol or pass an empty array here. BTW I can't find didReadDirectory in Inspector.json, is it already there?

Change to Inspector.json was in another unlanded patch, and I merged it to this.
Yes, we need "optional": true in it.

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:286
>> +    Frame* frame = m_instrumentingAgents->inspectorPageAgent()->frameForId(frameId);
> 
> We use InstrumentingAgents to access enabled agents only in InspectorInstrumentation.h/cpp If InspectorFileSystemAgent depends on another agent you should pass a pointer to the agent into InspectorFileSystemAgent constructor(see for example InspectorDOMAgent which depends on InspectorPageAgent).
> 
> Also you should check if a frame with given id exists and report an error if not.

I see. Will be done in next patch.

[Vsevolod Vlasov]

Comment on attachment 145994 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145994&action=review

> Source/WebCore/inspector/Inspector.json:1401
> +                    {

We have one line per property unless it is very complex usually.

> Source/WebCore/inspector/Inspector.json:1438
> +                "description": "Requests to read the directory content. Result should return on didReadDirectory event with request ID.",

request id

> Source/WebCore/inspector/Inspector.json:1450
> +                    {

One line per parameter please.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:105
> +    typedef TypeBuilder::FileSystem::Entry InspectorFileSystemEntry;

This seems confusing, let's use "using" instead, we'll have to write Array<Entry> then which is clearer.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:126
> +        m_frontendProvider = 0;

Shouldn't ReadDirectoryTask be destroyed after exiting this method? 
If it should - you don't need to explicitly clear frontendProvider
If it shouldn't - when will it be destroyed?

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:266
> +    readDirectoryEntries();

Isn't that an infinite loop? r- for that.
Shouldn't you call reportResult(static_cast<FileError::ErrorCode>(0), m_entries) instead?
Also I don't think you really need m_entries field, local variable should be enough.

> LayoutTests/http/tests/inspector/filesystem/read-directory.html:8
> +    function FileSystemDispatcher() {}

I think we should install sniffer on the dispatcher used in the front-end code instead.
I know it is not landed yet, so you should either land front end part first or update this test in the front end patch later.

Ideally this should look like:
var fileSystemModel = new WebInspector.FileSystemModel();
InspectorTest.addSniffer(WebInspector.FileSystemDispatcher.prototype, "didReadDirectory", nextStep, false);

> LayoutTests/http/tests/inspector/filesystem/read-directory.html:27
> +    InspectorTest.createDirectory("/hoge", function() {

We use named functions (either with meaningful names or with names like stepN) in inspector tests. Please make sure { starting a function always goes on the separate line.

> LayoutTests/http/tests/inspector/filesystem/read-directory.html:30
> +                FileSystemAgent.readDirectory(1, WebInspector.resourceTreeModel.mainFrame.id, "filesystem:http://127.0.0.1/temporary/hoge");

I think it's better to initiate directory reading from FileSystemModel so that we test how this machinery works from the front end point of view as a whole.

[Taiju Tsuiki]

Comment on attachment 145994 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145994&action=review

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:126
>> +        m_frontendProvider = 0;
> 
> Shouldn't ReadDirectoryTask be destroyed after exiting this method? 
> If it should - you don't need to explicitly clear frontendProvider
> If it shouldn't - when will it be destroyed?

ReadDirectoryTask is owned by callbacks, and callbacks are passed to each ports through AsyncFileSystem.
So, its lifetime depends on platform.
If it has longer life than context, reportResult is possibly called twice. So, we need to clear frontendProvider here.

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:266
>> +    readDirectoryEntries();
> 
> Isn't that an infinite loop? r- for that.
> Shouldn't you call reportResult(static_cast<FileError::ErrorCode>(0), m_entries) instead?
> Also I don't think you really need m_entries field, local variable should be enough.

No, it's not an infinite loop.
DirectoryReader may pass the directory contents in multiple chunk.
Its empty result (entries->length() == 0) means there is no more content to read,
 and non-empty result means we should call readEntries() again to read all.

[Vsevolod Vlasov]

Comment on attachment 145994 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145994&action=review

>>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:126
>>> +        m_frontendProvider = 0;
>> 
>> Shouldn't ReadDirectoryTask be destroyed after exiting this method? 
>> If it should - you don't need to explicitly clear frontendProvider
>> If it shouldn't - when will it be destroyed?
> 
> ReadDirectoryTask is owned by callbacks, and callbacks are passed to each ports through AsyncFileSystem.
> So, its lifetime depends on platform.
> If it has longer life than context, reportResult is possibly called twice. So, we need to clear frontendProvider here.

So if context is destroyed before callbacks are executed - shouldn't you manually delete ReadDirectoryTask then? ScriptExecutionContext does not own its ContextDestructionObservers and does not delete them.

>>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:266
>>> +    readDirectoryEntries();
>> 
>> Isn't that an infinite loop? r- for that.
>> Shouldn't you call reportResult(static_cast<FileError::ErrorCode>(0), m_entries) instead?
>> Also I don't think you really need m_entries field, local variable should be enough.
> 
> No, it's not an infinite loop.
> DirectoryReader may pass the directory contents in multiple chunk.
> Its empty result (entries->length() == 0) means there is no more content to read,
>  and non-empty result means we should call readEntries() again to read all.

I see, I didn't know how DirectoryReader works.

[Taiju Tsuiki]

Created attachment 146460 [details]
Patch

[Taiju Tsuiki]

Created attachment 146461 [details]
Patch

[Taiju Tsuiki]

Comment on attachment 145994 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145994&action=review

>> Source/WebCore/inspector/Inspector.json:1401
>> +                    {
> 
> We have one line per property unless it is very complex usually.

Done

>> Source/WebCore/inspector/Inspector.json:1438
>> +                "description": "Requests to read the directory content. Result should return on didReadDirectory event with request ID.",
> 
> request id

Done

>> Source/WebCore/inspector/Inspector.json:1450
>> +                    {
> 
> One line per parameter please.

Done

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:105
>> +    typedef TypeBuilder::FileSystem::Entry InspectorFileSystemEntry;
> 
> This seems confusing, let's use "using" instead, we'll have to write Array<Entry> then which is clearer.

There are WebCore::Entry and WebCore::EntryArray already,
it's syntactically OK to use "using" to them, but I feel it a bit confusing.

May I write something like this instead?
namespace Inspector {
  using TypeBulider::Filesystem::Entry;
  using TypeBulider::Array;
  typedef Array<Entry> EntryArray;
}

>>>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:126
>>>> +        m_frontendProvider = 0;
>>> 
>>> Shouldn't ReadDirectoryTask be destroyed after exiting this method? 
>>> If it should - you don't need to explicitly clear frontendProvider
>>> If it shouldn't - when will it be destroyed?
>> 
>> ReadDirectoryTask is owned by callbacks, and callbacks are passed to each ports through AsyncFileSystem.
>> So, its lifetime depends on platform.
>> If it has longer life than context, reportResult is possibly called twice. So, we need to clear frontendProvider here.
> 
> So if context is destroyed before callbacks are executed - shouldn't you manually delete ReadDirectoryTask then? ScriptExecutionContext does not own its ContextDestructionObservers and does not delete them.

I expect AsyncFileSystem will eventually drop callbacks and then ReadDirectoryTask (that is RefCounted) will be destroyed.
So I think we don't need to do manually.

>> LayoutTests/http/tests/inspector/filesystem/read-directory.html:8
>> +    function FileSystemDispatcher() {}
> 
> I think we should install sniffer on the dispatcher used in the front-end code instead.
> I know it is not landed yet, so you should either land front end part first or update this test in the front end patch later.
> 
> Ideally this should look like:
> var fileSystemModel = new WebInspector.FileSystemModel();
> InspectorTest.addSniffer(WebInspector.FileSystemDispatcher.prototype, "didReadDirectory", nextStep, false);

OK, let me update this after frontend is ready. I added comments for it.

>> LayoutTests/http/tests/inspector/filesystem/read-directory.html:27
>> +    InspectorTest.createDirectory("/hoge", function() {
> 
> We use named functions (either with meaningful names or with names like stepN) in inspector tests. Please make sure { starting a function always goes on the separate line.

Done

>> LayoutTests/http/tests/inspector/filesystem/read-directory.html:30
>> +                FileSystemAgent.readDirectory(1, WebInspector.resourceTreeModel.mainFrame.id, "filesystem:http://127.0.0.1/temporary/hoge");
> 
> I think it's better to initiate directory reading from FileSystemModel so that we test how this machinery works from the front end point of view as a whole.

OK, but let me replace it after frontend lands.

[Taiju Tsuiki]

Comment on attachment 146461 [details]
Patch

Thank you for reviewing, Vsevolod.

[Vsevolod Vlasov]

Overall looks good. Please remove contextDestroyed listener as discussed offline since we can filter out such requests on front-end.

> >> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:105
> >> +    typedef TypeBuilder::FileSystem::Entry InspectorFileSystemEntry;
> > 
> > This seems confusing, let's use "using" instead, we'll have to write Array<Entry> then which is clearer.
> 
> There are WebCore::Entry and WebCore::EntryArray already,
> it's syntactically OK to use "using" to them, but I feel it a bit confusing.
> 
> May I write something like this instead?
> namespace Inspector {
>   using TypeBulider::Filesystem::Entry;
>   using TypeBulider::Array;
>   typedef Array<Entry> EntryArray;
> }
I'd like to keep one style in all inspector agents, so you can use InspectorDebuggerAgent for the reference here.
I suggest adding "using TypeBulider::Array;" and use Array<TypeBulider::FileSystem::Entry> in code.

[Taiju Tsuiki]

Created attachment 146795 [details]
Patch

[Taiju Tsuiki]

(In reply to comment #17)
> Overall looks good. Please remove contextDestroyed listener as discussed offline since we can filter out such requests on front-end.
> 
> > >> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:105
> > >> +    typedef TypeBuilder::FileSystem::Entry InspectorFileSystemEntry;
> > > 
> > > This seems confusing, let's use "using" instead, we'll have to write Array<Entry> then which is clearer.
> > 
> > There are WebCore::Entry and WebCore::EntryArray already,
> > it's syntactically OK to use "using" to them, but I feel it a bit confusing.
> > 
> > May I write something like this instead?
> > namespace Inspector {
> >   using TypeBulider::Filesystem::Entry;
> >   using TypeBulider::Array;
> >   typedef Array<Entry> EntryArray;
> > }
> I'd like to keep one style in all inspector agents, so you can use InspectorDebuggerAgent for the reference here.
> I suggest adding "using TypeBulider::Array;" and use Array<TypeBulider::FileSystem::Entry> in code.

Make sense. I changed it so.

I removed ContextDestructionObserver as we chatted, and add some check if the context is alive.
Also, I changed FileSystemCallback with EntryCallback, since it seems easy to reuse similar implementation later.

[WebKit Review Bot]

Comment on attachment 146795 [details]
Patch

Clearing flags on attachment: 146795

Committed r120177: <http://trac.webkit.org/changeset/120177>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Taiju Tsuiki]

*** Bug 87702 has been marked as a duplicate of this bug. ***