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

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

 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  * Copyright (C) 2004, 2005, 2006, 2007, 2009, 2010 Apple Inc. All rights
  * reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
@@ skipping 159 matching lines @@
 
 void ImageLoader::Dispose() {
   RESOURCE_LOADING_DVLOG(1)
       << "~ImageLoader " << this
       << "; m_hasPendingLoadEvent=" << has_pending_load_event_
       << ", m_hasPendingErrorEvent=" << has_pending_error_event_;
 
   if (image_) {
     image_->RemoveObserver(this);
     image_ = nullptr;
+    delay_until_image_notify_finished_ = nullptr;
   }
 }
 
 DEFINE_TRACE(ImageLoader) {
   visitor->Trace(image_);
   visitor->Trace(image_resource_for_image_document_);
   visitor->Trace(element_);
 }
 
 void ImageLoader::SetImage(ImageResourceContent* new_image) {
   SetImageWithoutConsideringPendingLoadEvent(new_image);
 
   // Only consider updating the protection ref-count of the Element immediately
   // before returning from this function as doing so might result in the
   // destruction of this ImageLoader.
   UpdatedHasPendingEvent();
 }
 
 void ImageLoader::SetImageWithoutConsideringPendingLoadEvent(
     ImageResourceContent* new_image) {
   DCHECK(failed_load_url_.IsEmpty());
   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;
     }
     if (has_pending_error_event_) {
       ErrorEventSender().CancelEvent(this);
       has_pending_error_event_ = false;
     }
     image_complete_ = true;
     if (new_image) {
@@ skipping 43 matching lines @@
 }
 
 inline void ImageLoader::EnqueueImageLoadingMicroTask(
     UpdateFromElementBehavior update_behavior,
     ReferrerPolicy referrer_policy) {
   std::unique_ptr<Task> task =
       Task::Create(this, update_behavior, referrer_policy);
   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());
 }
 
 void ImageLoader::DoUpdateFromElement(BypassMainWorldBehavior bypass_behavior,
                                       UpdateFromElementBehavior update_behavior,
                                       const KURL& url,
                                       ReferrerPolicy referrer_policy) {
   // FIXME: According to
   // http://www.whatwg.org/specs/web-apps/current-work/multipage/embedded-content.html#the-img-element:the-img-element-55
   // When "update image" is called due to environment changes and the load
   // fails, onerror should not be called. That is currently not the case.
   //
   // We don't need to call clearLoader here: Either we were called from the
   // task, or our caller updateFromElement cleared the task's loader (and set
   // m_pendingTask to null).
   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())
     return;
 
   AtomicString image_source_url = element_->ImageSourceURL();
   ImageResourceContent* new_image = nullptr;
   if (!url.IsNull()) {
     // Unlike raw <img>, we block mixed content inside of <picture> or
     // <img srcset>.
@@ skipping 55 matching lines @@
     // FIXME: If both previous load and this one got blocked with an error, we
     // can receive one error event instead of two.
     if (has_pending_error_event_ && new_image) {
       ErrorEventSender().CancelEvent(this);
       has_pending_error_event_ = false;
     }
 
     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
     // event being queued to fire. Ensure this happens after beforeload is
     // dispatched.
     if (new_image) {
       new_image->AddObserver(this);
     }
     if (old_image) {
       old_image->RemoveObserver(this);
@@ skipping 49 matching lines @@
     return;
   }
   // Allow the idiom "img.src=''; img.src='.." to clear down the image before an
   // asynchronous load completes.
   if (image_source_url.IsEmpty()) {
     ImageResourceContent* image = image_.Get();
     if (image) {
       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
   // raw HTML parsing case by loading images we don't intend to display.
   Document& document = element_->GetDocument();
   if (document.IsActive())
     EnqueueImageLoadingMicroTask(update_behavior, referrer_policy);
 }
 
 KURL ImageLoader::ImageSourceToKURL(AtomicString image_source_url) const {
@@ skipping 22 matching lines @@
   // content when style recalc is over and DOM mutation is allowed again.
   if (!url.IsNull()) {
     Resource* resource = GetMemoryCache()->ResourceForURL(
         url, element_->GetDocument().Fetcher()->GetCacheIdentifier());
     if (resource && !resource->ErrorOccurred())
       return true;
   }
   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
       << "; m_hasPendingLoadEvent=" << has_pending_load_event_;
 
   DCHECK(failed_load_url_.IsEmpty());
   DCHECK_EQ(resource, image_.Get());
 
   image_complete_ = true;
+  delay_until_image_notify_finished_ = nullptr;
 
   // Update ImageAnimationPolicy for m_image.
   if (image_)
     image_->UpdateImageAnimationPolicy();
 
   UpdateLayoutObject();
 
   if (image_ && image_->GetImage() && image_->GetImage()->IsSVGImage())
     ToSVGImage(image_->GetImage())
         ->UpdateUseCounters(GetElement()->GetDocument());
@@ skipping 137 matching lines @@
 
 void ImageLoader::DispatchPendingLoadEvents() {
   LoadEventSender().DispatchPendingEvents();
 }
 
 void ImageLoader::DispatchPendingErrorEvents() {
   ErrorEventSender().DispatchPendingEvents();
 }
 
 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);
 }
 
 }  // namespace blink