Closed Bug 1303883 Opened 8 years ago Closed 8 years ago

Port a UI feature of Firefox for Android

Categories

(L20n :: General, defect)

Product:
L20n
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: stas, Assigned: stas)

References

Details

(Whiteboard: [gecko-l20n])

User Story

Experiment with using L20n in Fennec's Downloads view.

Attachments

(1 file, 3 obsolete files)

Bug 1303883 - Localize Fennec's about:downloads with L20n.
58 bytes, text/x-review-board-request
Pike
: review+
Details 
We'd like to experiment with using L20n in Firefox for Android. The Downloads view might be a good candidate.
I was able to build Fennec locally and install it on my old Android phone.

It looks like there are a few XHTML pages that we could pick for this exercise:

  - about:accounts,
  - about:addons,
  - about:downloads,
  - about:logins,
  - about:privatebrowsing.

I'll start with about:downloads because it has just a handful of strings but still uses plurals and interpolation as well as a few sync API calls in aboutDownload.js which will be interesting to port.
Assignee: nobody → stas
Status: NEW → ASSIGNED
I have a feature-complete WIP at http://hg.mozilla.org/users/smalolepszy_mozilla.com/larch/shortlog/1303883-l20n-android-downloads.  I'd like to test it a bit more before asking for reviews.
Depends on: 1307164
Comment on attachment 8797256 [details]
Bug 1303883 - Localize Fennec's about:downloads with L20n.

https://reviewboard.mozilla.org/r/82848/#review81722

Two nits, but r=me.

::: mobile/android/locales/en-US/browser/aboutDownloads.ftl:5
(Diff revision 1)
> +# 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/.
> +
> +[[ about-downloads ]]

Why have a section here? Seems our scope is well set by the filename.

