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

Status: VERIFIED FIXED

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

Description

[Lucas Rocha (:lucasr)]

Just like all other methods in the AboutReader prototype.

Comment 1

[Chelsea Carr]

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

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

Attachment: 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)]

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]

Attachment: 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)]

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]

Attachment: Renamed the the observe function to Reader_observe(...

Renamed the the observe function to Reader_observe(...

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

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]

Attachment: 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]

Attachment: Edited the name of the observe() method

Hopefully this works now. . .

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

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

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/236b043aca97

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/236b043aca97

Comment 14

[Cristian Nicolae (:xti)]

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