Closed
Bug 1356218
Opened 7 years ago
Closed 7 years ago
screen.colorDepth always returns primary monitor's color depth
Categories
(Core :: Widget: Win32, defect)
Tracking
()
RESOLVED
FIXED
mozilla55
| Tracking | Status | |
|---|---|---|
| firefox55 | --- | fixed |
People
(Reporter: emk, Unassigned)
References
Details
Attachments
(2 files)
Steps to reproduce: 1. Setup a multimonitor system. 2. Set primary monitor's color depth to 32bpp. 3. Set secondary monitor's color depth to 16bpp. 4. Launch Firefox and open an window on the secondary monitor. 5. Get screen.colorDepth (for example using web console). Actual result: 32 Expected result: 16
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
Reporter | |
Comment 3•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8857917 [details] Bug 1356218 - Make screen.colorDepth multi-monitor aware. https://reviewboard.mozilla.org/r/129948/#review132530 Hm, this patch did not work somehow. Investigating...
|
|
Reporter | |
Updated•7 years ago
|
Attachment #8857917 -
Flags: review?(jmathies)
|
|
Reporter | |
Comment 4•7 years ago
|
||
Oh, DOM screen object always uses the color depth from the primary screen: https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/dom/base/nsScreen.cpp#64-82 https://dxr.mozilla.org/mozilla-central/rev/f40e24f40b4c4556944c762d4764eace261297f5/gfx/src/nsDeviceContext.cpp#399-410 nsIScreen worked correctly with the patch.
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
||
Comment 7•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8858275 [details] Bug 1356218 - Fix nsDeviceContext::GetDepth to use the information from the correct monitor. https://reviewboard.mozilla.org/r/130262/#review132990 ::: gfx/src/nsDeviceContext.cpp:402 (Diff revision 1) > - if (mDepth == 0 && mScreenManager) { > - nsCOMPtr<nsIScreen> primaryScreen; > - mScreenManager->GetPrimaryScreen(getter_AddRefs(primaryScreen)); > - primaryScreen->GetColorDepth(reinterpret_cast<int32_t *>(&mDepth)); > + nsCOMPtr<nsIScreen> screen; > + FindScreen(getter_AddRefs(screen)); > + if (screen) { > + screen->GetColorDepth(reinterpret_cast<int32_t *>(&aDepth)); > } What should we do if FindScreen fails to return a screen here? (Maybe the most correct would be to return NOT_AVAILABLE, and check callers to confirm they will handle failure in a reasonable way.)
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Reporter | |
Comment 10•7 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8858275 [details] Bug 1356218 - Fix nsDeviceContext::GetDepth to use the information from the correct monitor. https://reviewboard.mozilla.org/r/130262/#review132990 > What should we do if FindScreen fails to return a screen here? (Maybe the most correct would be to return NOT_AVAILABLE, and check callers to confirm they will handle failure in a reasonable way.) I made GetDepth infallible. (GetPrimaryScreen will never fail, even in xpcshell or headless)
|
|
||
Comment 11•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8858275 [details] Bug 1356218 - Fix nsDeviceContext::GetDepth to use the information from the correct monitor. https://reviewboard.mozilla.org/r/130262/#review133166
Attachment #8858275 -
Flags: review?(jfkthame) → review+
|
||
Comment 12•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8857917 [details] Bug 1356218 - Make screen.colorDepth multi-monitor aware. https://reviewboard.mozilla.org/r/129950/#review133360 ::: widget/windows/ScreenHelperWin.cpp:73 (Diff revision 4) > ScreenHelperWin::RefreshScreens() > { > MOZ_LOG(sScreenLog, LogLevel::Debug, ("Refreshing screens")); > > AutoTArray<RefPtr<Screen>, 4> screens; > - BOOL result = ::EnumDisplayMonitors(nullptr, nullptr, > + HDC hdc = ::CreateDC(L"DISPLAY", nullptr, nullptr, nullptr); If this fails, is anything going to break here?
Attachment #8857917 -
Flags: review?(jmathies) → review+
| Comment hidden (mozreview-request) |
| Comment hidden (mozreview-request) |
|
|
Reporter | |
Comment 15•7 years ago
|
||
| mozreview-review | ||
Comment on attachment 8857917 [details] Bug 1356218 - Make screen.colorDepth multi-monitor aware. https://reviewboard.mozilla.org/r/129950/#review133382 ::: widget/windows/ScreenHelperWin.cpp:73 (Diff revision 4) > ScreenHelperWin::RefreshScreens() > { > MOZ_LOG(sScreenLog, LogLevel::Debug, ("Refreshing screens")); > > AutoTArray<RefPtr<Screen>, 4> screens; > - BOOL result = ::EnumDisplayMonitors(nullptr, nullptr, > + HDC hdc = ::CreateDC(L"DISPLAY", nullptr, nullptr, nullptr); If by any chance CreateDC fails, it will return NULL. Then CollectMonitors will receive NULL hDCScreen. Then GetDeviceCaps will fail. Then pixelDepth will be zero. I moved NS_ASSERTION here. But I don't think we need additional checks for the edge case.
|
||
Comment 16•7 years ago
|
||
Pushed by VYV03354@nifty.ne.jp: https://hg.mozilla.org/integration/autoland/rev/33856a082922 Make screen.colorDepth multi-monitor aware. r=jimm https://hg.mozilla.org/integration/autoland/rev/de2c3726bd36 Fix nsDeviceContext::GetDepth to use the information from the correct monitor. r=jfkthame
|
||
Comment 17•7 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/33856a082922 https://hg.mozilla.org/mozilla-central/rev/de2c3726bd36
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•