392252 - Need new APIs which convert AppUnits to pixels for gfx

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.0

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv1.1

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

This should get review from Eli first.

Comment 4

[Eli Friedman]

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.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.0

thanks. I changed as so.

Comment 6

[Eli Friedman]

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

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yes. gfxFloat is defined in thebes.

Comment 8

[Eli Friedman]

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 <[e-mail]>
>+ *   Håkan Waara <[e-mail]>

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.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(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 <[e-mail]>
> >+ *   Håkan Waara <[e-mail]>
> 
> 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??

Comment 10

[Eli Friedman]

(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 <[e-mail]>
> > >+ *   Håkan Waara <[e-mail]>
> > 
> > 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.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.1

changed nsIDeviceContext too.

I'll check-in midnight in PDT.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

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

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch rv2.2 (for checking-in)

I'll land soon.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

checked-in.

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

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

-> FIXED