Bug 160605 – Lay WebProcess/UIProcess groundwork for an IPC GamepadProvider

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

Lay WebProcess/UIProcess groundwork for an IPC GamepadProvider

[Brady Eidson]

Created attachment 285442 [details]
Patch

[Brady Eidson]

Created attachment 285443 [details]
Patch

[Brady Eidson]

Created attachment 285445 [details]
Patch

[Brady Eidson]

This EFL build error appears to be bull.

Source/WebKit2/CMakeFiles/WebKit2.dir/WebProcess/Gamepad/WebGamepadProvider.cpp.o.d -o Source/WebKit2/CMakeFiles/WebKit2.dir/WebProcess/Gamepad/WebGamepadProvider.cpp.o -c ../../Source/WebKit2/WebProcess/Gamepad/WebGamepadProvider.cpp
In file included from ../../Source/WebKit2/WebProcess/Gamepad/WebGamepadProvider.cpp:27:0:
../../Source/WebKit2/WebProcess/Gamepad/WebGamepadProvider.h:33:35: error: 'WebCore' has not been declared
 class WebGamepadProvider : public WebCore::GamepadProvider {

Except... yes, it has been, in <WebCore/GamepadProvider.h> which is included in this file.

[Brady Eidson]

(In reply to comment #4)
> This EFL build error appears to be bull.
> 
> Source/WebKit2/CMakeFiles/WebKit2.dir/WebProcess/Gamepad/WebGamepadProvider.
> cpp.o.d -o
> Source/WebKit2/CMakeFiles/WebKit2.dir/WebProcess/Gamepad/WebGamepadProvider.
> cpp.o -c ../../Source/WebKit2/WebProcess/Gamepad/WebGamepadProvider.cpp
> In file included from
> ../../Source/WebKit2/WebProcess/Gamepad/WebGamepadProvider.cpp:27:0:
> ../../Source/WebKit2/WebProcess/Gamepad/WebGamepadProvider.h:33:35: error:
> 'WebCore' has not been declared
>  class WebGamepadProvider : public WebCore::GamepadProvider {
> 
> Except... yes, it has been, in <WebCore/GamepadProvider.h> which is included
> in this file.

I tried to fix other Linux build errors a few times, but this one has got me confused.

Clearly the Mac build works, clearly the C++ is right - It's entirely unclear why the linux builds don't think the WebCore namespace exists, etc etc.

[Michael Catanzaro]

(In reply to comment #4)
> Except... yes, it has been, in <WebCore/GamepadProvider.h> which is included
> in this file.

Nope, because ENABLE(GAMEPAD) is OFF for both ports!

[Brady Eidson]

(In reply to comment #6)
> (In reply to comment #4)
> > Except... yes, it has been, in <WebCore/GamepadProvider.h> which is included
> > in this file.
> 
> Nope, because ENABLE(GAMEPAD) is OFF for both ports!

Duh!  Okay!

[Brady Eidson]

Created attachment 285446 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 285446 [details]
Patch

Clearing flags on attachment: 285446

Committed r204195: <http://trac.webkit.org/changeset/204195>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 160623

[Ryan Haddad]

(In reply to comment #11)
> Re-opened since this is blocked by bug 160623
LayoutTests and API tests started failing after this change.

ASSERTION FAILED: m_processPoolsUsingGamepads.contains(&pool)
/Volumes/Data/slave/elcapitan-debug/build/Source/WebKit2/UIProcess/Gamepad/UIGamepadProvider.cpp(77) : void WebKit::UIGamepadProvider::processPoolStoppedUsingGamepads(WebKit::WebProcessPool &)
1   0x106d5fff0 WTFCrash
2   0x109763af2 WebKit::UIGamepadProvider::processPoolStoppedUsingGamepads(WebKit::WebProcessPool&)
3   0x109ac03e2 WebKit::WebProcessPool::processStoppedUsingGamepads(WebKit::WebProcessProxy*)
4   0x109ac0368 WebKit::WebProcessPool::disconnectProcess(WebKit::WebProcessProxy*)
5   0x109aefbf9 WebKit::WebProcessProxy::shutDown()
6   0x109af016a WebKit::WebProcessProxy::removeWebPage(unsigned long long)
7   0x109948898 WebKit::WebPageProxy::close()
8   0x109bb28e1 WebKit::WebViewImpl::~WebViewImpl()
9   0x109bb2e35 WebKit::WebViewImpl::~WebViewImpl()
10  0x109c8b776 -[WKView dealloc]
11  0x7fff9b751ac4 (anonymous namespace)::AutoreleasePoolPage::pop(void*)
12  0x7fff8f3c0c12 _CFAutoreleasePoolPop
13  0x7fff940059ea -[NSAutoreleasePool drain]
14  0x1057cc84c main
15  0x7fff87f615ad start
16  0x2

https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/builds/7132

[Brady Eidson]

Created attachment 285482 [details]
New PFL

[Brady Eidson]

With the updated patch, API tests all run fine locally, and layout tests are 50% the way through without issue (finishing up as I type)

[WebKit Commit Bot]

Comment on attachment 285482 [details]
New PFL

Clearing flags on attachment: 285482

Committed r204222: <http://trac.webkit.org/changeset/204222>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 285446 [details]
Patch

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

Looks like you possibly already landed this?

> Source/WebKit2/UIProcess/Gamepad/UIGamepadProvider.cpp:72
> +    if (m_processPoolsUsingGamepads.size() == 1)
> +        platformStartMonitoringGamepads();

Same comment as above about the size 1 check.

> Source/WebKit2/UIProcess/Gamepad/UIGamepadProvider.cpp:81
> +    if (m_processPoolsUsingGamepads.isEmpty())
> +        platformStopMonitoringGamepads();

Same comment as above about the empty set check.

> Source/WebKit2/UIProcess/Gamepad/UIGamepadProvider.h:30
> +#include <WebCore/GamepadProvider.h>

I don’t understand why this include is needed.

> Source/WebKit2/UIProcess/Gamepad/UIGamepadProvider.h:41
> +    UIGamepadProvider();
> +    ~UIGamepadProvider() final;

Is there a way to avoid having these be public? Only the singleton function needs to run the constructor, and ideally the destructor would be deleted rather than final; we don’t want to generate the untested code for destroying an object that we never intend to destroy.

> Source/WebKit2/UIProcess/Gamepad/UIGamepadProvider.h:47
> +    void platformGamepadConnected(WebCore::PlatformGamepad&) final;
> +    void platformGamepadDisconnected(WebCore::PlatformGamepad&) final;
> +    void platformGamepadInputActivity() final;

Should these be private?

> Source/WebKit2/UIProcess/Gamepad/mac/UIGamepadProviderHID.cpp:40
> +    HIDGamepadProvider::singleton().startMonitoringGamepads(this);

Why does this function take a pointer rather than a reference?

> Source/WebKit2/UIProcess/Gamepad/mac/UIGamepadProviderHID.cpp:46
> +    HIDGamepadProvider::singleton().stopMonitoringGamepads(this);

Same question.

> Source/WebKit2/UIProcess/WebProcessPool.cpp:1248
> +    auto* webProcessProxy = WebProcessProxy::fromConnection(&connection);

Inside a short function like this, might be nicer to use a shorter name like either proxy or process for this local variable.

> Source/WebKit2/UIProcess/WebProcessPool.cpp:1256
> +    if (m_processesUsingGamepads.size() == 1)
> +        UIGamepadProvider::singleton().processPoolStartedUsingGamepads(*this);

Same comment as above about the size 1 check.

> Source/WebKit2/UIProcess/WebProcessPool.cpp:1261
> +    auto* webProcessProxy = WebProcessProxy::fromConnection(&connection);

Inside a short function like this, might be nicer to use a shorter name like either proxy or process for this local variable.

> Source/WebKit2/UIProcess/WebProcessPool.cpp:1274
> +    if (m_processesUsingGamepads.isEmpty())
> +        UIGamepadProvider::singleton().processPoolStoppedUsingGamepads(*this);

Same comment as above about the empty set check. Except unlike the others, this doesn’t have the assertion inside the function. Maybe it should.

> Source/WebKit2/UIProcess/WebProcessPool.h:394
> +    void processStoppedUsingGamepads(WebProcessProxy*);

Should take a reference, not a pointer.

> Source/WebKit2/UIProcess/WebProcessPool.h:396
> +    HashSet<WebProcessProxy*> m_processesUsingGamepads;

Normally we try to group data member declarations together rather than interspersing them among the function members even if it means multiple conditional sections. So could this go with the other data members?

> Source/WebKit2/WebProcess/Gamepad/WebGamepadProvider.cpp:59
> +    if (m_clients.size() == 1)
> +        WebProcess::singleton().send(Messages::WebProcessPool::StartedUsingGamepads(), 0);

Even though we assert that the set doesn’t already contain a client, code would be more robust if it checked for emptiness before and cached that in a boolean rather than checking the size after for 1. That way if the assertion is wrong we still can’t get mismatched started/stopped messages.

> Source/WebKit2/WebProcess/Gamepad/WebGamepadProvider.cpp:68
> +    if (m_clients.isEmpty())
> +        WebProcess::singleton().send(Messages::WebProcessPool::StoppedUsingGamepads(), 0);

Even though we assert that the set contains the client beforehand, code would be robust if it checked the return value of remove and only checked if the set is empty if the remove returned true (actually removed something). That way if the assertion is wrong we still can’t get mismatched started/stopped messages.

> Source/WebKit2/WebProcess/Gamepad/WebGamepadProvider.h:38
> +    WebGamepadProvider();
> +    ~WebGamepadProvider() final;

Is there a way to avoid having these be public? Only the NeverDestroyed inside the singleton function needs to run the constructor, and ideally the destructor would be deleted rather than final; we don’t want to generate the untested code for destroying an object that we never intend to destroy.

> Source/WebKit2/WebProcess/Gamepad/WebGamepadProvider.h:44
> +    void startMonitoringGamepads(WebCore::GamepadProviderClient*) final;
> +    void stopMonitoringGamepads(WebCore::GamepadProviderClient*) final;
> +    const Vector<WebCore::PlatformGamepad*>& platformGamepads() final;

Can we make these private instead of public?

[Darin Adler]

Comment on attachment 285446 [details]
Patch

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

>> Source/WebKit2/WebProcess/Gamepad/WebGamepadProvider.cpp:59
>> +        WebProcess::singleton().send(Messages::WebProcessPool::StartedUsingGamepads(), 0);
> 
> Even though we assert that the set doesn’t already contain a client, code would be more robust if it checked for emptiness before and cached that in a boolean rather than checking the size after for 1. That way if the assertion is wrong we still can’t get mismatched started/stopped messages.

This is the size 1 comment I kept referring to as "above". Guess it wasn’t above.

>> Source/WebKit2/WebProcess/Gamepad/WebGamepadProvider.cpp:68
>> +        WebProcess::singleton().send(Messages::WebProcessPool::StoppedUsingGamepads(), 0);
> 
> Even though we assert that the set contains the client beforehand, code would be robust if it checked the return value of remove and only checked if the set is empty if the remove returned true (actually removed something). That way if the assertion is wrong we still can’t get mismatched started/stopped messages.

This is the empty set check comment I kept referring to as "above". Guess it wasn’t above.

[Brady Eidson]

(In reply to comment #17)
> Comment on attachment 285446 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285446&action=review
> 
> Looks like you possibly already landed this?
> 

I did, but am revisiting it anyways.

Re: all of your feedback about set management, will do.

Re: your feedback on making constructors private an deleting destructors, I didn't simply because I would've had to friend NeverDestroyed<>, but we do that elsewhere so I'll also do it here.

Re: your feedback on pointer vs reference, that's leftover from years ago, just before we aggressively started referenc-izing the code base. Will-do now.

[Brady Eidson]

(In reply to comment #19)
> (In reply to comment #17)
> > Comment on attachment 285446 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=285446&action=review
> > 
> > Looks like you possibly already landed this?

> 
> Re: your feedback on making constructors private an deleting destructors, I
> didn't simply because I would've had to friend NeverDestroyed<>, but we do
> that elsewhere so I'll also do it here.
> 

I'm going to punt on deleting the destructors, because the base GamepadProvider class does currently expose a virtual destructor meaning it can't be deleted in derived classes.

Guaranteeing that any GamepadProvider is never deleted, and therefore deleting the constructor on the base class *is* something that should be possible, but it can wait.

In the meantime, I made all the derived class destructors private.

[Brady Eidson]

Reopening to attach new patch.

[Brady Eidson]

Created attachment 285570 [details]
Patch

[Brady Eidson]

New patch addressing all of Darin's review feedback.

Putting straight in cq+

[WebKit Commit Bot]

Comment on attachment 285570 [details]
Patch

Rejecting attachment 285570 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 285570, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Darin's suggestions found in /Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog does not appear to be a valid reviewer according to contributors.json.
/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.webkit.org/results/1835206

[Brady Eidson]

(In reply to comment #24)
> Comment on attachment 285570 [details]
> Patch
> 
> Rejecting attachment 285570 [details] from commit-queue.
> 
> Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch',
> '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01',
> 'validate-changelog', '--check-oops', '--non-interactive', 285570,
> '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
> 
> Darin's suggestions found in
> /Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog does not appear to be a
> valid reviewer according to contributors.json.
> /Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid
> reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case
> insensitive).
> 
> Full output: http://webkit-queues.webkit.org/results/1835206

Well, if the CQ won't let me be all cute with the reviewed-by-line, I'll land manually.

https://trac.webkit.org/changeset/204257