884336 - Remove trailing '/' from URLs in titlebar

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Peter Retzer (:pretzer)]

When the pref to show URLs in the titlebar is enabled, 'http://' and various subdomains like 'www.', 'm.', 'mobile.' are removed from the URL. It would be nice if any trailing '/' could be removed as well to be consistent with Desktop Firefox.

Comment 1

[:Margaret Leibovic]

Wes, I'm making you into a mentor here, hopefully you're okay with that :)

Comment 2

[Nicolas Carlo [:nickecarlo]]

Attachment: WIP

Is this something like what you're looking for?

PS: links with secure links retain their "https://" is that by design or do you want to also get rid of that?

Comment 3

[Nicolas Carlo [:nickecarlo]]

I can take this for now.

Comment 4

[Wesley Johnston (:wesj)]

Comment on attachment 764337 [details] [diff] [review]
WIP

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

Yes. And I think we want to do this even if the url is https. I've avoided removing "https" because desktop avoided removing it (I think because we had already spent time educating users that they should look for the "s" to know they were on a secure site).

Comment 5

[Nicolas Carlo [:nickecarlo]]

Attachment: Patch for bug 884336

1) Added an else if block that checks for whether the url starts with "https://" and then removes the trailing "/".

2) I nested an if statement within the else-if block which I think makes it easier for someone to remove https:// later if you decide to get rid of it from both the Desktop version and android (I personally think the lock icon conveys the message about it being secure but, to your point, a lot of people still look for the "s").

However, if you don't like the else-if with a nested if as it is right now then I can change it to:

    else if (url.startsWith("https://") && url.endsWith("/")) {
        // return url accordingly
    }

if you want.

Comment 6

[microrffr]

Can't we just do the same thing as desktop?

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/utilityOverlay.js#677

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 764525 [details] [diff] [review]
Patch for bug 884336

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

Looks good, but I'm going to go against what I've typically done and suggest we don't match desktop here.

::: mobile/android/base/util/StringUtils.java
@@ +51,5 @@
>  
>          if (url.startsWith("http://")) {
> +            if (url.endsWith("/")) {
> +                return url.substring(7, url.length()-1);
> +            }

I'm not really sure why Desktop only does this on http(s) or ftp urls. You have any idea? I think I'd rather we just do it all the time. Also, I'd like to get rid of all these nested returns. Can we do something like:

int start = 0;
int end = url.length();

if (http) start = 7;
if (ends with /) end--;

return url.substring(start, end);

We could use regex's like desktop but they're slightly more verbose in java and I don't think we gain that much from them.

Comment 8

[Nicolas Carlo [:nickecarlo]]

(In reply to Wesley Johnston (:wesj) from comment #7)
> Comment on attachment 764525 [details] [diff] [review]
> Patch for bug 884336
> 
> Review of attachment 764525 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but I'm going to go against what I've typically done and suggest
> we don't match desktop here.
> 
> ::: mobile/android/base/util/StringUtils.java
> @@ +51,5 @@
> >  
> >          if (url.startsWith("http://")) {
> > +            if (url.endsWith("/")) {
> > +                return url.substring(7, url.length()-1);
> > +            }
> 
> I'm not really sure why Desktop only does this on http(s) or ftp urls. You
> have any idea? I think I'd rather we just do it all the time. Also, I'd like
> to get rid of all these nested returns. Can we do something like:
> 
> int start = 0;
> int end = url.length();
> 
> if (http) start = 7;
> if (ends with /) end--;
> 
> return url.substring(start, end);
> 
> We could use regex's like desktop but they're slightly more verbose in java
> and I don't think we gain that much from them.

Thanks for the feedback and suggestions. I never know how much I'm allowed to add or take away from code here...probably a source of annoyance for you guys reviewing my patches :/ .

I'm not 100% sure why on desktop they only do it on http(s) and ftp. My guess would be these are the two (or three) protocols that deal with accessing directories? That's what I'm thinking off the top of my head.

Erm...I don't know much on regex's. I mean if you wanted to do regex's I could learn them and apply them here but it would take me a little while to get to it then. But if you did want to do regex's then I'm thinking you'd probably want to do them elsewhere in the class (like this method: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/util/StringUtils.java#58) too for consistency?

If it were up to me, I'd stick with the current style but its only because I don't know much about regex.

Comment 9

[Wesley Johnston (:wesj)]

Yeah. I'd just stick with the current thing, except simplified like I wrote in comment 7 (i.e. remove the trailing slash on all urls). This is only for display anyway.

Comment 10

[Nicolas Carlo [:nickecarlo]]

(In reply to Wesley Johnston (:wesj) from comment #9)
> Yeah. I'd just stick with the current thing, except simplified like I wrote
> in comment 7 (i.e. remove the trailing slash on all urls). This is only for
> display anyway.

Sorry been away from my "work" computer for a little bit. I'll do this ASAP. I'll also change the way this method: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/util/StringUtils.java#58 is so we get rid of all the return statements there too (except when url is null because we can't use substring for Strings that are null).

Comment 11

[Nicolas Carlo [:nickecarlo]]

Attachment: Patch for bug 884336

Removed nested return statements, as per the suggestions above, except for places where the String is null.

Comment 12

[Wesley Johnston (:wesj)]

Comment on attachment 766371 [details] [diff] [review]
Patch for bug 884336

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

Sorry for the delay. This looks good. Just needs code style fixed.

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

We should probably stick to our normal style. Braces on single line ifs. i.e.

if (url.startsWith("http://")) {
    start = 7;
}

same throughout.

Comment 13

[Nicolas Carlo [:nickecarlo]]

Attachment: Patch for bug 884336

Fixed the styling for relevant code.

Comment 14

[Wesley Johnston (:wesj)]

Comment on attachment 770224 [details] [diff] [review]
Patch for bug 884336

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

This applies on top of your old patch I think? Looks good though! Thanks!

Comment 15

[Nicolas Carlo [:nickecarlo]]

(In reply to Wesley Johnston (:wesj) from comment #14)
> Comment on attachment 770224 [details] [diff] [review]
> Patch for bug 884336
> 
> Review of attachment 770224 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This applies on top of your old patch I think? Looks good though! Thanks!

Thanks for the review :) .

The old patch is obsolete because I incorporated everything into this new patch. Right now I find that easier to handle. Hope its alright.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f568666066f0

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f568666066f0