1478212 - RefreshSystemMetrics() doesn't propagate media feature changes in subframes

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

Attachment: background-color: Highlight in an iframe

Because we use MediaFeatureValuesChanged instead of MediaFeatureValuesChangedAllDocuments there [1]

Attaching file has a div element in an iframe whose background-color is 'Highlight'.  The background-color is not changed when the corresponding system color is changed.

I guess emilio wants to avoid unnecessary flushes in subframes.  For example gtk-csd-xx changes shouldn't be propagated in content process, I guess.  So we need to classify which change needs to be propagated into subframes, or something like that. 

[1] https://hg.mozilla.org/mozilla-central/file/dd386b5b9fa7/layout/base/nsPresContext.cpp#l1759

Comment 1

[Emilio Cobos Álvarez (:emilio)]

(In reply to Hiroyuki Ikezoe (:hiro) (PTO on Aug 20) from comment #0)
> Created attachment 8994733 [details]
> background-color: Highlight in an iframe
> 
> Because we use MediaFeatureValuesChanged instead of
> MediaFeatureValuesChangedAllDocuments there [1]
> 
> Attaching file has a div element in an iframe whose background-color is
> 'Highlight'.  The background-color is not changed when the corresponding
> system color is changed.
> 
> I guess emilio wants to avoid unnecessary flushes in subframes.  For example
> gtk-csd-xx changes shouldn't be propagated in content process, I guess.  So
> we need to classify which change needs to be propagated into subframes, or
> something like that. 
> 
> [1]
> https://hg.mozilla.org/mozilla-central/file/dd386b5b9fa7/layout/base/
> nsPresContext.cpp#l1759

I think system metrics changes are so relatively rare that it doesn't matter much, fwiw.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

From a comment in https://phabricator.services.mozilla.com/D3296 by Karl;

> Devices will be registered and de-registered more often than themes usually
> change.
> How does a NotifyThemeChanged() affect the performance of the browser?
> Would a user notice if there were no CSS depending on the media-features that
> changed?

I did check the behavior on a build which has the patches for bug 1035774 and a patch replacing MediaFeatureValuesChanged in RefreshSystemMetrics with MediaFeatureValuesChangedAllDocuments.

I can't notice any laggy things in browser on an Android phone (a variant of LG G2 released 5 years ago) when I plugged on/off a mouse to the phone.   So I think it doesn't matter on normal desktops.

Note that actually there is a laggy thing when I plugged a mouse, but it's caused by Android itself, a mouse image slides in, it's bit laggy.

Taking this since I'd want this for the test case for bug 1486971.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1478212 - Propagate MediaFeatureChangeReason::SystemMetricsChange into sub frames. r=emilio

A test case covers this will be introduced in bug 1486971.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9006428 [details]
Bug 1478212 - Propagate MediaFeatureChangeReason::SystemMetricsChange into sub frames. r=emilio

Emilio Cobos Álvarez (:emilio) has approved the revision.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9298e2f8195
Propagate MediaFeatureChangeReason::SystemMetricsChange into sub frames. r=emilio

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d9298e2f8195

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 9006428 [details]
Bug 1478212 - Propagate MediaFeatureChangeReason::SystemMetricsChange into sub frames. r=emilio

Approval Request Comment
[Feature/Bug causing the regression]: None
[User impact if declined]:  User won't see results by changing theme changes or system setting changes for prefers-reduced-motion until reload the contents
[Is this code covered by automated tests?]: Yes, it's in bug 1486971
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: This should be uplifted with bug 1486971
[Is the change risky?]: Very very low (I'd say there is no risk)
[Why is the change risky/not risky?]: This patch just flushes styles in sub frames when it's needed
[String changes made/needed]: None

Comment 8

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9006428 [details]
Bug 1478212 - Propagate MediaFeatureChangeReason::SystemMetricsChange into sub frames. r=emilio

Uplift approved for 63 beta 8, thanks.

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/releases/mozilla-beta/rev/20990ad02e18

Comment 10

[Anca Soncutean, Desktop QA]

I reproduced this issue on Beta 63.0b5 (20180910132416) across all platform.
The issue is now fixed on Beta 63.0b11 (20181001131022) and Nightly 64.0a1 (2018-10-03) under Windows 10 x64, macOS 10.13 and Ubuntu 16.04 x64.