852257 - Implement Gamepad.timestamp property

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(not currently active) Ted Mielczarek]

See spec URL.

Comment 1

[Josh Matthews [:jdm]]

Also see http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Gamepad.webidl?force=1 and http://mxr.mozilla.org/mozilla-central/source/dom/gamepad/Gamepad.cpp?force=1 .

Comment 2

[Snigdha Agarwal]

Is the timestamp function supposed to look something like the functions here: https://github.com/etiennesegonzac/mozilla-central/blob/master/xpcom/ds/TimeStamp.cpp ?
(There seems to be some issue with my IRC, I can't connect)

Comment 3

[Snigdha Agarwal]

Attachment: timestamp property implemented

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 8367660 [details] [diff] [review]
timestamp property implemented

Review of attachment 8367660 [details] [diff] [review]:
-----------------------------------------------------------------

This is a great start, thanks for taking this on! You have a few things to fix up, but we should be able to get this in landable shape pretty soon.

I'm going to set this to r-, that just means it's not ready to land yet.

::: dom/gamepad/Gamepad.cpp
@@ +6,5 @@
>  #include "nsAutoPtr.h"
>  #include "nsTArray.h"
>  #include "nsVariant.h"
>  #include "mozilla/dom/GamepadBinding.h"
> +#include "nsGlobalWindow.h"

I think you only really need to #include nsPIDOMWindow.h, since you're not using nsGlobalWindow directly.

@@ +40,5 @@
>    }
>    mAxes.InsertElementsAt(0, aNumAxes, 0.0f);
>  }
>  
> +

Looks like you accidentally put an extra line here, you don't need this.

@@ +72,5 @@
>    }
>  }
>  
> +DOMHighResTimeStamp
> +Gamepad::Timestamp()

The indentation in this method is really inconsistent, look at the surrounding methods and try to format it the same way. We have a style guide you can look at if you're not sure:
https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Naming_and_Formatting_code

@@ +74,5 @@
>  
> +DOMHighResTimeStamp
> +Gamepad::Timestamp()
> +{
> +  nsCOMPtr<nsPIDOMWindow> newWindow;

You don't actually assign anything to newWindow! I think what you want here is:
nsCOMPtr<nsPIDOMWindow> newWindow(do_QueryInterface(mParent));

@@ +75,5 @@
> +DOMHighResTimeStamp
> +Gamepad::Timestamp()
> +{
> +  nsCOMPtr<nsPIDOMWindow> newWindow;
> +  nsPerformance* perf=nullptr;

Just move this down to where you call GetPerformance().

@@ +79,5 @@
> +  nsPerformance* perf=nullptr;
> +  if(newWindow)
> +	perf = newWindow->GetPerformance();
> +  if (perf) {
> +	mtimestamp =  perf->GetDOMTiming()->TimeStampToDOMHighRes(TimeStamp::Now());

So this code is correct, but it's not in the right place! Good job on putting all the pieces together, we just need to shuffle them a little.

I think what we want is to set mtimestamp when the Gamepad data changes. That means you probably want to put most of this code into a different method, maybe something like void UpdateTimestamp(), and then call it from all the methods that change the data, like SetButton, SetAxis, SyncState (probably at the end of the constructor as well, so we set an initial value for it.)

Then this method will simply "return mTimestamp;" You could inline that right into Gamepad.h like some of the other simple methods.

::: dom/gamepad/Gamepad.h
@@ +104,5 @@
>  
>    // Current state of buttons, axes.
>    nsTArray<nsRefPtr<GamepadButton>> mButtons;
>    nsTArray<double> mAxes;
> +  DOMHighResTimeStamp mtimestamp;

Can you capitalize this as "mTimestamp" for consistency with the other members?

Comment 5

[Snigdha Agarwal]

Attachment: Changes made

Comment 6

[(not currently active) Ted Mielczarek]

Comment on attachment 8368469 [details] [diff] [review]
Changes made

Review of attachment 8368469 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really good, there are just a few minor formatting things I'd like to see cleaned up before we land it, and I'd also like to see a slightly more thorough Mochitest. I'm happy to help with that, find me on IRC (or ask others in #introduction, since I'm going to be away for most of this weekend).

I'm granting r+ on this, but you'll need to upload a patch that addresses my points before setting checkin-needed.

In terms of a Mochitest, there's not really a good one you can piggyback on, but you may be able to start with one of the existing tests, like this one:
http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/gamepad/test_gamepad_connect_events.html?force=1

rip out all the frame logic, and just add two tests:
1) Add a gamepad, press a button
2) On the first event received, save Gamepad.timestamp, press a button again
3) On the second event received, verify that Gamepad.timestamp is now >= the old timestamp.

::: dom/gamepad/Gamepad.cpp
@@ +6,5 @@
>  #include "nsAutoPtr.h"
>  #include "nsTArray.h"
>  #include "nsVariant.h"
>  #include "mozilla/dom/GamepadBinding.h"
> +#include "nsPIDOMWindow.h"

Slight nit: I think the #includes are ordered alphabetically here, so this would go after nsAutoPtr.h. :)

@@ +21,5 @@
>  NS_INTERFACE_MAP_END
>  
>  NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_2(Gamepad, mParent, mButtons)
>  
> +void 

