Closed
Bug 1134537
Opened 10 years ago
Closed 10 years ago
Eliminate GnomeVFS calls
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: alexhenrie24, Assigned: alexhenrie24)
References
Details
Attachments
(5 files, 3 obsolete files)
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
seth
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
roc
:
review+
gps
:
review+
Gavin
:
feedback+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
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)
Updated•10 years ago
|
Component: Build Config → XPCOM
Updated•10 years ago
|
Attachment #8566417 -
Flags: review?(roc)
Attachment #8566417 -
Flags: review?(nfroyd)
Attachment #8566417 -
Flags: review?(karlt)
Attachment #8566417 -
Flags: review?(birunthan)
![]() |
||
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.
![]() |
||
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.
Attachment #8566417 -
Flags: review?(nfroyd) → review+
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?
Attachment #8566417 -
Attachment is obsolete: true
Attachment #8566770 -
Flags: review?(nfroyd)
![]() |
||
Updated•10 years ago
|
Attachment #8566770 -
Flags: review?(nfroyd) → review+
![]() |
||
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. :)
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.
Keywords: checkin-needed
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/333249f6088d
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/333249f6088d
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee | ||
Updated•10 years ago
|
Summary: Eliminate nsIGnomeVFSService → Eliminate GnomeVFS calls
Assignee | ||
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 9•10 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=98644a3f7e63
Attachment #8567439 -
Flags: review?(birunthan)
Attachment #8567439 -
Flags: review?(benjamin)
Assignee | ||
Comment 10•10 years ago
|
||
Corrected Try link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3e6eee12321
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.
Attachment #8567439 -
Flags: review?(nfroyd)
Attachment #8567439 -
Flags: review?(birunthan)
Attachment #8567439 -
Flags: review?(benjamin)
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`.
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?
Flags: needinfo?(alexhenrie24)
![]() |
||
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.
Attachment #8567439 -
Flags: review?(nfroyd) → feedback+
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.
Attachment #8567439 -
Attachment is obsolete: true
Flags: needinfo?(alexhenrie24)
Attachment #8567549 -
Flags: review?(nfroyd)
![]() |
||
Updated•10 years ago
|
Attachment #8567549 -
Flags: review?(nfroyd) → review+
![]() |
||
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.
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 17•10 years ago
|
||
Requesting checkin of attachment 8567549 [details] [diff] [review]
Keywords: checkin-needed,
leave-open
Comment 18•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/826578768e85
Keywords: checkin-needed
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
Attachment #8572440 -
Flags: review?(seth)
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.
Attachment #8572440 -
Attachment is obsolete: true
Attachment #8572440 -
Flags: review?(seth)
Attachment #8572446 -
Flags: review?(seth)
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.
Attachment #8572446 -
Flags: review?(seth) → review+
Assignee | ||
Comment 23•10 years ago
|
||
Requesting checkin of attachment 8572446 [details] [diff] [review]
Keywords: checkin-needed
Comment 24•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f72ee1bf5567
Keywords: checkin-needed
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.
Attachment #8574254 -
Flags: review?(mcmanus)
Updated•10 years ago
|
Attachment #8574254 -
Flags: review?(mcmanus) → review+
Assignee | ||
Comment 28•10 years ago
|
||
Requesting checkin of attachment 8574254 [details] [diff] [review]
Keywords: checkin-needed
Comment 29•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc720b50b4e9
Keywords: checkin-needed
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.
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 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).
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+
Attachment #8575061 -
Flags: review?(roc) → review+
Assignee | ||
Comment 33•10 years ago
|
||
Requesting checkin of attachment 8575061 [details] [diff] [review]
Keywords: leave-open → checkin-needed
Comment 34•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2706515176c8
Keywords: checkin-needed
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 :)
Attachment #8575061 -
Flags: review?(gps) → review+
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!
Comment 37•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2706515176c8
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•