Rename LayerComposite::GetShadowTransform to GetShadowBaseTransform

RESOLVED FIXED in Firefox 47

Status

()

Core
Graphics: Layers
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: botond, Assigned: matthewstroud101, Mentored)

Tracking

(Blocks: 1 bug)

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [gfx-noted] [lang=c++])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

2 years ago
This makes it clear that this function is more like GetBaseTransform() than GetTransform(), in that it does not include the pre- and post-scale.

We've gotten ourselves confused by this a number of times (see e.g. bug 1208829 comment 28).
(Assignee)

Comment 1

2 years ago
I would like to attempt this as a first bug if possible. Thanks
(Reporter)

Comment 2

2 years ago
Sure!

As a first step, could you do a local checkout and build of the codebase, as described here [1]?

Once you've done that, let me know and we can get started on the bug itself. Thanks!

[1] https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
(Assignee)

Comment 3

2 years ago
I have the build ready now. What would be the next step?
(Reporter)

Comment 4

2 years ago
Great!

You can use our code indexing tool, DXR, to find existing occurrences of Layer::GetShadowTransform() [1], and change each one to GetShadowBaseTransform(). (That search query also shows results for a different function, GetShadowTransformSetByAnimation() - you'll want to skip those.)

Once you've made the changes to your local checkout, rebuild to make sure the updated code builds successfully.

Once you verified the code builds successfully, you can commit your changes to your local repository clone, with a commit message that follows the conventions described here [2].

Finally, you can submit your patch in one of two ways:

  1) The old way:
     Export the patch as a file, as described here [3].
     Upload it as an attachment to the bug, and flag me for review.

  2) The new way:
     Submit your patch to our new review system, MozReview [4].
     (Note that when you get to the end of this part of the instructions [5],
     you'll want to choose HTTPS rather than SSH.)

I'll leave it up to you which submission method you prefer.

Let me know if you run into any issues!

[1] https://dxr.mozilla.org/mozilla-central/search?q=GetShadowTransform
[2] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions
[3] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
[4] http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview-user.html
[5] http://mozilla-version-control-tools.readthedocs.org/en/latest/mozreview/install-mercurial.html#mozreview-install-mercurial
(Assignee)

Comment 5

2 years ago
Created attachment 8719679 [details] [diff] [review]
This patch changes GetShadowTransform to GetShadowBaseTransform

Ok, so here is my patch. The only thing that I was not able to figure out was the commit message. For whatever reason, when I ran the command hg qnew name.patch, it never opened the text editor for me to type in the commit message.
Attachment #8719679 - Flags: review?(botond)
(Reporter)

Comment 6

2 years ago
(In reply to matthewstroud101 from comment #5)
> Ok, so here is my patch. The only thing that I was not able to figure out
> was the commit message. For whatever reason, when I ran the command hg qnew
> name.patch, it never opened the text editor for me to type in the commit
> message.

It looks like you need to pass the '-e' option to qnew for it to open an editor to type the commit message, or you can provide the message right there on the command line with the '-m' option.

(I myself don't use mq; I prefer to just use 'commit' to make commits, 'amend' to change my last commit, and (occasionally) 'histedit' to re-order commits or change a commit other than the last.)
(Reporter)

Comment 7

2 years ago
Comment on attachment 8719679 [details] [diff] [review]
This patch changes GetShadowTransform to GetShadowBaseTransform

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

This is on the right track, there are just a couple of issues:

  - We don't want to rename the GetShadowTransformSetByAnimation() function,
    just GetShadowTransform().

  - The "mozconfig" file should not be included in the patch - it's for
    your local use only.

Could you upload a revised version of the patch that addresses these? Thanks!
Attachment #8719679 - Flags: review?(botond) → feedback+
(Reporter)

Comment 8

2 years ago
If you're wondering how to remove the "mozconfig" file from your patch, here's a way:

  - Run "hg forget mozconfig" to tell mercurial to stop tracking the file.
    (Since it's a local configuration file, we don't want it to be part of 
    any commit.)

  - Run "hg qrefresh" to update the patch to exclude the (now untracked)
    file.
(Assignee)

Comment 9

2 years ago
Created attachment 8721096 [details] [diff] [review]
This is a new patch that changes GetShadowTransform to GetShadowBaseTransform

So, I have been trying a lot of things, but I could not figure out how to update the patch. So, I made a new one instead.
Attachment #8719679 - Attachment is obsolete: true
Attachment #8721096 - Flags: review?(botond)
(Reporter)

Comment 10

2 years ago
Matthew, I'm sorry to hear about your version control troubles.

For future reference, here is the simple version control workflow I recommend:

  <make changes to your working directory>
  hg commit
  <post changeset for review, either using MozReview or using 'hg export' to get a patch file>
  <to address review comments, make more changes to your working directory>
  hg commit --amend    # this folds the changes you just made into the commit
  <post updated changeset for review>
  <rinse and repeat>
(Reporter)

Comment 11

2 years ago
(In reply to Botond Ballo [:botond] from comment #7)
> Comment on attachment 8719679 [details] [diff] [review]
> 
>   - The "mozconfig" file should not be included in the patch - it's for
>     your local use only.

Oh, whoops! When reviewing your original patch, I overlooked the fact that you weren't *adding* a file named "mozconfig" - you were modifying an existing file, browser/config/mozconfig.

You're not supposed to modify this file. Rather, your local configuration options are meant to go into a file you add, called "mozconfig" or ".mozconfig", in the root directory of the source checkout. This file is then ignored by the version control system.

Given that you modified browser/config/mozconfig, this advice:

(In reply to Botond Ballo [:botond] from comment #8)
>   - Run "hg forget mozconfig" to tell mercurial to stop tracking the file.
>     (Since it's a local configuration file, we don't want it to be part of 
>     any commit.)

was wrong. My apologies for that!

It appears you followed this advice (probably modified to run "hg forget browser/config/mozconfig"), and as a result the browser/config/mozconfig file shows up as being removed by your patch. We'll need to fix this.
(Reporter)

Comment 12

2 years ago
Comment on attachment 8721096 [details] [diff] [review]
This is a new patch that changes GetShadowTransform to GetShadowBaseTransform

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

This looks good, exception for the browser/config/mozconfig file being removed. Let me think what is the simplest way to fix this.
Attachment #8721096 - Flags: review?(botond) → feedback+
(Reporter)

Comment 13

2 years ago
(In reply to Botond Ballo [:botond] from comment #12)
> This looks good, exception for the browser/config/mozconfig file being
> removed. Let me think what is the simplest way to fix this.

So, if you ran "hg forget browser/config/mozconfig" and didn't do anything further (like removing the file browser/config/mozconfig from the filesystem), the file you still be there, and you should just need to add it again:

  hg add browser/config/mozconfig

After that, you should be able to update your commit with this change, using "hg commit --amend" or "hg qrefresh".

Let me know if you run into any trouble!
(Assignee)

Comment 14

2 years ago
Created attachment 8721718 [details] [diff] [review]
shadow layer patch with mozconfig added back in

So, I ran the command to add the mozconfig back in, and then updated the commit using hg qrefresh.
Attachment #8721096 - Attachment is obsolete: true
Attachment #8721718 - Flags: review?(botond)
(Reporter)

Comment 15

2 years ago
Comment on attachment 8721718 [details] [diff] [review]
shadow layer patch with mozconfig added back in

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

Thanks!
Attachment #8721718 - Flags: review?(botond) → review+
(Reporter)

Comment 16

2 years ago
Comment on attachment 8721718 [details] [diff] [review]
shadow layer patch with mozconfig added back in

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

Only thing missing now is a commit message. Could you add one, please, by running:

  hg qrefresh -m "message"

(without having to make any other changes first)? Thanks!
Attachment #8721718 - Flags: review+ → feedback+
(Assignee)

Comment 17

2 years ago
Created attachment 8721725 [details] [diff] [review]
Updated patch with commit message

Ok, so I added the commit message. Hopefully this is finally correct.
Attachment #8721718 - Attachment is obsolete: true
Attachment #8721725 - Flags: review?(botond)
(Reporter)

Comment 18

2 years ago
Comment on attachment 8721725 [details] [diff] [review]
Updated patch with commit message

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

There's a typo in the bug number, but I can fix that for you :)

Thanks!
Attachment #8721725 - Flags: review?(botond) → review+
(Reporter)

Comment 19

2 years ago
Here's a build-only Try push to make sure the patch compiles on all platforms:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e84aecea8f3
(Assignee)

Comment 20

2 years ago
Thanks for your help. I am taking an Open Source Development class at Western Oregon University. It took me a while to get my set-up to be able to start on bugs, but I finally got it, and recognized a bug that I could do. I do have a C++ background, so if there are any other ones that I could try to tackle, that would be cool.
(Reporter)

Updated

2 years ago
Summary: Rename Layer::GetShadowTransform to GetShadowBaseTransform → Rename LayerComposite::GetShadowTransform to GetShadowBaseTransform
(Reporter)

Updated

2 years ago
Blocks: 1249936
(Reporter)

Comment 21

2 years ago
(In reply to matthewstroud101 from comment #20)
> Thanks for your help. I am taking an Open Source Development class at
> Western Oregon University. It took me a while to get my set-up to be able to
> start on bugs, but I finally got it, and recognized a bug that I could do. I
> do have a C++ background, so if there are any other ones that I could try to
> tackle, that would be cool.

There's a follow-up change to this that's only slightly more involved, bug 1249936 - you might be interested in it.
(Reporter)

Comment 22

2 years ago
Oh, and I should have assigned this to you when you started working on it :)
Assignee: nobody → matthewstroud101
(Reporter)

Updated

2 years ago
Blocks: 1249937
(Reporter)

Comment 23

2 years ago
Oh, and I suppose we should also rename SetShadowTransform(), for consistency. I filed bug 1249937 for that; feel free to take that too, if you'd like, otherwise I'll mark it as a [good first bug] and let someone else give it a try.
(Assignee)

Comment 24

2 years ago
I will probably take on bug 1249936, and let somebody else cover bug 1249937. I will probably let some of my classmates know about it because some of them are having a hard time finding bugs.
(Reporter)

Comment 25

2 years ago
(In reply to Botond Ballo [:botond] from comment #19)
> Here's a build-only Try push to make sure the patch compiles on all
> platforms:
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=0e84aecea8f3

Looking good! Landed on mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8cc5e038a0c7

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8cc5e038a0c7
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.