1666340 - Fold KeyboardScrollAnimation into SmoothScrollAnimation

Status: RESOLVED FIXED

(Core :: Panning and Zooming, task, P3)

Description

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

The KeyboardScrollAnimation class doesn't really need to exist any more. It is a subclass of the GenericScrollAnimation class which does the actual scrolling animation; the extra work that KeyboardScrollAnimation does is basically in the SettingsForType helper function, where it maps a keyboard scroll action into animation settings.

This entire class is largely redundant with the more general SmoothScrollAnimation class, which uses a similar helper, ComputeBezierAnimationSettingsForOrigin. This helper does the same sort of thing as SettingsForType but is more generic and uses a scroll origin as an input.

We should be able to remove KeyboardScrollAnimation completely, and just add a small helper to convert from a keyboard scroll action to an origin, and then use SmoothScrollAnimation at the one use site, here

Comment 1

[Sunita Sen]

Hello,
I am new to contributing here. Can I work on this one?
Thanks.

Comment 2

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

Yes, absolutely. The first step here is to ensure you are set up to build Firefox on your local machine, which you can do by following the instructions at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions

Once you have done that let us and we can guide you further.

Comment 3

[Sunita Sen]

Thanks. :)
I have the Firefox setup done on my local machine.
Can you please guide me here a bit?

I am not able to figure out how to convert a KeyboardScrollAction parameter to a ScrollOrigin parameter.

Comment 4

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

The conversion will be based on the functionality that currently resides in the SettingsForType function. So in this function, you can see that the eScrollCharacter and eScrollLline variants use the general_smoothScroll_lines_duration* prefs, which is the same as what ScrollOrigin::Lines does. Similarly eScrollPage is equivalent to ScrollOrigin::Pages and eScrollComplete is equivalent to ScrollOrigin::Other. So with this you should be able to write a function that takes a KeyboardScrollAction parameter as input and returns a ScrollOrigin, and then use it here to replace the usage of KeyboardScrollAnimation with SmoothScrollAnimation. Does that make sense?

Comment 5

[Sunita Sen]

Attachment: Bug 1666340 - Fold KeyboardScrollAnimation into SmoothScrollAnimation r=kats

Comment 6

[Sunita Sen]

Attachment: Bug 1666340 - Fold KeyboardScrollAnimation into SmoothScrollAnimation r = Kartikaya Gupta

Depends on D96819

Comment 7

[Sunita Sen]

I am extremely sorry, I wanted to merge the second commit into the first one and by mistake, a new patch got submitted.
I have updated the original one as well.

Comment 8

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

No problem! I'll drop the second patch and just look at the updated first one.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/876d78db241c
Fold KeyboardScrollAnimation into SmoothScrollAnimation r=kats

Comment 10

[Cosmin Sabou [:CosminS]]

Backed out for linux toolchain bustages on SmoothScrollAnimation.cpp.

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&revision=876d78db241c5fae5c8982e5b830daa4da0b09d9&selectedTaskRun=TIzYeXy-QUei6WB4SQKS5g.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=321638408&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/9ea00a2061a993e02cbbaf552f4bc1e3deb6d9f4

Comment 11

[Sunita Sen]

Hello,
sorry for the mistake.
I have made changes as suggested in the patch.
Do I need to do anything else?

Comment 12

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

Thanks! No need to do anything else; I've triggered the re-landing with the updated patch. Sorry for not catching the error during review; we build the code with multiple compiler versions on our CI infrastructure and the error only manifested in an older version of the compiler.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/deeb82c5b8b8
Fold KeyboardScrollAnimation into SmoothScrollAnimation r=kats

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/deeb82c5b8b8

Comment 15

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

Thank you Sunita! This bug is all done now, feel free to pick out another bug you'd like to work on. I'll be filing a bug shortly to remove KeyboardScrollAnimation since it's no longer used and I'll cc you on that in case you feel like working on that - but feel free to find something more challenging if that's what you'd prefer!