Bug 176568 – Clean up API::UIClient

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Clean up API::UIClient

[Alex Christensen]

Created attachment 320197 [details]
Patch

[Build Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Build Bot]

Attachment 320197 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/API/glib/WebKitUIClient.cpp:49:  The parameter name "resourceRequest" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit/UIProcess/API/glib/WebKitUIClient.cpp:49:  The parameter name "windowFeatures" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebKit/UIProcess/API/glib/WebKitUIClient.cpp:49:  The parameter name "navigationActionData" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Created attachment 320200 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r221771

[Ryan Haddad]

This change caused the following API tests to crash:
  WebKit.OpenAndCloseWindowAsync
  WebKit.OpenAsyncWithNil

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebKit              	0x000000010e5597d9 WebKit::UIDelegate::UIClient::createNewPage(WebKit::WebPageProxy&, WTF::Ref<API::FrameInfo>&&, WebCore::ResourceRequest&&, WebCore::WindowFeatures&&, WebKit::NavigationActionData&&, WTF::Function<void (WTF::RefPtr<WebKit::WebPageProxy>&&)>&&) + 993 (UIDelegate.mm:232)
1   com.apple.WebKit              	0x000000010e63b33f WebKit::WebPageProxy::createNewPage(WebKit::FrameInfoData const&, unsigned long long, WebCore::ResourceRequest&&, WebCore::WindowFeatures&&, WebKit::NavigationActionData&&, WTF::Ref<Messages::WebPageProxy::CreateNewPage::DelayedReply>&&) + 929 (WebPageProxy.cpp:3874)
2   com.apple.WebKit              	0x000000010e659454 void IPC::handleMessageDelayed<Messages::WebPageProxy::CreateNewPage, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(WebKit::FrameInfoData const&, unsigned long long, WebCore::ResourceRequest&&, WebCore::WindowFeatures&&, WebKit::NavigationActionData&&, WTF::Ref<Messages::WebPageProxy::CreateNewPage::DelayedReply>&&)>(IPC::Connection&, IPC::Decoder&, std::__1::unique_ptr<IPC::Encoder, std::__1::default_delete<IPC::Encoder> >&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(WebKit::FrameInfoData const&, unsigned long long, WebCore::ResourceRequest&&, WebCore::WindowFeatures&&, WebKit::NavigationActionData&&, WTF::Ref<Messages::WebPageProxy::CreateNewPage::DelayedReply>&&)) + 499 (HandleMessage.h:179)
3   com.apple.WebKit              	0x000000010e4599d3 IPC::MessageReceiverMap::dispatchSyncMessage(IPC::Connection&, IPC::Decoder&, std::__1::unique_ptr<IPC::Encoder, std::__1::default_delete<IPC::Encoder> >&) + 143 (MessageReceiverMap.cpp:140)
4   com.apple.WebKit              	0x000000010e6a848e WebKit::WebProcessProxy::didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::__1::unique_ptr<IPC::Encoder, std::__1::default_delete<IPC::Encoder> >&) + 28 (WebProcessProxy.cpp:606)
5   com.apple.WebKit              	0x000000010e422304 IPC::Connection::dispatchSyncMessage(IPC::Decoder&) + 202 (Connection.cpp:865)
6   com.apple.WebKit              	0x000000010e41f8b2 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) + 104 (Connection.cpp:926)
7   com.apple.WebKit              	0x000000010e422529 IPC::Connection::dispatchOneMessage() + 175 (Connection.cpp:959)
8   com.apple.JavaScriptCore      	0x000000010dc76ed4 WTF::RunLoop::performWork() + 164 (RunLoop.cpp:107)
9   com.apple.JavaScriptCore      	0x000000010dc770f2 WTF::RunLoop::performWork(void*) + 34 (RunLoopCF.cpp:39)
10  com.apple.CoreFoundation      	0x00007fffac7123e1 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
11  com.apple.CoreFoundation      	0x00007fffac6f365c __CFRunLoopDoSources0 + 556
12  com.apple.CoreFoundation      	0x00007fffac6f2b46 __CFRunLoopRun + 934
13  com.apple.CoreFoundation      	0x00007fffac6f2544 CFRunLoopRunSpecific + 420
14  com.apple.Foundation          	0x00007fffae1234e2 -[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 277
15  TestWebKitAPI                 	0x000000010cce386f TestWebKitAPI::Util::run(bool*) + 119 (UtilitiesCocoa.mm:34)
16  TestWebKitAPI                 	0x000000010cc481e7 WebKit_OpenAsyncWithNil_Test::TestBody() + 407 (OpenAndCloseWindow.mm:177)
17  TestWebKitAPI                 	0x000000010cd4b768 testing::Test::Run() + 92
18  TestWebKitAPI                 	0x000000010cd4bf40 testing::internal::TestInfoImpl::Run() + 178
19  TestWebKitAPI                 	0x000000010cd4c334 testing::TestCase::Run() + 188
20  TestWebKitAPI                 	0x000000010cd4f819 testing::internal::UnitTestImpl::RunAllTests() + 583
21  TestWebKitAPI                 	0x000000010cc92a1f TestWebKitAPI::TestsController::run(int, char**) + 131 (TestsController.cpp:84)
22  TestWebKitAPI                 	0x000000010cd27eda main + 344 (mainMac.mm:52)
23  libdyld.dylib                 	0x00007fffc242a235 start + 1

https://build.webkit.org/builders/Apple%20Sierra%20Release%20WK2%20%28Tests%29/builds/4156

[Ryan Haddad]

Reverted r221771 for reason:

This change caused two API tests to crash.

Committed r221787: <http://trac.webkit.org/changeset/221787>

[Michael Catanzaro]

(In reply to Ryan Haddad from comment #7)
> Reverted r221771 for reason:
> 
> This change caused two API tests to crash.
> 
> Committed r221787: <http://trac.webkit.org/changeset/221787>

I'm not sure it actually landed. You'll want to check when SVN is fixed.

[Alexey Proskuryakov]

SVN is actually the one thing that works. Git mirrors and Trac are broken.

[Alex Christensen]

Comment on attachment 320200 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:-227
> -        return nullptr;

Aha! I still need an early return here.

[Alex Christensen]

http://trac.webkit.org/r221899

[Joseph Pecoraro]

Comment on attachment 320200 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitUIClient.h:20
> -#ifndef WebKitUIClient_h
> -#define WebKitUIClient_h
> +#pragma once

Doesn't we normally not use pragma once in API headers?

[Joseph Pecoraro]

> Doesn't we normally not use pragma once in API headers?

There are already plenty of files in the same directory with `#pragma once` so it seems fine here. I was just asking in general.

[Michael Catanzaro]

We indeed don't use #pragma once in our public API headers, but that's actually an implementation header that's not installed. I guess it might not be obvious, but the easiest way to tell it's not installed is that it contains C++ code and follows WebKit coding style. The public headers are C and follow GNOME style.

[Radar WebKit Bug Importer]

<rdar://problem/34693747>

(In reply to Alex Christensen from comment #11)
> http://trac.webkit.org/r221899

This caused bug 182152.