Bug 167997 - Use smaller tiles in windows that are not active to facilitate App Napping
Summary: Use smaller tiles in windows that are not active to facilitate App Napping
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-02-08 08:57 PST by Chris Dumez
Modified: 2022-02-23 15:55 PST (History)
8 users (show)

See Also:


Attachments
WIP Patch (5.54 KB, patch)
2017-02-08 08:59 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (9.91 KB, patch)
2017-02-08 11:39 PST, Chris Dumez
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (994.02 KB, application/zip)
2017-02-08 12:47 PST, Build Bot
no flags Details
Archive of layout-test-results from ews126 for ios-simulator-wk2 (884.20 KB, application/zip)
2017-02-08 13:04 PST, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-elcapitan (1.61 MB, application/zip)
2017-02-08 13:17 PST, Build Bot
no flags Details
WIP Patch (14.53 KB, patch)
2017-02-08 13:39 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (22.25 KB, patch)
2017-02-08 14:12 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (22.41 KB, patch)
2017-02-08 15:04 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (22.42 KB, patch)
2017-02-08 15:11 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (20.06 KB, patch)
2017-02-08 15:24 PST, Chris Dumez
simon.fraser: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews121 for ios-simulator-wk2 (11.58 MB, application/zip)
2017-02-08 16:55 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-02-08 08:57:04 PST
Use smaller tiles in windows that are not active to facilitate App Napping.
Comment 1 Chris Dumez 2017-02-08 08:57:33 PST
<rdar://problem/30358835>
Comment 2 Chris Dumez 2017-02-08 08:59:05 PST
Created attachment 300906 [details]
WIP Patch

Working on adding a test.
Comment 3 Geoffrey Garen 2017-02-08 09:03:00 PST
Comment on attachment 300906 [details]
WIP Patch

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

> Source/WebCore/page/FrameView.cpp:2790
> +    // Use square tiles if the Window is not active to facilitate app napping.

I think you must have meant something other than square?
Comment 4 Chris Dumez 2017-02-08 09:05:07 PST
Comment on attachment 300906 [details]
WIP Patch

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

>> Source/WebCore/page/FrameView.cpp:2790
>> +    // Use square tiles if the Window is not active to facilitate app napping.
> 
> I think you must have meant something other than square?

smaller square tiles maybe? Will basically go back to 512x512, instead of (extra wide tiles or extra high tiles or one big tile).
Comment 5 Simon Fraser (smfr) 2017-02-08 09:10:54 PST
Comment on attachment 300906 [details]
WIP Patch

