nsCSSRendering.cpp: #ifdef XP_Unix needs to go away

VERIFIED DUPLICATE of bug 33119

Status

()

Core
CSS Parsing and Computation
P3
normal
VERIFIED DUPLICATE of bug 33119
19 years ago
13 years ago

People

(Reporter: Pierre Saslawsky, Assigned: Stuart Parmenter)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

19 years ago
This bug is just a reminder...

In nsCSSRendering.cpp, line 2195, you added some #ifdef XP_UNIX with the 
following comment:

// XXX this #ifdef needs to go away when we are sure that this works on windows 
and mac
(Reporter)

Comment 1

19 years ago
Putting myself as QA contact.
QA Contact: chrisd → pierre
(Assignee)

Comment 2

19 years ago
can you test this on windows and mac?  It should work fine and removing the
ifdef should be OK.
(Reporter)

Comment 3

19 years ago
If you provide a testcase along with 2 screen snapshots showing what should be 
displayed and what shouldn't depending on whether the code is enabled or not, 
then I'll gladly look at it but the fact is: a "#ifdef XP_something" has no place 
in Layout.
- Why was it fixed on Unix only? Did it happen on all the flavors of Unix or just 
Linux?
- Don't we have the same problem on Mac and Windows in other circumstances than 
the ones described in the original bug report? If not, why?
- And more importantly, are you sure it is a problem of bad clipping in Layout 
instead of a problem with DrawImage() in LinuxGFX? If the problem is in LinuxGFX, 
we need to move your fix there. If the problem is in Layout, have you checked all 
the other instances of DrawImage() to see if we were clipping correctly?

You should have done this verification before you checked in or eventually the 
day after, not mentioning asking for review and approval to dcone who is the 
original author of that part of the code. Sorry Pav.
(Assignee)

Updated

19 years ago
Assignee: pavlov → dcone
(Assignee)

Comment 4

19 years ago
pierre - the bug is in Mac and Windows GFX code.  I had a long discussion with
Don Cone on this issue and he told me to check this in #ifdef XP_UNIX until he
had a chance to verify it was ok on windows and mac.  He was going to make sure
it worked on Mac and Windows...

The problem in Windows and Mac GFX is that their drawing surfaces do not respect
the clipping set by the rendering context.  Since there is no way to set
clipping directly on a drawing surface, and because DrawImage is a call on the
rendering context, DrawImage should be using the clipping set.  This problem
occurs with new drawing surfaces.  For example (as it does in the code in
question), create new drawing surface on rendering context with clipping that
you don't want to apply to the drawing surface because it would clip everything,
then call DrawImage.  On Mac and Windows this works because the new drawing
surface has an empty clip rect and it does not draw using the clipping set on
the rendering context as it should.  On Linux, we do clip based on the clipping
from the rendering context, so the DrawImage calls all get clipped by the clip
rect previously set.  The fix is to set the clipping rect before drawing the
image on the new drawing surface.

I hope this clears things up.

The bug that this was causing before was the toolbar background on linux was not
drawing properly.

Stuart
(Reporter)

Comment 5

19 years ago
Ok. You guys rock, I'll go to bed!

Updated

19 years ago
Status: NEW → ASSIGNED

Comment 6

18 years ago
When I took those comments out, windows broke.  This bug will go away because 
the tiling is going to become platform specific (moved to the 
renderingcontexts).  So I am marking this as invalid.. this will not happen.
Status: ASSIGNED → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → WONTFIX
Target Milestone: M16
(Assignee)

Comment 7

18 years ago
this is still a bug and was in places like the old view manager.  if it broke on
windows, then windows needs to be fixed.  Mac and Windows should not contain the
broken behavior they have.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Reporter)

Comment 8

18 years ago
If you have a bug for the platform-specific tiling code, you can put a ***BIG 
NOTE*** there to remember to remove the #ifdef XP_Unix stuff and then close the 
present bug. There is no need to keep a separate bug open about a code-level 
problem.
(Assignee)

