Bug 72456 - [Inspector][FileSystem] Capture DOMFileSystem object
: [Inspector][FileSystem] Capture DOMFileSystem object
Status: RESOLVED FIXED
: WebKit
Web Inspector (Deprecated)
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
: 73204
: 68203 72691
  Show dependency treegraph
 
Reported: 2011-11-15 19:55 PST by
Modified: 2011-11-28 05:59 PST (History)


Attachments
Patch (37.46 KB, patch)
2011-11-15 20:29 PST, Taiju Tsuiki
no flags Review Patch | Details | Formatted Diff | Diff
Patch (rebase) (37.50 KB, patch)
2011-11-15 20:43 PST, Taiju Tsuiki
no flags Review Patch | Details | Formatted Diff | Diff
Patch (add CodeGenerator entry) (39.82 KB, patch)
2011-11-15 21:23 PST, Taiju Tsuiki
no flags Review Patch | Details | Formatted Diff | Diff
Patch (#if out for non-chrome) (39.97 KB, patch)
2011-11-15 22:26 PST, Taiju Tsuiki
no flags Review Patch | Details | Formatted Diff | Diff
Patch (omit FileSystem invalidation) (38.60 KB, patch)
2011-11-16 02:15 PST, Taiju Tsuiki
no flags Review Patch | Details | Formatted Diff | Diff
Patch (rebase) (38.50 KB, patch)
2011-11-16 02:26 PST, Taiju Tsuiki
no flags Review Patch | Details | Formatted Diff | Diff
Patch (32.92 KB, patch)
2011-11-17 01:04 PST, Taiju Tsuiki
no flags Review Patch | Details | Formatted Diff | Diff
Patch (33.27 KB, patch)
2011-11-18 10:55 PST, Taiju Tsuiki
no flags Review Patch | 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 Review Patch | Details | Formatted Diff | Diff
Patch (33.82 KB, patch)
2011-11-21 19:46 PST, Taiju Tsuiki
no flags Review Patch | Details | Formatted Diff | Diff
Patch (rebase) (33.83 KB, patch)
2011-11-23 16:25 PST, Taiju Tsuiki
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 2011-11-15 20:29:59 PST -------
Created an attachment (id=115309) [details]
Patch
------- Comment #2 From 2011-11-15 20:43:07 PST -------
Created an attachment (id=115312) [details]
Patch (rebase)
------- Comment #3 From 2011-11-15 21:04:50 PST -------
(From update of attachment 115312 [details])
Attachment 115312 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10487586
------- Comment #4 From 2011-11-15 21:23:02 PST -------
Created an attachment (id=115317) [details]
Patch (add CodeGenerator entry)
------- Comment #5 From 2011-11-15 21:28:23 PST -------
(From update of attachment 115317 [details])
Attachment 115317 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10372844
------- Comment #6 From 2011-11-15 22:26:00 PST -------
Created an attachment (id=115322) [details]
Patch (#if out for non-chrome)
------- Comment #7 From 2011-11-16 02:15:33 PST -------
Created an attachment (id=115351) [details]
Patch (omit FileSystem invalidation)
------- Comment #8 From 2011-11-16 02:26:42 PST -------
Created an attachment (id=115352) [details]
Patch (rebase)
------- Comment #9 From 2011-11-16 02:53:59 PST -------
(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.

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 From 2011-11-17 01:04:19 PST -------
Created an attachment (id=115540) [details]
Patch
------- Comment #11 From 2011-11-17 01:09:15 PST -------
(In reply to comment #9)
Thank you for reviewing.

> (From update of attachment 115352 [details] [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 From 2011-11-17 23:18:25 PST -------
(From update of attachment 115540 [details])
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 From 2011-11-18 10:55:15 PST -------
Created an attachment (id=115836) [details]
Patch
------- Comment #14 From 2011-11-18 11:00:32 PST -------
(From update of attachment 115540 [details])
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 From 2011-11-18 11:04:44 PST -------
(From update of attachment 115836 [details])
Attachment 115836 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10518166
------- Comment #16 From 2011-11-18 11:19:00 PST -------
(From update of attachment 115836 [details])
Attachment 115836 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10502017
------- Comment #17 From 2011-11-18 12:58:16 PST -------
(From update of attachment 115836 [details])
Attachment 115836 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10515176
------- Comment #18 From 2011-11-20 17:41:52 PST -------
Created an attachment (id=116015) [details]
Patch (add a entry to CodeGeneratorInspector.py)
------- Comment #19 From 2011-11-20 17:59:11 PST -------
(From update of attachment 116015 [details])
Fixed build error caused by conflict with recent upstream change.
------- Comment #20 From 2011-11-21 01:32:54 PST -------
(From update of attachment 116015 [details])
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 From 2011-11-21 19:46:25 PST -------
Created an attachment (id=116171) [details]
Patch
------- Comment #22 From 2011-11-21 19:49:13 PST -------
(From update of attachment 116015 [details])
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 From 2011-11-23 16:25:02 PST -------
Created an attachment (id=116449) [details]
Patch (rebase)
------- Comment #24 From 2011-11-24 00:01:55 PST -------
(From update of attachment 116449 [details])
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 From 2011-11-27 20:17:50 PST -------
(From update of attachment 116449 [details])
Thank you, pavel.
I'll fix it in next patch.
------- Comment #26 From 2011-11-28 02:00:57 PST -------
(From update of attachment 116449 [details])
Clearing flags on attachment: 116449

Committed r101236: <http://trac.webkit.org/changeset/101236>
------- Comment #27 From 2011-11-28 02:01:05 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #28 From 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 From 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