Bug 88602

Created attachment 147034 [details] Patch

Created attachment 147036 [details] Patch

Created attachment 147037 [details] Patch

Created attachment 147038 [details] Patch

Comment on attachment 147038 [details] Patch Sorry for upload spamming. I had updoaded wrong one before.

Created attachment 147237 [details] Patch

Comment on attachment 147237 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147237&action=review > Source/WebCore/inspector/Inspector.json:1408 > + "id": "Entry", Could you please make sure every type, property, command and event in FileSystem domain has a description, and that description is the last property for each of them? Could you please make corresponding drive-by changes to entities added in previous patch as well. > Source/WebCore/inspector/InspectorFileSystemAgent.cpp:2 > + * Copyright (C) 2011, 2012 Google Inc. All rights reserved. I believe part of this patch (e.g. this line) was already landed, could you please rebaseline?

Created attachment 147504 [details] Patch

Comment on attachment 147237 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147237&action=review >> Source/WebCore/inspector/Inspector.json:1408 >> + "id": "Entry", > > Could you please make sure every type, property, command and event in FileSystem domain has a description, and that description is the last property for each of them? > Could you please make corresponding drive-by changes to entities added in previous patch as well. OK, I added some description to them. >> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:2 >> + * Copyright (C) 2011, 2012 Google Inc. All rights reserved. > > I believe part of this patch (e.g. this line) was already landed, could you please rebaseline? I rebased it on ToT. Yes, this patch had some common part with a parallel patch, which you reviewed yesterday :)

Comment on attachment 147504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147504&action=review > Source/WebCore/inspector/front-end/FileSystemModel.js:34 > + * @implements {FileSystemAgent.Dispatcher} Could you please implement FileSystemDispatcher as a separate class that delegates all calls to FileSystemModel's private methods? Otherwise FileSystemModel would have public didReadDirectory() method that should not be called by front-end which is a bit confusing. > Source/WebCore/inspector/front-end/FileSystemModel.js:209 > + getFileSystem: function(origin, type, callback) Should be private. > Source/WebCore/inspector/front-end/FileSystemModel.js:211 > + var requestId = WebInspector.FileSystemModel.nextRequestId++; I suggest extracting request managing from FileSystemModel to a separate class (Like it is done for IndexedDBModel). Otherwise FileSystemModel becomes is flooded with methods with almost equal names and different semantics. > Source/WebCore/inspector/front-end/FileSystemModel.js:225 > + gotFileSystemRoot: function(requestId, errorCode, backendEntry) Should be private. > LayoutTests/http/tests/inspector/filesystem/filesystem-test.js:32 > + InspectorTest.clearFileSystem = function(callback) This is the same as the patch uploaded in https://bugs.webkit.org/show_bug.cgi?id=89066. Could you please separate one from another?

Comment on attachment 147504 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147504&action=review >> Source/WebCore/inspector/front-end/FileSystemModel.js:34 >> + * @implements {FileSystemAgent.Dispatcher} > > Could you please implement FileSystemDispatcher as a separate class that delegates all calls to FileSystemModel's private methods? > Otherwise FileSystemModel would have public didReadDirectory() method that should not be called by front-end which is a bit confusing. OK, I'll do it. But, it seems worth separating into another patch. Let me put in http://webkit.org/b/89191 >> Source/WebCore/inspector/front-end/FileSystemModel.js:209 >> + getFileSystem: function(origin, type, callback) > > Should be private. Will be done in next patch. >> Source/WebCore/inspector/front-end/FileSystemModel.js:211 >> + var requestId = WebInspector.FileSystemModel.nextRequestId++; > > I suggest extracting request managing from FileSystemModel to a separate class (Like it is done for IndexedDBModel). > Otherwise FileSystemModel becomes is flooded with methods with almost equal names and different semantics. ditto. I'll put it in http://webkit.org/b/89191 >> LayoutTests/http/tests/inspector/filesystem/filesystem-test.js:32 >> + InspectorTest.clearFileSystem = function(callback) > > This is the same as the patch uploaded in https://bugs.webkit.org/show_bug.cgi?id=89066. > Could you please separate one from another? Sorry, I'll drop it in next version.

Created attachment 147782 [details] Patch

Created attachment 148288 [details] Patch

Comment on attachment 148288 [details] Patch Forgot to add test expectation.

Comment on attachment 148288 [details] Patch Clearing flags on attachment: 148288 Committed r120702: <http://trac.webkit.org/changeset/120702>

All reviewed patches have been landed. Closing bug.