Bug 103465 – [chromium] Add latency calculation plumbing

• Component: New Bugs
• Status: RESOLVED WONTFIX

[John Bauman]

[chromium] Add latency calculation plumbing

[John Bauman]

Created attachment 176367 [details]
Patch

[John Bauman]

Created attachment 176576 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Darin Fisher (:fishd, Google)]

Comment on attachment 176576 [details]
Patch

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

> Source/WebKit/chromium/public/WebCompositorInputHandler.h:30
> +#include <stdint.h>

WebCommon.h already gets you int64_t, so you shouldn't need #include <stdint.h>

> Source/WebKit/chromium/public/WebCompositorInputHandler.h:48
>      virtual void handleInputEvent(const WebInputEvent&) = 0;

should the old handleInputEvent be modified to call the new one to help with migration?

[John Bauman]

Comment on attachment 176576 [details]
Patch

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

>> Source/WebKit/chromium/public/WebCompositorInputHandler.h:30
>> +#include <stdint.h>
> 
> WebCommon.h already gets you int64_t, so you shouldn't need #include <stdint.h>

It looks to me like it only adds up to int32_t, and then only on Windows. Maybe I'm missing something.

>> Source/WebKit/chromium/public/WebCompositorInputHandler.h:48
>>      virtual void handleInputEvent(const WebInputEvent&) = 0;
> 
> should the old handleInputEvent be modified to call the new one to help with migration?

I did that in the implementation in WebCompositorInputHandlerImpl, though I suppose I could move that change here.

[Darin Fisher (:fishd, Google)]

(In reply to comment #5)
> (From update of attachment 176576 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176576&action=review
> 
> >> Source/WebKit/chromium/public/WebCompositorInputHandler.h:30
> >> +#include <stdint.h>
> > 
> > WebCommon.h already gets you int64_t, so you shouldn't need #include <stdint.h>
> 
> It looks to me like it only adds up to int32_t, and then only on Windows. Maybe I'm missing something.

Hmm, that's surprising.  I would expect to see int64_t defined there.  Doesn't stddef.h end up including stdint.h?

I guess my main point is that it seems like WebCommon.h is trying to define int*_t, and if it is doing a poor job of that, then perhaps we should fix it.


> >> Source/WebKit/chromium/public/WebCompositorInputHandler.h:48
> >>      virtual void handleInputEvent(const WebInputEvent&) = 0;
> > 
> > should the old handleInputEvent be modified to call the new one to help with migration?
> 
> I did that in the implementation in WebCompositorInputHandlerImpl, though I suppose I could move that change here.

I see.  It is common to do it in the interface header, and to also add a comment indicating that the old function is DEPRECATED.

[John Bauman]

Created attachment 177383 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/Platform/chromium/public/WebCommon.h:77:  int64_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 14 files


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

[John Bauman]

Ok, I'll call that error a false-positive.

[Peter Beverloo (cr-android ews)]

Comment on attachment 177383 [details]
Patch

Attachment 177383 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15133185

[John Bauman]

Created attachment 177396 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/Platform/chromium/public/WebCommon.h:77:  int64_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 14 files


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

[John Bauman]

Created attachment 178290 [details]
Patch

[John Bauman]

Created attachment 179742 [details]
Patch

[John Bauman]

Created attachment 179800 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/Platform/chromium/public/WebCommon.h:77:  int64_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 17 files


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

[James Robinson]

Comment on attachment 179800 [details]
Patch

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

> Source/Platform/chromium/public/WebGraphicsContext3D.h:151
> +    class WebGraphicsLatencyInfoCallbackCHROMIUM {

As I mentioned on the chromium side, I think having this setup on the context itself is going to cause lots of dependency headaches.  Can you route this through something surface-aware?  Maybe cc::OutputSurface?

> Source/Platform/chromium/public/WebInputHandlerClient.h:57
> +    virtual bool scrollByIfPossible(int64_t, WebPoint point, WebSize size) { return scrollByIfPossible(point, size); }

Name the int64_t parameter, I can't figure out what it means from the signature alone.  There could be any number of scrollBy..() calls for the same input event (think of animations, flings, etc) so this is unclear.

> Source/WebKit/chromium/public/WebCompositorInputHandler.h:46
> +    virtual void handleInputEvent(const WebInputEvent&, int64_t inputNumber) = 0;

Instead of adding an additional parameter to every function that uses a WebInputEvent, did you consider putting it in the WebInputEvent struct itself?  Then you won't have to update all the interfaces and it will pass through functions that are ignorant of the inputNumber system without any extra code.

[John Bauman]

(In reply to comment #17)
> (From update of attachment 179800 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179800&action=review
> 
> 
> > Source/WebKit/chromium/public/WebCompositorInputHandler.h:46
> > +    virtual void handleInputEvent(const WebInputEvent&, int64_t inputNumber) = 0;
> 
> Instead of adding an additional parameter to every function that uses a WebInputEvent, did you consider putting it in the WebInputEvent struct itself?  Then you won't have to update all the interfaces and it will pass through functions that are ignorant of the inputNumber system without any extra code.

Yeah, I tried doing that originally, but the result was kind of odd. Every event had to be copied at the RenderWidgetHostImpl::SendInputEvent stage to add the input number (if I wanted to avoid polluting a ton of code with knowledge of the input number). It seems more straightforward to have the input number be lower-level, a part of ViewMsg_HandleInputEvent, and it can be manually passed through the few functions that need to know about it.

[John Bauman]

Created attachment 181135 [details]
Patch

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/Platform/ChangeLog', u'Source/Platf..." exit_code: 1
Source/Platform/chromium/public/WebCommon.h:77:  int64_t is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 16 files


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