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

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 1

[Chris Lewis]

Can I be assigned to work on this?

Comment 2

[Andreas Gal :gal]

Absolutely.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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

Comment 4

[Chris Lewis]

Thanks for the assistance, this would be my first patch.

Comment 5

[andi.draghici]

Can I work on this too?

Comment 6

[Chris Lewis]

(In reply to Kartikaya Gupta (email:[e-mail]) 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]).

Comment 7

[Chris Lewis]

Attachment: Patch v1

Patch compiles but not sure which test suite is appropriate.

Comment 8

[Andreas Gal :gal]

Since this is just a renaming we won't need a test here. No functionality changed.

Comment 9

[Andreas Gal :gal]

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.

Comment 10

[Chris Lewis]

Attachment: 974097 v2.patch

Thanks for your help Andreas. I've fixed the spacing issues so it should be ready for check-in.

Comment 11

[Andreas Gal :gal]

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!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72591882de26

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/72591882de26