Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2613853002:
Phase III Step 2: Call imageNotifyFinished() and image load event after SVG loading completes (Closed)
|
Created:
3 years, 11 months ago by hiroshige Modified:
3 years, 7 months ago CC:
ajuma+watch_chromium.org, blink-reviews, blink-reviews-platform-graphics_chromium.org, Rik, chromium-reviews, danakj+watch_chromium.org, dshwang, drott+blinkwatch_chromium.org, krit, f(malita), gavinp+loader_chromium.org, gyuyoung2, Nate Chapin, jbroman, Justin Novosad, kinuko+watch, kouhei+svg_chromium.org, loading-reviews_chromium.org, pdr+svgwatchlist_chromium.org, pdr+graphicswatchlist_chromium.org, rwlbuis, tyoshino+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPhase III Step 2: Call imageNotifyFinished() and image load event after SVG loading completes
design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8bE/edit#
https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8voc/edit#
This CL calls ImageResourceObserver::ImageNotifyFinished()
after SVG image loading is completed, not necessarily the same time as
ResourceLoader completion (e.g. DidFinishLoading()).
This makes SVG's <img> load event to be fired after SVG image loading
is completed even when the SVG has subresources loaded asynchronously.
1. Image::SetData() can return SizeAvailableAndLoadingAsynchronously
when SVG image loading is not completed synchronously (due to
subresources), and notifies the completion asynchronously via
ImageObserver:LoadCompleted().
LocalFrameClient::DispatchDidHandleOnloadEvents() is used as a
notification of SVG image loading completion in SVGImage.
2. ImageResourceContent delays calling ImageNotifyFinished() and
setting |content_status_| to kCached until SVG image loading is
completed.
3. ImageLoader additionally delays the Document load event
- from the first ImageChanged() during image loading
- until (potentially delayed) ImageNotifyFinished().
In image loading not by ImageLoader, the Document load event is not
delayed from ResourceLoader completion until ImageNotifyFinished(),
but this is not a regression.
In image loading by ImageLoader, the Document load event is delayed
until ImageNotifyFinished() by the combination of
ResourceFetcher::loaders_ and ImageLoader.
ImageLoader starts delaying the Document load event
- Not from |image_| is set, because we shouldn't delay the Document
load event when image loading is deferred.
- Not from ImageResourceContent::NotifyStartLoad(), to avoid additional
ImageResourceObserver callbacks.
BUG=382170, 690480, 667641
Review-Url: https://codereview.chromium.org/2613853002
Cr-Commit-Position: refs/heads/master@{#472229}
Committed: https://chromium.googlesource.com/chromium/src/+/396a1266fb7549e03e5bcff793ac138654e52d3a
Patch Set 1 #Patch Set 2 : test hack #Patch Set 3 : git cl try #Patch Set 4 : bug fix in error #Patch Set 5 : Rebase #Patch Set 6 : Rebase #Patch Set 7 : notifyStartLoad() #Patch Set 8 : Refine, introduce new enum #Patch Set 9 : More strict state checks #Patch Set 10 : Bug fix? #Patch Set 11 : Do not use notifyStartLoad(), Do not block already loaded image #Patch Set 12 : Rebase #Patch Set 13 : Rebase #Patch Set 14 : Refine/style #Patch Set 15 : Rebase #Patch Set 16 : Rebase #Patch Set 17 : Rebase (rewind) #Patch Set 18 : Rebase (rewind) #Patch Set 19 : format #Patch Set 20 : Rebase #
Total comments: 34
Patch Set 21 : rebase after Blink Renaming #Patch Set 22 : Reflect comments #Patch Set 23 : Forbid revalidation during waiting for SVG load completion. #
Total comments: 1
Patch Set 24 : Rebase #
Total comments: 20
Patch Set 25 : reflect comments #
Total comments: 2
Patch Set 26 : fix/nits #
Total comments: 2
Patch Set 27 : Reflect comments #
Total comments: 4
Patch Set 28 : Rebase #Patch Set 29 : Reflect yhirano's comments and Use raw ptr instead of WeakPtr. #Patch Set 30 : Rebase #Patch Set 31 : Rebase #Patch Set 32 : Rebase #Messages
Total messages: 146 (125 generated)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Call imageNotifyFinished() asynchronously in SVGImage with subresources BUG= ========== to ========== Call imageNotifyFinished() asynchronously in SVGImage with subresources BUG=382170 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Call imageNotifyFinished() asynchronously in SVGImage with subresources BUG=382170 ========== to ========== Call imageNotifyFinished() asynchronously in SVGImage with subresources BUG=382170, 690480, 667641 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Call imageNotifyFinished() asynchronously in SVGImage with subresources BUG=382170, 690480, 667641 ========== to ========== Call imageNotifyFinished() asynchronously in SVGImage with subresources This CL changes the timing of ImageResourceObserver::imageNotifyFinished(): makes it after SVG image loading is completed, and makes it not necessarily the same time as ResourceLoader completion (e.g. didFinishLoading()). 1. Image::setData() can return SizeAvailableAndLoadingAsynchronously when SVG image loading is not completed synchronously (due to subresources), and notifies the completion asynchronously via ImageObserver:loadCompleted(). LocalFrameClient::dispatchDidHandleOnloadEvents() is used as a notification of SVG image loading completion in SVGImage. 2. ImageResourceContent delays calling imageNotifyFinished() and setting |m_status| to Cached until SVG image loading is completed. However, delaying of Document load event by |ResourceFetcher::m_loaders| is not changed, and thus the period from ResourceLoader completion until imageNotifyFinished() should be covered by other means if needed. 3. ImageLoader additionally delays the Document load event - from the first imageChanged() during image loading - until (potentially delayed) imageNotifyFinished(). In image loading not by ImageLoader, the Document load event is not delayed from ResourceLoader completion until imageNotifyFinished(), but this is not a regression. In image loading by ImageLoader, the Document load event is delayed until imageNotifyFinished() by the combination of ResourceFetcher::m_loaders and ImageLoader. ImageLoader starts delaying the Document load event - Not from |m_image| is set, because we shouldn't delay the Document load event when image loading is deferred. - Not from ImageResourceContent::notifyStartLoad(), to avoid additional ImageResourceObserver callbacks. BUG=382170, 690480, 667641 ==========
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Call imageNotifyFinished() asynchronously in SVGImage with subresources This CL changes the timing of ImageResourceObserver::imageNotifyFinished(): makes it after SVG image loading is completed, and makes it not necessarily the same time as ResourceLoader completion (e.g. didFinishLoading()). 1. Image::setData() can return SizeAvailableAndLoadingAsynchronously when SVG image loading is not completed synchronously (due to subresources), and notifies the completion asynchronously via ImageObserver:loadCompleted(). LocalFrameClient::dispatchDidHandleOnloadEvents() is used as a notification of SVG image loading completion in SVGImage. 2. ImageResourceContent delays calling imageNotifyFinished() and setting |m_status| to Cached until SVG image loading is completed. However, delaying of Document load event by |ResourceFetcher::m_loaders| is not changed, and thus the period from ResourceLoader completion until imageNotifyFinished() should be covered by other means if needed. 3. ImageLoader additionally delays the Document load event - from the first imageChanged() during image loading - until (potentially delayed) imageNotifyFinished(). In image loading not by ImageLoader, the Document load event is not delayed from ResourceLoader completion until imageNotifyFinished(), but this is not a regression. In image loading by ImageLoader, the Document load event is delayed until imageNotifyFinished() by the combination of ResourceFetcher::m_loaders and ImageLoader. ImageLoader starts delaying the Document load event - Not from |m_image| is set, because we shouldn't delay the Document load event when image loading is deferred. - Not from ImageResourceContent::notifyStartLoad(), to avoid additional ImageResourceObserver callbacks. BUG=382170, 690480, 667641 ========== to ========== Call imageNotifyFinished() asynchronously in SVGImage with subresources design doc: https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8v... This CL changes the timing of ImageResourceObserver::imageNotifyFinished(): makes it after SVG image loading is completed, and makes it not necessarily the same time as ResourceLoader completion (e.g. didFinishLoading()). 1. Image::setData() can return SizeAvailableAndLoadingAsynchronously when SVG image loading is not completed synchronously (due to subresources), and notifies the completion asynchronously via ImageObserver:loadCompleted(). LocalFrameClient::dispatchDidHandleOnloadEvents() is used as a notification of SVG image loading completion in SVGImage. 2. ImageResourceContent delays calling imageNotifyFinished() and setting |m_status| to Cached until SVG image loading is completed. However, delaying of Document load event by |ResourceFetcher::m_loaders| is not changed, and thus the period from ResourceLoader completion until imageNotifyFinished() should be covered by other means if needed. 3. ImageLoader additionally delays the Document load event - from the first imageChanged() during image loading - until (potentially delayed) imageNotifyFinished(). In image loading not by ImageLoader, the Document load event is not delayed from ResourceLoader completion until imageNotifyFinished(), but this is not a regression. In image loading by ImageLoader, the Document load event is delayed until imageNotifyFinished() by the combination of ResourceFetcher::m_loaders and ImageLoader. ImageLoader starts delaying the Document load event - Not from |m_image| is set, because we shouldn't delay the Document load event when image loading is deferred. - Not from ImageResourceContent::notifyStartLoad(), to avoid additional ImageResourceObserver callbacks. BUG=382170, 690480, 667641 ==========
Description was changed from ========== Call imageNotifyFinished() asynchronously in SVGImage with subresources design doc: https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8v... This CL changes the timing of ImageResourceObserver::imageNotifyFinished(): makes it after SVG image loading is completed, and makes it not necessarily the same time as ResourceLoader completion (e.g. didFinishLoading()). 1. Image::setData() can return SizeAvailableAndLoadingAsynchronously when SVG image loading is not completed synchronously (due to subresources), and notifies the completion asynchronously via ImageObserver:loadCompleted(). LocalFrameClient::dispatchDidHandleOnloadEvents() is used as a notification of SVG image loading completion in SVGImage. 2. ImageResourceContent delays calling imageNotifyFinished() and setting |m_status| to Cached until SVG image loading is completed. However, delaying of Document load event by |ResourceFetcher::m_loaders| is not changed, and thus the period from ResourceLoader completion until imageNotifyFinished() should be covered by other means if needed. 3. ImageLoader additionally delays the Document load event - from the first imageChanged() during image loading - until (potentially delayed) imageNotifyFinished(). In image loading not by ImageLoader, the Document load event is not delayed from ResourceLoader completion until imageNotifyFinished(), but this is not a regression. In image loading by ImageLoader, the Document load event is delayed until imageNotifyFinished() by the combination of ResourceFetcher::m_loaders and ImageLoader. ImageLoader starts delaying the Document load event - Not from |m_image| is set, because we shouldn't delay the Document load event when image loading is deferred. - Not from ImageResourceContent::notifyStartLoad(), to avoid additional ImageResourceObserver callbacks. BUG=382170, 690480, 667641 ========== to ========== Call imageNotifyFinished() asynchronously in SVGImage with subresources design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8v... This CL changes the timing of ImageResourceObserver::imageNotifyFinished(): makes it after SVG image loading is completed, and makes it not necessarily the same time as ResourceLoader completion (e.g. didFinishLoading()). 1. Image::setData() can return SizeAvailableAndLoadingAsynchronously when SVG image loading is not completed synchronously (due to subresources), and notifies the completion asynchronously via ImageObserver:loadCompleted(). LocalFrameClient::dispatchDidHandleOnloadEvents() is used as a notification of SVG image loading completion in SVGImage. 2. ImageResourceContent delays calling imageNotifyFinished() and setting |m_status| to Cached until SVG image loading is completed. However, delaying of Document load event by |ResourceFetcher::m_loaders| is not changed, and thus the period from ResourceLoader completion until imageNotifyFinished() should be covered by other means if needed. 3. ImageLoader additionally delays the Document load event - from the first imageChanged() during image loading - until (potentially delayed) imageNotifyFinished(). In image loading not by ImageLoader, the Document load event is not delayed from ResourceLoader completion until imageNotifyFinished(), but this is not a regression. In image loading by ImageLoader, the Document load event is delayed until imageNotifyFinished() by the combination of ResourceFetcher::m_loaders and ImageLoader. ImageLoader starts delaying the Document load event - Not from |m_image| is set, because we shouldn't delay the Document load event when image loading is deferred. - Not from ImageResourceContent::notifyStartLoad(), to avoid additional ImageResourceObserver callbacks. BUG=382170, 690480, 667641 ==========
Description was changed from ========== Call imageNotifyFinished() asynchronously in SVGImage with subresources design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8v... This CL changes the timing of ImageResourceObserver::imageNotifyFinished(): makes it after SVG image loading is completed, and makes it not necessarily the same time as ResourceLoader completion (e.g. didFinishLoading()). 1. Image::setData() can return SizeAvailableAndLoadingAsynchronously when SVG image loading is not completed synchronously (due to subresources), and notifies the completion asynchronously via ImageObserver:loadCompleted(). LocalFrameClient::dispatchDidHandleOnloadEvents() is used as a notification of SVG image loading completion in SVGImage. 2. ImageResourceContent delays calling imageNotifyFinished() and setting |m_status| to Cached until SVG image loading is completed. However, delaying of Document load event by |ResourceFetcher::m_loaders| is not changed, and thus the period from ResourceLoader completion until imageNotifyFinished() should be covered by other means if needed. 3. ImageLoader additionally delays the Document load event - from the first imageChanged() during image loading - until (potentially delayed) imageNotifyFinished(). In image loading not by ImageLoader, the Document load event is not delayed from ResourceLoader completion until imageNotifyFinished(), but this is not a regression. In image loading by ImageLoader, the Document load event is delayed until imageNotifyFinished() by the combination of ResourceFetcher::m_loaders and ImageLoader. ImageLoader starts delaying the Document load event - Not from |m_image| is set, because we shouldn't delay the Document load event when image loading is deferred. - Not from ImageResourceContent::notifyStartLoad(), to avoid additional ImageResourceObserver callbacks. BUG=382170, 690480, 667641 ========== to ========== Phase III Step 2: Call imageNotifyFinished() and image onload after SVG loading completes design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8v... This CL changes the timing of ImageResourceObserver::imageNotifyFinished(): makes it after SVG image loading is completed, and makes it not necessarily the same time as ResourceLoader completion (e.g. didFinishLoading()). 1. Image::setData() can return SizeAvailableAndLoadingAsynchronously when SVG image loading is not completed synchronously (due to subresources), and notifies the completion asynchronously via ImageObserver:loadCompleted(). LocalFrameClient::dispatchDidHandleOnloadEvents() is used as a notification of SVG image loading completion in SVGImage. 2. ImageResourceContent delays calling imageNotifyFinished() and setting |m_status| to Cached until SVG image loading is completed. However, delaying of Document load event by |ResourceFetcher::m_loaders| is not changed, and thus the period from ResourceLoader completion until imageNotifyFinished() should be covered by other means if needed. 3. ImageLoader additionally delays the Document load event - from the first imageChanged() during image loading - until (potentially delayed) imageNotifyFinished(). In image loading not by ImageLoader, the Document load event is not delayed from ResourceLoader completion until imageNotifyFinished(), but this is not a regression. In image loading by ImageLoader, the Document load event is delayed until imageNotifyFinished() by the combination of ResourceFetcher::m_loaders and ImageLoader. ImageLoader starts delaying the Document load event - Not from |m_image| is set, because we shouldn't delay the Document load event when image loading is deferred. - Not from ImageResourceContent::notifyStartLoad(), to avoid additional ImageResourceObserver callbacks. BUG=382170, 690480, 667641 ==========
Description was changed from ========== Phase III Step 2: Call imageNotifyFinished() and image onload after SVG loading completes design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8v... This CL changes the timing of ImageResourceObserver::imageNotifyFinished(): makes it after SVG image loading is completed, and makes it not necessarily the same time as ResourceLoader completion (e.g. didFinishLoading()). 1. Image::setData() can return SizeAvailableAndLoadingAsynchronously when SVG image loading is not completed synchronously (due to subresources), and notifies the completion asynchronously via ImageObserver:loadCompleted(). LocalFrameClient::dispatchDidHandleOnloadEvents() is used as a notification of SVG image loading completion in SVGImage. 2. ImageResourceContent delays calling imageNotifyFinished() and setting |m_status| to Cached until SVG image loading is completed. However, delaying of Document load event by |ResourceFetcher::m_loaders| is not changed, and thus the period from ResourceLoader completion until imageNotifyFinished() should be covered by other means if needed. 3. ImageLoader additionally delays the Document load event - from the first imageChanged() during image loading - until (potentially delayed) imageNotifyFinished(). In image loading not by ImageLoader, the Document load event is not delayed from ResourceLoader completion until imageNotifyFinished(), but this is not a regression. In image loading by ImageLoader, the Document load event is delayed until imageNotifyFinished() by the combination of ResourceFetcher::m_loaders and ImageLoader. ImageLoader starts delaying the Document load event - Not from |m_image| is set, because we shouldn't delay the Document load event when image loading is deferred. - Not from ImageResourceContent::notifyStartLoad(), to avoid additional ImageResourceObserver callbacks. BUG=382170, 690480, 667641 ========== to ========== Phase III Step 2: Call imageNotifyFinished() and image load event after SVG loading completes design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8v... This CL changes the timing of ImageResourceObserver::imageNotifyFinished(): makes it after SVG image loading is completed, and makes it not necessarily the same time as ResourceLoader completion (e.g. didFinishLoading()). 1. Image::setData() can return SizeAvailableAndLoadingAsynchronously when SVG image loading is not completed synchronously (due to subresources), and notifies the completion asynchronously via ImageObserver:loadCompleted(). LocalFrameClient::dispatchDidHandleOnloadEvents() is used as a notification of SVG image loading completion in SVGImage. 2. ImageResourceContent delays calling imageNotifyFinished() and setting |m_status| to Cached until SVG image loading is completed. However, delaying of Document load event by |ResourceFetcher::m_loaders| is not changed, and thus the period from ResourceLoader completion until imageNotifyFinished() should be covered by other means if needed. 3. ImageLoader additionally delays the Document load event - from the first imageChanged() during image loading - until (potentially delayed) imageNotifyFinished(). In image loading not by ImageLoader, the Document load event is not delayed from ResourceLoader completion until imageNotifyFinished(), but this is not a regression. In image loading by ImageLoader, the Document load event is delayed until imageNotifyFinished() by the combination of ResourceFetcher::m_loaders and ImageLoader. ImageLoader starts delaying the Document load event - Not from |m_image| is set, because we shouldn't delay the Document load event when image loading is deferred. - Not from ImageResourceContent::notifyStartLoad(), to avoid additional ImageResourceObserver callbacks. BUG=382170, 690480, 667641 ==========
Description was changed from ========== Phase III Step 2: Call imageNotifyFinished() and image load event after SVG loading completes design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8v... This CL changes the timing of ImageResourceObserver::imageNotifyFinished(): makes it after SVG image loading is completed, and makes it not necessarily the same time as ResourceLoader completion (e.g. didFinishLoading()). 1. Image::setData() can return SizeAvailableAndLoadingAsynchronously when SVG image loading is not completed synchronously (due to subresources), and notifies the completion asynchronously via ImageObserver:loadCompleted(). LocalFrameClient::dispatchDidHandleOnloadEvents() is used as a notification of SVG image loading completion in SVGImage. 2. ImageResourceContent delays calling imageNotifyFinished() and setting |m_status| to Cached until SVG image loading is completed. However, delaying of Document load event by |ResourceFetcher::m_loaders| is not changed, and thus the period from ResourceLoader completion until imageNotifyFinished() should be covered by other means if needed. 3. ImageLoader additionally delays the Document load event - from the first imageChanged() during image loading - until (potentially delayed) imageNotifyFinished(). In image loading not by ImageLoader, the Document load event is not delayed from ResourceLoader completion until imageNotifyFinished(), but this is not a regression. In image loading by ImageLoader, the Document load event is delayed until imageNotifyFinished() by the combination of ResourceFetcher::m_loaders and ImageLoader. ImageLoader starts delaying the Document load event - Not from |m_image| is set, because we shouldn't delay the Document load event when image loading is deferred. - Not from ImageResourceContent::notifyStartLoad(), to avoid additional ImageResourceObserver callbacks. BUG=382170, 690480, 667641 ========== to ========== Phase III Step 2: Call imageNotifyFinished() and image load event after SVG loading completes design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8v... This CL calls ImageResourceObserver::imageNotifyFinished() after SVG image loading is completed, not necessarily the same time as ResourceLoader completion (e.g. didFinishLoading()). This makes SVG's <img> load event to be called after SVG image loading is completed even when the SVG has subresources loaded asynchronously. 1. Image::setData() can return SizeAvailableAndLoadingAsynchronously when SVG image loading is not completed synchronously (due to subresources), and notifies the completion asynchronously via ImageObserver:loadCompleted(). LocalFrameClient::dispatchDidHandleOnloadEvents() is used as a notification of SVG image loading completion in SVGImage. 2. ImageResourceContent delays calling imageNotifyFinished() and setting |m_status| to Cached until SVG image loading is completed. 3. ImageLoader additionally delays the Document load event - from the first imageChanged() during image loading - until (potentially delayed) imageNotifyFinished(). In image loading not by ImageLoader, the Document load event is not delayed from ResourceLoader completion until imageNotifyFinished(), but this is not a regression. In image loading by ImageLoader, the Document load event is delayed until imageNotifyFinished() by the combination of ResourceFetcher::m_loaders and ImageLoader. ImageLoader starts delaying the Document load event - Not from |m_image| is set, because we shouldn't delay the Document load event when image loading is deferred. - Not from ImageResourceContent::notifyStartLoad(), to avoid additional ImageResourceObserver callbacks. BUG=382170, 690480, 667641 ==========
Loading and SVG experts, PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ImageLoader.h:166: std::unique_ptr<IncrementLoadEventDelayCount> m_loadDelayCounter; m_delayLoadUntilUpdateFromElement? https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ImageLoader.h:182: std::unique_ptr<IncrementLoadEventDelayCount> m_loadDelayCounter2; Do we have a better name for this? m_delayLoadUntilImageNotifyFinished? https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:609: { remove { and unindent
(What is "Phase III Step 2", and does it need to be included in the commit subject?) https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-crash.html (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-crash.html:1: <html> Don't use quirks mode? https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-crash.html:3: <script src="../../resources/js-test.js"></script> Use testharness.js? (I.e two steps, one for each 'load'.) https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-invalid-data-url-crash.html (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-invalid-data-url-crash.html:1: <html> Ditto. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-invalid-data-url-crash.html:3: <script src="../../resources/js-test.js"></script> Ditto. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-invalid-font-crash.html (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-invalid-font-crash.html:1: <html> Ditto. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-invalid-font-crash.html:3: <script src="../../resources/js-test.js"></script> Ditto. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/as-image/resources/data-font-in-css.svg (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/resources/data-font-in-css.svg:5: <text x="50" y="50" font-family="ahem-data" font-size="16">Hell, World</text> Tihi =) https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ImageLoader.cpp:373: if (newImage) { This shuffle is non-obvious, what are the side-effects of removeObserver and addObserver (some of the side-effects of addObserver is described above, maybe that needs some updating too?) https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ImageLoader.h:182: std::unique_ptr<IncrementLoadEventDelayCount> m_loadDelayCounter2; On 2017/03/15 at 11:32:34, kouhei wrote: > Do we have a better name for this? > m_delayLoadUntilImageNotifyFinished? Anything other than ...2 would be an improvement IMHO =) https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:576: switch (m_image->m_loadStatus) { Could we have this just call a method on SVGImage, and thus keep all the state management there? https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:661: LocalFrame::create(new SVGImageLocalFrameClient(this->asWeakPtr()), What is the case that prompts the use of a WeakPtr here? (The FrameClient is on the GC heap.) https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/graphics/SVGImage.h (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImage.h:178: enum AsyncLoadCompletionStatus { Why Async...? That's just one possible outcome.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Phase III Step 2: Call imageNotifyFinished() and image load event after SVG loading completes design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8v... This CL calls ImageResourceObserver::imageNotifyFinished() after SVG image loading is completed, not necessarily the same time as ResourceLoader completion (e.g. didFinishLoading()). This makes SVG's <img> load event to be called after SVG image loading is completed even when the SVG has subresources loaded asynchronously. 1. Image::setData() can return SizeAvailableAndLoadingAsynchronously when SVG image loading is not completed synchronously (due to subresources), and notifies the completion asynchronously via ImageObserver:loadCompleted(). LocalFrameClient::dispatchDidHandleOnloadEvents() is used as a notification of SVG image loading completion in SVGImage. 2. ImageResourceContent delays calling imageNotifyFinished() and setting |m_status| to Cached until SVG image loading is completed. 3. ImageLoader additionally delays the Document load event - from the first imageChanged() during image loading - until (potentially delayed) imageNotifyFinished(). In image loading not by ImageLoader, the Document load event is not delayed from ResourceLoader completion until imageNotifyFinished(), but this is not a regression. In image loading by ImageLoader, the Document load event is delayed until imageNotifyFinished() by the combination of ResourceFetcher::m_loaders and ImageLoader. ImageLoader starts delaying the Document load event - Not from |m_image| is set, because we shouldn't delay the Document load event when image loading is deferred. - Not from ImageResourceContent::notifyStartLoad(), to avoid additional ImageResourceObserver callbacks. BUG=382170, 690480, 667641 ========== to ========== Phase III Step 2: Call imageNotifyFinished() and image load event after SVG loading completes design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8v... This CL calls ImageResourceObserver::ImageNotifyFinished() after SVG image loading is completed, not necessarily the same time as ResourceLoader completion (e.g. DidFinishLoading()). This makes SVG's <img> load event to be fired after SVG image loading is completed even when the SVG has subresources loaded asynchronously. 1. Image::SetData() can return SizeAvailableAndLoadingAsynchronously when SVG image loading is not completed synchronously (due to subresources), and notifies the completion asynchronously via ImageObserver:LoadCompleted(). LocalFrameClient::DispatchDidHandleOnloadEvents() is used as a notification of SVG image loading completion in SVGImage. 2. ImageResourceContent delays calling ImageNotifyFinished() and setting |content_status_| to kCached until SVG image loading is completed. 3. ImageLoader additionally delays the Document load event - from the first ImageChanged() during image loading - until (potentially delayed) ImageNotifyFinished(). In image loading not by ImageLoader, the Document load event is not delayed from ResourceLoader completion until ImageNotifyFinished(), but this is not a regression. In image loading by ImageLoader, the Document load event is delayed until ImageNotifyFinished() by the combination of ResourceFetcher::loaders_ and ImageLoader. ImageLoader starts delaying the Document load event - Not from |image_| is set, because we shouldn't delay the Document load event when image loading is deferred. - Not from ImageResourceContent::NotifyStartLoad(), to avoid additional ImageResourceObserver callbacks. BUG=382170, 690480, 667641 ==========
Hi reviewers, Sorry for a long delay, I was blocked by subtle corner cases (and other works as well). I expect I solved them and restarting the review. In this CL, in addition to reflecting comments, I made a new change that disables revalidation while waiting for SVG load completion (see my comment in ImageResource.cpp). https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-crash.html (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-crash.html:1: <html> On 2017/03/15 14:22:16, fs wrote: > Don't use quirks mode? Done. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-crash.html:3: <script src="../../resources/js-test.js"></script> On 2017/03/15 14:22:16, fs wrote: > Use testharness.js? (I.e two steps, one for each 'load'.) Done. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-crash.html:3: <script src="../../resources/js-test.js"></script> On 2017/03/15 14:22:16, fs wrote: > Use testharness.js? (I.e two steps, one for each 'load'.) Done. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-invalid-data-url-crash.html (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-invalid-data-url-crash.html:1: <html> On 2017/03/15 14:22:16, fs wrote: > Ditto. Done. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-invalid-data-url-crash.html:3: <script src="../../resources/js-test.js"></script> On 2017/03/15 14:22:16, fs wrote: > Ditto. Done. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-invalid-font-crash.html (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-invalid-font-crash.html:1: <html> On 2017/03/15 14:22:16, fs wrote: > Ditto. Done. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-invalid-font-crash.html:3: <script src="../../resources/js-test.js"></script> On 2017/03/15 14:22:16, fs wrote: > Ditto. Done. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ImageLoader.cpp:373: if (newImage) { On 2017/03/15 14:22:17, fs wrote: > This shuffle is non-obvious, what are the side-effects of removeObserver and > addObserver (some of the side-effects of addObserver is described above, maybe > that needs some updating too?) Oh, this was my mistake (unintentionally changed the code while refactoring). Reverted. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/ImageLoader.h (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ImageLoader.h:166: std::unique_ptr<IncrementLoadEventDelayCount> m_loadDelayCounter; On 2017/03/15 11:32:34, kouhei (on vacation) wrote: > m_delayLoadUntilUpdateFromElement? Done. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ImageLoader.h:182: std::unique_ptr<IncrementLoadEventDelayCount> m_loadDelayCounter2; On 2017/03/15 11:32:34, kouhei (on vacation) wrote: > Do we have a better name for this? > m_delayLoadUntilImageNotifyFinished? Done. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:576: switch (m_image->m_loadStatus) { On 2017/03/15 14:22:17, fs wrote: > Could we have this just call a method on SVGImage, and thus keep all the state > management there? Done. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:609: { On 2017/03/15 11:32:34, kouhei (on vacation) wrote: > remove { and unindent Done. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:661: LocalFrame::create(new SVGImageLocalFrameClient(this->asWeakPtr()), On 2017/03/15 14:22:17, fs wrote: > What is the case that prompts the use of a WeakPtr here? (The FrameClient is on > the GC heap.) While I don't know concrete cases, the following is what I thought: Previously the FrameClient doesn't have strong references to SVGImage, so I made this WeakPtr to keep the lifetime implication. Also SVGImage is ThreadSafeRefCounted and thus having RefPtr here might cause changes in the SVGImage destruction timing. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/graphics/SVGImage.h (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImage.h:178: enum AsyncLoadCompletionStatus { On 2017/03/15 14:22:17, fs wrote: > Why Async...? That's just one possible outcome. Done. Renamed to LoadState (and renamed some of enum value names). https://codereview.chromium.org/2613853002/diff/440001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right): https://codereview.chromium.org/2613853002/diff/440001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:188: if (!GetContent()->IsLoaded()) This is not clean, but ImageResourceContent has already special logics for revalidation (e.g. IsCacheValidator() call in AddObserver()). However, I expect allowing revalidation during waiting for SVG load complicates the state transition much more. Consider the following scenario: (1) DidFinishLoading() for an SVG image is called, and we start waiting SVG load completion. GetContentState() is kPending. (2) Revalidation starts. (3) SVG load completes. At this time, I think we shouldn't notify observers nor change the state to kCached, because we are in the middle of revalidation. (4) Revalidation response is received and its 304. The original SVG image continues to be used, and thus we have to notify its load completion. Therefore, we have to remember the notification at Step (3) and then process it here in Step (4).
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM w/ one minor issue (WeakPtr revokation) and some nits, comments and suggestions. I'll leave for loader people to comment on the revalidation conundrum. Nice job! =) https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:661: LocalFrame::create(new SVGImageLocalFrameClient(this->asWeakPtr()), On 2017/05/04 at 22:50:50, hiroshige wrote: > On 2017/03/15 14:22:17, fs wrote: > > What is the case that prompts the use of a WeakPtr here? (The FrameClient is on > > the GC heap.) > > While I don't know concrete cases, the following is what I thought: > Previously the FrameClient doesn't have strong references to SVGImage, so I made this WeakPtr to keep the lifetime implication. > Also SVGImage is ThreadSafeRefCounted and thus having RefPtr here might cause changes in the SVGImage destruction timing. Yes, a RefPtr would certainly not work. With the current setup we'll tear down the Page (SVGImage::page_) before any weak pointers are invalidated though, so we should probably at least explicitly revoke the WeakPtr before tearing down the Page, or this setup will not be meaningful AFAICS. https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-crash.html (right): https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-crash.html:3: <head> uNit: <html> and <head> (and corresponding end tags) don't add any value to the test, so they can be removed. https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-crash.html:7: var t1 = async_test("Loading of SVG with a valid font completes " + Personally I think I would've written these tests (this one and following ones) as a single test with multiple steps (to avoid having one test essentially depend on the other.) But that may be somewhat subjective, so If you prefer this way and want to keep it like this, it's fine with me. (Thought I'd just mention the alternative.) https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ImageLoader.cpp:205: delay_until_image_notify_finished_ = nullptr; 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? https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp (right): https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:481: size_available_ != Image::kSizeAvailableAndLoadingAsynchronously) { Nit: Maybe add a short comment about this additional part of the condition (it may not be 100% obvious.) https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:617: // The SVGImage is destructed before SVG load completion. Nit: s/is/was/ https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/graphics/SVGImage.h (right): https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImage.h:211: kLoadCompletedSynchronously, We don't really need both kLoadCompletedSynchronously and kLoadCompletedAsynchronously right? They represent the same state, and we have no need to know how we got into that state. So kLoadCompleted ought to suffice? https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/Image.h (right): https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/Image.h:104: enum SizeAvailability { uNit: Maybe order these in a sort of "lifecycle order" (Unavailable > AvailableAndLoadingAsync > Available) https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/Image.h:112: // (only when |this| is SVGImage and |allDataReceived| is true), and Nit: The Blink Renaming means naming here (and in other comments, seems unnecessary to flag all) ought to be updated. https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageObserver.h (right): https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageObserver.h:52: virtual void LoadCompleted(const Image*) = 0; Since this has a strong tie to the "...LoadingAsynchronously" "size state" (yes, that feels a bit weird TBH...) maybe this should rather be AsyncLoadCompleted?
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
loading lgtm % WeakPtr. I'll think more about this https://codereview.chromium.org/2613853002/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp (right): https://codereview.chromium.org/2613853002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:485: size_available_ != Image::kSizeAvailableAndLoadingAsynchronously) { Can we add DCHECKs about the possible conditions here? https://codereview.chromium.org/2613853002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp (right): https://codereview.chromium.org/2613853002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:404: CHECK_EQ(size_available_, Image::kSizeAvailableAndLoadingAsynchronously); DCHECK
Thanks for review/comments! Reflected the comments, except for the WeakPtr issue. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/as-image/resources/data-font-in-css.svg (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/resources/data-font-in-css.svg:5: <text x="50" y="50" font-family="ahem-data" font-size="16">Hell, World</text> On 2017/03/15 14:22:17, fs wrote: > Tihi =) Done. https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:661: LocalFrame::create(new SVGImageLocalFrameClient(this->asWeakPtr()), On 2017/05/05 10:52:42, fs wrote: > On 2017/05/04 at 22:50:50, hiroshige wrote: > > On 2017/03/15 14:22:17, fs wrote: > > > What is the case that prompts the use of a WeakPtr here? (The FrameClient is > on > > > the GC heap.) > > > > While I don't know concrete cases, the following is what I thought: > > Previously the FrameClient doesn't have strong references to SVGImage, so I > made this WeakPtr to keep the lifetime implication. > > Also SVGImage is ThreadSafeRefCounted and thus having RefPtr here might cause > changes in the SVGImage destruction timing. > > Yes, a RefPtr would certainly not work. With the current setup we'll tear down > the Page (SVGImage::page_) before any weak pointers are invalidated though, so > we should probably at least explicitly revoke the WeakPtr before tearing down > the Page, or this setup will not be meaningful AFAICS. Oh, and I noticed we cannot use WeakPtr with ThreadSafeRefCounted. https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-crash.html (right): https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-crash.html:3: <head> On 2017/05/05 10:52:42, fs wrote: > uNit: <html> and <head> (and corresponding end tags) don't add any value to the > test, so they can be removed. Done. https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/svg/as-image/data-font-in-css-crash.html:7: var t1 = async_test("Loading of SVG with a valid font completes " + On 2017/05/05 10:52:42, fs wrote: > Personally I think I would've written these tests (this one and following ones) > as a single test with multiple steps (to avoid having one test essentially > depend on the other.) But that may be somewhat subjective, so If you prefer this > way and want to keep it like this, it's fine with me. (Thought I'd just mention > the alternative.) Done. https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ImageLoader.cpp:205: delay_until_image_notify_finished_ = nullptr; On 2017/05/05 10:52:42, fs wrote: > 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? 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. https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp (right): https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:481: size_available_ != Image::kSizeAvailableAndLoadingAsynchronously) { On 2017/05/05 10:52:42, fs wrote: > Nit: Maybe add a short comment about this additional part of the condition (it > may not be 100% obvious.) Done. https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:617: // The SVGImage is destructed before SVG load completion. On 2017/05/05 10:52:42, fs wrote: > Nit: s/is/was/ Done. https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/graphics/SVGImage.h (right): https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImage.h:211: kLoadCompletedSynchronously, On 2017/05/05 10:52:42, fs wrote: > We don't really need both kLoadCompletedSynchronously and > kLoadCompletedAsynchronously right? They represent the same state, and we have > no need to know how we got into that state. So kLoadCompleted ought to suffice? Mostly. The state assertion at the bottom of SVGImage::DataChanged() is the only place where two states are distinguished, but the state assertion is relatively unimportant and unlikely. Merged them. https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/Image.h (right): https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/Image.h:104: enum SizeAvailability { On 2017/05/05 10:52:42, fs wrote: > uNit: Maybe order these in a sort of "lifecycle order" (Unavailable > > AvailableAndLoadingAsync > Available) Done. https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/Image.h:112: // (only when |this| is SVGImage and |allDataReceived| is true), and On 2017/05/05 10:52:42, fs wrote: > Nit: The Blink Renaming means naming here (and in other comments, seems > unnecessary to flag all) ought to be updated. Done. https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/graphics/ImageObserver.h (right): https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/graphics/ImageObserver.h:52: virtual void LoadCompleted(const Image*) = 0; On 2017/05/05 10:52:42, fs wrote: > Since this has a strong tie to the "...LoadingAsynchronously" "size state" (yes, > that feels a bit weird TBH...) maybe this should rather be AsyncLoadCompleted? Done. https://codereview.chromium.org/2613853002/diff/480001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp (right): https://codereview.chromium.org/2613853002/diff/480001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:485: size_available_ != Image::kSizeAvailableAndLoadingAsynchronously) { On 2017/05/08 13:07:04, kouhei (on transit) wrote: > Can we add DCHECKs about the possible conditions here? What kind of conditions? (Added one DCHECK in the latest Patch Set) https://codereview.chromium.org/2613853002/diff/500001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp (right): https://codereview.chromium.org/2613853002/diff/500001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:404: CHECK_EQ(size_available_, Image::kSizeAvailableAndLoadingAsynchronously); On 2017/05/08 13:07:04, kouhei (on transit) wrote: > DCHECK I'd like to leave this CHECK() to catch unexpected races between two activities, i.e. waiting for async SVG completion and Resource loading (including revalidationr/reload). Also failure of this CHECK() causes wrong state transitions (in Lines 406 and 407).
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:661: LocalFrame::create(new SVGImageLocalFrameClient(this->asWeakPtr()), On 2017/05/08 17:22:06, hiroshige wrote: > On 2017/05/05 10:52:42, fs wrote: > > On 2017/05/04 at 22:50:50, hiroshige wrote: > > > On 2017/03/15 14:22:17, fs wrote: > > > > What is the case that prompts the use of a WeakPtr here? (The FrameClient > is > > on > > > > the GC heap.) > > > > > > While I don't know concrete cases, the following is what I thought: > > > Previously the FrameClient doesn't have strong references to SVGImage, so I > > made this WeakPtr to keep the lifetime implication. > > > Also SVGImage is ThreadSafeRefCounted and thus having RefPtr here might > cause > > changes in the SVGImage destruction timing. > > > > Yes, a RefPtr would certainly not work. With the current setup we'll tear down > > the Page (SVGImage::page_) before any weak pointers are invalidated though, so > > we should probably at least explicitly revoke the WeakPtr before tearing down > > the Page, or this setup will not be meaningful AFAICS. > > Oh, and I noticed we cannot use WeakPtr with ThreadSafeRefCounted. So, this means SVGImage is always alive when the notification is called, right? > explicitly revoke the WeakPtr before tearing down the Page If this is the only case where WeakPtr should be null at the notification, we can check SVGImage::page_ instead (because page_ is already nullptr when thepage is being tearing down) and use a raw pointer instead of WeakPtr. However, raw pointers looks unsafe and I prefer WeakPtr though. Filed crbug.com/719617 for Image being ThreadSafeRefCounted.
https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2613853002/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:661: LocalFrame::create(new SVGImageLocalFrameClient(this->asWeakPtr()), On 2017/05/08 at 18:37:11, hiroshige wrote: > On 2017/05/08 17:22:06, hiroshige wrote: > > On 2017/05/05 10:52:42, fs wrote: > > > On 2017/05/04 at 22:50:50, hiroshige wrote: > > > > On 2017/03/15 14:22:17, fs wrote: > > > > > What is the case that prompts the use of a WeakPtr here? (The FrameClient > > is > > > on > > > > > the GC heap.) > > > > > > > > While I don't know concrete cases, the following is what I thought: > > > > Previously the FrameClient doesn't have strong references to SVGImage, so I > > > made this WeakPtr to keep the lifetime implication. > > > > Also SVGImage is ThreadSafeRefCounted and thus having RefPtr here might > > cause > > > changes in the SVGImage destruction timing. > > > > > > Yes, a RefPtr would certainly not work. With the current setup we'll tear down > > > the Page (SVGImage::page_) before any weak pointers are invalidated though, so > > > we should probably at least explicitly revoke the WeakPtr before tearing down > > > the Page, or this setup will not be meaningful AFAICS. > > > > Oh, and I noticed we cannot use WeakPtr with ThreadSafeRefCounted. > > So, this means SVGImage is always alive when the notification is called, right? Yes, AFAICS. > > explicitly revoke the WeakPtr before tearing down the Page > If this is the only case where WeakPtr should be null at the notification, we can check SVGImage::page_ instead (because page_ is already nullptr when thepage is being tearing down) and use a raw pointer instead of WeakPtr. > > However, raw pointers looks unsafe and I prefer WeakPtr though. > > Filed crbug.com/719617 for Image being ThreadSafeRefCounted. Thanks. https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ImageLoader.cpp:205: delay_until_image_notify_finished_ = nullptr; On 2017/05/08 at 17:22:06, hiroshige wrote: > On 2017/05/05 10:52:42, fs wrote: > > 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? > > 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. Acknowledged.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/ImageLoader.cpp:205: delay_until_image_notify_finished_ = nullptr; On 2017/05/08 18:52:33, fs wrote: > On 2017/05/08 at 17:22:06, hiroshige wrote: > > On 2017/05/05 10:52:42, fs wrote: > > > 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? > > > > 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. > > Acknowledged. 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;").
On 2017/05/09 at 00:46:31, hiroshige wrote: > https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right): > > https://codereview.chromium.org/2613853002/diff/460001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/loader/ImageLoader.cpp:205: delay_until_image_notify_finished_ = nullptr; > On 2017/05/08 18:52:33, fs wrote: > > On 2017/05/08 at 17:22:06, hiroshige wrote: > > > On 2017/05/05 10:52:42, fs wrote: > > > > 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? > > > > > > 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. > > > > Acknowledged. > > 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;"). Thanks, commented.
lgtm https://codereview.chromium.org/2613853002/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp (right): https://codereview.chromium.org/2613853002/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:450: if (!all_data_received) { This is same as L477-L479. Why do we have the same check with different styles? https://codereview.chromium.org/2613853002/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp (right): https://codereview.chromium.org/2613853002/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp:164: const char kSvgImageWithSubresource[] = constexpr
Discussed offline w/ hiroshige@. - SVGImageLFC to ref SVGImage via raw ptr. - Clarify obj graph in comment: -- SVGImage -(Pers)-> Page -(Mem)-> Frame -(Mem)-> FrameClient == SVGImageLFC -(raw)-> SVGImage - Clear SVGImageLFC->SVGImage ref in SVGImage dtor.
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
> Discussed offline w/ hiroshige@. > - SVGImageLFC to ref SVGImage via raw ptr. > - Clarify obj graph in comment: > -- SVGImage -(Pers)-> Page -(Mem)-> Frame -(Mem)-> FrameClient == SVGImageLFC > -(raw)-> SVGImage > - Clear SVGImageLFC->SVGImage ref in SVGImage dtor. Done. https://codereview.chromium.org/2613853002/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp (right): https://codereview.chromium.org/2613853002/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:450: if (!all_data_received) { On 2017/05/09 08:33:57, yhirano wrote: > This is same as L477-L479. Why do we have the same check with different styles? Done, unified the style. https://codereview.chromium.org/2613853002/diff/520001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp (right): https://codereview.chromium.org/2613853002/diff/520001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp:164: const char kSvgImageWithSubresource[] = On 2017/05/09 08:33:57, yhirano wrote: > constexpr Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by hiroshige@chromium.org
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fs@opera.com, kouhei@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2613853002/#ps620001 (title: "Rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hiroshige@chromium.org changed reviewers: + kinuko@chromium.org
platform/graphics OWNERS (pdr@, schenney@, or kinuko@), could you take a look as an OWNER?
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2017/05/15 19:28:12, hiroshige wrote: > platform/graphics OWNERS (pdr@, schenney@, or kinuko@), could you take a look as > an OWNER? lgtm for platform. Thanks so much for working on this.
lgtm/2
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
The CQ bit was unchecked by hiroshige@chromium.org
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by hiroshige@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 620001, "attempt_start_ts": 1494961617884170,
"parent_rev": "03d66d9ce1e602df26a9d544d9258a88b9194349", "commit_rev":
"396a1266fb7549e03e5bcff793ac138654e52d3a"}
Message was sent while issue was closed.
Description was changed from ========== Phase III Step 2: Call imageNotifyFinished() and image load event after SVG loading completes design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8v... This CL calls ImageResourceObserver::ImageNotifyFinished() after SVG image loading is completed, not necessarily the same time as ResourceLoader completion (e.g. DidFinishLoading()). This makes SVG's <img> load event to be fired after SVG image loading is completed even when the SVG has subresources loaded asynchronously. 1. Image::SetData() can return SizeAvailableAndLoadingAsynchronously when SVG image loading is not completed synchronously (due to subresources), and notifies the completion asynchronously via ImageObserver:LoadCompleted(). LocalFrameClient::DispatchDidHandleOnloadEvents() is used as a notification of SVG image loading completion in SVGImage. 2. ImageResourceContent delays calling ImageNotifyFinished() and setting |content_status_| to kCached until SVG image loading is completed. 3. ImageLoader additionally delays the Document load event - from the first ImageChanged() during image loading - until (potentially delayed) ImageNotifyFinished(). In image loading not by ImageLoader, the Document load event is not delayed from ResourceLoader completion until ImageNotifyFinished(), but this is not a regression. In image loading by ImageLoader, the Document load event is delayed until ImageNotifyFinished() by the combination of ResourceFetcher::loaders_ and ImageLoader. ImageLoader starts delaying the Document load event - Not from |image_| is set, because we shouldn't delay the Document load event when image loading is deferred. - Not from ImageResourceContent::NotifyStartLoad(), to avoid additional ImageResourceObserver callbacks. BUG=382170, 690480, 667641 ========== to ========== Phase III Step 2: Call imageNotifyFinished() and image load event after SVG loading completes design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8v... This CL calls ImageResourceObserver::ImageNotifyFinished() after SVG image loading is completed, not necessarily the same time as ResourceLoader completion (e.g. DidFinishLoading()). This makes SVG's <img> load event to be fired after SVG image loading is completed even when the SVG has subresources loaded asynchronously. 1. Image::SetData() can return SizeAvailableAndLoadingAsynchronously when SVG image loading is not completed synchronously (due to subresources), and notifies the completion asynchronously via ImageObserver:LoadCompleted(). LocalFrameClient::DispatchDidHandleOnloadEvents() is used as a notification of SVG image loading completion in SVGImage. 2. ImageResourceContent delays calling ImageNotifyFinished() and setting |content_status_| to kCached until SVG image loading is completed. 3. ImageLoader additionally delays the Document load event - from the first ImageChanged() during image loading - until (potentially delayed) ImageNotifyFinished(). In image loading not by ImageLoader, the Document load event is not delayed from ResourceLoader completion until ImageNotifyFinished(), but this is not a regression. In image loading by ImageLoader, the Document load event is delayed until ImageNotifyFinished() by the combination of ResourceFetcher::loaders_ and ImageLoader. ImageLoader starts delaying the Document load event - Not from |image_| is set, because we shouldn't delay the Document load event when image loading is deferred. - Not from ImageResourceContent::NotifyStartLoad(), to avoid additional ImageResourceObserver callbacks. BUG=382170, 690480, 667641 Review-Url: https://codereview.chromium.org/2613853002 Cr-Commit-Position: refs/heads/master@{#472229} Committed: https://chromium.googlesource.com/chromium/src/+/396a1266fb7549e03e5bcff793ac... ==========
Message was sent while issue was closed.
Committed patchset #32 (id:620001) as https://chromium.googlesource.com/chromium/src/+/396a1266fb7549e03e5bcff793ac...
Message was sent while issue was closed.
Description was changed from ========== Phase III Step 2: Call imageNotifyFinished() and image load event after SVG loading completes design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8v... This CL calls ImageResourceObserver::ImageNotifyFinished() after SVG image loading is completed, not necessarily the same time as ResourceLoader completion (e.g. DidFinishLoading()). This makes SVG's <img> load event to be fired after SVG image loading is completed even when the SVG has subresources loaded asynchronously. 1. Image::SetData() can return SizeAvailableAndLoadingAsynchronously when SVG image loading is not completed synchronously (due to subresources), and notifies the completion asynchronously via ImageObserver:LoadCompleted(). LocalFrameClient::DispatchDidHandleOnloadEvents() is used as a notification of SVG image loading completion in SVGImage. 2. ImageResourceContent delays calling ImageNotifyFinished() and setting |content_status_| to kCached until SVG image loading is completed. 3. ImageLoader additionally delays the Document load event - from the first ImageChanged() during image loading - until (potentially delayed) ImageNotifyFinished(). In image loading not by ImageLoader, the Document load event is not delayed from ResourceLoader completion until ImageNotifyFinished(), but this is not a regression. In image loading by ImageLoader, the Document load event is delayed until ImageNotifyFinished() by the combination of ResourceFetcher::loaders_ and ImageLoader. ImageLoader starts delaying the Document load event - Not from |image_| is set, because we shouldn't delay the Document load event when image loading is deferred. - Not from ImageResourceContent::NotifyStartLoad(), to avoid additional ImageResourceObserver callbacks. BUG=382170, 690480, 667641 Review-Url: https://codereview.chromium.org/2613853002 Cr-Commit-Position: refs/heads/master@{#472229} Committed: https://chromium.googlesource.com/chromium/src/+/396a1266fb7549e03e5bcff793ac... ========== to ========== Phase III Step 2: Call imageNotifyFinished() and image load event after SVG loading completes design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... https://docs.google.com/document/d/1obEb8K4mJpG9Y_x4_ZhvnbFHojZoLLzMCFsalkl8v... This CL calls ImageResourceObserver::ImageNotifyFinished() after SVG image loading is completed, not necessarily the same time as ResourceLoader completion (e.g. DidFinishLoading()). This makes SVG's <img> load event to be fired after SVG image loading is completed even when the SVG has subresources loaded asynchronously. 1. Image::SetData() can return SizeAvailableAndLoadingAsynchronously when SVG image loading is not completed synchronously (due to subresources), and notifies the completion asynchronously via ImageObserver:LoadCompleted(). LocalFrameClient::DispatchDidHandleOnloadEvents() is used as a notification of SVG image loading completion in SVGImage. 2. ImageResourceContent delays calling ImageNotifyFinished() and setting |content_status_| to kCached until SVG image loading is completed. 3. ImageLoader additionally delays the Document load event - from the first ImageChanged() during image loading - until (potentially delayed) ImageNotifyFinished(). In image loading not by ImageLoader, the Document load event is not delayed from ResourceLoader completion until ImageNotifyFinished(), but this is not a regression. In image loading by ImageLoader, the Document load event is delayed until ImageNotifyFinished() by the combination of ResourceFetcher::loaders_ and ImageLoader. ImageLoader starts delaying the Document load event - Not from |image_| is set, because we shouldn't delay the Document load event when image loading is deferred. - Not from ImageResourceContent::NotifyStartLoad(), to avoid additional ImageResourceObserver callbacks. BUG=382170, 690480, 667641 Review-Url: https://codereview.chromium.org/2613853002 Cr-Commit-Position: refs/heads/master@{#472229} Committed: https://chromium.googlesource.com/chromium/src/+/396a1266fb7549e03e5bcff793ac... ========== |
