Remove Reader_foo function names from method declarations in AboutReader.jsm

RESOLVED FIXED in Firefox 44

Status

()

Toolkit
Reader Mode
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Margaret, Assigned: Lewis Cowper, Mentored, NeedInfo)

Tracking

Trunk
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
This is an old convention that pre-dates more useful error logging. Let's update our code.

To fix this bug, just remove the function names in AboutReader, e.g. convert this:
handleEvent: function Reader_handleEvent(aEvent) {

To this:
handleEvent: function(aEvent) {
(Assignee)

Comment 1

3 years ago
I'd quite like to handle this, but if someone wants to take it as their first bug, they're more than welcome.
(Reporter)

Comment 2

3 years ago
(In reply to Lewis Cowper from comment #1)
> I'd quite like to handle this, but if someone wants to take it as their
> first bug, they're more than welcome.

There are always more bugs, so feel free to take this on if you like! It would be a nice clean-up.
(Assignee)

Comment 3

3 years ago
Created attachment 8567263 [details] [diff] [review]
reader_foo-patch.diff

That looks like it should take care of it. :)
Flags: needinfo?(margaret.leibovic)
(Reporter)

Comment 4

3 years ago
Comment on attachment 8567263 [details] [diff] [review]
reader_foo-patch.diff

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

Looks good! In the future, you should set the review? flag on a patch, instead of using needinfo? on the bug. That will help make sure your patch doesn't get lost in the shuffle! :)
Attachment #8567263 - Flags: review+
(Reporter)

Updated

3 years ago
Assignee: nobody → lewis.cowper
Flags: needinfo?(margaret.leibovic)
Keywords: checkin-needed
(Assignee)

Comment 5

3 years ago
Ah, I didn't know whether the review flag was for looking for reviews or responding to them. Thanks for the tip!
(Reporter)

Comment 6

3 years ago
Lewis, your patch failed to apply to the lastest fx-team. Could you update your tree to rebase?

I also just landed a few more AboutReader.jsm changes on fx-team, so unfortunately mozilla-central won't be caught up yet.
Flags: needinfo?(lewis.cowper)
Keywords: checkin-needed
(Assignee)

Comment 7

3 years ago
Sure! I've not got fx-team locally, so I'll take a bit of time to download and set up, it might be tomorrow by the time I get the chance to sort it out.
Flags: needinfo?(lewis.cowper)
(Assignee)

Comment 8

3 years ago
Created attachment 8567350 [details] [diff] [review]
fx-team-patch.diff

As it turns out, it was somewhat quicker than I originally thought.
Attachment #8567263 - Attachment is obsolete: true
Attachment #8567350 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8567350 - Flags: review+ → review?(margaret.leibovic)
(Reporter)

Comment 9

3 years ago
Comment on attachment 8567350 [details] [diff] [review]
fx-team-patch.diff

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

Hm, this is still failing to apply. Maybe I was too slow to review it and more changes landed since you posted this :(
(Reporter)

Comment 10

3 years ago
I'm sorry for all the churn! Would you mind rebasing your patch again? I'll pay close attention and make sure to land it for you quickly before it bit-rots again :)
Flags: needinfo?(lewis.cowper)
(Reporter)

Comment 11

3 years ago
Comment on attachment 8567350 [details] [diff] [review]
fx-team-patch.diff

Clearing review while waiting for a new patch.
Attachment #8567350 - Flags: review?(margaret.leibovic)
Created attachment 8665125 [details]
MozReview Request: Bug 1134940 - Modernize ReaderMode function declarations. r?nalexander

Bug 1134940 - Modernize ReaderMode function declarations. r?nalexander
Attachment #8665125 - Flags: review?(nalexander)
Comment on attachment 8665125 [details]
MozReview Request: Bug 1134940 - Modernize ReaderMode function declarations. r?nalexander

https://reviewboard.mozilla.org/r/20147/#review18087

Looking good.
Attachment #8665125 - Flags: review?(nalexander) → review+

Updated

2 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5906d37343fa
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment on attachment 8665125 [details]
MozReview Request: Bug 1134940 - Modernize ReaderMode function declarations. r?nalexander

Bug 1134940 - Modernize ReaderMode function declarations. r?nalexander
Attachment #8665125 - Flags: review+ → review?(nalexander)
Created attachment 8666189 [details]
MozReview Request: Bug: 1208626 Add needed parameter to getHistogramById r?rstrong

Bug: 1208626 Add needed parameter to getHistogramById r?rstrong
Attachment #8666189 - Flags: review?(robert.strong.bugs)

Updated

2 years ago
Attachment #8666189 - Attachment is obsolete: true
Attachment #8666189 - Flags: review?(robert.strong.bugs)

Updated

2 years ago
Attachment #8665125 - Attachment is obsolete: true
Attachment #8665125 - Flags: review?(nalexander)
You need to log in before you can comment on or make changes to this bug.