Closed Bug 1255006 Opened 8 years ago Closed 8 years ago

Displayports set via a rect should take priority over "suppressed margins" displayport

Categories

(Core :: Panning and Zooming, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: kats, Assigned: xfftlfanx, Mentored)

References

Details

(Keywords: correctness, Whiteboard: [gfx-noted][good first bug][lang=c++])

Attachments

(1 file, 2 obsolete files)

The code at http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?rev=565778f033af#1072 implements displayport suppression by assuming a 0-margin displayport. However, that check happens before we check for "rect" based displayports (which are only used in tests) but which should be taking priority. In other words, if a test is specifying a rect to use as the displayport, then displayport suppression should not override that and use 0-margin instead. Only if we are using margins-based displayport should the suppression kick in.

I don't think this change will have any impact on current behaviour or testing outcomes, because AFAIK none of the tests that set the displayport as a rect use displayport suppression. But it's a potential footgun that we should fix.
Mentor: bugmail.mozilla
Whiteboard: [good first bug][lang=c++]
Whiteboard: [good first bug][lang=c++] → [gfx-noted][good first bug][lang=c++]
I want to make this my first bug fix, but don't know how to get started. Can someone help me?
Flags: needinfo?(bugmail.mozilla)
Hi Roman,

The first thing to do is to check out the code and get a build of Firefox. There are instructions on how to do that at [1]. Once you have that successfully completed, you can start modifying the code - in this case the code is linked in comment 0, and you just need to reverse the order of the first two conditions in that if/elseif/else block. You can generate a patch with your changes using the instructions at [2] and attach it to this bug. Let me know if you run into any problems or questions!

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
[2] 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
Flags: needinfo?(bugmail.mozilla)
Attached patch name.patch (obsolete) — Splinter Review
I have attached the patch for review here.
Comment on attachment 8731531 [details] [diff] [review]
name.patch

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

Nice, thanks! The last step once you attach the patch is to flag me for review. You can do this on the attachment upload or attachment details page - change the "review" flag to "?" and type ":kats" into the text field that appears. I'll review this version of the patch anyway; for the next version please flag me for review.

There's some minor comments about whitespace below; otherwise the patch looks fine. Also please update the commit message to something like "Bug 1255006 - Ensure the displayport rect takes priority over a suppressed-margins displayport." as it is more meaningful with regards to the purpose of the patch (don't worry, I don't expect you to understand what this code is doing).

Once you make the changes, please upload the updated patch as a new attachment and flag me for review on it. Thanks!

::: layout/base/nsLayoutUtils.cpp
@@ +1075,5 @@
>    NS_ASSERTION((rectData == nullptr) != (marginsData == nullptr),
>                 "Only one of rectData or marginsData should be set!");
>  
>    nsRect result;
> +  if (rectData) { 

There's a trailing whitespace on this line, please remove it.

@@ +1077,5 @@
>  
>    nsRect result;
> +  if (rectData) { 
> +    result = GetDisplayPortFromRectData(aContent, rectData, aMultiplier);
> +  }else if (APZCCallbackHelper::IsDisplayportSuppressed()) {

Please put a space between the '}' and the 'else'

@@ +1082,3 @@
>      DisplayPortMarginsPropertyData noMargins(ScreenMargin(), 1);
>      result = GetDisplayPortFromMarginsData(aContent, &noMargins, aMultiplier);
> +  }else {

Same here - space between '}' and 'else'
Assignee: nobody → xfftlfanx
Keywords: correctness
OS: Unspecified → All
Hardware: Unspecified → All
Version: unspecified → Trunk
Attached patch bug1255006.patch (obsolete) — Splinter Review
I have updated the patch.
Attachment #8731868 - Flags: review?(bugmail.mozilla)
Comment on attachment 8731868 [details] [diff] [review]
bug1255006.patch

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

This second patch applies on top of the first patch, and corrects all the issues I pointed out. Thanks! But really what we want for landing is a single patch that combines the two changes - sorry if I was unclear on that. If you're using hg patch queues you can accomplish this using the |hg qfold| command to merge two patches (followed by |hg qref -e| to fix up the commit message if needed). Normally I wouldn't mind doing this for you but since the purpose of a good-first-bug is to learn the ropes it would be good for you to try doing this so you get the hang of it. :)
Attachment #8731868 - Flags: review?(bugmail.mozilla)
I tried this: 
[myprofile]:~/mozilla-central/.hg/patches$ hg qfold bug1255006.patch name.patch
skipping already folded patch bug1255006.patch
abort: qfold cannot fold already applied patch bug1255006.patch

I'm not familiar with hg, so I'm not sure what an applied patch is. How would I un-apply a patch?
Flags: needinfo?(bugmail.mozilla)
Applied patches are patches in your local queue that have been applied to your work tree. So for example if you have a patch that makes changes to nsLayoutUtils.cpp, as in this case, and it is "applied", then your local nsLayoutUtils.cpp file will have those changes. If you unapply the patch, your local nsLayoutUtils.cpp file will not have the changes; it will be restored to the previous version, and your patch will remain as an unapplied patch in the queue. You can use |hg qseries| to see which patches are in the queue, and which of those have been applied.

From your output above it sounds like you have the two patches, "bug1255006.patch" and "name.patch", and you tried folding them together while they were both applied. However what qfold does is "fold the named patches into the current patch" (see |hg help qfold|) so what you want to do is make "bug1255006.patch" the "current patch" and then use |hg qfold| to fold "name.patch" into it. You can do this by running |hg qpop| which will unapply the top patch (which should be "name.patch"). You can verify this by running |hg qseries| again, it should show one patch applied and one unapplied. Then you can run |hg qfold name.patch| and it should fold "name.patch" into the current patch which should be "bug1255006.patch" at that point.

Give that a shot and let me know how it goes. Thanks!
Flags: needinfo?(bugmail.mozilla)
Attached patch bug1255006.patchSplinter Review
I folded them.
Attachment #8733125 - Flags: review?(bugmail.mozilla)
Comment on attachment 8733125 [details] [diff] [review]
bug1255006.patch

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

Perfect, thanks! I'll land this patch shortly. Also just a note for the future, when uploading a new patch that replaces an older one, you mark the older patches "obsolete" - there is a checkbox on the attachment upload page to do this. Normally in Mozilla workflow we do this when uploading a new version of the patch so that people don't get confused as to which version is the latest one. The older ones are still accessible in Bugzilla.
Attachment #8733125 - Flags: review?(bugmail.mozilla) → review+
Attachment #8731531 - Attachment is obsolete: true
Attachment #8731868 - Attachment is obsolete: true
Thanks a lot for the help!
https://hg.mozilla.org/mozilla-central/rev/9866d07334e1
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Thank you for the patch! Let me know if you are looking for additional bugs to take on and want me to suggest something. Feel free to look for other mentored bugs as well.
You need to log in before you can comment on or make changes to this bug.