The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 18

Status

()

Firefox for Android
Reader View
P5
normal
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: lucasr, Assigned: Chelsea Carr)

Tracking

Trunk
Firefox 18
All
Android
Points:
---

Firefox Tracking Flags

(firefox18 verified)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
Just like all other methods in the AboutReader prototype.
(Reporter)

Updated

5 years ago
Priority: -- → P5
Assignee: nobody → carrche2
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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?
Attachment #658998 - Flags: review?(jaws)
Attachment #658998 - Flags: checkin?(bnicholson)
(Reporter)

Comment 4

5 years ago
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
(Assignee)

Comment 5

5 years ago
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.
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+
(Assignee)

Comment 7

5 years ago
Created attachment 659501 [details] [diff] [review]
Renamed the the observe function to Reader_observe(...

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.
Attachment #659501 - Flags: checkin+
(Assignee)

Comment 9

5 years ago
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.
Attachment #659710 - Flags: checkin+
(Assignee)

Comment 10

5 years ago
Created attachment 659731 [details] [diff] [review]
Edited the name of the observe() method

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+
(Reporter)

Comment 12

5 years ago
Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/236b043aca97
https://hg.mozilla.org/mozilla-central/rev/236b043aca97
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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
status-firefox18: --- → verified
You need to log in before you can comment on or make changes to this bug.