Comment 9

18 years ago
this bug should really be changed to say "creatin a new nsIDrawingSurface
implimentation on mac and windows don't obey the clipping from the rendering
context" I suppose.  This should be fixed.

Comment 10

18 years ago
The clipping works the way it should on Mac and windows.. There is a default 
clipping on the Mac and Windows.. in the RenderingContext, not on the 
DrawingSurface.. which needs to have a RenderingContext if it is to have 
clipping, we don't want to have clipping being kept track of in two different 
classes.  
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → INVALID
(Assignee)

Comment 11

18 years ago
What?  That isn't the bug here.  Mac and Windows should be using the clipping
from the rendering context that created them when doing operations on the
drawing surface.  Drawing surface not having its own clipping information is the
reason why it should be using the information from the rendering context that
created it.  We can not have differences in the way our platforms do rendering. 
I will go fix windows and mac if you like, but I would rather have someone that
knows these platforms do it.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---

Comment 12

18 years ago
The Mac and Windows drawingsurface both use the Clipping from the 
renderingContext which they are set into.  We can only call drawing/blitting 
commands on the RenderingContext.. the final drawing into the drawingsurface is 
determined by the RenderingContext and which drawingsurface is set in the 
RenderingContext.  The clipping is determined by this Renderingcontext and all 
drawing is clipped to this through the renderingcontext.  So I think we agree to 
how it should work..and I am saying that this is currently how it does work on 
Mac and Windows. I am not sure how it works on Unix.. but that should also work 
this way.
Status: REOPENED → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → INVALID
(Reporter)

Comment 13

18 years ago
I would have sided with Don just on the base of his explanations but I got 
confirmed in my feelings by looking at nsRenderingContextGTK.cpp. The function 
CopyOffScreenBits() ignores the flag NS_COPYBITS_USE_SOURCE_CLIP_REGION. Pav 
copied the code there with "#ifdef 0 // XXX impliment me" last October. It looks 
like the bug should be reopened and assigned to him.
(Assignee)

Comment 14

18 years ago
I'm not honestly sure what that function is meant to do.  I don't think that
that flag is actually valid.  Since the method takes a drawing surface as the
source and the drawing surface does not have a clipping area, how can we use
it's clipping?
(Reporter)

Comment 15

18 years ago
This function copies bits from a source surface (passed as a parameter) to 
another one.

If NS_COPYBITS_TO_BACK_BUFFER is 0, the destination surface is the screen. If it 
is 1, the destination surface is the surface currently selected in the 
RenderingContext.

If NS_COPYBITS_USE_SOURCE_CLIP_REGION is 0, we clip to the region currently set 
in the RenderingContext. If it is 1, we clip to the region that was last set for 
the source surface (on the Mac, this region is part of the basic MacOS structure 
called the GrafPort, which is attached to all the drawing surfaces).

As you can see, there is a clipping area attached to the drawing surface. What 
Don said is that all the clipping/drawing/blitting is done through the 
RenderingContext. After all, it really makes sense: that's how the MacOS and 
Windows work too, and I guess other platforms.

Reopened.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
(Reporter)

Comment 16

18 years ago
Back to you, Pav.
Assignee: dcone → pavlov
Status: REOPENED → NEW

Comment 17

18 years ago
Mass-moving all M16 non-feature bugs to M17, which we still consider to be 
part of beta2
Target Milestone: M16 → M17
(Reporter)

Comment 18

18 years ago
Marking dependency on bug 33119 which has been declared [nsbeta2+], but these 
bugs are duplicates really...
Depends on: 33119

Comment 19

18 years ago
nominating for nsbeta2.  Pierre, if this is really a dup, please resolve as 
such
Keywords: nsbeta2
(Reporter)

Comment 20

18 years ago


*** This bug has been marked as a duplicate of 33119 ***
Status: NEW → RESOLVED
Last Resolved: 18 years ago18 years ago
Resolution: --- → DUPLICATE
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.