1249936 - Add a LayerComposite::GetShadowTransform() method which returns the shadow base transform multiplied by the scales

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond] [away until Oct 19]]

Now that bug 1247450 has renamed LayerComposite::GetShadowTransform() to GetShadowBaseTransform() to indicate that it does not include the pre- and post-scales, we can add a method LayerComposite::GetShadowTransform() which returns the transform that does include the pre- and post-scales.

The implementation of this method would look very similar to the implementation of Layer::GetTransform(), except instead of using Layer::mTransform as the starting point, it would use LayerComposite::mShadowTransform.

Having done this, we can also simplify the implementation of Layer::GetLocalTransform() - we can have it return either LayerComposite::GetShadowTransform(), or Layer::GetTransform(), depending on whether the layer is a LayerComposite, and not multiply by the scales itself.

Comment 1

[matthewstroud101]

I will take on this bug.

Comment 2

[Botond Ballo [:botond] [away until Oct 19]]

Thanks! I assigned the bug to you.

Let me know if you have any questions!

Comment 3

[matthewstroud101]

One question would be a good IDE? One that is capable of running decently on a Virtual Machine?

Comment 4

[Botond Ballo [:botond] [away until Oct 19]]

(In reply to matthewstroud101 from comment #3)
> One question would be a good IDE? One that is capable of running decently on
> a Virtual Machine?

I'd say it's mostly a matter of taste. I use Eclipse CDT, and I find it works well, including on a virtual machine (although it's a bit of a memory hog, so it depends how much memory the virtual machine has). I know others who use Visual Studio or Xcode, or editors that aren't IDEs like Sublime Text or vim.

If you'd like to try Eclipse, our build system has a command that generates an Eclipse project suitable for editing mozilla-central code:

  ./mach build-backend -b CppEclipse

(There is a similar command for creating a Visual Studio project, too.)

Comment 5

[Botond Ballo [:botond] [away until Oct 19]]

Matthew, are you still working on this? If you're stuck on anything, I'm happy to help!

Comment 6

[matthewstroud101]

I became pretty busy with school since I asked to work on this bug. Right now, I feel like I do not have the time to work on it until I graduate in June. I am still interested in working on bug patches for Mozilla, but for now, if somebody else wants to work on this, then they should be able to.

Comment 7

[Botond Ballo [:botond] [away until Oct 19]]

That's no problem. I'll un-assign the bug from you, so others know it's available to be worked on.

Comment 8

[B. Dahse]

Attachment: add a GetShadowTransform method

Sorry about the "whitespace noise", I guess my editor did that when I saved.

Comment 9

[B. Dahse]

Attachment: simplify GetLocalTransform

Comment 10

[Botond Ballo [:botond] [away until Oct 19]]

Comment on attachment 8747428 [details] [diff] [review]
add a GetShadowTransform method

Review of attachment 8747428 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, this looks good!

(Normally, I'd ask for the 'const' to be removed from the return type, because I don't see any point in that when returning by value, but I see Layer::GetTransform() has it as well, so let's just remain consistent with that.)

The only other thing required to land the patch is a commit message, of the form "Bug <bugnumber> - <description>. r=<reviewer>". Could you upload a new version of the patch that includes that? The reviewer would be me ("botond"). Thanks!

Comment 11

[Botond Ballo [:botond] [away until Oct 19]]

Comment on attachment 8747429 [details] [diff] [review]
simplify GetLocalTransform

Review of attachment 8747429 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks!

As with the other patch, please add a commit message.

Comment 12

[B. Dahse]

(In reply to Botond Ballo [:botond] from comment #10)
> Comment on attachment 8747428 [details] [diff] [review]
> add a GetShadowTransform method
> 
> Review of attachment 8747428 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (Normally, I'd ask for the 'const' to be removed from the return type,
> because I don't see any point in that when returning by value, but I see
> Layer::GetTransform() has it as well, so let's just remain consistent with
> that.)
> 

I was confused about the decision to make Layer::GetTransform return a const value as well. The only thing I could think of, is if for some reason you want to prevent the codebase doing something like GetTransform().SomeNonConstFunction(); but even so that is rather pointless as one can just store the return value in a non-const value and then due a non-const operation on it. Of course it makes perfect sense if we were returning by reference like GetShadowBaseTransform.

Comment 13

[B. Dahse]

Attachment: add a GetShadowTransform method & simplify GetLocalTransform, 1st revision

Comment 14

[B. Dahse]

Attachment: add a GetShadowTransform method & simplify GetLocalTransform, 2nd revision

No code changes, just messed up last time in generating the patch in the format expected.

Comment 15

[B. Dahse]

Forgot to request review from you in BugZilla.

Comment 16

[Botond Ballo [:botond] [away until Oct 19]]

Comment on attachment 8747510 [details] [diff] [review]
add a GetShadowTransform method & simplify GetLocalTransform, 2nd revision

Review of attachment 8747510 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 17

[Botond Ballo [:botond] [away until Oct 19]]

Here's a build-only Try push with the patch applied, to make sure it builds on all platforms:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=337e6e49b5ac

Comment 18

[Botond Ballo [:botond] [away until Oct 19]]

(In reply to bd339 from comment #12)
> (In reply to Botond Ballo [:botond] from comment #10)
> > (Normally, I'd ask for the 'const' to be removed from the return type,
> > because I don't see any point in that when returning by value, but I see
> > Layer::GetTransform() has it as well, so let's just remain consistent with
> > that.)
> 
> I was confused about the decision to make Layer::GetTransform return a const
> value as well. The only thing I could think of, is if for some reason you
> want to prevent the codebase doing something like
> GetTransform().SomeNonConstFunction(); but even so that is rather pointless
> as one can just store the return value in a non-const value and then due a
> non-const operation on it. Of course it makes perfect sense if we were
> returning by reference like GetShadowBaseTransform.

If you'd like to write a follow-up patch to remove the const from the return types of each of Layer::GetTransform(), Layer::GetLocalTransform(), and LayerComposite::GetShadowTransform(), I would review that.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b7c287c6c05f

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7c287c6c05f

Comment 21

[B. Dahse]

(In reply to Botond Ballo [:botond] from comment #18)
> If you'd like to write a follow-up patch to remove the const from the return
> types of each of Layer::GetTransform(), Layer::GetLocalTransform(), and
> LayerComposite::GetShadowTransform(), I would review that.

Yes, I would like to do that.

Comment 22

[B. Dahse]

Should such a follow-up patch be submitted against a new bug or this one?

Comment 23

[Botond Ballo [:botond] [away until Oct 19]]

(In reply to bd339 from comment #22)
> Should such a follow-up patch be submitted against a new bug or this one?

Might as well do it in this bug.

Comment 24

[B. Dahse]

Attachment: remove unnecessary const from functions

Comment 25

[Botond Ballo [:botond] [away until Oct 19]]

Comment on attachment 8748171 [details] [diff] [review]
remove unnecessary const from functions

Review of attachment 8748171 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 26

[Botond Ballo [:botond] [away until Oct 19]]

Try push for second patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=221f3296e3ac

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/814ea77366d2

Comment 28

[Botond Ballo [:botond] [away until Oct 19]]

(In reply to Pulsebot from comment #27)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/814ea77366d2

^ Landed the second patch.

bd339, thanks for your contributions in this bug! If you're interesting in working on more bugs, and would like some suggestions, let me know.

Comment 29

[B. Dahse]

(In reply to Botond Ballo [:botond] from comment #28)
> bd339, thanks for your contributions in this bug! If you're interesting in
> working on more bugs, and would like some suggestions, let me know.

I am very much interested.

Comment 30

[Botond Ballo [:botond] [away until Oct 19]]

(In reply to bd339 from comment #29)
> (In reply to Botond Ballo [:botond] from comment #28)
> > bd339, thanks for your contributions in this bug! If you're interesting in
> > working on more bugs, and would like some suggestions, let me know.
> 
> I am very much interested.

From among the bugs I'm mentoring, bug 1170062 might be a good one for you.

Of course, there are also bugs others are mentoring, I'm just not familiar enough with them to recommend specific ones; http://www.joshmatthews.net/bugsahoy/ is a good tool for picking one.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/814ea77366d2