826025 - Clean up BrowserEventHandler.handleUserEvent()

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

I seem that we can clean up this part like following:

* Use "switch" instead of "else if" series in BrowserApp.observe().
* Add some variables to cache properties which are accessed many times & never changed in this method.

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Tetsuharu OHZEKI [:saneyuki_s] from comment #0)
> * Use "switch" instead of "else if" series in BrowserApp.observe().

Oops. This is `Use "switch" instead of "else if" series`.

Comment 2

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

> * Use "switch" instead of "else if" series.
> * Add some variables to cache properties which are accessed many times &
> never changed in this method.

Chris, how do you think about these plan?

Comment 3

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part1-v1: Use "switch" instead of "else if"

Comment 4

[:Margaret Leibovic]

Cc'ing kats, because it looks like he's worked in here a lot.

Comment 5

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

This patch looks fine. What properties were you looking to cache?

Comment 6

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part 2-v1

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> This patch looks fine. What properties were you looking to cache?

Part 1 is only changing to "switch" statement.
I made the part 2, this add some variables to cache |data.x|, |data.y|, and |this._scrollableElement|.
But caching |this._scrollableElement| may be risky.

Comment 7

[Chris Peterson [:cpeterson]]

Adding local variables for data.x and data.y looks good to me. The new names are shorter and the function is no longer modifying its input parameters (data), which is bad form. :)

But I agree with Kats that caching this._scrollableElement (or any "this.*" variables) sounds too risky. This function might (now or in the future) call other functions that depend on this._scrollableElement, so we would not want it to be out of date.

Comment 8

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: part 2-v2: add some variable to cache

I changed that adding only data.(x|y).

Chris, May I ask you a review?

Comment 9

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

(In reply to Chris Peterson (:cpeterson) from comment #7)
> But I agree with Kats that caching this._scrollableElement (or any "this.*"

It wasn't me that said that, but I agree with that anyway :)

Comment 10

[Chris Peterson [:cpeterson]]

Comment on attachment 697175 [details] [diff] [review]
part1-v1: Use "switch" instead of "else if"

Review of attachment 697175 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

Comment 11

[Chris Peterson [:cpeterson]]

Comment on attachment 697301 [details] [diff] [review]
part 2-v2: add some variable to cache

Review of attachment 697301 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

Comment 12

[Chris Peterson [:cpeterson]]

I can land the patches, but I'm waiting for test results from the Try server first:

https://tbpl.mozilla.org/?tree=Try&rev=b9e2b15cb82f

Comment 13

[Chris Peterson [:cpeterson]]

Looks like someone already landed these patches on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/190318a9118a
https://hg.mozilla.org/integration/mozilla-inbound/rev/29080e5ecaed

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/190318a9118a
https://hg.mozilla.org/integration/mozilla-inbound/rev/29080e5ecaed

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/190318a9118a
https://hg.mozilla.org/mozilla-central/rev/29080e5ecaed