blink: Simplify image advancement logic during catch up.

At the end of the catch up loop, we notify observers that the image is
dirty and schedule a task for the next frame. The latter is unnecessary
since when the image is repainted, it will call StartAnimation to tick
itself at the end of the draw call.

The original intent of the code was to catch up to the desired frame
and synchronously draw it (see [1]). The assumption at the time was
that since this draw will happen synchronously, there will be no
StartAnimation call to schedule the next frame and it needed to be
done immediately after the catch up loop.

This is no longer true. We notify observers async after catching up
and when they draw, we will get a StartAnimation call to tick the
animation. So this additional bit of code is unnecessary complexity.

[1]: https://bugs.webkit.org/show_bug.cgi?id=19663#c6

R=pdr@chromium.org

Change-Id: If9684b9aa92dd69b5e3b87cadea5fc7582e4e7a4
Reviewed-on: https://chromium-review.googlesource.com/727198Reviewed-by: Philip Rogers <pdr@chromium.org>
Commit-Queue: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510141}

third_party/WebKit/Source/platform/graphics/BitmapImage.cpp

@@ -636,22 +636,8 @@ Optional<size_t> BitmapImage::StartAnimationInternal(const double time) {
   desired_frame_start_time_ -=
       FrameDurationAtIndex(current_frame_index_).InSecondsF();
 
-  // Set up the timer for the next frame if required. Note that we have
-  // already advanced to the current_frame_index_ after catching up. And in
-  // the loop above, we either could not advance the animation further or we
-  // advanced it up till the desired time for the current frame. This ensures
-  // that calling StartAnimationInternal here with the same |time| will not
-  // need to perform any catch up skipping.
-  StartAnimationInternal(time);
-
-  // At this point, we've advanced to the |current_frame_index_|, and requested
-  // an invalidation from the observers, and potentially scheduled a task for
-  // further advancing the animation. If the task runs before the next draw,
-  // current_frame_index_ will be skipped, if not, we will draw with it. For the
-  // purpose of keeping the UMA tracking simple, we always exclude the
-  // |current_frame_index_|, since if we do end up drawing before the task runs,
-  // we won't emit an UMA entry for advancing to the next frame with no
-  // skipping.
+  // Don't include the |current_frame_index_|, which will be used on the next
+  // paint, in the number of frames skipped.
   return Optional<size_t>(frames_advanced - 1);
 }
 

third_party/WebKit/Source/platform/graphics/BitmapImageTest.cpp

@@ -62,13 +62,16 @@ class BitmapImageTest : public ::testing::Test {
       last_decoded_size_ = new_size;
     }
     bool ShouldPauseAnimation(const Image*) override { return false; }
-    void AnimationAdvanced(const Image*) override {}
+    void AnimationAdvanced(const Image*) override {
+      animation_advanced_ = true;
+    }
     void AsyncLoadCompleted(const Image*) override { NOTREACHED(); }
 
     virtual void ChangedInRect(const Image*, const IntRect&) {}
 
     size_t last_decoded_size_;
     int last_decoded_size_changed_delta_;
