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

Issue 2677993002: Use IOSImageDataFetcherWrapper for favicon (Closed)

Created:
3 years, 10 months ago by gambard
Modified:
3 years, 10 months ago
CC:
chromium-reviews, pkl (ping after 24h if needed), droger+watchlist_chromium.org, blundell+watchlist_chromium.org, sdefresne+watchlist_chromium.org, Eugene But (OOO till 7-30), noyau+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Use IOSImageDataFetcherWrapper for favicon Use IOSImageDataFetcher instead of ImageDataFetcher for downloading the favicon on iOS. This allows the download to be done in the favicon driver instead of the WebState. The favicon needs the http response code for optimization (if the favicon does not exist we try to download it only once). BUG=683918 Review-Url: https://codereview.chromium.org/2677993002 Cr-Original-Commit-Position: refs/heads/master@{#450317} Committed: https://chromium.googlesource.com/chromium/src/+/911c2949fcb7a8362449e8af1d60bbb93d3cfab5 Review-Url: https://codereview.chromium.org/2677993002 Cr-Commit-Position: refs/heads/master@{#450345} Committed: https://chromium.googlesource.com/chromium/src/+/ea69b8739b1bf1a220ea541cca7886597c04bf0b

Patch Set 1 #

Total comments: 16

Patch Set 2 : Address comments #

Patch Set 3 : Cleanup WebState #

Total comments: 11

Patch Set 4 : Address comment #

Patch Set 5 : Modify enum #

Patch Set 6 : Add header #

Total comments: 10

Patch Set 7 : Address comments #

Patch Set 8 : Rebase #

Patch Set 9 : Rebase #

Total comments: 2

Patch Set 10 : Add missing include #

Patch Set 11 : Actually adding the header #

Unified diffs Side-by-side diffs Delta from patch set Stats (+88 lines, -109 lines) Patch
M components/favicon/ios/BUILD.gn View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M components/favicon/ios/DEPS View 1 2 3 4 5 6 1 chunk +6 lines, -0 lines 0 comments Download
M components/favicon/ios/web_favicon_driver.h View 1 2 3 2 chunks +4 lines, -0 lines 0 comments Download
M components/favicon/ios/web_favicon_driver.mm View 1 2 3 4 5 6 7 8 9 10 4 chunks +42 lines, -6 lines 0 comments Download
M components/image_fetcher/image_data_fetcher.h View 1 2 3 4 5 6 7 2 chunks +9 lines, -0 lines 0 comments Download
M components/image_fetcher/image_data_fetcher.cc View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -1 line 0 comments Download
M components/image_fetcher/image_data_fetcher_unittest.cc View 1 2 3 4 5 6 7 8 5 chunks +5 lines, -0 lines 0 comments Download
M components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h View 1 2 3 4 5 6 7 1 chunk +3 lines, -1 line 0 comments Download
M components/image_fetcher/request_metadata.h View 1 2 3 4 5 6 7 1 chunk +3 lines, -0 lines 0 comments Download
M components/image_fetcher/request_metadata.cc View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M components/image_fetcher/request_metadata_unittest.cc View 1 2 3 4 5 6 7 2 chunks +10 lines, -0 lines 0 comments Download
M ios/web/public/test/fakes/test_web_state.h View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -5 lines 0 comments Download
M ios/web/public/test/fakes/test_web_state.mm View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -8 lines 0 comments Download
M ios/web/public/web_state/web_state.h View 1 2 3 4 5 6 7 8 3 chunks +0 lines, -31 lines 0 comments Download
D ios/web/web_state/DEPS View 1 2 1 chunk +0 lines, -4 lines 0 comments Download
M ios/web/web_state/web_state_impl.h View 1 2 3 4 5 6 7 8 3 chunks +0 lines, -9 lines 0 comments Download
M ios/web/web_state/web_state_impl.mm View 1 2 3 4 5 6 7 8 4 chunks +0 lines, -43 lines 0 comments Download

Messages