You've got a few lines with trailing whitespace here, you can see them if you click the "review" link on the patch in Bugzilla. You may want to find out if you can configure your text editor to make this obvious.

@@ +25,5 @@
> +void 
> +Gamepad::UpdateTimestamp()
> +{
> +  nsCOMPtr<nsPIDOMWindow> newWindow(do_QueryInterface(mParent));
> +  nsPerformance* perf=nullptr;

Move this down inside the if (newWindow) block where you call GetPerformance, like:
if (newWindow) {
   nsPerformance* perf = newWindow->GetPerformance();
   if (perf) {
     ...

Also, spacing nit, use one space on either side of the =.

@@ +26,5 @@
> +Gamepad::UpdateTimestamp()
> +{
> +  nsCOMPtr<nsPIDOMWindow> newWindow(do_QueryInterface(mParent));
> +  nsPerformance* perf=nullptr;
> +  if(newWindow){

Spacing nit: space after the if and before the {

@@ -71,2 @@
>  }
> -

Looks like you accidentally deleted this blank line, please put it back.

::: dom/gamepad/Gamepad.h
@@ +33,5 @@
>            uint32_t aNumButtons, uint32_t aNumAxes);
>    NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>    NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(Gamepad)
>  
> +  void UpdateTimestamp(); 

You can make this method private (just move it down with the private methods) since it's only called internally.

@@ -36,5 @@
>    void SetConnected(bool aConnected);
>    void SetButton(uint32_t aButton, bool aPressed, double aValue);
>    void SetAxis(uint32_t aAxis, double aValue);
>    void SetIndex(uint32_t aIndex);
> -

nit: Another deleted blank line.

Comment 7

[(not currently active) Ted Mielczarek]

I know you're still working on the Mochitest issues, but could you upload an updated copy of the rest of the patch? I realize that my review here isn't sufficient since I'm not a DOM peer, so we can get someone else to review this.

Comment 8

[Snigdha Agarwal]

Attachment: Mochitest added

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 8380980 [details] [diff] [review]
Mochitest added

Review of attachment 8380980 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, this totally fell out of my universe. Overall this looks good, I just have one issue and a few nitpicky things. This needs review by an actual DOM peer, so I'll tag smaug for that. If you don't have time to fix up this patch for my feedback comments and any review comments from him I will get that done for you.

::: dom/gamepad/Gamepad.cpp
@@ +26,5 @@
> +Gamepad::UpdateTimestamp()
> +{
> +  nsCOMPtr<nsPIDOMWindow> newWindow(do_QueryInterface(mParent));
> +  if(newWindow) {
> +    nsPerformance* perf = newWindow->GetPerformance();

I see some callers in the tree that use a raw pointer, and others that use an nsRefPtr. I'm not sure what the right thing is here, but I suspect you probably need an nsRefPtr here.

::: dom/gamepad/Gamepad.h
@@ +37,5 @@
>    void SetConnected(bool aConnected);
>    void SetButton(uint32_t aButton, bool aPressed, double aValue);
>    void SetAxis(uint32_t aAxis, double aValue);
>    void SetIndex(uint32_t aIndex);
> +  

nit: you have trailing whitespace here and elsewhere.

::: dom/tests/mochitest/gamepad/test_check_timestamp.html
@@ +16,5 @@
> +                                      4, // buttons
> +                                      2);// axes
> +
> +var timea=0;
> +window.addEventListener("gamepadbuttondown", connecthandler);

connecthandler is an odd name for a buttondown listener.

@@ +22,5 @@
> +var i=1;
> +GamepadService.newButtonEvent(index, 0, true);
> +GamepadService.newButtonEvent(index, 0, true);
> +
> +function checktimestamp(){

This isn't really checking a timestamp, I'd rename this.

@@ +41,5 @@
> +  if(i==0){
> +    SpecialPowers.executeSoon(checktimestamp);
> +  }
> +  else{
> +    i--;

You could just use true/false here.

::: dom/webidl/Gamepad.webidl
@@ +46,5 @@
>     */
>    [Pure, Cached, Frozen]
>    readonly attribute sequence<double> axes;
> +
> +  readonly    attribute DOMHighResTimeStamp timestamp;

You have some odd spacing here.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> I see some callers in the tree that use a raw pointer, and others that use
> an nsRefPtr. I'm not sure what the right thing is here, but I suspect you
> probably need an nsRefPtr here.
raw value should be ok


r+ with with ted's comments fixed.

Comment 11

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e8ce6c0d19a

Comment 12

[(not currently active) Ted Mielczarek]

I pushed this to Try and it looked good:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=782bcaf76bc1

...so I landed it (I was going to comment here first but got the Bugzilla expired session warning).

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Remove expected fail from the web-platform-tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/968449382cf8

Comment 14

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/332fa08016f8

Comment 15

[(not currently active) Ted Mielczarek]

...and a followup to skip the new test on b2g desktop.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8e8ce6c0d19a
https://hg.mozilla.org/mozilla-central/rev/968449382cf8
https://hg.mozilla.org/mozilla-central/rev/332fa08016f8

Comment 17

[(not currently active) Ted Mielczarek]

Snigdha: thanks for the patch! I'm sorry it took so long to get it landed, but it will ship in Firefox 37.