Closed Bug 1134537 Opened 5 years ago Closed 5 years ago

Eliminate GnomeVFS calls

Categories

(Core :: XPCOM, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: alexhenrie24, Assigned: alexhenrie24)

References

Details

Attachments

(5 files, 3 obsolete files)

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.
Depends on: 694870, 713802
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b03a73b4de6

This patch completes step 1 towards closing this bug.
Assignee: nobody → alexhenrie24
Attachment #8566417 - Flags: review?(roc)
Attachment #8566417 - Flags: review?(karlt)
Attachment #8566417 - Flags: review?(birunthan)
Component: Build Config → XPCOM
Attachment #8566417 - Flags: review?(roc)
Attachment #8566417 - Flags: review?(nfroyd)
Attachment #8566417 - Flags: review?(karlt)
Attachment #8566417 - Flags: review?(birunthan)
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 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.
Attachment #8566417 - Flags: review?(nfroyd) → review+
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?
Attachment #8566417 - Attachment is obsolete: true
Attachment #8566770 - Flags: review?(nfroyd)
Attachment #8566770 - Flags: review?(nfroyd) → review+
(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. :)
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/333249f6088d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Summary: Eliminate nsIGnomeVFSService → Eliminate GnomeVFS calls
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #8567439 - Flags: review?(nfroyd)
Attachment #8567439 - Flags: review?(birunthan)
Attachment #8567439 - Flags: review?(benjamin)
(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`.
(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?
Flags: needinfo?(alexhenrie24)
Blocks: 1135352
Blocks: 1135353
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.
Attachment #8567439 - Flags: review?(nfroyd) → feedback+
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.
Attachment #8567439 - Attachment is obsolete: true
Flags: needinfo?(alexhenrie24)
Attachment #8567549 - Flags: review?(nfroyd)
Attachment #8567549 - Flags: review?(nfroyd) → review+
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.
No longer blocks: 1135352
Depends on: 1135352
No longer blocks: 1135353
Depends on: 1135353
Requesting checkin of attachment 8567549 [details] [diff] [review]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f232b51bd96

libgnome, libgnomeui, and libgnomevfs are all deprecated. See https://wiki.gnome.org/Attic/LibgnomeMustDie
Attachment #8572440 - Flags: review?(seth)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f4f1790e3b0

Version 2 deletes InitWithGnome from the header file as well.
Attachment #8572440 - Attachment is obsolete: true
Attachment #8572440 - Flags: review?(seth)
Attachment #8572446 - Flags: review?(seth)
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.
Attachment #8572446 - Flags: review?(seth) → review+
Requesting checkin of attachment 8572446 [details] [diff] [review]
Keywords: checkin-needed
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.
Attachment #8574254 - Flags: review?(mcmanus)
Attachment #8574254 - Flags: review?(mcmanus) → review+
Requesting checkin of attachment 8574254 [details] [diff] [review]
Keywords: checkin-needed
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.
Attachment #8575061 - Flags: review?(mark.finkle)
Attachment #8575061 - Flags: review?(gps)
Attachment #8575061 - Flags: review?(gavin.sharp)
Attachment #8575061 - Flags: review?(gal)
Attachment #8575061 - Flags: review?(brendan)
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).
Attachment #8575061 - Flags: review?(roc)
Attachment #8575061 - Flags: review?(mark.finkle)
Attachment #8575061 - Flags: review?(gavin.sharp)
Attachment #8575061 - Flags: review?(gal)
Attachment #8575061 - Flags: review?(brendan)
Attachment #8575061 - Flags: feedback+
Requesting checkin of attachment 8575061 [details] [diff] [review]
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 :)
Attachment #8575061 - Flags: review?(gps) → review+
(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!
https://hg.mozilla.org/mozilla-central/rev/2706515176c8
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 1047752
You need to log in before you can comment on or make changes to this bug.