Created attachment 115309 [details] Patch

Created attachment 115312 [details] Patch (rebase)

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

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

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

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

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

Created attachment 115352 [details] Patch (rebase)

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

Created attachment 115540 [details] Patch

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

Created attachment 115836 [details] Patch

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 on attachment 115836 [details] Patch Attachment 115836 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/10518166

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

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

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

Comment on attachment 116015 [details] Patch (add a entry to CodeGeneratorInspector.py) Fixed build error caused by conflict with recent upstream change.

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.

Created attachment 116171 [details] Patch

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.

Created attachment 116449 [details] Patch (rebase)

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 on attachment 116449 [details] Patch (rebase) Thank you, pavel. I'll fix it in next patch.

Comment on attachment 116449 [details] Patch (rebase) Clearing flags on attachment: 116449 Committed r101236: <http://trac.webkit.org/changeset/101236>

All reviewed patches have been landed. Closing bug.

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

(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