Bug 90529

Summary: Web Inspector: Add FileContentView for FileSystemView
Product: WebKit Reporter: Taiju Tsuiki <tzik>
Component: Web Inspector (Deprecated)Assignee: Taiju Tsuiki <tzik>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, vsevik, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 90439, 90518    
Bug Blocks: 68203, 90592    
Attachments:
Description Flags
Patch
none
Patch none

Description Taiju Tsuiki 2012-07-04 01:46:42 PDT 
For FileSystem support of Inspector, I'd like to add a view to show content of a file.
This patch adds FileContentView, this view is only for text file.
Comment 1 Taiju Tsuiki 2012-07-04 01:49:15 PDT 
Created attachment 150738 [details]
Patch
Comment 2 Vsevolod Vlasov 2012-07-05 13:58:24 PDT 
Comment on attachment 150738 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150738&action=review

> Source/WebCore/inspector/front-end/FileContentView.js:49
> +            if (this._file.resourceType.isTextType())

You should use another way to detect text files, see https://bugs.webkit.org/show_bug.cgi?id=90518 for details.

> Source/WebCore/inspector/front-end/FileContentView.js:61
> +        if (errorCode !== 0 || !metadata)

if (errorCode || !metadata)

> Source/WebCore/inspector/front-end/FileContentView.js:82
> +        if (this._file.resourceType.isTextType())

Ditto.

> Source/WebCore/inspector/front-end/FileContentView.js:95
> +WebInspector.FileContentView.Content = function(file, metadata)

WebInspector.FileContentProvider
I would move all this logic to WebInspector.FileSystemModel.File actually.

> Source/WebCore/inspector/front-end/FileSystemView.js:114
> +            if (this._entry.isDirectory) {

Please remove braces.

> Source/WebCore/inspector/front-end/FileSystemView.js:196
> +        } else {

No braces for one liners.
Comment 3 Taiju Tsuiki 2012-07-09 02:17:26 PDT 
Created attachment 151217 [details]
Patch
Comment 4 Taiju Tsuiki 2012-07-09 02:26:03 PDT 
Comment on attachment 150738 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150738&action=review

>> Source/WebCore/inspector/front-end/FileContentView.js:49
>> +            if (this._file.resourceType.isTextType())
> 
> You should use another way to detect text files, see https://bugs.webkit.org/show_bug.cgi?id=90518 for details.

I moved it to back-end and added isTextFile property.

>> Source/WebCore/inspector/front-end/FileContentView.js:61
>> +        if (errorCode !== 0 || !metadata)
> 
> if (errorCode || !metadata)

Done

>> Source/WebCore/inspector/front-end/FileContentView.js:95
>> +WebInspector.FileContentView.Content = function(file, metadata)
> 
> WebInspector.FileContentProvider
> I would move all this logic to WebInspector.FileSystemModel.File actually.

Renamed to FileContentProvider.
I'd like to separate file and metadata to keep file immutable.
Anyway, should I move FileContentProvider under FileSystemModel?

>> Source/WebCore/inspector/front-end/FileSystemView.js:114
>> +            if (this._entry.isDirectory) {
> 
> Please remove braces.

Done

>> Source/WebCore/inspector/front-end/FileSystemView.js:196
>> +        } else {
> 
> No braces for one liners.

Done
Comment 5 Vsevolod Vlasov 2012-07-09 02:36:39 PDT 
> Anyway, should I move FileContentProvider under FileSystemModel?
Sounds reasonable.
Comment 6 WebKit Review Bot 2012-07-09 05:34:03 PDT 
Comment on attachment 151217 [details]
Patch

Clearing flags on attachment: 151217

Committed r122104: <http://trac.webkit.org/changeset/122104>
Comment 7 WebKit Review Bot 2012-07-09 05:34:08 PDT 
All reviewed patches have been landed.  Closing bug.