1134537 - Eliminate GnomeVFS calls

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Alex Henrie]

Firefox's support for GnomeVFS has been disabled by default since 2012, and GnomeVFS itself has been deprecated much longer than that. At this point, the code is dead weight and it would be best to remove it altogether.

Fixing this bug will require two steps:
1. Remove all references to nsIGnomeVFSService from nsLocalFileUnix.
2. Remove gnomevfs from configure.in.

Comment 1

[Alex Henrie]

Attachment: [PATCH] Only support GIO in nsLocalFileUnix.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b03a73b4de6

This patch completes step 1 towards closing this bug.

Comment 2

[Nathan Froyd [:froydnj]]

I feel like there are some more steps:

1. Remove nsIGnomeVFSService references from nsLocalFileUnix, as you've done.
2. Remove nsIGnomeVFSService references from uriloader/exthandler/unix/{nsMIMEInfo,nsGNOMERegistry}.cpp.
3. Remove nsIGnomeVFSService and its implementation.
4. Remove gnomevfs from configure.in.

Comment 3

[Nathan Froyd [:froydnj]]

Comment on attachment 8566417 [details] [diff] [review]
[PATCH] Only support GIO in nsLocalFileUnix.

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

::: xpcom/io/nsLocalFileUnix.cpp
@@ +1984,2 @@
>    } else if (giovfs &&
>               NS_SUCCEEDED(giovfs->OrgFreedesktopFileManager1ShowItems(mPath))) {

Seems like we ought to be able to remove the |giovfs &&| part now, since that will always be true.

Comment 4

[Alex Henrie]

Attachment: [PATCH v2] Only support GIO in nsLocalFileUnix.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff5cad86e5db

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #3)
> Comment on attachment 8566417 [details] [diff] [review]
> [PATCH] Only support GIO in nsLocalFileUnix.
> 
> Review of attachment 8566417 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/io/nsLocalFileUnix.cpp
> @@ +1984,2 @@
> >    } else if (giovfs &&
> >               NS_SUCCEEDED(giovfs->OrgFreedesktopFileManager1ShowItems(mPath))) {
> 
> Seems like we ought to be able to remove the |giovfs &&| part now, since
> that will always be true.

Good catch, thanks.

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #2)
> 2. Remove nsIGnomeVFSService references from
> uriloader/exthandler/unix/{nsMIMEInfo,nsGNOMERegistry}.cpp.

Bug 694870 took care of that.

> 3. Remove nsIGnomeVFSService and its implementation.
> 4. Remove gnomevfs from configure.in.

Won't these two steps be resolved in the same patch?

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Alex Henrie from comment #4)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #2)
> > 2. Remove nsIGnomeVFSService references from
> > uriloader/exthandler/unix/{nsMIMEInfo,nsGNOMERegistry}.cpp.
> 
> Bug 694870 took care of that.

Ah, thanks for pointing that out.

> > 3. Remove nsIGnomeVFSService and its implementation.
> > 4. Remove gnomevfs from configure.in.
> 
> Won't these two steps be resolved in the same patch?

I would personally break them up into two patches, as they're two very distinct things, but if you're the one writing the patch, you get to choose how to break it up. :)

Comment 6

[Alex Henrie]

Requesting checkin of attachment 8566770 [details] [diff] [review]

I will submit a patch for the next step of this bug in a day or two.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/333249f6088d

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/333249f6088d

Comment 9

[Alex Henrie]

Attachment: [PATCH] Remove references to GnomeVFS from nsGnomeModule.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=98644a3f7e63

Comment 10

[Alex Henrie]

Corrected Try link:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3e6eee12321

Comment 11

[poiru]

Comment on attachment 8567439 [details] [diff] [review]
[PATCH] Remove references to GnomeVFS from nsGnomeModule.

Redirecting r? to Nathan as he is a Toolkit peer. Be sure to check https://wiki.mozilla.org/Modules/All to find the correct reviewers.

Comment 12

[Alex Henrie]

