925986 - Code cleanup - Misc unused var declared in aboutReader method _toggleToolbarVisibility()

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect, P5)

Description

[Mark Capella [:capella]]

Currently found here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#474

Comment 1

[Tobias (:Tobbi) Markus]

Attachment: patch

Comment 2

[Mark Capella [:capella]]

Small change looks right to me ... have you / are you able to build Firefox Mobile with the patch to unit test it?

Comment 3

[Tobias (:Tobbi) Markus]

(In reply to Mark Capella [:capella] from comment #2)
> Small change looks right to me ... have you / are you able to build Firefox
> Mobile with the patch to unit test it?

Please bare with me for a moment, I am quite busy with college and freshly building FF Mobile takes quite a lot of time.

Comment 4

[Mark Capella [:capella]]

Tobias: Are you planning on finishing this? If not, I'll reset it to un-assigned.

Comment 5

[Bhavik Raja]

can i get assigned to this please?

Comment 6

[Nick Alexander :nalexander [he/him]]

This is a good first bug, it hasn't had recent movement, and we're short of first bugs, so clearing the assigned field.  capella, feel free to re-assign or change mentor as you see fit.

Comment 7

[Franz Sarmiento]

Attachment: 0001-Bug-925986-Code-cleanup-Misc-unused-var-declared-in-.patch

I have removed the unused variable in this function. After this small change, I built Fennec once again and opened an article. After opening the article, I enabled Reader Mode. Scrolling up and down the article hides and displays the toolbar, so I assume my small change did not cause any regression.

Comment 8

[Mark Capella [:capella]]

Comment on attachment 8444039 [details] [diff] [review]
0001-Bug-925986-Code-cleanup-Misc-unused-var-declared-in-.patch

To trigger/test the affected code involved, once the readermode is enabled/displayed, if you tap somewhere in the content of the page, pause, tap, pause, tap, etc... you'll see the readerbanner at the bottom of the screen display, hide, display, hide, etc.

(The thing with the three icons: book / Aa / Share)

This is a pretty safe patch as I mentioned in IRC for good-first-bug / learning the patch -> m-c release process.

While I'm happy to mentor for mobile, I'll ping margaret to do the final r? / approval for you :-)

Comment 9

[Franz Sarmiento]

Following Mark Capella's instructions, here are some screenshots from a build with this patch applied:

Reader mode w/ toolbar: http://i.imgur.com/ytaMzZt.png
Reader mode w/ toolbar hidden: http://i.imgur.com/cpvIfmV.png

I repeated this several times and the toolbar's visibility was indeed toggled every time.

Comment 10

[Franz Sarmiento]

I have a quick question regarding the solution I provided. I did a grep of the whole contents dir to see where else this function is called. The results of grep was that the only time it is ever called is here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#239

I am confused, in this line the function is called without any arguments, even if the function definition has set it to receive 1 arg named "visible". But upon trying a build before my patch, clicking on the page still toggles the toolbar's visibility. Shouldn't it fail because it's calling a function that's supposed to accept 1 arg without passing anything?

Comment 11

[:Margaret Leibovic]

Sorry for letting this bug slip off my radar! Thanks for taking the time to work on this!

(In reply to Franz Sarmiento from comment #10)
> I have a quick question regarding the solution I provided. I did a grep of
> the whole contents dir to see where else this function is called. The
> results of grep was that the only time it is ever called is here:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> aboutReader.js#239
> 
> I am confused, in this line the function is called without any arguments,
> even if the function definition has set it to receive 1 arg named "visible".
> But upon trying a build before my patch, clicking on the page still toggles
> the toolbar's visibility. Shouldn't it fail because it's calling a function
> that's supposed to accept 1 arg without passing anything?

This doesn't fail because JavaScript has a very flexible type/parameter system. So when you call a function with no parameters, if it expects a parameter and tries to use it in the body of the function, it will just be undefined.

So, really, that call is equivalent to:
this._toggleToolbarVisibility(undefined);

Comment 12

[:Margaret Leibovic]

Capella, can you help land this patch?

Comment 13

[Mark Capella [:capella]]

Absolutely!

franz, first I'll fixup your patch to match mozilla / mercurial standards ...
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Importantly, we add reviewer information to your commit message, some extra lines of context, and then push it to our automated integration build/test suite ... "TRY"
https://tbpl.mozilla.org/?tree=Try&rev=d5c2b14b29b3

When that passes, I'll followup and push it to fx-team. After that it'll be moved to mozilla-central, and your bug will be closed for you.

Basically at this point, your work is done, you can just watch the progress  :-D

Comment 14

[Franz Sarmiento]

Margaret: Oh, I see. Thanks for clarifying that.

Capella: Thanks! Next time, I will format it properly following that article. Sorry about that.

Comment 15

[Mark Capella [:capella]]

TRY push went well (didn't expect issues, but this is a pre-req)

Push to fx-team to make it official
https://hg.mozilla.org/integration/fx-team/rev/dcec428a089e

Comment 16

[Phil Ringnalda (:philor)]

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