Closed Bug 974097 Opened 11 years ago Closed 11 years ago

The AppendChild operation in LayersMessages.ipdlh should really be called PrependChild

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: kats, Assigned: chris.lewis2)

Details

(Whiteboard: [good first bug][lang=c++,ipdl][mentor=kats])

Attachments

(1 file, 1 obsolete file)

The implementation in LayerTransactionParent.cpp implements the "AppendChild" op by calling InsertAfter(aChild, null). If you look at the ContainerLayer::InsertAfter implementation in Layers.cpp this is really a prepend operation rather than an append operation - the new child becomes the new first child of the parent.
Can I be assigned to work on this?
Assignee: nobody → chris.lewis2
Absolutely.
Hi Chris, You can definitely work on this bug. I don't know if you've contributed patches to Mozilla before but if you haven't the first step is to get a build of Firefox up and running. There are instructions on how to to do this at [1]. For this bug a desktop build should be fine. The changes you need to make are pretty straightforward renames of some functions and variables - basically replacing all instances of "AppendChild" in the gfx/layers/ipc/ folder to be "PrependChild" instead. Once you've done that and verified that everything still compiles/works, you can submit a patch for review (see the process at [2]). Please feel free to ask questions either on this bug or via email, or join us on IRC (irc.mozilla.org server, #gfx channel) and ask us there. [1] https://developer.mozilla.org/en/docs/Developer_Guide/Build_Instructions [2] https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
Thanks for the assistance, this would be my first patch.
Can I work on this too?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #3) > Hi Chris, > > You can definitely work on this bug. I don't know if you've contributed > patches to Mozilla before but if you haven't the first step is to get a > build of Firefox up and running. There are instructions on how to to do this > at [1]. For this bug a desktop build should be fine. > > The changes you need to make are pretty straightforward renames of some > functions and variables - basically replacing all instances of "AppendChild" > in the gfx/layers/ipc/ folder to be "PrependChild" instead. Once you've done > that and verified that everything still compiles/works, you can submit a > patch for review (see the process at [2]). > > Please feel free to ask questions either on this bug or via email, or join > us on IRC (irc.mozilla.org server, #gfx channel) and ask us there. > > [1] https://developer.mozilla.org/en/docs/Developer_Guide/Build_Instructions > [2] > https://developer.mozilla.org/en-US/docs/Developer_Guide/ > How_to_Submit_a_Patch I've created a patch that compiles but I'm not sure what automated test suite to use (re: process at [2]).
Attached patch Patch v1 (obsolete) — Splinter Review
Patch compiles but not sure which test suite is appropriate.
Since this is just a renaming we won't need a test here. No functionality changed.
Comment on attachment 8380163 [details] [diff] [review] Patch v1 Review of attachment 8380163 [details] [diff] [review]: ----------------------------------------------------------------- If you attach a new patch with the fixes and set checkin-needed in the whiteboard someone will check it in for you. ::: gfx/layers/ipc/LayersMessages.ipdlh @@ +246,5 @@ > > // Monkey with the tree structure > struct OpSetRoot { PLayer root; }; > struct OpInsertAfter { PLayer container; PLayer childLayer; PLayer after; }; > +struct OpPrependChild { PLayer container; PLayer childLayer; }; Please fix the indentation here. ::: gfx/layers/ipc/ShadowLayers.cpp @@ +280,1 @@ > nullptr, Shadow(aChild))); Please fix the indentation here as well.
Attachment #8380163 - Flags: review+
Attached patch 974097 v2.patchSplinter Review
Thanks for your help Andreas. I've fixed the spacing issues so it should be ready for check-in.
Attachment #8380163 - Attachment is obsolete: true
Attachment #8380237 - Flags: checkin+
Comment on attachment 8380237 [details] [diff] [review] 974097 v2.patch We use checkin-needed for this. Thanks for the patch and welcome to Gecko hacking!
Attachment #8380237 - Flags: checkin+
Keywords: checkin-needed
Attachment #8380237 - Flags: review+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: