Subject: Re: bug#11068: 24.0.94; Face-remapped background does not extend to end of window

From: Chong Yidong <[email-masked]>
Date: Sun, 25 Mar 2012 11:01:17 +0800

Eli Zaretskii <[email-masked]> writes:

>> This looks a bit strange.  Why is DEFAULT_FACE_ID handled specially
>> here?
>
> In my testing, I didn't see the need to do it even for the default
> face, because it->face_id is already set to the ID of the remapped
> face.  So it's just me being paranoiac.

I'd just leave that bit out.

Also, in this hunk

@@ -18173,7 +18173,12 @@
 	  it->len = 1;
 
 	  if (default_face_p)
-	    it->face_id = DEFAULT_FACE_ID;
+	    {
+	      if (NILP (Vface_remapping_alist))
+		it->face_id = DEFAULT_FACE_ID;
+	      else
+		it->face_id = lookup_basic_face (it->f, DEFAULT_FACE_ID);
+	    }
 	  else if (it->face_before_selective_p)
 	    it->face_id = it->saved_face_id;
 	  face = FACE_FROM_ID (it->f, it->face_id);

You should call lookup_basic_face without the surrounding if statement,
because lookup_basic_face returns its second arg immediately if
face-remapping-alist is nil, making the extra check redundant.

And here

+	  if (row->reversed_p
+	      || lookup_basic_face (it->f, DEFAULT_FACE_ID) != DEFAULT_FACE_ID)

shouldn't you just compare default_face->id to DEFAULT_FACE_ID instead
of making another lookup_basic_face call?

With these changes, the patch seems pretty safe; even if something
screws up, it will only affect how remapped faces extend to the end of
the buffer, which was broken before anyway.  So feel free to commit.