Closed Bug 1086997 Opened 10 years ago Closed 9 years ago

Localize developer warnings issued by the manifest processor

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: marcosc, Assigned: marco)

References

(Blocks 1 open bug,
URL
)

Details

Attachments

(2 files, 4 obsolete files)

manifest_localize
(deleted), patch
baku
: review+
Pike
: feedback+
Details | Diff | Splinter Review
manifest_warnings_tests
(deleted), patch
baku
: review+
Details | Diff | Splinter Review
As the manifest file is being processed, the processor needs to emit errors to the developer console.
Blocks: 1079453
No longer blocks: 1079453
Depends on: 1079453
Need to also add i18n support as part of this.
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)
That largely depends on where the code is, and how it's supposed to be shown.
Flags: needinfo?(l10n)
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)
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)
(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)
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)
Blocks: webmanifest
(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)
(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)
(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)?
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)
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
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.
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: nobody → mcastelluccio
Status: NEW → ASSIGNED
Attached patch manifest_localize (obsolete) (deleted) — — Splinter Review
Who can review this?
Flags: needinfo?(l10n)
Attached patch manifest_warnings_tests (obsolete) (deleted) — — Splinter Review
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).
> 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)
Attached patch manifest_localize (obsolete) (deleted) — — Splinter Review
> 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
Attached patch manifest_warnings_tests (obsolete) (deleted) — — Splinter Review
Attachment #8708981 - Attachment is obsolete: true
Attachment #8708995 - Flags: review?(l10n)
Attachment #8708996 - Flags: review?(l10n)
Attached patch manifest_localize (deleted) — — Splinter Review
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)
Attached patch manifest_warnings_tests (deleted) — — Splinter Review
Attachment #8708996 - Attachment is obsolete: true
Attachment #8708996 - Flags: review?(l10n)
Attachment #8709552 - Flags: review?(l10n)
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 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)
Attachment #8709552 - Flags: review?(amarchesini)
Attachment #8709551 - Flags: review?(amarchesini)
(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+
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
https://hg.mozilla.org/mozilla-central/rev/355b483c2d6c https://hg.mozilla.org/mozilla-central/rev/95b66e9556e5
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: