[webkit-reviews] review denied: [Bug 45982] Web Inspector: FileSystem integration : [Attachment 71519] patch4

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Kavita Kanetkar
<kkanetkar at chromium.org>'s request for review:
Bug 45982: Web Inspector: FileSystem integration
https://bugs.webkit.org/show_bug.cgi?id=45982

Attachment 71519: patch4
https://bugs.webkit.org/attachment.cgi?id=71519&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71519&action=review

>> WebCore/inspector/InspectorFileSystemAgent.cpp:125
>> +void InspectorFileSystemAgent::getFileSystemPathAsync(unsigned int type,
const String& domain)
> 
> Since file systems are per security origin, should the second param be a
const SecurityOrigin& ?

@Michael: You can't pass SecurityOrigin to/from front-end, so passing domain is
our best guess here.

> WebCore/inspector/InspectorFileSystemAgent.cpp:138
> +	   // FIXME: Change this to iterate over all frames to match domain
when UI supports

I really don't see point in not supporting non-main frame domain since it is
fairly trivial. All you need is to traverse the frame tree (using
mainFrame()->tree()->traverseNext) and pick appropriate instance.

>> WebCore/inspector/Inspector.idl:229
>> +	    [notify] void didGetFileSystemError(out int type);
> 
> To summarize these interface changes...
> 
>	   [handler=FileSystem] void getFileSystemPathAsync(in unsigned int
type, in String securityOrigin);
>	   [handler=FileSystem] void revealFolderInOS(in String path);
>	   [notify] void didGetFileSystemPath(out String rootPath, out int
type, String securityOrigin);
>	   [notify] void didGetFileSystemError(out int type, String
securityOrigin);
> 
> I'm not familiar with these IDL annotations as applied to the inspector. Is
'out' correct on those method arguments?

@Michael: The problem with querying by security origin is that you need to push
it to the front-end first.