Bug 160605 - Lay WebProcess/UIProcess groundwork for an IPC GamepadProvider
Summary: Lay WebProcess/UIProcess groundwork for an IPC GamepadProvider
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on: 160623
Blocks: 134076 134675
  Show dependency treegraph
 
Reported: 2016-08-05 12:36 PDT by Brady Eidson
Modified: 2016-08-08 10:28 PDT (History)
4 users (show)

See Also:


Attachments
Patch (35.04 KB, patch)
2016-08-05 12:57 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (35.09 KB, patch)
2016-08-05 13:09 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (35.14 KB, patch)
2016-08-05 13:16 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (35.29 KB, patch)
2016-08-05 13:41 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
New PFL (34.47 KB, patch)
2016-08-05 22:22 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (18.73 KB, patch)
2016-08-08 09:54 PDT, Brady Eidson
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2016-08-05 12:36:26 PDT
Lay WebProcess/UIProcess groundwork for an IPC GamepadProvider
Comment 1 Brady Eidson 2016-08-05 12:57:38 PDT
Created attachment 285442 [details]
Patch
Comment 2 Brady Eidson 2016-08-05 13:09:11 PDT
Created attachment 285443 [details]
Patch
Comment 3 Brady Eidson 2016-08-05 13:16:27 PDT
Created attachment 285445 [details]
Patch
Comment 4 Brady Eidson 2016-08-05 13:21:47 PDT
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.
Comment 5 Brady Eidson 2016-08-05 13:28:39 PDT
(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.
Comment 6 Michael Catanzaro 2016-08-05 13:37:12 PDT
(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!
Comment 7 Brady Eidson 2016-08-05 13:38:55 PDT
(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!
Comment 8 Brady Eidson 2016-08-05 13:41:16 PDT
Created attachment 285446 [details]
Patch
Comment 9 WebKit Commit Bot 2016-08-05 14:56:23 PDT
Comment on attachment 285446 [details]
Patch

Clearing flags on attachment: 285446

Committed r204195: <http://trac.webkit.org/changeset/204195>
Comment 10 WebKit Commit Bot 2016-08-05 14:56:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 WebKit Commit Bot 2016-08-05 17:53:00 PDT
Re-opened since this is blocked by bug 160623
Comment 12 Ryan Haddad 2016-08-05 17:55:37 PDT
(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
Comment 13 Brady Eidson 2016-08-05 22:22:02 PDT
Created attachment 285482 [details]
New PFL
Comment 14 Brady Eidson 2016-08-05 22:24:26 PDT
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)
Comment 15 WebKit Commit Bot 2016-08-06 01:01:44 PDT
Comment on attachment 285482 [details]
New PFL

Clearing flags on attachment: 285482

Committed r204222: <http://trac.webkit.org/changeset/204222>
Comment 16 WebKit Commit Bot 2016-08-06 01:01:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Darin Adler 2016-08-07 10:10:42 PDT
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?
Comment 18 Darin Adler 2016-08-07 10:17:15 PDT
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.
Comment 19 Brady Eidson 2016-08-08 09:05:48 PDT
(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.
Comment 20 Brady Eidson 2016-08-08 09:48:41 PDT
(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.
Comment 21 Brady Eidson 2016-08-08 09:54:48 PDT
Reopening to attach new patch.
Comment 22 Brady Eidson 2016-08-08 09:54:50 PDT
Created attachment 285570 [details]
Patch
Comment 23 Brady Eidson 2016-08-08 09:55:30 PDT
New patch addressing all of Darin's review feedback.

Putting straight in cq+
Comment 24 WebKit Commit Bot 2016-08-08 09:55:49 PDT
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
Comment 25 Brady Eidson 2016-08-08 10:28:44 PDT
(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