Deduplicate subdomain removal in IconGenerator

RESOLVED FIXED in Firefox 53

Status

()

Firefox for Android
General
RESOLVED FIXED
8 months ago
7 months ago

People

(Reporter: ahunt, Assigned: Brian Chen, Mentored)

Tracking

Trunk
Firefox 53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

8 months ago
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
(Reporter)

Comment 1

8 months ago
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.)
Mentor: ahunt@mozilla.com
Whiteboard: [good first bug]
(Assignee)

Comment 2

8 months ago
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!
(Assignee)

Comment 3

8 months ago
Created attachment 8821370 [details] [diff] [review]
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.
Attachment #8821370 - Flags: review?(ahunt)
(Assignee)

Updated

8 months ago
Attachment #8821370 - Attachment is obsolete: true
Attachment #8821370 - Flags: review?(ahunt)
(Assignee)

Comment 4

8 months ago
Created attachment 8821376 [details] [diff] [review]
Changes subdomain removal method in IconGenerator

Sorry about that last patch
Attachment #8821376 - Flags: review?(ahunt)
(Assignee)

Comment 5

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

># HG changeset patch
># User Brian Chen <brianchen0810@hotmail.com>
># 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;
>     }
(Assignee)

Comment 6

8 months ago
Created attachment 8821411 [details] [diff] [review]
Edit error in code
Attachment #8821376 - Attachment is obsolete: true
Attachment #8821376 - Flags: review?(ahunt)
(Reporter)

Comment 7

8 months ago
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.)
Assignee: nobody → brianchen0810
Status: NEW → ASSIGNED
(Reporter)

Comment 8

8 months ago
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!
Attachment #8821376 - Flags: feedback+
(Assignee)

Comment 9

8 months ago
Created attachment 8821685 [details] [diff] [review]
Remove Common_Prefixes and gradle update

Would this be better?
Attachment #8821411 - Attachment is obsolete: true
Attachment #8821685 - Flags: review?(ahunt)
(Reporter)

Comment 10

7 months ago
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
Attachment #8821685 - Flags: review?(ahunt) → review+
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 11

7 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/651fbecc80af
Deduplicate subdomain removal in IconGenerator. r=ahunt
Keywords: checkin-needed

Comment 12

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/651fbecc80af
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.