Issue 29346916: Issue 3220 - FAQ and Privacy Policy links redirect to English site only (Closed)
|
Created:
June 22, 2016, 12:31 a.m. by diegocarloslima Modified:
Dec. 12, 2016, 4:27 p.m. CC:
René Jeschke Visibility:
Public. |
DescriptionIssue 3220 - FAQ and Privacy Policy links redirect to English site only
Patch Set 1 #
Total comments: 14
Patch Set 2 : Applying changes from code review #
Total comments: 3
MessagesTotal messages: 10
Looks good! Just some minor comments. https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/app/... File mobile/android/app/mobile.js (right): https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/app/... mobile/android/app/mobile.js:490: pref("app.faqURL", "https://adblockplus.org/redirect?link=adblock_browser_android_faq&lang=%LOCALE%"); Since we're leaving the URLs above alone, we should add a second comment pointing to issue #3220 here I think, seems we've so far preceeded every change in this file with a comment pointing to the relevant issue. https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/base... File mobile/android/base/preferences/GeckoPreferences.java (right): https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/base... mobile/android/base/preferences/GeckoPreferences.java:920: final String url = getResources().getString(R.string.faq_link, LOCALE); How I see it, you're just removing the VERSION and OS parameters. Why not leave them here? Even though we don't need them (yet), they don't hurt us if we don't use them, do they? https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/base... File mobile/android/base/resources/xml/preferences_vendor.xml (right): https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/base... mobile/android/base/resources/xml/preferences_vendor.xml:23: url="https://adblockplus.org/redirect?link=adblock_browser_android_beta_community" /> I just realised that this link originally pointed to about:feedback, and we changed it back before we modified and reactivated about:feedback. Now that it's back, we can revert this to the upstream version again. But that doesn't have much to do with #3220, I've created a new issue for this: https://issues.adblockplus.org/ticket/4439 https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/base... File mobile/android/base/strings.xml.in (right): https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/base... mobile/android/base/strings.xml.in:87: <!-- https://adblockplus.org/redirect?link=adblock_browser_android_faq&lang=%LOCALE% --> Nit: I think we should move this line below the "Changed" line, that seems consistent with how we normally do it. https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/chro... File mobile/android/chrome/content/aboutAdblockBrowser.xhtml (right): https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/chro... mobile/android/chrome/content/aboutAdblockBrowser.xhtml:110: try { Something seems wrong here with the indentation. https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/chro... mobile/android/chrome/content/aboutAdblockBrowser.xhtml:114: {id: "faqURL", pref: "app.faqURL"}, Given that we copied this code from about.js, we can adjust it to our coding style. So just one space after , - we usually avoid column indentation.
On 2016/06/22 00:32:57, diegocarloslima wrote: LGTM
Please note that I've posted some comments that weren't addressed yet, so NOT LGTM from my side.
https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/app/... File mobile/android/app/mobile.js (right): https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/app/... mobile/android/app/mobile.js:490: pref("app.faqURL", "https://adblockplus.org/redirect?link=adblock_browser_android_faq&lang=%LOCALE%"); On 2016/09/15 14:59:22, Felix Dahlke wrote: > Since we're leaving the URLs above alone, we should add a second comment > pointing to issue #3220 here I think, seems we've so far preceeded every change > in this file with a comment pointing to the relevant issue. Acknowledged. https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/base... File mobile/android/base/preferences/GeckoPreferences.java (right): https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/base... mobile/android/base/preferences/GeckoPreferences.java:920: final String url = getResources().getString(R.string.faq_link, LOCALE); On 2016/09/15 14:59:22, Felix Dahlke wrote: > How I see it, you're just removing the VERSION and OS parameters. Why not leave > them here? Even though we don't need them (yet), they don't hurt us if we don't > use them, do they? In order to not break the formatting when keeping VERSION and OS parameters, we'll need to change the faq_link in strings.xml.in to something like: <string name="faq_link">https://adblockplus.org/redirect?link=adblock_browser_android_faq&version=&formatS1;&os=&formatS2;&lang=&formatS3;</string> Is that ok? https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/base... File mobile/android/base/resources/xml/preferences_vendor.xml (right): https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/base... mobile/android/base/resources/xml/preferences_vendor.xml:23: url="https://adblockplus.org/redirect?link=adblock_browser_android_beta_community" /> On 2016/09/15 14:59:22, Felix Dahlke wrote: > I just realised that this link originally pointed to about:feedback, and we > changed it back before we modified and reactivated about:feedback. Now that it's > back, we can revert this to the upstream version again. But that doesn't have > much to do with #3220, I've created a new issue for this: > https://issues.adblockplus.org/ticket/4439 Acknowledged. https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/base... File mobile/android/base/strings.xml.in (right): https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/base... mobile/android/base/strings.xml.in:87: <!-- https://adblockplus.org/redirect?link=adblock_browser_android_faq&lang=%LOCALE% --> On 2016/09/15 14:59:22, Felix Dahlke wrote: > Nit: I think we should move this line below the "Changed" line, that seems > consistent with how we normally do it. Acknowledged. https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/chro... File mobile/android/chrome/content/aboutAdblockBrowser.xhtml (right): https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/chro... mobile/android/chrome/content/aboutAdblockBrowser.xhtml:110: try { On 2016/09/15 14:59:22, Felix Dahlke wrote: > Something seems wrong here with the indentation. Acknowledged. https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/chro... mobile/android/chrome/content/aboutAdblockBrowser.xhtml:114: {id: "faqURL", pref: "app.faqURL"}, On 2016/09/15 14:59:22, Felix Dahlke wrote: > Given that we copied this code from about.js, we can adjust it to our coding > style. So just one space after , - we usually avoid column indentation. Acknowledged.
https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/base... File mobile/android/base/preferences/GeckoPreferences.java (right): https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/base... mobile/android/base/preferences/GeckoPreferences.java:920: final String url = getResources().getString(R.string.faq_link, LOCALE); On 2016/10/21 13:37:43, diegocarloslima wrote: > On 2016/09/15 14:59:22, Felix Dahlke wrote: > > How I see it, you're just removing the VERSION and OS parameters. Why not > leave > > them here? Even though we don't need them (yet), they don't hurt us if we > don't > > use them, do they? > > In order to not break the formatting when keeping VERSION and OS parameters, > we'll need to change the faq_link in strings.xml.in to something like: > <string > name="faq_link">https://adblockplus.org/redirect?link=adblock_browser_android_faq&version=&formatS1;&os=&formatS2;&lang=&formatS3;</string> > > Is that ok? Hm, well if it's a position based format, that's true... But IMHO I'd prefer that change (i.e. adding more parameters that we are currently not interested in) than to change the code here.
https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/base... File mobile/android/base/preferences/GeckoPreferences.java (right): https://codereview.adblockplus.org/29346916/diff/29346917/mobile/android/base... mobile/android/base/preferences/GeckoPreferences.java:920: final String url = getResources().getString(R.string.faq_link, LOCALE); On 2016/10/28 10:14:52, Felix Dahlke wrote: > On 2016/10/21 13:37:43, diegocarloslima wrote: > > On 2016/09/15 14:59:22, Felix Dahlke wrote: > > > How I see it, you're just removing the VERSION and OS parameters. Why not > > leave > > > them here? Even though we don't need them (yet), they don't hurt us if we > > don't > > > use them, do they? > > > > In order to not break the formatting when keeping VERSION and OS parameters, > > we'll need to change the faq_link in strings.xml.in to something like: > > <string > > > name="faq_link">https://adblockplus.org/redirect?link=adblock_browser_android_faq&version=&formatS1;&os=&formatS2;&lang=&formatS3;</string> > > > > Is that ok? > > Hm, well if it's a position based format, that's true... But IMHO I'd prefer > that change (i.e. adding more parameters that we are currently not interested > in) than to change the code here. Acknowledged.
Almost there :) https://codereview.adblockplus.org/29346916/diff/29361530/mobile/android/chro... File mobile/android/chrome/content/aboutAdblockBrowser.xhtml (right): https://codereview.adblockplus.org/29346916/diff/29361530/mobile/android/chro... mobile/android/chrome/content/aboutAdblockBrowser.xhtml:108: // Using formatted URLs. See https://issues.adblockplus.org/ticket/3220 Don't we revert this to the original upstream version here? Because in that case we shouldn't add a comment here then.
https://codereview.adblockplus.org/29346916/diff/29361530/mobile/android/chro... File mobile/android/chrome/content/aboutAdblockBrowser.xhtml (right): https://codereview.adblockplus.org/29346916/diff/29361530/mobile/android/chro... mobile/android/chrome/content/aboutAdblockBrowser.xhtml:108: // Using formatted URLs. See https://issues.adblockplus.org/ticket/3220 On 2016/11/17 18:34:37, Felix Dahlke wrote: > Don't we revert this to the original upstream version here? Because in that case > we shouldn't add a comment here then. Yes, this is a snippet taken from the upstream code 'about.js'. But this is slightly different from the upstream version. In the links declaration, we just use faqURL, privacyURL and creditsURL, while they also have releaseNotesUrl and supportURL. Should I place some comment above the line 'let links = [' , then?
LGTM https://codereview.adblockplus.org/29346916/diff/29361530/mobile/android/chro... File mobile/android/chrome/content/aboutAdblockBrowser.xhtml (right): https://codereview.adblockplus.org/29346916/diff/29361530/mobile/android/chro... mobile/android/chrome/content/aboutAdblockBrowser.xhtml:108: // Using formatted URLs. See https://issues.adblockplus.org/ticket/3220 On 2016/11/21 14:37:41, diegocarloslima wrote: > On 2016/11/17 18:34:37, Felix Dahlke wrote: > > Don't we revert this to the original upstream version here? Because in that > case > > we shouldn't add a comment here then. > > Yes, this is a snippet taken from the upstream code 'about.js'. But this is > slightly different from the upstream version. In the links declaration, we just > use faqURL, privacyURL and creditsURL, while they also have releaseNotesUrl and > supportURL. Should I place some comment above the line 'let links = [' , then? Just realised this is not upstream code but rather our copy. So nevermind :) 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
