WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(12.76 KB, patch)
2012-05-30 08:41 PDT
,
Taiju Tsuiki
no flags
Details
Formatted Diff
Diff
Patch
(18.69 KB, patch)
2012-06-19 01:35 PDT
,
Taiju Tsuiki
no flags
Details
Formatted Diff
Diff
Patch
(22.43 KB, patch)
2012-06-20 21:36 PDT
,
Taiju Tsuiki
no flags
Details
Formatted Diff
Diff
Patch
(23.74 KB, patch)
2012-06-21 23:44 PDT
,
Taiju Tsuiki
no flags
Details
Formatted Diff
Diff
Patch
(23.71 KB, patch)
2012-06-22 06:10 PDT
,
Taiju Tsuiki
yurys
: review+
webkit.review.bot
: commit-queue-
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Taiju Tsuiki
Comment 1
2012-05-30 08:03:00 PDT
Created
attachment 144820
[details]
Patch
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
Created
attachment 144830
[details]
Patch
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
Created
attachment 148285
[details]
Patch
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
Created
attachment 148726
[details]
Patch
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
Created
attachment 148968
[details]
Patch
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
Created
attachment 149012
[details]
Patch
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
Committed
r121251
: <
http://trac.webkit.org/changeset/121251
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug