Closed Bug 392252 Opened 13 years ago Closed 13 years ago

Need new APIs which convert AppUnits to pixels for gfx

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Attachment #277374 - Attachment is patch: true
Attachment #277374 - Flags: superreview?(bzbarsky)
Attachment #277374 - Flags: review?(bzbarsky)
Attached patch Patch rv1.1 (obsolete) — Splinter Review
er, I forgot to change the non-my codes.
Attachment #277374 - Attachment is obsolete: true
Attachment #277377 - Flags: superreview?(bzbarsky)
Attachment #277377 - Flags: review?(bzbarsky)
Attachment #277374 - Flags: superreview?(bzbarsky)
Attachment #277374 - Flags: review?(bzbarsky)
Comment on attachment 277377 [details] [diff] [review]
Patch rv1.1

This should get review from Eli first.
Attachment #277377 - Flags: review?(bzbarsky) → review?(sharparrow1)
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.
Attached patch Patch rv2.0 (obsolete) — Splinter Review
thanks. I changed as so.
Attachment #277377 - Attachment is obsolete: true
Attachment #277615 - Flags: review?(sharparrow1)
Attachment #277377 - Flags: superreview?(bzbarsky)
Attachment #277377 - Flags: review?(sharparrow1)
Are all those Makefile changes supposed to be part of this patch?
Yes. gfxFloat is defined in thebes.
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.
Attachment #277615 - Flags: review?(sharparrow1) → review+
(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??
(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.
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.
Attached patch Patch rv2.1 (obsolete) — Splinter Review
changed nsIDeviceContext too.

I'll check-in midnight in PDT.
Attachment #277615 - Attachment is obsolete: true
Attachment #277647 - Flags: superreview?(roc)
Attachment #277647 - Flags: review+
Attachment #277647 - Flags: approval1.9?
Comment on attachment 277647 [details] [diff] [review]
Patch rv2.1

"GfxCoords" is not a good phrase; there is only one coordinate. Say "GfxUnits" instead.
Attachment #277647 - Flags: superreview?(roc)
Attachment #277647 - Flags: superreview+
Attachment #277647 - Flags: approval1.9?
Attachment #277647 - Flags: approval1.9+
I'll land soon.
Attachment #277647 - Attachment is obsolete: true
checked-in.

I'll check-in the additional patch if some machines are red.
Summary: Need |NSAppUnitsToDoublePixels| API → Need new APIs which convert AppUnits to pixels for gfx
-> FIXED
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.