Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(161)

Issue 2604953002: Remove mContext field from UrlManager (Closed)

Created:
3 years, 11 months ago by cco3
Modified:
3 years, 11 months ago
Reviewers:
nyquist, mattreynolds
CC:
chromium-reviews, agrieve+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove mContext field from UrlManager This has been the the application context for some time now, so there is no point in storing it when we can use ContextUtils as needed. BUG=678787 Review-Url: https://codereview.chromium.org/2604953002 Cr-Commit-Position: refs/heads/master@{#441778} Committed: https://chromium.googlesource.com/chromium/src/+/cad9e8497986d345637e875bcc05952bc34840de

Patch Set 1 #

Total comments: 2

Patch Set 2 : Remove context param in constructor #

Total comments: 2

Patch Set 3 : Use LazyHolder #

Patch Set 4 : Add bug number #

Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -56 lines) Patch
M chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java View 1 2 12 chunks +40 lines, -35 lines 0 comments Download
M chrome/android/javatests/src/org/chromium/chrome/browser/physicalweb/UrlManagerTest.java View 1 7 chunks +19 lines, -21 lines 0 comments Download

Messages

Total messages: 17 (7 generated)
cco3
3 years, 11 months ago (2017-01-05 19:43:01 UTC) #2
mattreynolds
https://codereview.chromium.org/2604953002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/2604953002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java#newcode104 chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:104: public UrlManager(Context context) { Would it make sense to ...
3 years, 11 months ago (2017-01-05 19:49:57 UTC) #3
cco3
https://codereview.chromium.org/2604953002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/2604953002/diff/1/chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java#newcode104 chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:104: public UrlManager(Context context) { On 2017/01/05 19:49:56, mattreynolds wrote: ...
3 years, 11 months ago (2017-01-05 21:17:43 UTC) #4
cco3
Hi nyquist@, here's another cleanup change if you have time for it.
3 years, 11 months ago (2017-01-05 21:18:13 UTC) #6
mattreynolds
lgtm
3 years, 11 months ago (2017-01-05 21:27:40 UTC) #7
nyquist
https://codereview.chromium.org/2604953002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/2604953002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java#newcode130 chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:130: public static UrlManager getInstance() { Now that the constructor ...
3 years, 11 months ago (2017-01-05 21:54:13 UTC) #8
cco3
https://codereview.chromium.org/2604953002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java File chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java (right): https://codereview.chromium.org/2604953002/diff/20001/chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java#newcode130 chrome/android/java/src/org/chromium/chrome/browser/physicalweb/UrlManager.java:130: public static UrlManager getInstance() { On 2017/01/05 21:54:13, nyquist ...
3 years, 11 months ago (2017-01-05 22:39:41 UTC) #10
nyquist
lgtm
3 years, 11 months ago (2017-01-05 22:54:51 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2604953002/60001
3 years, 11 months ago (2017-01-05 22:57:20 UTC) #14
commit-bot: I haz the power
3 years, 11 months ago (2017-01-06 01:15:06 UTC) #17
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/cad9e8497986d345637e875bcc05...

Powered by Google App Engine
This is Rietveld 408576698