Closed Bug 1478212 Opened Last year Closed Last year

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox63 --- verified
firefox64 --- verified

People

(Reporter: hiro, Assigned: hiro)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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
Summary: RefreshSystemMetrics() doesn't propagate media feature changes in subframes- → RefreshSystemMetrics() doesn't propagate media feature changes in subframes
Blocks: 1480662
(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.
Blocks: 1486971
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.
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Comment on attachment 9006428 [details]
Bug 1478212 - Propagate MediaFeatureChangeReason::SystemMetricsChange into sub frames. r=emilio

Emilio Cobos Álvarez (:emilio) has approved the revision.
Attachment #9006428 - Flags: review+
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d9298e2f8195
Propagate MediaFeatureChangeReason::SystemMetricsChange into sub frames. r=emilio
https://hg.mozilla.org/mozilla-central/rev/d9298e2f8195
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Blocks: 1492284
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
Attachment #9006428 - Flags: approval-mozilla-beta?
Comment on attachment 9006428 [details]
Bug 1478212 - Propagate MediaFeatureChangeReason::SystemMetricsChange into sub frames. r=emilio

Uplift approved for 63 beta 8, thanks.
Attachment #9006428 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.