Stop shipping filepicker.css and filepicker.xul on Mac and Windows

RESOLVED FIXED in Firefox 52

Status

()

Firefox
File Handling
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: florian, Assigned: florian)

Tracking

unspecified
Firefox 52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
filepicker.css references inexistant files on Mac:
missing chrome://global/skin/filepicker/dir-closed.gif referenced from chrome://global/skin/filepicker.css
missing chrome://global/skin/filepicker/blank.gif referenced from chrome://global/skin/filepicker.css
missing chrome://global/skin/filepicker/folder-up.gif referenced from chrome://global/skin/filepicker.css
missing chrome://global/skin/filepicker/folder-home.gif referenced from chrome://global/skin/filepicker.css

but it turns out filepicker.css is only used by filepicker.xul which is only used by nsFilePicker.js, which is only packaged on Linux: http://searchfox.org/mozilla-central/rev/c31ad35f39c6187b2e121aa6d3a39b7f67397010/browser/installer/package-manifest.in#411

So, I think the best way to avoid the errors listed above is to stop shipping filepicker.css and filepicker.xul on Mac and Windows.
(Assignee)

Comment 1

a year ago
Created attachment 8794297 [details] [diff] [review]
Patch

Not sure if there are enough moz.build changes to require a build peer review. Also not sure if I need someone who knows the firefox-ui tests well to rs the test_l10n.py change (the filepicker.dtd file wasn't actually used there, so I just changed the file name to the name of a file in the same folder that really exists on all platforms). filepicker.properties is used on all platforms by widget/nsBaseFilePicker.cpp.
Attachment #8794297 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Updated

a year ago
Assignee: nobody → florian
Status: NEW → ASSIGNED

Comment 2

a year ago
Comment on attachment 8794297 [details] [diff] [review]
Patch

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

I'm happy to say I can review the moz.build changes here, as they're pretty trivial. This patch looks good, bar the nit below.

::: toolkit/locales/jar.mn
@@ +45,5 @@
>  #endif
>    locale/@AB_CD@/global/extensions.properties           (%chrome/global/extensions.properties)
>    locale/@AB_CD@/global/fallbackMenubar.properties      (%chrome/global/fallbackMenubar.properties)
>    locale/@AB_CD@/global/filefield.properties            (%chrome/global/filefield.properties)
> +#ifdef MOZ_GTK

The abstraction into the moz.build is fine, but maybe then call it MOZ_INCLUDE_FILEPICKER, and in the moz.build use the exact same if as in the toolkit/components moz.build file. I would really like us to not end up with silly edgecases where we accidentally include the locale and not the filepicker or vice versa.

Also, I believe we now use the native gtk filepicker on Linux... maybe check with :karlt if we can just nix all this code?
Attachment #8794297 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Comment 3

a year ago
(In reply to :Gijs Kruitbosch from comment #2)

> Also, I believe we now use the native gtk filepicker on Linux... maybe check
> with :karlt if we can just nix all this code?
Flags: needinfo?(karlt)
(Assignee)

Comment 4

a year ago
(In reply to :Gijs Kruitbosch from comment #2)

> > +#ifdef MOZ_GTK
> 
> The abstraction into the moz.build is fine, but maybe then call it
> MOZ_INCLUDE_FILEPICKER, and in the moz.build use the exact same if as in the
> toolkit/components moz.build file. I would really like us to not end up with
> silly edgecases where we accidentally include the locale and not the
> filepicker or vice versa.

I was more or less wondering the same thing, because:

- nsFilePicker.js is packaged ifdef MOZ_GTK at http://searchfox.org/mozilla-central/rev/c31ad35f39c6187b2e121aa6d3a39b7f67397010/browser/installer/package-manifest.in#411
where MOZ_GTK is defined ifneq (,$(filter gtk%,$(MOZ_WIDGET_TOOLKIT))) at http://searchfox.org/mozilla-central/source/browser/installer/Makefile.in#33

- it is built 
if CONFIG['MOZ_XUL'] and \
    CONFIG['MOZ_WIDGET_TOOLKIT'] not in ('android', 'cocoa', 'windows'):
at http://searchfox.org/mozilla-central/source/toolkit/components/filepicker/moz.build#7

I'm not sure these conditions match in 100% of the cases. But given that the xul file picker is only opened by nsFilePicker.js, I decided to make the condition for packaging the dtd file match the condition for packaging nsFilePicker.js.


So do you want me to change the condition in toolkit/components/moz.build from
  if CONFIG['MOZ_WIDGET_TOOLKIT'] not in ('android', 'cocoa', 'windows'):
to
  if 'gtk' in CONFIG['MOZ_WIDGET_TOOLKIT']:
so that it's the same as in toolkit/locales/moz.build?

Comment 5

a year ago
(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> So do you want me to change the condition in toolkit/components/moz.build
> from
>   if CONFIG['MOZ_WIDGET_TOOLKIT'] not in ('android', 'cocoa', 'windows'):
> to
>   if 'gtk' in CONFIG['MOZ_WIDGET_TOOLKIT']:
> so that it's the same as in toolkit/locales/moz.build?

Right, given the additional context this is probably fine. Thanks for elaborating.
(Assignee)

Comment 7

a year ago
https://hg.mozilla.org/integration/fx-team/rev/4b5a9f8b52d493e4b867b666433fe2aae254c100
Bug 1305075 - Stop shipping filepicker.css and filepicker.xul on Mac and Windows, r=Gijs.

Comment 8

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4b5a9f8b52d4
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 52
(In reply to :Gijs Kruitbosch from comment #2)
> Also, I believe we now use the native gtk filepicker on Linux... maybe check
> with :karlt if we can just nix all this code?

The GTK file chooser is the default on Linux, but it is not nice to use, so the decision is not clear cut.  Bug 1284391 has been filed.
Flags: needinfo?(karlt)
You need to log in before you can comment on or make changes to this bug.