Fix two small code optimizations

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: capella, Assigned: tldmartin, Mentored)

Tracking

unspecified
Firefox 47
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: [good first bug] [lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

3 years ago
This is a fairly simple bug for a new contributor with basic Javascript skills, to learn the Mozilla development process. Estimated completion time should be easily less than two weeks, and more likely less than one.

You should have an android device for testing your solution locally, and should be able to build Firefox Mobile on your desktop. See [0] for instructions if this is new to you, and ask questions on irc channels #introduction, or #mobile.


ActionBarHandler has two small code optimizations that don't work, but aren't currently causing issues as they fail safe and don't impact the visible UI to the user.

The first is a too-clever compare here: [1], and the second is here: [2]. Identifying the problems and solutions, will be up to you.


[0] https://wiki.mozilla.org/Mobile/Fennec/Android

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ActionBarHandler.js?rev=5a814b26505b&mark=46-47#43
[2] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ActionBarHandler.js?rev=5a814b26505b&mark=209-209#196
Reporter

Updated

3 years ago
Summary: Fix two small code optimiaztions → Fix two small code optimizations
Reporter

Updated

3 years ago
Mentor: markcapella
Whiteboard: [good first bug] [lang=js]
Hey Capella,

I am new to contributing to Firefox for Android, and would like to work on this bug. I will read through and upload the patch.

Thanks.
Assignee: nobody → abhishekp.bugzilla
Status: NEW → ASSIGNED
I would like to work on this bug
Reporter

Comment 3

3 years ago
Rishabh (and all others), generally, we assign work to whoever can first demonstrate a viable patch. I noticed Abhishek previously assigned this to himself, but hasn't provided a solution.

Abhishek, if you're still interested in this, please comment. Otherwise, I'll be un-assigning or re-assigning this.
Flags: needinfo?(abhishekp.bugzilla)
(In reply to Mark Capella [:capella] from comment #3)
> Rishabh (and all others), generally, we assign work to whoever can first
> demonstrate a viable patch. I noticed Abhishek previously assigned this to
> himself, but hasn't provided a solution.
> 
> Abhishek, if you're still interested in this, please comment. Otherwise,
> I'll be un-assigning or re-assigning this.

Can you help me in setting up this project so i can start working on this bug.
Hey,

Apologies for having assigned this bug to me and not coming up with a patch in time.

@Rishabh, feel free to work on this bug!

I am un-assigning myself from this bug.

Thanks.
Status: ASSIGNED → NEW
Flags: needinfo?(abhishekp.bugzilla)
Assignee: abhishekp.bugzilla → nobody
Assignee

Comment 6

3 years ago
"All changes must be tested" according to https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch . 

How would you test the fix to this bug when the observable behaviour won't change? 

p.s. hi, I'm new :^)
Reporter

Comment 7

3 years ago
Probably the simplest answer is "by exercising the code involved, and observing internal workings before and after you've made your change(s)".

This particular code is involved in Firefox for Android's (mobile) text selection handling. By navigating to a test page such as [0], you can observe [1] and then by long-pressing the word |voice| observe it being selected / highlighted in orange [2], and the ActionBar being displayed on the top of the screen.

This all generates events into ActionBarHandler.js entering here: [3]

Placing appropriate logging messages or using a debugging tool like WebIDE should let you see the before/after behaviours and confirm your expectations.


[0] https://www.dropbox.com/s/jtbodbxw2qyiykg/simpleInput.html?dl=0

[1] https://www.dropbox.com/s/g7cvx9sjwmw0hjv/bug_1244197_p1.png?dl=0
[2] https://www.dropbox.com/s/qvz4q7lhsvygmos/bug_1244197_p2.png?dl=0

[3] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ActionBarHandler.js?rev=b8449a2d5fc1&mark=31-31#26
Assignee

Comment 8

3 years ago
Posted file ActionBarHandler.js (obsolete) —
Assignee

Comment 9

3 years ago
Comment on attachment 8723406 [details]
ActionBarHandler.js

Here's my patched file :) Let me know if anything needs changing.
Assignee

Comment 10

3 years ago
Oops, those files are identical... was trying to edit my post. Excuse my noob spam ;)
Reporter

Updated

3 years ago
Attachment #8723406 - Attachment is patch: true
Attachment #8723406 - Attachment mime type: application/javascript → text/plain
Reporter

Comment 11

3 years ago
Comment on attachment 8723406 [details]
ActionBarHandler.js

Ah, I thought you posted a patch without marking it a patch, but you've posted the entire modified module. Can you create and post a proper patch/diff file instead? see [0] for instructions ... it's not as bad as it looks :) You can ask most anybody on irc channel #introduction if you need help ...

