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

Issue 2886443002: [Doodle] Move image fetching from LogoBridge to DoodleService (Closed)

Created:
3 years, 7 months ago by Marc Treib
Modified:
3 years, 7 months ago
CC:
chromium-reviews, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, droger+watchlist_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

[Doodle] Move image fetching from LogoBridge to DoodleService This will allow us to transparently implement different image fetching logic (e.g. inlining the image data into the doodle config). And this logic doesn't really belong in the bridge anyway. BUG=719513 Review-Url: https://codereview.chromium.org/2886443002 Cr-Commit-Position: refs/heads/master@{#472732} Committed: https://chromium.googlesource.com/chromium/src/+/9a13b0fc00635fddc8fd1e073f267b37648e3b10

Patch Set 1 #

Patch Set 2 : build #

Total comments: 26

Patch Set 3 : review #

Patch Set 4 : comment #

Unified diffs Side-by-side diffs Delta from patch set Stats (+252 lines, -51 lines) Patch
M chrome/browser/android/logo_bridge.h View 2 chunks +1 line, -9 lines 0 comments Download
M chrome/browser/android/logo_bridge.cc View 4 chunks +11 lines, -35 lines 0 comments Download
M chrome/browser/doodle/doodle_service_factory.cc View 2 chunks +6 lines, -1 line 0 comments Download
M components/doodle/BUILD.gn View 1 2 chunks +4 lines, -0 lines 0 comments Download
M components/doodle/DEPS View 1 chunk +2 lines, -0 lines 0 comments Download
M components/doodle/doodle_service.h View 7 chunks +27 lines, -1 line 0 comments Download
M components/doodle/doodle_service.cc View 1 2 3 6 chunks +53 lines, -2 lines 0 comments Download
M components/doodle/doodle_service_unittest.cc View 1 2 6 chunks +145 lines, -2 lines 0 comments Download
M components/image_fetcher/core/image_fetcher_impl.h View 1 chunk +1 line, -1 line 0 comments Download
M components/image_fetcher/core/image_fetcher_impl.cc View 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 39 (24 generated)
Marc Treib
PTAL!
3 years, 7 months ago (2017-05-15 13:13:59 UTC) #8
fhorschig
I really welcome this change! https://codereview.chromium.org/2886443002/diff/20001/chrome/browser/android/logo_bridge.cc File chrome/browser/android/logo_bridge.cc (left): https://codereview.chromium.org/2886443002/diff/20001/chrome/browser/android/logo_bridge.cc#oldcode295 chrome/browser/android/logo_bridge.cc:295: // TODO(treib): For interactive ...
3 years, 7 months ago (2017-05-15 13:54:36 UTC) #9
Marc Treib
Comments addressed, PTAL again. https://codereview.chromium.org/2886443002/diff/20001/chrome/browser/android/logo_bridge.cc File chrome/browser/android/logo_bridge.cc (left): https://codereview.chromium.org/2886443002/diff/20001/chrome/browser/android/logo_bridge.cc#oldcode295 chrome/browser/android/logo_bridge.cc:295: // TODO(treib): For interactive doodles, ...
3 years, 7 months ago (2017-05-15 14:25:22 UTC) #10
fhorschig
lgtm (w/ nit), thanks! https://codereview.chromium.org/2886443002/diff/20001/components/doodle/doodle_service.cc File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2886443002/diff/20001/components/doodle/doodle_service.cc#newcode34 components/doodle/doodle_service.cc:34: // against downlading huge amounts ...
3 years, 7 months ago (2017-05-15 16:26:08 UTC) #11
Marc Treib
https://codereview.chromium.org/2886443002/diff/20001/components/doodle/doodle_service.cc File components/doodle/doodle_service.cc (right): https://codereview.chromium.org/2886443002/diff/20001/components/doodle/doodle_service.cc#newcode34 components/doodle/doodle_service.cc:34: // against downlading huge amounts of data, in case ...
3 years, 7 months ago (2017-05-16 06:28:28 UTC) #12
Marc Treib
+bauerb for logo_bridge.h/cc, PTAL!
3 years, 7 months ago (2017-05-16 06:29:01 UTC) #14
Bernhard Bauer
lgtm
3 years, 7 months ago (2017-05-16 13:02:20 UTC) #15
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/2886443002/60001
3 years, 7 months ago (2017-05-16 16:30:46 UTC) #21
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/438449)
3 years, 7 months ago (2017-05-16 16:51:39 UTC) #23
Marc Treib
+danakj for new DEPS from components/doodle to ui/gfx. PTAL!
3 years, 7 months ago (2017-05-17 07:58:32 UTC) #25
danakj
LGTM
3 years, 7 months ago (2017-05-17 14:34:56 UTC) #26
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/2886443002/60001
3 years, 7 months ago (2017-05-17 14:37:29 UTC) #28
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/447152)
3 years, 7 months ago (2017-05-17 15:58:24 UTC) #30
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/2886443002/60001
3 years, 7 months ago (2017-05-18 07:54:09 UTC) #36
commit-bot: I haz the power
3 years, 7 months ago (2017-05-18 07:59:53 UTC) #39
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/9a13b0fc00635fddc8fd1e073f26...

Powered by Google App Engine
This is Rietveld 408576698