Draft. Change PaintContext rects to TypedRects #7023 #7696

mskrzypkows · 2015-09-21T13:22:41Z

I created draft. I'm not sure if we need any units conversion in PaintContext. There is also strange 'clear' method, we use PagePx origin and ScreenPx size is it OK?

jdm · 2015-09-21T16:48:32Z

r? @mbrubeck

bors-servo · 2015-09-28T18:47:30Z

☔ The latest upstream changes (presumably #7596) made this pull request unmergeable. Please resolve the merge conflicts.

mbrubeck · 2015-09-28T21:30:11Z

Thanks for the patch! Here are a few comments...

-S-awaiting-review +S-needs-code-changes

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

^{components/gfx/display_list/mod.rs, line 486 [r1] (raw file):}
This could be let tile_size = paint_context.screen_rect.as_f32().to_untyped();

^{components/gfx/paint_context.rs, line 95 [r1] (raw file):}
This should return a ScaleFactor<PagePx, ScreenPx>. This could also benefit from servo/euclid#105 (requires updating the version of euclid used by Servo).

^{components/gfx/paint_context.rs, line 244 [r1] (raw file):}
Should page_rect be screen_rect here?

Also, I'm surprised that we are combining these different coordinates like this without scaling them. This may be a bug in the existing code.

Comments from the review on Reviewable.io

mskrzypkows · 2015-09-30T09:33:22Z

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

^{components/gfx/paint_context.rs, line 95 [r1] (raw file):}
As I see newest version (0.2) is already used. Will there be update to other?
If I change screen_pixels_per_px to return ScaleFactor then I have to change implementation of to_nearest_pixel in geometry.rs, to use ScaleFactor, right?

^{components/gfx/paint_context.rs, line 244 [r1] (raw file):}
I'll change to screen_rect

Comments from the review on Reviewable.io

mbrubeck · 2015-09-30T15:35:09Z

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

^{components/gfx/paint_context.rs, line 95 [r1] (raw file):}
I have now published euclid 0.2.1.

Yes, if you change this then the to_nearest_pixel should change to take ScaleFactor. Feel free to leave this for a separate PR if you'd like.

Comments from the review on Reviewable.io

mbrubeck · 2015-10-07T20:44:41Z

Oops, I didn't notice that my earlier attempt to publish euclid 0.2.1 failed. Now it has been published successfully, and you can run ./mach cargo-update -p euclid to update it in all of our Cargo.lock files.

mskrzypkows · 2015-10-08T06:28:02Z

OK, thanks :-)

bors-servo · 2015-10-28T18:44:54Z