Just looking here quickly I notice a few things, and wonder if you can update your patch ...

) For the correction in the caretStateChangedHandler() method, you're properly comparing |this._targetElement to Services.focus.focusedElement|, and |this._contentWindow to Services.focus.focusedWindow|. However, in the init() method where those values are initially set, we use the result returned from this._getSelectionTargets() which adds a little complexity to their values. Notably, |element| is sometimes returned as null.

fyi, this is because some focusable elements don't contain text and aren't valid for our use. Can you change the compare to use the results of an intermediate call to that method?

) For the correction in the _sendActionBarActions() method, I was expecting a simpler solution such as simply stringify-ing the action object and comparing / saving that value, but your bit of logic looks correct on quick glance. I worried about only comparing e.id but not also e.showAsAction, but it appears we always send it as true. Interestingly, the receiver method for that message [1] is able to handle both true and false  :-/

Small nits: if you keep your code version, can you tweak your style a little to better match moz standard? Place condition expressions such as |&&| at line end vs. start, and maybe for that second line, something like:

let foo = bar &&
  baz.every((e,i) => {
    return e.id === doh[i].id;
  });

Finally, can you quick describe: Have you built and tested on a device? How are you testing to observe results?


[0] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/TextSelection.java?mark=331-332,#317
Attachment #8723406 - Attachment is patch: false
Reporter

Updated

3 years ago
Assignee: nobody → tldmartin
Assignee

Comment 12

3 years ago
Posted patch 1244197.patch (obsolete) — Splinter Review
Hi Mark,

Thank you for your detailed reply. I've updated the patch as best I can, but I suspect you'll have some more changes for me.

(In reply to Mark Capella [:capella] from comment #11)

> Can you change the compare to use the results of an
> intermediate call to that method?

Sorry, I don't understand what you mean. You'll have to spell it out for me :P
 
> Finally, can you quick describe: Have you built and tested on a device? How
> are you testing to observe results?

I ran two versions of Fennec on my Samsung Note S3: one before the patch and one after. Each time I had WebIDE breakpoints at the two points where the bugs were, [0] and [1]. Before the patch, [0] was always false, and [1] was always true. After changing the code, the conditional logic worked as the original coder seemed to intend (they incorrectly assumed === and !== would compare arrays by their contents, rather than their reference).


[0] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ActionBarHandler.js?rev=5a814b26505b&mark=46-47#43
[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ActionBarHandler.js?rev=5a814b26505b&mark=209-209#196
Attachment #8723406 - Attachment is obsolete: true
Flags: needinfo?(markcapella)
Reporter

Comment 13

3 years ago
Comment on attachment 8723976 [details] [diff] [review]
1244197.patch

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

I always hate to "spell it out' ... I like reviewers to give me hints, rather than answers ... This seems easy enough though  :-)

Sounds like you're being pretty thorough! I've had contributors before offer code hypothetically (no buulding, testing, etc) ... and we have to do extra checking in that case on their behalf.

Can you update your commit message before you re-post, to just use the reviwers nick? like:
Bug 1244197 - Fixed conditional logic for two performance optimizations, r=capella

Other than that, we'll just need you to push to integration test "TRY server" ... or I can do that for you if you don't have at least L3 commit access yet.

If Moz can look forward to your continuing to help, you might like to check into that [0]


[0] https://www.mozilla.org/en-US/about/governance/policies/commit/

::: mobile/android/chrome/content/ActionBarHandler.js
@@ +63,5 @@
>  
>      // Else, update an open ActionBar.
>      if (this._selectionID) {
> +      if (this._targetElement === Services.focus.focusedElement &&
> +        this._contentWindow === Services.focus.focusedWindow) {

I was thinking that here we could do like:
+  let [element, win] = this._getSelectionTargets();
+  if (this._targetElement === element &&
+      this._contentWindow === win) {
Assignee

Comment 14

3 years ago
Here's the amended patch. Could you please submit it to the "TRY server"? I'm assuming I don't have L3 access.

I'm new to pretty much everything in your stack (Mercurial, IntelliJ, Ecmascript6, Android development, etc) but I'm slowly making sense of it :^)
Attachment #8723976 - Attachment is obsolete: true
Flags: needinfo?(markcapella)
Reporter

Comment 15

3 years ago
Comment on attachment 8724010 [details] [diff] [review]
Bug 1244197 - Fixed conditional logic for two performance optimizations

Aha :) You forgot to flag me for action, and I forgot about this ... back into my queue ...
Attachment #8724010 - Flags: review?(markcapella)
Reporter

Comment 16

3 years ago
Comment on attachment 8724010 [details] [diff] [review]
Bug 1244197 - Fixed conditional logic for two performance optimizations

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

Looks good! Pushing to try, expecting no issues
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c223c15e791
Attachment #8724010 - Flags: review?(markcapella) → review+

Comment 18

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a53a8563879b
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
You need to log in before you can comment on or make changes to this bug.