Bug 87724

Summary: Web Inspector: Implement InspectorFileSystemAgent::readDirectory.
Product: WebKit Reporter: Taiju Tsuiki <tzik>
Component: Web Inspector (Deprecated)Assignee: Taiju Tsuiki <tzik>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, kinuko, loislo, pfeldman, pmuellr, rakuco, rik, timothy, vsevik, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 87635, 87702    
Bug Blocks: 68203    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Taiju Tsuiki 2012-05-29 03:52:52 PDT
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.
Comment 1 Taiju Tsuiki 2012-05-29 03:57:24 PDT
Created attachment 144506 [details]
Patch
Comment 2 Kinuko Yasuda 2012-05-29 04:26:55 PDT
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())
Comment 3 Taiju Tsuiki 2012-05-29 06:22:21 PDT
Created attachment 144540 [details]
Patch
Comment 4 Taiju Tsuiki 2012-05-29 06:23:22 PDT
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
Comment 5 Yury Semikhatsky 2012-05-31 04:49:34 PDT
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.
Comment 6 Taiju Tsuiki 2012-05-31 23:11:44 PDT
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?
Comment 7 Yury Semikhatsky 2012-05-31 23:32:08 PDT
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.
Comment 8 Taiju Tsuiki 2012-06-06 05:09:49 PDT
Created attachment 145994 [details]
Patch
Comment 9 Taiju Tsuiki 2012-06-06 05:13:08 PDT
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.
Comment 10 Vsevolod Vlasov 2012-06-06 11:28:44 PDT
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.
Comment 11 Taiju Tsuiki 2012-06-06 23:26:30 PDT
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.
Comment 12 Vsevolod Vlasov 2012-06-07 03:31:30 PDT
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.
Comment 13 Taiju Tsuiki 2012-06-07 20:06:00 PDT
Created attachment 146460 [details]
Patch
Comment 14 Taiju Tsuiki 2012-06-07 20:11:18 PDT
Created attachment 146461 [details]
Patch
Comment 15 Taiju Tsuiki 2012-06-07 20:12:48 PDT
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.
Comment 16 Taiju Tsuiki 2012-06-07 20:14:18 PDT
Comment on attachment 146461 [details]
Patch

Thank you for reviewing, Vsevolod.
Comment 17 Vsevolod Vlasov 2012-06-08 05:42:28 PDT
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.
Comment 18 Taiju Tsuiki 2012-06-11 00:00:32 PDT
Created attachment 146795 [details]
Patch
Comment 19 Taiju Tsuiki 2012-06-11 00:10:31 PDT
(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.
Comment 20 WebKit Review Bot 2012-06-13 02:52:35 PDT
Comment on attachment 146795 [details]
Patch

Clearing flags on attachment: 146795

Committed r120177: <http://trac.webkit.org/changeset/120177>
Comment 21 WebKit Review Bot 2012-06-13 02:52:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Taiju Tsuiki 2012-06-14 00:33:54 PDT
*** Bug 87702 has been marked as a duplicate of this bug. ***