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

Issue 29606596: Issue 6032 - Page not added to the Reading List (Closed)

Created:
Nov. 13, 2017, 10:23 p.m. by diegocarloslima
Modified:
Nov. 14, 2017, 10:20 p.m.
Reviewers:
anton, jens
CC:
René Jeschke, Felix Dahlke
Visibility:
Public.

Description

Issue 6032 - Page not added to the Reading List

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+16 lines, -14 lines) Patch
M mobile/android/chrome/content/browser.js View 2 chunks +0 lines, -14 lines 0 comments Download
M mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java View 2 chunks +16 lines, -0 lines 2 comments Download

Messages

Total messages: 6
diegocarloslima
Nov. 13, 2017, 10:24 p.m. (2017-11-13 22:24:00 UTC) #1
anton
On 2017/11/13 22:24:00, diegocarloslima wrote: LGTM (though i have no deep understanding on what's going ...
Nov. 14, 2017, 10:55 a.m. (2017-11-14 10:55:10 UTC) #2
jens
On 2017/11/13 22:24:00, diegocarloslima wrote: Shouldn't we add a comment with a link to the ...
Nov. 14, 2017, 11:17 a.m. (2017-11-14 11:17:44 UTC) #3
jens
https://codereview.adblockplus.org/29606596/diff/29606597/mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java File mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java (right): https://codereview.adblockplus.org/29606596/diff/29606597/mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java#newcode144 mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java:144: if (strippedUrl != null) { Maybe we should also ...
Nov. 14, 2017, 11:17 a.m. (2017-11-14 11:17:52 UTC) #4
diegocarloslima
https://codereview.adblockplus.org/29606596/diff/29606597/mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java File mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java (right): https://codereview.adblockplus.org/29606596/diff/29606597/mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java#newcode144 mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java:144: if (strippedUrl != null) { On 2017/11/14 11:17:51, jens ...
Nov. 14, 2017, 11:22 a.m. (2017-11-14 11:22:10 UTC) #5
jens
Nov. 14, 2017, 12:46 p.m. (2017-11-14 12:46:48 UTC) #6 
On 2017/11/14 11:22:10, diegocarloslima wrote:
>
https://codereview.adblockplus.org/29606596/diff/29606597/mobile/android/geck...
> File
>
mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java
> (right):
> 
>
https://codereview.adblockplus.org/29606596/diff/29606597/mobile/android/geck...
>
mobile/android/geckoview/src/main/java/org/mozilla/gecko/mozglue/SafeIntent.java:144:
> if (strippedUrl != null) {
> On 2017/11/14 11:17:51, jens wrote:
> > Maybe we should also check && !strippedUrl.isEmpty() to prevent for some
weird
> > errors?
> 
> Actually I'm just using the solution that Mozilla itself came up (I should
have
> made it more explicit). That way, we'll avoid merge issues when merging back
to
> 56 bookmark. That's also why I didn't include any comments referring to the
> #6032 ticket
> 
> The Mozilla commit can be found here:
> https://hg.mozilla.org/mozilla-central/rev/04554768a8e0

Alright, LGTM then :)

Powered by Google App Engine
This is Rietveld