Hi Alex, sorry for the delay. It would be great if you could take these patches and drive them to completion! I think the code itself is fine, it's more that the patch series isn't fully optimized for ease of review. If I remember correctly, the things left to do were: - Merge and re-split the patches so that the visual look is maintained at any point in the patch stack. Specifically, there's one part which gets rid of the borders and shadows, and another part which adds a new element with the shadow to restore the original look, so that "solution" should probably be moved earlier into the stack. - Split the "Move mouse motion detection to the parent, and add snap animation when the mouse button is lifted." patch into the two pieces described by its commit message - Do another review pass of the patches and maybe split out things that don't really belong in a patch. For example I see that D265176 is also centralizing the pref reading, and it's moving the initialization of the dynamic toolbar behavior down so that it's not interspersed between the DOM element creation that the rest of the function is doing. So maybe those things would be better in a separate patch.
Bug 1978145 Comment 9 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
Hi Alex, sorry for the delay. It would be great if you could take these patches and drive them to completion! I think the code itself is fine, it's more that the patch series isn't fully optimized for ease of review. If I remember correctly, the things left to do were: - Merge and re-split the patches so that the visual look is maintained at any point in the patch stack. Specifically, there's one part which gets rid of the borders and shadows, and another part which adds a new element with the shadow to restore the original look, so I think it would make sense to move this "solution" to the start of the stack. - Split the "Move mouse motion detection to the parent, and add snap animation when the mouse button is lifted." patch into the two pieces described by its commit message - Do another review pass of the patches and maybe split out things that don't really belong in a patch. For example I see that D265176 is also centralizing the pref reading, and it's moving the initialization of the dynamic toolbar behavior down so that it's not interspersed between the DOM element creation that the rest of the function is doing. So maybe those things would be better in a separate patch.