Created attachment 245058 [details] CachedImage: ensure clients overrides imageChanged instead of notifyFinished CachedImageClient defines imageChanged which is meant to be called whenever a frame of an image changes because we got more data from the network. But some clients still override CachedResource::notifyFinished because in imageChanged it is not possible to know if the image is acutally enterly loaded or still loading. (Indeed when imageChanged is called, isLoaded() never return true, even from CachedImage::finishLoading) If we compare CachedImage to CachedFont, the later has all its clients that do not override notifyFinished. Indeed in CachedFont::finishLoading there is a call to setLoaded(false) which is not there in CachedImage::finishLoading. So the following patch tries to simplify the situation by marking CachedImageClient::notifyFinished final and making the refactoring that this change implies.
Comment on attachment 245058 [details] CachedImage: ensure clients overrides imageChanged instead of notifyFinished Attachment 245058 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5537325064912896 Number of test failures exceeded the failure limit.
Created attachment 245059 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 245058 [details] CachedImage: ensure clients overrides imageChanged instead of notifyFinished Attachment 245058 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4897212265922560 Number of test failures exceeded the failure limit.
Created attachment 245060 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 245061 [details] CachedImage: ensure clients overrides imageChanged instead of notifyFinished Forgot to replace ImageLoader::notifyFinished(cachedImage); by ImageLoader::imageChanged(cachedImage); in HTMLImageLoader::imageChanged.
Comment on attachment 245061 [details] CachedImage: ensure clients overrides imageChanged instead of notifyFinished This needs a changelog entry explaining the what/why.
Created attachment 245134 [details] CachedImage: ensure clients overrides imageChanged instead of notifyFinished Add ChangeLog
Created attachment 245530 [details] CachedImage: ensure clients overrides imageChanged instead of notifyFinished git rebase to current head
Comment on attachment 245530 [details] CachedImage: ensure clients overrides imageChanged instead of notifyFinished View in context: https://bugs.webkit.org/attachment.cgi?id=245530&action=review > Source/WebCore/ChangeLog:9 > + notifyFinished was called when the image was enterely loaded. sp.: "entirely" > Source/WebCore/ChangeLog:13 > + some both (ex: RenderImage). comma at the end of this line and into the 'which' > Source/WebCore/ChangeLog:16 > + For example when the image finished to load, both imageChanged "finished to load" should probably read "finished loading" > Source/WebCore/ChangeLog:18 > + first one isLoaded() return false. s/return/returned/ > Source/WebCore/ChangeLog:19 > + It could result in functions called twice in a row, "being called" > Source/WebCore/ChangeLog:23 > + CachedImageClient::notifyFinished final. In order to In order to what? > Source/WebCore/ChangeLog:25 > + Clients can now differenciates intermediates and end sp.: differentiate and intermediate should not be plural > Source/WebCore/ChangeLog:28 > + Because I moved setLoading(false) from CachedResource::finishLoading > + call to an explicit call in CachedImage::finishLoading. Not sure what this means or if it needs to be here. > Source/WebCore/ChangeLog:32 > + * html/HTMLImageLoader.cpp: It's possible you should split your changelog up down here and put relevant bits next to the names of the functions that those changes affected. > Source/WebCore/html/HTMLImageLoader.h:38 > + virtual void imageChanged(CachedImage*, const IntRect* = 0) override; 0 should be nullptr here. > Source/WebCore/loader/ImageLoader.h:76 > + virtual void imageChanged(CachedImage*, const IntRect* = 0) override; 0 should be nullptr here. > Source/WebCore/rendering/RenderImage.cpp:-368 > -void RenderImage::notifyFinished(CachedResource* newImage) Because of laziness, I'm not completely convinced that RenderImage::imageChanged's call to RenderImage::imageDimensionsChanged is going to always end up doing the invalidateBackgroundObscurationStatus and contentChanged that were here in all the same cases as it previously would have. Can you please confirm that it will? Most concerningly, here we unconditionally call contentChanged, but in imageDimensionsChanged we only call it conditionally (with a whole bunch of conditions).
Created attachment 245545 [details] CachedImage: ensure clients overrides imageChanged instead of notifyFinished I addressed remarks (grammar, missing end of a sentense, file explnations, nullptr) About the 2 other points concerning removal of RenderImage::notifyFinished: 1* invalidateBackgroundObscurationStatus: It will not always endup with invalidateBackgroundObscurationStatus as previously. Instead it will be done from RenderReplaced::layout() when imageDimensionsChanged calls setNeedsLayout(). The condition being: imageSizeChanged || hasOverrideSize || containingBlockNeedsToRecomputePreferredSize || layoutSizeDependsOnIntrinsicSize (also RenderReplaced::layout() can be triggered this way: Breakpoint 1, 0x00007fe784e5caf0 in WebCore::RenderReplaced::layout() () (gdb) bt #0 0x00007fe784e5caf0 in WebCore::RenderReplaced::layout() #1 0x00007fe784de92e5 in WebCore::RenderImage::layout() #2 0x00007fe784d8433b in WebCore::RenderBlockFlow::layoutLineBoxes(bool, WebCore::LayoutUnit&, WebCore::LayoutUnit&) #3 0x00007fe784d74805 in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) #4 0x00007fe784d5a056 in WebCore::RenderBlock::layout() #5 0x00007fe784d6f479 in WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) #6 0x00007fe784d704b8 in WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) #7 0x00007fe784d74d82 in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) #8 0x00007fe784d5a056 in WebCore::RenderBlock::layout() #9 0x00007fe784d6f479 in WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&) #10 0x00007fe784d704b8 in WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&) #11 0x00007fe784d74d82 in WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) #12 0x00007fe784d5a056 in WebCore::RenderBlock::layout() #13 0x00007fe784eb62b9 in WebCore::RenderView::layoutContent(WebCore::LayoutState const&) #14 0x00007fe784eb6719 in WebCore::RenderView::layout() #15 0x00007fe784c20d2d in WebCore::FrameView::layout(bool) #16 0x00007fe7848eee65 in WebCore::Document::implicitClose() #17 0x00007fe784b57807 in WebCore::FrameLoader::checkCompleted() #18 0x00007fe784bc09fe in WebCore::CachedResourceLoader::loadDone(WebCore::CachedResource*, bool) #19 0x00007fe784b80362 in WebCore::SubresourceLoader::notifyDone() #20 0x00007fe784b812a2 in WebCore::SubresourceLoader::didFinishLoading(double) #21 0x00007fe78507f4e8 in WebCore::readCallback(_GObject*, _GAsyncResult*, void*) Also it is worth to mention that the call to invalidateBackgroundObscurationStatus has been added by commit: git show cae24e092 which also added test LayoutTests/fast/backgrounds/obscured-background-child-style-change.html. And the test still passes with my patch. 2* contentChanged(ImageChanged): It will not always endup with contentChanged(ImageChanged) as previously. Indeed now it is always done except if intrinsicSizeChanged && !(imageSizeChanged || hasOverrideSize || containingBlockNeedsToRecomputePreferredSize || layoutSizeDependsOnIntrinsicSize). Which means when it should repaint. Also I suspect that RenderImage::notifyFinished, see commit ce3a4e007, has been added because of this setLoading(false) not being set before notifying clients (= one of the point of my patch). You can see in that commit that layer()->rendererContentChanged() (old name contentChanged(ImageChanged)) has been put both in imageChanged and notifyFinished. Also this commit contains test LayoutTests/compositing/direct-image-compositing.html which still passes.
Comment on attachment 245545 [details] CachedImage: ensure clients overrides imageChanged instead of notifyFinished Clearing flags on attachment: 245545 Committed r179340: <http://trac.webkit.org/changeset/179340>
All reviewed patches have been landed. Closing bug.
Comment on attachment 245545 [details] CachedImage: ensure clients overrides imageChanged instead of notifyFinished View in context: https://bugs.webkit.org/attachment.cgi?id=245545&action=review > Source/WebCore/html/HTMLImageLoader.cpp:77 > + ASSERT(cachedImage == image().get()); It broke the debug builds: 1>..\html\HTMLImageLoader.cpp(77): error C2228: left of '.get' must have class/struct/union type is 'WebCore::CachedImage *' did you intend to use '->' instead?
Comment on attachment 245545 [details] CachedImage: ensure clients overrides imageChanged instead of notifyFinished View in context: https://bugs.webkit.org/attachment.cgi?id=245545&action=review >> Source/WebCore/html/HTMLImageLoader.cpp:77 >> + ASSERT(cachedImage == image().get()); > > It broke the debug builds: > > 1>..\html\HTMLImageLoader.cpp(77): error C2228: left of '.get' must have class/struct/union > type is 'WebCore::CachedImage *' > did you intend to use '->' instead? Thx for raising it. It should be only image(). I am gonna make a patch right now.
Created attachment 245621 [details] HTMLImageLoader: fix build failure on assert condition after r179340
Comment on attachment 245621 [details] HTMLImageLoader: fix build failure on assert condition after r179340 rs=me, but please don't upload followup patches to closed bugs next time.
Comment on attachment 245621 [details] HTMLImageLoader: fix build failure on assert condition after r179340 Clearing flags on attachment: 245621 Committed r179344: <http://trac.webkit.org/changeset/179344>
Re-opened since this is blocked by bug 142598
This caused various problems with ImageLoaders getting stuck with m_hasPendingLoadEvent=true, which prevents the garbage collector from deleting these elements. I'm rolling the change out since it wasn't clearly fixing a bug in the first place. I suspect that the main cause of problems is that FrameLoader::stopAllLoaders(), which is called on navigation, ends up causing pending CachedResources to become cancelled, but cancelling a CachedResources fires notifyFinished(), not imageChanged().
Could we have detected this regression in a layout test?
(In reply to comment #21) > Could we have detected this regression in a layout test? Yes, it should be possible to construct a test which schedules a bunch of image loads, and then navigates to a new page before they can complete. The test would then have to verify that those image elements went away. It could do this using DOM node counters in window.internals, for example.
Hi I understand now why it has made a regression. Well there is no easy way to handle it so lets keep imageChanged/notifyFinished. Instead I suggested this patch: https://bugs.webkit.org/attachment.cgi?id=249745&action=diff. If it gets accepted and merged then I suggest to simply close the bug.