Closed Bug 1157727 Opened 5 years ago Closed 4 years ago

Unicode 6.3 Bidi Support - Bracket Pairs

Categories

(Core :: Layout: Text and Fonts, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla45
blocking-b2g 2.5+
Tracking Status
firefox45 --- fixed

People

(Reporter: tedders1, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

(Whiteboard: [systemsfe])

Attachments

(6 files, 2 obsolete files)

This is part of Bug 922963.
Assignee: nobody → tclancy
Blocks: 922963
Whiteboard: [systemsfe]
Is there a ballpark estimate for when this would happen?  (Context: FxOS Email app seems to be impacted by the lack of this in bug 1177350 and if this is very far off, we might want to embed FSI/PDI characters to address/workaround the problem.  But if it's happening in time for gecko 44, maybe we'll be lazy and do nothing.)
Flags: needinfo?(tclancy)
> Is there a ballpark estimate for when this would happen?

End of September.
Flags: needinfo?(tclancy)
Ted, wanted to check back in with you. This is blocking bug 1177350, which is a v2.5 blocker so I want to see if you are still planning on to fix this one or if we should find another way around it. (options discussed in bug 1177350 comment 5)
Flags: needinfo?(tclancy)
Attached patch bug-1157727-part-2.patch (obsolete) — Splinter Review
Hi Dylan,

Yes, I am working on this, and I just attached my work-in-progress patches. They work for me in my ad hoc testing.

However, I still have to update the tests. Because this feature changes how brackets have traditionally been rendered, there are potentially dozens of tests that need to be updated.
Flags: needinfo?(tclancy)
Blocking a blocker
blocking-b2g: --- → 2.5+
Duplicate of this bug: 1177350
Priority: -- → P2
Attachment #8670945 - Attachment is obsolete: true
Attachment #8679648 - Flags: review?(smontagu)
Attachment #8670943 - Flags: review?(smontagu)
Attachment #8670943 - Flags: review?(smontagu) → review?(jfkthame)
Attachment #8679648 - Flags: review?(smontagu) → review?(jfkthame)
Comment on attachment 8670943 [details] [diff] [review]
bug-1157727-part-1.patch

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

::: intl/unicharutil/tools/genUnicodePropertyData.pl
@@ +554,5 @@
> +    s/#.*//;
> +    if (m/([0-9A-F]{4,6});\s*([0-9A-F]{4,6})\s+(.)/) {
> +        my $pbt = uc($3);
> +        warn "unknown Bidi Bracket type" unless exists $pairedBracketTypeCode{$pbt};
> +	$pairedBracketType[hex "0x$1"] = $pbt;

Looks like a stray <tab> has crept in here; please de-tab and use spaces instead.

(I notice there are a bunch of these already in the file; bonus points for a separate whitespace-only patch that fixes them up too.)

@@ +758,5 @@
>  struct nsCharProps1 {
>    unsigned char mMirrorOffsetIndex:5;
>    unsigned char mHangulType:3;
>    unsigned char mCombiningClass:8;
> +  unsigned char mPairedBracketType:2;

Adding this field to nsCharProps1 means the struct will no longer fit in two bytes, so we're increasing its size by 50% just for the sake of these two bits. There are unused bits in nsCharProps2 at the moment; did you consider putting this data there instead?

(It's not necessarily obvious which option will result in better packing, because in addition to the actual charProps structs, we need to consider that putting data into currently-unused bits will affect the sharing of blocks, and thus the size of the indexes that need to be created.)

::: intl/unicharutil/util/nsUnicodeProperties.cpp
@@ +185,5 @@
> +{
> +#if ENABLE_INTL_API
> +  return u_getBidiPairedBracket(aCh);
> +#else
> +  return GetPairedBracketType(aCh) ? GetMirroredChar(aCh) : aCh;

This assumes that every character that has a Bidi_Paired_Bracket (in BidiBrackets.txt) also has the same mapping for Bidi_Mirroring_Glyph (in BidiMirroring.txt), and therefore we don't need to store the paired-bracket mappings separately.

I expect that's probably true (and is likely to remain so in future versions), but I haven't noticed any explicit guarantee about it in Unicode. So it'd be good to validate this assumption when reading the data files in genUnicodePropertyData.pl.
(In reply to Ted Clancy [:tedders1] from comment #10)
> Green try run:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=381cbc7ef374

So no current tests are affected by the changes? Then we definitely need to be adding a set of testcases that explicitly depend on the modified behavior here.
(In reply to Andrew Sutherland [:asuth] from comment #1)
> Is there a ballpark estimate for when this would happen?  (Context: FxOS
> Email app seems to be impacted by the lack of this in bug 1177350 and if
> this is very far off, we might want to embed FSI/PDI characters to
> address/workaround the problem.

Note you can also use <bdi> blocks.
(In reply to Julien Wajsberg [:julienw] from comment #13)
> (In reply to Andrew Sutherland [:asuth] from comment #1)
> > Is there a ballpark estimate for when this would happen?  (Context: FxOS
> > Email app seems to be impacted by the lack of this in bug 1177350 and if
> > this is very far off, we might want to embed FSI/PDI characters to
> > address/workaround the problem.
> 
> Note you can also use <bdi> blocks.

Yes, for other apps, definitely be aware of <bdi> as a viable workaround (or perhaps even better alternative, I am no RTL expert!).  In the email case, we're talking about text/plain content (AKA just a text stream with no HTML markup supported).  Although the content-editable we use for composition can be made to use <bdi>, it would be lost on transit and draft save/reload and recipient mail user agents would need to either have this ICU functionality or hacks in place.  (Upconversion to text/html is not believed to be a viable option at this time for a variety of reasons including the SMS app's inability to handle text/html per bug 1159939.)
Comment on attachment 8679648 [details] [diff] [review]
bug-1157727-part-2.patch

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

I'm marking this feedback+ for now, as I think it's basically doing the right thing AFAICT, but I'd like to see a cleaned-up patch and a set of tests before going for a full r+.

The existing code in nsBidi isn't very Gecko-style-conformant (a result of its age and history, no doubt), but for the significant new chunks of code, at least, I think it'd help readability to update in a number of ways, as suggested below -- e.g. declare local variables at point of use, mark pointers as const when they're not intended to change (or replace them with C++ references, when they're really just being used as a local shorthand bound to a fixed element from an array, for example) use more Geckoish naming, etc.

I haven't commented on each individual place where these kind of updates could be applied, but I hope the smattering below give the general idea; it'd be good to have them done consistently throughout.

::: layout/base/nsBidi.cpp
@@ +55,5 @@
>  #define DIRPROP_FLAG_E(level) flagE[(level)&1]
>  #define DIRPROP_FLAG_O(level) flagO[(level)&1]
>  
> +#define NO_OVERRIDE(level)  ((level)&~NSBIDI_LEVEL_OVERRIDE)
> +#define DIR_FROM_STRONG(strong) ((strong)==L ? L : R)

I suggest turning this into a static inline function; then you can add an assertion that the given dirProp is in fact a strong type, as this wouldn't be valid for dirProp values in general. In an opt build (where the assertion is compiled out) it should end up equally efficient.

And to support that, I suggest we add something like

  #define IS_STRONG_TYPE(dirProp) ((dirProp) <= R || (dirProp) == AL)

and then use that wherever you need to check for strong types, as that seems to come up several times in the code.

@@ +605,5 @@
> +   one-after-last openings entries for pending opening brackets it
> +   contains.  The next openings entry to use is the one-after-last of the
> +   most deeply nested isoRun entry.
> +   isoRun entries also contain their current embedding level and the last
> +   encountered strong character, since these will be needed to resolve

Not actually "the last encountered strong character" but "the bidi class of the last encountered strong character", if I'm reading correctly.

@@ +610,5 @@
> +   the level of paired brackets.  */
> +
> +nsBracketData::nsBracketData(nsBidi *aBidi)
> +{
> +  pBidi = aBidi;

I don't think you really need to store this (see comments later); the code below can just refer to aBidi. Which can then be passed as a const nsBidi*, I think.

@@ +629,5 @@
> +}
> +
> +/* LRE, LRO, RLE, RLO, PDF */
> +void
> +nsBracketData::processBoundary(int32_t aLastCcPos,

Please spell out "Cc" as [Bidi]ControlChar, or add a comment; maybe it's just me but I tend to misinterpret "Cc" as "combining class" or "combining character" when I'm thinking about Unicode-related stuff!

@@ +633,5 @@
> +nsBracketData::processBoundary(int32_t aLastCcPos,
> +                               nsBidiLevel aContextLevel,
> +                               nsBidiLevel aEmbeddingLevel)
> +{
> +  IsoRun *pLastIsoRun = &isoRuns[isoRunLast];

Use a reference?
  IsoRun& lastIsoRun = mIsoRuns[mIsoRunLast];

@@ +634,5 @@
> +                               nsBidiLevel aContextLevel,
> +                               nsBidiLevel aEmbeddingLevel)
> +{
> +  IsoRun *pLastIsoRun = &isoRuns[isoRunLast];
> +  DirProp *dirProps = pBidi->mDirProps;

I'm suggesting (below) that you don't store a pointer to the nsBidi, in which case you'd pass in a |const DirProp* aDirProps| instead of this line.

@@ +652,5 @@
> +/* LRI or RLI */
> +void
> +nsBracketData::processLRI_RLI(nsBidiLevel aLevel)
> +{
> +  IsoRun *pLastIsoRun = &isoRuns[isoRunLast];

Rather than this...

@@ +655,5 @@
> +{
> +  IsoRun *pLastIsoRun = &isoRuns[isoRunLast];
> +  int16_t lastLimit;
> +  pLastIsoRun->lastBase = O_N;
> +  lastLimit = pLastIsoRun->limit;

...just say
  lastLimit = mIsoRuns[mIsoRunLast].limit;
here...

@@ +657,5 @@
> +  int16_t lastLimit;
> +  pLastIsoRun->lastBase = O_N;
> +  lastLimit = pLastIsoRun->limit;
> +  isoRunLast++;
> +  pLastIsoRun++;

...followed by
  IsoRun& lastIsoRun = mIsoRuns[mIsoRunLast];
here, and
  lastIsoRun.start = ...
below.

@@ +669,5 @@
> +/* PDI */
> +void
> +nsBracketData::processPDI()
> +{
> +  IsoRun *pLastIsoRun;

This local really isn't needed at all in this method.

@@ +826,5 @@
> +/* return true if success */
> +bool
> +nsBracketData::processChar(int32_t aPosition, char16_t aCh)
> +{
> +  IsoRun *pLastIsoRun = &isoRuns[isoRunLast];

We don't normally use a |p| prefix for pointers; that smacks of Hungarian conventions that aren't part of Mozilla style. I'd prefer this to be plain |lastIsoRun|; and once all the member variables (looking at |isoRuns| and |isoRunLast|) are suitably m-prefixed, this won't seem so confusing.

Applies to a number of places in this patch, I think.

Also, the * should be attached to the type, not the variable; and because (IIUC) this is just serving as a convenient shorthand (or optimization) for accessing a fixed item in the array, let's declare it const:

  IsoRun* const lastIsoRun = &mIsoRuns[mIsoRunLast];

@@ +853,5 @@
> +      pLastIsoRun->contextPos = aPosition;
> +      level = pBidi->mLevels[aPosition];
> +      if (level & NSBIDI_LEVEL_OVERRIDE) {    /* X4, X5 */
> +        uint16_t flag;
> +        int32_t i;

I'd prefer the local (scratch) variables to be declared at the point of use whenever possible, please; applies to a bunch of places in this patch.....

@@ +854,5 @@
> +      level = pBidi->mLevels[aPosition];
> +      if (level & NSBIDI_LEVEL_OVERRIDE) {    /* X4, X5 */
> +        uint16_t flag;
> +        int32_t i;
> +        newProp = level & 1;

Please use GET_LR_FROM_LEVEL to document this apparently-magic conversion from level to bidi property.

@@ +856,5 @@
> +        uint16_t flag;
> +        int32_t i;
> +        newProp = level & 1;
> +        pLastIsoRun->lastStrong = newProp;
> +        flag = DIRPROP_FLAG(newProp);

...e.g. you can declare |flag| here with
    uint16_t flag = ....

@@ +857,5 @@
> +        int32_t i;
> +        newProp = level & 1;
> +        pLastIsoRun->lastStrong = newProp;
> +        flag = DIRPROP_FLAG(newProp);
> +        for (i = pLastIsoRun->start; i < idx; i++) {

...and |i| only exists for use in this loop, so
    for (int32_t i = pLastIsoRun->start; ...

There are other similar cases above, too.

@@ +870,5 @@
> +    }
> +    /* We get here only if the ON character is not a matching closing
> +       bracket or it is a case of N0d */
> +    /* Now see if it is an opening bracket */
> +    if(aCh) {

nit: space before (

@@ +895,5 @@
> +    }
> +  }
> +  level = pBidi->mLevels[aPosition];
> +  if (level & NSBIDI_LEVEL_OVERRIDE) {    /* X4, X5 */
> +    newProp = level & 1;

GET_LR_FROM_LEVEL(level)

@@ +903,5 @@
> +    pLastIsoRun->lastBase = newProp;
> +    pLastIsoRun->lastStrong = newProp;
> +    pLastIsoRun->contextDir = newProp;
> +    pLastIsoRun->contextPos = aPosition;
> +  } else if (dirProp <= R || dirProp == AL) {

IS_STRONG_TYPE(dirProp)

@@ +943,5 @@
> +  } else {
> +    newProp = dirProp;
> +    pLastIsoRun->lastBase = dirProp;
> +  }
> +  if (newProp <= R || newProp == AL) {

IS_STRONG_TYPE(newProp)

@@ +1030,5 @@
>      /* no embeddings, set all levels to the paragraph level */
>      for(i=0; i<length; ++i) {
>        levels[i]=level;
> +      dirProp = dirProps[i];
> +      if (dirProp == BN) {

Don't bother with the assignment to |dirProp| here, just test |dirProps[i]| directly in the |if|.

@@ +1216,4 @@
>            }
> +          previousLevel = embeddingLevel;
> +          levels[i] = embeddingLevel;
> +          if(!bracketData.processChar(i, aText[i])) {

space before ( in "if ("

@@ +1216,5 @@
>            }
> +          previousLevel = embeddingLevel;
> +          levels[i] = embeddingLevel;
> +          if(!bracketData.processChar(i, aText[i])) {
> +            // TODO: For some reason, we don't indicate an error, but set the direction to -1? Huh?

Why -1?

(AFAICS, the only case where bracketData.ProcessChar can fail (return false) is if it fails to allocate memory in AddOpening, right? I would have thought we'd default to returning NSBIDI_LTR as the simplest fallback in an OOM situation -- as you do at the other TODO note above. Why the difference?)

::: layout/base/nsBidi.h
@@ +244,5 @@
>  /* number of isolate entries allocated initially without malloc */
>  #define SIMPLE_ISOLATES_SIZE 5
>  
> +/* number of isolate run entries for paired brackets allocated initially without malloc */
> +#define SIMPLE_OPENINGS_COUNT   20

Please run some tests (if you haven't already done so) with this #defined really small (e.g. make it 1) so as to exercise the dynamic-allocation codepath as well.

@@ +397,5 @@
> +  int32_t contextPos;                 /* position of last strong char found before opening */
> +  uint16_t flags;                     /* bits for L or R/AL found within the pair */
> +  DirProp contextDir;                 /* L or R according to last strong char before opening */
> +  uint8_t filler;                     /* to complete a nice multiple of 4 chars */
> +};

Here, and in the other structs below, please use the "m" prefix for members, as per style guidelines. Reading the .cpp code, I find myself often distracted by seeing things that look like local variables, but are in fact struct members; the prefix would make that clearer.

@@ +411,5 @@
> +};
> +
> +class nsBidi;
> +
> +class nsBracketData {

No need for the "ns" prefix on this, I think.

@@ +413,5 @@
> +class nsBidi;
> +
> +class nsBracketData {
> +private:
> +  nsBidi* pBidi;

mBidi, not pBidi. Though if the memory allocation for Openings is tidied up (as per next comment), I think you probably don't need to keep a reference back to the nsBidi here at all. Aside from the openings array, I think the only other thing it's used for (after the constructor) is to access the DirProps and Levels arrays in a few methods; those could just as well be passed as parameters to the methods that need them.

@@ +415,5 @@
> +class nsBracketData {
> +private:
> +  nsBidi* pBidi;
> +  /* array of opening entries which should be enough in most cases; no malloc() */
> +  Opening  simpleOpenings[SIMPLE_OPENINGS_COUNT];

It seems odd to have this declared in the (ns)BracketData, when the nsBidi is responsible for ownership of its mOpeningsMemory. ISTM it'd be more logical to put the preallocated array in the nsBidi as well.

(Or alternatively -- perhaps better -- manage the openings array entirely within the BracketData, and don't ask the parent nsBidi to keep track of it at all.)

@@ +433,5 @@
> +  void processPDI();
> +  bool addOpening(char16_t aMatch, int32_t aPosition);
> +  void fixN0c(int32_t aOpeningIndex, int32_t aNewPropPosition, DirProp aNewProp);
> +  DirProp processClosing(int32_t aOpenIdx, int32_t aPosition);
> +  bool processChar(int32_t aPosition, char16_t aCh);

Capitalize method names.
Attachment #8679648 - Flags: review?(jfkthame) → feedback+
For context, this seems to be the state of this functionality in other platforms:
- WebKit: apparently not yet implemented, tracked on https://bugs.webkit.org/show_bug.cgi?id=130823 (Note that the bug has seen no discussion, so it might need to be duped, but at least some cursory searching of their bug tracker seems to show no other public bugs that are open or fixed.)
- IE: has it since IE10 per the above webkit bug at https://bugs.webkit.org/show_bug.cgi?id=130823
- Chromium/Google Chrome: not yet implemented, tracked on https://code.google.com/p/chromium/issues/detail?id=302469 with a slightly more lively tracker bug at https://code.google.com/p/chromium/issues/detail?id=242238
Ted, are you actively working further on this at the moment, or would it be helpful for me to adopt the patches here and try to get them finished up?
Flags: needinfo?(ted.clancy)
Hey Jonathan. If you could do that, that would be great. I'm travelling at the moment and probably won't get around to this for a few more weeks.
Flags: needinfo?(ted.clancy)
Comment on attachment 8670943 [details] [diff] [review]
bug-1157727-part-1.patch

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

Just noting a couple more fixes needed here; I've incorporated these locally.

::: intl/unicharutil/tools/genUnicodePropertyData.pl
@@ +551,5 @@
> +    last if /Date:/;
> +}
> +while (<FH>) {
> +    s/#.*//;
> +    if (m/([0-9A-F]{4,6});\s*([0-9A-F]{4,6})\s+(.)/) {

This doesn't match.... it needs to handle the semicolon after the second Unicode value, before the space(s) and bracket type.

@@ +554,5 @@
> +    s/#.*//;
> +    if (m/([0-9A-F]{4,6});\s*([0-9A-F]{4,6})\s+(.)/) {
> +        my $pbt = uc($3);
> +        warn "unknown Bidi Bracket type" unless exists $pairedBracketTypeCode{$pbt};
> +	$pairedBracketType[hex "0x$1"] = $pbt;

This should be

    $pairedBracketType[hex "0x$1"] = $pairedBracketTypeCode{$pbt};

otherwise they all end up with zero in the table. :(
These tests are examples from the Unicode spec, with various directionality applied; a bunch of them currently fail because we don't implement the bracket pairing.
Attachment #8688904 - Flags: review?(smontagu)
Assignee: ted.clancy → jfkthame
Status: NEW → ASSIGNED
To reduce clutter in nsBidi.cpp and make it easier to focus on relevant code, I'd like to rip out a bunch of #ifdef'd stuff that we never use.
Attachment #8688905 - Flags: review?(smontagu)
This is Ted's patch, with my review comments and bug-fixes addressed. By putting the bracket data into the unused bits in nsCharProps2, we avoid any increase in data size here.
Attachment #8670943 - Flags: review?(jfkthame)
Again, this is mostly Ted's patch, updated as per my review comments. I also changed the implementation of the canonical-equivalent support for the right-angle brackets, as it seems simpler this way. One issue we don't yet handle is that in Unicode 8, there's a hard limit on bracket nesting (see http://unicode.org/reports/tr9/#BD16), so we should stop processing brackets within an isolating run if we hit that; but currently, the code here doesn't impose such a limit.
Wait, are we rolling our own implementation of the bracket pairs support? I thought the idea was to copy-paste code from ICU's ubidi* source files into nsBidi (which was originally based on an earlier version of ubidi), as Ted did for isolation in bug 1157726
It looks to me like Ted's patch (above) is essentially porting the code from ubidi.c into our nsBidi -- wrapping static C functions into the class, etc., but it's the same logic.
Comment on attachment 8688905 [details] [diff] [review]
Part 0: Preliminary cleanup, remove a bunch of #ifdef'd dead code from nsBidi.cpp

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

I suppose. We have moved some stuff out of the #ifdef in the past when we needed it, but I doubt if we are going to do that again.
Attachment #8688905 - Flags: review?(smontagu) → review+
(In reply to Simon Montagu :smontagu from comment #29)
> I suppose. We have moved some stuff out of the #ifdef in the past when we
> needed it, but I doubt if we are going to do that again.

Yeah, I don't see us making those kind of changes (e.g. starting to use a whole new aspect of bidi functionality), or at least not separately from a move to direct use of ICU's bidi API.

Moreover, given that we don't compile (let alone test) that stuff anywhere, there's a decent chance it's already broken and we just don't know it.
Comment on attachment 8688904 [details] [diff] [review]
Reftests for bidi bracket pairing, based on UAX #9 examples

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

This seems a bit overkill to me: is having the same test with dir, FSI, RLI and LRI actually testing anything different each time (apart from the effect of the isolate codes themselves, which we have other tests for)?

I would rather just have the same text in one each of LTR and RTL paragraphs, and maybe add a version of test 1 with the order of RTL and LTR text within the line swapped, as you already did with test 2, and similarly a version of test 5 where the brackets enclose RTL text.
Fair enough. I was kinda wondering about that, too. Actually, though, what I think I'd also like to do is add some tests with content *outside* the isolates, and check that brackets outside and inside don't interact. I'll do an updated tests patch...
A more sensible collection of tests. (Note that a bunch of the variants pass already without the bracket-pair patch, so all we're really checking with those cases is that the pairing doesn't disrupt things that were working OK "naturally" under the older algorithm.)
Attachment #8690797 - Flags: review?(smontagu)
Attachment #8688904 - Attachment is obsolete: true
Attachment #8688904 - Flags: review?(smontagu)
Comment on attachment 8690797 [details] [diff] [review]
Reftests for bidi bracket pairing, based on UAX #9 examples

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

::: layout/reftests/bidi/brackets-3b-ltr.html
@@ +12,5 @@
> +  http://unicode.org/reports/tr9/#N0, Example 3, LTR and RTL exchanged:
> +    arabic BOOK(S)
> +-->
> +<!-- LTR -->
> +<div>arabic &#x643;&#x650;&#x62a;&#x64e;&#x627;&#x628;(&#x643;&#x64f;&#x62a;&#x64f;&#x628;)</div>

Thumbs up for using the actual Arabic words!
Attachment #8690797 - Flags: review?(smontagu) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/bbd26d9d2c3eb4949a86b84d58c668dc0da3a07b
Bug 1157727 - Reftests for bidi bracket pairing, based on UAX #9 examples. r=smontagu

https://hg.mozilla.org/integration/mozilla-inbound/rev/00c2df3df22fb93879a65d623949af10258c6b95
Bug 1157727 - Part 0: Preliminary cleanup, remove a bunch of #ifdef'd dead code from nsBidi.cpp. r=smontagu

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb72e312f13742cc5a3234cb0e45b5a8332c9ce3
Bug 1157727 - Part 1: Add paired bracket type property to our character data (patch originally by :tedders1, updated by :jfkthame). r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/21730adb78b62b34cd4ed45eb08b9841bb1524ef
Bug 1157727 - Part 2: Update bidi algorithm for bracket matching (patch originally by :tedders1, updated by :jfkthame). r=jfkthame
sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=17816304&repo=mozilla-inbound
Flags: needinfo?(jfkthame)
Sigh.... the reftest failures were all related to (platform- and font-dependent) pixel-rounding issues in the reference files, especially with the Arabic text when direction is forced to LTR and the char sequence reversed.

We can work around the Linux and Windows failures by instead leaving the Arabic text in its natural RTL order, and using unicode-bidi:isolate on the RTL runs to separate them from the surrounding LTR text; and for the Android failures I'm just going to annotate them as fuzzy.
Flags: needinfo?(jfkthame)
https://hg.mozilla.org/integration/mozilla-inbound/rev/a34fb1388c8d4f417ccba8fdea7f685a4807d3fc
Bug 1157727 - Reftests for bidi bracket pairing, based on UAX #9 examples. r=smontagu

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b36055ffd7a53d5f091d740060390fecc3e84e8
Bug 1157727 - Part 0: Preliminary cleanup, remove a bunch of #ifdef'd dead code from nsBidi.cpp. r=smontagu

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdc03cf18366ebca12495b86d4be4681bcb3416a
Bug 1157727 - Part 1: Add paired bracket type property to our character data (patch originally by :tedders1, updated by :jfkthame). r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/aeafc4cca44784d13cfc2006371848794186dbd8
Bug 1157727 - Part 2: Update bidi algorithm for bracket matching (patch originally by :tedders1, updated by :jfkthame). r=jfkthame
Depends on: 1231175
Depends on: CVE-2016-2838
You need to log in before you can comment on or make changes to this bug.