New issue
Advanced search Search tips

Issue 718670 link

Starred by 0 users

Issue metadata

Status: Available
Owner: ----
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Bug



Sign in to add a comment

hterm: invalid character-map-overrides settings are not rejected

Project Member Reported by vapier@chromium.org, May 5 2017

Issue description

if you enter a value like {"":""} into character-map-overrides, then hterm fails to load properly, and the JS console has errors like:
  Error in event handler for storage.onChanged: TypeError: Cannot read property 'glmap' of undefined

we should validate this more before accepting it.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/apps/libapps/+/bc7089f9cfcfcda129c367ce8a4b50e1cb65d496

commit bc7089f9cfcfcda129c367ce8a4b50e1cb65d496
Author: Mike Frysinger <vapier@chromium.org>
Date: Wed Jun 28 00:03:47 2017

hterm: character maps: make override handling cumulative

The current override logic ends up clobbering the entire map.  So if
someone wants to tweak just one mapping for an existing character map,
they have to duplicate the entire thing.

Let's change the override logic so that it's cumulative: if the user
wants to change just "a"->"A", that now happens without the rest of
the character map being thrown away.

This splits out the reset logic into more reasonable behavior: it
actually resets the object state back to a pristine state.  This also
breaks the API, but we haven't been expecting anyone to use this
directly, so hopefully no one even notices.

This also makes the character overriding work in a few more scenarios.
Notably, when the map is null like the US map.

BUG=chromium:718670

Change-Id: I8771b3f78ddb7395ec0d7cdb8f194abb35b6682e
Reviewed-on: https://chromium-review.googlesource.com/539176
Reviewed-by: Brandon Gilmore <varz@google.com>
Tested-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/bc7089f9cfcfcda129c367ce8a4b50e1cb65d496/hterm/js/hterm_vt_character_map_tests.js
[modify] https://crrev.com/bc7089f9cfcfcda129c367ce8a4b50e1cb65d496/hterm/js/hterm_terminal.js
[modify] https://crrev.com/bc7089f9cfcfcda129c367ce8a4b50e1cb65d496/hterm/js/hterm_vt_character_map.js

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/apps/libapps/+/095d406b72154ef69acb229bb347b6cdfd91c9e3

commit 095d406b72154ef69acb229bb347b6cdfd91c9e3
Author: Mike Frysinger <vapier@chromium.org>
Date: Wed Jun 28 00:04:27 2017

hterm: character maps: add a container for sets of maps

The current mapping logic is a global variable, so whenever a single
terminal instance modifies its own set of maps, it ends up changing
the mappings for all terminal instances.  Lets create a new structure
instead to handle these mappings so each terminal can modify things
w/out interfering with other ones.

This also allows us to push down the map overriding from the terminal
to the character maps.

BUG=chromium:718670

Change-Id: Id55f477e3216d45809eff250f8c64339cfab2bf2
Reviewed-on: https://chromium-review.googlesource.com/539177
Reviewed-by: Brandon Gilmore <varz@google.com>
Tested-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/095d406b72154ef69acb229bb347b6cdfd91c9e3/hterm/js/hterm_vt_character_map_tests.js
[modify] https://crrev.com/095d406b72154ef69acb229bb347b6cdfd91c9e3/hterm/js/hterm_terminal.js
[modify] https://crrev.com/095d406b72154ef69acb229bb347b6cdfd91c9e3/hterm/js/hterm_vt_character_map.js
[modify] https://crrev.com/095d406b72154ef69acb229bb347b6cdfd91c9e3/hterm/js/hterm_vt.js

Comment 3 Deleted

Comment 4 Deleted

Sign in to add a comment