Android URL formatting: Disable RTL URLs for Android 4.2 and below.

components/url_formatter/url_formatter.cc

Index: components/url_formatter/url_formatter.cc
diff --git a/components/url_formatter/url_formatter.cc b/components/url_formatter/url_formatter.cc
index d54b67913ce76d400907266963475bae6920cdd9..718049566937fdd987e6d8d2e43a6648566ac6d5 100644
--- a/components/url_formatter/url_formatter.cc
+++ b/components/url_formatter/url_formatter.cc
@@ -25,6 +25,11 @@
 #include "url/gurl.h"
 #include "url/third_party/mozilla/url_parse.h"
 
+#if defined(OS_ANDROID)
+#include "base/android/build_info.h"
+#include "base/i18n/rtl.h"
+#endif
+
 namespace url_formatter {
 
 namespace {
@@ -825,6 +830,14 @@ base::string16 FormatUrlWithAdjustments(
     AdjustAllComponentsButScheme(delta, new_parsed);
   }
 
+#if defined(OS_ANDROID)
+  // Android versions 4.2 and before have broken RTL support.
+  if (base::android::BuildInfo::GetInstance()->sdk_int() <=
+      base::android::SDK_VERSION_JELLY_BEAN_MR1) {
+    url_string = base::i18n::GetDisplayStringInLTRDirectionality(url_string);

[tommycli, 2017/04/25 00:06:21]

Hey, I'm not sure this is the correct thing to do.

The other thing I investigated was modifying escape.cc as you suggested. I
looked into modifying HasThreeByteBidiControlCharAtIndex. However, it didn't
seem like an exact fit.

The other thing I was considering was using
base::i18n::StringContainsStrongRTLChars within escape.cc, but I wasn't sure
that all RTL characters were three bytes (are they?)

At that point I decided to throw something up and seek some feedback before
proceeding further... thanks!

[Matt Giuca, 2017/04/26 00:01:05]

It's probably the case that they are all three bytes, but you shouldn't make
that assumption -- new characters are added all the time.

Yeah escape.cc is annoyingly structured around the number of bytes representing
a character, which invites bugs and is an irrelevant implementation detail when
deciding what characters to use.
I don't think this will work (have you tested on Android 4.2?) This just wraps
the string in LRE...PDF; we already do an equivalent thing and it *should* work,
but relies on the renderer to behave correctly. If RTL is broken on Android 4.2
as is claimed, then we can't expect this to work.

[Matt Giuca, 2017/04/26 05:57:09]

After we had a chat:

It looks like this *is* the correct way to fix it. Currently, we fix this by
using the ApiCompatibilityUtils.setTextDirection() API on Android, which
accomplishes the same thing but doesn't work before 4.2. So it's correct to
insert those characters instead.

Perhaps in the future we can remove the use of setTextDirection and just go with
this for all versions. But for now let's fix the bug and that can be a refactor.

As discussed, my concern is that this changes the text in the Omnibox that is
user-editable. If the user can copy the string and it includes the bidi mark
characters, that is bad, and we need to find a way to insert those characters
later in the pipeline (after the editable text field is updated, before
rendering).

The fact that this is the same function that removes "http://" indicates that it
is indeed changing the user-editable text string (not just the display string).

[tommycli, 2017/04/27 01:36:33]

You're right, this does mess up the copy and pasted content.

I have a new patch...

+  }
+#endif
+
   return url_string;
 }