Bug 174829 – Async image decoding can be enabled till the page milestone DidHitRelevantRepaintedObjectsAreaThreshold is reached

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Images

[Said Abou-Hallawa]

After http://trac.webkit.org/changeset/219876, the async image decoding for large images is enabled only for the first time a tile is painted. This restriction can be relaxed while the page milestone 'DidHitRelevantRepaintedObjectsAreaThreshold' has not been reached. Reaching this milestone means the page has repainted enough contents to be readable for the first time.

To know whether the page has reached this milestone or not, every time a relevant object is repainted, its rectangle is added to the page relevant objects region. If the total areas of the non-intersected rectangles of this region becomes more than a threshold value, the page is marked as didReachLayoutMilestone(DidHitRelevantRepaintedObjectsAreaThreshold).

Page::didFinishLoad() cancels counting the renderers' rectangles as an optimization. The reason of this optimization is this milestone is usually used as an indication if the page has enough contents to be readable or not. If the page is completely loaded or the page painted enough contents for the first time to be readable, the UI can mark this page loaded. But this optimization can disable the async image decoding for most of the pages for the first time these pages are painted. We can overcome this problem if we continue counting the relevant renderers' rectangles for 50ms after the Page::didFinishLoad() fires.

[Said Abou-Hallawa]

Created attachment 316384 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/33520358>

[Simon Fraser (smfr)]

Comment on attachment 316384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316384&action=review

> Source/WebCore/page/Page.cpp:966
> +    static const double timeAfterDidFinishLoadToPaint = 0.5;

Seconds

> Source/WebCore/page/Page.cpp:1831
> +    if (m_timeResetRelevantPaintedObjectCounter && m_timeResetRelevantPaintedObjectCounter < currentTime())

MonotonicTime::now()

> Source/WebCore/page/Page.cpp:1943
> +bool Page::objectPaintRecrIntersectsRelevantPaintedRegion(const LayoutRect& objectPaintRect) const

"RecrIntersects"

> Source/WebCore/page/Page.cpp:1945
> +    return m_topRelevantPaintedRegion.intersects(snappedIntRect(objectPaintRect)) || m_bottomRelevantPaintedRegion.intersects(snappedIntRect(objectPaintRect));

Does this all work in iframes?

> Source/WebCore/page/Page.h:764
> +    double m_timeResetRelevantPaintedObjectCounter { 0 };

Please use Seconds or Monotonic time (I can't tell from the variable name if this is a duration or an absolute time so it needs a better name).

[Said Abou-Hallawa]

Created attachment 316458 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 316384 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=316384&action=review

>> Source/WebCore/page/Page.cpp:966
>> +    static const double timeAfterDidFinishLoadToPaint = 0.5;
> 
> Seconds

Seconds is used instead of double.

>> Source/WebCore/page/Page.cpp:1831
>> +    if (m_timeResetRelevantPaintedObjectCounter && m_timeResetRelevantPaintedObjectCounter < currentTime())
> 
> MonotonicTime::now()

MonotonicTime::now() is used instead of currentTime()

>> Source/WebCore/page/Page.cpp:1943
>> +bool Page::objectPaintRecrIntersectsRelevantPaintedRegion(const LayoutRect& objectPaintRect) const
> 
> "RecrIntersects"

Fixed.

>> Source/WebCore/page/Page.cpp:1945
>> +    return m_topRelevantPaintedRegion.intersects(snappedIntRect(objectPaintRect)) || m_bottomRelevantPaintedRegion.intersects(snappedIntRect(objectPaintRect));
> 
> Does this all work in iframes?

No. Should it work in iframes?. At the beginning of Page::addRelevantRepaintedObject(), we return if (&object->frame() != &mainFrame()) is true. I think this was added because most of the ad contents are added in separate <iframe> elements. And we do not want the ad contents to affect the page loading state.

>> Source/WebCore/page/Page.h:764
>> +    double m_timeResetRelevantPaintedObjectCounter { 0 };
> 
> Please use Seconds or Monotonic time (I can't tell from the variable name if this is a duration or an absolute time so it needs a better name).

Type of the variable was changed to MonotonicTime and the name was changed to m_resetRelevantPaintedObjectCounterTime.

[Said Abou-Hallawa]

I talked to Simon and we think this issue is not critical to be fixed now.