1244197 - Fix two small code optimizations

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Text Selection, defect)

Description

[Mark Capella [:capella]]

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

Comment 1

[Abhishek Potnis [:avp]]

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.

Comment 2

[rishabh yadav [:rishabh]]

I would like to work on this bug

Comment 3

[Mark Capella [:capella]]

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.

Comment 4

[rishabh yadav [:rishabh]]

(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.

Comment 5

[Abhishek Potnis [:avp]]

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.

Comment 6

[Tristan]

"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 :^)

Comment 7

[Mark Capella [:capella]]

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

Comment 8

[Tristan]

Attachment: ActionBarHandler.js

Comment 9

[Tristan]

Comment on attachment 8723406 [details]
ActionBarHandler.js

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

Comment 10

[Tristan]

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

Comment 11

[Mark Capella [:capella]]

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

Comment 12

[Tristan]

Attachment: 1244197.patch

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

Comment 13

[Mark Capella [:capella]]

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) {

Comment 14

[Tristan]

Attachment: Bug 1244197 - Fixed conditional logic for two performance optimizations

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 :^)

Comment 15

[Mark Capella [:capella]]

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 ...

Comment 16

[Mark Capella [:capella]]

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

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/a53a8563879b

Comment 18

[Carsten Book [:Tomcat]]

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