Closed Bug 826025 Opened 7 years ago Closed 7 years ago

Clean up BrowserEventHandler.handleUserEvent()

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 20
Tracking Status
firefox19 --- wontfix
firefox20 --- fixed

People

(Reporter: tetsuharu, Assigned: tetsuharu)

Details

Attachments

(2 files, 1 obsolete file)

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.
(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`.
> * 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?
Cc'ing kats, because it looks like he's worked in here a lot.
This patch looks fine. What properties were you looking to cache?
Attached patch part 2-v1 (obsolete) — Splinter Review
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #697216 - Flags: feedback?(bugmail.mozilla)
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.
Assignee: nobody → saneyuki.s.snyk
Status: NEW → ASSIGNED
I changed that adding only data.(x|y).

Chris, May I ask you a review?
Attachment #697216 - Attachment is obsolete: true
Attachment #697216 - Flags: feedback?(bugmail.mozilla)
Attachment #697301 - Flags: review?(cpeterson)
Attachment #697175 - Flags: review?(cpeterson)
(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 on attachment 697175 [details] [diff] [review]
part1-v1: Use "switch" instead of "else if"

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

LGTM!
Attachment #697175 - Flags: review?(cpeterson) → review+
Comment on attachment 697301 [details] [diff] [review]
part 2-v2: add some variable to cache

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

LGTM!
Attachment #697301 - Flags: review?(cpeterson) → review+
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
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
Keywords: checkin-needed
Target Milestone: --- → Firefox 20
You need to log in before you can comment on or make changes to this bug.