Bug 87635 - Web Inspector: Add InspectorFileSystemAgent::FrontendProvider
Summary: Web Inspector: Add InspectorFileSystemAgent::FrontendProvider
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Taiju Tsuiki
URL:
Keywords:
Depends on:
Blocks: 68203 87724
  Show dependency tree / graph
 
Reported: 2012-05-28 01:30 PDT by Taiju Tsuiki
Modified: 2012-05-29 09:00 PDT (History)
11 users (show)

See Also:

Attachments
Patch (5.71 KB, patch)
2012-05-28 01:33 PDT, Taiju Tsuiki 		no flags Details | Formatted Diff | Diff
Patch (5.69 KB, patch)
2012-05-28 01:56 PDT, Taiju Tsuiki 		no flags Details | Formatted Diff | Diff
Patch (4.84 KB, patch)
2012-05-28 04:25 PDT, Taiju Tsuiki 		no flags Details | Formatted Diff | Diff
Patch (4.82 KB, patch)
2012-05-28 05:21 PDT, Taiju Tsuiki 		no flags Details | Formatted Diff | Diff
Patch (5.01 KB, patch)
2012-05-28 23:12 PDT, Taiju Tsuiki 		no flags Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.