Alex Henrie Assignee
	Description • 10 years ago

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.

	Alex Henrie Assignee
	Comment 1 • 10 years ago

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

This patch completes step 1 towards closing this bug.

	Nathan Froyd [:froydnj]
	Comment 2 • 10 years ago

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.

	Nathan Froyd [:froydnj]
	Comment 3 • 10 years ago

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.

	Alex Henrie Assignee
	Comment 4 • 10 years ago

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?

	Nathan Froyd [:froydnj]
	Comment 5 • 10 years ago

(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. :)

	Alex Henrie Assignee
	Comment 6 • 10 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 7 • 10 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 8 • 10 years ago

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

	Alex Henrie Assignee
	Comment 9 • 10 years ago

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

	Alex Henrie Assignee
	Comment 10 • 10 years ago

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

	Biru Mohanathas [:poiru]
	Comment 11 • 10 years ago

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.

	Alex Henrie Assignee
	Comment 12 • 10 years ago

(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`.

	Ian Neal
	Comment 13 • 10 years ago

(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?

	Nathan Froyd [:froydnj]
	Comment 14 • 10 years ago

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.

	Alex Henrie Assignee
	Comment 15 • 10 years ago

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.

	Nathan Froyd [:froydnj]
	Comment 16 • 10 years ago

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.

	Alex Henrie Assignee
	Comment 17 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 18 • 10 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 19 • 10 years ago

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

	Alex Henrie Assignee
	Comment 20 • 10 years ago

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

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

	Alex Henrie Assignee
	Comment 21 • 10 years ago

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

Version 2 deletes InitWithGnome from the header file as well.

	Seth Fowler [:seth] [:s2h]
	Comment 22 • 10 years ago

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.

	Alex Henrie Assignee
	Comment 23 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 24 • 10 years ago

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

	Alex Henrie Assignee
	Comment 25 • 10 years ago

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

	Alex Henrie Assignee
	Comment 26 • 10 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 10 years ago

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

	Alex Henrie Assignee
	Comment 28 • 10 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 10 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 30 • 10 years ago

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

	Alex Henrie Assignee
	Comment 31 • 10 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 32 • 10 years ago

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).

	Alex Henrie Assignee
	Comment 33 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 34 • 10 years ago

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

	Gregory Szorc [:gps]
	Comment 35 • 10 years ago

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 :)

	Alex Henrie Assignee
	Comment 36 • 10 years ago

(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!

	Ryan VanderMeulen [:RyanVM]
	Comment 37 • 10 years ago

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