Closed Bug 1048411 Opened 10 years ago Closed 10 years ago

Remove GAIA_INLINE_LOCALES optimization

Categories

(Firefox OS Graveyard :: Gaia::L10n, defect, P3)

Product:
Firefox OS Graveyard
Component:
Gaia::L10n
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: stas, Assigned: zbraniecki)

Details

User Story

Gaia uses two optimizations flags - GAIA_CONCAT_LOCALES and GAIA_INLINE_LOCALES.
 - concat scans HTML files, combines all l10n resources into a single JSON file and links it instead of multiple l10n resources files.
 - inline takes a subset of strings, builds JSON resource out of them and includes the resource for all locales inside the HTML file

The goal of the GAIA_INLINE_LOCALES is to provide a way to quickly translate the visible DOM without having to load any external resource to avoid flashes of unstyled content.

The approach does not currently work at all. Turning off GAIA_INLINE_LOCALES does not seem to introduce any FOUCs at all. 
Testing on Tarako, Flame, Keon, Buri and Peak it seems that current l10n.js is capable of downloading resources and localizing DOM using them before firstPaint without regressing performance. In fact, turning GAIA_INLINE_LOCALES off provides minor performance wins for non default locale scenarios.

At the same time, GAIA_INLINE_LOCALES costs us:
 - it blows the HTML file size by adding a JSON object per locale
 - it makes l10n startup code much more complex
 - it costs us memory because we have to store the partial localizations

We'd like to turn off this optimization for 2.1 cycle and if it does not regress proceed to remove the functionality in the next cycle.

Attachments

(2 files)

pull request
(deleted), text/x-github-pull-request
Details
patch
(deleted), patch
stas
: review+
Details | Diff | Splinter Review 
In bug 994370 we turned off GAIA_INLINE_LOCALES.  If there is no evidence of perf regressions after a few weeks, the plan is to remove the feature all together. This will simplify the codebase quite much.
Priority: -- → P3
Attached file pull request (deleted) — 

  

  


Assignee: nobody → gandalf
Status: NEW → ASSIGNED

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch (deleted)
        — 
          — Splinter Review
      

    


  

  

  



        Attachment #8490586 -
        Flags: review?(stas)

    
    

    
  
Comment on attachment 8490586 [details] [diff] [review]
patch

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

r=me.  Let me just say that I'm impressed by how bug 1055328 helped to set the stage for this one :)  I found one problem with neterror when testing but not related to this patch, so I filed bug 1068588 which I'll try to fix.

I wonder if it's possible to test this on Tarako?  I was only able to flash v1.3 onto mine;  have you had more luck flashing a newer version with the refactored l10n.js?

::: locales/README.md
@@ -8,5 @@
>        LOCALES_FILE=locales/languages_basecamp.json \
>        GAIA_DEFAULT_LOCALE=pt-BR
>  
> -Use `GAIA_INLINE_LOCALES=1` to precompile all HTML to include text content in 
> -the deafult locale.

Keep this in but change it to mention GAIA_PRETRANSLATE=1 maybe?

::: shared/js/l10n.js
@@ +1469,3 @@
>  
>    function init(pretranslate) {
>      if (pretranslate) {

Now that we're removing inlineLocalization, the semantics of the 'pretranslate' flag are slightly different.  Maybe rename it to something else so that it doesn't get confused with the GAIA_PRETRANSLATE setting?

@@ -1504,5 @@
> -        code: locale.id,
> -        direction: getDirection(locale.id)
> -      }
> -    };
> -    translateDocument.call(l10n);

You should be able to remove Locale.prototype.getEntity now, too.  I don't think it was used anywhere else but here.

  

  



        Attachment #8490586 -
        Flags: review?(stas) → review+

    
    

    
  
(In reply to Staś Małolepszy :stas from comment #3)

> I wonder if it's possible to test this on Tarako?  I was only able to flash
> v1.3 onto mine;  have you had more luck flashing a newer version with the
> refactored l10n.js?

I was able to build and flash Gaia master (2.2) and Gecko 35 onto my Tarako.  Pretty impressive how it holds up.  I didn't see any flashes of English content except for the Clock app, which I AFAIR is a known bug.

  

  



    
    

    
  
(In reply to Staś Małolepszy :stas from comment #4)
> I was able to build and flash Gaia master (2.2) and Gecko 35 onto my Tarako.
> Pretty impressive how it holds up.  I didn't see any flashes of English
> content except for the Clock app, which I AFAIR is a known bug.

Awesome! Yes, it's bug 992725.

  

  



    
    

    
  
(In reply to Staś Małolepszy :stas from comment #3)
> ::: shared/js/l10n.js
> @@ +1469,3 @@
> >  
> >    function init(pretranslate) {
> >      if (pretranslate) {
> 
> Now that we're removing inlineLocalization, the semantics of the
> 'pretranslate' flag are slightly different.  Maybe rename it to something
> else so that it doesn't get confused with the GAIA_PRETRANSLATE setting?

I would actually prefer to leave it. Runtime pretranslate is exactly what happens - we go and pretranslate the whole document before firstPaint.

  

  



    
    

    
  
Commit: https://github.com/mozilla-b2g/gaia/commit/e1efcef59c92472bcc5aa4fb38130583281d6325
Merge: https://github.com/mozilla-b2g/gaia/commit/f33bf136220e1c114aadaf63237be6bbdc834b53

  

  


Status: ASSIGNED → RESOLVED
Closed: 10 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: