Issue 29640558: Issue 6187 - Adjust query string parameters for Yandex (Closed)
|
Created:
Dec. 15, 2017, 3:53 p.m. by jens Modified:
Dec. 19, 2017, 4:48 p.m. CC:
René Jeschke Visibility:
Public. |
DescriptionIssue 6187 - Adjust query string parameters for Yandex
Patch Set 1 #
Total comments: 2
Patch Set 2 : Added package name of alpha and beta versions of both browsers #
Total comments: 2
Patch Set 3 : Renamed method to isPackageInstalled #MessagesTotal messages: 8
https://codereview.adblockplus.org/29640558/diff/29640559/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/AppInfo.java (right): https://codereview.adblockplus.org/29640558/diff/29640559/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/AppInfo.java:134: private String checkForCompatibleInstalledBrowser(final PackageManager packageManager) I think that we should also consider all the alpha/beta variants packages: com.sec.android.app.sbrowser.beta com.yandex.browser.alpha com.yandex.browser.beta
https://codereview.adblockplus.org/29640558/diff/29640559/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/AppInfo.java (right): https://codereview.adblockplus.org/29640558/diff/29640559/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/AppInfo.java:134: private String checkForCompatibleInstalledBrowser(final PackageManager packageManager) On 2017/12/15 16:13:43, diegocarloslima wrote: > I think that we should also consider all the alpha/beta variants packages: > com.sec.android.app.sbrowser.beta > com.yandex.browser.alpha > com.yandex.browser.beta Thats actually a good idea :)
On 2017/12/18 08:41:34, jens wrote: > https://codereview.adblockplus.org/29640558/diff/29640559/adblockplussbrowser... > File > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/AppInfo.java > (right): > > https://codereview.adblockplus.org/29640558/diff/29640559/adblockplussbrowser... > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/AppInfo.java:134: > private String checkForCompatibleInstalledBrowser(final PackageManager > packageManager) > On 2017/12/15 16:13:43, diegocarloslima wrote: > > I think that we should also consider all the alpha/beta variants packages: > > com.sec.android.app.sbrowser.beta > > com.yandex.browser.alpha > > com.yandex.browser.beta > > Thats actually a good idea :) Published a new patch set
https://codereview.adblockplus.org/29640558/diff/29643555/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/AppInfo.java (right): https://codereview.adblockplus.org/29640558/diff/29643555/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/AppInfo.java:157: private boolean packageIsInstalled(final PackageManager packageManager, final String packageName) I would prefer this method to be called isPackageInstalled in order to follow naming convention for this kind of boolean methods
Added a new patch set https://codereview.adblockplus.org/29640558/diff/29643555/adblockplussbrowser... File adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/AppInfo.java (right): https://codereview.adblockplus.org/29640558/diff/29643555/adblockplussbrowser... adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/AppInfo.java:157: private boolean packageIsInstalled(final PackageManager packageManager, final String packageName) On 2017/12/18 09:15:30, diegocarloslima wrote: > I would prefer this method to be called isPackageInstalled in order to follow > naming convention for this kind of boolean methods Acknowledged.
On 2017/12/18 09:29:48, jens wrote: > Added a new patch set > > https://codereview.adblockplus.org/29640558/diff/29643555/adblockplussbrowser... > File > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/AppInfo.java > (right): > > https://codereview.adblockplus.org/29640558/diff/29643555/adblockplussbrowser... > adblockplussbrowser/src/main/java/org/adblockplus/sbrowser/contentblocker/engine/AppInfo.java:157: > private boolean packageIsInstalled(final PackageManager packageManager, final > String packageName) > On 2017/12/18 09:15:30, diegocarloslima wrote: > > I would prefer this method to be called isPackageInstalled in order to follow > > naming convention for this kind of boolean methods > > Acknowledged. LGTM
On 2017/12/15 16:04:36, jens wrote: LGTM 
|
