Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 30018598: Issue 7281 - Remove Google+ links (Closed)

Created:
Feb. 26, 2019, 12:07 p.m. by diegocarloslima
Modified:
March 18, 2019, 8:46 p.m.
Reviewers:
anton, jens
Visibility:
Public.

Description

Issue 7281 - Remove Google+ links

Patch Set 1 #

Total comments: 4

Patch Set 2 : Adjustments in regards to Anton's comments #

Messages

Total messages: 7
diegocarloslima
Feb. 26, 2019, 12:10 p.m. (2019-02-26 12:10:44 UTC) #1
jens
On 2019/02/26 12:10:44, diegocarloslima wrote: LGTM
Feb. 26, 2019, 12:48 p.m. (2019-02-26 12:48:14 UTC) #2
anton
https://codereview.adblockplus.org/30018598/diff/30018599/mobile/android/base/java/org/adblockplus/browser/FeedbackPreference.java File mobile/android/base/java/org/adblockplus/browser/FeedbackPreference.java (right): https://codereview.adblockplus.org/30018598/diff/30018599/mobile/android/base/java/org/adblockplus/browser/FeedbackPreference.java#newcode58 mobile/android/base/java/org/adblockplus/browser/FeedbackPreference.java:58: getContext().startActivity(intent); do we assume email client is always installed? ...
Feb. 26, 2019, 12:52 p.m. (2019-02-26 12:52:55 UTC) #3
diegocarloslima
https://codereview.adblockplus.org/30018598/diff/30018599/mobile/android/base/java/org/adblockplus/browser/FeedbackPreference.java File mobile/android/base/java/org/adblockplus/browser/FeedbackPreference.java (right): https://codereview.adblockplus.org/30018598/diff/30018599/mobile/android/base/java/org/adblockplus/browser/FeedbackPreference.java#newcode58 mobile/android/base/java/org/adblockplus/browser/FeedbackPreference.java:58: getContext().startActivity(intent); On 2019/02/26 12:52:54, anton wrote: > do we ...
Feb. 27, 2019, 4:37 p.m. (2019-02-27 16:37:57 UTC) #4
anton
On 2019/02/27 16:37:57, diegocarloslima wrote: > https://codereview.adblockplus.org/30018598/diff/30018599/mobile/android/base/java/org/adblockplus/browser/FeedbackPreference.java > File mobile/android/base/java/org/adblockplus/browser/FeedbackPreference.java > (right): > > https://codereview.adblockplus.org/30018598/diff/30018599/mobile/android/base/java/org/adblockplus/browser/FeedbackPreference.java#newcode58 ...
Feb. 27, 2019, 4:39 p.m. (2019-02-27 16:39:21 UTC) #5
anton
On 2019/02/27 16:39:21, anton wrote: > On 2019/02/27 16:37:57, diegocarloslima wrote: > > > https://codereview.adblockplus.org/30018598/diff/30018599/mobile/android/base/java/org/adblockplus/browser/FeedbackPreference.java ...
Feb. 28, 2019, 8:58 a.m. (2019-02-28 08:58:36 UTC) #6
jens
Feb. 28, 2019, 9:54 a.m. (2019-02-28 09:54:27 UTC) #7
On 2019/02/28 08:58:36, anton wrote:
> On 2019/02/27 16:39:21, anton wrote:
> > On 2019/02/27 16:37:57, diegocarloslima wrote:
> > >
> >
>
https://codereview.adblockplus.org/30018598/diff/30018599/mobile/android/base...
> > > File
> mobile/android/base/java/org/adblockplus/browser/FeedbackPreference.java
> > > (right):
> > > 
> > >
> >
>
https://codereview.adblockplus.org/30018598/diff/30018599/mobile/android/base...
> > >
mobile/android/base/java/org/adblockplus/browser/FeedbackPreference.java:58:
> > > getContext().startActivity(intent);
> > > On 2019/02/26 12:52:54, anton wrote:
> > > > do we assume email client is always installed? otherwise we should try {
}
> > > catch
> > > > {}
> > > 
> > > I don't think that an exception will ever occur, but I see your point,
will
> > add
> > > a try catch block to handle this gracefully.
> > 
> > Yeah, i don't think it will actually happen as most of users have email
> client,
> > but anyway.
> > 
> > > 
> > >
> >
>
https://codereview.adblockplus.org/30018598/diff/30018599/mobile/android/base...
> > >
mobile/android/base/java/org/adblockplus/browser/FeedbackPreference.java:87:
> > //
> > > TODO: Replace by ConfigurationCompat when the support lib is >= 26.1.0
> > > On 2019/02/26 12:52:54, anton wrote:
> > > > afaik somewhere in code style guide there is requirement to remove TODO:
> > from
> > > > code and create a ticket instead. How do you think?
> > > 
> > > Acknowledged.
> 
> LGTM

LGTM

Powered by Google App Engine
This is Rietveld