Total messages: 42 (16 generated)
gambard
PTAL for API change in ImageDataFetcher. https://codereview.chromium.org/2677993002/diff/1/components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h File components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h (right): https://codereview.chromium.org/2677993002/diff/1/components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h#newcode26 components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h:26: // Callback that ...
3 years, 10 months ago (2017-02-06 12:21:46 UTC) #3
Marc Treib
LGTM with some nits https://codereview.chromium.org/2677993002/diff/1/components/favicon/ios/web_favicon_driver.h File components/favicon/ios/web_favicon_driver.h (right): https://codereview.chromium.org/2677993002/diff/1/components/favicon/ios/web_favicon_driver.h#newcode65 components/favicon/ios/web_favicon_driver.h:65: // Image Fetcher used to ...
3 years, 10 months ago (2017-02-06 12:35:50 UTC) #4
gambard
Thanks! https://codereview.chromium.org/2677993002/diff/1/components/favicon/ios/web_favicon_driver.h File components/favicon/ios/web_favicon_driver.h (right): https://codereview.chromium.org/2677993002/diff/1/components/favicon/ios/web_favicon_driver.h#newcode65 components/favicon/ios/web_favicon_driver.h:65: // Image Fetcher used to images. On 2017/02/06 ...
3 years, 10 months ago (2017-02-06 15:58:28 UTC) #5
gambard
sdefresne@: PTAL components/favicon/ios rohirao@: PTAL ios/chrome eugenebut@: PTAL ios/web reed@: PTAL DEPS include of skia/ ...
3 years, 10 months ago (2017-02-06 16:12:19 UTC) #7
mef
https://codereview.chromium.org/2677993002/diff/40001/components/favicon/ios/web_favicon_driver.h File components/favicon/ios/web_favicon_driver.h (right): https://codereview.chromium.org/2677993002/diff/40001/components/favicon/ios/web_favicon_driver.h#newcode10 components/favicon/ios/web_favicon_driver.h:10: #import "components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h" is this #import needed here if you ...
3 years, 10 months ago (2017-02-06 16:36:17 UTC) #8
Eugene But (OOO till 7-30)
ios/web lgtm
3 years, 10 months ago (2017-02-06 17:03:34 UTC) #9
sdefresne
lgtm https://codereview.chromium.org/2677993002/diff/40001/components/favicon/ios/web_favicon_driver.h File components/favicon/ios/web_favicon_driver.h (right): https://codereview.chromium.org/2677993002/diff/40001/components/favicon/ios/web_favicon_driver.h#newcode10 components/favicon/ios/web_favicon_driver.h:10: #import "components/image_fetcher/ios/ios_image_data_fetcher_wrapper.h" On 2017/02/06 16:36:17, mef wrote: > ...
3 years, 10 months ago (2017-02-06 17:33:34 UTC) #10
gambard
Thanks. Treib@: I have added the invalid response code to ImageDataFetcher. https://codereview.chromium.org/2677993002/diff/40001/components/favicon/ios/web_favicon_driver.h File components/favicon/ios/web_favicon_driver.h (right): ...
3 years, 10 months ago (2017-02-07 10:03:33 UTC) #11
Marc Treib
https://codereview.chromium.org/2677993002/diff/100001/components/image_fetcher/image_data_fetcher.h File components/image_fetcher/image_data_fetcher.h (right): https://codereview.chromium.org/2677993002/diff/100001/components/image_fetcher/image_data_fetcher.h#newcode30 components/image_fetcher/image_data_fetcher.h:30: // Imposible http response code. Used to signal that ...
3 years, 10 months ago (2017-02-07 10:10:21 UTC) #12
sdefresne
lgtm https://codereview.chromium.org/2677993002/diff/100001/components/favicon/ios/DEPS File components/favicon/ios/DEPS (right): https://codereview.chromium.org/2677993002/diff/100001/components/favicon/ios/DEPS#newcode4 components/favicon/ios/DEPS:4: "+skia/ext/skia_utils_ios.h", Please add the following above the rules ...
3 years, 10 months ago (2017-02-07 10:18:30 UTC) #13
Marc Treib
https://codereview.chromium.org/2677993002/diff/100001/components/image_fetcher/image_data_fetcher.h File components/image_fetcher/image_data_fetcher.h (right): https://codereview.chromium.org/2677993002/diff/100001/components/image_fetcher/image_data_fetcher.h#newcode33 components/image_fetcher/image_data_fetcher.h:33: RESPONSE_CODE_INVALID = net::URLFetcher::RESPONSE_CODE_INVALID On 2017/02/07 10:10:21, Marc Treib wrote: ...
3 years, 10 months ago (2017-02-07 10:23:14 UTC) #14
mef
This lg tm, especially considering that there is no added DEPS on net.
3 years, 10 months ago (2017-02-07 20:16:26 UTC) #15
gambard
Thanks. reed@: PTAL. https://codereview.chromium.org/2677993002/diff/100001/components/favicon/ios/DEPS File components/favicon/ios/DEPS (right): https://codereview.chromium.org/2677993002/diff/100001/components/favicon/ios/DEPS#newcode4 components/favicon/ios/DEPS:4: "+skia/ext/skia_utils_ios.h", On 2017/02/07 10:18:30, sdefresne wrote: ...
3 years, 10 months ago (2017-02-08 14:48:13 UTC) #16
gambard
+bsalomon: PTAL DEPS include of skia/ and third_party/skia in components/favicon/ios/DEPS
3 years, 10 months ago (2017-02-10 08:48:15 UTC) #18
bsalomon
skia deps lgtm
3 years, 10 months ago (2017-02-13 17:33:19 UTC) #19
reed1
lgtm
3 years, 10 months ago (2017-02-13 17:50:30 UTC) #20
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/2677993002/140001
3 years, 10 months ago (2017-02-14 09:05:30 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/152973) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, ...
3 years, 10 months ago (2017-02-14 09:07:39 UTC) #25
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/2677993002/160001
3 years, 10 months ago (2017-02-14 09:43:10 UTC) #28
commit-bot: I haz the power
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/911c2949fcb7a8362449e8af1d60bbb93d3cfab5
3 years, 10 months ago (2017-02-14 10:44:20 UTC) #31
msramek
A revert of this CL (patchset #9 id:160001) has been created in https://codereview.chromium.org/2693043003/ by msramek@chromium.org. ...
3 years, 10 months ago (2017-02-14 11:42:31 UTC) #32
tzik
On 2017/02/14 11:42:31, msramek wrote: > A revert of this CL (patchset #9 id:160001) has ...
3 years, 10 months ago (2017-02-14 12:35:51 UTC) #34
tzik
https://codereview.chromium.org/2677993002/diff/160001/components/favicon/ios/web_favicon_driver.mm File components/favicon/ios/web_favicon_driver.mm (right): https://codereview.chromium.org/2677993002/diff/160001/components/favicon/ios/web_favicon_driver.mm#newcode135 components/favicon/ios/web_favicon_driver.mm:135: web::WebThread::GetBlockingPool()) {} Could you add #include "base/threading/sequenced_worker_pool.h" for GetBlockingPool()?
3 years, 10 months ago (2017-02-14 12:37:37 UTC) #35
gambard
Thanks for the fix! https://codereview.chromium.org/2677993002/diff/160001/components/favicon/ios/web_favicon_driver.mm File components/favicon/ios/web_favicon_driver.mm (right): https://codereview.chromium.org/2677993002/diff/160001/components/favicon/ios/web_favicon_driver.mm#newcode135 components/favicon/ios/web_favicon_driver.mm:135: web::WebThread::GetBlockingPool()) {} On 2017/02/14 12:37:37, ...
3 years, 10 months ago (2017-02-14 13:05:13 UTC) #36
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/2677993002/200001
3 years, 10 months ago (2017-02-14 13:05:52 UTC) #39
commit-bot: I haz the power
3 years, 10 months ago (2017-02-14 14:06:40 UTC) #42
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/ea69b8739b1bf1a220ea541cca78...

Powered by Google App Engine
This is Rietveld 408576698