1325488 - Deduplicate subdomain removal in IconGenerator

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Andrzej Hunt :ahunt]

IconGenerator has code to strip common subdomains from domains (this is used to obtain a representative character for a given domain, for our generated favicons):
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/icons/loader/IconGenerator.java#160

We have a very similar method in StringUtils, which we probably want to use instead:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/StringUtils.java#122

Comment 1

[Andrzej Hunt :ahunt]

Both of these implementations are subtly different

IconGenerator iterates over all prefixes, and removes them one by one, so I expect the following would happen:
www.mobile.foo.com -> foo.com
mobile.www.foo.com -> www.foo.com (unexpected!)

StringUtils only removes the first "common" subdomain, so:
www.mobile.foo.com -> mobile.foo.com
mobile.www.foo.com -> www.foo.com

I'm not sure if these combinations happen in real life, but I feel like I've seen a www.mobile.foo.com at least once. (Feel free to ignore if not.)

Comment 2

[Brian Chen]

Hello Andrzej, I am new to this. Can I take this task? If so, do I just change the subdomain removal to use the one in StringUtil? Thanks!

Comment 3

[Brian Chen]

Attachment: Change the subdomain removal method in IconGenerator

Is this how you do this? I wasn't sure who to pick as my reviewer so it suggested you.

Comment 4

[Brian Chen]

Attachment: Changes subdomain removal method in IconGenerator

Sorry about that last patch

Comment 5

[Brian Chen]

Comment on attachment 8821376 [details] [diff] [review]
Changes subdomain removal method in IconGenerator

># HG changeset patch
># User Brian Chen <[e-mail]>
># Date 1482453466 18000
>#      Thu Dec 22 19:37:46 2016 -0500
># Node ID a773aa50e72812ad6d2c4ba1d8b195dc7a3e1d99
># Parent  f179934df0c1bab590c558485d419c7910e41325
>Change Subdomain Removal
>
>diff --git a/build.gradle b/build.gradle
>--- a/build.gradle
>+++ b/build.gradle
>@@ -33,7 +33,7 @@ buildscript {
>     }
> 
>     dependencies {
>-        classpath 'com.android.tools.build:gradle:2.1.3'
>+        classpath 'com.android.tools.build:gradle:2.2.3'
>         classpath('com.stanfy.spoon:spoon-gradle-plugin:1.0.4') {
>             // Without these, we get errors linting.
>             exclude module: 'guava'
>diff --git a/mobile/android/base/java/org/mozilla/gecko/icons/loader/IconGenerator.java b/mobile/android/base/java/org/mozilla/gecko/icons/loader/IconGenerator.java
>--- a/mobile/android/base/java/org/mozilla/gecko/icons/loader/IconGenerator.java
>+++ b/mobile/android/base/java/org/mozilla/gecko/icons/loader/IconGenerator.java
>@@ -21,6 +21,7 @@ import android.util.TypedValue;
> import org.mozilla.gecko.R;
> import org.mozilla.gecko.icons.IconRequest;
> import org.mozilla.gecko.icons.IconResponse;
>+import org.mozilla.gecko.util.StringUtils;
> 
> /**
>  * This loader will generate an icon in case no icon could be loaded. In order to do so this needs
>@@ -156,12 +157,8 @@ public class IconGenerator implements Ic
>             return "?";
>         }
> 
>-        // Strip common prefixes that we do not want to use to determine the representative character
>-        for (String prefix : COMMON_PREFIXES) {
>-            if (snippet.startsWith(prefix)) {
>-                snippet = snippet.substring(prefix.length());
>-            }
>-        }
>+        // Strip common prefixes that we do not want to use to determine the representative characterS
>+        snipper = StringUtils.stripCommonSubdomains(snippet);
> 
>         return snippet;
>     }

Comment 6

[Brian Chen]

Attachment: Edit error in code

Comment 7

[Andrzej Hunt :ahunt]

Looks almost good - it seems like the error-fix created a separate commit - could you combine them into one patch please?

(It's easiest use the |hg histedit| extension to "fold" the fix patch into the first commit.)

Comment 8

[Andrzej Hunt :ahunt]

Comment on attachment 8821376 [details] [diff] [review]
Changes subdomain removal method in IconGenerator

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

Looks good, just some minor nits!

JFYI: if you have an existing commit locally (which you can using |hg log|), and then want to modify that commit, you can use |hg commit --amend| to add changes to that commit (whereas |hg commit| by itself produces a new separate commit).

::: build.gradle
@@ +33,4 @@
>      }
>  
>      dependencies {
> +        classpath 'com.android.tools.build:gradle:2.2.3'

We shouldn't update the gradle plugin here, we have a separate bug for that (Bug 1305494).

(I'm guessing Android Studio offered to update the gradle plugin for you, but we want to do that independetly of bug fixes.)

::: mobile/android/base/java/org/mozilla/gecko/icons/loader/IconGenerator.java
@@ -156,5 @@
>              return "?";
>          }
>  
> -        // Strip common prefixes that we do not want to use to determine the representative character
> -        for (String prefix : COMMON_PREFIXES) {

We should also remove the (now unused) declaration of COMMON_PREFIXES at the top of IconGenerator!

Comment 9

[Brian Chen]

Attachment: Remove Common_Prefixes and gradle update

Would this be better?

Comment 10

[Andrzej Hunt :ahunt]

Comment on attachment 8821685 [details] [diff] [review]
Remove Common_Prefixes and gradle update

Thank you, this looks great!

Apologies for the delay, I was away on Vacation over Christmas.

To get this landed, you'll want to set the "checkin-needed" flag on your patch, see:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_checked_into_the_tree

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/651fbecc80af
Deduplicate subdomain removal in IconGenerator. r=ahunt

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/651fbecc80af