Last Comment Bug 786604 - observe() method in aboutReader.js should use a named function
: observe() method in aboutReader.js should use a named function
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: Reader View (show other bugs)
: Trunk
: All Android
: P5 normal (vote)
: Firefox 18
Assigned To: Chelsea Carr
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-29 03:16 PDT by Lucas Rocha (:lucasr)
Modified: 2012-09-19 07:39 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified


Attachments
I added a name to the function being used by the observe() method (18.68 KB, patch)
2012-09-06 14:21 PDT, Chelsea Carr
no flags Details | Diff | Splinter Review
Patch showing 8 lines of context for the code change (806 bytes, patch)
2012-09-07 05:29 PDT, Chelsea Carr
jaws: review+
Details | Diff | Splinter Review
Renamed the the observe function to Reader_observe(... (18.70 KB, patch)
2012-09-08 10:12 PDT, Chelsea Carr
no flags Details | Diff | Splinter Review
Patch of method Observe() changes (806 bytes, patch)
2012-09-10 07:36 PDT, Chelsea Carr
carrche2: checkin+
Details | Diff | Splinter Review
Edited the name of the observe() method (717 bytes, patch)
2012-09-10 08:53 PDT, Chelsea Carr
jaws: review+
Details | Diff | Splinter Review

Description Lucas Rocha (:lucasr) 2012-08-29 03:16:15 PDT
Just like all other methods in the AboutReader prototype.
Comment 1 Chelsea Carr 2012-09-06 13:10:12 PDT
I'm a little confused on what this ticket is asking for.  So the current observe method is:

observe: function(aMessage, aTopic, aData){
. . .
}

Do you want it to be something like:

observe: function Reader_observe(aMessage, aTopic, aData) {
. . . 
}

Or should observe() be calling another function?
Comment 2 Brian Nicholson (:bnicholson) 2012-09-06 13:48:54 PDT
(In reply to Chelsea Carr from comment #1)
> Do you want it to be something like:
> 
> observe: function Reader_observe(aMessage, aTopic, aData) {
> . . . 
> }
> 

Yeah, that's all this bug is for. I believe certain types of logging can print the function name if it's given, and it appears this one was accidentally forgotten.
Comment 3 Chelsea Carr 2012-09-06 14:21:54 PDT
Created attachment 658998 [details] [diff] [review]
I added a name to the function being used by the observe() method

I had one other question about the observe() method, not really pertaining to the bug, but more out of curiosity.  I couldn't seem to find where this method is used in the code.  Is it dynamically called by any observers that are added to the app?
Comment 4 Lucas Rocha (:lucasr) 2012-09-07 01:46:05 PDT
Thanks Chelsea! See my comments below.

(In reply to Chelsea Carr from comment #3)
> Created attachment 658998 [details] [diff] [review]
> I added a name to the function being used by the observe() method

You have to submit a patch instead of the whole file you've changed. Have a look at this page for instructions:

  https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch

> I had one other question about the observe() method, not really pertaining
> to the bug, but more out of curiosity.  I couldn't seem to find where this
> method is used in the code.  Is it dynamically called by any observers that
> are added to the app?

The observe() method is part of the nsiObserver interface:
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIObserver

observe() will be used after you add an observer to track certain messages (look for an addObserver() call in the same file):
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/aboutReader.js#31
Comment 5 Chelsea Carr 2012-09-07 05:29:56 PDT
Created attachment 659225 [details] [diff] [review]
Patch showing 8 lines of context for the code change

Lucas- Thanks for the info on the observe method and how to submit patches.
Comment 6 Jared Wein [:jaws] (please needinfo? me) 2012-09-07 05:39:07 PDT
Comment on attachment 659225 [details] [diff] [review]
Patch showing 8 lines of context for the code change

Looks good to me.

Chelsea, can you add a checkin comment and your name to the patch? See https://developer.mozilla.org/en-US/docs/Creating_a_patch_that_can_be_checked_in for steps on how to do it.

Once you do that, you can upload the new patch and set the "checkin-needed" Keyword on the bug.
Comment 7 Chelsea Carr 2012-09-08 10:12:05 PDT
Created attachment 659501 [details] [diff] [review]
Renamed the the observe function to Reader_observe(...

Renamed the the observe function to Reader_observe(...
Comment 8 Jared Wein [:jaws] (please needinfo? me) 2012-09-10 01:13:56 PDT
Chelsea: The latest version of the patch that was uploaded is the full file. Can you upload the patch file? If you are using Mercurial Queues, then it will be located in .hg/patches inside of your mozilla-central folder.
Comment 9 Chelsea Carr 2012-09-10 07:36:32 PDT
Created attachment 659710 [details] [diff] [review]
Patch of method Observe() changes

I can't seem to get MQ to work.  I uncommented the line "mq=" line in mercurial.ini and also added the line "git =1".  Yet it isn't quite working.  I couldn't seem to find a tutorial for mac, only ones for windows.  Any suggestions?  I did manage to create a patch, but I don't think it is quite the same as if I had used MQ.
Comment 10 Chelsea Carr 2012-09-10 08:53:13 PDT
Created attachment 659731 [details] [diff] [review]
Edited the name of the observe() method

Hopefully this works now. . .
Comment 11 Jared Wein [:jaws] (please needinfo? me) 2012-09-11 01:22:10 PDT
Comment on attachment 659731 [details] [diff] [review]
Edited the name of the observe() method

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

This looks fine, although I would have used "Bug 786604: Named the observe function".

Also, this little bit can be confusing, but when a patch is uploaded it usually isn't marked checkin+. There is "Keyword" field on the bug itself (not the attachment), that is used for saying something needs to be checked in. The keyword is "checkin-needed". The difference here can be confusing. You can hover over the labels (review, feedback, checkin) to get descriptions about how each of those fields are used.
Comment 12 Lucas Rocha (:lucasr) 2012-09-11 06:24:06 PDT
Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/236b043aca97
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-09-11 18:47:28 PDT
https://hg.mozilla.org/mozilla-central/rev/236b043aca97
Comment 14 Cristian Nicolae (:xti) 2012-09-19 07:39:25 PDT
Changes were applied on the latest Nightly. Closing bug as verified fixed on:

Firefox 18.0a1 (2012-09-19)
Device: Galaxy Note
OS: Android 4.0.4

Note You need to log in before you can comment on or make changes to this bug.