Looks generally OK. Does this patch have any iOS impact?
Comment 6 Chris Dumez 2017-02-08 09:26:49 PST
(In reply to comment #5)
> Comment on attachment 300906 [details]
> WIP Patch
> 
> Looks generally OK. Does this patch have any iOS impact?

My patch may impact the tile sizes in the split screen case on iPad if you have 2 safari windows. I need to confirm. That said, we do not care about App Napping there so I do not need to impact iOS.
Comment 7 Chris Dumez 2017-02-08 11:39:23 PST
Created attachment 300932 [details]
WIP patch
Comment 8 Chris Dumez 2017-02-08 11:40:13 PST
Comment on attachment 300932 [details]
WIP patch

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

> LayoutTests/compositing/tiling/non-active-window-tiles-size.html:18
> +    setTimeout(function() {

Simon: I need a better way to synchronize than this setTimeout(), any idea? a setTimeout(x, 0) does not suffice.
Comment 9 Simon Fraser (smfr) 2017-02-08 11:47:30 PST
The tile size is adjusted on a:

static const auto tileSizeUpdateDelay = std::chrono::milliseconds { 500 };

timer.
Comment 10 Chris Dumez 2017-02-08 12:41:13 PST
(In reply to comment #9)
> The tile size is adjusted on a:
> 
> static const auto tileSizeUpdateDelay = std::chrono::milliseconds { 500 };
> 
> timer.

So you think, my setTimeout() is the way to go?
Comment 11 Tim Horton 2017-02-08 12:47:26 PST
(In reply to comment #10)
> (In reply to comment #9)
> > The tile size is adjusted on a:
> > 
> > static const auto tileSizeUpdateDelay = std::chrono::milliseconds { 500 };
> > 
> > timer.
> 
> So you think, my setTimeout() is the way to go?

Certainly a 500ms timer in a test is not the way to go.

Maybe add an internals mechanism to force the update to happen immediately?
Comment 12 Build Bot 2017-02-08 12:47:35 PST
Comment on attachment 300932 [details]
WIP patch

Attachment 300932 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3026693

New failing tests:
compositing/tiling/non-active-window-tiles-size.html
Comment 13 Build Bot 2017-02-08 12:47:40 PST
Created attachment 300938 [details]
Archive of layout-test-results from ews100 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 14 Build Bot 2017-02-08 13:04:03 PST
Comment on attachment 300932 [details]
WIP patch

Attachment 300932 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3026720

New failing tests:
compositing/tiling/non-active-window-tiles-size.html
Comment 15 Build Bot 2017-02-08 13:04:07 PST
Created attachment 300944 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 16 Build Bot 2017-02-08 13:17:39 PST
Comment on attachment 300932 [details]
WIP patch

Attachment 300932 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3026739

New failing tests:
compositing/tiling/non-active-window-tiles-size.html
Comment 17 Build Bot 2017-02-08 13:17:43 PST
Created attachment 300948 [details]
Archive of layout-test-results from ews114 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 18 Chris Dumez 2017-02-08 13:39:44 PST
Created attachment 300955 [details]
WIP Patch
Comment 19 Chris Dumez 2017-02-08 14:12:20 PST
Created attachment 300963 [details]
Patch
Comment 20 Simon Fraser (smfr) 2017-02-08 14:20:24 PST
Comment on attachment 300963 [details]
Patch

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

> Source/WebCore/page/Page.h:365
> +    bool isWindowActive() const;

Would read a bit better as "isWindowActive(), or maybe isInActiveWindow()

Also, confusing with DOMWindow is likely, and do they have some notion of being "active"? Can we rename this to make it clear it's the native window that's active? Say frontmost?

> LayoutTests/compositing/tiling/non-active-window-tiles-size.html:7
> +<div id="activeResult"></div>
> +<p>Tiles when window is not active:</p>
> +<div id="nonActiveResult"></div>

Make the divs <pre> to preserve formatting.
Comment 21 Chris Dumez 2017-02-08 14:27:47 PST
Comment on attachment 300963 [details]
Patch

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

>> Source/WebCore/page/Page.h:365
>> +    bool isWindowActive() const;
> 
> Would read a bit better as "isWindowActive(), or maybe isInActiveWindow()
> 
> Also, confusing with DOMWindow is likely, and do they have some notion of being "active"? Can we rename this to make it clear it's the native window that's active? Say frontmost?

I tried to match the name of our activity state: ActivityState::WindowIsActive. Also note that we already have a bool isVisibleAndActive() const; getter on Page which queries this state already.

>> LayoutTests/compositing/tiling/non-active-window-tiles-size.html:7
>> +<div id="nonActiveResult"></div>
> 
> Make the divs <pre> to preserve formatting.

Will do
Comment 22 Chris Dumez 2017-02-08 15:04:46 PST
Created attachment 300972 [details]
Patch
Comment 23 Tim Horton 2017-02-08 15:09:32 PST
Comment on attachment 300972 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:398
> +            backing->setTileSizeUpdateDelayDisabledForTesting(true);

Shouldn't this say false???
Comment 24 Chris Dumez 2017-02-08 15:10:34 PST
Comment on attachment 300972 [details]
Patch

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

>> Source/WebCore/testing/Internals.cpp:398
>> +            backing->setTileSizeUpdateDelayDisabledForTesting(true);
> 
> Shouldn't this say false???

Oops :D
Comment 25 Chris Dumez 2017-02-08 15:11:23 PST
Created attachment 300973 [details]
Patch
Comment 26 Tim Horton 2017-02-08 15:14:17 PST
Comment on attachment 300972 [details]
Patch

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

> Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm:165
> +    [platformView() _updateActivityState];

This really isn't right. We already listen to windowDidBecomeKey and windowDidResignKey notifications, WKTR just isn't sending them because of the way it mocks out keyness. Seems like it would be better to fix that rather than expose bizarro internals as SPI.
Comment 27 Chris Dumez 2017-02-08 15:24:47 PST
Created attachment 300974 [details]
Patch
Comment 28 Simon Fraser (smfr) 2017-02-08 15:26:53 PST
Comment on attachment 300974 [details]
Patch

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

> Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm:168
> +    if (m_windowIsKey)
> +        [m_window makeKeyWindow];
> +    else
> +        [m_window resignKeyWindow];

Does this get reset between tests?

> LayoutTests/platform/ios-simulator-wk2/compositing/tiling/non-active-window-tiles-size-expected.txt:7
> +(GraphicsLayer
> +(anchor 0.00 0.00)
> +(bounds 800.00 600.00)
> +(visible rect 0.00, 0.00 800.00 x 600.00)
> +(coverage rect 0.00, 0.00 800.00 x 600.00)

I think you need to re-generate these results.
Comment 29 Chris Dumez 2017-02-08 15:29:37 PST
Comment on attachment 300974 [details]
Patch

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

>> Tools/WebKitTestRunner/mac/PlatformWebViewMac.mm:168
>> +        [m_window resignKeyWindow];
> 
> Does this get reset between tests?

TestController::resetStateToConsistentValues() calls m_mainWebView->focus(); which calls setWindowIsKey(true);

>> LayoutTests/platform/ios-simulator-wk2/compositing/tiling/non-active-window-tiles-size-expected.txt:7
>> +(coverage rect 0.00, 0.00 800.00 x 600.00)
> 
> I think you need to re-generate these results.

I know, I'll get them from EWS :P
Comment 30 Build Bot 2017-02-08 16:55:10 PST
Comment on attachment 300974 [details]
Patch

Attachment 300974 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3028159

New failing tests:
compositing/tiling/non-active-window-tiles-size.html
Comment 31 Build Bot 2017-02-08 16:55:16 PST
Created attachment 300985 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 32 Chris Dumez 2017-02-08 16:58:18 PST
Committed r211910: <http://trac.webkit.org/changeset/211910>