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

components/url_formatter/url_formatter.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/url_formatter/url_formatter.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_offset_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_local_storage.h"
 #include "third_party/icu/source/common/unicode/schriter.h"
 #include "third_party/icu/source/common/unicode/uidna.h"
 #include "third_party/icu/source/common/unicode/uniset.h"
 #include "third_party/icu/source/common/unicode/uscript.h"
 #include "third_party/icu/source/common/unicode/uvernum.h"
 #include "third_party/icu/source/i18n/unicode/regex.h"
 #include "third_party/icu/source/i18n/unicode/uspoof.h"
 #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 {
 
 base::string16 IDNToUnicodeWithAdjustments(
     base::StringPiece host,
     base::OffsetAdjuster::Adjustments* adjustments);
 bool IDNToUnicodeOneComponent(const base::char16* comp,
                               size_t comp_len,
                               bool is_tld_ascii,
@@ skipping 780 matching lines @@
     if (prefix_end)
       *prefix_end -= kHTTPSize;
 
     // Adjust new_parsed.
     DCHECK(new_parsed->scheme.is_valid());
     int delta = -(new_parsed->scheme.len + 3);  // +3 for ://.
     new_parsed->scheme.reset();
     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;
 }
 
 bool CanStripTrailingSlash(const GURL& url) {
   // Omit the path only for standard, non-file URLs with nothing but "/" after
   // the hostname.
   return url.IsStandard() && !url.SchemeIsFile() && !url.SchemeIsFileSystem() &&
          !url.has_query() && !url.has_ref() && url.path_piece() == "/";
 }
 
@@ skipping 12 matching lines @@
   return base::StartsWith(text, www, base::CompareCase::SENSITIVE)
       ? text.substr(www.length()) : text;
 }
 
 base::string16 StripWWWFromHost(const GURL& url) {
   DCHECK(url.is_valid());
   return StripWWW(base::ASCIIToUTF16(url.host_piece()));
 }
 
 }  // namespace url_formatter