1352773 - After setting different layout.css.devPixelsPerPx, push notifications aren't displayed at the lower right corner of the screen

Status: RESOLVED FIXED

(Toolkit Graveyard :: Notifications and Alerts, defect)

Description

[684sigma]

During the testing of Bug 1352764, Bug 1352765 and Bug 1352769, I detected a problem in Nightly 55 (2017-04-01). It didn't happen in Nightly (2017-03-20).
Push notifications and notifications from Video Download Helper extension don't appear exactly at the lower right corner of the screen. Here's how to reproduce the bug:

1. Open about:config, set the pref layout.css.devPixelsPerPx to 0.9
2. Open https://nickersoft.github.io/push.js/ , click on the button "Demo" on the page
3. Open about:config, set the pref layout.css.devPixelsPerPx to 1.6
4. Open https://nickersoft.github.io/push.js/ , click on the button "Demo" on the page

Result: when the pref is set to 0.9, push notification appears further from the upper left corner of the screen. when the pref is set to 1.6, push notification appears closer to the upper left corner of the screen.

Expected: Push notification should always appear at the lower right corner of the screen.

Comment 1

[Loic]

I can't reproduce it with screen resolution at 100% or 125% on 1080p display. Could you attach a screenshot of the issue.

Comment 2

[Loic]

Nevermind, I'm able to reproduce it with an old profile and Nightly. With a fresh profile, I'm hitting bug 1352772.

Comment 3

[Loic]

It regressed by bug 1194751:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9a714b38cd55604a5aff97c7e9a3fcdf41338f1e&tochange=7d3e8986a676a085a65e8bf0e060b66b8c344cd2

A fix about the notification tooltip being offset has been pushed in bug 1351329, but it's incomplete with a non-default value for layout.css.devPixelsPerPx.

Comment 4

[Masatoshi Kimura [:emk]]

That's strange... After bug 1351329, the defaultCSSScale calculation should be exactly the same as before bug 1194751.

Comment 5

[Masatoshi Kimura [:emk]]

Hm, defaultCSSScale is 1.0 (system scale) regardless of layout.css.devPixelsPerPx setting on the latest Nightly. DefaultScaleOverride does not work somehow...

Comment 6

[Masatoshi Kimura [:emk]]

After bug 1351329, the screen list will be initialized very early in the startup process. So per-profile layout.css.devPixelsPerPx setting will not affect defaultCSSScaleFactor. If I set the pref globally (via defaults/pref), desktop notifications are positioned correctly.

We need to refresh the screen list when layout.css.devPixelsPerPx is changed.

Comment 7

[Masatoshi Kimura [:emk]]

