Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2558393004:
Refactoring: Moves image fetching and caching into CachedImageFetcher (Closed)
DescriptionRefactoring: Moves the image fetching and caching logic out of the RemoteSuggestionsProvider into a CachedImageFetcher class.
There's a small functional change: After fetching an image from the network, the old version used to verify the suggestion still exist before storing it into the database. This is extremely unlikely to happen and even if it happens not as bad as we clear orphaned images on start-up.
BUG=
Committed: https://crrev.com/8e18ae99daecab970828838db34f8e40df344f89
Cr-Commit-Position: refs/heads/master@{#438818}
Patch Set 1 #
Total comments: 7
Patch Set 2 : comments and lint warnings (primarily missing utility include for std::move) #
Total comments: 1
Patch Set 3 : fixed indentation #
Total comments: 2
Patch Set 4 : rebase #Patch Set 5 : extend test error message #
Messages
Total messages: 23 (8 generated)
tschumann@chromium.org changed reviewers: + jkrcal@chromium.org
your last CL reminded me of a clean-up a planned some time ago :-)
Nice! Much better, thanks! lgtm / nits below. https://codereview.chromium.org/2558393004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_provider.cc (right): https://codereview.chromium.org/2558393004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_provider.cc:1021: GURL image_url = FindSnippetImageUrl(suggestion_id); What about empty |image_url| (because the snippet does not exist any more, etc)? I see it treated neither here nor in CachedImageFetcher. https://codereview.chromium.org/2558393004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2558393004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_provider.h:428: CachedImageFetcher image_fetcher_; Nit: cached_image_fetcher_? (This member name can be confusing as the constructor receives image_fetcher but this one is quite a different type.)
treib@chromium.org changed reviewers: + treib@chromium.org
Nice! One more drive-by nit :) https://codereview.chromium.org/2558393004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2558393004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_provider.h:428: CachedImageFetcher image_fetcher_; nit2: This is between |database_| and |database_load_start_|, which does not seem like a good location. Move it after?
+treib: there's one question for you in the comments i'd like your input on. https://codereview.chromium.org/2558393004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_provider.cc (right): https://codereview.chromium.org/2558393004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_provider.cc:1021: GURL image_url = FindSnippetImageUrl(suggestion_id); On 2016/12/12 09:01:11, jkrcal wrote: > What about empty |image_url| (because the snippet does not exist any more, etc)? > I see it treated neither here nor in CachedImageFetcher. interesting! The actual behavior is still the same (we check the URL after a failed database lookup). However, chances for that lookup to succeed are very small, given we don't have any trace of the image. We can probably save the database access -- I added a test for this. Marc, wdyt? https://codereview.chromium.org/2558393004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2558393004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_provider.h:428: CachedImageFetcher image_fetcher_; On 2016/12/12 10:01:03, Marc Treib wrote: > nit2: This is between |database_| and |database_load_start_|, which does not > seem like a good location. Move it after? moved it down. I'd like to push back on renaming it to cached_image fetcher, as I'd much have the constructor take the CachedImageFetcher instead of the image_fetcher::ImageFetcher. (IMO, we should only include type information in variable names if we really need it and that wouldn't be the case then anymore ;-)).
https://codereview.chromium.org/2558393004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_provider.h (right): https://codereview.chromium.org/2558393004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_provider.h:428: CachedImageFetcher image_fetcher_; On 2016/12/13 20:59:09, tschumann wrote: > On 2016/12/12 10:01:03, Marc Treib wrote: > > nit2: This is between |database_| and |database_load_start_|, which does not > > seem like a good location. Move it after? > > moved it down. I'd like to push back on renaming it to cached_image fetcher, as > I'd much have the constructor take the CachedImageFetcher instead of the > image_fetcher::ImageFetcher. (IMO, we should only include type information in > variable names if we really need it and that wouldn't be the case then anymore > ;-)). Fine for me...
On 2016/12/09 19:34:01, tschumann wrote: > your last CL reminded me of a clean-up a planned some time ago :-) Drive-by nit: <=72 characters CL title.
https://codereview.chromium.org/2558393004/diff/1/components/ntp_snippets/rem... File components/ntp_snippets/remote/remote_suggestions_provider.cc (right): https://codereview.chromium.org/2558393004/diff/1/components/ntp_snippets/rem... components/ntp_snippets/remote/remote_suggestions_provider.cc:1021: GURL image_url = FindSnippetImageUrl(suggestion_id); On 2016/12/13 20:59:09, tschumann wrote: > On 2016/12/12 09:01:11, jkrcal wrote: > > What about empty |image_url| (because the snippet does not exist any more, > etc)? > > I see it treated neither here nor in CachedImageFetcher. > > interesting! The actual behavior is still the same (we check the URL after a > failed database lookup). However, chances for that lookup to succeed are very > small, given we don't have any trace of the image. > We can probably save the database access -- I added a test for this. > > Marc, wdyt? Yup, this SGTM - I don't see what a DB lookup with an empty GURL would ever accomplish :) https://codereview.chromium.org/2558393004/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider.cc (right): https://codereview.chromium.org/2558393004/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider.cc:1018: FROM_HERE, base::Bind(callback, gfx::Image())); nit: Wrong indentation.
Marc, wanna LGTM or removed from reviewers? Jan, want to have another look?
Still lgtm https://codereview.chromium.org/2558393004/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2558393004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:1534: gfx::Image image = gfx::test::CreateImage(2, 2); A test-readability nit: Why do you create two different images (1x1 and 2x2) when in the end you only test that the returned image is empty? Would not an empty "gfx::Image image()" do the trick because you anyway EXPECT_CALL that will overwrite it?
On 2016/12/15 10:39:26, tschumann wrote: > Marc, wanna LGTM or removed from reviewers? > Jan, want to have another look? Sure, LGTM! (For the record, you could have submitted this with just Jan's approval)
https://codereview.chromium.org/2558393004/diff/40001/components/ntp_snippets... File components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc (right): https://codereview.chromium.org/2558393004/diff/40001/components/ntp_snippets... components/ntp_snippets/remote/remote_suggestions_provider_unittest.cc:1534: gfx::Image image = gfx::test::CreateImage(2, 2); On 2016/12/15 10:51:24, jkrcal wrote: > A test-readability nit: > Why do you create two different images (1x1 and 2x2) when in the end you only > test that the returned image is empty? > > Would not an empty "gfx::Image image()" do the trick because you anyway > EXPECT_CALL that will overwrite it? I did this more for the mental model to distinguish where they are coming from. If this test fails, all you need is to look at the dimension (maybe I should change that to the EXPECT statement ;-)). Made use of this in the error message.
Now it makes sense to me, thanks!
Description was changed from ========== Refactoring: Moves the image fetching and caching logic out of the RemoteSuggestionsProvider into a CachedImageFetcher class. There's a small functional change: After fetching an image from the network, the old version used to verify the suggestion still exist before storing it into the database. This is extremely unlikely to happen and even if it happens not as bad as we clear orphaned images on start-up. BUG= ========== to ========== Refactoring: Moves the image fetching and caching logic out of the RemoteSuggestionsProvider into a CachedImageFetcher class. There's a small functional change: After fetching an image from the network, the old version used to verify the suggestion still exist before storing it into the database. This is extremely unlikely to happen and even if it happens not as bad as we clear orphaned images on start-up. BUG= ==========
The CQ bit was checked by tschumann@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org, jkrcal@chromium.org Link to the patchset: https://codereview.chromium.org/2558393004/#ps80001 (title: "extend test error message")
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": 1481806494832190,
"parent_rev": "da47de0b7a21dec9dda2370f87a6506b2f703d4c", "commit_rev":
"9b3a2c10494560aa5a92c76f2afec4ba15b367d2"}
Message was sent while issue was closed.
Description was changed from ========== Refactoring: Moves the image fetching and caching logic out of the RemoteSuggestionsProvider into a CachedImageFetcher class. There's a small functional change: After fetching an image from the network, the old version used to verify the suggestion still exist before storing it into the database. This is extremely unlikely to happen and even if it happens not as bad as we clear orphaned images on start-up. BUG= ========== to ========== Refactoring: Moves the image fetching and caching logic out of the RemoteSuggestionsProvider into a CachedImageFetcher class. There's a small functional change: After fetching an image from the network, the old version used to verify the suggestion still exist before storing it into the database. This is extremely unlikely to happen and even if it happens not as bad as we clear orphaned images on start-up. BUG= Review-Url: https://codereview.chromium.org/2558393004 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Refactoring: Moves the image fetching and caching logic out of the RemoteSuggestionsProvider into a CachedImageFetcher class. There's a small functional change: After fetching an image from the network, the old version used to verify the suggestion still exist before storing it into the database. This is extremely unlikely to happen and even if it happens not as bad as we clear orphaned images on start-up. BUG= Review-Url: https://codereview.chromium.org/2558393004 ========== to ========== Refactoring: Moves the image fetching and caching logic out of the RemoteSuggestionsProvider into a CachedImageFetcher class. There's a small functional change: After fetching an image from the network, the old version used to verify the suggestion still exist before storing it into the database. This is extremely unlikely to happen and even if it happens not as bad as we clear orphaned images on start-up. BUG= Committed: https://crrev.com/8e18ae99daecab970828838db34f8e40df344f89 Cr-Commit-Position: refs/heads/master@{#438818} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/8e18ae99daecab970828838db34f8e40df344f89 Cr-Commit-Position: refs/heads/master@{#438818} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
