Closed Bug 1319762 Opened 8 years ago Closed 7 years ago

Remove chrome://mozapps/content/downloads/downloads.xul and related unused files

Categories

(Firefox :: Downloads Panel, defect, P3)

Product:
Firefox
Component:
Downloads Panel
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: florian, Assigned: sajarvis)

References

Details

Attachments

(1 file)

Bug 1319762 - Remove download.xul and related unused files.
58 bytes, text/x-review-board-request
florian
: review+
Details 
chrome://mozapps/content/downloads/downloads.xul is only referenced in nsDownloadManagerUI.js which we no longer ship since bug 1114614 (https://hg.mozilla.org/mozilla-central/rev/a4335e10398f#l6.1)

So in addition to downloads.xul, we can likely remove from the tree nsDownloadManagerUI.js, nsDownloadManagerUI.js, nsDownloadManagerUI.manifest

browser/base/content/downloadManagerOverlay.xul is used only as an overlay on downloads.xul, so it should be removed too.

We should also review all the chrome files referenced by downloads.xul, as some may become unreferenced after we remove downloads.xul
We should check Seamonkey is not using these, https://bugzilla.mozilla.org/show_bug.cgi?id=888915#c32 seems to point out nsDownloadManagerUI.js should be safe to remove, and afaict indeed they seem to be using nsISuiteDownloadManagerUI and nsSuiteDownloadManagerUI.

downloadManagerOverlay.xul sounds like an easy removal.
Priority: -- → P3
SeaMonkey references downloads.xul at http://searchfox.org/comm-central/source/suite/common/src/nsSuiteDownloadManagerUI.js#14 but it's only used if the browser.download.manager.useToolkitUI pref (which isn't set anywhere) is true, so I think it's safe to remove (and if SeaMonkey really wants to keep this legacy piece of code, they can still move it to suite/ in the future).
Attachment #8817337 - Flags: review?(florian)
Just by grep'ing through the source, there seem to be references to the other chrome:// files in downloads.xul elsewhere.

I ran this through the try server and there were issues that seem to be related to known test bugs, but between retrying and testing locally all looks green.

Run 2: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0dd2bd149510d3c6a80bbce05ba93a81d7f762d
Run 3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a75c2819ff3f353004706a8dbd9d0d0d5e918cd6 

I have a more general hg question as well, when pushing this review I got the error:

    abort: cannot submit reviews referencing multiple bugs
    (limit reviewed changesets with "-c" or "-r" arguments)

