Bug 87635 – Web Inspector: Add InspectorFileSystemAgent::FrontendProvider

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

[Taiju Tsuiki]

InspectorFileSystemAgent needs weak reference to its frontend to perform asynchronous operation.

[Taiju Tsuiki]

Created attachment 144302 [details]
Patch

[Taiju Tsuiki]

Created attachment 144307 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 144307 [details]
Patch

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

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:54
> +    InspectorFrontend::FileSystem* getFrontend() const

style nit: we don't use get prefixes in WebKit, simply frontend()

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:102
> +        m_frontendProvider->enable();

You could store a pointer to the agent itself, that way you wouldn't need to propagate enable/disable calls to the FrontendProvider, the getFrontend() method would look like

if (m_agent && m_agent->m_enabled)
     return m_agent->mfrontend;
return 0;

[Taiju Tsuiki]

Created attachment 144331 [details]
Patch

[Taiju Tsuiki]

Comment on attachment 144307 [details]
Patch

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

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:54
>> +    InspectorFrontend::FileSystem* getFrontend() const
> 
> style nit: we don't use get prefixes in WebKit, simply frontend()

Done

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:102
>> +        m_frontendProvider->enable();
> 
> You could store a pointer to the agent itself, that way you wouldn't need to propagate enable/disable calls to the FrontendProvider, the getFrontend() method would look like
> 
> if (m_agent && m_agent->m_enabled)
>      return m_agent->mfrontend;
> return 0;

Done

Looks cleaner. Thanks!

[Yury Semikhatsky]

Comment on attachment 144331 [details]
Patch

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

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:82
> +    if (m_frontendProvider.get())

No need for .get(), if (m_frontendProvider) should work

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:111
> +    if (m_frontendProvider.get()) {

if (m_frontendProvider.get()) { -> if (m_frontendProvider) {

[Taiju Tsuiki]

Created attachment 144337 [details]
Patch

[Taiju Tsuiki]

Comment on attachment 144331 [details]
Patch

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

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:82
>> +    if (m_frontendProvider.get())
> 
> No need for .get(), if (m_frontendProvider) should work

Done

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:111
>> +    if (m_frontendProvider.get()) {
> 
> if (m_frontendProvider.get()) { -> if (m_frontendProvider) {

Done

[Kinuko Yasuda]

Comment on attachment 144337 [details]
Patch

Drive-by comments.

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

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:68
> +    FrontendProvider(InspectorFileSystemAgent* agent, InspectorFrontend::FileSystem* frontend) : m_agent(agent), m_frontend(frontend) { }

style-nit: each initializer should be on a separate line

> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:125
>      : InspectorBaseAgent<InspectorFileSystemAgent>("FileSystem", instrumentingAgents, state),

style-nit: constructor style looks different from the style guide?

MyClass::MyClass(...)
    : m_member1(...)
    , m_member2(...)
    , ...

[Taiju Tsuiki]

Created attachment 144451 [details]
Patch

[Taiju Tsuiki]

Comment on attachment 144337 [details]
Patch

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

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:68
>> +    FrontendProvider(InspectorFileSystemAgent* agent, InspectorFrontend::FileSystem* frontend) : m_agent(agent), m_frontend(frontend) { }
> 
> style-nit: each initializer should be on a separate line

Done

>> Source/WebCore/inspector/InspectorFileSystemAgent.cpp:125
>>      : InspectorBaseAgent<InspectorFileSystemAgent>("FileSystem", instrumentingAgents, state),
> 
> style-nit: constructor style looks different from the style guide?
> 
> MyClass::MyClass(...)
>     : m_member1(...)
>     , m_member2(...)
>     , ...

Done

[WebKit Review Bot]

Comment on attachment 144451 [details]
Patch

Clearing flags on attachment: 144451

Committed r118783: <http://trac.webkit.org/changeset/118783>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.