1182981 - Use nsTHashTable::Iterator in dom/{animation,indexedDB}/

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

Because iterators are so much nicer than enumerate functions.

There are ten occurrences of EnumerateEntries() in dom/{animation,indexedDB}/ to be dealt with.

Comment 1

[Brian Birtles (:birtles, Feb 26~ parental leave)]

I'll take care of PendingAnimationTracker and AnimationTimeline which accounts for 5 occurrences.

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to Brian Birtles (:birtles) from comment #1)
> I'll take care of PendingAnimationTracker and AnimationTimeline which
> accounts for 5 occurrences.

Want to do the indexedDB ones as well? They're pretty mechanical and easy :)  If not, can you please file a spin-off bug for indexedDB and rename this one? Thank you.

Comment 3

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Nicholas Nethercote [:njn] from comment #2)
> Want to do the indexedDB ones as well? 

Sure, no problem.

Comment 4

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Use nsTHashtable::Iterator in PendingAnimationTracker

Comment 5

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 2 - Use nsTHashtable::Iterator in AnimationTimeline

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 3 - Use nsTHashtable::Iterator in ActorsParent

Comment 7

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 4 - Use nsTHashtable::Iterator in IDBDatabase

Comment 8

[Brian Birtles (:birtles, Feb 26~ parental leave)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0bd3b4b8ab3

Comment 9

[Nicholas Nethercote [inactive]]

Comment on attachment 8633272 [details] [diff] [review]
part 1 - Use nsTHashtable::Iterator in PendingAnimationTracker

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

::: dom/animation/PendingAnimationTracker.h
@@ +75,5 @@
>                   const AnimationSet& aSet) const;
>  
> +  static void TriggerAnimationsAtTime(AnimationSet& aAnimationSet,
> +                                      const TimeStamp& aReadyTime);
> +  static void TriggerAndClearAnimations(AnimationSet& aSet);

Do these need to be class methods? If not, I'd be inclined to put them entirely within PendingAnimationTracker.cpp, like TriggerAnimationAtTime() and TriggerAnimationNow() were, just to avoid exposing unnecessary details in the header file. (Note that the old functions should have been marked as |static|.)

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Comment on attachment 8633272 [details] [diff] [review]
> part 1 - Use nsTHashtable::Iterator in PendingAnimationTracker
> 
> Review of attachment 8633272 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/animation/PendingAnimationTracker.h
> @@ +75,5 @@
> >                   const AnimationSet& aSet) const;
> >  
> > +  static void TriggerAnimationsAtTime(AnimationSet& aAnimationSet,
> > +                                      const TimeStamp& aReadyTime);
> > +  static void TriggerAndClearAnimations(AnimationSet& aSet);
> 
> Do these need to be class methods? If not, I'd be inclined to put them
> entirely within PendingAnimationTracker.cpp, like TriggerAnimationAtTime()
> and TriggerAnimationNow() were, just to avoid exposing unnecessary details
> in the header file. (Note that the old functions should have been marked as
> |static|.)

Initially I did that, but the AnimationSet typedef is defined within the scope of PendingAnimationTracker so it was just easier to make them class scope. I suppose I could just redefine that inside PendingAnimationTracker.cpp

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 8633274 [details] [diff] [review]
part 3 - Use nsTHashtable::Iterator in ActorsParent

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

Looks good to me except that I'm not sure about the AssertIsOnBackgroundThread() call in NoteFinishedTransaction(). The obvious thing to do it to put it just before the iterator loop, but as we discussed on IRC it's not clear if that's necessary given that there is a AssertIsOnOwningThread() call at the start of that function. I'll let someone who knows this code better decide.

Comment 12

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Comment on attachment 8633274 [details] [diff] [review]
part 3 - Use nsTHashtable::Iterator in ActorsParent

Ben, do you mind looking at this? The main part I'm not sure about is what to do with the AssertIsOnBackgroundThread(). Do we need it in addition to the AssertIsOnOwningThread() at the start of the method? Or is one of the two enough?

Comment 13

[Biru [:poiru]]

(In reply to Brian Birtles (:birtles) from comment #10)
> Initially I did that, but the AnimationSet typedef is defined within the
> scope of PendingAnimationTracker so it was just easier to make them class
> scope. I suppose I could just redefine that inside
> PendingAnimationTracker.cpp

Another option would be to use lamda functions. E.g.:

  void
  PendingAnimationTracker::TriggerPendingAnimationsOnNextTick(...)
  {
    auto triggerAnimationAtTime = [](...) {
    };

    triggerAnimationAtTime(...);
  }

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Birunthan Mohanathas [:poiru] from comment #13)
> Another option would be to use lamda functions. E.g.:
> 
>   void
>   PendingAnimationTracker::TriggerPendingAnimationsOnNextTick(...)
>   {
>     auto triggerAnimationAtTime = [](...) {
>     };
> 
>     triggerAnimationAtTime(...);
>   }

Oh, neat. I didn't even think of that.

Comment 15

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Use nsTHashtable::Iterator in PendingAnimationTracker

Hi Nicholas, care to have a quick look at this again? I replaced the class scope static methods with lambda functions as suggested by Birunthan. Other than that it should be identical to the previous patch.

Comment 16

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Attachment: part 1 - Use nsTHashtable::Iterator in PendingAnimationTracker

Sorry, slight simplification. Still new to lambdas.

Comment 17

[Nicholas Nethercote [inactive]]

Comment on attachment 8633303 [details] [diff] [review]
part 1 - Use nsTHashtable::Iterator in PendingAnimationTracker

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

::: dom/animation/PendingAnimationTracker.cpp
@@ +51,5 @@
>  void
>  PendingAnimationTracker::TriggerPendingAnimationsOnNextTick(const TimeStamp&
>                                                          aReadyTime)
>  {
> +  auto triggerAnimationsAtTime = [aReadyTime](AnimationSet& aAnimationSet) {

Call this triggerAnimationsAtReadyTime?

Comment 18

[Nicholas Nethercote [inactive]]

birtles: bent just stopped working for Mozilla and I'm not sure what his reviewing status is right now.

Comment 19

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

Comment on attachment 8633274 [details] [diff] [review]
part 3 - Use nsTHashtable::Iterator in ActorsParent

I think janv should review this.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7b704a33d93
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5237a8342b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/def8fc96ab63

Comment 21

[Brian Birtles (:birtles, Feb 26~ parental leave)]

Pushed parts 1, 2, and 4.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4dd53b2940a

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7b704a33d93
https://hg.mozilla.org/mozilla-central/rev/f5237a8342b1
https://hg.mozilla.org/mozilla-central/rev/def8fc96ab63
https://hg.mozilla.org/mozilla-central/rev/a4dd53b2940a