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:

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

Taiju Tsuiki

Reported 2012-05-28 01:30:15 PDT

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

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 attachment proposed patch, testcase, etc.

Taiju Tsuiki

Comment 1 2012-05-28 01:33:20 PDT

Created attachment 144302 [details] Patch

Taiju Tsuiki

Comment 2 2012-05-28 01:56:48 PDT

Created attachment 144307 [details] Patch

Yury Semikhatsky

Comment 3 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;

Taiju Tsuiki

Comment 4 2012-05-28 04:25:44 PDT

Created attachment 144331 [details] Patch

Taiju Tsuiki

Comment 5 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!

Yury Semikhatsky

Comment 6 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) {

Taiju Tsuiki

Comment 7 2012-05-28 05:21:27 PDT

Created attachment 144337 [details] Patch

Taiju Tsuiki

Comment 8 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

Kinuko Yasuda

Comment 9 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(...) , ...

Taiju Tsuiki

Comment 10 2012-05-28 23:12:55 PDT

Created attachment 144451 [details] Patch

Taiju Tsuiki

Comment 11 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

WebKit Review Bot

Comment 12 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>

WebKit Review Bot

Comment 13 2012-05-29 09:00:56 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.