[B2g][Keyboard] some keyboard layouts are broken with js errors

RESOLVED FIXED

Status

--
blocker
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Pike, Assigned: Pike)

Tracking

unspecified

Firefox Tracking Flags

(blocking-b2g:koi+, b2g-v1.2 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 831392 [details] [diff] [review]
more reporting, and fix js in keyboard layouts

In bug 884752, we split out a few keyboards into separate files.

The js in those is broken, which makes them completely unusable.

Affected are cs, mk, jp-kanji.

I have a fix in hand, and some changes to the build logic to actually report errors.

I'm starting with a feedback request on Yuren, because I have no idea how we'd actually should do error reporting in our js commands here.

The dump()s feel a tad dirty to me.

Requesting koi as the broken keyboards affect 1.2 and locales we intend to ship on 1.2 (cs, mk)
Attachment #831392 - Flags: feedback?(yurenju.mozilla)
(Assignee)

Comment 1

5 years ago
I'm playing around with 

define run-js-command
	echo "run-js-command $1";
	$(XULRUNNERSDK) $(XPCSHELLSDK) \
		-e "const GAIA_BUILD_DIR='$(BUILDDIR)'" \
		-f build/xpcshell-commonjs.js -e "try{require('$(strip $1)').execute()} catch(e){dump(e+'\n'+e.fileName+':'+e.lineNumber+'\n');throw e;}"
endef

in Makefile. I like that in principle, but

  let layouts = layoutNames.map(function(layoutName) {
    let layoutFile = utils.getFile(layoutSrc.path, layoutName + '.js');

    try {
      return getLayoutDetails(layoutName, layoutFile);
    }
    catch(e) {
      throw new Error('Unknown keyboard layout "' + layoutName +
                      '" in GAIA_KEYBOARD_LAYOUTS');
    }
  });

in keyboard-config.js hides much of the error detail in our case then.
Also I believe last time I built Gaia with AR layout (2 days ago).. Arabic seemed broken to me
Will have to investigate further though starting from this line it doesn't seem okay..
https://github.com/mozilla-b2g/gaia/blob/master/keyboard/layouts/ar.js#L28
Yes I confirm there is a bug in AR layout, Only second layout appears, but when tapping it sends first layout's characters. Should I file a new bug or just put a PR here ?
Flags: needinfo?(l10n)
(Assignee)

Comment 4

5 years ago
I'd suggest to file a new bug, let's keep this one to the js errors and their reporting.
Flags: needinfo?(l10n)
Comment on attachment 831392 [details] [diff] [review]
more reporting, and fix js in keyboard layouts

looks good, and you can use utils.log to dump message.
https://github.com/mozilla-b2g/gaia/blob/master/build/utils.js#L422-L436
Attachment #831392 - Flags: feedback?(yurenju.mozilla) → feedback+
This need koi+ so we could fix unusable cs.js (Czech layout, which is a shipping locale)
Axel, do you want to submit a pull request for review or do you want someone from Taipei to take over?
Assignee: nobody → l10n
blocking-b2g: koi? → koi+
Flags: needinfo?(l10n)
(Assignee)

Comment 8

5 years ago
I'm working on a PR now. Also, master is much better in terms of Makefile's run-js-command. Sadly the goodness is distributed over a host of different patches that didn't get uplifted to 1.2. I wonder if it'd make sense to create a patch just for that fragment so that partners have a more reliable way to debug local issues when doing 1.2 builds with custom keyboards.
Flags: needinfo?(l10n)
(Assignee)

Comment 9

5 years ago
Created attachment 832182 [details] [review]
better error propagation, and fix the two remaining js errors

So, this turned out to get a tad bigger than I expected, and a tad smaller.

The biggest change I made was to replace eval() with loadSubScript(). That's nice for all the reasons eval is not, but more importantly, it reports file and line number for problems in the layout js files.

The rest of the changes in keyboard-config.js just makes sure we're not throwing the stack information away.

The layout file fixes are a 'cz' vs 'cs', the latter is the language code and wins. And we don't have a dictionary file for Macedonian, so removing that.
Attachment #831392 - Attachment is obsolete: true
Attachment #832182 - Flags: review?(rlu)
Attachment #832182 - Flags: review?(dflanagan)
Comment on attachment 832182 [details] [review]
better error propagation, and fix the two remaining js errors

Flagging Yuren for build script part.

I'm fine with the changes in keyboard/layouts/...
Thanks!
Attachment #832182 - Flags: review?(yurenju.mozilla)
Attachment #832182 - Flags: review?(rlu)
Attachment #832182 - Flags: review+
Whiteboard: [3rd-party-keyboard]
Whiteboard: [3rd-party-keyboard]
Attachment #832182 - Flags: review?(yurenju.mozilla) → review+
it would be great if file a follow up bug for changing js configuration file to json format.
Comment on attachment 832182 [details] [review]
better error propagation, and fix the two remaining js errors

Thanks Axel!  Sorry for the messed up layout file.

I never knew that you could edit the message of an error object and rethrow it!

Yuren: as part of my keyboard refactoring, I'm changing the layout format and will use pure JSON. So it is not worth modifying this current format to be pure JSON.
Attachment #832182 - Flags: review?(dflanagan) → review+
(Assignee)

Comment 13

5 years ago
(In reply to David Flanagan [:djf] from comment #12)
> Comment on attachment 832182 [details] [review]
> better error propagation, and fix the two remaining js errors
> 
> Thanks Axel!  Sorry for the messed up layout file.
> 
> I never knew that you could edit the message of an error object and rethrow
> it!

I learned by trying, with this patch in fact :-)

Thanks for the reviews, I'll need a merging buddy.
Keywords: checkin-needed
Travis CI is still red, will merge that for you after Travis CI turn red.
master: https://github.com/mozilla-b2g/gaia/commit/42a20aee4866b67d1adb83a96a826040ed5b2c48
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.