Windows media queries and system metrics are broken in e10s content processes

RESOLVED FIXED in Firefox 55

Status

()

Core
Widget: Win32
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Gijs, Assigned: jimm)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

a year ago
STR:

1. on Windows 10, using a non-high-contrast theme, evaluate this in the web console:

matchMedia("(-moz-windows-default-theme)")

ER:
an object with matches: true. (Compare evaluating the same thing in the browser console)

AR:
an object with matches: false.

This breaks video controls CSS and possibly other in-content styling we'd like to do.

On OS X, matchMedia("(-moz-mac-yosemite-theme)") returns the correct result in the content process, so I assume this is a Windows issue. Not sure if this is layout or widget brokenness, but either way.

Dan, do you know who could look into fixing this?
Flags: needinfo?(dholbert)
(Reporter)

Updated

a year ago
See Also: → bug 1350325
Since this is only an issue on Windows, it's very likely a Windows-widget issue. The layout/style code for system media queries just calls out to abstract platform-specific LookAndFeel::GetInt() code, here (highlighting the chunks for the metrics you mentioned):
https://dxr.mozilla.org/mozilla-central/rev/272ce6c2572164f5f6a9fba2a980ba9ccf50770c/layout/style/nsCSSRuleProcessor.cpp#1114-1118,1125-1128


That ^^ goes through the windows nsLookAndFeel.cpp switch statement, which calls into a getter for this bool:
  nsUXThemeData::sIsDefaultWindowsTheme
...which is set in nsUXThemeData::UpdateNativeThemeInfo()
https://dxr.mozilla.org/mozilla-central/rev/272ce6c2572164f5f6a9fba2a980ba9ccf50770c/widget/windows/nsUXThemeData.cpp#304

So my initial guess is that something is horked in that function (though it could also be correct and we're dropping its value on the floor somewhere).

Looks like jimm has "hg blame" on some of the UpdateNativeThemeInfo() code, so I'll punt to him.
Component: Layout → Widget: Win32
Flags: needinfo?(dholbert) → needinfo?(jmathies)
(Assignee)

Comment 2

a year ago
Sorry for the delay, looking at this this week.
(Assignee)

Comment 3

a year ago
Created attachment 8858003 [details] [diff] [review]
patch

Still need to look into tests for this.
Assignee: nobody → jmathies
Flags: needinfo?(jmathies)
(Assignee)

Comment 4

a year ago
Looked into tests, I think I'll punt. The theme id value is largely obsolete now since everything post Vista is aero except when you're running an accessible theme, which I can't tweak on slaves. Everything is a default theme on our slaves so that value never changes either.
(Assignee)

Comment 5

a year ago
Comment on attachment 8858003 [details] [diff] [review]
patch

Mike, I think you put this cache together for the e10s project. Can you review?
Attachment #8858003 - Flags: review?(mconley)
Comment on attachment 8858003 [details] [diff] [review]
patch

Review of attachment 8858003 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay! Thanks!
Attachment #8858003 - Flags: review?(mconley) → review+
(Assignee)

Comment 7

a year ago
Thanks!
Keywords: checkin-needed

Comment 8

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8ef6055e88f
Add Windows theme related look and feel values to the content process look and feel cache. r=mconley
Keywords: checkin-needed
(Assignee)

Comment 10

a year ago
(In reply to Phil Ringnalda (:philor) from comment #9)
> This is bound to be fun: backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c148d5f3d36c for Win8
> reftest bustage,
> https://treeherder.mozilla.org/logviewer.html#?job_id=92529022&repo=mozilla-
> inbound

Interesting, these are TEST-UNEXPECTED-PASS. I think this patch may have simply fixed bug 1254832.
Blocks: 1254832
(Reporter)

Comment 11

a year ago
(In reply to Jim Mathies [:jimm] from comment #10)
> (In reply to Phil Ringnalda (:philor) from comment #9)
> > This is bound to be fun: backed out in
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/c148d5f3d36c for Win8
> > reftest bustage,
> > https://treeherder.mozilla.org/logviewer.html#?job_id=92529022&repo=mozilla-
> > inbound
> 
> Interesting, these are TEST-UNEXPECTED-PASS. I think this patch may have
> simply fixed bug 1254832.

\o/
Comment on attachment 8860148 [details] [diff] [review]
test enabled patch

sweeet
Attachment #8860148 - Flags: review?(ryanvm) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 14

a year ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/991b591e9bbf
Add Windows theme related look and feel values to the content process look and feel cache. r=mconley
https://hg.mozilla.org/integration/mozilla-inbound/rev/85a4387ca0f8
Enable a reftest this work fixes. r=RyanVM
Keywords: checkin-needed

Comment 15

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/991b591e9bbf
https://hg.mozilla.org/mozilla-central/rev/85a4387ca0f8
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.