Bug 127068 - Web Inspector: Page should use std::unique_ptr for InspectorController
Summary: Web Inspector: Page should use std::unique_ptr for InspectorController
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-01-15 15:00 PST by BJ Burg
Modified: 2014-01-18 11:11 PST (History)
20 users (show)

See Also:

Attachments
v1 (81.87 KB, patch)
2014-01-15 16:47 PST, BJ Burg 		no flags Details | Formatted Diff | Diff
v2 (83.65 KB, patch)
2014-01-16 09:41 PST, BJ Burg 		no flags Details | Formatted Diff | Diff
v3 (84.45 KB, patch)
2014-01-16 15:26 PST, BJ Burg 		joepeck: review+
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Show Obsolete (2) 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 BJ Burg 2014-01-15 15:00:44 PST 
I think i'll split this task from https://bugs.webkit.org/show_bug.cgi?id=126771 since this refactoring step mostly deals with call sites non-inspector code.
Comment 1 BJ Burg 2014-01-15 16:47:08 PST 
Created attachment 221317 [details]
v1
Comment 2 WebKit Commit Bot 2014-01-15 16:48:29 PST 
Attachment 221317 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/bindings/js/JSDOMWindowBase.cpp', u'Source/WebCore/dom/Node.cpp', u'Source/WebCore/inspector/InspectorController.cpp', u'Source/WebCore/inspector/InspectorController.h', u'Source/WebCore/inspector/InspectorFrontendClientLocal.cpp', u'Source/WebCore/inspector/InspectorOverlay.cpp', u'Source/WebCore/inspector/InspectorOverlay.h', u'Source/WebCore/inspector/InstrumentingAgents.cpp', u'Source/WebCore/inspector/WorkerInspectorController.cpp', u'Source/WebCore/inspector/WorkerInspectorController.h', u'Source/WebCore/loader/FrameLoader.cpp', u'Source/WebCore/page/ContextMenuController.cpp', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/page/Page.cpp', u'Source/WebCore/page/Page.h', u'Source/WebCore/page/PageDebuggable.cpp', u'Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/workers/WorkerGlobalScope.cpp', u'Source/WebCore/workers/WorkerGlobalScope.h', u'Source/WebCore/workers/WorkerMessagingProxy.cpp', u'Source/WebCore/workers/WorkerThread.cpp', u'Source/WebKit/efl/ChangeLog', u'Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp', u'Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp', u'Source/WebKit/efl/ewk/ewk_view.cpp', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp', u'Source/WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp', u'Source/WebKit/gtk/webkit/webkitwebinspector.cpp', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm', u'Source/WebKit/mac/WebInspector/WebInspector.mm', u'Source/WebKit/mac/WebInspector/WebNodeHighlighter.mm', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebCoreSupport/WebInspectorClient.cpp', u'Source/WebKit/win/WebInspector.cpp', u'Source/WebKit/win/WebNodeHighlight.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorFrontendClient.cpp', u'Source/WebKit2/WebProcess/WebPage/WebInspector.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebKit/win/WebNodeHighlight.cpp:168:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebKit/win/WebNodeHighlight.cpp:168:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Radar WebKit Bug Importer 2014-01-15 16:48:55 PST 
<rdar://problem/15830904>
Comment 4 Radar WebKit Bug Importer 2014-01-15 16:49:22 PST 
<rdar://problem/15830903>
Comment 5 BJ Burg 2014-01-15 16:57:31 PST 
Filed check-webkit-style bug: https://bugs.webkit.org/show_bug.cgi?id=127076
Comment 6 EFL EWS Bot 2014-01-15 17:00:48 PST 
Comment on attachment 221317 [details]
v1

Attachment 221317 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/5536423700594688
Comment 7 kov's GTK+ EWS bot 2014-01-15 18:11:20 PST 
Comment on attachment 221317 [details]
v1

Attachment 221317 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/5462541673168896
Comment 8 Sam Weinig 2014-01-15 18:54:33 PST 
Comment on attachment 221317 [details]
v1

r=me in principle.  But you should fix the builds.
Comment 9 Joseph Pecoraro 2014-01-15 19:39:04 PST 
View in context: https://bugs.webkit.org/attachment.cgi?id=221317&action=review

> Source/WebCore/inspector/InspectorController.h:80
> +    explicit InspectorController(Page&, InspectorClient*);

The explicit keyword is only needed on constructors with a single parameter, to prevent implicit conversions. It is not needed here (and on plenty of other cases in this patch).

> Source/WebCore/inspector/WorkerInspectorController.cpp:137
> +    m_frontendChannel.reset();

