Bug 87856

Summary: Web Inspector: Add requestMetadata command and metadataReceived event to FileSystem
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, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 89420, 89555, 89739    
Bug Blocks: 68203, 89961    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch yurys: review+, webkit.review.bot: commit-queue-

Description Taiju Tsuiki 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.
Comment 1 Taiju Tsuiki 2012-05-30 08:03:00 PDT
Created attachment 144820 [details]
Patch
Comment 2 Kinuko Yasuda 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
Comment 3 Taiju Tsuiki 2012-05-30 08:41:30 PDT
Created attachment 144830 [details]
Patch
Comment 4 Taiju Tsuiki 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
Comment 5 Taiju Tsuiki 2012-06-19 01:35:57 PDT
Created attachment 148285 [details]
Patch
Comment 6 Pavel Feldman 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.
Comment 7 Taiju Tsuiki 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.
Comment 8 Taiju Tsuiki 2012-06-20 21:36:56 PDT
Created attachment 148726 [details]
Patch
Comment 9 Taiju Tsuiki 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
Comment 10 Yury Semikhatsky 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)

== -> ===
Comment 11 Taiju Tsuiki 2012-06-21 23:44:56 PDT
Created attachment 148968 [details]
Patch
Comment 12 Taiju Tsuiki 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
Comment 13 WebKit Review Bot 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
Comment 14 Taiju Tsuiki 2012-06-22 06:10:47 PDT
Created attachment 149012 [details]
Patch
Comment 15 WebKit Review Bot 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
Comment 16 Kinuko Yasuda 2012-06-26 03:26:56 PDT
Committed r121251: <http://trac.webkit.org/changeset/121251>