Closed Bug 1293738 Opened 4 years ago Closed 4 years ago

Remove unneeded aWM arg from some FlexboxAxisTracker methods

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: dholbert, Assigned: theeviltwin, Mentored)

Details

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

Attachments

(1 file, 3 obsolete files)

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.
(In particular, InitAxesFromModernProps and InitAxesFromLegacyProps don't seem to need their aWM parameter; they should just use mWM instead.)
Mentor: dholbert
Keywords: good-first-bug
Whiteboard: [good first bug][lang=c++]
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?
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
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 :)
Flags: needinfo?(ealvarez)
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 :)
Flags: needinfo?(ealvarez)
Attached file bug-1293738.cpp (obsolete) —
Hi, Sorry to keep nagging.
I see what you mean now. Is this somewhat what we wish to do?
Flags: needinfo?(ealvarez)
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".
(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/
)
Attached patch patch-1.patch (obsolete) — Splinter Review
Good enough?
Thanks for bearing with my beginner stuff. :P
Attachment #8780325 - Flags: review?(ealvarez)
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!
Attachment #8780325 - Flags: review?(ealvarez) → review+
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.
Flags: needinfo?(ealvarez)
Attached patch patch-2.patch (obsolete) — Splinter Review
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
Flags: needinfo?(dholbert)
Attachment #8780327 - Flags: review?(dholbert)
Oppps, forgot about addding stuff to the commit message. Want me to do it again? Or this would do?
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.)
Flags: needinfo?(dholbert)
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!
Disregard the bug I mentioned above, at least until we confirm there's no-one else working on it at the moment :)
Attached patch patch-3.patchSplinter Review
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.
Attachment #8780339 - Flags: review?(dholbert)
Flags: needinfo?(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
Attachment #8780339 - Flags: review?(dholbert) → review+
Wow! Sounds Awesome. Glad I could be of help.
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.
Ah! Sad. Anyways do tell me whatever happens because I am really excited about my first piece of contribution :)
Try run looks good! Let's call this checkin-needed.
Assignee: nobody → theeviltwin
Status: NEW → ASSIGNED
Flags: needinfo?(dholbert)
Keywords: checkin-needed
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.)
Attachment #8780325 - Attachment is obsolete: true
Attachment #8780327 - Attachment is obsolete: true
Attachment #8780327 - Flags: review?(dholbert)
Attachment #8780313 - Attachment is obsolete: true
(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.)
(Also: usually "checkin-needed" would've gotten triaged by now. If this is still pending on Monday, I'll check it in myself.)
Flags: needinfo?(dholbert)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd96fa41021
Remove unneeded aWM arg from some FlexboxAxisTracker methods. r=dholbert, r=emilio
Keywords: checkin-needed
Thanks for the bug recommendation. I will work on it. Do tell me if there are more like these I can work on.
https://hg.mozilla.org/mozilla-central/rev/8bd96fa41021
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Flags: needinfo?(dholbert)
You need to log in before you can comment on or make changes to this bug.