I think we prefer "= nullptr" instead of reset. I certainly have with Inspector unique_ptrs. Am I out of date on that?
Comment 10 BJ Burg 2014-01-16 09:41:31 PST 
Created attachment 221384 [details]
v2
Comment 11 WebKit Commit Bot 2014-01-16 09:43:12 PST 
Attachment 221384 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/bindings/js/JSDOMWindowBase.cpp', u'Source/WebCore/dom/Node.cpp', u'Source/WebCore/inspector/InspectorController.cpp', u'Source/WebCore/inspector/InspectorController.h', u'Source/WebCore/inspector/InspectorFrontendClientLocal.cpp', u'Source/WebCore/inspector/InspectorOverlay.cpp', u'Source/WebCore/inspector/InspectorOverlay.h', u'Source/WebCore/inspector/InstrumentingAgents.cpp', u'Source/WebCore/inspector/WorkerInspectorController.cpp', u'Source/WebCore/inspector/WorkerInspectorController.h', u'Source/WebCore/loader/FrameLoader.cpp', u'Source/WebCore/page/ContextMenuController.cpp', u'Source/WebCore/page/FrameView.cpp', u'Source/WebCore/page/Page.cpp', u'Source/WebCore/page/Page.h', u'Source/WebCore/page/PageDebuggable.cpp', u'Source/WebCore/platform/graphics/texmap/coordinated/CompositingCoordinator.cpp', u'Source/WebCore/testing/Internals.cpp', u'Source/WebCore/workers/WorkerGlobalScope.cpp', u'Source/WebCore/workers/WorkerGlobalScope.h', u'Source/WebCore/workers/WorkerMessagingProxy.cpp', u'Source/WebCore/workers/WorkerThread.cpp', u'Source/WebKit/efl/ChangeLog', u'Source/WebKit/efl/WebCoreSupport/DumpRenderTreeSupportEfl.cpp', u'Source/WebKit/efl/WebCoreSupport/InspectorClientEfl.cpp', u'Source/WebKit/efl/ewk/ewk_view.cpp', u'Source/WebKit/gtk/ChangeLog', u'Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp', u'Source/WebKit/gtk/WebCoreSupport/InspectorClientGtk.cpp', u'Source/WebKit/gtk/webkit/webkitwebinspector.cpp', u'Source/WebKit/mac/ChangeLog', u'Source/WebKit/mac/WebCoreSupport/WebInspectorClient.mm', u'Source/WebKit/mac/WebInspector/WebInspector.mm', u'Source/WebKit/mac/WebInspector/WebNodeHighlighter.mm', u'Source/WebKit/win/ChangeLog', u'Source/WebKit/win/WebCoreSupport/WebInspectorClient.cpp', u'Source/WebKit/win/WebInspector.cpp', u'Source/WebKit/win/WebNodeHighlight.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorClient.cpp', u'Source/WebKit2/WebProcess/WebCoreSupport/WebInspectorFrontendClient.cpp', u'Source/WebKit2/WebProcess/WebPage/WebInspector.cpp', '--commit-queue']" exit_code: 1
ERROR: Source/WebKit/win/WebNodeHighlight.cpp:168:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebKit/win/WebNodeHighlight.cpp:168:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 EFL EWS Bot 2014-01-16 09:56:29 PST 
Comment on attachment 221384 [details]
v2

Attachment 221384 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/5411048974712832
Comment 13 BJ Burg 2014-01-16 15:26:04 PST 
Created attachment 221420 [details]
v3
Comment 14 WebKit Commit Bot 2014-01-16 15:28:54 PST 
Attachment 221420 [details] did not pass style-queue:


ERROR: Source/WebKit/win/WebNodeHighlight.cpp:168:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
ERROR: Source/WebKit/win/WebNodeHighlight.cpp:168:  Wrong number of spaces before statement. (expected: 8)  [whitespace/indent] [4]
Total errors found: 2 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 EFL EWS Bot 2014-01-16 16:01:35 PST 
Comment on attachment 221420 [details]
v3

Attachment 221420 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/5504649196994560
Comment 16 BJ Burg 2014-01-16 16:38:12 PST 
I think I've addressed all of the build failures, but EWS is uncooperative at this time.
Comment 17 Joseph Pecoraro 2014-01-17 17:15:49 PST 
Comment on attachment 221420 [details]
v3

rs=me

I suggest when you land this that you watch the bots just in case.
Comment 18 BJ Burg 2014-01-18 11:11:50 PST 
Committed r162264: <http://trac.webkit.org/changeset/162264>