I'm assuming the commits I made previously are still being picked up as additions, even though they've been merged into central. Coming from a Git background, I think what I'm missing with Mercurial is that "pull" does not include a "merge", it needs to be done explicitly. Is that true?
(In reply to Steve Jarvis [:sajarvis] from comment #4)
> Just by grep'ing through the source, there seem to be references to the
> other chrome:// files in downloads.xul elsewhere.

Let's take downloads.js as an example:
http://searchfox.org/mozilla-central/search?q=downloads.js
There are several references to files named downloads.js, but there's only one for chrome://mozapps/content/downloads/downloads.js (chrome://browser/content/downloads/downloads.js is a different file), and it's in the downloads.xul file we are removing.

downloads.js is just an example, there are other files in the same situation.

> I have a more general hg question as well, when pushing this review I got
> the error:
> 
>     abort: cannot submit reviews referencing multiple bugs
>     (limit reviewed changesets with "-c" or "-r" arguments)
> 
> I'm assuming the commits I made previously are still being picked up as
> additions, even though they've been merged into central. Coming from a Git
> background, I think what I'm missing with Mercurial is that "pull" does not
> include a "merge", it needs to be done explicitly. Is that true?

I'm not familiar with this error message, but in general after 'hg pull' you'll want to run 'hg update' or 'hg rebase'. 'hg merge' would create a merge changeset, which isn't what you want in most cases.
Assignee: nobody → sajarvis
(In reply to Florian Quèze [:florian] [:flo] from comment #5)
> Let's take downloads.js as an example:
> http://searchfox.org/mozilla-central/search?q=downloads.js
> There are several references to files named downloads.js, but there's only
> one for chrome://mozapps/content/downloads/downloads.js
> (chrome://browser/content/downloads/downloads.js is a different file), and
> it's in the downloads.xul file we are removing.
> 
> downloads.js is just an example, there are other files in the same situation.

Thanks, I learned more about the chrome:// system and removed more files here. One that still has me confused is chrome://mozapps/locale/downloads/downloads.dtd. It has no references outside of files removed here, but during packaging the l10n locale scripts seem to pull it in implicitly and builds fail without it. 

One such build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1c4403e821e517d3b6b97b4aaecd0dad96ed961&selectedJob=32492569

So whether it ought to be in the patch or not, could anyone explain what's up with downloads.dtd? The developer wiki on locale implies it should be pretty straight forward, but it's not adding up for me. Thanks.
(In reply to Steve Jarvis [:sajarvis] from comment #7)
> (In reply to Florian Quèze [:florian] [:flo] from comment #5)
> > Let's take downloads.js as an example:
> > http://searchfox.org/mozilla-central/search?q=downloads.js
> > There are several references to files named downloads.js, but there's only
> > one for chrome://mozapps/content/downloads/downloads.js
> > (chrome://browser/content/downloads/downloads.js is a different file), and
> > it's in the downloads.xul file we are removing.
> > 
> > downloads.js is just an example, there are other files in the same situation.
> 
> Thanks, I learned more about the chrome:// system and removed more files
> here.

Thanks for updating the patch!

> One that still has me confused is
> chrome://mozapps/locale/downloads/downloads.dtd. It has no references
> outside of files removed here, but during packaging the l10n locale scripts
> seem to pull it in implicitly and builds fail without it.

Have you removed the reference at
http://searchfox.org/mozilla-central/rev/5ee2bd8800b007d6c120d9521d5bf01131885afb/toolkit/locales/jar.mn#104
?
I think the nsIDownloadProgressListener interface is also becoming unused, but given it may be used by add-ons, I'm not comfortable removing it now.
(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> (In reply to Steve Jarvis [:sajarvis] from comment #7)
> > (In reply to Florian Quèze [:florian] [:flo] from comment #5)
> > One that still has me confused is
> > chrome://mozapps/locale/downloads/downloads.dtd. It has no references
> > outside of files removed here, but during packaging the l10n locale scripts
> > seem to pull it in implicitly and builds fail without it.
> 
> Have you removed the reference at
> http://searchfox.org/mozilla-central/rev/
> 5ee2bd8800b007d6c120d9521d5bf01131885afb/toolkit/locales/jar.mn#104
> ?

I did actually, for the linked try run the patch removed the reference, but still packaging failure. 
https://hg.mozilla.org/try/diff/f1c4403e821e/toolkit/locales/jar.mn

I'll remove DownloadTaskbarProgress.jsm, good catch, thanks. Let me know if you have any other thoughts on the dtd file and I'll update the review.
(In reply to Steve Jarvis [:sajarvis] from comment #11)
> (In reply to Florian Quèze [:florian] [:flo] from comment #8)
> > (In reply to Steve Jarvis [:sajarvis] from comment #7)
> > > (In reply to Florian Quèze [:florian] [:flo] from comment #5)
> > > One that still has me confused is
> > > chrome://mozapps/locale/downloads/downloads.dtd. It has no references
> > > outside of files removed here, but during packaging the l10n locale scripts
> > > seem to pull it in implicitly and builds fail without it.
> > 
> > Have you removed the reference at
> > http://searchfox.org/mozilla-central/rev/
> > 5ee2bd8800b007d6c120d9521d5bf01131885afb/toolkit/locales/jar.mn#104
> > ?
> 
> I did actually, for the linked try run the patch removed the reference, but
> still packaging failure. 
> https://hg.mozilla.org/try/diff/f1c4403e821e/toolkit/locales/jar.mn
> 
> [...] Let me know if
> you have any other thoughts on the dtd file and I'll update the review.

I don't see a problem in the patch you pushed to try, so I think it's a problem with the L10n job.

Flod, is there anything to do in the patch here to prevent the failure of the 'L10n' job on https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1c4403e821e517d3b6b97b4aaecd0dad96ed961 ?
Flags: needinfo?(francesco.lodolo)
I don't see anything that touches l10n to explain the failure, but I also have very little knowledge of the build system. Trying to redirect NI to Pike, hoping he knows more.
Flags: needinfo?(francesco.lodolo) → needinfo?(l10n)
Side note: if we're removing download.xul, please make sure to remove also the associated strings if they're unused.
Thanks for testing l10n on try, first.

The problem is that try is repacking a nightly right now, and not the build you did.

You can hack yourself out of that, but I haven't done so myself. You'd first push to try the patch you're testing, and then in a second step, edit the mozconfig to set en_us_binary_url, as hinted at by https://wiki.mozilla.org/ReleaseEngineering/TryServer#Desktop_l10n_jobs.

You also want to reduce the locales in all-locales, as otherwise you'll see timeout failures on try. fr, it, ru are a good list.

We talked about this a bit on Hawaii, and also that we don't have a full buglist. Or do we, Callek?
Flags: needinfo?(l10n)
(In reply to Steve Jarvis [:sajarvis] from comment #11)

> I'll remove DownloadTaskbarProgress.jsm, good catch, thanks. Let me know if
> you have any other thoughts on the dtd file and I'll update the review.

Given comment 13 to 15, I think you can just remove the dtd file and ignore the error shown on try (which is repacking using a nightly instead of the build from your push).

Have you checked if there are strings in http://searchfox.org/mozilla-central/source/toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties that are no longer be used? (this file is used from a couple other places, as shown by http://searchfox.org/mozilla-central/search?q=mozapps%2Flocale%2Fdownloads%2Fdownloads.properties )
(In reply to Florian Quèze [:florian] [:flo] from comment #16)
> Have you checked if there are strings in
> http://searchfox.org/mozilla-central/source/toolkit/locales/en-US/chrome/
> mozapps/downloads/downloads.properties that are no longer be used? (this
> file is used from a couple other places, as shown by
> http://searchfox.org/mozilla-central/
> search?q=mozapps%2Flocale%2Fdownloads%2Fdownloads.properties )

Well I did now! Latest version of the patch removes a handful of strings from downloads.properties that seem to be unreferenced.
How did you check if they were unreferenced? Because there seem to be references to at least some of the strings you removed (haven't checked the others)
https://dxr.mozilla.org/mozilla-central/rev/8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/toolkit/mozapps/downloads/content/downloads.js#75
(In reply to Francesco Lodolo [:flod] (mostly out of office until Dec 19) from comment #19)
> How did you check if they were unreferenced? Because there seem to be
> references to at least some of the strings you removed (haven't checked the
> others)
> https://dxr.mozilla.org/mozilla-central/rev/
> 8103c612b79c2587ea4ca1b0a9f9f82db4b185b8/toolkit/mozapps/downloads/content/
> downloads.js#75

toolkit/mozapps/downloads/content/downloads.js is being removed by the patch.
Comment on attachment 8817337 [details]
Bug 1319762 - Remove download.xul and related unused files.

https://reviewboard.mozilla.org/r/97660/#review99076

Looks good to me. I would like someone more familiar with the download manager code to have a quick look to ensure we aren't removing things that are about to become used again.

::: toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties:42
(Diff revision 3)
>  dontQuitButtonWin=Don’t Exit
>  dontQuitButtonMac=Don’t Quit
>  dontGoOfflineButton=Stay Online
>  dontLeavePrivateBrowsingButton2=Stay in Private Browsing
>  downloadsCompleteTitle=Downloads Complete
> -downloadsCompleteMsg=All files have finished downloading. 
> +downloadsCompleteMsg=All files have finished downloading.

flod, can you confirm that removing the trailing space here has no (undesirable) effect?

::: toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties
(Diff revision 3)
> -
> -# LOCALIZATION NOTE (downloadsTitleFiles, downloadsTitlePercent): Semi-colon list of
> -# plural forms. See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> -# #1 number of files; #2 overall download percent (only for downloadsTitlePercent)
> -# examples: 2% of 1 file - Downloads; 22% of 11 files - Downloads
> -downloadsTitleFiles=#1 file - Downloads;#1 files - Downloads

Interestingly, there are 2 localization notes with this string id in the unrelated aboutSupport.properties file. I opened bug 1323711 for this.

::: toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties:106
(Diff revision 3)
> -# #1 number of files; #2 overall download percent (only for downloadsTitlePercent)
> -# examples: 2% of 1 file - Downloads; 22% of 11 files - Downloads
> -downloadsTitleFiles=#1 file - Downloads;#1 files - Downloads
> -downloadsTitlePercent=#2% of #1 file - Downloads;#2% of #1 files - Downloads
> -
>  fileExecutableSecurityWarning=“%S” is an executable file. Executable files may contain viruses or other malicious code that could harm your computer. Use caution when opening this file. Are you sure you want to launch “%S”?

These 3 fileExecutableSecurityWarning strings seem to be used here in toolkit, but unused in browser/locales/en-US/chrome/browser/downloads/downloads.properties
That can be cleaned up in a separate bug.
Attachment #8817337 - Flags: review?(florian) → review+
(In reply to Florian Quèze [:florian] [:flo] from comment #21)

> ::: toolkit/locales/en-US/chrome/mozapps/downloads/downloads.properties:42
> (Diff revision 3)
> >  dontQuitButtonWin=Don’t Exit
> >  dontQuitButtonMac=Don’t Quit
> >  dontGoOfflineButton=Stay Online
> >  dontLeavePrivateBrowsingButton2=Stay in Private Browsing
> >  downloadsCompleteTitle=Downloads Complete
> > -downloadsCompleteMsg=All files have finished downloading. 
> > +downloadsCompleteMsg=All files have finished downloading.
> 
> flod, can you confirm that removing the trailing space here has no
> (undesirable) effect?
Flags: needinfo?(francesco.lodolo)
(In reply to Florian Quèze [:florian] [:flo] from comment #21)
> Comment on attachment 8817337 [details]
> Bug 1319762 - Remove download.xul and related unused files.
> 
> https://reviewboard.mozilla.org/r/97660/#review99076
> 
> Looks good to me. I would like someone more familiar with the download
> manager code to have a quick look to ensure we aren't removing things that
> are about to become used again.

Marco, can you just have a quick look at this patch before we land it? (or forward the needinfo to someone else who's familiar with our plans for the download manager panel)
Flags: needinfo?(mak77)
We trim unescaped whitespace at the start and end of strings in .properties, https://dxr.mozilla.org/mozilla-central/source/xpcom/ds/nsPersistentProperties.cpp#102.

There's a bit of noise changing strings creates in part of our toolchain, I think, in the gettext "fuzzy" sense. In that sense, I'd prefer to keep the string as is.
Flags: needinfo?(francesco.lodolo)
it won't be a quick review on my side since my queue is not very short and these days are a bit complicate. I'll add a needinfo on Paolo for a first come first serve strategy.
Flags: needinfo?(mak77) → needinfo?(paolo.mozmail)
Flags: needinfo?(mak77)
(In reply to Axel Hecht [:Pike] from comment #24)
> We trim unescaped whitespace at the start and end of strings in .properties,
> https://dxr.mozilla.org/mozilla-central/source/xpcom/ds/
> nsPersistentProperties.cpp#102.
> 
> There's a bit of noise changing strings creates in part of our toolchain, I
> think, in the gettext "fuzzy" sense. In that sense, I'd prefer to keep the
> string as is.

Thanks Pike, I put the trailing space back in the patch, so that string is unchanged.
This seems all to be code that we planned to remove eventually. We don't need it for future reuse.

Thank you very much for helping with this removal of unused code!
Blocks: 851471
No longer blocks: 1114614
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(mak77)
Pushed by florian@queze.net:
https://hg.mozilla.org/integration/autoland/rev/96ba3fd9872e
Remove download.xul and related unused files. r=florian
https://hg.mozilla.org/mozilla-central/rev/96ba3fd9872e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Blocks: 1324119
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: