Localize developer warnings issued by the manifest processor

RESOLVED FIXED in Firefox 47

Status

()

Core
DOM
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: marcosc, Assigned: marco)

Tracking

(Blocks: 1 bug)

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(URL)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

3 years ago
As the manifest file is being processed, the processor needs to emit errors to the developer console.
(Reporter)

Updated

3 years ago
Blocks: 1079453
(Reporter)

Updated

3 years ago
No longer blocks: 1079453
Depends on: 1079453
(Reporter)

Comment 1

3 years ago
Need to also add i18n support as part of this.
(Reporter)

Comment 2

3 years ago
Is there some kind of guide to adding localized strings to an API? Or could someone mentor me? 

Trying to add developer warnings to the developer console for web manifests.
Flags: needinfo?(l10n)

Comment 3

3 years ago
That largely depends on where the code is, and how it's supposed to be shown.
Flags: needinfo?(l10n)
(Reporter)

Comment 4

3 years ago
Hi Axel, 

(In reply to Axel Hecht [:Pike] from comment #3)
> That largely depends on where the code is, and how it's supposed to be shown.

The code is currently in `dom/manifest/ManifestProcessor.jsm` and it's supposed to be shown in the web developer console.
Flags: needinfo?(l10n)

Comment 5

3 years ago
I suspect that ManifestProcessor runs on the debugged device?

Then the best way to handle this would be to package up the data for the message, key and meta/context data, marshall it to the developer console, and on the desktop/debugging side, localize it.

That's for two reasons:

Debugged device and debugging desktop don't need to be on the same language, and it's better to have the devtools be consistent in language.

And also, we don't localize the dom code on the device at all.
Flags: needinfo?(l10n)
(Reporter)

Comment 6

3 years ago
(In reply to Axel Hecht [:Pike] from comment #5)
> I suspect that ManifestProcessor runs on the debugged device?

It just runs in the Gecko, like any other bit of DOM code. It's supposed to be device independent. 

> Then the best way to handle this would be to package up the data for the
> message, key and meta/context data, marshall it to the developer console,
> and on the desktop/debugging side, localize it.
> 
> That's for two reasons:
> 
> Debugged device and debugging desktop don't need to be on the same language,
> and it's better to have the devtools be consistent in language.
> 
> And also, we don't localize the dom code on the device at all.

Sorry, I'm still fairly new to contributing to Gecko and I'm having a hard time following what you are saying. Let me try to be more clear about what I'm trying to do as I'm a bit lost.  

A website can, through a HTML link element, declare that it uses a "web manifest". This looks like this:

```HTML
<!doctype html>
<link rel=manifest href="myManifest.json">
```

When the end user wants to bookmark a web page, Gecko downloads the "myManifest.json" file, and sends it to the ManifestProcessor.jsm file for processing. As ManifestProcessor is doing its thing, I want it to emit "developer warnings" to the developer console. Like:

```
//Just a made up example
console.warn("The start_url member needs to be a URL."); 
```

In https://bugzilla.mozilla.org/show_bug.cgi?id=1079453#c5, :flod said that "Since [the warnings are] exposed to users in the UI, even if hidden in the Web Console, it would be correct to make them localizable." 

So that's what I'm trying to do - make the warnings localizable. 

What I don't know is: 

1. where do I put the .properties file? Is there some special location?
2. how do I load the .properties file? I see in dxr there is a "Services.strings.createBundle()" API being used in various places? Is that what I am supposed to use? 
3. I assume I need to list the .properties file in the .mozbuild file? Does it need to go somewhere else too?  
 
And so on... just really basic stuff. 

I will confirm with Dev's tools people if I'm using the correct console. Currently, I'm loading this API:


```
const { ConsoleAPI } = Cu.import('resource://gre/modules/devtools/Console.jsm');
const console = new ConsoleAPI({prefix: 'Web Manifest: '});
```

But that's a separate thing. Anyway, I hope that makes a bit more sense. 

If there is some example or documentation I should follow, that would be most helpful. What I'm trying to do (I assume) is pretty basic (just put the .properties somewhere, and load strings as needed to `console.log()` them - then other contributors can add localized strings to that file if they want to improve dev tools. I only intend to add the warnings in English).
Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)

Comment 7

3 years ago
I don't think this is a "basic" problem of localization, thus I'm suggesting that you're doing something more complex.

This is a question about which processes run which code, and if `console.warn` does anything constructive. My assumption is that in particular in the context of "I install my app on the phone and connect via devtools" it doesn't.
Flags: needinfo?(l10n)
Flags: needinfo?(francesco.lodolo)
(Reporter)

Updated

3 years ago
Blocks: 997779
(In reply to Marcos Caceres [:marcosc] from comment #8)
> clues... https://pastebin.mozilla.org/8834484

This pastebin has expired, is there any more information available on what you found?
Flags: needinfo?(mcaceres)
(Reporter)

Comment 10

2 years ago
(In reply to Brendan Dahl [:bdahl] from comment #9)
> (In reply to Marcos Caceres [:marcosc] from comment #8)
> > clues... https://pastebin.mozilla.org/8834484
> 
> This pastebin has expired, is there any more information available on what
> you found?

Hey, so I think I know how to do this... basically, in the manifest processor, we should have a thing that collects all the warnings. Then, in the child process, we can grab contentWindow.console.warn(), and just use that to spit out the warnings.
Flags: needinfo?(mcaceres)
(Assignee)

Comment 11

2 years ago
(In reply to Marcos Caceres [:marcosc] from comment #10)
> Hey, so I think I know how to do this... basically, in the manifest
> processor, we should have a thing that collects all the warnings. Then, in
> the child process, we can grab contentWindow.console.warn(), and just use
> that to spit out the warnings.

Wouldn't the messages still be localized with the wrong language in this case (as Axel was saying in comment 7)?
(Assignee)

Comment 12

2 years ago
Marcos, correct me if I'm wrong. This bug isn't about adding developer warnings (which are already there), but about localizing them.
Flags: needinfo?(mcaceres)
(Assignee)

Comment 13

2 years ago
I see what's the other issue now, the messages aren't printed when you connect via the developer tools to a remote device.

I've opened bug 1237021 for that issue and I've morphed this to only track localization.
Flags: needinfo?(mcaceres)
Summary: Processing manifest needs to issue developer warnings → Localize developer warnings issued by the manifest processor
(Assignee)

Comment 14

2 years ago
On IRC, jryans suggested we open a discussion on dev-developer-tools, because there isn't any other similar case where we localize the messages on the debugger device.
(Assignee)

Comment 15

2 years ago
The localization is never done on the debugger device, so I think we shouldn't try to solve that problem in this bug. I've opened bug 1240203 for finding a general solution for all warnings and errors.

In this bug we should just localize the strings like we do in other modules, without worrying about which device is going to perform the localization.
(Assignee)

Updated

2 years ago
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
(Assignee)

Comment 16

2 years ago
Created attachment 8708979 [details] [diff] [review]
manifest_localize

Who can review this?
(Assignee)

Updated

2 years ago
Flags: needinfo?(l10n)
(Assignee)

Comment 17

2 years ago
Created attachment 8708981 [details] [diff] [review]
manifest_warnings_tests
Comment on attachment 8708979 [details] [diff] [review]
manifest_localize

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

I think you need to fix a bit of consistency in the messages, e.g. "same-origin as Document" vs "same origin as document".

I'd also expect this patch to go through standard code review.

::: dom/locales/en-US/chrome/dom/dom.properties
@@ +194,5 @@
> +ManifestScopeNotSameOrigin=Scope needs to be same-origin as Document.
> +ManifestStartURLOutsideScope=The start URL is outside the scope, so scope is invalid.
> +ManifestStartURLInvalid=Invalid start URL.
> +ManifestStartURLShouldBeSameOrigin=start_url must be same origin as document.
> +ManifestInvalidType=Expected the %1$S's %2$S member to be a %3$S.

This definitely need a comment explaining what these placeholders are (and an example with replacements).
(Assignee)

Comment 19

2 years ago
> I think you need to fix a bit of consistency in the messages, e.g. "same-origin as Document" vs "same origin as document".

I've just copied the already existing warnings from the source file. I can definitely change them, maybe in another bug specifically focused on improving them.

> This definitely need a comment explaining what these placeholders are (and an example with replacements).

I'll add a localization note. Should the example be in the localization note as well?
(In reply to Marco Castelluccio [:marco] from comment #19)
> I've just copied the already existing warnings from the source file. I can
> definitely change them, maybe in another bug specifically focused on
> improving them.

If you land this version, and then an updated one, you'll need to change string IDs. Without considering that some localizers will translate them twice. I strongly suggest to land only the final version of the text.
Flags: needinfo?(l10n)
(Assignee)

Comment 21

2 years ago
Created attachment 8708995 [details] [diff] [review]
manifest_localize

> If you land this version, and then an updated one, you'll need to change string IDs. Without considering that some localizers will translate them twice. I strongly suggest to land only the final version of the text.

OK, thank you for the info. I've updated the patch.
Attachment #8708979 - Attachment is obsolete: true
(Assignee)

Comment 22

2 years ago
Created attachment 8708996 [details] [diff] [review]
manifest_warnings_tests
Attachment #8708981 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8708995 - Flags: review?(l10n)
(Assignee)

Updated

2 years ago
Attachment #8708996 - Flags: review?(l10n)
(Assignee)

Comment 23

2 years ago
Created attachment 8709551 [details] [diff] [review]
manifest_localize

Updated patch after bug 1240490 and bug 1195018. I landed them first so we don't have changes in the strings to localize.
Attachment #8708995 - Attachment is obsolete: true
Attachment #8708995 - Flags: review?(l10n)
Attachment #8709551 - Flags: review?(l10n)
(Assignee)

Comment 24

2 years ago
Created attachment 8709552 [details] [diff] [review]
manifest_warnings_tests
Attachment #8708996 - Attachment is obsolete: true
Attachment #8708996 - Flags: review?(l10n)
Attachment #8709552 - Flags: review?(l10n)

Comment 25

2 years ago
Comment on attachment 8709551 [details] [diff] [review]
manifest_localize

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

Sorry for the lag.

The strings look OK to me. Flod and I debated a bit on whether start and scope are things to translate or not, but given that start is start_url in the manifest, I guess they're both fine to translate.

Not sure if there'd be value in enumerating the possible types in the manifest.

As for the rest of the changes, I'd prefer a reviewer familiar with the code to do the actual review on those, sorry. Thus making this an f+
Attachment #8709551 - Flags: review?(l10n) → feedback+

Comment 26

2 years ago
Comment on attachment 8709552 [details] [diff] [review]
manifest_warnings_tests

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

Not sure about this test, also better to ask someone with more experience on those tests.

I'm not fond of tests that only pass in en-US, but I'd be surprised if there weren't a bunch of the same kind right next to this.
Attachment #8709552 - Flags: review?(l10n)
(Assignee)

Updated

2 years ago
Attachment #8709552 - Flags: review?(amarchesini)
(Assignee)

Updated

2 years ago
Attachment #8709551 - Flags: review?(amarchesini)
(Assignee)

Comment 27

2 years ago
(In reply to Axel Hecht [:Pike] from comment #25)
> As for the rest of the changes, I'd prefer a reviewer familiar with the code
> to do the actual review on those, sorry. Thus making this an f+

Thank you for the review! Let's see if baku can review them.
Attachment #8709551 - Flags: review?(amarchesini) → review+
Attachment #8709552 - Flags: review?(amarchesini) → review+
(Assignee)

Comment 29

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/355b483c2d6c4b89c2cff578d392a20ce70ff2be
Bug 1086997 - Localize developer warnings issued by the manifest processor. r=baku

https://hg.mozilla.org/integration/mozilla-inbound/rev/95b66e9556e585b364389b32db895fefd7f2674a
Bug 1086997 - Test that the ManifestProcessor prints warnings using the ConsoleAPI. r=baku

Comment 30

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/355b483c2d6c
https://hg.mozilla.org/mozilla-central/rev/95b66e9556e5
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.