1293738 - Remove unneeded aWM arg from some FlexboxAxisTracker methods

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

As emilio noticed in one of his patches on bug 1000957, we currently pass aWM into some FlexboxAxisTracker methods where we don't need it (since we have it cached as mWM).

Filing this bug on cleaning that up.

Comment 1

[Daniel Holbert [:dholbert]]

(In particular, InitAxesFromModernProps and InitAxesFromLegacyProps don't seem to need their aWM parameter; they should just use mWM instead.)

Comment 2

[Pranaydeep Singh [:theeviltwin]]

Hi, I am a new contributor and I have the firefox development environment setup. I would like to work on this bug. Could someone help me get started?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Hi, sure! We're talking about the layout/generic/nsFlexContainerFrame.cpp.

In that file, there's a class, called FlexboxAxisTracker, with some helper methods, InitAxesFromLegacyProps and InitAxesFromModernProps[1].

Those methods take two arguments, aFlexContainer, and aWM (writing-mode). If you take a look to the call-sites (the constructor), you'll see that the aWM parameter is already available as the mWM member variable, so we should take rid of those arguments, and use mWM directly.

I think that should be enough to get you started, but feel free to ask any other question you might have! (tip: you can use the "Need more information from" checkbox and type our names so we get a more noisy notification and don't miss it :)).

Thanks for being interested in fixing this! :)

[1]: http://searchfox.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp#378

Comment 4

[Pranaydeep Singh [:theeviltwin]]

Hi,
Thanks for the quick reply. Since, I am a new contributor so I am quite excited about my first bug. As far as I understand this problem it can be fixed by replacing awM with mWM in the function calls:
if (IsLegacyBox(aFlexContainer->StyleDisplay(),
                  aFlexContainer->StyleContext())) {
    InitAxesFromLegacyProps(aFlexContainer, mWM);
  } else {
    InitAxesFromModernProps(aFlexContainer, mWM);
  }

If I am wrong, please do help me understand this better. Thank You :)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Hi, no problem :)

No, those are member functions, so there's no need to pass mWM around. The idea is stop passing the argument and use the member directly. That way, you'd end up having something like:

  if (IsLegacyBox(aFlexContainer->StyleDisplay(),
                  aFlexContainer->StyleContext())) {
    InitAxesFromLegacyProps(aFlexContainer);
  } else {
    InitAxesFromModernProps(aFlexContainer);
  }

You should, of course, replace all uses of aWM to use mWM :)

Comment 6

[Pranaydeep Singh [:theeviltwin]]

Attachment: bug-1293738.cpp

Hi, Sorry to keep nagging.
I see what you mean now. Is this somewhat what we wish to do?

Comment 7

[Daniel Holbert [:dholbert]]

