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

[YAMAMOTO Mitsuharu]

>>>>> On Thu, 09 May 2013 23:03:15 +0300, Eli Zaretskii <address@hidden> said:

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

If the callback can be called from the thread/context other than the
one for EnumDisplayMonitors and that can cause memory synchronization
issue, then API documentation should clearly say so, because such kind
of problem can happen even with other data structures.

And it is not Fcons but Feval that can cause GC.

> 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?

The display-monitor-function lists all the monitors regardless of the
existence of Emacs frames in it.

>> +       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.)

I'm aware of that.  It's just for resemblance to X11 code.  I'd rather
leave it to make it easier to compare with the code for X11 if it were
for the Mac port, but I don't have a strong opinion for W32.

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

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

make_unibyte_string does not assume that the passed bytes are only of
ASCII characters.  I made it on the safe side in case the contents of
szDevice member accidentally matches with Emacs internal encoding and
misinterpreted.

Of course, you can decode the string if its encoding is known (I'm not
sure how it is encoded because I'm not familiar with W32).  But you
should be careful because code conversions can involve GC in general
unlike Fcons.  Alternatively, as the `name' attribute is optional, you
can remove it entirely if you don't like make_unibyte_string.

                                     YAMAMOTO Mitsuharu
                                address@hidden