linux_aura: Enable HIDPI support

linux_aura: Enable HIDPI support

Do the preparation work for enabling HIDPI support on linux aura: enable DIP and calculate device scale factor at DesktopScreenX11.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235967

[Mikhail, 2013-10-14 07:48:42]

Could you please have a look?

[kenneth.r.christiansen, 2013-10-14 08:20:54]

https://codereview.chromium.org/27156003/diff/1/ui/views/widget/desktop_aura/...
File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):

https://codereview.chromium.org/27156003/diff/1/ui/views/widget/desktop_aura/...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:34: const int
kCSSPixelsPerInch = 96;
kDefaultCSSPixelsPerInch ?

https://codereview.chromium.org/27156003/diff/1/ui/views/widget/desktop_aura/...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:50: gfx::Rect
bounds_in_pixels(0, 0, width, height);
device pixels? or css pixels? not obvious

https://codereview.chromium.org/27156003/diff/1/ui/views/widget/desktop_aura/...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:51: gfx::Display
gfx_display(0, bounds_in_pixels);
Why are we assuming screen 0 here?

[Mikhail, 2013-10-14 09:15:52]

https://codereview.chromium.org/27156003/diff/1/ui/views/widget/desktop_aura/...
File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):

https://codereview.chromium.org/27156003/diff/1/ui/views/widget/desktop_aura/...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:34: const int
kCSSPixelsPerInch = 96;
On 2013/10/14 08:20:54, kenneth.r.christiansen wrote:
> kDefaultCSSPixelsPerInch ?

Yeah, I would even say kDefaultDIPPerInch to be more aligned with the rest of
the code.

https://codereview.chromium.org/27156003/diff/1/ui/views/widget/desktop_aura/...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:50: gfx::Rect
bounds_in_pixels(0, 0, width, height);
On 2013/10/14 08:20:54, kenneth.r.christiansen wrote:
> device pixels? or css pixels? not obvious
Device pixels. From what I saw postfix "_in_pixels" always means device pixels
within the chromium codebase.

https://codereview.chromium.org/27156003/diff/1/ui/views/widget/desktop_aura/...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:51: gfx::Display
gfx_display(0, bounds_in_pixels);
On 2013/10/14 08:20:54, kenneth.r.christiansen wrote:
> Why are we assuming screen 0 here?

Guess, this is just a fallback display number. Anyway this is not something
introduced by the patch.

[Elliot Glaysher, 2013-10-14 20:03:49]

Redirecting to oshima because I don't know anything about DPI.

[oshima, 2013-10-14 20:21:59]

I'm OOO today, so it'll be tomorrow. That's being said,
I'd like to know how you're planning to support high DPI
on linux. Is there a design doc? If not, can you write one?
It doesn't have to be big or complete. Just want to know
the direction you're heading.

[Mikhail, 2013-10-15 14:56:31]

On 2013/10/14 20:21:59, oshima wrote:
> I'm OOO today, so it'll be tomorrow. That's being said,
> I'd like to know how you're planning to support high DPI
> on linux. Is there a design doc? If not, can you write one?
> It doesn't have to be big or complete. Just want to know
> the direction you're heading.

I'm just going to re-use the existing design: basically do the same as for Aura
Windows desktop.

[oshima, 2013-10-15 18:12:32]

Ok, so here is high level comments.

1) Xlib's screen no longer corresponds to display. You need to use Xrandr to get
display info.

2) Win_Aura does not support per display scale factor right now. (There is a
plan, but not implemented). Using different scale factor would cause problem in
current implementation. (see ui/gfx/screen_win.cc)

3) DPI info may not be available or can be bogus. You need a fallback mechanism
(see ash/display/display_change_observer_chromeos.cc)

4) Which scale factors are you planning to support? On Windows, we support 1.0,
1.4 and 1.8. If you want to do the same you need to change
ui/base/resources/resource_bundle.cc as well.

[Mikhail, 2013-10-16 06:36:06]

On 2013/10/15 18:12:32, oshima wrote:
> Ok, so here is high level comments.
> 
> 1) Xlib's screen no longer corresponds to display. You need to use Xrandr to
get
> display info.
> 
> 2) Win_Aura does not support per display scale factor right now. (There is a
> plan, but not implemented). Using different scale factor would cause problem
in
> current implementation. (see ui/gfx/screen_win.cc)
> 
> 3) DPI info may not be available or can be bogus. You need a fallback
mechanism
> (see ash/display/display_change_observer_chromeos.cc)
> 
> 4) Which scale factors are you planning to support? On Windows, we support
1.0,
> 1.4 and 1.8. If you want to do the same you need to change
> ui/base/resources/resource_bundle.cc as well.

Thank you for your comments! I will update the patch accordingly.

[Mikhail, 2013-10-18 15:25:44]

Oshima, could you please have a look at the updated version of patch?

[oshima, 2013-10-22 05:35:09]

https://codereview.chromium.org/27156003/diff/20001/ui/base/resource/resource...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/27156003/diff/20001/ui/base/resource/resource...
ui/base/resource/resource_bundle.cc:579:
supported_scale_factors.push_back(SCALE_FACTOR_180P);
FYI: These scale factors will use 200P assets with image scaling.

By the way, have to discuss this with anyone in UX team?
Certain part of UI uses different assets between Windows and Linux
and you need to make sure UX team is aware of it and produces the
right assets for linux high DPI as well.

https://codereview.chromium.org/27156003/diff/20001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):

https://codereview.chromium.org/27156003/diff/20001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:65: DCHECK_NE(0.0f, scale);
scale must be larger than 1.0f

DCHECK_LE(1.0f, scale)

any this is separate function? can't you just set g_device_scale_factor below?

https://codereview.chromium.org/27156003/diff/20001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:69: void
InitDeviceScaleFactorFromScreenDim(int screen_dim, int mm_screen_dim) {
....ScreenDimension

https://codereview.chromium.org/27156003/diff/20001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:80: DCHECK_NE(0.0f,
g_device_scale_factor);
ditto. nuke "inline"

