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.
Created attachment 144506 [details] Patch
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())
Created attachment 144540 [details] Patch
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 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 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 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.
Created attachment 145994 [details] Patch
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 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 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 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.
Created attachment 146460 [details] Patch
Created attachment 146461 [details] Patch
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 on attachment 146461 [details] Patch Thank you for reviewing, Vsevolod.
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.
Created attachment 146795 [details] Patch
(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 on attachment 146795 [details] Patch Clearing flags on attachment: 146795 Committed r120177: <http://trac.webkit.org/changeset/120177>
All reviewed patches have been landed. Closing bug.
*** Bug 87702 has been marked as a duplicate of this bug. ***