Re: x-display-pixel-width/height inconsistency

[Eli Zaretskii]

> Date: Thu, 09 May 2013 09:09:33 +0900
> From: YAMAMOTO Mitsuharu <address@hidden>
> Cc: address@hidden
> 
> Thanks for testing.  I updated the w32fns.c part below.

I have a couple more comments.

> + static BOOL CALLBACK
> + w32_monitor_enum (HMONITOR monitor, HDC hdc, RECT *rcMonitor, LPARAM dwData)
> + {
> +   Lisp_Object *monitor_list = (Lisp_Object *) dwData;
> + 
> +   *monitor_list = Fcons (make_save_pointer (monitor), *monitor_list);
> + 
> +   return TRUE;
> + }

It makes me nervous to have consing inside a Win32 API callback.  Are
we even sure the callback gets called in the context of our main
tread?  If not, and if consing causes GC, we could blow up the stack.

And actually, I don't see why you need the list of monitor HMONITOR
handles, you get them from MonitorFromWindow anyway.  All you need is
the count of the monitors, and then you can create the vector in
monitor_frames by simply inserting every monitor you get from
MonitorFromWindow, and leave the rest at nil.  Or am I missing
something?

So could we please get rid of the consing inside the callback, and
just count them instead?

> +       if (FRAME_W32_P (f) && FRAME_W32_DISPLAY_INFO (f) == dpyinfo
> +       && !EQ (frame, tip_frame))

The test against dpyinfo is redundant, it will always be true: there's
only one dpyinfo on Windows.  I would like to remove it, because it's
confusing.  (There's one more such test in another place in the
patch.)

> +       name = make_unibyte_string (mi.szDevice, strlen (mi.szDevice));

Why make_unibyte_string?  Is szDevice documented to be an ASCII-only
string?

> +   attributes = Fcons (Fcons (Qname, build_string ("combined screen")),
> +                   attributes);

And using build_string here with a pure ASCII string is at least
inconsistent.  (But I actually suggest to use build_string in both
cases.)

Thanks.