Make image-loading-lazy-slow.html pass. Note that this was not tested before because it had no custom webkit-test-runner directive to enable lazy image loading.
Created attachment 407961 [details] Patch
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Created attachment 407968 [details] Patch
Comment on attachment 407968 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407968&action=review > Source/WebCore/loader/ImageLoader.cpp:280 > + if (document.lazyLoadImageObserver().isObserved(element())) Do we need this if check? > Source/WebCore/loader/ImageLoader.cpp:281 > + LazyLoadImageObserver::unobserve(element(), element().document()); This is ok as is, but it would be a bit better if it was reverse so that we unobserve closer to setting m_image. Something like: if (!m_image) unobserve.... There are other code paths that sets back m_image to nullptr. Should we unobserve in those cases as well? Maybe we should introduce a clearImage() method that would set back m_image to nullptr and unobserve.
Comment on attachment 407968 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407968&action=review > LayoutTests/ChangeLog:8 > + Mark image-loading-lazy-slow.html as ImageOnlyFailure. So this patch doesn't do what its title says?
(In reply to Alexey Proskuryakov from comment #5) > Comment on attachment 407968 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=407968&action=review > > > LayoutTests/ChangeLog:8 > > + Mark image-loading-lazy-slow.html as ImageOnlyFailure. > > So this patch doesn't do what its title says? You are right, I was expecting the test would pass once the crash is fixed, but with the crash fixed we run into https://bugs.webkit.org/show_bug.cgi?id=186045.
Created attachment 408165 [details] Patch
<rdar://problem/68454263>
Comment on attachment 407968 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407968&action=review >> Source/WebCore/loader/ImageLoader.cpp:280 >> + if (document.lazyLoadImageObserver().isObserved(element())) > > Do we need this if check? Right, that one was a bit ugly, I changed isDeferred to include testing for LoadImmediate, and instead of above check check for isDeferred. >> Source/WebCore/loader/ImageLoader.cpp:281 >> + LazyLoadImageObserver::unobserve(element(), element().document()); > > This is ok as is, but it would be a bit better if it was reverse so that we unobserve closer to setting m_image. > Something like: > if (!m_image) > unobserve.... > > There are other code paths that sets back m_image to nullptr. > Should we unobserve in those cases as well? > > Maybe we should introduce a clearImage() method that would set back m_image to nullptr and unobserve. I think in the other places we do not need to unobserve. But I added a method resetLazyImageLoading which we can re-use if needed.
Committed r266720: <https://trac.webkit.org/changeset/266720> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408165 [details].