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

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Keyboard, defect)

Description

[Axel Hecht [:Pike]]

Attachment: 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)

Comment 1

[Axel Hecht [:Pike]]

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.

Comment 2

[Ahmed Nefzaoui [:Nefzaoui]]

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

Comment 3

[Ahmed Nefzaoui [:Nefzaoui]]

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 ?

Comment 4

[Axel Hecht [:Pike]]

I'd suggest to file a new bug, let's keep this one to the js errors and their reporting.

Comment 5

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

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

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

This need koi+ so we could fix unusable cs.js (Czech layout, which is a shipping locale)

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Axel, do you want to submit a pull request for review or do you want someone from Taipei to take over?

Comment 8

[Axel Hecht [:Pike]]

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.

Comment 9

[Axel Hecht [:Pike]]

Attachment: 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.

Comment 10

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

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!

Comment 11

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

it would be great if file a follow up bug for changing js configuration file to json format.

Comment 12

[David Flanagan [:djf]]

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.

Comment 13

[Axel Hecht [:Pike]]

(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.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Travis CI is still red, will merge that for you after Travis CI turn red.

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

master: https://github.com/mozilla-b2g/gaia/commit/42a20aee4866b67d1adb83a96a826040ed5b2c48

Comment 16

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Uplifted to v1.2,
https://github.com/mozilla-b2g/gaia/commit/534d968a028254b1914a6e8664ca6319ec2ebd59