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

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: kats, Assigned: chris.lewis2)

Tracking

unspecified
mozilla30
x86
macOS
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

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]).
Posted 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+
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+
https://hg.mozilla.org/mozilla-central/rev/72591882de26
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.