Bug 187504 - REGRESSION (r233480): Mail contents flash black when activating
Summary: REGRESSION (r233480): Mail contents flash black when activating
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-07-09 20:35 PDT by Tim Horton
Modified: 2018-07-10 23:22 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.95 KB, patch)
2018-07-09 20:35 PDT, Tim Horton
mitz: review+
Details | Formatted Diff | Diff
Patch (10.20 KB, patch)
2018-07-10 17:54 PDT, Tim Horton
no flags Details | Formatted Diff | Diff
Patch (12.53 KB, patch)
2018-07-10 22:42 PDT, Tim Horton
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2018-07-09 20:35:00 PDT
REGRESSION (r233480): Mail contents flash black when activating Mail
Comment 1 Tim Horton 2018-07-09 20:35:15 PDT
Created attachment 344658 [details]
Patch
Comment 2 Tim Horton 2018-07-09 20:35:28 PDT
<rdar://problem/41752351>
Comment 3 mitz 2018-07-09 20:42:42 PDT
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.
Comment 4 Tim Horton 2018-07-09 21:17:39 PDT
(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...
Comment 5 Simon Fraser (smfr) 2018-07-09 22:13:03 PDT
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?
Comment 6 Tim Horton 2018-07-09 22:42:38 PDT
I’m sure there’s some reason I didn’t use UIApplicationWillEnterForegroundNotification in the original patch, but I will check why :)
Comment 7 Tim Horton 2018-07-09 23:11:51 PDT
(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.
Comment 8 mitz 2018-07-09 23:13:13 PDT
Don’t forget app extensions and view services!
Comment 9 Tim Horton 2018-07-10 16:38:35 PDT
Sadly, this is insufficient.
Comment 10 Tim Horton 2018-07-10 17:54:30 PDT
Created attachment 344741 [details]
Patch
Comment 11 Simon Fraser (smfr) 2018-07-10 18:58:44 PDT
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.
Comment 12 Tim Horton 2018-07-10 19:15:19 PDT
I will try to make it easier to understand. If only I could draw pictures. Maybe I can!
Comment 13 Tim Horton 2018-07-10 19:16:03 PDT
I seem to have missed a piece of “remove the old thing”
Comment 14 Tim Horton 2018-07-10 22:42:31 PDT
Created attachment 344754 [details]
Patch
Comment 15 Tim Horton 2018-07-10 22:43:15 PDT
(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.
Comment 16 WebKit Commit Bot 2018-07-10 23:22:21 PDT
Comment on attachment 344754 [details]
Patch

Clearing flags on attachment: 344754

Committed r233723: <https://trac.webkit.org/changeset/233723>
Comment 17 WebKit Commit Bot 2018-07-10 23:22:22 PDT
All reviewed patches have been landed.  Closing bug.