+    bool animation_advanced_ = false;
   };
 
   static RefPtr<SharedBuffer> ReadFile(const char* file_name) {
@@ -509,6 +512,7 @@ TEST_F(BitmapImageTestWithMockDecoder, FrameSkipTracking) {
 
   // Start the animation at 10s. This should schedule a timer for the next frame
   // after 10 seconds.
+  EXPECT_FALSE(image_observer_->animation_advanced_);
   StartAnimation();
   EXPECT_EQ(image_->PaintImageForCurrentFrame().frame_index(), 0u);
 
@@ -517,42 +521,51 @@ TEST_F(BitmapImageTestWithMockDecoder, FrameSkipTracking) {
 
   // Advance the time to 15s. The frame is still at 0u because the posted task
   // should run at 20s.
+  image_observer_->animation_advanced_ = false;
   task_runner->AdvanceTimeAndRun(5);
   EXPECT_EQ(image_->PaintImageForCurrentFrame().frame_index(), 0u);
+  EXPECT_FALSE(image_observer_->animation_advanced_);
 
   // Advance the time to 20s. The task runs and advances the animation forward
   // to 1u.
   task_runner->AdvanceTimeAndRun(5);
+  EXPECT_TRUE(image_observer_->animation_advanced_);
+
+  // If we were to paint now, we should use the second frame.
   EXPECT_EQ(image_->PaintImageForCurrentFrame().frame_index(), 1u);
-  EXPECT_EQ(image_->last_num_frames_skipped_for_testing().value(), 0u);
 
-  // Set now_ to 41 seconds. Since the animation started at 10s, and each frame
-  // has a duration of 10s, we should see the fourth frame at 41 seconds.
+  // Now assume painting the next frame was delayed to 41 seconds. We should
+  // first draw with the |current_frame_index_| and then call StartAnimation to
+  // advance the frame.
+  // Since the animation started at 10s, and each frame has a duration of 10s,
+  // we should see the fourth frame at 41 seconds.
   now_ = 41;
   task_runner->SetTime(41);
   StartAnimation();
-  EXPECT_EQ(image_->PaintImageForCurrentFrame().frame_index(), 3u);
-  EXPECT_EQ(image_->last_num_frames_skipped_for_testing().value(), 1u);
 
-  // We should have scheduled a task to move to the fifth frame in
-  // StartAnimation above, at 50s.
-  // Advance by 5s, not time for the next frame yet.
-  task_runner->AdvanceTimeAndRun(5);
-  EXPECT_EQ(image_->PaintImageForCurrentFrame().frame_index(), 3u);
-
-  // Advance to the fifth frame.
-  task_runner->SetTime(50);
+  // Since we just skipped frames while advancing this animation, we should see
+  // a notification to dirty immediately.
+  image_observer_->animation_advanced_ = false;
   task_runner->AdvanceTimeAndRun(0);
-  EXPECT_EQ(image_->PaintImageForCurrentFrame().frame_index(), 4u);
+  EXPECT_TRUE(image_observer_->animation_advanced_);
+
+  // On the next draw, we should see the fourth frame.
+  EXPECT_EQ(image_->PaintImageForCurrentFrame().frame_index(), 3u);
+  EXPECT_EQ(image_->last_num_frames_skipped_for_testing().value(), 1u);
 
-  // At 70s, we would want to display the last frame and would skip 1 frame.
-  // But because its incomplete, we advanced to the sixth frame and did not need
-  // to skip anything.
+  // At 70s, we would want to display the last frame and would skip 2 frames.
+  // But because its incomplete, we advanced to the sixth frame and only skipped
+  // 1 frame.
   now_ = 71;
   task_runner->SetTime(71);
   StartAnimation();
   EXPECT_EQ(image_->PaintImageForCurrentFrame().frame_index(), 5u);
-  EXPECT_EQ(image_->last_num_frames_skipped_for_testing().value(), 0u);
+  EXPECT_EQ(image_->last_num_frames_skipped_for_testing().value(), 1u);
+
+  // We should still see a notification to move to the sixth frame.
+  image_observer_->animation_advanced_ = false;
+  task_runner->AdvanceTimeAndRun(0);
+  EXPECT_TRUE(image_observer_->animation_advanced_);
 
   // Run any pending tasks and try to animate again. Can't advance the animation
   // because the last frame is not complete.
@@ -565,9 +578,15 @@ TEST_F(BitmapImageTestWithMockDecoder, FrameSkipTracking) {
   // But no frame skipped because we just advanced to the last frame.
   last_frame_complete_ = true;
   image_->SetData(SharedBuffer::Create("data", sizeof("data")), true);
+
   StartAnimation();
   EXPECT_EQ(image_->PaintImageForCurrentFrame().frame_index(), 6u);
   EXPECT_EQ(image_->last_num_frames_skipped_for_testing().value(), 0u);
+
+  // Finishing the animation always notifies observers async.
+  image_observer_->animation_advanced_ = false;
+  task_runner->AdvanceTimeAndRun(0);
+  EXPECT_TRUE(image_observer_->animation_advanced_);
 }
 
 TEST_F(BitmapImageTestWithMockDecoder, ResetAnimation) {