Phase III Step 2: Call imageNotifyFinished() and image load event after SVG loading completes

third_party/WebKit/Source/core/loader/ImageLoader.cpp

Index: third_party/WebKit/Source/core/loader/ImageLoader.cpp
diff --git a/third_party/WebKit/Source/core/loader/ImageLoader.cpp b/third_party/WebKit/Source/core/loader/ImageLoader.cpp
index 490377410ba0ff4f4e74b179e9bbc9350a0aff91..6009426ade2710d17bf7552ab58d6ac0692e1096 100644
--- a/third_party/WebKit/Source/core/loader/ImageLoader.cpp
+++ b/third_party/WebKit/Source/core/loader/ImageLoader.cpp
@@ -177,6 +177,7 @@ void ImageLoader::Dispose() {
   if (image_) {
     image_->RemoveObserver(this);
     image_ = nullptr;
+    delay_until_image_notify_finished_ = nullptr;
   }
 }
 
@@ -201,6 +202,7 @@ void ImageLoader::SetImageWithoutConsideringPendingLoadEvent(
   ImageResourceContent* old_image = image_.Get();
   if (new_image != old_image) {
     image_ = new_image;
+    delay_until_image_notify_finished_ = nullptr;

[fs, 2017/05/05 10:52:42]

Clearing this is tied either to a change to |complete|, or when just clearing
out |image_|, so maybe we could make that more obvious by adding a
SetComplete/UpdateComplete and possibly a ClearImage, WDYT?

[hiroshige, 2017/05/08 17:22:06]

Also when |image_| is changed (to non-null), like here.
I'd also like to dedup the delay_until_image_notify_finished_ code, but not so
sure what is the clear way to do that.
- When we set |image_| to a new value, there are couple of things that are done
together in addition to clearing |delay_until_image_notify_finished|, e.g.
Add/RemoveObserver() and CancelEvent() calls, but each modification sites of
|image_| are slightly different.
- ImageLoader already has SetImage() and
SetImageWithoutConsideringPendingLoadEvent(). Introducing e.g.
SetImageInternal() might look confusing.

Currently I'd like to postpone this unless there's a good way.

[fs, 2017/05/08 18:52:33]

Acknowledged.

[hiroshige, 2017/05/09 00:46:31]

I found another issue that motivates me to introduce
SetImageInternal(), and prepared a CL:
https://codereview.chromium.org/2864253003/.

Still the names are confusing, and the ClearImage() I introduced there is
equivalent to the current SetImage(nullptr) and thus is not what you mean by
ClearImage() here (="image_ = nullptr; delay_until_image_notify_finished_ =
nullptr;").

     if (has_pending_load_event_) {
       LoadEventSender().CancelEvent(this);
       has_pending_load_event_ = false;
@@ -264,7 +266,7 @@ inline void ImageLoader::EnqueueImageLoadingMicroTask(
   pending_task_ = task->CreateWeakPtr();
   Microtask::EnqueueMicrotask(
       WTF::Bind(&Task::Run, WTF::Passed(std::move(task))));
-  load_delay_counter_ =
+  delay_until_do_update_from_element_ =
       IncrementLoadEventDelayCount::Create(element_->GetDocument());
 }
 
@@ -283,7 +285,7 @@ void ImageLoader::DoUpdateFromElement(BypassMainWorldBehavior bypass_behavior,
   pending_task_.reset();
   // Make sure to only decrement the count when we exit this function
   std::unique_ptr<IncrementLoadEventDelayCount> load_delay_counter;
-  load_delay_counter.swap(load_delay_counter_);
+  load_delay_counter.swap(delay_until_do_update_from_element_);
 
   Document& document = element_->GetDocument();
   if (!document.IsActive())
@@ -359,6 +361,7 @@ void ImageLoader::DoUpdateFromElement(BypassMainWorldBehavior bypass_behavior,
     image_ = new_image;
     has_pending_load_event_ = new_image;
     image_complete_ = !new_image;
+    delay_until_image_notify_finished_ = nullptr;
 
     UpdateLayoutObject();
     // If newImage exists and is cached, addObserver() will result in the load
@@ -428,6 +431,7 @@ void ImageLoader::UpdateFromElement(UpdateFromElementBehavior update_behavior,
       image->RemoveObserver(this);
     }
     image_ = nullptr;
+    delay_until_image_notify_finished_ = nullptr;
   }
 
   // Don't load images for inactive documents. We don't want to slow down the
@@ -470,6 +474,20 @@ bool ImageLoader::ShouldLoadImmediately(const KURL& url) const {
   return (isHTMLObjectElement(element_) || isHTMLEmbedElement(element_));
 }
 
+void ImageLoader::ImageChanged(ImageResourceContent* content, const IntRect*) {
+  DCHECK_EQ(content, image_.Get());
+  if (image_complete_ || !content->IsLoading() ||
+      delay_until_image_notify_finished_)
+    return;
+
+  Document& document = element_->GetDocument();
+  if (!document.IsActive())
+    return;
+
+  delay_until_image_notify_finished_ =
+      IncrementLoadEventDelayCount::Create(document);
+}
+
 void ImageLoader::ImageNotifyFinished(ImageResourceContent* resource) {
   RESOURCE_LOADING_DVLOG(1)
       << "ImageLoader::imageNotifyFinished " << this
@@ -479,6 +497,7 @@ void ImageLoader::ImageNotifyFinished(ImageResourceContent* resource) {
   DCHECK_EQ(resource, image_.Get());
 
   image_complete_ = true;
+  delay_until_image_notify_finished_ = nullptr;
 
   // Update ImageAnimationPolicy for m_image.
   if (image_)
@@ -636,8 +655,14 @@ void ImageLoader::DispatchPendingErrorEvents() {
 }
 
 void ImageLoader::ElementDidMoveToNewDocument() {
-  if (load_delay_counter_)
-    load_delay_counter_->DocumentChanged(element_->GetDocument());
+  if (delay_until_do_update_from_element_) {
+    delay_until_do_update_from_element_->DocumentChanged(
+        element_->GetDocument());
+  }
+  if (delay_until_image_notify_finished_) {
+    delay_until_image_notify_finished_->DocumentChanged(
+        element_->GetDocument());
+  }
   ClearFailedLoadURL();
   SetImage(0);
 }