Extract MLS fallback logic from Core Location provider for use by other providers

RESOLVED FIXED in Firefox 39

Status

()

Core
Geolocation
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: garvan, Assigned: garvan)

Tracking

unspecified
mozilla39
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
This is an extension of bug 1121265.

The MLS fallback logic can be moved into its own class.

It appears that the Windows 8 geolocation provider (bug 1129633) also requires an MLS fallback. I am hoping this refactor will be helpful for that bug.
(Assignee)

Comment 1

3 years ago
Created attachment 8565691 [details] [diff] [review]
Moved mls fallback code to new class MLSFallback
Attachment #8565691 - Flags: review?(josh)
(Assignee)

Updated

3 years ago
Summary: Extract MLS Fallback logic for use by other providers from Core Location provider → Extract MLS fallback logic from Core Location provider for use by other providers
(Assignee)

Comment 2

3 years ago
Comment on attachment 8565691 [details] [diff] [review]
Moved mls fallback code to new class MLSFallback

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

::: dom/geolocation/MLSFallback.cpp
@@ +48,5 @@
> +NS_IMETHODIMP
> +MLSFallback::Notify(nsITimer* aTimer)
> +{
> +  CreateMLSFallbackProvider();
> +  return NS_OK;

return CreateMLSFallbackProvider();

::: dom/geolocation/MLSFallback.h
@@ +30,5 @@
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSITIMERCALLBACK
> +
> +  MLSFallback(uint32_t delayMs = 2000);
> +  nsresult Startup(nsCOMPtr<nsIGeolocationUpdate> watcher);

Hmm, I think our convention is pass the unboxed pointer (as in nsIGeolocationUpdate*)

@@ +39,5 @@
> +  virtual ~MLSFallback(){}
> +  nsCOMPtr<nsITimer> mHandoffTimer;
> +  nsCOMPtr<nsIGeolocationProvider> mMLSFallbackProvider;
> +  nsCOMPtr<nsIGeolocationUpdate> mUpdateWatcher;
> +  uint32_t mDelayMs;

const
(Assignee)

Comment 3

3 years ago
Created attachment 8569460 [details] [diff] [review]
Moved mls fallback code to new class MLSFallback

This patch: Moved mls fallback code to new class MLSFallback.

Updated the prev. patch for some minor nits I noticed.
Attachment #8565691 - Attachment is obsolete: true
Attachment #8565691 - Flags: review?(josh)
Attachment #8569460 - Flags: review?(josh)

Comment 4

3 years ago
Comment on attachment 8569460 [details] [diff] [review]
Moved mls fallback code to new class MLSFallback

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

::: dom/geolocation/MLSFallback.cpp
@@ +1,1 @@
> +#include "MLSFallback.h"

Add a license header.

@@ +6,5 @@
> +
> +MLSFallback::MLSFallback(uint32_t delay)
> +: mHandoffTimer(nullptr),
> +  mMLSFallbackProvider(nullptr),
> +  mUpdateWatcher(nullptr),

No need to initialize the smart pointer members; they default to null.

@@ +12,5 @@
> +{
> +}
> +
> +nsresult
> +MLSFallback::Startup(nsCOMPtr<nsIGeolocationUpdate> aWatcher)

Passing nsCOMPtr by value is uncommon.

::: dom/geolocation/MLSFallback.h
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#include "nsCOMPtr.h"
> +#include "nsIGeolocationProvider.h"
> +#include "nsITimer.h"

Both of these can be forward declarations.

@@ +17,5 @@
> + The intent is that the primary provider is started, then MLSFallback
> + is started with sufficient delay that the primary provider will respond first
> + if successful (in the majority of cases).
> +
> + MLS has an ave response of 3s, so with the 2s default delay, a response can

s/ave/average/

@@ +29,5 @@
> +public:
> +  NS_DECL_ISUPPORTS
> +  NS_DECL_NSITIMERCALLBACK
> +
> +  MLSFallback(uint32_t delayMs = 2000);

Constructors with one argument should be explicit.

@@ +35,5 @@
> +  nsresult Shutdown();
> +
> +private:
> +  nsresult CreateMLSFallbackProvider();
> +  virtual ~MLSFallback(){}

This will need to be defined in the cpp instead of inline.

::: dom/system/mac/CoreLocationLocationProvider.h
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsCOMPtr.h"
>  #include "nsIGeolocationProvider.h"
> +#include "MLSFallback.h"

This can just be a forward declaration.
Attachment #8569460 - Flags: review?(josh) → review+
(Assignee)

Comment 5

3 years ago
Created attachment 8570536 [details] [diff] [review]
moved mls fallback code to new class MLSFallback

Thanks Josh, addressed all the review comments and updated patch.
Attachment #8569460 - Attachment is obsolete: true
Attachment #8570536 - Flags: review+
(Assignee)

Comment 6

3 years ago
Created attachment 8570539 [details] [diff] [review]
Moved mls fallback code to new class MLSFallback

Just uploaded the patch for a different bug, _this_ is the patch.
Attachment #8570536 - Attachment is obsolete: true
Attachment #8570539 - Flags: review+
(Assignee)

Comment 7

3 years ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d029ea83925
(Assignee)

Comment 8

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/75456ea22fda
https://hg.mozilla.org/mozilla-central/rev/75456ea22fda
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.