Bug 167997 – Use smaller tiles in windows that are not active to facilitate App Napping

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Chris Dumez]

Use smaller tiles in windows that are not active to facilitate App Napping.

[Chris Dumez]

<rdar://problem/30358835>

[Chris Dumez]

Created attachment 300906 [details]
WIP Patch

Working on adding a test.

[Geoffrey Garen]

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?

[Chris Dumez]

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).

[Simon Fraser (smfr)]

Comment on attachment 300906 [details]
WIP Patch

Looks generally OK. Does this patch have any iOS impact?

[Chris Dumez]

(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.

[Chris Dumez]

Created attachment 300932 [details]
WIP patch

[Chris Dumez]

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.

[Simon Fraser (smfr)]

The tile size is adjusted on a:

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

timer.

[Chris Dumez]

(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?

[Tim Horton]

(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?

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Chris Dumez]

Created attachment 300955 [details]
WIP Patch

[Chris Dumez]

Created attachment 300963 [details]
Patch

[Simon Fraser (smfr)]

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.

[Chris Dumez]

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

[Chris Dumez]

Created attachment 300972 [details]
Patch

[Tim Horton]

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???

[Chris Dumez]

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

[Chris Dumez]

Created attachment 300973 [details]
Patch

[Tim Horton]

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.

[Chris Dumez]

Created attachment 300974 [details]
Patch

[Simon Fraser (smfr)]

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.

[Chris Dumez]

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

[Build Bot]

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

[Build Bot]

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

[Chris Dumez]

Committed r211910: <http://trac.webkit.org/changeset/211910>