(In reply to Masatoshi Kimura [:emk] from comment #6)
> After bug 1351329, the screen list will be initialized very early in the

After bug 1194751

Comment 8

[Masatoshi Kimura [:emk]]

Attachment: Bug 1352773 - Take into account DefaultScaleOverride in Screen::GetDefaultCSSScaleFactor.

Review commit: https://reviewboard.mozilla.org/r/126346/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126346/

Comment 9

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8854397 [details]
Bug 1352773 - Take into account DefaultScaleOverride in Screen::GetDefaultCSSScaleFactor.

https://reviewboard.mozilla.org/r/126346/#review128882

::: layout/base/nsPresContext.cpp:694
(Diff revision 1)
>  void
>  nsPresContext::PreferenceChanged(const char* aPrefName)
>  {
>    nsDependentCString prefName(aPrefName);
>    if (prefName.EqualsLiteral("layout.css.dpi") ||
>        prefName.EqualsLiteral("layout.css.devPixelsPerPx")) {
> +    ScreenManager::GetSingleton().RefreshScreens();

I'd like to keep the ScreenManager interface minimum and restricted to widget level if possiblle.

Could you change Screen::GetDefaultCSSScaleFactor to use nsIWidget::DefaultScaleOverride() as override? I can't find where does cocoa use this pref but I assume the pref applies to all platforms.

Also I noticed that nsIWidget::DefaultScaleOverride() is used on some hot paths so it should be updated to use pref cache and probably cache the parsing result.

Thanks for fixing this!

Comment 10

[Marcia Knous [:marcia]]

Tracking 55+ for this regression.

Comment 11

[Masatoshi Kimura [:emk]]

Comment on attachment 8854397 [details]
Bug 1352773 - Take into account DefaultScaleOverride in Screen::GetDefaultCSSScaleFactor.

https://reviewboard.mozilla.org/r/126346/#review128882

> I'd like to keep the ScreenManager interface minimum and restricted to widget level if possiblle.
> 
> Could you change Screen::GetDefaultCSSScaleFactor to use nsIWidget::DefaultScaleOverride() as override? I can't find where does cocoa use this pref but I assume the pref applies to all platforms.
> 
> Also I noticed that nsIWidget::DefaultScaleOverride() is used on some hot paths so it should be updated to use pref cache and probably cache the parsing result.
> 
> Thanks for fixing this!

I tried caching, but it did not work. Probably because I could not guarantee that the pref callback for "layout.css.devPixelsPerPx" is called early enough. It would not be a good idea to change Screen::GetDefaultCSSScaleFactor so that it uses nsIWidget::DefaultScaleOverride() without caching.

Comment 12

[Masatoshi Kimura [:emk]]

Attachment: aa0b3009c855

Failed attempt for the record.

Comment 13

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Masatoshi Kimura [:emk] from comment #11)
> Comment on attachment 8854397 [details]
> Bug 1352773 - Refresh the screen list when the "layout.css.devPixelsPerPx"
> pref is changed.
> 
> https://reviewboard.mozilla.org/r/126346/#review128882
> 
> > I'd like to keep the ScreenManager interface minimum and restricted to widget level if possiblle.
> > 
> > Could you change Screen::GetDefaultCSSScaleFactor to use nsIWidget::DefaultScaleOverride() as override? I can't find where does cocoa use this pref but I assume the pref applies to all platforms.
> > 
> > Also I noticed that nsIWidget::DefaultScaleOverride() is used on some hot paths so it should be updated to use pref cache and probably cache the parsing result.
> > 
> > Thanks for fixing this!
> 
> I tried caching, but it did not work. Probably because I could not guarantee
> that the pref callback for "layout.css.devPixelsPerPx" is called early
> enough. It would not be a good idea to change
> Screen::GetDefaultCSSScaleFactor so that it uses
> nsIWidget::DefaultScaleOverride() without caching.

Did you try Preferences::AddFloatVarCache? You can use that without a callback.

Comment 14

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8854397 [details]
Bug 1352773 - Take into account DefaultScaleOverride in Screen::GetDefaultCSSScaleFactor.

https://reviewboard.mozilla.org/r/126346/#review129024

::: layout/base/nsPresContext.cpp:694
(Diff revision 1)
>  void
>  nsPresContext::PreferenceChanged(const char* aPrefName)
>  {
>    nsDependentCString prefName(aPrefName);
>    if (prefName.EqualsLiteral("layout.css.dpi") ||
>        prefName.EqualsLiteral("layout.css.devPixelsPerPx")) {
> +    ScreenManager::GetSingleton().RefreshScreens();

Please add a comment here to explain why we need to refresh screens here.

Comment 15

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Masatoshi Kimura [:emk] from comment #11)
> I tried caching, but it did not work. Probably because I could not guarantee
> that the pref callback for "layout.css.devPixelsPerPx" is called early
> enough. It would not be a good idea to change
> Screen::GetDefaultCSSScaleFactor so that it uses
> nsIWidget::DefaultScaleOverride() without caching.

Variable caching also use the observer internally so the order of cache update and observer callback is not guaranteed, which is sad.

I think we still need to fix nsIWidget::DefaultScaleOverride() to use caches to avoid read preferences in hot paths.

Comment 16

[Masatoshi Kimura [:emk]]

(In reply to Kan-Ru Chen [:kanru] (UTC-4, EDT) from comment #13)
> Did you try Preferences::AddFloatVarCache? You can use that without a
> callback.

I didn't know about Preferences::AddFloatVarCache! But it did not work either. It is using a callback internally.

Comment 17

[Masatoshi Kimura [:emk]]

Comment on attachment 8854397 [details]
Bug 1352773 - Take into account DefaultScaleOverride in Screen::GetDefaultCSSScaleFactor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126346/diff/1-2/

Comment 18

[Masatoshi Kimura [:emk]]

Comment on attachment 8854397 [details]
Bug 1352773 - Take into account DefaultScaleOverride in Screen::GetDefaultCSSScaleFactor.

Bug 1353493 made this patch much simpler.

Comment 19

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8854397 [details]
Bug 1352773 - Take into account DefaultScaleOverride in Screen::GetDefaultCSSScaleFactor.

https://reviewboard.mozilla.org/r/126346/#review129470

Comment 20

[Kan-Ru Chen [:kanru] (UTC+9)]

Thanks again for fixing this!

Comment 21

[Masatoshi Kimura [:emk]]

Comment on attachment 8854397 [details]
Bug 1352773 - Take into account DefaultScaleOverride in Screen::GetDefaultCSSScaleFactor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126346/diff/2-3/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68a13da9b6f2
Take into account DefaultScaleOverride in Screen::GetDefaultCSSScaleFactor. r=kanru

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/68a13da9b6f2

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/68a13da9b6f2