RESOLVED FIXED 87724
Web Inspector: Implement InspectorFileSystemAgent::readDirectory.
https://bugs.webkit.org/show_bug.cgi?id=87724
Summary Web Inspector: Implement InspectorFileSystemAgent::readDirectory.
Taiju Tsuiki
Reported 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.
Attachments
Patch (8.87 KB, patch)
2012-05-29 03:57 PDT, Taiju Tsuiki
no flags
Patch (10.08 KB, patch)
2012-05-29 06:22 PDT, Taiju Tsuiki
no flags
Patch (25.31 KB, patch)
2012-06-06 05:09 PDT, Taiju Tsuiki
no flags
Patch (26.37 KB, patch)
2012-06-07 20:06 PDT, Taiju Tsuiki
no flags
Patch (26.36 KB, patch)
2012-06-07 20:11 PDT, Taiju Tsuiki
no flags
Patch (26.05 KB, patch)
2012-06-11 00:00 PDT, Taiju Tsuiki
no flags
Taiju Tsuiki
Comment 1 2012-05-29 03:57:24 PDT
Kinuko Yasuda
Comment 2 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())
Taiju Tsuiki
Comment 3 2012-05-29 06:22:21 PDT
Taiju Tsuiki
Comment 4 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
Yury Semikhatsky
Comment 5 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.
Taiju Tsuiki
Comment 6 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?
Yury Semikhatsky
Comment 7 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.
Taiju Tsuiki
Comment 8 2012-06-06 05:09:49 PDT
Taiju Tsuiki
Comment 9 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.
Vsevolod Vlasov
Comment 10 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.
Taiju Tsuiki
Comment 11 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.
Vsevolod Vlasov
Comment 12 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.
Taiju Tsuiki
Comment 13 2012-06-07 20:06:00 PDT
Taiju Tsuiki
Comment 14 2012-06-07 20:11:18 PDT
Taiju Tsuiki
Comment 15 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.
Taiju Tsuiki
Comment 16 2012-06-07 20:14:18 PDT
Comment on attachment 146461 [details] Patch Thank you for reviewing, Vsevolod.
Vsevolod Vlasov
Comment 17 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.
Taiju Tsuiki
Comment 18 2012-06-11 00:00:32 PDT
Taiju Tsuiki
Comment 19 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.
WebKit Review Bot
Comment 20 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>
WebKit Review Bot
Comment 21 2012-06-13 02:52:41 PDT
All reviewed patches have been landed. Closing bug.
Taiju Tsuiki
Comment 22 2012-06-14 00:33:54 PDT
*** Bug 87702 has been marked as a duplicate of this bug. ***
Note You need to log in before you can comment on or make changes to this bug.