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

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[:Gijs (he/him)]

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?

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[Jim Mathies [:jimm]]

Sorry for the delay, looking at this this week.

Comment 3

[Jim Mathies [:jimm]]

Attachment: patch

Still need to look into tests for this.

Comment 4

[Jim Mathies [:jimm]]

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.

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 8858003 [details] [diff] [review]
patch

Mike, I think you put this cache together for the e10s project. Can you review?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8858003 [details] [diff] [review]
patch

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

Sorry for the delay! Thanks!

Comment 7

[Jim Mathies [:jimm]]

Thanks!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Phil Ringnalda (:philor)]

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

Comment 10

[Jim Mathies [:jimm]]

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

Comment 11

[:Gijs (he/him)]

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

[Jim Mathies [:jimm]]

Attachment: test enabled patch

try runs look good:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f027ed6c262a759aeb6bff1b0a6b7c6cd7f0d9f

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f6dca8db6dae0ab38671152ad2c354186a968fe

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

sweeet

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/991b591e9bbf
https://hg.mozilla.org/mozilla-central/rev/85a4387ca0f8