Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2528043002:
Implement DownloadImage directly in WebStateImpl (Closed)
DescriptionImplement DownloadImage directly in WebStateImpl
The DownloadImage method in WebState was implemented in Tab.
This CL moves the implementation to WebStateImpl.
BUG=664988
Committed: https://crrev.com/36b0a346e9fd8a05fc993f51b44a64850248cc63
Cr-Commit-Position: refs/heads/master@{#436319}
Patch Set 1 #Patch Set 2 : Add deps #
Total comments: 16
Patch Set 3 : Address comments #
Total comments: 4
Patch Set 4 : Address comments #
Total comments: 2
Patch Set 5 : Restrict DEPS #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 35 (16 generated)
gambard@chromium.org changed reviewers: + eugenebut@chromium.org
PTAL.
gambard@chromium.org changed reviewers: + kkhorimoto@chromium.org
+kkhorimoto@ as eugene is OOO. PTAL.
kkhorimoto@chromium.org changed reviewers: + rohitrao@chromium.org
The code generally looks good, but I don't really understand the purpose for this CL or how it helps with favicon fetching. Is it because the distiller uses WebStates without any delegates? I'm generally okay with moving this code into web//, except for possible layering violations. This lgtm if Rohit is okay with referencing skia from within web//. https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/DEPS File ios/web/web_state/DEPS (right): https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/DEPS#... ios/web/web_state/DEPS:3: "+third_party/skia", I'm pretty sure that we don't want to introduce third_party deps from within web//.
Thanks. rohitrao@: PTAL. Yes, during the distillation of the reading list entry, the favicon will be fetched. As the distillation is not associated with a Tab, there is no delegate to fetch the image. It also makes more sense to have the WebState doing it.
Please explain the reason for the change in CL description, which is "Do not use CRWWebDelegate for image downloading, which mirrors content approach." https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/DEPS File ios/web/web_state/DEPS (right): https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/DEPS#... ios/web/web_state/DEPS:3: "+third_party/skia", On 2016/11/28 23:56:50, kkhorimoto_ wrote: > I'm pretty sure that we don't want to introduce third_party deps from within > web//. +1 Can we use UIKit API for extracting image size? ios/web is a library and we should be cautious about it's binary size. https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/web_s... File ios/web/web_state/web_state_impl.h (right): https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.h:373: // Image Fetcher used to download favicons. This can download any image, not only favicons. Right? https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/web_s... File ios/web/web_state/web_state_impl.mm (right): https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.mm:548: static int g_download_id = 0; nit: g_ prefix means "global", so please don't use it here. https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.mm:549: int localDownloadId = ++g_download_id; s/localDownloadId/local_download_id Same comments for other names https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.mm:553: ^(const GURL& originalUrl, const int responseCode, NSData* data) { nit: you can drop |originalUrl| https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.mm:565: } We should also call callback for failure cases, unless there is a good reason for not doing that (and if that's the case we should document that in API comments). BTW, does content calls this callback for failure cases? https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.mm:567: // |imageFetcher_| may have been reset if the tab got closed. Please don't mention tab here, web does not know anything about tabs.
Thanks, PTAL. https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/DEPS File ios/web/web_state/DEPS (right): https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/DEPS#... ios/web/web_state/DEPS:3: "+third_party/skia", On 2016/11/30 17:13:49, Eugene But wrote: > On 2016/11/28 23:56:50, kkhorimoto_ wrote: > > I'm pretty sure that we don't want to introduce third_party deps from within > > web//. > +1 Can we use UIKit API for extracting image size? ios/web is a library and we > should be cautious about it's binary size. As explain in another CL: UIImage makes some assumptions about the format/scale of the image. For example if you download a file favicon.ico containing multiple images, a UIImage representation will keep only one of them (from my understanding). https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/DEPS#... ios/web/web_state/DEPS:3: "+third_party/skia", On 2016/11/30 17:13:49, Eugene But wrote: > On 2016/11/28 23:56:50, kkhorimoto_ wrote: > > I'm pretty sure that we don't want to introduce third_party deps from within > > web//. > +1 Can we use UIKit API for extracting image size? ios/web is a library and we > should be cautious about it's binary size. Actually, the ImageFetchedCallback type use a SkBitmap as a parameter type, so I don't see how I can be able to do it without a skia include. https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/web_s... File ios/web/web_state/web_state_impl.h (right): https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.h:373: // Image Fetcher used to download favicons. On 2016/11/30 17:13:50, Eugene But wrote: > This can download any image, not only favicons. Right? Yes but for now |downloadImage| is only called for favicon. Done. https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/web_s... File ios/web/web_state/web_state_impl.mm (right): https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.mm:548: static int g_download_id = 0; On 2016/11/30 17:13:50, Eugene But wrote: > nit: g_ prefix means "global", so please don't use it here. Done. https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.mm:549: int localDownloadId = ++g_download_id; On 2016/11/30 17:13:50, Eugene But wrote: > s/localDownloadId/local_download_id > > Same comments for other names Done. https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.mm:553: ^(const GURL& originalUrl, const int responseCode, NSData* data) { On 2016/11/30 17:13:50, Eugene But wrote: > nit: you can drop |originalUrl| Done. https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.mm:565: } On 2016/11/30 17:13:50, Eugene But wrote: > We should also call callback for failure cases, unless there is a good reason > for not doing that (and if that's the case we should document that in API > comments). BTW, does content calls this callback for failure cases? This is not for failure. From the comments on the enum: // Imposible http response code. Used to signal that no http response code was received. From what I understood of content they are calling the callback every time. https://codereview.chromium.org/2528043002/diff/20001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.mm:567: // |imageFetcher_| may have been reset if the tab got closed. On 2016/11/30 17:13:50, Eugene But wrote: > Please don't mention tab here, web does not know anything about tabs. Done.
lgtm CCing Mike to let him know that binary size may be increased by this change https://codereview.chromium.org/2528043002/diff/40001/ios/web/web_state/web_s... File ios/web/web_state/web_state_impl.mm (right): https://codereview.chromium.org/2528043002/diff/40001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.mm:19: #include "ios/web/public/image_fetcher/image_data_fetcher.h" s/include/import https://codereview.chromium.org/2528043002/diff/40001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.mm:548: static int static_download_id = 0; How about s/static_download_id/downloaded_image_count
Thanks! https://codereview.chromium.org/2528043002/diff/40001/ios/web/web_state/web_s... File ios/web/web_state/web_state_impl.mm (right): https://codereview.chromium.org/2528043002/diff/40001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.mm:19: #include "ios/web/public/image_fetcher/image_data_fetcher.h" On 2016/12/01 17:08:53, Eugene But wrote: > s/include/import Done. https://codereview.chromium.org/2528043002/diff/40001/ios/web/web_state/web_s... ios/web/web_state/web_state_impl.mm:548: static int static_download_id = 0; On 2016/12/01 17:08:53, Eugene But wrote: > How about s/static_download_id/downloaded_image_count Done.
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2528043002/#ps60001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
gambard@chromium.org changed reviewers: + bsalomon@google.com
+bsalomon@google.com@ for skia includes.
The CQ bit was unchecked by commit-bot@chromium.org
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_presub...)
https://codereview.chromium.org/2528043002/diff/60001/ios/web/web_state/DEPS File ios/web/web_state/DEPS (right): https://codereview.chromium.org/2528043002/diff/60001/ios/web/web_state/DEPS#... ios/web/web_state/DEPS:2: "+skia/ext", Could you restrict these rules a bit further, to the set that's in https://codereview.chromium.org/2541083002/diff/30001/ios/chrome/DEPS? Per the comment in that file, we only use parts of skia, so we should keep the deps rules as tight as we can.
Thanks. https://codereview.chromium.org/2528043002/diff/60001/ios/web/web_state/DEPS File ios/web/web_state/DEPS (right): https://codereview.chromium.org/2528043002/diff/60001/ios/web/web_state/DEPS#... ios/web/web_state/DEPS:2: "+skia/ext", On 2016/12/02 11:45:21, rohitrao wrote: > Could you restrict these rules a bit further, to the set that's in > https://codereview.chromium.org/2541083002/diff/30001/ios/chrome/DEPS Per the > comment in that file, we only use parts of skia, so we should keep the deps > rules as tight as we can. Done.
The CQ bit was checked by gambard@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
gambard@chromium.org changed reviewers: + reed@google.com
+reed@google.com as skia owner
lgtm
Thanks!
The CQ bit was checked by gambard@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2528043002/#ps80001 (title: "Restrict DEPS")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480951565950550,
"parent_rev": "48fc35daf2896451dacc88e98ec49400201450ae", "commit_rev":
"22a7fe33b9b3c61a79036521de926d1bd813e2b4"}
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Implement DownloadImage directly in WebStateImpl The DownloadImage method in WebState was implemented in Tab. This CL moves the implementation to WebStateImpl. BUG=664988 ========== to ========== Implement DownloadImage directly in WebStateImpl The DownloadImage method in WebState was implemented in Tab. This CL moves the implementation to WebStateImpl. BUG=664988 Committed: https://crrev.com/36b0a346e9fd8a05fc993f51b44a64850248cc63 Cr-Commit-Position: refs/heads/master@{#436319} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/36b0a346e9fd8a05fc993f51b44a64850248cc63 Cr-Commit-Position: refs/heads/master@{#436319} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