::: mobile/android/locales/jar.mn:39
(Diff revision 1)
>  #ifndef RELEASE_BUILD
>    locale/@AB_CD@/browser/webcompatReporter.properties (%chrome/webcompatReporter.properties)
>  #endif
>  % resource search-plugins chrome://browser/locale/searchplugins/
>  
> +  browser                                         (%browser/*.ftl)

Sorry, reconsidered. Let's use mobile instead of browser.
Attachment #8797256 - Flags: review?(l10n) → review+
Comment on attachment 8797257 [details]
Bug 1303883 - Part 3: Localize aboutDownloads.xhtml.

https://reviewboard.mozilla.org/r/82850/#review81724
Attachment #8797257 - Flags: review?(l10n) → review+
Comment on attachment 8797258 [details]
Bug 1303883 - Part 4: Localize aboutDownloads.js.

https://reviewboard.mozilla.org/r/82852/#review81726

Leaving this request open for now to hear back on wether we should actually have download-size-known.

::: mobile/android/chrome/content/aboutDownloads.js:206
(Diff revision 1)
>      let finished = this.finished;
>      if (finished.length == 0) {
>        return;
>      }
>  
> -    let title = strings.GetStringFromName("downloadAction.deleteAll");
> +    document.l10n.get('main').formatValues(

I wonder if it'd make sense to have

.l10n.formatValues be an alias to .l10n.get('main').formatValues?

Not as part of this bug, of course.

::: mobile/android/locales/en-US/browser/aboutDownloads.ftl:39
(Diff revision 1)
>  download-state-failed      = Failed
>  download-state-paused      = Paused
>  download-state-starting    = Starting…
> +download-state-unknown     = Uknown
>  
> -unknown-size = Unknown size
> +download-size-known   = { $size }

Add a localization note here? Also, do we expect that localizers actually should see this string or is this just a convenience for the patch?
Generally speaking, I think these patches are small enough to be reviewed in one go, and for regression testing, it'd probably be better to not have individual commits. I think we should squash the three feature commits at least for landing.

Which makes me wonder, did you run tests on this?
Comment on attachment 8797258 [details]
Bug 1303883 - Part 4: Localize aboutDownloads.js.

https://reviewboard.mozilla.org/r/82852/#review81728

::: mobile/android/locales/en-US/browser/aboutDownloads.ftl:37
(Diff revision 1)
>  download-state-downloading = Downloading…
>  download-state-canceled    = Canceled
>  download-state-failed      = Failed
>  download-state-paused      = Paused
>  download-state-starting    = Starting…
> +download-state-unknown     = Uknown

Typo
(In reply to Axel Hecht [:Pike] from comment #7)

> > +[[ about-downloads ]]
> 
> Why have a section here? Seems our scope is well set by the filename.

You're right.  I wanted to try and see if it made sense to always use a section.  This could make strings more resilient to moves across files and the section comment would be attached to strings in a more explicit manner.

> > +  browser                                         (%browser/*.ftl)
> 
> Sorry, reconsidered. Let's use mobile instead of browser.

No worries, I'll change it.

(In reply to Axel Hecht [:Pike] from comment #9)

> I wonder if it'd make sense to have
> 
> .l10n.formatValues be an alias to .l10n.get('main').formatValues?
> 
> Not as part of this bug, of course.

We could but I'd prefer to delay it as long as possible :)  If we add it now, it's part of the official API and we need to maintain it.


> ::: mobile/android/locales/en-US/browser/aboutDownloads.ftl:39
> (Diff revision 1)
> >  download-state-failed      = Failed
> >  download-state-paused      = Paused
> >  download-state-starting    = Starting…
> > +download-state-unknown     = Uknown
> >  
> > -unknown-size = Unknown size
> > +download-size-known   = { $size }
> 
> Add a localization note here? Also, do we expect that localizers actually
> should see this string or is this just a convenience for the patch?

Right now it's mostly convenience (avoid assigning to textContent and having to remove data-l10n-id) but in the future we should be able to use the UNIT() builtin here instead of going via DownloadUtils.jsm on the source code side.  Would you prefer to not expose this string to the localizers for now?

I'd like to have a bit more discussion about this.  In the current patch, I created two strings, download-size-known and download-size-unknown for this part of the UI, but it could have also be handled with a single string like so (assuming #size is a string with the size or a special-case "unknown" string):

  download-size = { $size ->
      [unknown] Unknown size
     *[other]   { $size }
  }

Is this something that we'd prefer?  This approach gives more power to the localizer but I'm not sure if the localizer benefits really from this additional power here and also 1) this makes the string more complex and 2) makes it hard for tools like compare-locales to ensure that the "unknown" case is handled properly by the translation.

I'd like to get a consensus on the good practice here.


(In reply to Axel Hecht [:Pike] from comment #10)
> Generally speaking, I think these patches are small enough to be reviewed in
> one go, and for regression testing, it'd probably be better to not have
> individual commits. I think we should squash the three feature commits at
> least for landing.

Sure, I'll squash and submit a new patch when I'm done applying your feedback.

> Which makes me wonder, did you run tests on this?

Not yet, I'll push to try when I'm done with this iteration.

(In reply to Francesco Lodolo [:flod] from comment #11)

> > +download-state-unknown     = Uknown
> 
> Typo

Good catch, thanks!
(In reply to Staś Małolepszy :stas from comment #12)
> (In reply to Axel Hecht [:Pike] from comment #7)
> 
> > > +[[ about-downloads ]]
> > 
> > Why have a section here? Seems our scope is well set by the filename.
> 
> You're right.  I wanted to try and see if it made sense to always use a
> section.  This could make strings more resilient to moves across files and
> the section comment would be attached to strings in a more explicit manner.

I kinda doubt that tools like pontoon will use sections for anything but showing section comments anytime soon. In particular if we use kinda code-namish ones like about-downloads.

Do we have a semantic concept for a file-wide localization note? 

> (In reply to Axel Hecht [:Pike] from comment #9)
> > ::: mobile/android/locales/en-US/browser/aboutDownloads.ftl:39
> > (Diff revision 1)
> > >  download-state-failed      = Failed
> > >  download-state-paused      = Paused
> > >  download-state-starting    = Starting…
> > > +download-state-unknown     = Uknown
> > >  
> > > -unknown-size = Unknown size
> > > +download-size-known   = { $size }
> > 
> > Add a localization note here? Also, do we expect that localizers actually
> > should see this string or is this just a convenience for the patch?
> 
> Right now it's mostly convenience (avoid assigning to textContent and having
> to remove data-l10n-id) but in the future we should be able to use the
> UNIT() builtin here instead of going via DownloadUtils.jsm on the source
> code side.  Would you prefer to not expose this string to the localizers for
> now?

Yeah, in particular if we'd use it semantically different in the future, we should add the string with those semantics in the future, IMHO.

> I'd like to have a bit more discussion about this.  In the current patch, I
> created two strings, download-size-known and download-size-unknown for this
> part of the UI, but it could have also be handled with a single string like
> so (assuming #size is a string with the size or a special-case "unknown"
> string):
> 
>   download-size = { $size ->
>       [unknown] Unknown size
>      *[other]   { $size }
>   }
> 
> Is this something that we'd prefer?  This approach gives more power to the
> localizer but I'm not sure if the localizer benefits really from this
> additional power here and also 1) this makes the string more complex and 2)
> makes it hard for tools like compare-locales to ensure that the "unknown"
> case is handled properly by the translation.
> 
> I'd like to get a consensus on the good practice here.

I was never too happy about the "use" of 0 as a special case for a semantically different string in fxos, and this would be somewhat along those lines.

I much prefer semantic IDs for strings, where we use a ID for unknown, and only a ID for known once we can actually have localizers do something with it.
(In reply to Axel Hecht [:Pike] from comment #13)
> Do we have a semantic concept for a file-wide localization note? 

Yes, FTL parser separates file-wide comment.
The test failures in the run above are related to bug 1307164.  I have a new patch ready for review which I'll attach in a few.  Depending on when we land it, I might need to incorporate changes from bug 1280680, too.
Attachment #8797257 - Attachment is obsolete: true
Attachment #8797258 - Attachment is obsolete: true
Attachment #8797258 - Flags: review?(l10n)
Comment on attachment 8797255 [details]
Bug 1303883 - Part 1: Add bindings for chrome-privileged XHTML.

I'll land this in bug 1280670.
Attachment #8797255 - Attachment is obsolete: true
Attachment #8797255 - Flags: review?(gandalf)
Depends on: 1280670
diff --git a/mobile/android/locales/jar.mn b/mobile/android/locales/jar.mn
--- a/mobile/android/locales/jar.mn
+++ b/mobile/android/locales/jar.mn
@@ -97,4 +97,5 @@
 % override chrome://global/locale/plugins.properties chrome://browser/locale/overrides/plugins.properties
 
 [localization] @AB_CD@.jar:
+relativesrcdir mobile/android/locales:
   mobile                                          (%mobile/*.ftl)



it is. The problem is that the various override thingies for toolkit stuff confuzzle jar.mn about being in mobile/android, so adding this line sets things back to normal.
(In reply to Staś Małolepszy :stas from comment #23)
> Comment on attachment 8797256 [details]
> Bug 1303883 - Localize Fennec's about:downloads with L20n.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/82848/diff/3-4/

All comments and packaging problems are addressed, this is good to land.
This is now fixed.  Thanks for the review!
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: