Bug 87635

Summary: Web Inspector: Add InspectorFileSystemAgent::FrontendProvider
Product: WebKit Reporter: Taiju Tsuiki <tzik>
Component: Web Inspector (Deprecated)Assignee: Taiju Tsuiki <tzik>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 68203, 87724    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Taiju Tsuiki 2012-05-28 01:30:15 PDT
InspectorFileSystemAgent needs weak reference to its frontend to perform asynchronous operation.
Comment 1 Taiju Tsuiki 2012-05-28 01:33:20 PDT
Created attachment 144302 [details]
Patch
Comment 2 Taiju Tsuiki 2012-05-28 01:56:48 PDT
Created attachment 144307 [details]
Patch
Comment 3 Yury Semikhatsky 2012-05-28 03:32:19 PDT
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;
Comment 4 Taiju Tsuiki 2012-05-28 04:25:44 PDT
Created attachment 144331 [details]
Patch
Comment 5 Taiju Tsuiki 2012-05-28 04:28:10 PDT
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!
Comment 6 Yury Semikhatsky 2012-05-28 05:13:59 PDT
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) {
Comment 7 Taiju Tsuiki 2012-05-28 05:21:27 PDT
Created attachment 144337 [details]
Patch
Comment 8 Taiju Tsuiki 2012-05-28 05:22:25 PDT
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
Comment 9 Kinuko Yasuda 2012-05-28 21:44:57 PDT
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(...)
    , ...
Comment 10 Taiju Tsuiki 2012-05-28 23:12:55 PDT
Created attachment 144451 [details]
Patch
Comment 11 Taiju Tsuiki 2012-05-28 23:15:29 PDT
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
Comment 12 WebKit Review Bot 2012-05-29 09:00:51 PDT
Comment on attachment 144451 [details]
Patch

Clearing flags on attachment: 144451

Committed r118783: <http://trac.webkit.org/changeset/118783>
Comment 13 WebKit Review Bot 2012-05-29 09:00:56 PDT
All reviewed patches have been landed.  Closing bug.