Bug 90529 - Web Inspector: Add FileContentView for FileSystemView
Summary: Web Inspector: Add FileContentView for FileSystemView
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Taiju Tsuiki
URL:
Keywords:
Depends on: 90439 90518
Blocks: 68203 90592
  Show dependency treegraph
 
Reported: 2012-07-04 01:46 PDT by Taiju Tsuiki
Modified: 2012-07-09 05:34 PDT (History)
12 users (show)

See Also:


Attachments
Patch (12.41 KB, patch)
2012-07-04 01:49 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (12.57 KB, patch)
2012-07-09 02:17 PDT, Taiju Tsuiki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.