☔ The latest upstream changes (presumably #8242) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2015-10-28T18:56:36Z

Oops, this was rebased but also had build errors according to the travis log:

/home/travis/build/servo/servo/components/gfx/paint_context.rs:1647:46: 1647:59 error: mismatched types:

 expected `f32`,

    found `euclid::scale_factor::ScaleFactor<util::geometry::PagePx, util::geometry::ScreenPx, f32>`

(expected f32,

    found struct `euclid::scale_factor::ScaleFactor`) [E0308]

/home/travis/build/servo/servo/components/gfx/paint_context.rs:1647         Point2D::new(self.x.to_nearest_pixel(pixels_per_px) as AzFloat,

                                                                                                                 ^~~~~~~~~~~~~

mskrzypkows · 2015-10-29T07:49:14Z

Yes, I know that there are build errors, it's because my changes need update in app_unit module, to_nearest_pixel method should take ScaleFactor<PagePx, ScreenPx, f32> as argument. I don't know how to modify app_unit, because PagePx and ScreenPx are defined in util::geometry module which is not available from app_unit. How can I use PagePx and ScreenPx from app_unit?

jdm · 2015-10-29T11:24:04Z

It sounds like perhaps we should define PagePx and ScreenPx in app_unit instead.

mbrubeck · 2015-10-29T17:26:53Z

If you'd like to land the work you've done so far, you could split the changes that depend on app_unit into a separate commit to be merged later.

mskrzypkows · 2015-10-30T08:15:08Z

OK, I'll push another version.

bors-servo · 2015-11-04T03:38:41Z

☔ The latest upstream changes (presumably #8306) made this pull request unmergeable. Please resolve the merge conflicts.


        Change PaintContext rects to TypedRects #7023

Needs update to_nearest_pixel method in app_unit module. Argument of to_nearest_pixel should be: ScaleFactor<PagePx, ScreenPx, f32>

mskrzypkows · 2015-11-06T07:30:43Z

Rebased, @mbrubeck what do you think? Could you do a review?

mbrubeck · 2015-11-06T16:14:21Z

Reviewed 3 of 3 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

mbrubeck · 2015-11-06T16:14:44Z

@bors-servo r+

Thanks!

bors-servo · 2015-11-06T16:14:45Z

📌 Commit 2a7927d has been approved by mbrubeck

bors-servo · 2015-11-06T17:00:41Z

⌛ Testing commit 2a7927d with merge 96a9653...


        Auto merge of #7696 - mskrzypkows:PaintContext_Units, r=mbrubeck

Draft. Change PaintContext rects to TypedRects #7023 I created draft. I'm not sure if we need any units conversion in PaintContext. There is also strange 'clear' method, we use PagePx origin and ScreenPx size is it OK?  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7696)

bors-servo · 2015-11-06T18:06:57Z

💔 Test failed - mac-rel-wpt

mbrubeck · 2015-11-06T18:12:03Z

@bors-servo retry

#7787

bors-servo · 2015-11-06T18:53:00Z

⌛ Testing commit 2a7927d with merge 8d8ffa8...


        Auto merge of #7696 - mskrzypkows:PaintContext_Units, r=mbrubeck

Draft. Change PaintContext rects to TypedRects #7023 I created draft. I'm not sure if we need any units conversion in PaintContext. There is also strange 'clear' method, we use PagePx origin and ScreenPx size is it OK?  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7696)

bors-servo · 2015-11-06T19:33:51Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review label Sep 21, 2015

highfive assigned mbrubeck Sep 21, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Sep 28, 2015

mbrubeck added S-needs-code-changes and removed S-awaiting-review labels Sep 28, 2015

mskrzypkows force-pushed the mskrzypkows:PaintContext_Units branch from 55a3c0c to 82623dc Oct 28, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 28, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Oct 28, 2015

jdm added S-needs-code-changes and removed S-awaiting-review labels Oct 28, 2015

mskrzypkows force-pushed the mskrzypkows:PaintContext_Units branch from 82623dc to bd61308 Oct 30, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 30, 2015

jdm removed the S-needs-rebase label Oct 30, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Nov 4, 2015

Change PaintContext rects to TypedRects #7023

Loading status checks…

2a7927d

Needs update to_nearest_pixel method in app_unit module. Argument of to_nearest_pixel should be: ScaleFactor<PagePx, ScreenPx, f32>

mskrzypkows force-pushed the mskrzypkows:PaintContext_Units branch from bd61308 to 2a7927d Nov 4, 2015

jdm removed the S-needs-rebase label Nov 6, 2015

highfive added S-awaiting-merge and removed S-awaiting-review labels Nov 6, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Nov 6, 2015

highfive added S-awaiting-merge and removed S-tests-failed labels Nov 6, 2015

servo / servo

Draft. Change PaintContext rects to TypedRects #7023 #7696

Draft. Change PaintContext rects to TypedRects #7023 #7696

mskrzypkows commented Sep 21, 2015

jdm commented Sep 21, 2015

bors-servo commented Sep 28, 2015

mbrubeck commented Sep 28, 2015

mskrzypkows commented Sep 30, 2015

mbrubeck commented Sep 30, 2015

mbrubeck commented Oct 7, 2015

mskrzypkows commented Oct 8, 2015

bors-servo commented Oct 28, 2015

jdm commented Oct 28, 2015

mskrzypkows commented Oct 29, 2015

jdm commented Oct 29, 2015

mbrubeck commented Oct 29, 2015

mskrzypkows commented Oct 30, 2015

bors-servo commented Nov 4, 2015

mskrzypkows commented Nov 6, 2015

mbrubeck commented Nov 6, 2015

mbrubeck commented Nov 6, 2015

bors-servo commented Nov 6, 2015

bors-servo commented Nov 6, 2015

bors-servo commented Nov 6, 2015

mbrubeck commented Nov 6, 2015

bors-servo commented Nov 6, 2015

bors-servo commented Nov 6, 2015

servo / servo

Join GitHub today

Draft. Change PaintContext rects to TypedRects #7023 #7696

Draft. Change PaintContext rects to TypedRects #7023 #7696

Conversation

mskrzypkows commented Sep 21, 2015

jdm commented Sep 21, 2015

bors-servo commented Sep 28, 2015

mbrubeck commented Sep 28, 2015

mskrzypkows commented Sep 30, 2015

mbrubeck commented Sep 30, 2015

mbrubeck commented Oct 7, 2015

mskrzypkows commented Oct 8, 2015

bors-servo commented Oct 28, 2015

jdm commented Oct 28, 2015

mskrzypkows commented Oct 29, 2015

jdm commented Oct 29, 2015

mbrubeck commented Oct 29, 2015

mskrzypkows commented Oct 30, 2015

bors-servo commented Nov 4, 2015

mskrzypkows commented Nov 6, 2015

mbrubeck commented Nov 6, 2015

mbrubeck commented Nov 6, 2015

bors-servo commented Nov 6, 2015

bors-servo commented Nov 6, 2015

bors-servo commented Nov 6, 2015

mbrubeck commented Nov 6, 2015

bors-servo commented Nov 6, 2015

bors-servo commented Nov 6, 2015