Bug 87856

Created attachment 144820 [details] Patch

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

Created attachment 144830 [details] Patch

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

Created attachment 148285 [details] Patch

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 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.

Created attachment 148726 [details] Patch

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 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) == -> ===

Created attachment 148968 [details] Patch

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 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

Created attachment 149012 [details] Patch

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

Committed r121251: <http://trac.webkit.org/changeset/121251>