(In reply to Birunthan Mohanathas [:poiru] from comment #11)
> Redirecting r? to Nathan as he is a Toolkit peer. Be sure to check
> https://wiki.mozilla.org/Modules/All to find the correct reviewers.

Thanks for the tip. I was selecting reviewers based on `hg blame`.

Comment 13

[Ian Neal]

(In reply to Alex Henrie from comment #0)
> 2. Remove gnomevfs from configure.in.
Does this step include removing #ifdef MOZ_ENABLE_GNOMEVFS from the package-manifest.in files?

image/decoders/icon/gtk/nsIconChannel.cpp makes reference to libgnomevfs
netwerk/base/nsIOService.cpp makes reference to moz-gnomevfs
Are those relevant to this bug?

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8567439 [details] [diff] [review]
[PATCH] Remove references to GnomeVFS from nsGnomeModule.

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

This is a good start, but you also need to |hg rm| nsGnomeVFSService.{cpp,h} and xpcom/system/nsIGnomeVFSService.idl, and remove their respective references from moz.build files, as those are all dead code once you've deleted the bits in nsGnomeModule.cpp.

Comment 15

[Alex Henrie]

Attachment: [PATCH Delete nsGnomeVFSService.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f6e261591af5

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #14)
> This is a good start, but you also need to |hg rm| nsGnomeVFSService.{cpp,h}
> and xpcom/system/nsIGnomeVFSService.idl, and remove their respective
> references from moz.build files, as those are all dead code once you've
> deleted the bits in nsGnomeModule.cpp.

Done.

(In reply to Ian Neal from comment #13)
> (In reply to Alex Henrie from comment #0)
> > 2. Remove gnomevfs from configure.in.
> Does this step include removing #ifdef MOZ_ENABLE_GNOMEVFS from the
> package-manifest.in files?
> 
> image/decoders/icon/gtk/nsIconChannel.cpp makes reference to libgnomevfs
> netwerk/base/nsIOService.cpp makes reference to moz-gnomevfs
> Are those relevant to this bug?

You're right; I was overly optimistic when I said GnomeVFS could be removed in only 2 steps. Before this bug can be marked resolved, there are still changes to be made to netwerk/base/nsIOService.cpp, extensions/gnomevfs, image/decoders/icon/gtk/nsIconChannel.cpp, configure.in, browser/installer/package-manifest.in, and mobile/android/installer/package-manifest.in.

Comment 16

[Nathan Froyd [:froydnj]]

Also, if you don't want bugs to be closed when their patches are merged to mozilla-central, you should add the leave-open keyword to the bug (and remember to remove it when all the work in the bug has been completed, of course).

Given that the most recent patch completely removes nsIGnomeVFSService, it might be good to complete the work mentioned in comment 15 in other bugs, so that it's obvious that work on the dependent bugs here can proceed.

Comment 17

[Alex Henrie]

Requesting checkin of attachment 8567549 [details] [diff] [review]

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/826578768e85

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/826578768e85

Comment 20

[Alex Henrie]

Attachment: [PATCH] Only support GIO in nsIconChannel.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f232b51bd96

libgnome, libgnomeui, and libgnomevfs are all deprecated. See https://wiki.gnome.org/Attic/LibgnomeMustDie

Comment 21

[Alex Henrie]

Attachment: [PATCH v2] Only support GIO in Linux's nsIconChannel.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f4f1790e3b0

Version 2 deletes InitWithGnome from the header file as well.

Comment 22

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8572446 [details] [diff] [review]
[PATCH v2] Only support GIO in Linux's nsIconChannel.

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

I love it. Thanks.

Comment 23

[Alex Henrie]

Requesting checkin of attachment 8572446 [details] [diff] [review]

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f72ee1bf5567

Comment 25

[Alex Henrie]

https://hg.mozilla.org/mozilla-central/rev/826578768e85

Comment 26

[Alex Henrie]

Attachment: [PATCH] Only support GIO in nsIOService.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3ea6f96cdbc

By the way, I edited purple-url-handler.desktop from https://bugzilla.mozilla.org/show_bug.cgi?id=694870#c5 to handle the "foobarirc:" protocol instead of the "irc:" protocol, and then Firefox gained the ability to open "foobarirc:" links. So we can mark bug 234714 as fixed.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f72ee1bf5567

Comment 28

[Alex Henrie]

Requesting checkin of attachment 8574254 [details] [diff] [review]

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc720b50b4e9

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bc720b50b4e9

Comment 31

[Alex Henrie]

Attachment: [PATCH] Delete GnomeVFS extension.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ea443f55c6e

I realize that this patch touches a lot of files (why is why I have tagged 5 separate reviewers), but I think that it will be easiest to delete all the remaining references to the GnomeVFS extension at the same time.

Comment 32

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8575061 [details] [diff] [review]
[PATCH] Delete GnomeVFS extension.

We don't need signoff from each of these module owners to remove simple references to disabled-by-default obsolete code.

A build peer to review the build changes is reasonable.

Someone with a bit more context about comment 0 should probably sign off on killing it. Perhaps Roc can do that (or suggest someone else who can).

Comment 33

[Alex Henrie]

Requesting checkin of attachment 8575061 [details] [diff] [review]

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2706515176c8

Comment 35

[Gregory Szorc [:gps]]

Comment on attachment 8575061 [details] [diff] [review]
[PATCH] Delete GnomeVFS extension.

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

Build bits look fine. Always happy to review diffs that delete code :)

Comment 36

[Alex Henrie]

(In reply to Gregory Szorc [:gps] from comment #35)
> Build bits look fine. Always happy to review diffs that delete code :)

Ah, sorry I didn't wait for your review. I thought roc's review was sufficient. Glad you like it though!

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2706515176c8