Bug 187504 – REGRESSION (r233480): Mail contents flash black when activating

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

REGRESSION (r233480): Mail contents flash black when activating Mail

[Tim Horton]

Created attachment 344658 [details]
Patch

[Tim Horton]

<rdar://problem/41752351>

Comment on attachment 344658 [details]
Patch

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

> Source/WebKit/UIProcess/ApplicationStateTracker.mm:100
> +    m_didCreateWindowContextObserver = [notificationCenter addObserverForName:@"_UICanvasDidActivateNotification" object:window._canvas queue:nil usingBlock:[weakThis](NSNotification *) {

This is repeating the mistake of hardcoding a string instead of importing a symbol.

[Tim Horton]

(In reply to mitz from comment #3)
> Comment on attachment 344658 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344658&action=review
> 
> > Source/WebKit/UIProcess/ApplicationStateTracker.mm:100
> > +    m_didCreateWindowContextObserver = [notificationCenter addObserverForName:@"_UICanvasDidActivateNotification" object:window._canvas queue:nil usingBlock:[weakThis](NSNotification *) {
> 
> This is repeating the mistake of hardcoding a string instead of importing a
> symbol.

Sadly I have learned no lessons because that wouldn’t have saved us last time...

[Simon Fraser (smfr)]

Comment on attachment 344658 [details]
Patch

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

>>> Source/WebKit/UIProcess/ApplicationStateTracker.mm:100
>>> +    m_didCreateWindowContextObserver = [notificationCenter addObserverForName:@"_UICanvasDidActivateNotification" object:window._canvas queue:nil usingBlock:[weakThis](NSNotification *) {
>> 
>> This is repeating the mistake of hardcoding a string instead of importing a symbol.
> 
> Sadly I have learned no lessons because that wouldn’t have saved us last time...

Can we just use some public "app will come to the foreground" notification?

[Tim Horton]

I’m sure there’s some reason I didn’t use UIApplicationWillEnterForegroundNotification in the original patch, but I will check why :)

[Tim Horton]

(In reply to Tim Horton from comment #6)
> I’m sure there’s some reason I didn’t use
> UIApplicationWillEnterForegroundNotification in the original patch, but I
> will check why :)

There totally is. We need to be awoken and blanked out in snapshots that happen in the background. But now I don’t know if this patch is enough. Will continue investigating.

Don’t forget app extensions and view services!

[Tim Horton]

Sadly, this is insufficient.

[Tim Horton]

Created attachment 344741 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 344741 [details]
Patch

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

Please make it build.

> Source/WebKit/ChangeLog:15
> +        The point of hiding the web view's contents in didCreateWindowContext
> +        was to ensure we did not hide it before the initial background snapshot
> +        (so that in the case of simple app, the background snapshot contains all
> +        WKWebViews), but then hide it if the app was reawoken in the background
> +        for snapshot updates (because layer contents could be emptied at that point),
> +        or (at the latest) when the app itself comes back to the foreground (for
> +        the same reason).

This is hard to understand.

[Tim Horton]

I will try to make it easier to understand. If only I could draw pictures. Maybe I can!

[Tim Horton]

I seem to have missed a piece of “remove the old thing”

[Tim Horton]

Created attachment 344754 [details]
Patch

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #11)
> This is hard to understand.

I really hope this is better :) At least, it will be good if we need to look back on the problem again in the future.

[WebKit Commit Bot]

Comment on attachment 344754 [details]
Patch

Clearing flags on attachment: 344754

Committed r233723: <https://trac.webkit.org/changeset/233723>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.