Closed
Bug 1325488
Opened 7 years ago
Closed 7 years ago
Deduplicate subdomain removal in IconGenerator
Categories
(Firefox for Android Graveyard :: General, defect)
Firefox for Android Graveyard
General
Tracking
(firefox53 fixed)
RESOLVED
FIXED
Firefox 53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: ahunt, Assigned: brianchen0810, Mentored)
Details
(Whiteboard: [good first bug])
Attachments
(1 file, 3 obsolete files)
1.99 KB,
patch
|
ahunt
:
review+
|
Details | Diff | Splinter Review |
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•7 years 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
Whiteboard: [good first bug]
Assignee | ||
Comment 2•7 years 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•7 years ago
|
||
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•7 years ago
|
Attachment #8821370 -
Attachment is obsolete: true
Attachment #8821370 -
Flags: review?(ahunt)
Assignee | ||
Comment 4•7 years ago
|
||
Sorry about that last patch
Attachment #8821376 -
Flags: review?(ahunt)
Assignee | ||
Comment 5•7 years 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•7 years ago
|
||
Attachment #8821376 -
Attachment is obsolete: true
Attachment #8821376 -
Flags: review?(ahunt)
Reporter | ||
Comment 7•7 years 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•7 years 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•7 years ago
|
||
Would this be better?
Attachment #8821411 -
Attachment is obsolete: true
Attachment #8821685 -
Flags: review?(ahunt)
Reporter | ||
Comment 10•7 years 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 years ago
|
Keywords: checkin-needed
Comment 11•7 years 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 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/651fbecc80af
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•