1132341 - Geolocation prompt immediately dismisses on google maps

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Go to Google maps and look up directions. Choose "my location" for the starting point. Prompt will flash up and disappear

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

While on maps.google.com, go to Menu > Page > Edit Site Settings

Is "Share Location" already checked?

If so, clear it and try loading 'maps.google.com' by tapping the search suggestion that eventually appears as you type it out. Do you see the doorhanger appear and disappear?

I wonder if it's a redirect. On one of my phones, I just loaded 'maps.google.com' and I saw a quick flash as the site redirected (or I think it redirected). When in this situation, I see a spinning throbber in the searchbox on the webpage.

If I tap on the "bulls-eye" button to find my location, the doorhanger appears and I can share my location. Now, I don't see the doorhanger flash when loading the website.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: maps-doorhanger-log.txt

Here is a simple log of opening 'maps.google.com' when I see the dooghanger appear/disappear.

I see two LOCATION_CHANGE events

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

I bet we get confused over the "new" URL:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/DoorHangerPopup.java#131

It could just be a query string or anchor change.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #4)
After a test:

> 1. Why are we trying to add the doorhanger twice, without a location change
> in between?

The webpage seems to be calling the API twice, causing two attempts to display the prompt.

> 2. We do remove the doorhanger due to a location change
> 
> I can try to add the same "fix" Firefox (Desktop and Mobile) use for login
> prompts: Add ways to allow the doorhanger to persist between fast page
> redirects.

This works. We just need to think about the repercussions.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: share-location-doorhanger v0.1

This patch adds a new persist style to doorhangers: persistDomain

The doorhanger will stay visible across pageloads as long as the domain stays the same.

Take a look at the approach and see what you think.

Comment 7

[:Margaret Leibovic]

Comment on attachment 8573311 [details] [diff] [review]
share-location-doorhanger v0.1

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

Sorry for the super slow feedback. This is generally looking good, although it's probably going to conflict with liuche's recent doorhanger work.

::: mobile/android/base/DoorHangerPopup.java
@@ +131,5 @@
>              case LOCATION_CHANGE:
>                  // Only remove doorhangers if the popup is hidden or if we're navigating to a new URL
> +                if (!isShowing() || !data.equals(tab.getURL())) {
> +                    boolean sameHost = StringUtils.getHost(data.toString()).equals(StringUtils.getHost(tab.getURL()));
> +                    removeTransientDoorHangers(tab.getId(), sameHost);

Instead of doing this host comparison here, could we do it somewhere more central and pass it along with the location change message? We already pass the base domain along with the location change message from JS -> Java:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4531

::: mobile/android/base/util/StringUtils.java
@@ +246,5 @@
> +     *
> +     * @param url
> +     * @return host
> +     */
> +    public static String getHost(String url){

We don't already have code that does this somewhere? Is it that we only have it in JS right now?

::: mobile/android/base/widget/DoorHanger.java
@@ +341,2 @@
>       */
> +    public boolean shouldRemove(boolean isShowing, PersistDomain domainChange) {

Why do you need this tri-state enum when you just use this like a boolean? You don't even use the UNKNOWN state, so I think you should get rid of the enum and just use a boolean.

::: mobile/android/components/ContentPermissionPrompt.js
@@ +125,5 @@
>  
>      let requestor = chromeWin.BrowserApp.manifest ? "'" + chromeWin.BrowserApp.manifest.name + "'" : request.principal.URI.host;
>      let message = browserBundle.formatStringFromName(entityName + ".ask", [requestor], 1);
> +    let options = {
> +      persistSameDomain: true,

You should also update the doorhanger options docs here:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2212

Comment 9

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.