Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Description • 17 years ago

This is spun off from bug 365336.

I was not used NSAppUnitsToFloatPixels in the patch of bug 365336. Because cairo/thebes uses double for non-rounded device pixels, so we should create |NSAppUnitsToDoublePixels| and use it.

# And I will fix the typos in bug 365336 and bug 365414 in this bug, sorry.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 2 • 17 years ago

er, I forgot to change the non-my codes.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 17 years ago

Comment on attachment 277377 [details] [diff] [review]
Patch rv1.1

This should get review from Eli first.

	Eli Friedman
	Comment 4 • 17 years ago

I think it would be nice to add something like gfxFloat nsPresContext::AppUnitsToGfxCoords(nscoord aAppUnits), so that the code doesn't have to mess with the conversion factor as much. Otherwise, it looks good from a rough glance.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 5 • 17 years ago

thanks. I changed as so.

	Eli Friedman
	Comment 6 • 17 years ago

Are all those Makefile changes supposed to be part of this patch?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 7 • 17 years ago

Yes. gfxFloat is defined in thebes.

	Eli Friedman
	Comment 8 • 17 years ago

Comment on attachment 277615 [details] [diff] [review]
Patch rv2.0

About the Makefile changes: our dependency system sucks, and it seems you need the Thebes dependencies.  I'll let the sr decide whether that's the best thing to do. (It would also be nice to comment on how you decided where the additional dependencies were needed.)

>-    double size = NSAppUnitsToFloatPixels(aFont.size, mP2A);
>+    double size = double(aFont.size) / mP2A;

Should be gfxFloat, not double.

>- *   H�kan Waara <hwaara@chello.se>
>+ *   Håkan Waara <hwaara@chello.se>

I think your text editor is misconfigured?

>-    // Currently, only undeline is overflowable.
>+    // Currently, only underline is overflowable.

Try to avoid putting spelling corrections to irrelevant files into diffs; it just makes things harder to review.

>-  float t2p = 1.0f / aPresContext->AppUnitsPerDevPixel();
>+  PRInt32 app = aPresContext->AppUnitsPerDevPixel();
>   aCtx->NewPath();
>   // pixel-snap
>-  aCtx->Rectangle(gfxRect(r.X()*t2p, r.Y()*t2p, r.Width()*t2p, r.Height()*t2p), PR_TRUE);
>+  aCtx->Rectangle(gfxRect(r.X() / app, r.Y() / app,
>+                          r.Width() / app, r.Height() / app), PR_TRUE);
>   aCtx->SetColor(gfxRGBA(aColor));
>   aCtx->Fill();
> }

Can you fix this to use the prescontext helpers?

>   gfxFont::Metrics fontMetrics = GetFontMetrics(aProvider.GetFontGroup());
>-  gfxFloat a2p = 1.0 / aTextPaintStyle.PresContext()->AppUnitsPerDevPixel();
>+  PRInt32 app = aTextPaintStyle.PresContext()->AppUnitsPerDevPixel();
> 
>   // XXX aFramePt is in AppUnits, shouldn't it be nsFloatPoint?
>-  gfxPoint pt(aFramePt.x * a2p, aFramePt.y * a2p);
>-  gfxSize size(GetRect().width * a2p, 0);
>-  gfxFloat ascent = mAscent * a2p;
>+  gfxPoint pt(aFramePt.x / app, aFramePt.y / app);
>+  gfxSize size(GetRect().width / app, 0);
>+  gfxFloat ascent = mAscent / app;

Same here.

>-      gfxFloat a2p = 1.0 / aTextPaintStyle.PresContext()->AppUnitsPerDevPixel();
>+      PRInt32 app = aTextPaintStyle.PresContext()->AppUnitsPerDevPixel();
>       // XXX aTextBaselinePt is in AppUnits, shouldn't it be nsFloatPoint?
>-      gfxPoint pt((aTextBaselinePt.x + xOffset) * a2p,
>-                  (aTextBaselinePt.y - mAscent) * a2p);
>-      gfxFloat width = PR_ABS(advance) * a2p;
>+      gfxPoint pt((aTextBaselinePt.x + xOffset) / app,
>+                  (aTextBaselinePt.y - mAscent) / app);
>+      gfxFloat width = PR_ABS(advance) / app;
>       DrawSelectionDecorations(aCtx, aSelectionType, aTextPaintStyle,
>-                               pt, width, mAscent * a2p, decorationMetrics,
>+                               pt, width, mAscent / app, decorationMetrics,
>                                mTextRun->IsRightToLeft());

And here.

r=me with those changes; I'd suggest asking roc for sr.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 9 • 17 years ago

(In reply to comment #8)
> (From update of attachment 277615 [details] [diff] [review])
> About the Makefile changes: our dependency system sucks, and it seems you need
> the Thebes dependencies.  I'll let the sr decide whether that's the best thing
> to do. (It would also be nice to comment on how you decided where the
> additional dependencies were needed.)

yeah, I built fx/tb/seamonkey very very many times for finding the dependencies :-(

> >- *   H�kan Waara <hwaara@chello.se>
> >+ *   Håkan Waara <hwaara@chello.se>
> 
> I think your text editor is misconfigured?

No, the file is encoded as (maybe) Latin-1. I think that we should use UTF-8N for our source code.

> >-  float t2p = 1.0f / aPresContext->AppUnitsPerDevPixel();
> >+  PRInt32 app = aPresContext->AppUnitsPerDevPixel();
> >   aCtx->NewPath();
> >   // pixel-snap
> >-  aCtx->Rectangle(gfxRect(r.X()*t2p, r.Y()*t2p, r.Width()*t2p, r.Height()*t2p), PR_TRUE);
> >+  aCtx->Rectangle(gfxRect(r.X() / app, r.Y() / app,
> >+                          r.Width() / app, r.Height() / app), PR_TRUE);
> >   aCtx->SetColor(gfxRGBA(aColor));
> >   aCtx->Fill();
> > }
> 
> Can you fix this to use the prescontext helpers?

If so, we need |gfxFloat AppUnitsToGfxCoords(gfxFloat aAppUnits)|. But I think that this is wrong. Because nscoord should not be gfxFloat. Only on nsTextFrameThebes, nscoord as gfxFloat(double) is used. If they are float, we should create |gfxFloat AppUnitsToGfxCoords(float aAppUnits)|. Should I create new API only for nsTextFrameThebes??

	Eli Friedman
	Comment 10 • 17 years ago

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 277615 [details] [diff] [review] [details])
> > About the Makefile changes: our dependency system sucks, and it seems you need
> > the Thebes dependencies.  I'll let the sr decide whether that's the best thing
> > to do. (It would also be nice to comment on how you decided where the
> > additional dependencies were needed.)
> 
> yeah, I built fx/tb/seamonkey very very many times for finding the dependencies
> :-(

All right... I guess watch for bustage on the tinderboxes when you check in.

> > >- *   H�kan Waara <hwaara@chello.se>
> > >+ *   Håkan Waara <hwaara@chello.se>
> > 
> > I think your text editor is misconfigured?
> 
> No, the file is encoded as (maybe) Latin-1. I think that we should use UTF-8N
> for our source code.

Hmm, you're right... I guess someone else (possibly me) broke it.

> > >-  float t2p = 1.0f / aPresContext->AppUnitsPerDevPixel();
> > >+  PRInt32 app = aPresContext->AppUnitsPerDevPixel();
> > >   aCtx->NewPath();
> > >   // pixel-snap
> > >-  aCtx->Rectangle(gfxRect(r.X()*t2p, r.Y()*t2p, r.Width()*t2p, r.Height()*t2p), PR_TRUE);
> > >+  aCtx->Rectangle(gfxRect(r.X() / app, r.Y() / app,
> > >+                          r.Width() / app, r.Height() / app), PR_TRUE);
> > >   aCtx->SetColor(gfxRGBA(aColor));
> > >   aCtx->Fill();
> > > }
> > 
> > Can you fix this to use the prescontext helpers?
> 
> If so, we need |gfxFloat AppUnitsToGfxCoords(gfxFloat aAppUnits)|. But I think
> that this is wrong. Because nscoord should not be gfxFloat. Only on
> nsTextFrameThebes, nscoord as gfxFloat(double) is used. If they are float, we
> should create |gfxFloat AppUnitsToGfxCoords(float aAppUnits)|. Should I create
> new API only for nsTextFrameThebes??

No, nevermind... it's okay as-is for the moment.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 11 • 17 years ago

Should this new method be in nsIDeviceContext as well, so we don't have to add aPresContext parameters anywhere?

Don't create anything new for nsTextFrameThebes. Just leave it.

+    pxSrc.pos.x = aPresContext->AppUnitsToGfxCSSPixels(aSourceRect->x);
+    pxSrc.pos.y = aPresContext->AppUnitsToGfxCSSPixels(aSourceRect->y);
+    pxSrc.size.width = aPresContext->AppUnitsToGfxCSSPixels(aSourceRect->width);
+    pxSrc.size.height = aPresContext->AppUnitsToGfxCSSPixels(aSourceRect->height);

These are static, so call them with nsIPresContext:: instead of using aPresContext.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 12 • 17 years ago

changed nsIDeviceContext too.

I'll check-in midnight in PDT.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 13 • 17 years ago

Comment on attachment 277647 [details] [diff] [review]
Patch rv2.1

"GfxCoords" is not a good phrase; there is only one coordinate. Say "GfxUnits" instead.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 14 • 17 years ago

I'll land soon.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 15 • 17 years ago

checked-in.

I'll check-in the additional patch if some machines are red.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 16 • 17 years ago

-> FIXED