Closed Bug 1585806 Opened 5 years ago Closed 5 years ago

Make SideBits an enum class

Categories

(Core :: Graphics, task, P3)

Product:
Core
Component:
Graphics
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla72
Tracking Flags:
Tracking Status
firefox72 --- fixed

People

(Reporter: botond, Assigned: james.hooks, Mentored)

Details

(Keywords: good-first-bug)

Attachments

(1 file, 1 obsolete file)

Bug 1585806 - Make SideBits an enum class, add casting where necessary. r=botond
47 bytes, text/x-phabricator-request
Details | Review

We have an enum named SideBits that's used to represent combinations of sides (top/right/bottom/left) in graphics code.

It would be nice to convert this to an enum class.

As part of this change, the enumerators can be renamed to remove the enumeration's name (e.g. eSideBitsNone change to eNone) since references to them will now have the form SideBits::eNone, and SideBits::eSideBitsNone would be redundant.

This is a good first bug for someone who has some basic familiarity with C++.

Hi Botond, I'd like to work on this improvement

Hi James, thanks for your interest!

The first step is to build Firefox locally as described in this guide.

Let me know once you've done that and we can discuss the next steps. If you run into any issues, feel free to ask here!

Okay awesome! I have firefox built locally so I'm ready to discuss the next steps.

Okay, great!

In the meantime, I realized that there are a couple of patches that haven't landed yet that you'll want to apply locally before working on this. (I sort of imagined that they would land before someone expresses interest in working on this, but you jumped on it quicker than that :)).

Anyways, so the first thing to do would be to apply these two patches locally, with hg import <url> (or, if you use git, let me know and I can give you alternative instructions):

With that done, here's an outline for making the changes for this bug:

  • Change the declaration of SideBits from enum to enum class.
  • Rename its enumerators as suggested (e.g. eSideBitsNone to eNone).
  • Use our Searchfox code search tool to locate existing usages of the enumerators. You can click on one of them in the declaration linked above, and click on "Search for enum constant" in the popup menu. Change existing usages to reflect the new enumerator names, and the fact the enum is scoped now (e.g. eSideBitsNone to SideBits::eNone).

Once that's done, build the code again with mach build to make sure everything still compiles after the changes. I believe the changes described above should be sufficient to make things compile, but there might be a few places where we need to insert a cast or something -- if compiler errors come up and you're not sure how to solve them, feel free to ask here!

Once you have a patch that's compiling, you can submit it for code review; instructions for that can be found here.

Excellent, thanks for the breakdown Botond.
I'll get started and let you know if I have any questions.

Hi Botond
I've made the changes requested. I may have made a mistake, I used moz-phab again but my change has appeared as another attachment. Please let me know if I need to submit my change another way.

It would be better to re-submit the updated patch in a way that Phabricator recognizes it as an update to the existing one. The following should do it:

  • Run hg commit --amend -e to edit the patch's commit message.
  • The commit message will presumably include a line that says Differential Revision: https://phabricator.services.mozilla.com//D51546
  • Edit that line to instead say Differential Revision: https://phabricator.services.mozilla.com//D51291. (I got this from the URL of the original Phabricator revision.)
  • Re-submit the patch with moz-phab submit.

I have submitted it correctly but now there's another issue. The step "Build 194514: Source Code Analysis" is failing and I'm not sure why. It points to line 289 in gfx/layers/composite/AsyncCompositionManager.cpp, however this line is a comment in my revision.

I'm wondering if It has to do with the other two patches I applied first. Do I need to include these revisions when submitting with moz-phab? I only specify my revision right now.

Assignee: nobody → james.hooks

(In reply to James Hooks from comment #10)

I have submitted it correctly but now there's another issue. The step "Build 194514: Source Code Analysis" is failing and I'm not sure why. It points to line 289 in gfx/layers/composite/AsyncCompositionManager.cpp, however this line is a comment in my revision.

I'm wondering if It has to do with the other two patches I applied first. Do I need to include these revisions when submitting with moz-phab? I only specify my revision right now.

Phabricator is trying to apply your patch to the latest revision in mozilla-central. Since the time I wrote comment 4, the two patches I mentioned have been committed, and an additional change that shuffles around some code that uses SideBits in AsyncCompositionManager.cpp has been committed as well. As a result, your patch does not apply cleanly onto the latest mozilla-central.

We'll need to rebase your patch onto the latest mozilla-central before committing it. Would you like to give that a try? It would involve using hg pull to get the latest revision, hg rebase to rebase your patch on top of it, fixing any rebase conflicts and/or compilation errors, and submitting the updated patch.

Hi Botond,

I have performed an hg pull and hg rebase with my changes. I had to do "hg rebase -d default" specifically for it to work. However the patch still doesn't want to apply and is failing with the error "abort: push failed on remote". What step might I be missing? I appreciate your patience in helping me sort this out.

It looks to me like your rebase is correct. I applied your patch locally, and it applied cleanly to a recent mozilla-central.

I suspect the "push failed on remote" error that Phabricator is showing is an unrelated error in our automation infrastructure that we can ignore.

I did realize in the meantime that I haven't finished reviewing your patch. I wrote some comments in BaseMargin.h (which you have since addressed), but I hadn't looked at later parts of your patch. I'd like to take a more detailed look at those parts, with a view to possibly avoiding some of the other static_casts being added. I might not get a chance to do so until next week, as I'm at a conference right now.

In any case, the patch is definitely on the right track, thanks for your work so far!

Attachment #9106081 - Attachment is obsolete: true

Hi Botond,
I've made the changes from your last review. I think the patch may be complete, please take a look when you have time. Thank you!

Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8921129925a8
Make SideBits an enum class, add casting where necessary. r=botond

https://hg.mozilla.org/mozilla-central/rev/8921129925a8

Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox72: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla72

Hey James, thanks for your work here! As you can see the patch has passed all tests and landed now :)

If you're interested in other mentored bugs, feel free to explore!

Awesome! Thanks for helping me with this bug, I really appreciate it.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: