913214 - Create StringUtils method to provide readable URL / title string

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Capella [:capella]]

Small code cleanup / followup from bug 912523 to avoid duplication of URL string methods

Comment 1

[Mark Capella [:capella]]

Attachment: bug913214 v1

For your reviewing pleasure ...

Comment 2

[Peter Retzer (:pretzer)]

Comment on attachment 800420 [details] [diff] [review]
bug913214 v1

You could convert these two single calls to the new combined method as well, right?
http://dxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserToolbar.java#l1084

Comment 3

[Mark Capella [:capella]]

ick ... forgot to grab that ...

Comment 4

[Mark Capella [:capella]]

Attachment: bug913214 v2

Incorporate code from comment #2 .... sorry for the bugzilla churn

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 800744 [details] [diff] [review]
bug913214 v2

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

I think maybe we could strip https in the urlbar, but I'd rather do it explicitly in a different bug where we can get some ux/sec review.

::: mobile/android/base/util/StringUtils.java
@@ +56,5 @@
>          int end = url.length();
>  
>          if (url.startsWith("http://")) {
>              start = 7;
> +        } else if (url.startsWith("https://")) {

Hmm... so we intentionally don't do this in the urlbar for security reasons (although I kinda want to do it anyway, and we already show a lock there...). I would suggest, if we want it gone in some places, but not others that you add a flag to this method and then overload it. i.e.

class UrlFlags {
  public static final int NONE = 0;
  public static final int STRIP_HTTPS = 1; // (or maybe DONT_STRIP_HTTPS?)
}

stripScheme(String url) {
  return stripScheme(url, UrlFlags.NONE);
}

stripScheme(String url, int flags) {
  // ...
  else if (url.startsWith("https://") && flags == UrlFlags.STRIP_HTTPS) {
  }
  // ...
}

Comment 6

[Mark Capella [:capella]]

Attachment: bug913214 v3

As requested, a little more tighter.

I agree it's hacky with the security conscious flag ... I'd like very much to follow-up and file that "explicitly different bug where we can get some ux/sec review".

Comment 7

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 801005 [details] [diff] [review]
bug913214 v3

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

This looks good to me. Though I would like to use enums.

Comment 8

[Mark Capella [:capella]]

Try try again ...
https://tbpl.mozilla.org/?tree=Try&rev=4730b21c5f48

Comment 9

[Mark Capella [:capella]]

Onward and upward!

https://hg.mozilla.org/integration/fx-team/rev/3057a627c682

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3057a627c682