https://codereview.chromium.org/27156003/diff/20001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:384: &&
!ShouldIgnoreScreen(output_info->mm_width, output_info->mm_height)) {
if (i == 0 &&
    !....

https://codereview.chromium.org/27156003/diff/20001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:385: // As Aura does not
support per display scale factor right now,
desktop aura does not support ...

I believe this limitation comes form OS, not aura.

https://codereview.chromium.org/27156003/diff/20001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:387:
InitDeviceScaleFactorFromScreenDim(crtc->width, output_info->mm_width);
indent

You may just want to pass crtc/output_info instead.

[Elliot Glaysher, 2013-10-22 17:40:17]

https://codereview.chromium.org/27156003/diff/20001/ui/base/resource/resource...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/27156003/diff/20001/ui/base/resource/resource...
ui/base/resource/resource_bundle.cc:579:
supported_scale_factors.push_back(SCALE_FACTOR_180P);
On 2013/10/22 05:35:10, oshima wrote:
> FYI: These scale factors will use 200P assets with image scaling.
> 
> By the way, have to discuss this with anyone in UX team?
> Certain part of UI uses different assets between Windows and Linux
> and you need to make sure UX team is aware of it and produces the
> right assets for linux high DPI as well.

IIRC, there are only a few assets that linux_aura uses that windows doesn't,
like the linux min/max/close buttons. You can find the 100P assets here:
ui/resources/default_100_percent/linux. I notice that there doesn't appear to be
200P versions of all those.

[Mikhail, 2013-10-23 17:15:01]

Thanks a lot for having a look!

https://codereview.chromium.org/27156003/diff/20001/ui/base/resource/resource...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/27156003/diff/20001/ui/base/resource/resource...
ui/base/resource/resource_bundle.cc:579:
supported_scale_factors.push_back(SCALE_FACTOR_180P);
On 2013/10/22 17:40:18, Elliot Glaysher wrote:
> On 2013/10/22 05:35:10, oshima wrote:
> > FYI: These scale factors will use 200P assets with image scaling.
> > 
> > By the way, have to discuss this with anyone in UX team?
> > Certain part of UI uses different assets between Windows and Linux
> > and you need to make sure UX team is aware of it and produces the
> > right assets for linux high DPI as well.
> 
> IIRC, there are only a few assets that linux_aura uses that windows doesn't,
> like the linux min/max/close buttons. You can find the 100P assets here:
> ui/resources/default_100_percent/linux. I notice that there doesn't appear to
be
> 200P versions of all those.

Should it be a stopper for this CL?

https://codereview.chromium.org/27156003/diff/20001/ui/views/widget/desktop_a...
File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):

https://codereview.chromium.org/27156003/diff/20001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:65: DCHECK_NE(0.0f, scale);
On 2013/10/22 05:35:10, oshima wrote:
> scale must be larger than 1.0f
> 
> DCHECK_LE(1.0f, scale)
> 
> any this is separate function? can't you just set g_device_scale_factor below?
I've done it just to enforce DCHECK, but we surely can do without these
functions

https://codereview.chromium.org/27156003/diff/20001/ui/views/widget/desktop_a...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:387:
InitDeviceScaleFactorFromScreenDim(crtc->width, output_info->mm_width);
On 2013/10/22 05:35:10, oshima wrote:
> indent
> 
> You may just want to pass crtc/output_info instead.
This function is also used from GetFallbackDisplayList, so I decided to make it
xrandr-independent

[Elliot Glaysher, 2013-10-23 17:51:19]

https://codereview.chromium.org/27156003/diff/20001/ui/base/resource/resource...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/27156003/diff/20001/ui/base/resource/resource...
ui/base/resource/resource_bundle.cc:579:
supported_scale_factors.push_back(SCALE_FACTOR_180P);
On 2013/10/23 17:15:02, mikhail.pozdnyakov wrote:
> On 2013/10/22 17:40:18, Elliot Glaysher wrote:
> > On 2013/10/22 05:35:10, oshima wrote:
> > > FYI: These scale factors will use 200P assets with image scaling.
> > > 
> > > By the way, have to discuss this with anyone in UX team?
> > > Certain part of UI uses different assets between Windows and Linux
> > > and you need to make sure UX team is aware of it and produces the
> > > right assets for linux high DPI as well.
> > 
> > IIRC, there are only a few assets that linux_aura uses that windows doesn't,
> > like the linux min/max/close buttons. You can find the 100P assets here:
> > ui/resources/default_100_percent/linux. I notice that there doesn't appear
to
> be
> > 200P versions of all those.
> 
> Should it be a stopper for this CL?

It shouldn't block the patch, but it should (possibly) block eventual release
for whatever you're doing. Upscaled resources look pretty bad, and I cringe when
I see obviously upscaled resources on my Pixel.

Make sure to file a bug to track this. CC it to me and I'll try to get the
designers to add 200P resources here.

https://codereview.chromium.org/27156003/diff/160001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):

https://codereview.chromium.org/27156003/diff/160001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:85: &&
!ShouldIgnoreScreen(mm_width, mm_height)) {
&& on previous line.

[oshima, 2013-10-23 18:10:58]

https://codereview.chromium.org/27156003/diff/20001/ui/base/resource/resource...
File ui/base/resource/resource_bundle.cc (right):

https://codereview.chromium.org/27156003/diff/20001/ui/base/resource/resource...
ui/base/resource/resource_bundle.cc:579:
supported_scale_factors.push_back(SCALE_FACTOR_180P);
On 2013/10/23 17:51:19, Elliot Glaysher wrote:
> On 2013/10/23 17:15:02, mikhail.pozdnyakov wrote:
> > On 2013/10/22 17:40:18, Elliot Glaysher wrote:
> > > On 2013/10/22 05:35:10, oshima wrote:
> > > > FYI: These scale factors will use 200P assets with image scaling.
> > > > 
> > > > By the way, have to discuss this with anyone in UX team?
> > > > Certain part of UI uses different assets between Windows and Linux
> > > > and you need to make sure UX team is aware of it and produces the
> > > > right assets for linux high DPI as well.
> > > 
> > > IIRC, there are only a few assets that linux_aura uses that windows
doesn't,
> > > like the linux min/max/close buttons. You can find the 100P assets here:
> > > ui/resources/default_100_percent/linux. I notice that there doesn't appear
> to
> > be
> > > 200P versions of all those.
> > 
> > Should it be a stopper for this CL?
> 
> It shouldn't block the patch, but it should (possibly) block eventual release
> for whatever you're doing. Upscaled resources look pretty bad, and I cringe
when
> I see obviously upscaled resources on my Pixel.
> 
> Make sure to file a bug to track this. CC it to me and I'll try to get the
> designers to add 200P resources here.

sg

https://codereview.chromium.org/27156003/diff/160001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):

https://codereview.chromium.org/27156003/diff/160001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:46: bool
ShouldIgnoreScreen(unsigned long mm_width, unsigned long mm_height) {
I thought I left the comment, but apparently not, sorry.

I believe you took this from ash/display/display_change_observer_chromeos.cc.
Can you
move this utility function to ui/base/x/x11_util and use it instead of
duplicating the code?

The name should be someting like

IsDisplaySizeBlackListed

[Mikhail, 2013-10-25 10:55:30]

On 2013/10/23 18:10:58, oshima wrote:
>
https://codereview.chromium.org/27156003/diff/20001/ui/base/resource/resource...
> File ui/base/resource/resource_bundle.cc (right):
> 
>
https://codereview.chromium.org/27156003/diff/20001/ui/base/resource/resource...
> ui/base/resource/resource_bundle.cc:579:
> supported_scale_factors.push_back(SCALE_FACTOR_180P);
> On 2013/10/23 17:51:19, Elliot Glaysher wrote:
> > On 2013/10/23 17:15:02, mikhail.pozdnyakov wrote:
> > > On 2013/10/22 17:40:18, Elliot Glaysher wrote:
> > > > On 2013/10/22 05:35:10, oshima wrote:
> > > > > FYI: These scale factors will use 200P assets with image scaling.
> > > > > 
> > > > > By the way, have to discuss this with anyone in UX team?
> > > > > Certain part of UI uses different assets between Windows and Linux
> > > > > and you need to make sure UX team is aware of it and produces the
> > > > > right assets for linux high DPI as well.
> > > > 
> > > > IIRC, there are only a few assets that linux_aura uses that windows
> doesn't,
> > > > like the linux min/max/close buttons. You can find the 100P assets here:
> > > > ui/resources/default_100_percent/linux. I notice that there doesn't
appear
> > to
> > > be
> > > > 200P versions of all those.
> > > 
> > > Should it be a stopper for this CL?
> > 
> > It shouldn't block the patch, but it should (possibly) block eventual
release
> > for whatever you're doing. Upscaled resources look pretty bad, and I cringe
> when
> > I see obviously upscaled resources on my Pixel.
> > 
> > Make sure to file a bug to track this. CC it to me and I'll try to get the
> > designers to add 200P resources here.
> 
> sg
> 
>
https://codereview.chromium.org/27156003/diff/160001/ui/views/widget/desktop_...
> File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):
> 
>
https://codereview.chromium.org/27156003/diff/160001/ui/views/widget/desktop_...
> ui/views/widget/desktop_aura/desktop_screen_x11.cc:46: bool
> ShouldIgnoreScreen(unsigned long mm_width, unsigned long mm_height) {
> I thought I left the comment, but apparently not, sorry.
> 
> I believe you took this from ash/display/display_change_observer_chromeos.cc.
> Can you
> move this utility function to ui/base/x/x11_util and use it instead of
> duplicating the code?
> 
> The name should be someting like
> 
> IsDisplaySizeBlackListed

I think this proposal is great, implemented in the patch set #5, thanks.

[Mikhail, 2013-10-28 19:22:12]

On 2013/10/25 10:55:30, mikhail.pozdnyakov wrote:
> On 2013/10/23 18:10:58, oshima wrote:
> >
>
https://codereview.chromium.org/27156003/diff/20001/ui/base/resource/resource...
> > File ui/base/resource/resource_bundle.cc (right):
> > 
> >
>
https://codereview.chromium.org/27156003/diff/20001/ui/base/resource/resource...
> > ui/base/resource/resource_bundle.cc:579:
> > supported_scale_factors.push_back(SCALE_FACTOR_180P);
> > On 2013/10/23 17:51:19, Elliot Glaysher wrote:
> > > On 2013/10/23 17:15:02, mikhail.pozdnyakov wrote:
> > > > On 2013/10/22 17:40:18, Elliot Glaysher wrote:
> > > > > On 2013/10/22 05:35:10, oshima wrote:
> > > > > > FYI: These scale factors will use 200P assets with image scaling.
> > > > > > 
> > > > > > By the way, have to discuss this with anyone in UX team?
> > > > > > Certain part of UI uses different assets between Windows and Linux
> > > > > > and you need to make sure UX team is aware of it and produces the
> > > > > > right assets for linux high DPI as well.
> > > > > 
> > > > > IIRC, there are only a few assets that linux_aura uses that windows
> > doesn't,
> > > > > like the linux min/max/close buttons. You can find the 100P assets
here:
> > > > > ui/resources/default_100_percent/linux. I notice that there doesn't
> appear
> > > to
> > > > be
> > > > > 200P versions of all those.
> > > > 
> > > > Should it be a stopper for this CL?
> > > 
> > > It shouldn't block the patch, but it should (possibly) block eventual
> release
> > > for whatever you're doing. Upscaled resources look pretty bad, and I
cringe
> > when
> > > I see obviously upscaled resources on my Pixel.
> > > 
> > > Make sure to file a bug to track this. CC it to me and I'll try to get the
> > > designers to add 200P resources here.
> > 
> > sg
> > 
> >
>
https://codereview.chromium.org/27156003/diff/160001/ui/views/widget/desktop_...
> > File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):
> > 
> >
>
https://codereview.chromium.org/27156003/diff/160001/ui/views/widget/desktop_...
> > ui/views/widget/desktop_aura/desktop_screen_x11.cc:46: bool
> > ShouldIgnoreScreen(unsigned long mm_width, unsigned long mm_height) {
> > I thought I left the comment, but apparently not, sorry.
> > 
> > I believe you took this from
ash/display/display_change_observer_chromeos.cc.
> > Can you
> > move this utility function to ui/base/x/x11_util and use it instead of
> > duplicating the code?
> > 
> > The name should be someting like
> > 
> > IsDisplaySizeBlackListed
> 
> I think this proposal is great, implemented in the patch set #5, thanks.

any update here, please?

[oshima, 2013-10-28 21:00:28]

lgtm

[Elliot Glaysher, 2013-10-28 21:33:59]

lgtm

[commit-bot: I haz the power, 2013-10-29 08:54:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/27156003/...

[commit-bot: I haz the power, 2013-10-29 09:11:42]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Mikhail, 2013-10-29 09:42:51]

Missing LGTM from an OWNER for these files:
    ui/base/resource/resource_bundle.cc
    ui/base/x/x11_util.cc
    ui/base/x/x11_util.h

Could I kindly ask ui/base owners to have a look?

[Daniel Erat, 2013-10-29 14:28:24]

https://codereview.chromium.org/27156003/diff/250001/ash/display/display_chan...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/27156003/diff/250001/ash/display/display_chan...
ash/display/display_change_observer_chromeos.cc:51: bool
DisplayChangeObserver::ShouldIgnoreSize(unsigned long mm_width,
please remove this method and make the caller call
ui::IsXDisplaySizeBlackListed() directly.

also, please make the unit tests for this method test
IsXDisplaySizeBlackListed() instead, and move them to an x_util_unittest.cc
file.

https://codereview.chromium.org/27156003/diff/250001/ui/base/x/x11_util.cc
File ui/base/x/x11_util.cc (right):

https://codereview.chromium.org/27156003/diff/250001/ui/base/x/x11_util.cc#ne...
ui/base/x/x11_util.cc:1330: mm_height <= kInvalidDisplaySizeList[0][1]) {
nit: indent this two more spaces to line up with the 'mm' on the previous line

https://codereview.chromium.org/27156003/diff/250001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):

https://codereview.chromium.org/27156003/diff/250001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:33: float
g_device_scale_factor = 1.0f;
why does this need to be global? it looks like it gets assigned by
GetFallbackDisplayList() (which is called by BuildDisplaysFromXRandRInfo() if
the XRandR call fails) and later by BuildDisplaysFromXRandRInfo() if the first
display isn't blacklisted, but i don't see where it's being read.

https://codereview.chromium.org/27156003/diff/250001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:35: float
GetDeviceScaleFactor(int screen_dim, int mm_screen_dim) {
what does "dim" stand for here?

https://codereview.chromium.org/27156003/diff/250001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:36: const int
kCSSDefaultDIPCountPerInch = 96;
where does this number come from?

https://codereview.chromium.org/27156003/diff/250001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:39: float screen_DPI =
(screen_dim * kInchInMm) / mm_screen_dim;
nit: screen_dpi

[Mikhail, 2013-10-29 14:50:55]

https://codereview.chromium.org/27156003/diff/250001/ash/display/display_chan...
File ash/display/display_change_observer_chromeos.cc (right):

https://codereview.chromium.org/27156003/diff/250001/ash/display/display_chan...
ash/display/display_change_observer_chromeos.cc:51: bool
DisplayChangeObserver::ShouldIgnoreSize(unsigned long mm_width,
On 2013/10/29 14:28:25, Daniel Erat wrote:
> please remove this method and make the caller call
> ui::IsXDisplaySizeBlackListed() directly.
> 
> also, please make the unit tests for this method test
> IsXDisplaySizeBlackListed() instead, and move them to an x_util_unittest.cc
> file.

Actually I was planning to make it as a separate CL, but sure.

https://codereview.chromium.org/27156003/diff/250001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):

https://codereview.chromium.org/27156003/diff/250001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:33: float
g_device_scale_factor = 1.0f;
On 2013/10/29 14:28:25, Daniel Erat wrote:
> why does this need to be global? it looks like it gets assigned by
> GetFallbackDisplayList() (which is called by BuildDisplaysFromXRandRInfo() if
> the XRandR call fails) and later by BuildDisplaysFromXRandRInfo() if the first
> display isn't blacklisted, but i don't see where it's being read.

Indeed! Thanks for noticing it!

https://codereview.chromium.org/27156003/diff/250001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:35: float
GetDeviceScaleFactor(int screen_dim, int mm_screen_dim) {
On 2013/10/29 14:28:25, Daniel Erat wrote:
> what does "dim" stand for here?

dimention, probably I should put the whole word.

https://codereview.chromium.org/27156003/diff/250001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:36: const int
kCSSDefaultDIPCountPerInch = 96;
On 2013/10/29 14:28:25, Daniel Erat wrote:
> where does this number come from?
We always assume 96 CSS pixels in an inch

[Mikhail, 2013-10-29 16:23:05]

Daniel, could you please look at the updated version?

[Daniel Erat, 2013-10-29 16:27:02]

https://codereview.chromium.org/27156003/diff/400002/ui/ui_unittests.gypi
File ui/ui_unittests.gypi (right):

https://codereview.chromium.org/27156003/diff/400002/ui/ui_unittests.gypi#new...
ui/ui_unittests.gypi:295: 'base/x/x11_util_unittest.cc',
i think that the usual pattern i've seen is to list all source files in
'all_sources' above and later do exclusions via conditions. you may not need to
exclude this file at all, though, as i think that there are already broad
conditions that will exclude this file on non-X11 platforms (see
build/filename_rules.gypi).

https://codereview.chromium.org/27156003/diff/400002/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):

https://codereview.chromium.org/27156003/diff/400002/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:33: float
GetDeviceScaleFactor(int screen_dimention, int mm_screen_dimention) {
s/dimention/dimension/g

the units of the first argument are ambiguous -- if it's pixels, could you
rename these to something like "screen_pixels" and "screen_mm"?

https://codereview.chromium.org/27156003/diff/400002/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:34: const int
kCSSDefaultDIPCountPerInch = 96;
is something shorter like kCSSDefaultDPI still accurate? feel free to ignore
this suggestion if you want to keep DIP in the name.

https://codereview.chromium.org/27156003/diff/400002/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:37: float screen_dpi =
(screen_dimention * kInchInMm) / mm_screen_dimention;
maybe a bit clearer:

  float screen_inches = screen_mm / kInchInMm;
  float screen_dpi = screen_pixels / screen_inches;

[Mikhail, 2013-10-29 23:16:02]

On 2013/10/29 16:27:02, Daniel Erat wrote:
> https://codereview.chromium.org/27156003/diff/400002/ui/ui_unittests.gypi
> File ui/ui_unittests.gypi (right):
> 
>
https://codereview.chromium.org/27156003/diff/400002/ui/ui_unittests.gypi#new...
> ui/ui_unittests.gypi:295: 'base/x/x11_util_unittest.cc',
> i think that the usual pattern i've seen is to list all source files in
> 'all_sources' above and later do exclusions via conditions. you may not need
to
> exclude this file at all, though, as i think that there are already broad
> conditions that will exclude this file on non-X11 platforms (see
> build/filename_rules.gypi).
> 
>
https://codereview.chromium.org/27156003/diff/400002/ui/views/widget/desktop_...
> File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):
> 
>
https://codereview.chromium.org/27156003/diff/400002/ui/views/widget/desktop_...
> ui/views/widget/desktop_aura/desktop_screen_x11.cc:33: float
> GetDeviceScaleFactor(int screen_dimention, int mm_screen_dimention) {
> s/dimention/dimension/g
> 
> the units of the first argument are ambiguous -- if it's pixels, could you
> rename these to something like "screen_pixels" and "screen_mm"?
> 
>
https://codereview.chromium.org/27156003/diff/400002/ui/views/widget/desktop_...
> ui/views/widget/desktop_aura/desktop_screen_x11.cc:34: const int
> kCSSDefaultDIPCountPerInch = 96;
> is something shorter like kCSSDefaultDPI still accurate? feel free to ignore
> this suggestion if you want to keep DIP in the name.
> 
>
https://codereview.chromium.org/27156003/diff/400002/ui/views/widget/desktop_...
> ui/views/widget/desktop_aura/desktop_screen_x11.cc:37: float screen_dpi =
> (screen_dimention * kInchInMm) / mm_screen_dimention;
> maybe a bit clearer:
> 
>   float screen_inches = screen_mm / kInchInMm;
>   float screen_dpi = screen_pixels / screen_inches;

thanks for your comments, they are met at patch set #8

[Daniel Erat, 2013-10-29 23:31:07]

LGTM with a nit

https://codereview.chromium.org/27156003/diff/570001/ui/views/widget/desktop_...
File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):

https://codereview.chromium.org/27156003/diff/570001/ui/views/widget/desktop_...
ui/views/widget/desktop_aura/desktop_screen_x11.cc:350: output_info->mm_width);
nit: fix indenting by unindenting this two spaces

[Mikhail, 2013-10-29 23:56:07]

On 2013/10/29 23:31:07, Daniel Erat wrote:
> LGTM with a nit
> 
>
https://codereview.chromium.org/27156003/diff/570001/ui/views/widget/desktop_...
> File ui/views/widget/desktop_aura/desktop_screen_x11.cc (right):
> 
>
https://codereview.chromium.org/27156003/diff/570001/ui/views/widget/desktop_...
> ui/views/widget/desktop_aura/desktop_screen_x11.cc:350:
output_info->mm_width);
> nit: fix indenting by unindenting this two spaces

Thank you very much! Indenting is fixed now.

Tony, could you please also have a look at changes in
ui/base/resource/resource_bundle.cc ?

[tony, 2013-10-30 00:05:48]

ui/base/resource LGTM

[commit-bot: I haz the power, 2013-10-30 07:59:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/27156003/...

[commit-bot: I haz the power, 2013-10-30 11:22:16]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-10-30 11:27:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/27156003/...

[commit-bot: I haz the power, 2013-10-30 16:28:49]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-10-31 09:03:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/27156003/...

[commit-bot: I haz the power, 2013-10-31 12:45:12]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[Mikhail, 2013-11-01 18:34:07]

On 2013/10/31 12:45:12, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_aura for step(s) browser_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

Tests fail due to assertion "Check failed: g_supported_scale_factors != NULL.",
same happens if just launch chrome browser (content shell works fine!).
 [0x7f0c77b245a2] ui::GetSupportedScaleFactor()
 [0x7f0c71cf628b] (anonymous namespace)::GetDeviceScaleFactor()
 [0x7f0c71cf78c5] views::DesktopScreenX11::BuildDisplaysFromXRandRInfo()
 [0x7f0c71cf6630] views::DesktopScreenX11::DesktopScreenX11()
 [0x7f0c71cf7b00] views::CreateDesktopScreen()
 [0x7f0c7c491bbd] ChromeBrowserMainExtraPartsAura::PostMainMessageLoopStart()
 [0x7f0c7ba24122] ChromeBrowserMainParts::PostMainMessageLoopStart()
 [0x7f0c7b58839e] ChromeBrowserMainPartsPosix::PostMainMessageLoopStart()
 [0x7f0c75d420f3] content::BrowserMainLoop::MainMessageLoopStart()
 [0x7f0c75d4a46a] content::BrowserMainRunnerImpl::Initialize()
 [0x7f0c75d3f45e] content::BrowserMain()
 [0x7f0c75cfd4a7] content::RunNamedProcessTypeMain()
 [0x7f0c75cfe676] content::ContentMainRunnerImpl::Run()
 [0x7f0c75cfc937] content::ContentMain()


Looks like ResourceBundle::InitSharedInstance is not invoked for browser process
(invoked for zygote only).
Could please anyone give a hint how ResourceBundle::InitSharedInstance is
invoked on Windows (since it has the similar logic and
'g_supported_scale_factors' are initialized in the same place) ?

[oshima, 2013-11-01 23:12:29]

On 2013/11/01 18:34:07, mikhail.pozdnyakov wrote:
> On 2013/10/31 12:45:12, I haz the power (commit-bot) wrote:
> > Retried try job too often on linux_aura for step(s) browser_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
> 
> Tests fail due to assertion "Check failed: g_supported_scale_factors !=
NULL.",
> same happens if just launch chrome browser (content shell works fine!).
>  [0x7f0c77b245a2] ui::GetSupportedScaleFactor()
>  [0x7f0c71cf628b] (anonymous namespace)::GetDeviceScaleFactor()
>  [0x7f0c71cf78c5] views::DesktopScreenX11::BuildDisplaysFromXRandRInfo()
>  [0x7f0c71cf6630] views::DesktopScreenX11::DesktopScreenX11()
>  [0x7f0c71cf7b00] views::CreateDesktopScreen()
>  [0x7f0c7c491bbd] ChromeBrowserMainExtraPartsAura::PostMainMessageLoopStart()
>  [0x7f0c7ba24122] ChromeBrowserMainParts::PostMainMessageLoopStart()
>  [0x7f0c7b58839e] ChromeBrowserMainPartsPosix::PostMainMessageLoopStart()
>  [0x7f0c75d420f3] content::BrowserMainLoop::MainMessageLoopStart()
>  [0x7f0c75d4a46a] content::BrowserMainRunnerImpl::Initialize()
>  [0x7f0c75d3f45e] content::BrowserMain()
>  [0x7f0c75cfd4a7] content::RunNamedProcessTypeMain()
>  [0x7f0c75cfe676] content::ContentMainRunnerImpl::Run()
>  [0x7f0c75cfc937] content::ContentMain()
> 
> 
> Looks like ResourceBundle::InitSharedInstance is not invoked for browser
process
> (invoked for zygote only).
> Could please anyone give a hint how ResourceBundle::InitSharedInstance is
> invoked on Windows (since it has the similar logic and
> 'g_supported_scale_factors' are initialized in the same place) ?

The failure is in child process. In child process, this is initialized in 

 [0x7fc80f1ee05e] ui::ResourceBundle::InitSharedInstance()
 [0x7fc80f1edfe7] ui::ResourceBundle::InitSharedInstanceWithLocale()
 [0x7fc8120babb2] ChromeBrowserMainParts::PreCreateThreadsImpl()
 [0x7fc8120b9ee6] ChromeBrowserMainParts::PreCreateThreads()
 [0x7fc802386b5d] content::BrowserMainLoop::PreCreateThreads()
 [0x7fc80238eaf2] base::internal::RunnableAdapter<>::Run()
 [0x7fc80238ea5c] base::internal::InvokeHelper<>::MakeItSo()
 [0x7fc80238ea0a] base::internal::Invoker<>::Run()
 [0x7fc8026fee8e] base::Callback<>::Run()
 [0x7fc80292d56c] content::StartupTaskRunner::RunAllTasksNow()
 [0x7fc80238712b] content::BrowserMainLoop::CreateStartupTasks()
 [0x7fc802392c24] content::BrowserMainRunnerImpl::Initialize()
 [0x7fc802383ee5] content::BrowserMain()
 [0x7fc80232ba61] content::RunNamedProcessTypeMain()
 [0x7fc80232cd4d] content::ContentMainRunnerImpl::Run()
 [0x7fc80232aec4] content::ContentMain()
 [0x7fc80f76d20e] ChromeMain

which seems to be happening after
ChromeBrowserMainExtraPartsAura::PostMainMessageLoopStart()

Is it possible to move the creation of DesktopScreen later than this?

[Elliot Glaysher, 2013-11-01 23:39:51]

On 2013/11/01 23:12:29, oshima wrote:
> On 2013/11/01 18:34:07, mikhail.pozdnyakov wrote:
> > On 2013/10/31 12:45:12, I haz the power (commit-bot) wrote:
> > > Retried try job too often on linux_aura for step(s) browser_tests
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
> > 
> > Tests fail due to assertion "Check failed: g_supported_scale_factors !=
> NULL.",
> > same happens if just launch chrome browser (content shell works fine!).
> >  [0x7f0c77b245a2] ui::GetSupportedScaleFactor()
> >  [0x7f0c71cf628b] (anonymous namespace)::GetDeviceScaleFactor()
> >  [0x7f0c71cf78c5] views::DesktopScreenX11::BuildDisplaysFromXRandRInfo()
> >  [0x7f0c71cf6630] views::DesktopScreenX11::DesktopScreenX11()
> >  [0x7f0c71cf7b00] views::CreateDesktopScreen()
> >  [0x7f0c7c491bbd]
ChromeBrowserMainExtraPartsAura::PostMainMessageLoopStart()
> >  [0x7f0c7ba24122] ChromeBrowserMainParts::PostMainMessageLoopStart()
> >  [0x7f0c7b58839e] ChromeBrowserMainPartsPosix::PostMainMessageLoopStart()
> >  [0x7f0c75d420f3] content::BrowserMainLoop::MainMessageLoopStart()
> >  [0x7f0c75d4a46a] content::BrowserMainRunnerImpl::Initialize()
> >  [0x7f0c75d3f45e] content::BrowserMain()
> >  [0x7f0c75cfd4a7] content::RunNamedProcessTypeMain()
> >  [0x7f0c75cfe676] content::ContentMainRunnerImpl::Run()
> >  [0x7f0c75cfc937] content::ContentMain()
> > 
> > 
> > Looks like ResourceBundle::InitSharedInstance is not invoked for browser
> process
> > (invoked for zygote only).
> > Could please anyone give a hint how ResourceBundle::InitSharedInstance is
> > invoked on Windows (since it has the similar logic and
> > 'g_supported_scale_factors' are initialized in the same place) ?
> 
> The failure is in child process. In child process, this is initialized in 
> 
>  [0x7fc80f1ee05e] ui::ResourceBundle::InitSharedInstance()
>  [0x7fc80f1edfe7] ui::ResourceBundle::InitSharedInstanceWithLocale()
>  [0x7fc8120babb2] ChromeBrowserMainParts::PreCreateThreadsImpl()
>  [0x7fc8120b9ee6] ChromeBrowserMainParts::PreCreateThreads()
>  [0x7fc802386b5d] content::BrowserMainLoop::PreCreateThreads()
>  [0x7fc80238eaf2] base::internal::RunnableAdapter<>::Run()
>  [0x7fc80238ea5c] base::internal::InvokeHelper<>::MakeItSo()
>  [0x7fc80238ea0a] base::internal::Invoker<>::Run()
>  [0x7fc8026fee8e] base::Callback<>::Run()
>  [0x7fc80292d56c] content::StartupTaskRunner::RunAllTasksNow()
>  [0x7fc80238712b] content::BrowserMainLoop::CreateStartupTasks()
>  [0x7fc802392c24] content::BrowserMainRunnerImpl::Initialize()
>  [0x7fc802383ee5] content::BrowserMain()
>  [0x7fc80232ba61] content::RunNamedProcessTypeMain()
>  [0x7fc80232cd4d] content::ContentMainRunnerImpl::Run()
>  [0x7fc80232aec4] content::ContentMain()
>  [0x7fc80f76d20e] ChromeMain
> 
> which seems to be happening after
> ChromeBrowserMainExtraPartsAura::PostMainMessageLoopStart()
> 
> Is it possible to move the creation of DesktopScreen later than this?

I don't think so, because the DesktopScreen constructor starts listening to the
x11 root window, and we need a MessagePump for that.

[Elliot Glaysher, 2013-11-01 23:40:18]

On 2013/11/01 23:39:51, Elliot Glaysher wrote:
> On 2013/11/01 23:12:29, oshima wrote:
> > On 2013/11/01 18:34:07, mikhail.pozdnyakov wrote:
> > > On 2013/10/31 12:45:12, I haz the power (commit-bot) wrote:
> > > > Retried try job too often on linux_aura for step(s) browser_tests
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
> > > 
> > > Tests fail due to assertion "Check failed: g_supported_scale_factors !=
> > NULL.",
> > > same happens if just launch chrome browser (content shell works fine!).
> > >  [0x7f0c77b245a2] ui::GetSupportedScaleFactor()
> > >  [0x7f0c71cf628b] (anonymous namespace)::GetDeviceScaleFactor()
> > >  [0x7f0c71cf78c5] views::DesktopScreenX11::BuildDisplaysFromXRandRInfo()
> > >  [0x7f0c71cf6630] views::DesktopScreenX11::DesktopScreenX11()
> > >  [0x7f0c71cf7b00] views::CreateDesktopScreen()
> > >  [0x7f0c7c491bbd]
> ChromeBrowserMainExtraPartsAura::PostMainMessageLoopStart()
> > >  [0x7f0c7ba24122] ChromeBrowserMainParts::PostMainMessageLoopStart()
> > >  [0x7f0c7b58839e] ChromeBrowserMainPartsPosix::PostMainMessageLoopStart()
> > >  [0x7f0c75d420f3] content::BrowserMainLoop::MainMessageLoopStart()
> > >  [0x7f0c75d4a46a] content::BrowserMainRunnerImpl::Initialize()
> > >  [0x7f0c75d3f45e] content::BrowserMain()
> > >  [0x7f0c75cfd4a7] content::RunNamedProcessTypeMain()
> > >  [0x7f0c75cfe676] content::ContentMainRunnerImpl::Run()
> > >  [0x7f0c75cfc937] content::ContentMain()
> > > 
> > > 
> > > Looks like ResourceBundle::InitSharedInstance is not invoked for browser
> > process
> > > (invoked for zygote only).
> > > Could please anyone give a hint how ResourceBundle::InitSharedInstance is
> > > invoked on Windows (since it has the similar logic and
> > > 'g_supported_scale_factors' are initialized in the same place) ?
> > 
> > The failure is in child process. In child process, this is initialized in 
> > 
> >  [0x7fc80f1ee05e] ui::ResourceBundle::InitSharedInstance()
> >  [0x7fc80f1edfe7] ui::ResourceBundle::InitSharedInstanceWithLocale()
> >  [0x7fc8120babb2] ChromeBrowserMainParts::PreCreateThreadsImpl()
> >  [0x7fc8120b9ee6] ChromeBrowserMainParts::PreCreateThreads()
> >  [0x7fc802386b5d] content::BrowserMainLoop::PreCreateThreads()
> >  [0x7fc80238eaf2] base::internal::RunnableAdapter<>::Run()
> >  [0x7fc80238ea5c] base::internal::InvokeHelper<>::MakeItSo()
> >  [0x7fc80238ea0a] base::internal::Invoker<>::Run()
> >  [0x7fc8026fee8e] base::Callback<>::Run()
> >  [0x7fc80292d56c] content::StartupTaskRunner::RunAllTasksNow()
> >  [0x7fc80238712b] content::BrowserMainLoop::CreateStartupTasks()
> >  [0x7fc802392c24] content::BrowserMainRunnerImpl::Initialize()
> >  [0x7fc802383ee5] content::BrowserMain()
> >  [0x7fc80232ba61] content::RunNamedProcessTypeMain()
> >  [0x7fc80232cd4d] content::ContentMainRunnerImpl::Run()
> >  [0x7fc80232aec4] content::ContentMain()
> >  [0x7fc80f76d20e] ChromeMain
> > 
> > which seems to be happening after
> > ChromeBrowserMainExtraPartsAura::PostMainMessageLoopStart()
> > 
> > Is it possible to move the creation of DesktopScreen later than this?
> 
> I don't think so, because the DesktopScreen constructor starts listening to
the
> x11 root window, and we need a MessagePump for that.

nm, I misread what you said.

[Mikhail, 2013-11-04 17:27:19]

On 2013/11/01 23:12:29, oshima wrote:
> On 2013/11/01 18:34:07, mikhail.pozdnyakov wrote:
> > On 2013/10/31 12:45:12, I haz the power (commit-bot) wrote:
> > > Retried try job too often on linux_aura for step(s) browser_tests
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
> > 
> > Tests fail due to assertion "Check failed: g_supported_scale_factors !=
> NULL.",
> > same happens if just launch chrome browser (content shell works fine!).
> >  [0x7f0c77b245a2] ui::GetSupportedScaleFactor()
> >  [0x7f0c71cf628b] (anonymous namespace)::GetDeviceScaleFactor()
> >  [0x7f0c71cf78c5] views::DesktopScreenX11::BuildDisplaysFromXRandRInfo()
> >  [0x7f0c71cf6630] views::DesktopScreenX11::DesktopScreenX11()
> >  [0x7f0c71cf7b00] views::CreateDesktopScreen()
> >  [0x7f0c7c491bbd]
ChromeBrowserMainExtraPartsAura::PostMainMessageLoopStart()
> >  [0x7f0c7ba24122] ChromeBrowserMainParts::PostMainMessageLoopStart()
> >  [0x7f0c7b58839e] ChromeBrowserMainPartsPosix::PostMainMessageLoopStart()
> >  [0x7f0c75d420f3] content::BrowserMainLoop::MainMessageLoopStart()
> >  [0x7f0c75d4a46a] content::BrowserMainRunnerImpl::Initialize()
> >  [0x7f0c75d3f45e] content::BrowserMain()
> >  [0x7f0c75cfd4a7] content::RunNamedProcessTypeMain()
> >  [0x7f0c75cfe676] content::ContentMainRunnerImpl::Run()
> >  [0x7f0c75cfc937] content::ContentMain()
> > 
> > 
> > Looks like ResourceBundle::InitSharedInstance is not invoked for browser
> process
> > (invoked for zygote only).
> > Could please anyone give a hint how ResourceBundle::InitSharedInstance is
> > invoked on Windows (since it has the similar logic and
> > 'g_supported_scale_factors' are initialized in the same place) ?
> 
> The failure is in child process. In child process, this is initialized in 
> 
>  [0x7fc80f1ee05e] ui::ResourceBundle::InitSharedInstance()
>  [0x7fc80f1edfe7] ui::ResourceBundle::InitSharedInstanceWithLocale()
>  [0x7fc8120babb2] ChromeBrowserMainParts::PreCreateThreadsImpl()
>  [0x7fc8120b9ee6] ChromeBrowserMainParts::PreCreateThreads()
>  [0x7fc802386b5d] content::BrowserMainLoop::PreCreateThreads()
>  [0x7fc80238eaf2] base::internal::RunnableAdapter<>::Run()
>  [0x7fc80238ea5c] base::internal::InvokeHelper<>::MakeItSo()
>  [0x7fc80238ea0a] base::internal::Invoker<>::Run()
>  [0x7fc8026fee8e] base::Callback<>::Run()
>  [0x7fc80292d56c] content::StartupTaskRunner::RunAllTasksNow()
>  [0x7fc80238712b] content::BrowserMainLoop::CreateStartupTasks()
>  [0x7fc802392c24] content::BrowserMainRunnerImpl::Initialize()
>  [0x7fc802383ee5] content::BrowserMain()
>  [0x7fc80232ba61] content::RunNamedProcessTypeMain()
>  [0x7fc80232cd4d] content::ContentMainRunnerImpl::Run()
>  [0x7fc80232aec4] content::ContentMain()
>  [0x7fc80f76d20e] ChromeMain
> 
> which seems to be happening after
> ChromeBrowserMainExtraPartsAura::PostMainMessageLoopStart()
> 
> Is it possible to move the creation of DesktopScreen later than this?

That helps indeed :) https://codereview.chromium.org/52783003/ Thank you!

[commit-bot: I haz the power, 2013-11-08 08:34:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/27156003/...

[commit-bot: I haz the power, 2013-11-08 12:09:45]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-08 12:22:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/27156003/...

[commit-bot: I haz the power, 2013-11-08 16:18:42]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[Mikhail, 2013-11-08 16:52:52]

Most failing tests fail with:
 [0x7f2d83c332d6] <unknown>
 [0x7f2d83c3381f] <unknown>
 [0x7f2d83c3c668] SECMOD_LoadModule
 [0x7f2d83c3c706] SECMOD_LoadModule
 [0x7f2d83c164b3] <unknown>
 [0x7f2d83c16c16] NSS_InitReadWrite
 [0x0000022485c3] base::DefaultLazyInstanceTraits<>::New()
 [0x000002248b0c] crypto::EnsureNSSInit()
 [0x0000018bd081] net::NSSCertDatabase::NSSCertDatabase()
 [0x0000018bd107] net::NSSCertDatabase::GetInstance()
 [0x0000018af8f5] net::CertDatabase::CertDatabase()
 [0x0000018adeab] net::CertDatabase::GetInstance()
 [0x0000019be8d1] base::DefaultLazyInstanceTraits<>::New()
 [0x0000019beb6c] net::ClientSocketFactory::GetDefaultFactory()
 [0x000001a9943b] net::AddressSorter::CreateAddressSorter()
 [0x0000019134ab] net::DnsClient::CreateClient()
 [0x000001932d4e] net::HostResolverImpl::SetDnsClientEnabled()
 [0x000001063ef6] IOThread::InitAsync()
 [0x0000017ce403] base::MessageLoop::RunTask()
 [0x0000017d326b] base::MessageLoop::DeferOrRunPendingTask()
 [0x0000017d3773] base::MessageLoop::DoWork()
 [0x000001796b82] base::MessagePumpLibevent::Run()
 [0x0000017d2795] base::MessageLoop::RunInternal()
 [0x0000017f0c48] base::RunLoop::Run()

Looks like it should not be because of my patch

[commit-bot: I haz the power, 2013-11-08 16:53:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/27156003/...

[commit-bot: I haz the power, 2013-11-08 20:49:15]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-10 20:14:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/27156003/...

[commit-bot: I haz the power, 2013-11-10 23:34:49]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-11 08:53:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/27156003/...

[commit-bot: I haz the power, 2013-11-11 09:24:39]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-11 19:03:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/27156003/...

[commit-bot: I haz the power, 2013-11-12 01:40:08]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[Mikhail, 2013-11-12 16:19:41]

On 2013/11/12 01:40:08, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_aura for step(s) browser_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

I have run browser_tests locally with the patch and they all passed for me.
I have noticed also that same problem turns up for other CL :
http://build.chromium.org/p/tryserver.chromium/builders/linux_aura/builds/95557
has also around 80 failed browser tests on linux_aura. All the failing tests
have the following similarity in the log:
Xlib:  extension "RANDR" missing on display ":9".
Xlib:  extension "RANDR" missing on display ":9".

Looks like there are some problems on bot rather than with the patch.

[commit-bot: I haz the power, 2013-11-13 09:18:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/27156003/...

[commit-bot: I haz the power, 2013-11-13 09:18:38]

Failed to apply patch for ui/views/widget/desktop_aura/desktop_screen_x11.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file ui/views/widget/desktop_aura/desktop_screen_x11.cc
  Hunk #1 FAILED at 14.
  Hunk #2 succeeded at 31 (offset 2 lines).
  Hunk #3 succeeded at 151 (offset 2 lines).
  Hunk #4 succeeded at 317 (offset 4 lines).
  Hunk #5 succeeded at 343 (offset 4 lines).
  1 out of 5 hunks FAILED -- saving rejects to file
ui/views/widget/desktop_aura/desktop_screen_x11.cc.rej

Patch:       ui/views/widget/desktop_aura/desktop_screen_x11.cc
Index: ui/views/widget/desktop_aura/desktop_screen_x11.cc
diff --git a/ui/views/widget/desktop_aura/desktop_screen_x11.cc
b/ui/views/widget/desktop_aura/desktop_screen_x11.cc
index
c54378e1857ba7a2baf59ae8aa5ab07a5fd24a22..9c0e35a552e6c42855ba0a33ef89a5ff053bd029
100644
--- a/ui/views/widget/desktop_aura/desktop_screen_x11.cc
+++ b/ui/views/widget/desktop_aura/desktop_screen_x11.cc
@@ -14,6 +14,7 @@
 #include "base/x11/edid_parser_x11.h"
 #include "ui/aura/root_window.h"
 #include "ui/aura/root_window_host.h"
+#include "ui/base/layout.h"
 #include "ui/base/x/x11_util.h"
 #include "ui/gfx/display.h"
 #include "ui/gfx/display_observer.h"
@@ -29,14 +30,35 @@ namespace {
 // in |Dispatch()|.
 const int64 kConfigureDelayMs = 500;
 
+float GetDeviceScaleFactor(int screen_pixels, int screen_mm) {
+  const int kCSSDefaultDPI = 96;
+  const float kInchInMm = 25.4f;
+
+  float screen_inches = screen_mm / kInchInMm;
+  float screen_dpi = screen_pixels / screen_inches;
+  float scale = screen_dpi / kCSSDefaultDPI;
+
+  return ui::GetImageScale(ui::GetSupportedScaleFactor(scale));
+}
+
 std::vector<gfx::Display> GetFallbackDisplayList() {
   ::XDisplay* display = gfx::GetXDisplay();
   ::Screen* screen = DefaultScreenOfDisplay(display);
   int width = WidthOfScreen(screen);
   int height = HeightOfScreen(screen);
+  int mm_width = WidthMMOfScreen(screen);
+  int mm_height = HeightMMOfScreen(screen);
+
+  gfx::Rect bounds_in_pixels(0, 0, width, height);
+  gfx::Display gfx_display(0, bounds_in_pixels);
+  if (!gfx::Display::HasForceDeviceScaleFactor() &&
+      !ui::IsXDisplaySizeBlackListed(mm_width, mm_height)) {
+    float device_scale_factor = GetDeviceScaleFactor(width, mm_width);
+    DCHECK_LE(1.0f, device_scale_factor);
+    gfx_display.SetScaleAndBounds(device_scale_factor, bounds_in_pixels);
+  }
 
-  return std::vector<gfx::Display>(
-      1, gfx::Display(0, gfx::Rect(0, 0, width, height)));
+  return std::vector<gfx::Display>(1, gfx_display);
 }
 
 }  // namespace
@@ -128,7 +150,7 @@ void DesktopScreenX11::ProcessDisplayChange(
 // DesktopScreenX11, gfx::Screen implementation:
 
 bool DesktopScreenX11::IsDIPEnabled() {
-  return false;
+  return true;
 }
 
 gfx::Point DesktopScreenX11::GetCursorScreenPoint() {
@@ -292,6 +314,7 @@ std::vector<gfx::Display>
DesktopScreenX11::BuildDisplaysFromXRandRInfo() {
     has_work_area = true;
   }
 
+  float device_scale_factor = 1.0f;
   for (int i = 0; i < resources->noutput; ++i) {
     RROutput output_id = resources->outputs[i];
     XRROutputInfo* output_info =
@@ -317,6 +340,19 @@ std::vector<gfx::Display>
DesktopScreenX11::BuildDisplaysFromXRandRInfo() {
 
       gfx::Rect crtc_bounds(crtc->x, crtc->y, crtc->width, crtc->height);
       gfx::Display display(display_id, crtc_bounds);
+
+      if (!gfx::Display::HasForceDeviceScaleFactor()) {
+        if (i == 0 && !ui::IsXDisplaySizeBlackListed(output_info->mm_width,
+                                                     output_info->mm_height)) {
+          // As per display scale factor is not supported right now,
+          // the primary display's scale factor is always used.
+          device_scale_factor = GetDeviceScaleFactor(crtc->width,
+                                                     output_info->mm_width);
+          DCHECK_LE(1.0f, device_scale_factor);
+        }
+        display.SetScaleAndBounds(device_scale_factor, crtc_bounds);
+      }
+
       if (has_work_area) {
         gfx::Rect intersection = crtc_bounds;
         intersection.Intersect(work_area);

[commit-bot: I haz the power, 2013-11-13 10:29:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/27156003/...

[commit-bot: I haz the power, 2013-11-13 13:15:01]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Mikhail, 2013-11-18 13:27:22]

I have updated the patch according to https://codereview.chromium.org/52713010
"Remove reference to missing assets. 140 and 180 have been removed" - I
 removed introducing supported scale factors for linux  within
ResourceBundle::InitSharedInstance. 
Do I need another review after that? (the change is quite trivial but maybe you
would like to have a look).

[Elliot Glaysher, 2013-11-18 18:59:56]

On 2013/11/18 13:27:22, mikhail.pozdnyakov wrote:
> I have updated the patch according to https://codereview.chromium.org/52713010
> "Remove reference to missing assets. 140 and 180 have been removed" - I
>  removed introducing supported scale factors for linux  within
> ResourceBundle::InitSharedInstance. 
> Do I need another review after that? (the change is quite trivial but maybe
you
> would like to have a look).

I don't think so. slgtm.

[commit-bot: I haz the power, 2013-11-19 08:47:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mikhail.pozdnyakov@intel.com/27156003/...

[commit-bot: I haz the power, 2013-11-19 10:33:43]

Change committed as 235967

[asl.pavel_gmail.com, 2014-01-04 17:00:17]

Hi guys!

Sorry for stupid question, but is it already possible to enable HiDPI for 
linux?
I have latest build 33.0.1750.5 aura (241733), and this tiny interface is 
driving me crazy... Thanks!

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.