Staś Małolepszy :stas Reporter
	Description • 11 years ago

Currently, it's an XOR:  we first check for the manifest link, and only if it's missing do we go on to add resources defined in <script> elements.

I think it might be useful and certainly will be more predictable to make the both co-exist peacefully.

As per bug 897862, the translations defined in the script are added to all initialized locales.

	Staś Małolepszy :stas Reporter
	Comment 1 • 11 years ago

I have a WIP patch for this which I'll attach in a few hours.

	Staś Małolepszy :stas Reporter
	Comment 2 • 11 years ago

This is a pretty simple change which simplifies the API.  I'd like to take this in 1.0.

	Staś Małolepszy :stas Reporter
	Comment 3 • 11 years ago

I remembered you were worried about the cost of qSA, so I did a round of perf tests (Settings, l20n-master, airplane mode on):

  l20n-master:  median:1097, mean:1100, std: 20
  with patch:   median:1097, mean:1104, std: 27

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 4 • 11 years ago

Comment on attachment 820873 [details] [diff] [review]
Patch

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

::: bindings/l20n/gaia.js
@@ +117,5 @@
>      }
>  
> +    // 2. Freeze the context and start fetching the resources
> +    if (manifestLink) {
> +      loadManifest(manifestLink.getAttribute('href'));

one consequence of that that I see is that resources loaded from manifest will be loaded behind resources loaded from file, which means that if entities are overwritten, the ones from script will overwrite the ones from manifest-linked resources.

As a developer I think I would expect it to be the other way around?

@@ +133,5 @@
> +        });
> +      } else if (scripts.length) {
> +        ctx.requestLocales();
> +      } else {
> +        console.warn('L20n: No resources defined.');

that got me thinking. So an app with no resources cannot rely on DocumentLocalized... should we fire ctx.requestLocales in that case anyway?

@@ +139,4 @@
>      }
>  
> +    // 3. Localize the DOM
> +    collectNodes();

and should we skip collecting nodes if we have no resources?

	Staś Małolepszy :stas Reporter
	Comment 5 • 11 years ago

Great review comments, thanks Gandalf.  I think you convinced me that this should not be part of 1.0, as the consequences are many.  Should we consider it for 1.1 instead?

(In reply to Zbigniew Braniecki [:gandalf] from comment #4)

> one consequence of that that I see is that resources loaded from manifest
> will be loaded behind resources loaded from file, which means that if
> entities are overwritten, the ones from script will overwrite the ones from
> manifest-linked resources.
> 
> As a developer I think I would expect it to be the other way around?

I actually don't know.  There are reasons to have it both ways.  That's why I think we should no land this for 1.0.  Forbidding both the manifest and inline resources is a bit limiting, but also much more predictable.

> @@ +133,5 @@
> > +        });
> > +      } else if (scripts.length) {
> > +        ctx.requestLocales();
> > +      } else {
> > +        console.warn('L20n: No resources defined.');
> 
> that got me thinking. So an app with no resources cannot rely on
> DocumentLocalized... should we fire ctx.requestLocales in that case anyway?

Sounds like a different bug.  We could also throw.

> @@ +139,4 @@
> >      }
> >  
> > +    // 3. Localize the DOM
> > +    collectNodes();
> 
> and should we skip collecting nodes if we have no resources?

Yeah, I think we should throw :)

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 6 • 11 years ago

yup, let's take it for 1.1

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 7 • 9 years ago

That's invalid because we discontinued manifests.