Closed Bug 512305 Opened 15 years ago Closed 6 years ago

Set Firefox as the default web browser using xdg-settings

Categories

(Firefox :: Shell Integration, enhancement)

x86
Linux
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox59 + fixed
firefox60 + fixed

People

(Reporter: thestig, Assigned: olivier)

References

Details

Attachments

(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.13) Gecko/2009080317 Ubuntu/8.04 (hardy) Firefox/3.0.13
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.0.13) Gecko/2009080317 Ubuntu/8.04 (hardy) Firefox/3.0.13

The Chromium web browser uses the xdg-settings tool [1] on Linux to figure out
what the default web browser is, and set itself as the default web browser if
the user requested it. 

It would be great if all Linux web browsers have a common way of
getting/setting the default web browser. Can we change Firefox to use
xdg-settings if it's available? Xdg-settings is not widely deployed yet, but it
should be included in the next xdg-utils release.


Reproducible: Always
I'll second this. The feature has never worked for me as implemented and unless I disable checking, Firefox always asks whether I want it to make itself the default browser, even after it's purportedly already done so.
Snap packages (powered by Ubuntu) would benefit from this feature. Olivier Tilloy (cc'd) reported we can't set Firefox as the default browser (when packaged as Snap) without it. Olivier pointed out the code currently lives at [1]. It uses gconf. He added we could do something similar to what Chromium does: 

> xdg-settings set default-web-browser firefox.desktop

Finally, he said xdg-settings inside a snap doesn't work yet, but it is currently being implemented in snapd.

[1] https://dxr.mozilla.org/mozilla-central/rev/c38d22170d6f27e94c3c53093215d20255fab27a/browser/components/shell/nsGNOMEShellService.cpp#250
Blocks: snappy
Attached patch use-xdg-settings.patch (obsolete) — Splinter Review
Here is a first shot at using xdg-settings to get/set the default browser.
Feedback welcome.
Comment on attachment 8947234 [details] [diff] [review]
use-xdg-settings.patch

Martin, you've reviewed other code in this file. Could you take a look?
Attachment #8947234 - Flags: review?(stransky)
Comment on attachment 8947234 [details] [diff] [review]
use-xdg-settings.patch

Jan usually does this system integration stuff so moving to him. I guess the xdg can be also used for Flatpak builds.
Attachment #8947234 - Flags: review?(stransky) → review?(jhorak)
Comment on attachment 8947234 [details] [diff] [review]
use-xdg-settings.patch

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

::: browser/components/shell/nsGNOMEShellService.cpp
@@ +129,5 @@
> +{
> +  if (PR_GetEnv("SNAP") != nullptr) {
> +    return "firefox_firefox.desktop";
> +  }
> +  return "firefox.desktop";

What current implementation allows is to download upstream binary and set it as a default browser because the GIO creates and install the .desktop for us (by  giovfs->CreateAppFromCommand). 

This won't be possible with your changes. You can try to use xdg-settings as a fallback implementation if the gio one fails rather than replacing it.
Attachment #8947234 - Flags: review?(jhorak) → review-
Olivier: Is this something you can do? We're trying to get this change in fairly soon.
Flags: needinfo?(olivier)
Jan: thanks for the review!

Mike: I should be able to get to it within the next couple of days.
Flags: needinfo?(olivier)
Olivier: 

Is this by chance something you'll be able to do soon? We were hoping to get it in Firefox 59 (although that's very tight at this point)

Thanks
Attached patch snap-use-xdg-settings.patch (obsolete) — Splinter Review
Attaching an updated patch that preserves the existing logic and uses xdg-settings only when running as a snap.
Attachment #8954711 - Flags: review?(jhorak)
Is PR_GetEnv("SNAP") the best way to check for a Snap?
(In reply to Mike Kaply [:mkaply] from comment #12)
> Is PR_GetEnv("SNAP") the best way to check for a Snap?

It's the simplest way, especially if we don't want to add any further dependencies.
(In reply to Mike Kaply [:mkaply] from comment #12)
> Is PR_GetEnv("SNAP") the best way to check for a Snap?

We could be a tad more specific and compare the value of $SNAP_NAME to "firefox". But that's essentially the same mechanism.
Note that this makes it convenient to test the functionality when running a local build, not packaged as a snap: export SNAP before running and you simulate the snap confinement.
Sorry, I should have marked this earlier.

[Tracking Requested - why for this release]: We are looking to release a Firefox Snap for 59. This is a Snap only fix for 59. I plan to have it in today.
So look at this further (and Jan's comments), this seems like a temporary Snap solution as opposed to a full xdg-settings solution. I'll try to get it in for now, but ideally we should do this:

> You can try to use xdg-settings as a fallback implementation if the gio one fails rather than replacing it.
Comment on attachment 8954868 [details]
Bug 512305 - Use xdg-settings for Firefox Snap.

https://reviewboard.mozilla.org/r/224014/#review230174
Attachment #8954868 - Flags: review?(jhorak) → review+
See:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b774fd54f6f3f8be6434527017428af0bccdd038&selectedJob=165200234

/builds/worker/workspace/build/src/browser/components/shell/nsGNOMEShellService.cpp:218:58: error: ISO C++ forbids converting a string constant to 'gchar* {aka char*}
Please address the issues in try run (see above).
Comment on attachment 8954868 [details]
Bug 512305 - Use xdg-settings for Firefox Snap.

https://reviewboard.mozilla.org/r/224014/#review230298
Attachment #8954868 - Flags: review+
Updated patch that fixes the build error (it was only a warning when I built it locally, I should have been more careful).
Attachment #8947234 - Attachment is obsolete: true
Attachment #8954711 - Attachment is obsolete: true
Attachment #8954711 - Flags: review?(jhorak)
Attachment #8955182 - Flags: review?(jhorak)
Assuming this would need to land for 60 as well.
Once this lands on mozilla-central can you make sure someone requests uplift to m-r?
Flags: needinfo?(mozilla)
Comment on attachment 8954868 [details]
Bug 512305 - Use xdg-settings for Firefox Snap.

https://reviewboard.mozilla.org/r/224014/#review230552

Good. Thanks.
Attachment #8954868 - Flags: review?(jhorak) → review+
Comment on attachment 8954868 [details]
Bug 512305 - Use xdg-settings for Firefox Snap.

Approval Request Comment
[Feature/Bug causing the regression]: Setting default browser on Snap isn't working
[User impact if declined]: No ability to set default browser on Snap
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Snap/Linux specific.
[String changes made/needed]: None
Flags: needinfo?(mozilla)
Attachment #8954868 - Flags: approval-mozilla-release?
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/581dc5ffd572
Use xdg-settings for Firefox Snap. r=jhorak
https://hg.mozilla.org/mozilla-central/rev/581dc5ffd572
Status: UNCONFIRMED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Assignee: nobody → olivier
Comment on attachment 8954868 [details]
Bug 512305 - Use xdg-settings for Firefox Snap.

Fixes setting Firefox as the default browser when using Snap builds. Approved for 59rc1.
Attachment #8954868 - Flags: approval-mozilla-release? → approval-mozilla-release+
Attachment #8955182 - Flags: review?(jhorak)
See Also: → 1680527
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: