1074128 - Add support to Moz2D's AutoSaveTransform for setting the DrawTarget lazily

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jonathan Watt [:jwatt]]

In some functions we only set the current transform under certain |if| blocks. In the case that we don't hit those blocks it will give us better performance to avoid restoring the transform that the DrawTarget currently has.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: part 1 - Add support to Moz2D's AutoSaveTransform for setting the DrawTarget lazily

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: part 2 - Rename Moz2D's AutoSaveTransform to AutoRestoreTransform

I'd much rather this class were called AutoRestoreTransform.

It irks me that the name is AutoSaveTransform (Save), when the important action that this thing takes is the _restoring_ of the transform (obviously the saving is implicit if it then goes on to restore).

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: part 3 - Update the Mozilla code to use AutoSaveTransform's new name

Comment 4

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/841bb208fd83
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f3df060d475
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b3954b323c6

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/841bb208fd83
https://hg.mozilla.org/mozilla-central/rev/4f3df060d475
https://hg.mozilla.org/mozilla-central/rev/3b3954b323c6

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jonathan Watt [:jwatt] from comment #4)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/841bb208fd83
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4f3df060d475
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3b3954b323c6

It would be better to squash part 2 and part 3 into a single commit so that the build is not broken inbetween the two for people doing bisections.

Comment 7

[Jonathan Watt [:jwatt]]

Hmm, Bas specifically likes them separate to make Moz2D merging less of a headache. Is this an issue for regression finding tools? How about I add DONTBUILD to the commit doing the Moz2D changes?

Comment 8

[Jeff Muizelaar [:jrmuizel]]

(In reply to Jonathan Watt [:jwatt] from comment #7)
> Hmm, Bas specifically likes them separate to make Moz2D merging less of a
> headache. Is this an issue for regression finding tools? How about I add
> DONTBUILD to the commit doing the Moz2D changes?

Just regular hg bisect or git bisect won't pay attention to DONTBUILD. Bas, can you just filter out the non-moz2d part? What's your workflow for bringing changes into Moz2D?

Comment 9

[Bas Schouten (:bas.schouten)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #8)
> (In reply to Jonathan Watt [:jwatt] from comment #7)
> > Hmm, Bas specifically likes them separate to make Moz2D merging less of a
> > headache. Is this an issue for regression finding tools? How about I add
> > DONTBUILD to the commit doing the Moz2D changes?
> 
> Just regular hg bisect or git bisect won't pay attention to DONTBUILD. Bas,
> can you just filter out the non-moz2d part? What's your workflow for
> bringing changes into Moz2D?

I have migration scripts. And I think Moz2D changes should be filtered out. I've suggested better ways of doing this by having a separate repository that's linked to (but I believe there's practical concerns there), or importing changes from Moz2D into central in separate commits. Neither of those were deemed desirable. Without that, this is for me a much preferred approach. Since I think fundamentally a Moz2D change should be considered atomically from the changes in central. Fwiw, there's changesets that don't compile all the time on central (due to bustages with fixing changesets and such), so I don't think it's actually a big issue.

Comment 10

[Bas Schouten (:bas.schouten)]

Fwiw, I'm fine with doing this in 3 commits, one adding the new function, another changing m-c, another removing the old one from Moz2D. That would be much like what you'd do if you had longer standing migrations to a new API. But it seems a little cumbersome in this case.