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

Status: RESOLVED FIXED

(Firefox :: File Handling, defect)

Description

[Florian Quèze [:florian]]

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.

Comment 1

[Florian Quèze [:florian]]

Attachment: 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.

Comment 2

[:Gijs (he/him)]

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?

Comment 3

[Florian Quèze [:florian]]

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

Comment 4

[Florian Quèze [:florian]]

(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

[:Gijs (he/him)]

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

Comment 6

[Florian Quèze [:florian]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76f19d7258f3

Comment 7

[Florian Quèze [:florian]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4b5a9f8b52d4

Comment 9

[Karl Tomlinson (:karlt)]

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