769718 - metrofx should load it's searchplugins from the runtime folder

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Jim Mathies [:jimm]]

Currently we store a copy of these in the app sub dir. To simplify localization we'll need to load these from dist/bin rather than dist/bin/metro.

Comment 1

[Jim Mathies [:jimm]]

So I'm not seeing a way to "reuse" firefox's search plugins. Once bug 755724 lands, desktop resources including it's locale dir with be in dist/bin/browser, and metro resources will be in dist/bin/metro. For the plugin service we set a pref to locate these files:

pref("browser.search.jarURIs", "chrome://browser/locale/searchplugins/");

I don't see a way for metro to reference desktop's locale dir. So potentially we'll need to duplicate these files?

cc'ing benjamin since I'm sure he is familiar with this set up.

Comment 2

[Axel Hecht [:Pike]]

Duplicating as part of the build process should be fine, but we won't be able to maintain duplicate sources.

Comment 3

[Jim Mathies [:jimm]]

(In reply to Axel Hecht [:Pike] from comment #2)
> Duplicating as part of the build process should be fine, but we won't be
> able to maintain duplicate sources.

So can you fill me in a bit on where these files come from? We have what looks like a default searchplugins folder in dist/bin. These appear to come from browser/locales/en-US/searchplugins. I assume they would normally get pulled from some other source when we build localized builds. I can certainly pull the files from browser/locales/en-US/searchplugins into dist/bin/metro/searchplugins, but I sense that is not a complete solution.

Comment 4

[Axel Hecht [:Pike]]

browser uses magic between vpath and list.txt, http://mxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.in#16 and http://mxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.in#129.

Mirroring that to a different dist destination should probably be fine.

Comment 5

[Jim Mathies [:jimm]]

Attachment: removed files

Comment 6

[Jim Mathies [:jimm]]

Attachment: config v.1

Comment 7

[Jim Mathies [:jimm]]

Attachment: config v.1

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 694037 [details] [diff] [review]
removed files

Removing the old search files we no longer need.

Comment 9

[Jim Mathies [:jimm]]

Attachment: config v.2

Pull search plugins from /browser. Also updates search service related prefs so that they better mimic what the desktop product does.

Comment 10

[Axel Hecht [:Pike]]

Comment on attachment 694052 [details] [diff] [review]
config v.2

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

We should get rid of region.properties, too.

I've add the ability to override relativesrcdir in jar.mn, we might be able to use that to pick up file from browser/ in browser/metro, without having to put them into the source twice.

Comment 11

[Jim Mathies [:jimm]]

Attachment: config v.3

A slightly modified version of the search plugins make logic. I had to pull in a local copy of MERGE_FILE and adapt it to our situation here - the last test relied on srcdir, which doesn't work in the case where we are using search plugins located at a different root dir.

http://mxr.mozilla.org/mozilla-central/source/config/config.mk#721

Comment 12

[Jim Mathies [:jimm]]

(In reply to Axel Hecht [:Pike] from comment #10)
> Comment on attachment 694052 [details] [diff] [review]
> config v.2
> 
> Review of attachment 694052 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We should get rid of region.properties, too.
> 
> I've add the ability to override relativesrcdir in jar.mn, we might be able
> to use that to pick up file from browser/ in browser/metro, without having
> to put them into the source twice.

Filed bug 823503 on this.

Comment 13

[Jim Mathies [:jimm]]

Attachment: config v.3

- cleaned up the white space issue in SEARCH_PLUGIN processing.

Comment 14

[Axel Hecht [:Pike]]

I'm wondering, can we call into the browser/locales makefile to just put the searchplugins where you need them? Factor out the rule there?

Comment 15

[Jim Mathies [:jimm]]

(In reply to Axel Hecht [:Pike] from comment #14)
> I'm wondering, can we call into the browser/locales makefile to just put the
> searchplugins where you need them? Factor out the rule there?

Wouldn't we make a total mess of browser/locales/Makefile.in to filter out all the processing we wouldn't want?

http://mxr.mozilla.org/mozilla-central/source/browser/locales/Makefile.in

Comment 16

[Axel Hecht [:Pike]]

I don't think so, relatively speaking. I was thinking of a target like
searchplugins-%::
and make libs depend on that

Comment 17

[Axel Hecht [:Pike]]

... not strongly opinioned on whether this should be a pattern rule or not, too.

Comment 18

[Jim Mathies [:jimm]]

Attachment: config v.4

Sweet, thanks for the suggestion. That worked.

Comment 19

[Axel Hecht [:Pike]]

Comment on attachment 694863 [details] [diff] [review]
config v.4

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

This looks good to me, but I'd prefer glandium to do the real review here so that we're not breaking stuff.

Did you test official branding? Some paths in dist like to have ' ' in them, which is funky when passing around. Not sure if this really affects this, just calling it out.

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 694863 [details] [diff] [review]
config v.4

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

::: browser/locales/Makefile.in
@@ +136,5 @@
>  	for SEARCH_PLUGIN in $^; do\
>  	  SEARCH_PLUGIN_BASE=`basename $$SEARCH_PLUGIN`;\
>  	  $(PYTHON) $(topsrcdir)/config/Preprocessor.py $(DEFINES) $(ACDEFINES) \
>  	    $$SEARCH_PLUGIN > $(FINAL_TARGET)/searchplugins/$$SEARCH_PLUGIN_BASE; \
>  	done

Please change this to use the generic preprocessing rules.
See http://hg.mozilla.org/mozilla-central/file/f5ed2691d901/config/rules.mk#l1579

Then define a sp-libs rules (searchplugins would probably be more explicit) with something like this:
searchplugins: $(addprefix $(FINAL_TARGET)/searchplugins/,$(addsuffix .xml,$(SEARCH_PLUGINS))
.PHONY: searchplugins

::: browser/metro/locales/Makefile.in
@@ +13,5 @@
>  include $(DEPTH)/config/autoconf.mk
>  include $(topsrcdir)/config/config.mk
>  
> +# process desktop related resources we reuse here:
> +DIRS = import

You don't really need a separate Makefile for this.

@@ +25,2 @@
>  PREF_JS_EXPORTS = $(firstword $(wildcard $(LOCALE_SRCDIR)/metro-l10n.js) \
>                      @srcdir@/en-US/metro-l10n.js )

While you're here, replace @srcdir@ with $(srcdir)

::: browser/metro/locales/en-US/chrome/region.properties
@@ +7,5 @@
>  
>  # Search engine order (order displayed in the search bar dropdown)s
>  browser.search.order.1=Google
> +browser.search.order.2=Yahoo
> +browser.search.order.3=Bing

Unrelated changes should probably go in a separate patch or even a separate bug.

Comment 21

[Jim Mathies [:jimm]]

(In reply to Mike Hommey [:glandium] from comment #20)
> Please change this to use the generic preprocessing rules.
> See
> http://hg.mozilla.org/mozilla-central/file/f5ed2691d901/config/rules.mk#l1579
> 
> Then define a sp-libs rules (searchplugins would probably be more explicit)
> with something like this:
> searchplugins: $(addprefix $(FINAL_TARGET)/searchplugins/,$(addsuffix
> .xml,$(SEARCH_PLUGINS))
> .PHONY: searchplugins

updated.

> ::: browser/metro/locales/Makefile.in
> @@ +13,5 @@
> >  include $(DEPTH)/config/autoconf.mk
> >  include $(topsrcdir)/config/config.mk
> >  
> > +# process desktop related resources we reuse here:
> > +DIRS = import
> 
> You don't really need a separate Makefile for this.

It's not needed but I grouped desktop import logic together. I use the separate file so that I can set a custom relativesrcdir for bookmark processing, which has to jump through some hoops to get the desktop bookmarks.inc for preprocessing the json file and then packaged using the metro jar file. (bug 769724)

> 
> @@ +25,2 @@
> >  PREF_JS_EXPORTS = $(firstword $(wildcard $(LOCALE_SRCDIR)/metro-l10n.js) \
> >                      @srcdir@/en-US/metro-l10n.js )
> 
> While you're here, replace @srcdir@ with $(srcdir)

updated.

> ::: browser/metro/locales/en-US/chrome/region.properties
> @@ +7,5 @@
> >  
> >  # Search engine order (order displayed in the search bar dropdown)s
> >  browser.search.order.1=Google
> > +browser.search.order.2=Yahoo
> > +browser.search.order.3=Bing
> 
> Unrelated changes should probably go in a separate patch or even a separate
> bug.

Breaking it out is pretty pointless since this is going to land on elm.

Comment 22

[Jim Mathies [:jimm]]

Attachment: config v.5

Comment 23

[Jim Mathies [:jimm]]

Attachment: mc searchplugin patch v.1

Breaking this part out so I can land it on mc separately.

Comment 24

[Jim Mathies [:jimm]]

Attachment: elm bits

Comment 25

[Jim Mathies [:jimm]]

https://hg.mozilla.org/projects/elm/rev/ac9aa8437aa3
https://hg.mozilla.org/projects/elm/rev/6693ca8c4307
https://hg.mozilla.org/projects/elm/rev/79d904b6ea00

Comment 26

[Matt Brubeck (:mbrubeck)]

Resolving bugs in the Firefox for Metro product that are fixed on the elm branch.  Sorry for the bugspam.  Search your email for "bugspam-elm" if you want to find and delete all of these messages at once.

Comment 27

[Jim Mathies [:jimm]]

https://hg.mozilla.org/mozilla-central/rev/59baa6cf573c