Thanks for taking a look at this bug!  Please submit your changes as a patch, rather than posting the whole C++ file. That way, it's easier to see what you're actually changing (and if someone else modifies other stuff in the file, your change won't stomp on their changes). This page has some steps on that:
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

As a quick-and-easy approximation, "hg diff > mypatch.patch" is one way to generate a patch from your changes. But really you'll want to use "hg commit" to generate an actual changeset, which you can then export to a file with "hg export [CSETID] > mypatch.patch".

Comment 8

[Daniel Holbert [:dholbert]]

(or instead of hg commit / hg export, you can also use Mercurial Queues, which is a workflow that involves commands like "hg qnew", "hg qpush", and "hg qref".  The developer.mozilla.org page links to instructions about that, which are also available here:  https://www.mercurial-scm.org/wiki/MqExtension   That workflow is also a decent way to generate patches.

There are also videos here which might be helpful -- in particular, "setting up mercurial" and "creating a patch file":
http://codefirefox.com/
)

Comment 9

[Pranaydeep Singh [:theeviltwin]]

Attachment: patch-1.patch

Good enough?
Thanks for bearing with my beginner stuff. :P

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8780325 [details] [diff] [review]
patch-1.patch

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

LGTM, thanks for doing this!

Officially I think dholbert will need to confirm it, but... Can you update another version of the patch with that nit addressed, and flag him for review?

Thanks again!

::: layout/generic/nsFlexContainerFrame.cpp
@@ +3260,3 @@
>    // So, we need to reverse the corresponding flex axis to match.
>    // (Note this we don't toggle "mIsMainAxisReversed" for this condition,
>    // because the main axis will still match aWM's inline direction.)

uber-nit: Can you update the comments too? Thanks!

Comment 11

[Daniel Holbert [:dholbert]]

Yup, this looks good to me, with Emilio's comment-nit addressed. (good catch, Emilio!)

I'll grant swift r+, if you upload a patch with that fixed. Also, while you're at it, you can put "r=dholbert r=emilio" at the end of your commit message, to indicate that those are the reviewers.  (And then when someone comes along to land your patch, they won't have to manually add that themselves [since they might forget or mess it up, as sometimes happens]. :))

(If you're using patch queues, you'd edit the commit message with "hg qref -m [new-commit-message-here]" or "hg qref -e" to pop up an editor. If you're *not* using patch queues, you'd use "hg commit --amend" I think.

Comment 12

[Pranaydeep Singh [:theeviltwin]]

Attachment: patch-2.patch

There it is. Thanks a lot for being so co-operative. This was my first attempt at a contribution and even though it might not even be relevant, I thoroughly enjoyed it. Could you help me with where to go from here?
Thanks a lot once again

Comment 13

[Pranaydeep Singh [:theeviltwin]]

Oppps, forgot about addding stuff to the commit message. Want me to do it again? Or this would do?

Comment 14

[Daniel Holbert [:dholbert]]

Thanks!

It looks like you've generated & attached a second patch here, which layers on top of the first patch.  Really, we'd like to take your fix as one single excellent patch file.  Could you squash your two patches together into a single patch file?  (If you're using mercurial queues, "hg qfold" is the command to do this. (For future reference -- I'm guessing you ran "hg qnew" again, and that generated a second patch -- but really you wanted to use "hg qref" to update the existing patch.))

Also, please do update the commit message as noted in comment 11 -- it should end up looking like this:
  Bug 1293738 - Remove unneeded aWM arg from some FlexboxAxisTracker methods. r=dholbert r=emilio

>  This was my first attempt at a contribution and even though it might not even be relevant, I thoroughly enjoyed it.

Hooray! Thanks for the contribution!

> Could you help me with where to go from here?

http://www.joshmatthews.net/bugsahoy/ may help you find more bugs to work on (perhaps that's how you came across this one).  I don't have any specific bugs to direct you at, at the moment.

(Unfortunately we're only semi-good at filing "good first bugs", and we're not very good at filing "good second bugs". :)  That's partly because it's hard up-front to know how much work/knowledge will be involved for less-trivial bugs, I guess.)

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Also, you can try https://starters.servo.org/ if you feel like writing some Rust.

I know another layout bug, a bit more hard, that Xidorn filled not long ago: bug 1290312, but I'm sure there are plenty more of them!

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Disregard the bug I mentioned above, at least until we confirm there's no-one else working on it at the moment :)

Comment 17

[Pranaydeep Singh [:theeviltwin]]

Attachment: patch-3.patch

There!
THanks again, do tell me if there are any other issues, and do tell me if I can help in any other way.
Thank You.

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8780339 [details] [diff] [review]
patch-3.patch

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

Looks great! Thanks for fixing that up.  r=me

I'll give this a run on our TryServer* (to be sure it passes tests), and if everything looks good, then we can add the "checkin-needed" keyword here & one of our tree-sheriffs will check it in.

* https://wiki.mozilla.org/ReleaseEngineering/TryServer

Comment 19

[Pranaydeep Singh [:theeviltwin]]

Wow! Sounds Awesome. Glad I could be of help.

Comment 20

[Daniel Holbert [:dholbert]]

Sure!  BTW, TryServer is closed at the moment (to clear a long backlog of pending test runs) so I'll push to Try later on when it reopens.

Comment 21

[Pranaydeep Singh [:theeviltwin]]

Ah! Sad. Anyways do tell me whatever happens because I am really excited about my first piece of contribution :)

Comment 22

[Daniel Holbert [:dholbert]]

OK, pushed to Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f77ea456cf3f

Comment 23

[Daniel Holbert [:dholbert]]

Try run looks good! Let's call this checkin-needed.

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8780325 [details] [diff] [review]
patch-1.patch

(--> Marking older patches as obsolete. For future reference: bugzilla also gives you an opportunity to mark them as obsolete [via checkboxes] when you upload newer versions of patches -- that's usually a good idea.)

Comment 25

[Daniel Holbert [:dholbert]]

(If you're looking for another easy-ish bug to work on, bug 1291483 should be relatively straightforward as well, as a admittedly-unglamorous but useful code-modernization improvement.)

Comment 26

[Daniel Holbert [:dholbert]]

(Also: usually "checkin-needed" would've gotten triaged by now. If this is still pending on Monday, I'll check it in myself.)

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd96fa41021
Remove unneeded aWM arg from some FlexboxAxisTracker methods. r=dholbert, r=emilio

Comment 28

[Pranaydeep Singh [:theeviltwin]]

Thanks for the bug recommendation. I will work on it. Do tell me if there are more like these I can work on.

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8bd96fa41021