Closed Bug 997772 Opened 11 years ago Closed 11 years ago

Bootstrap suggested sites framework

Categories

(Firefox for Android Graveyard :: Awesomescreen, defect)

Product:
Firefox for Android Graveyard
Component:
Awesomescreen
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
Firefox 32

People

(Reporter: lucasr, Assigned: lucasr)

References

Details

Attachments

(4 files, 10 obsolete files)

Introduce SuggestedSites component (r=mfinkle)
14.20 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
Add default suggested sites for en-US (r=nalexander)
111.52 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
Generate suggestedsites.json from locale (r=nalexander)
12.49 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
TO BE FOLDED : Generate suggestedsites.json from locale
13.37 KB, patch
Details | Diff | Splinter Review
Land the suggested sites framework and the necessary changes to the build system.
Blocks: 997782
Comment on attachment 8408334 [details] [diff] [review] Generate suggestedsites.json from locale (r=nalexander) These are the necessary changes to our build system to generate the default suggested sites at build time in a locale-friendly way. This system is heavily inspired by how we generate the list of search engines per locale. For each locale's mobile/android/base, you'll have something like this: suggestedsites suggestedsites/hdpi suggestedsites/hdpi/mozilla.png suggestedsites/hdpi/wikipedia.png suggestedsites/mdpi suggestedsites/mdpi/mozilla.png suggestedsites/mdpi/wikipedia.png suggestedsites/xhdpi suggestedsites/xhdpi/mozilla.png suggestedsites/xhdpi/wikipedia.png suggestedsites/mozilla.json suggestedsites/list.txt suggestedsites/wikipedia.json Where list.txt is a list of site names (one per line) matching the json file names. So, for a list.txt like this... wikipedia mozilla ...we'd have matching wikipedia.json and mozilla.json files. The JSON files look like this: { "title": "Mozilla", "url": "http://mozilla.org", "bgColor": "#ffcc00" } So, the idea is that locales can override any of the files from en-US at build time (the list.txt itself, any specific JSON file, or any of the images). Besides that, locales can also add new sites by adding a reference to it in their list.txt and add their own JSON file for the new site. At build time, we resolve all file names accounting for the target locale, and call generate_suggestedsites_json.py with list of resolved JSON files as input. The imageUrl for each website in automatically generated by generate_suggestedsites_json.py based on a given image URL template. The main reason for doing that is to avoid forcing translator to deal with image URL conventions (which is very error-prone). By generating the image URL dynamically, they only need to care about providing the JSON file and images for each density matching the same site name. The implicit convention here is: SITENAME.json mdpi/SITENAME.png hdpi/SITENAME.png xhdpi/SITENAME.png The generated files suggestedsites.json and the images for all sites are installed as Android resources (raw and drawables, respectively). The main reason to use Android's resource system here is that we get image density handling for free, besides all the multi-locale support that comes with it. We're already doing some similar with strings.xml. This is working well for single locale builds (en-US anyway) but I'm having problems with multi-locale builds (still investigating). My autotools skills are a bit rusty so I'm pretty sure there ways to make the Makefile.in changes a bit more efficient/succinct. Nick, I'm counting on your input to improve things here :-)
Attachment #8408334 - Flags: review?(nalexander)
Attachment #8408334 - Flags: feedback?(mark.finkle)
Forgot to mention: ignore the specific list of sites in this patch. We're going to come up with the initial list in bug 997765.
Comment on attachment 8408335 [details] [diff] [review] Introduce SuggestedSites component (r=mfinkle) Ok, once we get the build stuff in place (i.e. previous patch), we'll have a raw resource R.raw.suggestedsites and all matching drawables shipping with Fennec. This patch introduces the component that reads the list of sites and returns a Cursor to be 'blended' with the other queries in the top sites panel. The R.raw.suggestedsites resources is only used as the default list. For now, it's the only list. In the long-term though, we'll try to read a suggestedsites.json file from a standard location first. This file could come from a distribution file or downloaded from our servers.
Attachment #8408335 - Flags: review?(mark.finkle)
Comment on attachment 8408334 [details] [diff] [review] Generate suggestedsites.json from locale (r=nalexander) Looks generally good to me, but I can live with build stuff that "just gets it done." Let's see if Nick has some feedback. I don't want to re-write the build system to gets this landed though. I think we can iterate on the rough points as we go. Maybe an OCD nit: > +++ b/mobile/android/base/locales/en-US/suggestedsites/mozilla.json > +{ "title": "Mozilla", > + "url": "http://mozilla.org", > + "bgColor": "#ffcc00" } bgColor -> bgcolor ? I worry that mixed case could be a common pain point for l10n.
Attachment #8408334 - Flags: feedback?(mark.finkle) → feedback+
Comment on attachment 8408334 [details] [diff] [review] Generate suggestedsites.json from locale (r=nalexander) On other point that Richard brought up: If possible we should try to find a way to reduce duplicate images for different locales. Ideally, there would be only one locale and others would be downloaded, but that won't happen right away. If reducing the dupes is not easy or adds too much complexity, then maybe we wait until locales are downloadable to reduce APK size. Another thought: We should make sure we use the pngquant compressor on the images to squeeze them as small as possible.
Comment on attachment 8408335 [details] [diff] [review] Introduce SuggestedSites component (r=mfinkle) >diff --git a/mobile/android/base/db/SuggestedSites.java b/mobile/android/base/db/SuggestedSites.java >+ private List<Site> refresh() { >+ // Update cached list of sites >+ cachedSites = new SoftReference<List<Site>>(Collections.unmodifiableList(sites)); Android docs [1] seem to be pushing us to use LruCache, but I'm not sure LruCache would work as you intend here. I think we'd still need to manually evict, or set a small limit on the cache. I'm OK with using SoftReference. [1] http://developer.android.com/reference/java/lang/ref/SoftReference.html >\ No newline at end of file Add one
Attachment #8408335 - Flags: review?(mark.finkle) → review+
(In reply to Mark Finkle (:mfinkle) from comment #7) > Comment on attachment 8408334 [details] [diff] [review] > Generate suggestedsites.json from locale (r=nalexander) > > On other point that Richard brought up: If possible we should try to find a > way to reduce duplicate images for different locales. > > Ideally, there would be only one locale and others would be downloaded, but > that won't happen right away. If reducing the dupes is not easy or adds too > much complexity, then maybe we wait until locales are downloadable to reduce > APK size. The build system changes were done in such a way that locales only need to override the files that actually need to be localized. For instance, you can override the site's JSON file but re-use the original images from en-US. Locales will only need to provide their own images if: 1) The site images themselves need to localized for some reason. 2) The locale adds a new suggested site to the list. So, we're already avoiding most duplicate images by design here. I expect locales to only override JSON files in most cases. > Another thought: We should make sure we use the pngquant compressor on the > images to squeeze them as small as possible. Good reminder. The final list of sites (and respective images) will only land in bug 997765 though. I added a comment there as a note to self.
Comment on attachment 8408334 [details] [diff] [review] Generate suggestedsites.json from locale (r=nalexander) Review of attachment 8408334 [details] [diff] [review]: ----------------------------------------------------------------- I've made a few small comments throughout, but let me suggest a different implementation approach here. You've wisely implemented the JSON munging in Python. Rather than implementing the copying and name shuffling in Make, let us implement that in Python as well. That should let us reduce the Make to a few lines that determine AB_CD, the input directory, and the output directory. That is, we expand generate_suggestedsites_json into a larger "build action" and use the existing |py_action| support. We have good Python tools for install manifests which will make the image installation pretty clean. Plus, we can test/verify the Python code easily \o/ This is probably the best thing to do for the dependencies (or lack of them). At the moment, we FORCE the sub-make anyway; this corresponds to always running the "build action", and having it handle its own dependencies. In some future world (not this ticket!) we could add a dependency-generating mode that writes a .pp file for Make to consume. I am curious to explore this and will spend a few hours this afternoon seeing what it feels like. ::: mobile/android/base/Makefile.in @@ +48,5 @@ > classes.dex \ > gecko.ap_ \ > res/values/strings.xml \ > + res/raw/suggestedsites.json \ > + res/drawable-*/suggestedsites_* \ nit: be consistent about what files the suggestedsites feature owns, namely 'suggestedsites*'. @@ +208,5 @@ > > res/values/strings.xml: FORCE > $(MAKE) -C locales > > +res/raw/suggested-sites.json: FORCE I'd really like to avoid running this sub-make twice. I'll think on this a little. ::: mobile/android/base/moz.build @@ +498,5 @@ > OBJDIR + '/res', > ] > > ANDROID_GENERATED_RESFILES += [ > + 'res/raw/suggestedsites.json', These deps don't include res/drawable-*/suggestedsites_*. Since these things are determined by the build system, and need to interact with l10n re-packs, these deps can't be added in a reasonable way. Eventually, we will run into bad incremental builds because of this, but I see no alternatives right now. ::: mobile/android/locales/Makefile.in @@ +33,5 @@ > # which this target is for > clobber-stage: > $(RM) -rf $(STAGEDIST) > $(RM) $(DEPTH)/mobile/android/base/res/values-*/strings.xml > + $(RM) $(DEPTH)/mobile/android/base/res/raw-*/suggestedsites.json nit: be consistent: suggestedsites* I'd rather make clear that this features owns the entire suggestedsites* namespace. ::: python/mozbuild/mozbuild/action/generate_suggestedsites_json.py @@ +46,5 @@ > + > + # Use the template to generate the final image URL > + # for the suggested site. > + name = os.path.splitext(os.path.basename(json_file))[0] > + site['imageUrl'] = image_url_template.replace('@SUGGESTEDSITE@', name, 1) nit: let's go lower case keys throughout. Same with mfinkle's bgcolor, etc. @@ +50,5 @@ > + site['imageUrl'] = image_url_template.replace('@SUGGESTEDSITE@', name, 1) > + > + json_array.append(site) > + > + write_output_file(output_file, json.dumps(json_array)) Use FileAvoidWrite for this: http://mxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/util.py#112. We don't want to dirty this file every build. @@ +51,5 @@ > + > + json_array.append(site) > + > + write_output_file(output_file, json.dumps(json_array)) > + return 0; nit: no trailing semi. @@ +54,5 @@ > + write_output_file(output_file, json.dumps(json_array)) > + return 0; > + > +def main(args): > + if len(args) < 3: nit: funky indenting. In general, we indent Python at 4 spaces throughout the tree. Adding emacs and vi modelines to the header is good.
Attachment #8408334 - Flags: review?(nalexander) → feedback+
Comment on attachment 8408334 [details] [diff] [review] Generate suggestedsites.json from locale (r=nalexander) Review of attachment 8408334 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/Makefile.in @@ +208,5 @@ > > res/values/strings.xml: FORCE > $(MAKE) -C locales > > +res/raw/suggested-sites.json: FORCE This should be suggestedsites.json. Make is both hard and unhelpful.
(In reply to Nick Alexander :nalexander from comment #10) > Comment on attachment 8408334 [details] [diff] [review] > Generate suggestedsites.json from locale (r=nalexander) > > Review of attachment 8408334 [details] [diff] [review]: > ----------------------------------------------------------------- > > I've made a few small comments throughout, but let me suggest a different > implementation approach here. > > You've wisely implemented the JSON munging in Python. Rather than > implementing the copying and name shuffling in Make, let us implement that > in Python as well. That should let us reduce the Make to a few lines that > determine AB_CD, the input directory, and the output directory. That is, we > expand generate_suggestedsites_json into a larger "build action" and use the > existing |py_action| support. We have good Python tools for install > manifests which will make the image installation pretty clean. Plus, we can > test/verify the Python code easily \o/ Digging into the mechanics, I see you are already using py_action. \o/
Status update: I got the Python part up today; will post patches with the Make part tomorrow.
(In reply to Nick Alexander :nalexander from comment #13) > Status update: I got the Python part up today; will post patches with the > Make part tomorrow. Nick, I'm assuming you're taking over the necessary build system changes? Just wondering whether I should update my original patch based on your initial feedback or not.
Flags: needinfo?(nalexander)
(In reply to Lucas Rocha (:lucasr) from comment #14) > (In reply to Nick Alexander :nalexander from comment #13) > > Status update: I got the Python part up today; will post patches with the > > Make part tomorrow. > > Nick, I'm assuming you're taking over the necessary build system changes? > Just wondering whether I should update my original patch based on your > initial feedback or not. I'll push the next iteration to you, and we'll move forward from there.
Flags: needinfo?(nalexander)
> I'll push the next iteration to you, and we'll move forward from there. Sorry for the delay, but I've run into a problem that I don't think we want to work around. tl;dr: non en-US locales cannot add resources not present in en-US. Let me explain why. Regular builds run aapt with en-US to generate R.java and gecko.ap_. R.java assigns IDs to resources; gecko.ap_ contains the binary resources. Regular builds compile all the Java code with the generated R.java. At this point, the assigned resource IDs are fixed. l10n re-packs re-run aapt with a res/ directory that contains everything that a regular build contains, but also contains additional values-*/strings.xml files corresponding to additional languages. This aapt invocation produces both a new R.java and gecko.ap_; the new R.java is *thrown away*, and the new gecko.ap_ replaces the gecko.ap_ from the original build. This replacement process works only because the set of resource IDs does not change from regular build to l10n repack. (This is completely unspecified aapt behaviour, but it seems to work!) For those interested/appalled, I've included guidance to where this build behaviour is described below. The relevant Make targets are .aapt.deps/.aapt.nodeps, and the details of this scheme are commented at: http://hg.mozilla.org/mozilla-central/file/3b166b8add93/mobile/android/base/Makefile.in#l197 http://hg.mozilla.org/mozilla-central/file/3b166b8add93/mobile/android/base/Makefile.in#l248 http://hg.mozilla.org/mozilla-central/file/3b166b8add93/mobile/android/base/Makefile.in#l287 and http://hg.mozilla.org/mozilla-central/annotate/09a19b25b9cf/toolkit/mozapps/installer/packager.mk#l442
(In reply to Nick Alexander :nalexander from comment #16) > > I'll push the next iteration to you, and we'll move forward from there. > > Sorry for the delay, but I've run into a problem that I don't think we want > to work around. tl;dr: non en-US locales cannot add resources not present > in en-US. Let me explain why. Now that I've described the issue, and how to understand it more thoroughly, let me discuss alternatives. I see essentially 3. 1. We fix the build system to do l10n re-packs that allow changing the resource set at re-package time. 2. We always include "new" suggestedsites resources in en-US, even if they're not consumed by en-US. 3. We don't use the Android resource system for density-specific drawables. There's a reason that we haven't yet done 1: it's a lot of work. It's not just that it's a lot of in-tree work -- that would be great -- but it's a lot of TBPL/automation work. The l10n re-pack logic is complicated and not in the tree. There are a bazillion hidden assumptions (some of which I discovered and documented above!). In theory, this is not hard -- just do more work as part of |make package| -- but in practice this will be laborious and hard to get right. There's also a reason we (lucasr) wanted Android to handle density-specific drawables, so option 3 is not awesome. One reason I think we should consider it, however, is that we're going to serve the suggestedsites metadata remotely at some point. At that time, we won't be able to use Android drawables at all, since said drawables are static. Perhaps we build that functionality up front and use it for the remotely served suggestedsites, and the pre-shipped sites. Finally, option 2 is the clearly expedient choice. It does mean that localizers can't customize suggestedsites without committing to m-c, but early discussions with lucasr suggested that restriction might be a feature, not a bug. Option 2 also obviates most of this build system work: it's probably not necessary to parse list.txt to create suggestedsites.json, etc; we can just preprocess suggestedsites.json.in and handle doing that for additional locales. As a final benefit, all concerns around duplicating drawables are mitigated, since all drawables are managed in locales/en-US.
> Now that I've described the issue, and how to understand it more thoroughly, > let me discuss alternatives. I see essentially 3. ... > 2. We always include "new" suggestedsites resources in en-US, even if > they're not consumed by en-US. And finally, so that people can easily reference my opinion, I think we want to do option 2. Option 2 unblocks lucasr in the short term, whilst not dramatically changing his original approach, and leaves us free to build remotely-served suggestedsites in the future, when we know more about what is wanted.
In channel, lucasr asked: 08:40 lucasr: [08:38:50] nalexander, this issue only applies to the drawables I assume > This replacement process works only because the set of resource IDs does not > change from regular build to l10n repack. (This is completely unspecified > aapt behaviour, but it seems to work!) For those interested/appalled, I've > included guidance to where this build behaviour is described below. Anything that changes the set of resource IDs will bust this delicate process :( So adding any resource ID (a single resource -- of any kind -- not present in en-US) will throw off the resource numbering. Accidentally *not including* a resource will do the same. So the problem is generic, but for suggestedsites, it only concerns the drawables (since raw[-*]/suggestedsites.json is present for all locales).
(In reply to Nick Alexander :nalexander from comment #17) > (In reply to Nick Alexander :nalexander from comment #16) > > > I'll push the next iteration to you, and we'll move forward from there. > > > > Sorry for the delay, but I've run into a problem that I don't think we want > > to work around. tl;dr: non en-US locales cannot add resources not present > > in en-US. Let me explain why. > > Now that I've described the issue, and how to understand it more thoroughly, > let me discuss alternatives. I see essentially 3. > > 1. We fix the build system to do l10n re-packs that allow changing the > resource set at re-package time. > > 2. We always include "new" suggestedsites resources in en-US, even if > they're not consumed by en-US. > > 3. We don't use the Android resource system for density-specific drawables. > > There's a reason that we haven't yet done 1: it's a lot of work. It's not > just that it's a lot of in-tree work -- that would be great -- but it's a > lot of TBPL/automation work. The l10n re-pack logic is complicated and not > in the tree. There are a bazillion hidden assumptions (some of which I > discovered and documented above!). In theory, this is not hard -- just do > more work as part of |make package| -- but in practice this will be > laborious and hard to get right. Ok, let's not even consider option 1 then. > There's also a reason we (lucasr) wanted Android to handle density-specific > drawables, so option 3 is not awesome. One reason I think we should > consider it, however, is that we're going to serve the suggestedsites > metadata remotely at some point. At that time, we won't be able to use > Android drawables at all, since said drawables are static. Perhaps we build > that functionality up front and use it for the remotely served > suggestedsites, and the pre-shipped sites. The images for suggested sites are loaded using Picasso which is able to handle a good range of image sources: Android resource (the one I'm using in the current patch via the android.resource:// scheme), local files (through the file:// scheme), and remote images (via HTTP requests and all). So, the exact location of the built-in images doesn't really affect our ability to load remote images later. > Finally, option 2 is the clearly expedient choice. It does mean that > localizers can't customize suggestedsites without committing to m-c, but > early discussions with lucasr suggested that restriction might be a feature, > not a bug. Option 2 also obviates most of this build system work: it's > probably not necessary to parse list.txt to create suggestedsites.json, etc; > we can just preprocess suggestedsites.json.in and handle doing that for > additional locales. As a final benefit, all concerns around duplicating > drawables are mitigated, since all drawables are managed in locales/en-US. Even though option 2 would simplify things for me (i.e. I wouldn't have to change any of my original patches, yay), I'm not so keen on it because it kinda disrupts the current l10n workflow. So, with that said, here's what I'd like to explore: - Store the images somewhere else in the APK (maybe as raw assets in the APK?) - Implement a custom 'BitmapHunter'[1] in Picasso that understands image paths like gecko://suggestedsite/SITENAME. Under the hood, this BitmapHunter would know where the built-in drawables for each density are stored. I was already planning to do something similar for thumbnails anyway (e.g. gecko://thumbnail/ENCODEDURL). Nick, would storing the drawables as assets in the APK be any simpler? [1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/thirdparty/com/squareup/picasso/BitmapHunter.java#193
we should > > consider it, however, is that we're going to serve the suggestedsites > > metadata remotely at some point. At that time, we won't be able to use > > Android drawables at all, since said drawables are static. Perhaps we build > > that functionality up front and use it for the remotely served > > suggestedsites, and the pre-shipped sites. > > The images for suggested sites are loaded using Picasso which is able to > handle a good range of image sources: Android resource (the one I'm using in > the current patch via the android.resource:// scheme), local files (through > the file:// scheme), and remote images (via HTTP requests and all). So, the > exact location of the built-in images doesn't really affect our ability to > load remote images later. My claim is not that this choice to use Android resources blocks us later; my claim is that later, we will have to do *something* to choose images based on density. If we're going to do it ourselves later, we might as well just do it now. > > Finally, option 2 is the clearly expedient choice. It does mean that > > localizers can't customize suggestedsites without committing to m-c, but > > early discussions with lucasr suggested that restriction might be a feature, > > not a bug. Option 2 also obviates most of this build system work: it's > > probably not necessary to parse list.txt to create suggestedsites.json, etc; > > we can just preprocess suggestedsites.json.in and handle doing that for > > additional locales. As a final benefit, all concerns around duplicating > > drawables are mitigated, since all drawables are managed in locales/en-US. > > Even though option 2 would simplify things for me (i.e. I wouldn't have to > change any of my original patches, yay), I'm not so keen on it because it > kinda disrupts the current l10n workflow. Can you elaborate on the disruption? I agree it's not the same flow as localizing *_strings.dtd, but it could be very similar to localizing mobile/searchplugins. If we're just generating suggestedsites.json per-locale in some way, we could make this quite nice to localize. > So, with that said, here's what I'd like to explore: > - Store the images somewhere else in the APK (maybe as raw assets in the > APK?) > - Implement a custom 'BitmapHunter'[1] in Picasso that understands image > paths like gecko://suggestedsite/SITENAME. Under the hood, this BitmapHunter > would know where the built-in drawables for each density are stored. I was > already planning to do something similar for thumbnails anyway (e.g. > gecko://thumbnail/ENCODEDURL). > > Nick, would storing the drawables as assets in the APK be any simpler? Yes, I expect so. Further to our #mobile conversation, we can store assets as 1. "Gecko assets" in omni.ja. 2. "Android assets" in the assets/ folder. For 1, this is very standard: we add chrome assets or package-manifest.in entries, and they get packed into omni.ja. Consulting gavin, it's not clear that such assets can be localized easily :( There are examples of localized assets (like http://hg.mozilla.org/l10n-central/de/file/0bbbf5379c54/mobile/searchplugins/list.txt) but the build system handles them like special snow-flakes. It would be nice to not add additional snow-flakes. On the other hand, there is support for multiple chrome skins, and perhaps one can skin per-locale. If that's the case, maybe this is a solved problem. For 2, I'm pretty confident we can add whatever we want. The interface to fetch assets/ isn't awesome -- see [1] -- but it can be bent to our will. And I'm confident that we can make the build system bits work (including having assets coming from en-US and other locales). For example, I think aapt handles multiple asset paths; if not, we can merge assets ourselves. [1] http://developer.android.com/reference/android/content/res/AssetManager.html
Here's another idea to consider: use generic names for the default suggested sites' drawables. So, instead of doing drawable-[mdpi|hdpi|xhdpi]/suggestedsites_wikipedia.png, we'd do something like drawable-[mdpi|hdpi|xhdpi]/suggestedsites1.png. This means we'd reuse resource IDs across different locales. I can only think of one limitation in this approach: the locales wouldn't be able to have more suggested sites than en-US (which doesn't seem to be a big deal at first sight). Thoughts?
Flags: needinfo?(nalexander)
Flags: needinfo?(mark.finkle)
(In reply to Lucas Rocha (:lucasr) from comment #22) > Here's another idea to consider: use generic names for the default suggested > sites' drawables. So, instead of doing > drawable-[mdpi|hdpi|xhdpi]/suggestedsites_wikipedia.png, we'd do something > like drawable-[mdpi|hdpi|xhdpi]/suggestedsites1.png. This means we'd reuse > resource IDs across different locales. I can only think of one limitation in > this approach: the locales wouldn't be able to have more suggested sites > than en-US (which doesn't seem to be a big deal at first sight). Thoughts? We can probably mitigate the limitation described above by adding drawable stubs (i.e. suggestedsite1.xml pointing to android.R.drawable.transparent) in en-US so that we have a big enough maximum number of suggested sites (which doesn't necessarily need to be the number of suggested sites in en-US). This way we might give a bit more flexibility to localizers. In case it's not entirely obvious: the point of using generic drawable names here would be to workaround the limitation described in comment #16.
(In reply to Lucas Rocha (:lucasr) from comment #23) > (In reply to Lucas Rocha (:lucasr) from comment #22) > > Here's another idea to consider: use generic names for the default suggested > > sites' drawables. So, instead of doing > > drawable-[mdpi|hdpi|xhdpi]/suggestedsites_wikipedia.png, we'd do something > > like drawable-[mdpi|hdpi|xhdpi]/suggestedsites1.png. This means we'd reuse > > resource IDs across different locales. I can only think of one limitation in > > this approach: the locales wouldn't be able to have more suggested sites > > than en-US (which doesn't seem to be a big deal at first sight). Thoughts? > > We can probably mitigate the limitation described above by adding drawable > stubs (i.e. suggestedsite1.xml pointing to android.R.drawable.transparent) > in en-US so that we have a big enough maximum number of suggested sites > (which doesn't necessarily need to be the number of suggested sites in > en-US). This way we might give a bit more flexibility to localizers. > > In case it's not entirely obvious: the point of using generic drawable names > here would be to workaround the limitation described in comment #16. I see no technical reason this could not be made to work. It does make the build system generate.py script essential, and I reckon we'd see at least one bug due to drift there, but it could be made to work.
Flags: needinfo?(nalexander)
I did a little investigation into packing locale-specific resources into omni.ja. It should be easy! In fact, suite (SeaMonkey) already does this -- see http://hg.mozilla.org/l10n-central/de/file/0bbbf5379c54/suite/chrome/common/help/images and work backwards from the filenames using suite's mxr for examples. We can add .png resources to http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/jar.mn, like: locale/@AB_CD@/browser/suggestedsite_wikipedia.png (%chrome/suggestedsite_wikipedia.png) and then put the relevant file into the l10n repo at the correct place, and it should be packed into omni.ja by the existing mechanism. Of course, this doesn't address our density/fallback concerns, but looks possible. Caveat: I have not verified this works as intended.
(In reply to Nick Alexander :nalexander from comment #25) > I did a little investigation into packing locale-specific resources into > omni.ja. It should be easy! In fact, suite (SeaMonkey) already does this > -- see > http://hg.mozilla.org/l10n-central/de/file/0bbbf5379c54/suite/chrome/common/ > help/images and work backwards from the filenames using suite's mxr for > examples. > > We can add .png resources to > http://mxr.mozilla.org/mozilla-central/source/mobile/android/locales/jar.mn, > like: > > locale/@AB_CD@/browser/suggestedsite_wikipedia.png > (%chrome/suggestedsite_wikipedia.png) > > and then put the relevant file into the l10n repo at the correct place, and > it should be packed into omni.ja by the existing mechanism. Of course, this > doesn't address our density/fallback concerns, but looks possible. Caveat: > I have not verified this works as intended. Yeah, the main thing I'd like to avoid here (if possible anyway) is writing custom code for things that the Android platform gives us for free. So, I'd like to see how my suggested approach (comment #22) would look like before going ahead with either packing drawables in omni.ja or using Android assets.
(In reply to Nick Alexander :nalexander from comment #28) > Created attachment 8414576 [details] [diff] [review] > WIP on build system integration. These two patches are work in progress towards build system integration with minimal Make entanglement. You can see how I've essentially duplicated the strings.xml logic. There are several points in base/Makefile.in and base/moz.build that would require heavy commenting with this approach. With lucasr's "placeholder drawables" approach, this commenting would be simpler.
(In reply to Nick Alexander :nalexander from comment #29) > (In reply to Nick Alexander :nalexander from comment #28) > > Created attachment 8414576 [details] [diff] [review] > > WIP on build system integration. > > These two patches are work in progress towards build system integration with > minimal Make entanglement. You can see how I've essentially duplicated the > strings.xml logic. There are several points in base/Makefile.in and > base/moz.build that would require heavy commenting with this approach. With > lucasr's "placeholder drawables" approach, this commenting would be simpler. lucasr's placeholder drawables approach would be much better for the build system, since the list of "generated" resources is now known at build time. That is, we might add resources/drawable/suggestedsites_drawable{0-9}.xml (as placeholders, assuming they would get overridden by any density-specific drawable) and locales/en-US/suggestedsites/res/$DPI/$NAME.png. The set of generated resources is $OBJDIR/res/raw/suggestedsites.json and all {0-9}.png files. We use the generate_*.py script to do the actual lifting/renaming/input validation.
(In reply to Nick Alexander :nalexander from comment #30) > (In reply to Nick Alexander :nalexander from comment #29) > > (In reply to Nick Alexander :nalexander from comment #28) > > > Created attachment 8414576 [details] [diff] [review] > > > WIP on build system integration. > > > > These two patches are work in progress towards build system integration with > > minimal Make entanglement. You can see how I've essentially duplicated the > > strings.xml logic. There are several points in base/Makefile.in and > > base/moz.build that would require heavy commenting with this approach. With > > lucasr's "placeholder drawables" approach, this commenting would be simpler. > > lucasr's placeholder drawables approach would be much better for the build > system, since the list of "generated" resources is now known at build time. > That is, we might add > > resources/drawable/suggestedsites_drawable{0-9}.xml > > (as placeholders, assuming they would get overridden by any density-specific > drawable) and > > locales/en-US/suggestedsites/res/$DPI/$NAME.png. > > The set of generated resources is $OBJDIR/res/raw/suggestedsites.json and > all {0-9}.png files. We use the generate_*.py script to do the actual > lifting/renaming/input validation. Yep, that's exactly what I had in mind. Just to be extra clear, the locale-specific drawables would be installed as: objdir/mobile/android/base/res/drawable-$DPI/suggestedsites_drawable{0-9}.png (for en-US) ...and... objdir/mobile/android/base/res/drawable-$AB_rCD-$DPI/suggestedsites_drawable{0-9}.png (for other locales) And the placeholder drawables would look like: <?xml version="1.0" encoding="utf-8"?> <bitmap xmlns:android="http://schemas.android.com/apk/res/android" android:src="@android:color/transparent"/>
Attached patch Build system changes (obsolete) — — Splinter Review
Here's my WIP patch. Started doing the logic for resolving filenames based on multiple source directories but didn't have time to finish today. Also changed the drawable naming bits to follow the generic scheme suggestedsites_POSITION.
Comment on attachment 8415415 [details] [diff] [review] Add placeholder drawables for suggested sites (r=nalexander) This is the patch that adds the placeholder drawables for suggested sites. Starting with 10.
Attachment #8415415 - Flags: review?(nalexander)
Attached patch Build system changes (obsolete) — — Splinter Review
Flags: needinfo?(mark.finkle)
Attachment #8415414 - Attachment is obsolete: true
Comment on attachment 8415905 [details] [diff] [review] Build system changes Ok, this works fine with the (big) caveat that it duplicates images from en-US in the locales using the same website. But I changed enough things to be worth some feedback so here it is. That exposes another limitation that I missed when proposed using generic names for drawables: you can't just reuse images (i.e. avoid duplicating the images) if the same website is in different positions between en-US and the other locales. In theory, we could generate an alias drawable to re-map sites at different positions between locales but this doesn't sound like a good solution... Let me think about this a bit more.
Attachment #8415905 - Flags: feedback?(nalexander)
Attachment #8408334 - Attachment is obsolete: true
Attachment #8408335 - Attachment is obsolete: true
Attachment #8414575 - Attachment is obsolete: true
Attachment #8414576 - Attachment is obsolete: true
Attachment #8415415 - Attachment is obsolete: true
Attachment #8415415 - Flags: review?(nalexander)
Attachment #8415905 - Attachment is obsolete: true
Attachment #8415905 - Flags: feedback?(nalexander)
Comment on attachment 8418697 [details] [diff] [review] Add default suggested sites for en-US (r=nalexander) Note that we need the placeholders to ensure that all the possible drawable ids are around for any combination of suggested sites defined per-locale. For every new suggested site, we'll need to: 1. Add a placeholder empty drawable in mobile/android/base/resources/drawable/suggestedsite_NAME.xml 2. Add respective drawables per density to mobile/android/base/en-US/suggestedsites/res/*
Attachment #8418697 - Flags: review?(nalexander)
Comment on attachment 8418698 [details] [diff] [review] Generate suggestedsites.json from locale (r=nalexander) Ok, this is working pretty well. The main change: suggested site drawables always get installed from mobile/android/base/locales/en-US/suggestedsites/res/* to the respective general drawable resource directories for each density. This means a multi-locale build will only contain drawables for suggested sites actually being used by the target locales without any duplicated images.
Attachment #8418698 - Flags: review?(nalexander)
Comment on attachment 8418772 [details] [diff] [review] Introduce SuggestedSites component (r=mfinkle) Rebased/updated patch. Carrying on the r+ from mfinkle. Replace imageUrl/bgColor with imageurl/bgcolor as suggested.
Attachment #8418772 - Flags: review+
Comment on attachment 8418698 [details] [diff] [review] Generate suggestedsites.json from locale (r=nalexander) Review of attachment 8418698 [details] [diff] [review]: ----------------------------------------------------------------- In #mobile, lucasr and I decided to make this much simpler: we're going to land the relevant drawables in mobile/android/base/resources and simplify the script by not doing the per-locale drawable copying. The size penalty for shipping extra drawables is not huge, since most of our users are getting multi-locale APKs from the Google Play store anyway. ::: mobile/android/base/locales/Makefile.in @@ +40,5 @@ > chrome-%:: AB_CD=$* > +chrome-%:: > + @$(MAKE) \ > + $(dir-res-values)-$(AB_rCD)/strings.xml \ > + $(dir-res-raw)-$(AB_rCD)/suggestedsites.json \ hit: The indentation is a little funky here -- looks like '\t ' vs '\t\t'. ::: mobile/android/base/locales/en-US/suggestedsites/list.txt @@ -1,1 @@ > -mozilla Any reason to not fold this into the previous patch? ::: python/mozbuild/mozbuild/action/generate_suggestedsites.py @@ +2,5 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +'''Script to generate the suggestedsites.json file for Fennec.''' nit: spaces after, and let's document this now. @@ +40,5 @@ > + > + > +def main(args): > + parser = argparse.ArgumentParser() > + parser.add_argument('--srcdir', metavar='SRCDIR', I think an append would be good here -- no need to limit to one, and have a special ordering requirement. @@ +59,5 @@ > + parser.add_argument('--verbose', '-v', default=False, action='store_true', > + help='be verbose') > + opts = parser.parse_args(args) > + > + print(opts) kill. @@ +63,5 @@ > + print(opts) > + > + image_url_template = 'android.resource://%s/drawable/suggestedsites_{name}' % opts.android_package_name > + > + names = [s.strip() for s in open(resolve_fname(opts, 'list.txt'), 'rt').readlines()] nit: comment explaining what these are. @@ +67,5 @@ > + names = [s.strip() for s in open(resolve_fname(opts, 'list.txt'), 'rt').readlines()] > + > + sites = [] > + for name in names: > + fname = resolve_fname(opts, name.strip() + '.json') not needed, since you strip when defining names. @@ +97,5 @@ > + bumped |= updated > + > + manifest = InstallManifest() > + > + # We don't want to delete the resource we just wrote when we nit: based on remove_unaccounted=False below, this is not necessary anymore. @@ +124,5 @@ > + bumped = True > + os.utime(output_json, None) > + > + if bumped: > + print("touch %s" % output_json) I think we should add the os.utime here, based on bumped.
Attachment #8418698 - Flags: review?(nalexander) → feedback+
Attachment #8418697 - Attachment is obsolete: true
Attachment #8418697 - Flags: review?(nalexander)
Attachment #8418698 - Attachment is obsolete: true
Comment on attachment 8419379 [details] [diff] [review] Add default suggested sites for en-US (r=nalexander) Removed placeholder images, moved drawables directly to the respective drawable directories.
Attachment #8419379 - Flags: review?(nalexander)
Attachment #8419380 - Attachment is obsolete: true
Comment on attachment 8419388 [details] [diff] [review] Generate suggestedsites.json from locale (r=nalexander) Ok, this is working pretty well. Main changes: - Add documentation and extra comments in the script - Removed the code to copy drawables around from the script - Removed --basedir option and only use the list of source directories (In reply to Nick Alexander :nalexander from comment #44) > Comment on attachment 8418698 [details] [diff] [review] > Generate suggestedsites.json from locale (r=nalexander) > > Review of attachment 8418698 [details] [diff] [review]: > ----------------------------------------------------------------- > > In #mobile, lucasr and I decided to make this much simpler: we're going to > land the relevant drawables in mobile/android/base/resources and simplify > the script by not doing the per-locale drawable copying. > > The size penalty for shipping extra drawables is not huge, since most of our > users are getting multi-locale APKs from the Google Play store anyway. This is all implemented in this patch. > ::: mobile/android/base/locales/Makefile.in > @@ +40,5 @@ > > chrome-%:: AB_CD=$* > > +chrome-%:: > > + @$(MAKE) \ > > + $(dir-res-values)-$(AB_rCD)/strings.xml \ > > + $(dir-res-raw)-$(AB_rCD)/suggestedsites.json \ > > hit: The indentation is a little funky here -- looks like '\t ' vs '\t\t'. Fixed. > ::: mobile/android/base/locales/en-US/suggestedsites/list.txt > @@ -1,1 @@ > > -mozilla > > Any reason to not fold this into the previous patch? Leftover from local tests. Removed. > ::: python/mozbuild/mozbuild/action/generate_suggestedsites.py > @@ +2,5 @@ > > +# This Source Code Form is subject to the terms of the Mozilla Public > > +# License, v. 2.0. If a copy of the MPL was not distributed with this > > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > > + > > +'''Script to generate the suggestedsites.json file for Fennec.''' > > nit: spaces after, and let's document this now. Added documentation about what the script does. > @@ +40,5 @@ > > + > > + > > +def main(args): > > + parser = argparse.ArgumentParser() > > + parser.add_argument('--srcdir', metavar='SRCDIR', > > I think an append would be good here -- no need to limit to one, and have a > special ordering requirement. Yep, done. > @@ +59,5 @@ > > + parser.add_argument('--verbose', '-v', default=False, action='store_true', > > + help='be verbose') > > + opts = parser.parse_args(args) > > + > > + print(opts) > > kill. Done. > @@ +63,5 @@ > > + print(opts) > > + > > + image_url_template = 'android.resource://%s/drawable/suggestedsites_{name}' % opts.android_package_name > > + > > + names = [s.strip() for s in open(resolve_fname(opts, 'list.txt'), 'rt').readlines()] > > nit: comment explaining what these are. Done. > @@ +67,5 @@ > > + names = [s.strip() for s in open(resolve_fname(opts, 'list.txt'), 'rt').readlines()] > > + > > + sites = [] > > + for name in names: > > + fname = resolve_fname(opts, name.strip() + '.json') > > not needed, since you strip when defining names. Right, removed the strip() call. > @@ +97,5 @@ > > + bumped |= updated > > + > > + manifest = InstallManifest() > > + > > + # We don't want to delete the resource we just wrote when we > > nit: based on remove_unaccounted=False below, this is not necessary anymore. Removed. > @@ +124,5 @@ > > + bumped = True > > + os.utime(output_json, None) > > + > > + if bumped: > > + print("touch %s" % output_json) > > I think we should add the os.utime here, based on bumped. I don't follow. Add os.utime where?
Attachment #8419388 - Flags: review?(nalexander)
This patch: * removes things un-used in the script; * replaces the destination director with the output filename; * throws if a matching drawable is not found in a given resources directory.
Comment on attachment 8419610 [details] [diff] [review] TO BE FOLDED : Generate suggestedsites.json from locale The version of the script you posted has a lot of stuff that's not needed any more (imports, etc), so I really can't r+ it as is. However, there's nothing blocking a landing at this point, just nits and clean-up, which I've implemented here. Have a look and if you're happy, fold it in to your existing work, and bombs away, r=nalexander. It's you and me fixing any multi-locale fallout anyway :)
(In reply to Nick Alexander :nalexander from comment #50) > Created attachment 8419610 [details] [diff] [review] > TO BE FOLDED : Generate suggestedsites.json from locale > > This patch: > > * removes things un-used in the script; > * replaces the destination director with the output filename; > * throws if a matching drawable is not found in a given resources > directory. This should land with the existing --verbose flag left in for a while; once it's working well, we'll remove the --verbose (and possibly even add --silent). I really want the logging for multi-locale builds on infra for a while.
Apologies! I should have audited the patch for unused code before submitting for review. Your changes look all good to me. Only had to fix a typo in a comment. Pushed with your changes: https://hg.mozilla.org/integration/fx-team/rev/ec9ea3c7600b https://hg.mozilla.org/integration/fx-team/rev/cd5a3e3b0d3d https://hg.mozilla.org/integration/fx-team/rev/6ebcd2ebbed4 One possible follow-up: we create a raw-LOCALE/suggestedsites.json file for each locale, even if the locale doesn't override the list.txt. Maybe we should just avoid creating the locale-specific suggestedsites.json in this case? Thoughts?
Flags: needinfo?(nalexander)
Comment on attachment 8419379 [details] [diff] [review] Add default suggested sites for en-US (r=nalexander) Flipping to r+ for clarity (see comment #51)
Attachment #8419379 - Flags: review?(nalexander) → review+
Comment on attachment 8419388 [details] [diff] [review] Generate suggestedsites.json from locale (r=nalexander) Flipping to r+ for clarity (see comment #51)
Attachment #8419388 - Flags: review?(nalexander) → review+
Blocks: 1008230
> One possible follow-up: we create a raw-LOCALE/suggestedsites.json file for > each locale, even if the locale doesn't override the list.txt. Maybe we > should just avoid creating the locale-specific suggestedsites.json in this > case? Thoughts? FYI: filed bug 1008230 to track this.
(In reply to Lucas Rocha (:lucasr) from comment #56) > > One possible follow-up: we create a raw-LOCALE/suggestedsites.json file for > > each locale, even if the locale doesn't override the list.txt. Maybe we > > should just avoid creating the locale-specific suggestedsites.json in this > > case? Thoughts? > > FYI: filed bug 1008230 to track this. This is reasonable: even for single locale repacks, we include the default (really en-US) resources, so we will never leave an APK without something matching raw/suggestedsites.
Flags: needinfo?(nalexander)
https://hg.mozilla.org/mozilla-central/rev/ec9ea3c7600b https://hg.mozilla.org/mozilla-central/rev/cd5a3e3b0d3d https://hg.mozilla.org/mozilla-central/rev/6ebcd2ebbed4
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
This is looking good. I downloaded http://ftp.mozilla.org/pub/mozilla.org/mobile/nightly/2014/05/2014-05-10-03-02-04-mozilla-central-android/fennec-32.0a1.multi.android-arm.apk and |aapt dump resources| reveals reasonable drawables and raw/suggestedsites for multiple locales: 1191: spec resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: flags=0x00000004 1194: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x000000ea (s=0x0008 r=0x00) 1196: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x00000310 (s=0x0008 r=0x00) 1198: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x00000316 (s=0x0008 r=0x00) 1200: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x00000311 (s=0x0008 r=0x00) 1202: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x00000313 (s=0x0008 r=0x00) 1204: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x0000031e (s=0x0008 r=0x00) 1206: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x00000319 (s=0x0008 r=0x00) 1208: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x0000031a (s=0x0008 r=0x00) 1210: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x00000317 (s=0x0008 r=0x00) 1212: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x00000314 (s=0x0008 r=0x00) 1214: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x0000030f (s=0x0008 r=0x00) 1216: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x00000315 (s=0x0008 r=0x00) 1218: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x0000031d (s=0x0008 r=0x00) 1220: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x0000031f (s=0x0008 r=0x00) 1222: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x00000320 (s=0x0008 r=0x00) 1224: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x00000318 (s=0x0008 r=0x00) 1226: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x0000031b (s=0x0008 r=0x00) 1228: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x00000312 (s=0x0008 r=0x00) 1230: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x0000031c (s=0x0008 r=0x00) 1232: resource 0x7f060001 org.mozilla.fennec:raw/suggestedsites: t=0x03 d=0x00000321 (s=0x0008 r=0x00)
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: