892942 - Remove www. and m. etc from URLs in Reader Mode

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Reader View, defect)

Description

[Nicolas Carlo [:nickecarlo]]

This is a follow up to bug 891446 (see comment 4).

Comment 1

[Nicolas Carlo [:nickecarlo]]

Taking this bug. Margaret, I'm CC'ing you because for now I think you are going to review patches here and I'm going to bother you if I hit a wall. :D

Comment 2

[Nicolas Carlo [:nickecarlo]]

Margaret,

Should we get rid of only www. or should we get rid of m. and mobile. and everything else too? Looks kinda weird without m or mobile if its a mobile site.

Comment 3

[:Margaret Leibovic]

(In reply to Nicolas Carlo [:nickecarlo] from comment #2)
> Margaret,
> 
> Should we get rid of only www. or should we get rid of m. and mobile. and
> everything else too? Looks kinda weird without m or mobile if its a mobile
> site.

This is Ian's call, but I think it would be fine to get rid of the m. or mobile. They don't really lend more information to the domain that's displayed, since by convention, using m. or mobile. as a subdomain just changes the layout of the text, not the content.

What do we do when we show URLs in the urlbar? Do we get rid of the m. or mobile. there?

Comment 4

[Wesley Johnston (:wesj)]

We do hide them in the urlbar, although its a bit contentious with some people I think. (see bug 898855)

Comment 5

[Ian Barlow (:ibarlow)]

(In reply to :Margaret Leibovic from comment #3)
> (In reply to Nicolas Carlo [:nickecarlo] from comment #2)
> > Margaret,
> > 
> > Should we get rid of only www. or should we get rid of m. and mobile. and
> > everything else too? Looks kinda weird without m or mobile if its a mobile
> > site.
> 
> This is Ian's call, but I think it would be fine to get rid of the m. or
> mobile. They don't really lend more information to the domain that's
> displayed, since by convention, using m. or mobile. as a subdomain just
> changes the layout of the text, not the content.

(In reply to :Margaret Leibovic from comment #3)
> (In reply to Nicolas Carlo [:nickecarlo] from comment #2)
> > Margaret,
> > 
> > Should we get rid of only www. or should we get rid of m. and mobile. and
> > everything else too? Looks kinda weird without m or mobile if its a mobile
> > site.
> 
> This is Ian's call, but I think it would be fine to get rid of the m. or
> mobile. They don't really lend more information to the domain that's
> displayed, since by convention, using m. or mobile. as a subdomain just
> changes the layout of the text, not the content.


Yeah, I think we can get rid of m. as well. In Reader Mode, the URL is more there to show the name of the source of the article you're reading, not so much the kind of website it's coming from.

Comment 6

[Nicolas Carlo [:nickecarlo]]

Attachment: bug-892942 patch for feedback

Added a method (_stripHost) to strip URLs of www., m., and mobile.

Margaret,

This patch works fine. I'm just wondering if I'm actually going about this the right way or not.

Comment 7

[:Margaret Leibovic]

Comment on attachment 783964 [details] [diff] [review]
bug-892942 patch for feedback

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

This looks good to me. It matches what we do in Java:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/util/StringUtils.java#69

Also asking wesj for feedback in case he knows if we already have a utility for this in JS.

::: mobile/android/chrome/content/aboutReader.js
@@ +579,5 @@
>  
>      this._doc.title = error;
>    },
>  
> +  _stripHost: function Reader_stripHost(host) {

It would be safer to add a check at the top of this function to make sure we have a valid string, similar to the null check in the Java implementation. Something like:

if (!host) {
  return host;
}

@@ +590,5 @@
> +    } else if (host.startsWith("mobile")) {
> +      start = 7;
> +    }
> +
> +    return host.substr(start);

I always need to look up the difference between substr and substring... it looks like the only difference is the second parameter, so I don't think it matters which one we use here, but it might be nice to use substring, since that's consistent with our Java implementation.

Comment 8

[:Margaret Leibovic]

Oh, also, if we go ahead with this approach, let's add a comment at the top of this function explaining that it's a JS version of Java's StringUtils.stripCommonSubdomains.

Comment 9

[Wesley Johnston (:wesj)]

Comment on attachment 783964 [details] [diff] [review]
bug-892942 patch for feedback

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

I'm not aware of any libraries we've got to help here. Desktop's implementation is at:

http://hg.mozilla.org/mozilla-central/annotate/05d3797276d3/toolkit/components/places/nsPlacesAutoComplete.js#l156

I actually think its less efficient than this, since indexOf will search the whole string for something, and startsWith SHOULD be able to bail quickly if things don't match. We should file a bug with them about that...

::: mobile/android/chrome/content/aboutReader.js
@@ +588,5 @@
> +    } else if (host.startsWith("m")) {
> +      start = 2;
> +    } else if (host.startsWith("mobile")) {
> +      start = 7;
> +    }

In JS, you're welcome to remove these single line braces.

Comment 10

[Nicolas Carlo [:nickecarlo]]

Margaret, Wes:

Thanks for the feedback.

Who should I ask to do the review when I put up the patch? Margaret or Wes? :/ Or both?

Comment 11

[:Margaret Leibovic]

(In reply to Nicolas Carlo [:nickecarlo] from comment #10)
> Margaret, Wes:
> 
> Thanks for the feedback.
> 
> Who should I ask to do the review when I put up the patch? Margaret or Wes?
> :/ Or both?

Either one of us would be happy to do the review! You can choose at random, or flag us both and see who gets to it first :)

Comment 12

[Nicolas Carlo [:nickecarlo]]

Attachment: bug-892842.patch

Sorry about the delay in posting this. I was away for my exams.

- Addressed the feedback by both Margaret and Wes:
  - Added the comment for the function
  - Added a check to make sure host is valid
  - Removed the brackets from one line if statements
  - Changed substr to substring.

Comment 13

[:Margaret Leibovic]

Comment on attachment 789720 [details] [diff] [review]
bug-892842.patch

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

Looks good, thanks!

Comment 14

[Nicolas Carlo [:nickecarlo]]

Thanks Margaret.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/4868b76a3aa9

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4868b76a3aa9

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 789720 [details] [diff] [review]
bug-892842.patch

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

::: mobile/android/chrome/content/aboutReader.js
@@ +590,5 @@
> +    if (host.startsWith("www"))
> +      start = 4;
> +    else if (host.startsWith("m"))
> +      start = 2;
> +    else if (host.startsWith("mobile"))

I'm confused. Is this ever hit? Surely we'd end up in the previous branch here.

Comment 18

[:Margaret Leibovic]

(In reply to :Ms2ger from comment #17)
> Comment on attachment 789720 [details] [diff] [review]
> bug-892842.patch
> 
> Review of attachment 789720 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/chrome/content/aboutReader.js
> @@ +590,5 @@
> > +    if (host.startsWith("www"))
> > +      start = 4;
> > +    else if (host.startsWith("m"))
> > +      start = 2;
> > +    else if (host.startsWith("mobile"))
> 
> I'm confused. Is this ever hit? Surely we'd end up in the previous branch
> here.

Oof, yeah, good catch. We include periods in the StringUtils imlementation:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/util/StringUtils.java#69

Nicolas, can you file a follow-up to fix this? You can flag me for review.

Comment 19

[Nicolas Carlo [:nickecarlo]]

Oops, sorry my bad. I overlooked the periods.

I can file a follow-up but I do actually get rid of periods in all these cases. For example, the one with www starts at the character at index 4 (which is the 5th character) so it skips the period as well as www.

Just wondering if it is necessary and if so, is there a reasoning behind it or just for consistency between JS and Java implementations?

:)

Comment 20

[:Margaret Leibovic]

(In reply to Nicolas Carlo [:nickecarlo] from comment #19)
> Oops, sorry my bad. I overlooked the periods.
> 
> I can file a follow-up but I do actually get rid of periods in all these
> cases. For example, the one with www starts at the character at index 4
> (which is the 5th character) so it skips the period as well as www.
> 
> Just wondering if it is necessary and if so, is there a reasoning behind it
> or just for consistency between JS and Java implementations?

Yes, because the period is omitted, the "m" case will match "mobile.foo.bar" and turn it into "bile.foo.bar". Even "margaretleibovic.com" would get munged.

Comment 21

[Nicolas Carlo [:nickecarlo]]

Oh oops. Don't want margaretleibovic.com turning into rgaretleibovic.com.

I'll do a follow up today and link to it here.

Sorry about this mess up.

Comment 22

[Nicolas Carlo [:nickecarlo]]

Filed the bug here: https://bugzilla.mozilla.org/show_bug.cgi?id=906798

Will post a patch in a couple of minutes.