Closed Bug 786604 Opened 9 years ago Closed 9 years ago

observe() method in aboutReader.js should use a named function

Categories

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

All
Android
defect

Tracking

(firefox18 verified)

VERIFIED FIXED
Firefox 18
Tracking Status
firefox18 --- verified

People

(Reporter: lucasr, Assigned: carrche2)

Details

Attachments

(1 file, 4 obsolete files)

Just like all other methods in the AboutReader prototype.
Priority: -- → P5
Assignee: nobody → carrche2
Status: NEW → ASSIGNED
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?
(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.
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?
Attachment #658998 - Flags: review?(jaws)
Attachment #658998 - Flags: checkin?(bnicholson)
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
Lucas- Thanks for the info on the observe method and how to submit patches.
Attachment #659225 - Flags: review?(jaws)
Attachment #659225 - Flags: review?(bnicholson)
Attachment #659225 - Flags: checkin?
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.
Attachment #659225 - Attachment is patch: true
Attachment #659225 - Flags: review?(jaws)
Attachment #659225 - Flags: review?(bnicholson)
Attachment #659225 - Flags: review+
Renamed the the observe function to Reader_observe(...
Attachment #659501 - Flags: checkin+
Attachment #658998 - Attachment is patch: true
Attachment #658998 - Flags: review?(jaws)
Attachment #658998 - Flags: checkin?(bnicholson)
Attachment #659225 - Attachment is obsolete: true
Attachment #659225 - Flags: checkin?
Attachment #658998 - Attachment is obsolete: true
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.
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.
Attachment #659710 - Flags: checkin+
Hopefully this works now. . .
Attachment #659501 - Attachment is obsolete: true
Attachment #659710 - Attachment is obsolete: true
Attachment #659731 - Flags: checkin+
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.
Attachment #659731 - Flags: checkin+ → review+
https://hg.mozilla.org/mozilla-central/rev/236b043aca97
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
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
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.