Bug 72456 – [Inspector][FileSystem] Capture DOMFileSystem object

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Taiju Tsuiki]

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.

[Taiju Tsuiki]

Created attachment 115309 [details]
Patch

[Taiju Tsuiki]

Created attachment 115312 [details]
Patch (rebase)

[Gyuyoung Kim]

Comment on attachment 115312 [details]
Patch (rebase)

Attachment 115312 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10487586

[Taiju Tsuiki]

Created attachment 115317 [details]
Patch (add CodeGenerator entry)

[Gyuyoung Kim]

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

[Taiju Tsuiki]

Created attachment 115322 [details]
Patch (#if out for non-chrome)

[Taiju Tsuiki]

Created attachment 115351 [details]
Patch (omit FileSystem invalidation)

[Taiju Tsuiki]

Created attachment 115352 [details]
Patch (rebase)

[Pavel Feldman]

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

[Taiju Tsuiki]

Created attachment 115540 [details]
Patch

[Taiju Tsuiki]

(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.

[Kinuko Yasuda]

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?

[Taiju Tsuiki]

Created attachment 115836 [details]
Patch

[Taiju Tsuiki]

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.

[Gustavo Noronha (kov)]

Comment on attachment 115836 [details]
Patch

Attachment 115836 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10518166

[Gyuyoung Kim]

Comment on attachment 115836 [details]
Patch

Attachment 115836 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10502017

[Early Warning System Bot]

Comment on attachment 115836 [details]
Patch

Attachment 115836 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10515176

[Taiju Tsuiki]

Created attachment 116015 [details]
Patch (add a entry to CodeGeneratorInspector.py)

[Taiju Tsuiki]

Comment on attachment 116015 [details]
Patch (add a entry to CodeGeneratorInspector.py)

Fixed build error caused by conflict with recent upstream change.

[Pavel Feldman]

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.

[Taiju Tsuiki]

Created attachment 116171 [details]
Patch

[Taiju Tsuiki]

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.

[Taiju Tsuiki]

Created attachment 116449 [details]
Patch (rebase)

[Pavel Feldman]

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.

[Taiju Tsuiki]

Comment on attachment 116449 [details]
Patch (rebase)

Thank you, pavel.
I'll fix it in next patch.

[WebKit Review Bot]

Comment on attachment 116449 [details]
Patch (rebase)

Clearing flags on attachment: 116449

Committed r101236: <http://trac.webkit.org/changeset/101236>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because it broke all inspector tests on the Qt bot:
http://build.webkit.org/builders/Qt%20Linux%20Release/builds/40267

[Csaba Osztrogonác]

(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