Closed Bug 1308600 Opened 3 years ago Closed 3 years ago

Remove ServiceReady, GetCurrentPositionReady, and WatchPositionReady methods from Geolocation

Categories

(Core :: DOM: Geolocation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jdm, Assigned: adamtomasv, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [lang=c++])

Attachments

(1 file, 1 obsolete file)

Geolocation::ServiceReady is no longer used by any code, so we should remove it. That means that GetCurrentPositionReady and WatchPositionReady only have one caller each, so we should merge each of those methods with GetCurrentPosition and WatchPosition. This will simplify the code in the file and make it easier to reason about.

Code: dom/geolocation/Geolocation.cpp, dom/geolocation/Geolocation.h

No need to run any tests; if the changes compile, then it's good enough to submit a patch for review.
i a beginner and unable to find code , could you please specify where is dom/geolocation directory
Attached patch for the suggested changes.
Assignee: nobody → adamtomasv
Comment on attachment 8799149 [details] [diff] [review]
Removed ServiceReady(), Merged *Ready methods into GetCurrentPosition, WatchPosition

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

This looks great! We'll need to add a commit message like "Bug 1308600 - Remove unused ServiceReady method from Geolocation. r=jdm" to the patch as well.
Attachment #8799149 - Flags: review+
Hi Josh, I added the commit message (from instructions here: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch).
Attachment #8799149 - Attachment is obsolete: true
Attachment #8799537 - Flags: review+
Thanks! Do you have access to our tryserver (https://wiki.mozilla.org/ReleaseEngineering/TryServer)? If not, please apply for level 1 access by following the steps at https://www.mozilla.org/hacking/committer/ ; I will vouch for you. This will allow you to ensure that your changes build on the major supported platforms (windows, linux, mac).
Thanks for help, Josh. I am continuing with the ticket, I (hopefully) got a correct setup of the accounts to be able to push to try. This will be my next step.
I let the tryserver to build with my patch. 
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f953e9e3ef2
Josh, what should be the next step?
Flags: needinfo?(josh)
Looks good! Go ahead and add the `checkin-needed` keyword to the Keywords field in this bug, and then the patch will be automatically merged for you.
Flags: needinfo?(josh)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c784f8b12ed
Remove unused ServiceReady method from Geolocation. r=jdm
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4c784f8b12ed
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.