[Webkit-unassigned] [Bug 213306] WIP: we should be able to capture mouse, keyboard, and wheel events to investigate tracking

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=213306

--- Comment #2 from John Wilander <wilander at apple.com> ---
Comment on attachment 402138
  --> https://bugs.webkit.org/attachment.cgi?id=402138
Patch

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

Nice to see some code! We should consider gating this behind a runtime flag and to put the code behind the RESOURCE_LOAD_STATISTICS compile-time flag.

The build error on non-Cocoa platforms seems to be that they don't recognize the header file. I wonder if it needs to go to some CMake text file for those platforms?

> Source/WebCore/ChangeLog:3
> +        Add support to capture mouse, keyboard, and wheel events.

I think this title is a bit too generic. I would say "Capture mouse, keyboard, and wheel events for statistical purposes". I don't think we use periods in change log titles.

> Source/WebCore/ChangeLog:15
> +        * page/EventCollector.h: Added.

Make sure to move the new files to their correct alphabetical position in the Project Navigator.

> Source/WebKit/ChangeLog:11
> +        (WebKit::m_schemeRegistry): Deleted.

This looks worrying. However, looking at the code, I don't think it was deleted. Please make sure and then remove this line in the change log.

> Source/WebCore/page/EventCollector.cpp:2
> + * Copyright (C) 2018 Apple Inc. All rights reserved.

There is a template for creating new WebKit files which should get the year right. New file –> scroll down.

> Source/WebCore/page/EventCollector.cpp:45
> +    WTFLogAlways("Keyboard event with key %s, key identifier %s, code %s, text '%s'", platformKeyboardEvent.key().ascii().data(), platformKeyboardEvent.keyIdentifier().ascii().data(), platformKeyboardEvent.code().ascii().data(), platformKeyboardEvent.text().ascii().data());

I use .utf8(), not .ascii() for log out put of strings. That's safer than ASCII.

> Source/WebCore/page/EventCollector.cpp:66
> +    // TODO: Store in database

I know this is a comment that'll be removed soon but we always use periods in comments.

> Source/WebCore/page/EventCollector.h:2
> + * Copyright (C) 2015 Apple Inc. All rights reserved.

Same thing here. Use the template instead.

> Source/WebCore/page/EventCollector.h:51
> +class EventCollector

I think this name is too generic. Maybe EventStatisticsCollector?

> Source/WebCore/page/EventCollector.h:53
> +    // keeping this in WebCore for the moment; could also go to WebKit?

If you intend to land this comment: It needs to start with upper case and WebCore doesn't know about "WebKit" so we should probably not mention it in a comment.

> Source/WebCore/page/EventCollector.h:55
> +    EventCollector() { }

Was something in Xcode or the build telling you to add an empty constructor?

> Source/WebCore/page/EventCollector.h:63
> +    // something like this but with hashable key

If WallTime is not hashable you can try getting its raw seconds.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:114
> +    , m_eventCollector(EventCollector::create())

I'm not sure this does what you want it to do. You're creating two different EventCollector objects – one in the network process and one in every web content process. So when you call it here in the network process to say that something happened, it's not going to be the same object the ones in the web content processes. My impression is that such an object is only needed in the web content processes. Is the instance here in the network process only for the purposes of telling the web content process for a specific page ID that the page loaded? If so, I'm sure there is such an event already available in the web content process since the page code itself gets the onload event.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:535
> +    // TODO: send info to WebProcess

Period here too.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2713
> +    WebProcess::singleton().eventCollector().collectMouseEvent(page->identifier(), platformMouseEvent);

Unless we need to do something right away with the event, we try to collect it as late as possible so that performance critical code can run first. See if it makes sense to collect this later in the function.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2821
> +    WebProcess::singleton().eventCollector().collectKeyEvent(platform(keyboardEvent));

Same thing here on where to collect the data.

> Source/WebKit/WebProcess/WebProcess.h:645
> +    Ref<WebCore::EventCollector> m_eventCollector;

I think this name is too generic. Maybe m_eventStatisticsCollector?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20200617/cd3e6c07/attachment-0001.htm>