Bug 72456 - [Inspector][FileSystem] Capture DOMFileSystem object
: [Inspector][FileSystem] Capture DOMFileSystem object
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Nobody
:
Depends on: 73204
Blocks: 68203 72691
  Show dependency treegraph
 
Reported: 2011-11-15 19:55 PST by Taiju Tsuiki
Modified: 2011-11-28 05:59 PST (History)
15 users (show)

See Also:


Attachments
Patch (37.46 KB, patch)
2011-11-15 20:29 PST, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (rebase) (37.50 KB, patch)
2011-11-15 20:43 PST, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (add CodeGenerator entry) (39.82 KB, patch)
2011-11-15 21:23 PST, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (#if out for non-chrome) (39.97 KB, patch)
2011-11-15 22:26 PST, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (omit FileSystem invalidation) (38.60 KB, patch)
2011-11-16 02:15 PST, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (rebase) (38.50 KB, patch)
2011-11-16 02:26 PST, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (32.92 KB, patch)
2011-11-17 01:04 PST, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (33.27 KB, patch)
2011-11-18 10:55 PST, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (add a entry to CodeGeneratorInspector.py) (33.76 KB, patch)
2011-11-20 17:41 PST, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (33.82 KB, patch)
2011-11-21 19:46 PST, Taiju Tsuiki
no flags Details | Formatted Diff | Diff
Patch (rebase) (33.83 KB, patch)
2011-11-23 16:25 PST, 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 2011-11-15 19:55:37 PST
This is a splitted patch of https://bugs.webkit.org/show_bug.cgi?id=68203, part1.
- Adding files for FileSystem support,
- Capturing DOMFileSystem object creation and invalidation.
Comment 1 Taiju Tsuiki 2011-11-15 20:29:59 PST
Created attachment 115309 [details]
Patch
Comment 2 Taiju Tsuiki 2011-11-15 20:43:07 PST
Created attachment 115312 [details]
Patch (rebase)
Comment 3 Gyuyoung Kim 2011-11-15 21:04:50 PST
Comment on attachment 115312 [details]
Patch (rebase)

Attachment 115312 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10487586
Comment 4 Taiju Tsuiki 2011-11-15 21:23:02 PST
Created attachment 115317 [details]
Patch (add CodeGenerator entry)
Comment 5 Gyuyoung Kim 2011-11-15 21:28:23 PST
Comment on attachment 115317 [details]
Patch (add CodeGenerator entry)

Attachment 115317 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10372844
Comment 6 Taiju Tsuiki 2011-11-15 22:26:00 PST
Created attachment 115322 [details]
Patch (#if out for non-chrome)
Comment 7 Taiju Tsuiki 2011-11-16 02:15:33 PST
Created attachment 115351 [details]
Patch (omit FileSystem invalidation)
Comment 8 Taiju Tsuiki 2011-11-16 02:26:42 PST
Created attachment 115352 [details]
Patch (rebase)
Comment 9 Pavel Feldman 2011-11-16 02:53:59 PST
Comment on attachment 115352 [details]
Patch (rebase)

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

Looks good except for blank JavaScript files.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:73
> +    m_instrumentingAgents->setInspectorFileSystemAgent(this);

We don't like auxiliary agents getting into the instrumenting mode in the constructor. A much better pattern is to introduce ::enable and ::disable commands that would be called by the front-end lazily. No events should be sent to the front-end unless it is subscribed to them via the ::enable call. Your FS support on the front-end will then query FS state lazily upon Resources panel open via calling ::enable, ::getFileSystems and receive incremental updates from the instrumentation.

In case there is no place you could get active filesystems at a random time, you can start instrumenting in here, but please do not send notifications unless ::enable has been called.

> Source/WebCore/inspector/InspectorFileSystemInstrumentation.h:40
> +inline void InspectorInstrumentation::didOpenFileSystem(PassRefPtr<DOMFileSystem> fileSystem)

Given that you don't actually include any headers from this file, it can be a part of the InspectorInstrumentation. All we really care about is for InspectorInstrumentation not to get too many imports.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:778
> +    if (!inspectorAgent || !inspectorAgent->enabled())

You need this check since you enter instrumenting mode prior to the inspector opening. You would not have needed it otherwise.

> Source/WebCore/inspector/front-end/FileSystem.js:1
> +/*

There is no need to add blank JavaScript files. Please add them with the content in a subsequent change. You will need to add them into the Source/WebCore/WebCore.vcproj/WebCore.vcproj as well.

> Source/WebCore/inspector/front-end/FileSystemView.js:1
> +/*

ditto
Comment 10 Taiju Tsuiki 2011-11-17 01:04:19 PST
Created attachment 115540 [details]
Patch
Comment 11 Taiju Tsuiki 2011-11-17 01:09:15 PST
(In reply to comment #9)
Thank you for reviewing.

> (From update of attachment 115352 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115352&action=review
> 
> Looks good except for blank JavaScript files.
> 
> > Source/WebCore/inspector/InspectorFileSystemAgent.cpp:73
> > +    m_instrumentingAgents->setInspectorFileSystemAgent(this);
> 
> We don't like auxiliary agents getting into the instrumenting mode in the constructor. A much better pattern is to introduce ::enable and ::disable commands that would be called by the front-end lazily. No events should be sent to the front-end unless it is subscribed to them via the ::enable call. Your FS support on the front-end will then query FS state lazily upon Resources panel open via calling ::enable, ::getFileSystems and receive incremental updates from the instrumentation.
> 

Done. I had splitted out it, and now merged some of it back.

> In case there is no place you could get active filesystems at a random time, you can start instrumenting in here, but please do not send notifications unless ::enable has been called.
> 
> > Source/WebCore/inspector/InspectorFileSystemInstrumentation.h:40
> > +inline void InspectorInstrumentation::didOpenFileSystem(PassRefPtr<DOMFileSystem> fileSystem)
> 
> Given that you don't actually include any headers from this file, it can be a part of the InspectorInstrumentation. All we really care about is for InspectorInstrumentation not to get too many imports.
> 

I see. In my previous CL, I added #include "DOMFileSystem.h" to InspectorInstrumentation.h.
Now, I moved it to InspectorFileSystemInstrumentation.h.

> > Source/WebCore/inspector/InspectorInstrumentation.cpp:778
> > +    if (!inspectorAgent || !inspectorAgent->enabled())
> 
> You need this check since you enter instrumenting mode prior to the inspector opening. You would not have needed it otherwise.
> 
> > Source/WebCore/inspector/front-end/FileSystem.js:1
> > +/*
> 
> There is no need to add blank JavaScript files. Please add them with the content in a subsequent change. You will need to add them into the Source/WebCore/WebCore.vcproj/WebCore.vcproj as well.
> 
> > Source/WebCore/inspector/front-end/FileSystemView.js:1
> > +/*
> 
> ditto

Done.
Comment 12 Kinuko Yasuda 2011-11-17 23:18:25 PST
Comment on attachment 115540 [details]
Patch

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

Made a few cosmetic comments.  (Feel free to ignore my comments if any of them conflict with pavel's or inspector people's comment)

> Source/WebCore/ChangeLog:10
> +        No new tests.

Could you add some comments if you're going to add tests later or why this needs no tests?

> Source/WebCore/ChangeLog:27
> +        (WebCore::InspectorFileSystemAgent::create):

nit: in general we don't need to put the functions list for newly added files.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:63
> +    m_enabled = true;

Maybe worth adding some TODO comment or for now you can just get rid of line 61-62?  It's not very clear why we need to early-exit in line 62.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:72
> +{

ASSERT(frontend) ?

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:85
> +{

ASSERT(m_instrumentingAgents) ?

> Source/WebCore/inspector/InspectorFileSystemAgent.h:33
> +#if ENABLE(INSPECTOR) && ENABLE(FILE_SYSTEM)

nit: might be good to put one extra line between line 32-33?
Comment 13 Taiju Tsuiki 2011-11-18 10:55:15 PST
Created attachment 115836 [details]
Patch
Comment 14 Taiju Tsuiki 2011-11-18 11:00:32 PST
Comment on attachment 115540 [details]
Patch

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

>> Source/WebCore/ChangeLog:10
>> +        No new tests.
> 
> Could you add some comments if you're going to add tests later or why this needs no tests?

Done.

>> Source/WebCore/ChangeLog:27
>> +        (WebCore::InspectorFileSystemAgent::create):
> 
> nit: in general we don't need to put the functions list for newly added files.

Done.

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:63
>> +    m_enabled = true;
> 
> Maybe worth adding some TODO comment or for now you can just get rid of line 61-62?  It's not very clear why we need to early-exit in line 62.

Done. I added a TODO comment.

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:72
>> +{
> 
> ASSERT(frontend) ?

Done.

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:85
>> +{
> 
> ASSERT(m_instrumentingAgents) ?

Done.

>> Source/WebCore/inspector/InspectorFileSystemAgent.h:33
>> +#if ENABLE(INSPECTOR) && ENABLE(FILE_SYSTEM)
> 
> nit: might be good to put one extra line between line 32-33?

Done.
Comment 15 Gustavo Noronha (kov) 2011-11-18 11:04:44 PST
Comment on attachment 115836 [details]
Patch

Attachment 115836 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10518166
Comment 16 Gyuyoung Kim 2011-11-18 11:19:00 PST
Comment on attachment 115836 [details]
Patch

Attachment 115836 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10502017
Comment 17 Early Warning System Bot 2011-11-18 12:58:16 PST
Comment on attachment 115836 [details]
Patch

Attachment 115836 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10515176
Comment 18 Taiju Tsuiki 2011-11-20 17:41:52 PST
Created attachment 116015 [details]
Patch (add a entry to CodeGeneratorInspector.py)
Comment 19 Taiju Tsuiki 2011-11-20 17:59:11 PST
Comment on attachment 116015 [details]
Patch (add a entry to CodeGeneratorInspector.py)

Fixed build error caused by conflict with recent upstream change.
Comment 20 Pavel Feldman 2011-11-21 01:32:54 PST
Comment on attachment 116015 [details]
Patch (add a entry to CodeGeneratorInspector.py)

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

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:59
> +    // Store |fileSystem| and notify the frontend using |fileSystemAdded| event.

We are not using | | notation in WebKit.

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:98
> +    m_frontend = 0;

You should clear the enabled state here:
m_enabled = false;
m_state->setBoolean(FileSystemAgentState::fileSystemAgentEnabled, m_enabled);

disconnecting front-end should end up in disabling instrumentation.

> Source/WebCore/inspector/InspectorFileSystemAgent.h:53
> +    void invalidateFileSystem(PassRefPtr<DOMFileSystem>);

fileSystemInvalidated? If this is a signal from WebCore, it should be named as event.
Comment 21 Taiju Tsuiki 2011-11-21 19:46:25 PST
Created attachment 116171 [details]
Patch
Comment 22 Taiju Tsuiki 2011-11-21 19:49:13 PST
Comment on attachment 116015 [details]
Patch (add a entry to CodeGeneratorInspector.py)

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

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:59
>> +    // Store |fileSystem| and notify the frontend using |fileSystemAdded| event.
> 
> We are not using | | notation in WebKit.

Done.

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:98
>> +    m_frontend = 0;
> 
> You should clear the enabled state here:
> m_enabled = false;
> m_state->setBoolean(FileSystemAgentState::fileSystemAgentEnabled, m_enabled);
> 
> disconnecting front-end should end up in disabling instrumentation.

Done.

>> Source/WebCore/inspector/InspectorFileSystemAgent.h:53
>> +    void invalidateFileSystem(PassRefPtr<DOMFileSystem>);
> 
> fileSystemInvalidated? If this is a signal from WebCore, it should be named as event.

Done.
Comment 23 Taiju Tsuiki 2011-11-23 16:25:02 PST
Created attachment 116449 [details]
Patch (rebase)
Comment 24 Pavel Feldman 2011-11-24 00:01:55 PST
Comment on attachment 116449 [details]
Patch (rebase)

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

Please do not make incremental changes to this patch - it is getting hard to track what has changed. I think it is fine to land it as is.

> Source/WebCore/inspector/Inspector.json:1006
> +                "description": "Disables events from backend.."

Extra . in the end.
Comment 25 Taiju Tsuiki 2011-11-27 20:17:50 PST
Comment on attachment 116449 [details]
Patch (rebase)

Thank you, pavel.
I'll fix it in next patch.
Comment 26 WebKit Review Bot 2011-11-28 02:00:57 PST
Comment on attachment 116449 [details]
Patch (rebase)

Clearing flags on attachment: 116449

Committed r101236: <http://trac.webkit.org/changeset/101236>
Comment 27 WebKit Review Bot 2011-11-28 02:01:05 PST
All reviewed patches have been landed.  Closing bug.
Comment 28 Csaba Osztrogonác 2011-11-28 04:13:42 PST
Reopen, because it broke all inspector tests on the Qt bot:
http://build.webkit.org/builders/Qt%20Linux%20Release/builds/40267
Comment 29 Csaba Osztrogonác 2011-11-28 05:59:56 PST
(In reply to comment #28)
> Reopen, because it broke all inspector tests on the Qt bot:
> http://build.webkit.org/builders/Qt%20Linux%20Release/builds/40267

Fixed by http://trac.webkit.org/changeset/101252