| Summary: | Clean up API::UIClient | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | beidson, berto, buildbot, cgarcia, gustavo, joepeck, mcatanzaro, mitz, ryanhaddad, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Alex Christensen
2017-09-07 15:58:21 PDT
Created attachment 320197 [details]
Patch
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 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.
Created attachment 320200 [details]
Patch
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 Reverted r221771 for reason: This change caused two API tests to crash. Committed r221787: <http://trac.webkit.org/changeset/221787> (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. SVN is actually the one thing that works. Git mirrors and Trac are broken. 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. 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? > 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.
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. (In reply to Alex Christensen from comment #11) > http://trac.webkit.org/r221899 This caused bug 182152. |