Closed
Bug 1081604
Opened 10 years ago
Closed 10 years ago
Don't ignore the result of String.replace()
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 36
People
(Reporter: ckitching, Assigned: rnewman)
Details
Attachments
(2 files)
1.12 KB,
patch
|
mfinkle
:
review-
|
Details | Diff | Splinter Review |
1.89 KB,
patch
|
wesj
:
review+
|
Details | Diff | Splinter Review |
Strings are immutable. Calling String.replace and ignoring the result is a nop. ... It's good to know we've got test coverage here... :P
Reporter | ||
Comment 1•10 years ago
|
||
Attachment #8503708 -
Flags: review?(rnewman)
Assignee | ||
Updated•10 years ago
|
Attachment #8503708 -
Flags: review?(rnewman) → review?(mark.finkle)
Comment 2•10 years ago
|
||
Comment on attachment 8503708 [details] [diff] [review] Don't ignore the result of String.replace >diff --git a/mobile/android/base/GeckoApp.java b/mobile/android/base/GeckoApp.java > if ((action != null && action.startsWith(ACTION_WEBAPP_PREFIX)) || ACTION_HOMESCREEN_SHORTCUT.equals(action)) { > uri = StringUtils.getStringExtra(intent, "args"); > if (uri != null && uri.startsWith("--url=")) { >- uri.replace("--url=", ""); >+ uri = uri.replace("--url=", ""); > } > } > return uri; I believe this block of code exists for old XUL Fennec homescreen shortcuts and old-school webapps. I think we should remove the entire block of code. Wes added this in bug 718959 for backward compat. I think this is no longer needed, so let's just remove it. I don't mind keeping getURIFromIntent as a utility, but it should just be a simple wrapper for intent.getDataString(). Wes - What do you think?
Attachment #8503708 -
Flags: review?(mark.finkle) → review-
Flags: needinfo?(wjohnston)
Comment 3•10 years ago
|
||
Comment on attachment 8503708 [details] [diff] [review] Don't ignore the result of String.replace Review of attachment 8503708 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, I'd agree. We may break someone with old webapp shortcuts, but I think we broke them long ago. Kill it.
Assignee | ||
Comment 4•10 years ago
|
||
Your wish is my command.
Attachment #8504136 -
Flags: review?(wjohnston)
Assignee | ||
Updated•10 years ago
|
Assignee: chriskitching → rnewman
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•10 years ago
|
||
Imagine that I didn't add that `static` modifier when you review!
Comment 6•10 years ago
|
||
Comment on attachment 8504136 [details] [diff] [review] Remove old --url munging code. v1 Review of attachment 8504136 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/GeckoApp.java @@ +1850,4 @@ > * Handles getting a URI from an intent in a way that is backwards- > * compatible with our previous implementations. > */ > + protected static String getURIFromIntent(Intent intent) { STATIC! (heh, I kid).
Attachment #8504136 -
Flags: review?(wjohnston) → review+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/1ecb840280ed
https://hg.mozilla.org/mozilla-central/rev/1ecb840280ed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Updated•10 years ago
|
Flags: needinfo?(wjohnston)
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
•