RESOLVED FIXED 87856
Web Inspector: Add requestMetadata command and metadataReceived event to FileSystem
https://bugs.webkit.org/show_bug.cgi?id=87856
Summary Web Inspector: Add requestMetadata command and metadataReceived event to File...
Taiju Tsuiki
Reported 2012-05-30 08:00:34 PDT
FileSystem support of Inspector need a way to getting file metadata, including modification time, file size and view type.
Attachments
Patch (13.00 KB, patch)
2012-05-30 08:03 PDT, Taiju Tsuiki
no flags
Patch (12.76 KB, patch)
2012-05-30 08:41 PDT, Taiju Tsuiki
no flags
Patch (18.69 KB, patch)
2012-06-19 01:35 PDT, Taiju Tsuiki
no flags
Patch (22.43 KB, patch)
2012-06-20 21:36 PDT, Taiju Tsuiki
no flags
Patch (23.74 KB, patch)
2012-06-21 23:44 PDT, Taiju Tsuiki
no flags
Patch (23.71 KB, patch)
2012-06-22 06:10 PDT, Taiju Tsuiki
yurys: review+
webkit.review.bot: commit-queue-
Taiju Tsuiki
Comment 1 2012-05-30 08:03:00 PDT
Kinuko Yasuda
Comment 2 2012-05-30 08:20:47 PDT
Comment on attachment 144820 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144820&action=review > Source/WebCore/inspector/InspectorFileSystemAgent.cpp:2 > + * Copyright (C) 2011,2012 Google Inc. All rights reserved. nit: please put space after comma > Source/WebCore/inspector/InspectorFileSystemAgent.cpp:144 > + virtual bool handleEvent(FileError* entry) OVERRIDE nit: entry -> error? > Source/WebCore/inspector/InspectorFileSystemAgent.cpp:208 > + scriptExecutionContext(), type, m_path)); (These many line breaks may not look very common in WebKit... though if other files in this directory look like this it'd be probably ok) > Source/WebCore/inspector/InspectorFileSystemAgent.h:2 > + * Copyright (C) 2011,2012 Google Inc. All rights reserved. please put space after comma > Source/WebCore/inspector/InspectorFileSystemAgent.h:49 > +typedef String ErrorString; No need of typedef
Taiju Tsuiki
Comment 3 2012-05-30 08:41:30 PDT
Taiju Tsuiki
Comment 4 2012-05-30 08:44:13 PDT
Comment on attachment 144820 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144820&action=review >> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:2 >> + * Copyright (C) 2011,2012 Google Inc. All rights reserved. > > nit: please put space after comma Done >> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:144 >> + virtual bool handleEvent(FileError* entry) OVERRIDE > > nit: entry -> error? Done >> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:208 >> + scriptExecutionContext(), type, m_path)); > > (These many line breaks may not look very common in WebKit... though if other files in this directory look like this it'd be probably ok) I'll look this tomorrow. Thanks. >> Source/WebCore/inspector/InspectorFileSystemAgent.h:2 >> + * Copyright (C) 2011,2012 Google Inc. All rights reserved. > > please put space after comma Done >> Source/WebCore/inspector/InspectorFileSystemAgent.h:49 >> +typedef String ErrorString; > > No need of typedef Done
Taiju Tsuiki
Comment 5 2012-06-19 01:35:57 PDT
Pavel Feldman
Comment 6 2012-06-19 06:55:11 PDT
Comment on attachment 148285 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148285&action=review > Source/WebCore/inspector/Inspector.json:1456 > + "name": "getMetadata", requestMetadata (since the result is reported asynchronously) > Source/WebCore/inspector/Inspector.json:1458 > + { "name": "requestId", "type": "integer", "description": "Request Identifier of the command. The backend should pass back this value on corresponding gotMetadata event." }, Please typedef the type. You should also make backend generate it and return here to guarantee it is unique. > Source/WebCore/inspector/Inspector.json:1484 > + "name": "gotMetadata", metadataReceived (since we always put subject first in the notification naming) > LayoutTests/http/tests/inspector/filesystem/filesystem-test.js:107 > + function gotFileSystem(fileSystem) We use didGet notation in the rest of the inspector code.
Taiju Tsuiki
Comment 7 2012-06-20 02:42:02 PDT
Comment on attachment 148285 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148285&action=review >> Source/WebCore/inspector/Inspector.json:1458 >> + { "name": "requestId", "type": "integer", "description": "Request Identifier of the command. The backend should pass back this value on corresponding gotMetadata event." }, > > Please typedef the type. You should also make backend generate it and return here to guarantee it is unique. It seems to affect other command and event. Let me fix it in another bug. I filed http://webkit.org/b/89555 and attached a patch.
Taiju Tsuiki
Comment 8 2012-06-20 21:36:56 PDT
Taiju Tsuiki
Comment 9 2012-06-20 21:38:00 PDT
Comment on attachment 148285 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148285&action=review Updated. Thanks for reviewing. >> Source/WebCore/inspector/Inspector.json:1456 >> + "name": "getMetadata", > > requestMetadata (since the result is reported asynchronously) Done >> Source/WebCore/inspector/Inspector.json:1484 >> + "name": "gotMetadata", > > metadataReceived (since we always put subject first in the notification naming) Done >> LayoutTests/http/tests/inspector/filesystem/filesystem-test.js:107 >> + function gotFileSystem(fileSystem) > > We use didGet notation in the rest of the inspector code. Done
Yury Semikhatsky
Comment 10 2012-06-21 01:34:49 PDT
Comment on attachment 148726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148726&action=review > Source/WebCore/inspector/Inspector.json:1497 > + { "name": "requestId", "type": "integer", "description": "Request Identifier that was passed to corresponding getMetadata request." }, Request Identifier that was returned in response to the corresponding getMetadata request. > Source/WebCore/inspector/InspectorFileSystemAgent.cpp:461 > + if (!m_enabled || !m_frontendProvider) Report an error in the ErrorString? > Source/WebCore/inspector/front-end/FileSystemModel.js:460 > + store[requestId] = callback; not: you can pass requestAccepted.bind(this) as FileSystemAgent.requestMetadata parameter above and call this._pendingMetadataRequests[requestId] = callback; here. This is how we usually deal with bound callbacks in the front-end code. > LayoutTests/http/tests/inspector/filesystem/filesystem-test.js:120 > + if (writer.readyState == writer.DONE) == -> ===
Taiju Tsuiki
Comment 11 2012-06-21 23:44:56 PDT
Taiju Tsuiki
Comment 12 2012-06-21 23:47:02 PDT
Comment on attachment 148726 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148726&action=review >> Source/WebCore/inspector/Inspector.json:1497 >> + { "name": "requestId", "type": "integer", "description": "Request Identifier that was passed to corresponding getMetadata request." }, > > Request Identifier that was returned in response to the corresponding getMetadata request. Done. >> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:461 >> + if (!m_enabled || !m_frontendProvider) > > Report an error in the ErrorString? Done >> Source/WebCore/inspector/front-end/FileSystemModel.js:460 >> + store[requestId] = callback; > > not: you can pass requestAccepted.bind(this) as FileSystemAgent.requestMetadata parameter above and call this._pendingMetadataRequests[requestId] = callback; here. This is how we usually deal with bound callbacks in the front-end code. Since this is an asynchronous callback, I'm worried if _pendingMetadataRequests is reset between requestMetadata and requestAccepted call. If I use _pendingMetadataRequests directly, the callback will be possibly invoked after reset. Ah, I didn't reset it anywhere now. I'll add it afterward. >> LayoutTests/http/tests/inspector/filesystem/filesystem-test.js:120 >> + if (writer.readyState == writer.DONE) > > == -> === .DONE
WebKit Review Bot
Comment 13 2012-06-22 05:25:14 PDT
Comment on attachment 148968 [details] Patch Rejecting attachment 148968 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: d/FileSystemModel.js.rej patching file LayoutTests/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file LayoutTests/http/tests/inspector/filesystem/filesystem-test.js patching file LayoutTests/http/tests/inspector/filesystem/get-metadata-expected.txt patching file LayoutTests/http/tests/inspector/filesystem/get-metadata.html Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Yury Semik..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/ Full output: http://queues.webkit.org/results/13075012
Taiju Tsuiki
Comment 14 2012-06-22 06:10:47 PDT
WebKit Review Bot
Comment 15 2012-06-26 03:06:38 PDT
Comment on attachment 149012 [details] Patch Rejecting attachment 149012 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 11844 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' 47>At revision 11844. ________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' ________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium' Updating webkit projects from gyp files... Full output: http://queues.webkit.org/results/13088455
Kinuko Yasuda
Comment 16 2012-06-26 03:26:56 PDT
Note You need to log in before you can comment on or make changes to this bug.