Bug 174336 – RenderImage should not add itself as a RelevantRepaintedObject if its image frame is being decoded

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

Since nothing will be drawn till the image frame finishes decoding we should treat returning ImageDrawResult::DidRequestDecoding from BitmapImage the same as we do when the image is still loading.

[Said Abou-Hallawa]

Created attachment 315050 [details]
Patch

[Andreas Kling]

Comment on attachment 315050 [details]
Patch

Is this about avoiding unnecessary repaints? If so, could we make a repaint test for it?

[Said Abou-Hallawa]

(In reply to Andreas Kling from comment #2)
> Comment on attachment 315050 [details]
> Patch
> 
> Is this about avoiding unnecessary repaints? If so, could we make a repaint
> test for it?

No. Adding the rectangle of a renderer to the Page::m_topRelevantPaintedRegion or m_bottomRelevantPaintedRegion helps making the decision whether the page reached the milestone 'DidHitRelevantRepaintedObjectsAreaThreshold' or not. Reaching this milestone means that the page has drawn enough relevant contents in the viewport such that we can consider, the page has completed its first draw.

This patch is fixing false information which the RenderImage is providing to the page when it says, it has drawn itself when it has just requested asynchronous decoding.

I don't know how this patch can be tested without adding new functions to the Internals. The problem also is Page::didFinishLoad() cancels counting the relevant repaint objects by calling resetRelevantPaintedObjectCounter().

[Tim Horton]

(In reply to Said Abou-Hallawa from comment #3)
> (In reply to Andreas Kling from comment #2)
> > Comment on attachment 315050 [details]
> > Patch
> > 
> > Is this about avoiding unnecessary repaints? If so, could we make a repaint
> > test for it?
> 
> No. Adding the rectangle of a renderer to the
> Page::m_topRelevantPaintedRegion or m_bottomRelevantPaintedRegion helps
> making the decision whether the page reached the milestone
> 'DidHitRelevantRepaintedObjectsAreaThreshold' or not. Reaching this
> milestone means that the page has drawn enough relevant contents in the
> viewport such that we can consider, the page has completed its first draw.
> 
> This patch is fixing false information which the RenderImage is providing to
> the page when it says, it has drawn itself when it has just requested
> asynchronous decoding.
> 
> I don't know how this patch can be tested without adding new functions to
> the Internals. The problem also is Page::didFinishLoad() cancels counting
> the relevant repaint objects by calling resetRelevantPaintedObjectCounter().

You can block didFinishLoad by making the main resource load hang (with either an HTTP test or an API test that gets its data from e.g. a custom URL scheme with WKURLSchemeHandler). We should have milestone callbacks in Internals or test runner or somewhere (or if it's an API test, it's easy to get them)

[WebKit Commit Bot]

Comment on attachment 315050 [details]
Patch

Clearing flags on attachment: 315050

Committed r219360: <http://trac.webkit.org/changeset/219360>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.