Bug 187374 – [iOS] When bringing MobileSafari to the foreground, images are drawn asynchronously after removing a snapshot that included them

• Component: Images
• Status: RESOLVED FIXED

[Said Abou-Hallawa]

When Mobile Safari is brought to the foreground, it may have a snapshot from the page before it went to sleep. An immediate update is needed to refresh the page in case the contents have changed. In this case, we need to draw all the images synchronously even if the tile is painted for the first time. This will prevent unwanted flashing in case the page contents were not changed.

To do that:

-- SetActivityState message will include an activityStateChangeID member.

-- LayerTreeTransaction will include an activityStateChangeID member.

-- UIProcess sends an activityStateChangeID with the SetActivityState message if FlushImmediately paint is needed. Otherwise it sends zero. UIProcess waits for the LayerTreeTransaction whose activityStateChangeID is greater than or equal to the one it sent. If it receives the desired LayerTreeTransaction, the tree layer will be committed otherwise it will be ignored.

-- WebProcess will read the activityStateChangeID from the SetActivityState message and will pass it in LayerTreeTransaction. Otherwise it will pass zero.

[Said Abou-Hallawa]

Created attachment 344387 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/41249545>

[Tim Horton]

Comment on attachment 344387 [details]
Patch

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

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreeTransaction.h:264
> +    uint64_t activityStateChangeID() const { return m_activityStateChangeID; }

3...2...1... until Simon shows up and asks for a typedef here.

> Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm:451
> +    while (m_webPageProxy.process().connection()->waitForAndDispatchImmediately<Messages::RemoteLayerTreeDrawingAreaProxy::CommitLayerTree>(m_webPageProxy.pageID(), activityStateUpdateTimeout, IPC::WaitForOption::InterruptWaitingIfSyncMessageArrives)) {

You've completely defeated the timeout here.

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:488
> -void RemoteLayerTreeDrawingArea::activityStateDidChange(ActivityState::Flags, bool wantsDidUpdateActivityState, const Vector<CallbackID>&)
> +void RemoteLayerTreeDrawingArea::activityStateDidChange(ActivityState::Flags, uint64_t activityStateChangeID, const Vector<CallbackID>&)

This is a little odd (and also breaking the build). It's weird that activityStateChangeID isn't for every activityStateChange, only ones that want a reply.

[Simon Fraser (smfr)]

Comment on attachment 344387 [details]
Patch

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

>> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreeTransaction.h:264
>> +    uint64_t activityStateChangeID() const { return m_activityStateChangeID; }
> 
> 3...2...1... until Simon shows up and asks for a typedef here.

Yes please.

> Source/WebKit/UIProcess/DrawingAreaProxy.h:87
> +    virtual void waitForDidUpdateActivityState(uint64_t) { }

That would make this more readable.

> Source/WebKit/UIProcess/WebPageProxy.h:1130
> +    uint64_t takeNextActivityStateChangeID() { return ++m_currentActivityStateChangeID; }

I'd like a comment that tells me what an "activity state" is in this context.

[EWS Watchlist]

Comment on attachment 344387 [details]
Patch

Attachment 344387 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8451512

New failing tests:
http/tests/security/contentSecurityPolicy/video-redirect-allowed.html

[EWS Watchlist]

Created attachment 344395 [details]
Archive of layout-test-results from ews201 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Said Abou-Hallawa]

Created attachment 344427 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 344387 [details]
Patch

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

>>> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreeTransaction.h:264
>>> +    uint64_t activityStateChangeID() const { return m_activityStateChangeID; }
>> 
>> 3...2...1... until Simon shows up and asks for a typedef here.
> 
> Yes please.

A typedef for ActivityStateChangeID was added.

>> Source/WebKit/UIProcess/DrawingAreaProxy.h:87
>> +    virtual void waitForDidUpdateActivityState(uint64_t) { }
> 
> That would make this more readable.

Done.

>> Source/WebKit/UIProcess/WebPageProxy.h:1130
>> +    uint64_t takeNextActivityStateChangeID() { return ++m_currentActivityStateChangeID; }
> 
> I'd like a comment that tells me what an "activity state" is in this context.

A comment was added to explain the when the ActivityStateChangeID is sent and what it is used for.

>> Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm:451
>> +    while (m_webPageProxy.process().connection()->waitForAndDispatchImmediately<Messages::RemoteLayerTreeDrawingAreaProxy::CommitLayerTree>(m_webPageProxy.pageID(), activityStateUpdateTimeout, IPC::WaitForOption::InterruptWaitingIfSyncMessageArrives)) {
> 
> You've completely defeated the timeout here.

Yes you are right. I missed this point in the last patch. I will update it.

>> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.mm:488
>> +void RemoteLayerTreeDrawingArea::activityStateDidChange(ActivityState::Flags, uint64_t activityStateChangeID, const Vector<CallbackID>&)
> 
> This is a little odd (and also breaking the build). It's weird that activityStateChangeID isn't for every activityStateChange, only ones that want a reply.

I think passing wantsDidUpdateActivityState and activityStateChangeID will be redundant. If wantsDidUpdateActivityState = false, activityStateChangeID has to be = 0. And if wantsDidUpdateActivityState = true, activityStateChangeID has be > 0.

[Said Abou-Hallawa]

Created attachment 344430 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 344387 [details]
Patch

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

>>> Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm:451
>>> +    while (m_webPageProxy.process().connection()->waitForAndDispatchImmediately<Messages::RemoteLayerTreeDrawingAreaProxy::CommitLayerTree>(m_webPageProxy.pageID(), activityStateUpdateTimeout, IPC::WaitForOption::InterruptWaitingIfSyncMessageArrives)) {
>> 
>> You've completely defeated the timeout here.
> 
> Yes you are right. I missed this point in the last patch. I will update it.

Done.

[Tim Horton]

Comment on attachment 344430 [details]
Patch

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

> Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm:454
> +    while (m_webPageProxy.process().connection()->waitForAndDispatchImmediately<Messages::RemoteLayerTreeDrawingAreaProxy::CommitLayerTree>(m_webPageProxy.pageID(), activityStateUpdateTimeout - (MonotonicTime::now() - startTime), IPC::WaitForOption::InterruptWaitingIfSyncMessageArrives)) {

Is this OK if it timeout - delta goes negative?

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.h:166
> +    ActivityStateChangeID m_activityStateChangeID { ActivityStateChangeAsynchronous };

IDs with “special values” that actually mean something are a little weird :P

[Said Abou-Hallawa]

Comment on attachment 344430 [details]
Patch

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

>> Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeDrawingAreaProxy.mm:454
>> +    while (m_webPageProxy.process().connection()->waitForAndDispatchImmediately<Messages::RemoteLayerTreeDrawingAreaProxy::CommitLayerTree>(m_webPageProxy.pageID(), activityStateUpdateTimeout - (MonotonicTime::now() - startTime), IPC::WaitForOption::InterruptWaitingIfSyncMessageArrives)) {
> 
> Is this OK if it timeout - delta goes negative?

Yes.

Connection::waitForAndDispatchImmediately() calls Connection::waitForMessage() timeout < 0.
Connection::waitForMessage() calculates absoluteTimeout = MonotonicTime::now() + timeout.
Connection::waitForMessage() calls Condition::waitUntil() with timeout < MonotonicTime::now().
Condition::waitUntil() will return false because it checks timeout < timeout.nowWithSameClock() at the beginning which will be true in this case.

>> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/RemoteLayerTreeDrawingArea.h:166
>> +    ActivityStateChangeID m_activityStateChangeID { ActivityStateChangeAsynchronous };
> 
> IDs with “special values” that actually mean something are a little weird :P

is this better?

ActivityStateChangeID m_activityStateChangeID { ActivityStateChangeIDNil };

[Said Abou-Hallawa]

Created attachment 344854 [details]
Patch

[Tim Horton]

Comment on attachment 344854 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        [iOS] When asking for FlushImmediately, the UIProcess should ignore all the layer tree transactions which are sent before this request is received by the WebProcess

The title could use some love. “When asking for FlushImmediately” means nothing to me unless I already have context.

Also, we’re not /ignoring/ them (that would be disastrous), we’re just synchronously applying a bunch in a row.

> Source/WebKit/Shared/DrawingAreaInfo.h:42
> +    ActivityStateChangeIDNil = 0

Not really better. Go with the previous one.

[EWS Watchlist]

Comment on attachment 344854 [details]
Patch

Attachment 344854 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8517587

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 344869 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Said Abou-Hallawa]

Created attachment 344879 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 344879 [details]
Patch

Clearing flags on attachment: 344879

Committed r233781: <https://trac.webkit.org/changeset/233781>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.