Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
259863007:
Local salient image storage for enhanced bookmark experiment. (Closed)
|
Created:
6 years, 7 months ago by Kibeom Kim (inactive) Modified:
6 years, 6 months ago Reviewers:
sky, tfarina, noyau (Ping after 24h), bsalomon, blundell, Scott Hess - ex-Googler, jar (doing other things) CC:
chromium-reviews, browser-components-watch_chromium.org, Ted C, Yaron Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionLocal salient image storage for enhanced bookmark experiment.
BUG=368034
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269703
Patch Set 1 #
Total comments: 4
Patch Set 2 : nits #
Total comments: 51
Patch Set 3 : addressed comments #Patch Set 4 : added image.ToUIImage(); #Patch Set 5 : rebase #Patch Set 6 : header&constructor&destructor fix #Patch Set 7 : header naming fix #Patch Set 8 : . #Patch Set 9 : added thread consistency DCHECK #Patch Set 10 : removed ios and minor fixes #Patch Set 11 : minor bookmarks.gypi clean up #
Total comments: 2
Patch Set 12 : independent bookmarks_enhanced component, added NonThreadSafe to ImageStore class #
Total comments: 2
Patch Set 13 : Changed from NonThreadSafe to ThreadChecker #
Total comments: 55
Patch Set 14 : addressed patch set 13's comment except the saving only 1x image issue #Patch Set 15 : use jpeg #Patch Set 16 : comment update #Patch Set 17 : removed unneeded header include #
Total comments: 16
Patch Set 18 : addressed patch set 17's comments #
Total comments: 6
Patch Set 19 : gyp fix #Patch Set 20 : addressed patch set 18's comments #Patch Set 21 : components_tests.gyp fix (missed gyp generation failed) #Patch Set 22 : add gtest to bookmarks_enhanced.gypi #
Total comments: 2
Patch Set 23 : TEST_IMAGE_STORE #Patch Set 24 : rebase + compiler error fixes on windows and ios #
Total comments: 2
Patch Set 25 : move image_store_unittest.cc to components_unittests target #
Total comments: 12
Patch Set 26 : patch set 25's comments #Patch Set 27 : rebase #Patch Set 28 : apply_issue step fail test #Patch Set 29 : revert test #
Total comments: 10
Patch Set 30 : patch set 29's comments #Patch Set 31 : gyp reordering #
Total comments: 3
Patch Set 32 : remove 'enhanced_bookmarks/image_store_unittest.cc', from ios black list #Patch Set 33 : gyp renaming fix #
Total comments: 2
Patch Set 34 : kBitmap::allocN32Pixels() #
Total comments: 7
Patch Set 35 : remove sql query string indent #Patch Set 36 : tools/metrics/histograms/histograms.xml #
Total comments: 2
Patch Set 37 : DISALLOW_COPY_AND_ASSIGN(ImageStore); #Patch Set 38 : windows compiler error fix #Patch Set 39 : rebase #Messages
Total messages: 83 (0 generated)
Moving ios repository enhanced bookmark code to upstream. I made only trivial changes on these files. diff: https://chromereviews.googleplex.com/39637014
Do we checkin code that has "no" callers yet? Are you planning to upload the consumers of this code in a follow up?
On 2014/04/29 02:46:50, tfarina wrote: > Do we checkin code that has "no" callers yet? Are you planning to upload the > consumers of this code in a follow up? Actually ios is already using this in their downstream, and also Android downstream will be using shortly, which is the main reason moving this code. So no chromium public code will be calling this in fact.
lgtm with nits. But then again, as I wrote this code in the first place I'm probably not the best person to review it with a critical eye :) https://codereview.chromium.org/259863007/diff/1/chrome/browser/bookmarks/enh... File chrome/browser/bookmarks/enhanced/image_store.h (right): https://codereview.chromium.org/259863007/diff/1/chrome/browser/bookmarks/enh... chrome/browser/bookmarks/enhanced/image_store.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. This probably should be 2014 as this is a new file in Chromium. https://codereview.chromium.org/259863007/diff/1/chrome/browser/bookmarks/enh... File chrome/browser/bookmarks/enhanced/image_store_ios.mm (right): https://codereview.chromium.org/259863007/diff/1/chrome/browser/bookmarks/enh... chrome/browser/bookmarks/enhanced/image_store_ios.mm:29: base::scoped_nsobject<NSData> data_; indent.
https://codereview.chromium.org/259863007/diff/1/chrome/browser/bookmarks/enh... File chrome/browser/bookmarks/enhanced/image_store.h (right): https://codereview.chromium.org/259863007/diff/1/chrome/browser/bookmarks/enh... chrome/browser/bookmarks/enhanced/image_store.h:1: // Copyright 2013 The Chromium Authors. All rights reserved. On 2014/04/29 08:17:39, noyau wrote: > This probably should be 2014 as this is a new file in Chromium. Done. https://codereview.chromium.org/259863007/diff/1/chrome/browser/bookmarks/enh... File chrome/browser/bookmarks/enhanced/image_store_ios.mm (right): https://codereview.chromium.org/259863007/diff/1/chrome/browser/bookmarks/enh... chrome/browser/bookmarks/enhanced/image_store_ios.mm:29: base::scoped_nsobject<NSData> data_; On 2014/04/29 08:17:39, noyau wrote: > indent. Done.
https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced/image_store.cc (right): https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.cc:4: #include "chrome/browser/bookmarks/enhanced/image_store.h" nit: newline between 3/4. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.cc:21: scoped_refptr<base::RefCountedMemory> bytesForImage(gfx::Image image) { BytesForImage https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.cc:27: return image.As1xPNGBytes(); Why are you only 1x image? https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.cc:31: gfx::Image imageForBytes(scoped_refptr<base::RefCountedMemory> bytes) { ImageForBytes https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.cc:40: void ImageStore::ChangeImageURL(const GURL& from, const GURL& to) { Make order match header. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced/image_store.h (right): https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:4: #ifndef CHROME_BROWSER_BOOKMARKS_ENHANCED_IMAGE_STORE_H_ Shouldn't all this code live in components now? https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:4: #ifndef CHROME_BROWSER_BOOKMARKS_ENHANCED_IMAGE_STORE_H_ nit: newline between 3/4. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:10: #include "base/files/file_path.h" nit: forward declare as much as you can. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:18: class ImageStore : public base::RefCounted<ImageStore> { What thread is all this code used from? https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:36: // image for this url. It also returns the imageUrl where the image was imageUrl->image_url https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:38: virtual gfx::Image Get(const GURL& page_url, GURL& image_url) = 0; style guide says all refs should be const. If you need to have an out param it should be a pointer. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:40: // Returns the size of the image stored for this URL or CGSizeZero if no CGSizeZero->empty size https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:45: virtual std::vector<GURL> AllKeys() = 0; From the name it isn't clear if this is intended to be all the imageurls, or all the pageurls. Change name to better reflect that. Maybe should like GetAllPageURLs. Also, I think this should take a std::vector<GURL>* https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:55: private: nit: newline between 54/55. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:58: DISALLOW_COPY_AND_ASSIGN(ImageStore); I don't believe this is necessary for a pure virtual class. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:64: class MemoryImageStore : public ImageStore { Move these into their own files. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:75: protected: nit: newline before new section headers (except for public). https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced/image_store_ios.h (right): https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store_ios.h:4: #ifndef CHROME_BROWSER_BOOKMARKS_ENHANCED_IMAGE_STORE_IOS_H_ nit: newline 3/4. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store_ios.h:12: namespace image_store_ios { Don't use a namespace in chrome. But once you move this to components you'll want a namespace. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store_ios.h:12: namespace image_store_ios { Why is this only ios? It would be a lot cleaner if you didn't make this IOS and instead had platform specific implementations of these functions. That way you're minimizing the ifdefs in image_store.cc https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store_ios.h:14: const scoped_refptr<base::RefCountedMemory> bytesForImage(gfx::Image image); Using C++ naming, eg BytesForImage. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced/image_store_unittest.cc (right): https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store_unittest.cc:6: #include "chrome/browser/bookmarks/enhanced/image_store.h" nit: this should be your first include (use same order as you do in image_store.cc). https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store_unittest.cc:20: bitmap.eraseARGB(rand()%255, rand()%255, rand()%255, rand()%255); Why the random here? Also, use spaces, eg rand() % 255. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store_unittest.cc:29: bool CompareImages(gfx::Image image_1, gfx::Image image_2) { const gfx::Image&?
https://codereview.chromium.org/259863007/diff/20001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/259863007/diff/20001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:311: 'browser/bookmarks/enhanced/image_store.cc', One more, shouldn't these only be built where it is going to be used? Same comment of the test.
https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced/image_store.cc (right): https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.cc:4: #include "chrome/browser/bookmarks/enhanced/image_store.h" On 2014/04/30 18:09:13, sky wrote: > nit: newline between 3/4. Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.cc:21: scoped_refptr<base::RefCountedMemory> bytesForImage(gfx::Image image) { On 2014/04/30 18:09:13, sky wrote: > BytesForImage Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.cc:27: return image.As1xPNGBytes(); On 2014/04/30 18:09:13, sky wrote: > Why are you only 1x image? I think above comment #16-#20 explains this issue. noyau@ : could you confirm this? https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.cc:31: gfx::Image imageForBytes(scoped_refptr<base::RefCountedMemory> bytes) { On 2014/04/30 18:09:13, sky wrote: > ImageForBytes Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.cc:40: void ImageStore::ChangeImageURL(const GURL& from, const GURL& to) { On 2014/04/30 18:09:13, sky wrote: > Make order match header. Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced/image_store.h (right): https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:4: #ifndef CHROME_BROWSER_BOOKMARKS_ENHANCED_IMAGE_STORE_H_ On 2014/04/30 18:09:13, sky wrote: > nit: newline between 3/4. Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:4: #ifndef CHROME_BROWSER_BOOKMARKS_ENHANCED_IMAGE_STORE_H_ On 2014/04/30 18:09:13, sky wrote: > Shouldn't all this code live in components now? Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:10: #include "base/files/file_path.h" On 2014/04/30 18:09:13, sky wrote: > nit: forward declare as much as you can. Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:18: class ImageStore : public base::RefCounted<ImageStore> { On 2014/04/30 18:09:13, sky wrote: > What thread is all this code used from? Thanks for pointing this out! ImageStore will be used only in BookmarkImageService class https://chrome-internal.googlesource.com/bling/chromium/+/master/ios_internal... On Android, we construct and destruct BookmarkImageService on UI thread which has |scoped_refptr<ImageStore> store_;| as a member variable, and all the function calls to |store_| will be by calling content::BrowserThread::PostBlockingPoolSequencedTask(kSequenceToken, ...) on UI thread. So potential collision would be a function call on kSequenceToken thread and BookmarkImageService's destructor on UI thread. But BookmarkImageService is KeyedService, and on Android, we don't delete it for regular profile, but only delete for incognito profile (discussed with tedchoc@). Since BookmarkImageService won't be used with incognito profile, so I think technically it doesn't go wrong. But I think it's better to make it thread safe because that relies on many subtle assumptions. noyau@ : is this correct? https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:36: // image for this url. It also returns the imageUrl where the image was On 2014/04/30 18:09:13, sky wrote: > imageUrl->image_url Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:38: virtual gfx::Image Get(const GURL& page_url, GURL& image_url) = 0; On 2014/04/30 18:09:13, sky wrote: > style guide says all refs should be const. If you need to have an out param it > should be a pointer. Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:40: // Returns the size of the image stored for this URL or CGSizeZero if no On 2014/04/30 18:09:13, sky wrote: > CGSizeZero->empty size Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:45: virtual std::vector<GURL> AllKeys() = 0; On 2014/04/30 18:09:13, sky wrote: > From the name it isn't clear if this is intended to be all the imageurls, or all > the pageurls. Change name to better reflect that. Maybe should like > GetAllPageURLs. Also, I think this should take a std::vector<GURL>* Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:55: private: On 2014/04/30 18:09:13, sky wrote: > nit: newline between 54/55. Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:58: DISALLOW_COPY_AND_ASSIGN(ImageStore); On 2014/04/30 18:09:13, sky wrote: > I don't believe this is necessary for a pure virtual class. Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:64: class MemoryImageStore : public ImageStore { On 2014/04/30 18:09:13, sky wrote: > Move these into their own files. Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:75: protected: On 2014/04/30 18:09:13, sky wrote: > nit: newline before new section headers (except for public). Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced/image_store_ios.h (right): https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store_ios.h:4: #ifndef CHROME_BROWSER_BOOKMARKS_ENHANCED_IMAGE_STORE_IOS_H_ On 2014/04/30 18:09:13, sky wrote: > nit: newline 3/4. Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store_ios.h:12: namespace image_store_ios { On 2014/04/30 18:09:13, sky wrote: > Don't use a namespace in chrome. But once you move this to components you'll > want a namespace. Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store_ios.h:12: namespace image_store_ios { On 2014/04/30 18:09:13, sky wrote: > Why is this only ios? It would be a lot cleaner if you didn't make this IOS and > instead had platform specific implementations of these functions. That way > you're minimizing the ifdefs in image_store.cc Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store_ios.h:14: const scoped_refptr<base::RefCountedMemory> bytesForImage(gfx::Image image); On 2014/04/30 18:09:13, sky wrote: > Using C++ naming, eg BytesForImage. Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced/image_store_unittest.cc (right): https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store_unittest.cc:6: #include "chrome/browser/bookmarks/enhanced/image_store.h" On 2014/04/30 18:09:13, sky wrote: > nit: this should be your first include (use same order as you do in > image_store.cc). Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store_unittest.cc:20: bitmap.eraseARGB(rand()%255, rand()%255, rand()%255, rand()%255); On 2014/04/30 18:09:13, sky wrote: > Why the random here? Also, use spaces, eg rand() % 255. Done. https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store_unittest.cc:29: bool CompareImages(gfx::Image image_1, gfx::Image image_2) { On 2014/04/30 18:09:13, sky wrote: > const gfx::Image&? Done. https://codereview.chromium.org/259863007/diff/20001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/259863007/diff/20001/chrome/chrome_browser.gy... chrome/chrome_browser.gypi:311: 'browser/bookmarks/enhanced/image_store.cc', On 2014/04/30 21:10:33, sky wrote: > One more, shouldn't these only be built where it is going to be used? Same > comment of the test. Done.
https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... File chrome/browser/bookmarks/enhanced/image_store.h (right): https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... chrome/browser/bookmarks/enhanced/image_store.h:18: class ImageStore : public base::RefCounted<ImageStore> { On 2014/05/01 19:02:26, Kibeom Kim wrote: > On 2014/04/30 18:09:13, sky wrote: > > What thread is all this code used from? > > Thanks for pointing this out! > > ImageStore will be used only in BookmarkImageService class > https://chrome-internal.googlesource.com/bling/chromium/+/master/ios_internal... > > On Android, we construct and destruct BookmarkImageService on UI thread which > has |scoped_refptr<ImageStore> store_;| as a member variable, and all the > function calls to |store_| Doesn't that mean you could potentially be destroying on the UI thread? Which would invoke closing a db on the UI thread? > content::BrowserThread::PostBlockingPoolSequencedTask(kSequenceToken, ...) on UI > thread. So potential collision would be a function call on kSequenceToken thread > and BookmarkImageService's destructor on UI thread. But BookmarkImageService is > KeyedService, and on Android, we don't delete it for regular profile, but only > delete for incognito profile (discussed with tedchoc@). Since > BookmarkImageService won't be used with incognito profile, so I think > technically it doesn't go wrong. But I think it's better to make it thread safe > because that relies on many subtle assumptions. noyau@ : is this correct?
Also, how about DCHECKs that this isn't on the UI thread?
On 2014/05/01 20:11:55, sky wrote: > https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... > File chrome/browser/bookmarks/enhanced/image_store.h (right): > > https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... > chrome/browser/bookmarks/enhanced/image_store.h:18: class ImageStore : public > base::RefCounted<ImageStore> { > On 2014/05/01 19:02:26, Kibeom Kim wrote: > > On 2014/04/30 18:09:13, sky wrote: > > > What thread is all this code used from? > > > > Thanks for pointing this out! > > > > ImageStore will be used only in BookmarkImageService class > > > https://chrome-internal.googlesource.com/bling/chromium/+/master/ios_internal... > > > > On Android, we construct and destruct BookmarkImageService on UI thread which > > has |scoped_refptr<ImageStore> store_;| as a member variable, and all the > > function calls to |store_| > > Doesn't that mean you could potentially be destroying on the UI thread? Which > would invoke closing a db on the UI thread? > > > content::BrowserThread::PostBlockingPoolSequencedTask(kSequenceToken, ...) on > UI > > thread. So potential collision would be a function call on kSequenceToken > thread > > and BookmarkImageService's destructor on UI thread. But BookmarkImageService > is > > KeyedService, and on Android, we don't delete it for regular profile, but only > > delete for incognito profile (discussed with tedchoc@). Since > > BookmarkImageService won't be used with incognito profile, so I think > > technically it doesn't go wrong. But I think it's better to make it thread > safe > > because that relies on many subtle assumptions. noyau@ : is this correct? Oh sorry, miswording. To clarify: BookmarkImageService's destruction code path is coupled with Profile's destruction. But Android doesn't destroy ProfileImpl, only OffTheRecordProfile. And we won't be using BookmarkImageService with OffTheRecordProfile, so it never gets destroyed. I'm not sure about ios, do you know Eric? I'll add DCHECKs ensuring non-UI threads on member functions.
On 2014/05/01 23:56:28, Kibeom Kim wrote: > On 2014/05/01 20:11:55, sky wrote: > > > https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... > > File chrome/browser/bookmarks/enhanced/image_store.h (right): > > > > > https://codereview.chromium.org/259863007/diff/20001/chrome/browser/bookmarks... > > chrome/browser/bookmarks/enhanced/image_store.h:18: class ImageStore : public > > base::RefCounted<ImageStore> { > > On 2014/05/01 19:02:26, Kibeom Kim wrote: > > > On 2014/04/30 18:09:13, sky wrote: > > > > What thread is all this code used from? > > > > > > Thanks for pointing this out! > > > > > > ImageStore will be used only in BookmarkImageService class > > > > > > https://chrome-internal.googlesource.com/bling/chromium/+/master/ios_internal... > > > > > > On Android, we construct and destruct BookmarkImageService on UI thread > which > > > has |scoped_refptr<ImageStore> store_;| as a member variable, and all the > > > function calls to |store_| > > > > Doesn't that mean you could potentially be destroying on the UI thread? Which > > would invoke closing a db on the UI thread? > > > > > content::BrowserThread::PostBlockingPoolSequencedTask(kSequenceToken, ...) > on > > UI > > > thread. So potential collision would be a function call on kSequenceToken > > thread > > > and BookmarkImageService's destructor on UI thread. But BookmarkImageService > > is > > > KeyedService, and on Android, we don't delete it for regular profile, but > only > > > delete for incognito profile (discussed with tedchoc@). Since > > > BookmarkImageService won't be used with incognito profile, so I think > > > technically it doesn't go wrong. But I think it's better to make it thread > > safe > > > because that relies on many subtle assumptions. noyau@ : is this correct? > > > Oh sorry, miswording. To clarify: BookmarkImageService's destruction code path > is coupled with Profile's destruction. But Android doesn't destroy ProfileImpl, > only OffTheRecordProfile. And we won't be using BookmarkImageService with > OffTheRecordProfile, so it never gets destroyed. I'm not sure about ios, do you > know Eric? > > I'll add DCHECKs ensuring non-UI threads on member functions. Yes, same on ios, the main profile is never deleted and the class is always used from the proper file thread. That's why I never spotted the DB closing on the wrong thread on destruction (BTW, all the db methods are already checking that they are invoked on the right thread).
ptal. added thread consistency checking (including destructor) and removed ios parts. I'll do ios as a seperate CL.
Ben suggested moving this code to it's own component, maybe bookmarks-enhanced or something. That way deps are easier to manage and it becomes its own thing.
https://codereview.chromium.org/259863007/diff/180001/components/bookmarks/co... File components/bookmarks/core/browser/enhanced/image_store.h (right): https://codereview.chromium.org/259863007/diff/180001/components/bookmarks/co... components/bookmarks/core/browser/enhanced/image_store.h:62: bool IsRunningThreadConsistent(); Couldn't you use base::NonThreadSafe instead?
moved to bookmarks_enhanced component https://codereview.chromium.org/259863007/diff/180001/components/bookmarks/co... File components/bookmarks/core/browser/enhanced/image_store.h (right): https://codereview.chromium.org/259863007/diff/180001/components/bookmarks/co... components/bookmarks/core/browser/enhanced/image_store.h:62: bool IsRunningThreadConsistent(); On 2014/05/04 17:27:12, noyau wrote: > Couldn't you use base::NonThreadSafe instead? Done. (didn't know NonThreadSafe, thanks!)
https://codereview.chromium.org/259863007/diff/200001/components/bookmarks_en... File components/bookmarks_enhanced/image_store.h (right): https://codereview.chromium.org/259863007/diff/200001/components/bookmarks_en... components/bookmarks_enhanced/image_store.h:19: class ImageStore : public base::NonThreadSafe, No, do not use NonThreadSafe. Instead, could you change this to have a member variable of base::ThreadChecker (composition is generally preferred over inheritance)? Please, refer to lines 56-60 from non_thread_safe.h header for my above comment. Thanks,
https://codereview.chromium.org/259863007/diff/200001/components/bookmarks_en... File components/bookmarks_enhanced/image_store.h (right): https://codereview.chromium.org/259863007/diff/200001/components/bookmarks_en... components/bookmarks_enhanced/image_store.h:19: class ImageStore : public base::NonThreadSafe, On 2014/05/05 17:42:59, tfarina wrote: > No, do not use NonThreadSafe. Instead, could you change this to have a member > variable of base::ThreadChecker (composition is generally preferred over > inheritance)? > > Please, refer to lines 56-60 from non_thread_safe.h header for my above comment. > > Thanks, Done. Thanks!
https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/image_store_unittest.cc (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:17: const SkBitmap CreateBitmap(int width, int height, int a, int r, int g, int b) { The images had randomness in them to make sure that the image will be different if it is rotated/scaled/resized in any way. Using a flat color image tends to hide to off by one pixel errors. When the image is not symmetrical in any dimension, those issues are immediately visible. The iOS test uses an off centered circular gradient image from one color to another as it's really easy to do with UIImage.
https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/image_store_unittest.cc (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:17: const SkBitmap CreateBitmap(int width, int height, int a, int r, int g, int b) { On 2014/05/05 18:09:50, noyau wrote: > The images had randomness in them to make sure that the image will be different > if it is rotated/scaled/resized in any way. Using a flat color image tends to > hide to off by one pixel errors. When the image is not symmetrical in any > dimension, those issues are immediately visible. The iOS test uses an off > centered circular gradient image from one color to another as it's really easy > to do with UIImage. IIRC, I think the original code randomly chose one color to clear. I thought that should be enough since this doesn't perform any image manipulation. Do you think it's worth to have a non-symmetrical image for testing? (I can do it)
https://codereview.chromium.org/259863007/diff/210001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/259863007/diff/210001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:32: '../components/components.gyp:bookmarks_enhanced', Remove the conditionals from components/bookmarks_enhanced.gypi and instead only pull in that file for ios/android. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced.gypi (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced.gypi:20: ['OS=="android"', { I thought this code was for iOS too? https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced.gypi:26: 'bookmarks_enhanced/memory_image_store.cc', If memory_image_store is only for tests it should only be built in the test target. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/image_store.h (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store.h:18: // The ImageStore keeps an image for each URL. Document thread expectations. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store.h:39: virtual gfx::Image Get(const GURL& page_url, GURL* image_url) = 0; Wouldn't it be easier to make this return a std::Pair rather than returning both a value and a parameter? https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store.h:45: // Populates |url| with all the urls that have an image in the store. nit: |url|->|urls| and you've got two spaces after with https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store.h:46: virtual void GetAllPageUrls(std::vector<GURL>* urls) = 0; vector->set https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/image_store_unittest.cc (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:82: EXPECT_TRUE(success); Seems like this should be an ASSERT. And I would move it to SetUp. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:93: virtual ~ImageStoreUnitTest() {} destructor should be right after constructor. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:98: }; DISALOW... https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:110: EXPECT_EQ(0ul, all_urls.size()); You shouldn't need the 'l' in any of these, '0u' should be fine. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/image_store_util.h (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_util.h:13: namespace image_store_util { Namespaces generally should not match files. Instead use a namespace of bookmarks_enhanced. In fact all this code should be in bookmark_enhanced namespace. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_util.h:18: // Encode the UIImage representation of a gfx::Image. UIImage? https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_util.h:19: scoped_refptr<base::RefCountedMemory> BytesForImage(gfx::Image image); const gfx::Image& https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_util.h:21: // Decode the UIImage in the bytes and returns a gfx::Image. UIImage? https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/image_store_util_android.cc (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_util_android.cc:10: return image.As1xPNGBytes(); How come you only store the 1x image? Shouldn't you store all the image reps? https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/memory_image_store.cc (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/memory_image_store.cc:13: MemoryImageStore::~MemoryImageStore() { Order should match header. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/memory_image_store.cc:67: for (ImageMap::const_iterator it = store_.begin(); it != store_.end(); ++it) { nit: no {} https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/memory_image_store.cc:84: // Removes all images. remove comment (already documented in header). https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/memory_image_store.cc:88: std::vector<GURL> all_keys; Can't all this be replaced by store_.clear()? https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/persistent_image_store.cc (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/persistent_image_store.cc:75: PersistentImageStore::~PersistentImageStore() { Order should match header. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/persistent_image_store.cc:157: int64 width = statement.ColumnInt64(0); Why the int64? gfx::Size is only int. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/persistent_image_store.cc:169: OpenDatabase(); Shouldn't you check the status every where? https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/persistent_image_store.h (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/persistent_image_store.h:36: const base::FilePath path_; nit: newline between 35/36.
didn't address saving only 1x image issue yet. https://codereview.chromium.org/259863007/diff/210001/chrome/chrome_browser.gypi File chrome/chrome_browser.gypi (right): https://codereview.chromium.org/259863007/diff/210001/chrome/chrome_browser.g... chrome/chrome_browser.gypi:32: '../components/components.gyp:bookmarks_enhanced', On 2014/05/06 16:14:20, sky wrote: > Remove the conditionals from components/bookmarks_enhanced.gypi and instead only > pull in that file for ios/android. Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced.gypi (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced.gypi:20: ['OS=="android"', { On 2014/05/06 16:14:20, sky wrote: > I thought this code was for iOS too? I'll do ios as a separate CL. Also removed ios *.mm files from this CL. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced.gypi:26: 'bookmarks_enhanced/memory_image_store.cc', On 2014/05/06 16:14:20, sky wrote: > If memory_image_store is only for tests it should only be built in the test > target. Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/image_store.h (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store.h:18: // The ImageStore keeps an image for each URL. On 2014/05/06 16:14:20, sky wrote: > Document thread expectations. Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store.h:39: virtual gfx::Image Get(const GURL& page_url, GURL* image_url) = 0; On 2014/05/06 16:14:20, sky wrote: > Wouldn't it be easier to make this return a std::Pair rather than returning both > a value and a parameter? Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store.h:45: // Populates |url| with all the urls that have an image in the store. On 2014/05/06 16:14:20, sky wrote: > nit: |url|->|urls| and you've got two spaces after with Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store.h:46: virtual void GetAllPageUrls(std::vector<GURL>* urls) = 0; On 2014/05/06 16:14:20, sky wrote: > vector->set Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/image_store_unittest.cc (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:82: EXPECT_TRUE(success); On 2014/05/06 16:14:20, sky wrote: > Seems like this should be an ASSERT. And I would move it to SetUp. Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:93: virtual ~ImageStoreUnitTest() {} On 2014/05/06 16:14:20, sky wrote: > destructor should be right after constructor. Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:98: }; On 2014/05/06 16:14:20, sky wrote: > DISALOW... Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:110: EXPECT_EQ(0ul, all_urls.size()); On 2014/05/06 16:14:20, sky wrote: > You shouldn't need the 'l' in any of these, '0u' should be fine. Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/image_store_util.h (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_util.h:13: namespace image_store_util { On 2014/05/06 16:14:20, sky wrote: > Namespaces generally should not match files. Instead use a namespace of > bookmarks_enhanced. In fact all this code should be in bookmark_enhanced > namespace. Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_util.h:18: // Encode the UIImage representation of a gfx::Image. On 2014/05/06 16:14:20, sky wrote: > UIImage? Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_util.h:19: scoped_refptr<base::RefCountedMemory> BytesForImage(gfx::Image image); On 2014/05/06 16:14:20, sky wrote: > const gfx::Image& Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_util.h:21: // Decode the UIImage in the bytes and returns a gfx::Image. On 2014/05/06 16:14:20, sky wrote: > UIImage? Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/memory_image_store.cc (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/memory_image_store.cc:13: MemoryImageStore::~MemoryImageStore() { On 2014/05/06 16:14:20, sky wrote: > Order should match header. Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/memory_image_store.cc:67: for (ImageMap::const_iterator it = store_.begin(); it != store_.end(); ++it) { On 2014/05/06 16:14:20, sky wrote: > nit: no {} Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/memory_image_store.cc:84: // Removes all images. On 2014/05/06 16:14:20, sky wrote: > remove comment (already documented in header). Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/memory_image_store.cc:88: std::vector<GURL> all_keys; On 2014/05/06 16:14:20, sky wrote: > Can't all this be replaced by store_.clear()? Actually, this function and above ChangeImageURL(...) belong to ImageStore class, my mistake. Moved to image_store.h. I guess this class can override ClearAll() and just call store_.clear(), but since this is only used for testing and not important, I'll skip it. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/persistent_image_store.cc (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/persistent_image_store.cc:75: PersistentImageStore::~PersistentImageStore() { On 2014/05/06 16:14:20, sky wrote: > Order should match header. Done. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/persistent_image_store.cc:157: int64 width = statement.ColumnInt64(0); On 2014/05/06 16:14:20, sky wrote: > Why the int64? gfx::Size is only int. Done. Also added GetSize() check in TYPED_TEST(ImageStoreUnitTest, retrieve). Eric, could you confirm this is ok? I ran the above test with, saving by |statement.BindInt64(3, image.Size().width());| and reading by |statement.ColumnInt(0);| and it passed the test. So I think this change should be fine with an existing sql file. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/persistent_image_store.cc:169: OpenDatabase(); On 2014/05/06 16:14:20, sky wrote: > Shouldn't you check the status every where? Actually, we just crash on OpenDatabase failure (#195) so I changed the return type to void. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/persistent_image_store.h (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/persistent_image_store.h:36: const base::FilePath path_; On 2014/05/06 16:14:20, sky wrote: > nit: newline between 35/36. Done.
https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/image_store_util_android.cc (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_util_android.cc:10: return image.As1xPNGBytes(); On 2014/05/06 16:14:20, sky wrote: > How come you only store the 1x image? Shouldn't you store all the image reps? I thought about it. Since this is a local image cache, I think it's sufficient to store one image scale that's closest to the current device dpi. I'll add Image::AsPNGBytes(float desired_scale); function.
https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/image_store_util_android.cc (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_util_android.cc:10: return image.As1xPNGBytes(); On 2014/05/07 09:56:30, Kibeom Kim wrote: > On 2014/05/06 16:14:20, sky wrote: > > How come you only store the 1x image? Shouldn't you store all the image reps? > > I thought about it. Since this is a local image cache, I think it's sufficient > to store one image scale that's closest to the current device dpi. I'll add > Image::AsPNGBytes(float desired_scale); function. Might a particular platform end up with more than one scale? I know this can happen on chromeos if you switch to a projector. That said, I haven't seen where you're getting the images from. If your source only ever provides one scale then there is no point in dealing with multiple (DCHECK).
https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/image_store_util_android.cc (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/image_store_util_android.cc:10: return image.As1xPNGBytes(); On 2014/05/07 16:25:30, sky wrote: > On 2014/05/07 09:56:30, Kibeom Kim wrote: > > On 2014/05/06 16:14:20, sky wrote: > > > How come you only store the 1x image? Shouldn't you store all the image > reps? > > > > I thought about it. Since this is a local image cache, I think it's sufficient > > to store one image scale that's closest to the current device dpi. I'll add > > Image::AsPNGBytes(float desired_scale); function. > > Might a particular platform end up with more than one scale? I know this can > happen on chromeos if you switch to a projector. That said, I haven't seen where > you're getting the images from. If your source only ever provides one scale then > there is no point in dealing with multiple (DCHECK). ptal. Yes actually, on Android, we'll be feeding images from BitmapFetcher, and we're not dealing with multiple scales. I added DCHECK. Also I changed to use jpeg.
https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/persistent_image_store.cc (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/persistent_image_store.cc:169: OpenDatabase(); On 2014/05/07 00:09:46, Kibeom Kim wrote: > On 2014/05/06 16:14:20, sky wrote: > > Shouldn't you check the status every where? > > Actually, we just crash on OpenDatabase failure (#195) so I changed the return > type to void. Isn't it possible that openning the database can fail? Why do you want to crash? https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... File components/bookmarks_enhanced/image_store.h (right): https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... components/bookmarks_enhanced/image_store.h:38: // Returns the image associated with this url. Returns nil if there are no nil->empty image? empty url? https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... components/bookmarks_enhanced/image_store.h:54: void ClearAll(); I think this should remain pure virtual. In particular the persistent implementation can be a lot more efficient than you currently have it. Same thing for the in memory one.
https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... File components/bookmarks_enhanced.gypi (right): https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... components/bookmarks_enhanced.gypi:20: 'bookmarks_enhanced/image_store.h', fix indentation here, only two spaces from ' https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... components/bookmarks_enhanced.gypi:27: ['OS!="ios"', { what if you invert that check? OS == ios and do sources! and keep listing image_store_util.cc above? https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... File components/bookmarks_enhanced/DEPS (right): https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... components/bookmarks_enhanced/DEPS:10: } rm this extra blank line from here and the OWNERS file. https://codereview.chromium.org/259863007/diff/290001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/259863007/diff/290001/components/components_t... components/components_tests.gyp:420: 'bookmarks_enhanced/image_store_unittest.cc', I think we keep tests grouped in the main sources list, and instead exclude it from where it does not work. If this is android specific may be it should be named image_store_android_unittest.cc? https://codereview.chromium.org/259863007/diff/290001/components/components_t... components/components_tests.gyp:421: 'bookmarks_enhanced/memory_image_store.cc', is memory_image_store.* test code only?
https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... File components/bookmarks_enhanced/memory_image_store.h (right): https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... components/bookmarks_enhanced/memory_image_store.h:13: class MemoryImageStore : public ImageStore { One more. How about naming this TestImageStore to make it clear what this is intended for.
https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/persistent_image_store.cc (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/persistent_image_store.cc:169: OpenDatabase(); On 2014/05/08 15:33:08, sky wrote: > On 2014/05/07 00:09:46, Kibeom Kim wrote: > > On 2014/05/06 16:14:20, sky wrote: > > > Shouldn't you check the status every where? > > > > Actually, we just crash on OpenDatabase failure (#195) so I changed the return > > type to void. > > Isn't it possible that openning the database can fail? Why do you want to crash? Done. (I will add failure returns for public functions as caller needs them in the future) https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... File components/bookmarks_enhanced.gypi (right): https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... components/bookmarks_enhanced.gypi:20: 'bookmarks_enhanced/image_store.h', On 2014/05/08 15:41:08, tfarina wrote: > fix indentation here, only two spaces from ' Done. https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... components/bookmarks_enhanced.gypi:27: ['OS!="ios"', { On 2014/05/08 15:41:08, tfarina wrote: > what if you invert that check? OS == ios and do sources! and keep listing > image_store_util.cc above? Done. https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... File components/bookmarks_enhanced/DEPS (right): https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... components/bookmarks_enhanced/DEPS:10: } On 2014/05/08 15:41:08, tfarina wrote: > rm this extra blank line from here and the OWNERS file. Done. https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... File components/bookmarks_enhanced/image_store.h (right): https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... components/bookmarks_enhanced/image_store.h:38: // Returns the image associated with this url. Returns nil if there are no On 2014/05/08 15:33:08, sky wrote: > nil->empty image? empty url? Done. https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... components/bookmarks_enhanced/image_store.h:54: void ClearAll(); On 2014/05/08 15:33:08, sky wrote: > I think this should remain pure virtual. In particular the persistent > implementation can be a lot more efficient than you currently have it. Same > thing for the in memory one. Done. (Also added ClearAll unittest) https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... File components/bookmarks_enhanced/memory_image_store.h (right): https://codereview.chromium.org/259863007/diff/290001/components/bookmarks_en... components/bookmarks_enhanced/memory_image_store.h:13: class MemoryImageStore : public ImageStore { On 2014/05/08 15:58:06, sky wrote: > One more. How about naming this TestImageStore to make it clear what this is > intended for. Done. https://codereview.chromium.org/259863007/diff/290001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/259863007/diff/290001/components/components_t... components/components_tests.gyp:420: 'bookmarks_enhanced/image_store_unittest.cc', On 2014/05/08 15:41:08, tfarina wrote: > I think we keep tests grouped in the main sources list, and instead exclude it > from where it does not work. If this is android specific may be it should be > named image_store_android_unittest.cc? Done. (I wasn't sure that running tests on other platforms makes sense since we only compile and link these new files on Android) https://codereview.chromium.org/259863007/diff/290001/components/components_t... components/components_tests.gyp:421: 'bookmarks_enhanced/memory_image_store.cc', On 2014/05/08 15:41:08, tfarina wrote: > is memory_image_store.* test code only? Yes, just renamed to TestImageStore.
Forgot to upload? I don't see a new patchset for the new changes.
On 2014/05/08 18:01:29, tfarina wrote: > Forgot to upload? I don't see a new patchset for the new changes. Ah, there it is! Thanks.
https://codereview.chromium.org/259863007/diff/310001/components/bookmarks_en... File components/bookmarks_enhanced.gypi (right): https://codereview.chromium.org/259863007/diff/310001/components/bookmarks_en... components/bookmarks_enhanced.gypi:20: 'bookmarks_enhanced/image_store.h', sort this list, i.e., .cc before .h https://codereview.chromium.org/259863007/diff/310001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/259863007/diff/310001/components/components_t... components/components_tests.gyp:68: 'bookmarks_enhanced/enhanced_bookmark_utils_unittest.cc', Forgot to git add this file? I only see image_store_unittest.cc in the change list. https://codereview.chromium.org/259863007/diff/310001/components/components_t... components/components_tests.gyp:70: 'bookmarks_enhanced/test_image_store.cc', Please, create a bookmarks_enhanced_test_support target (static_library) and move test_image_store there.
https://codereview.chromium.org/259863007/diff/310001/components/bookmarks_en... File components/bookmarks_enhanced.gypi (right): https://codereview.chromium.org/259863007/diff/310001/components/bookmarks_en... components/bookmarks_enhanced.gypi:20: 'bookmarks_enhanced/image_store.h', On 2014/05/08 18:07:59, tfarina wrote: > sort this list, i.e., .cc before .h Done. https://codereview.chromium.org/259863007/diff/310001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/259863007/diff/310001/components/components_t... components/components_tests.gyp:68: 'bookmarks_enhanced/enhanced_bookmark_utils_unittest.cc', On 2014/05/08 18:07:59, tfarina wrote: > Forgot to git add this file? I only see image_store_unittest.cc in the change > list. Done. (it's another CL's file depends on this but somehow leaked to here) https://codereview.chromium.org/259863007/diff/310001/components/components_t... components/components_tests.gyp:70: 'bookmarks_enhanced/test_image_store.cc', On 2014/05/08 18:07:59, tfarina wrote: > Please, create a bookmarks_enhanced_test_support target (static_library) and > move test_image_store there. Done.
missed that gyp generation failed, uploaded a new one.
LGTM https://codereview.chromium.org/259863007/diff/390001/components/bookmarks_en... File components/bookmarks_enhanced/test_image_store.h (right): https://codereview.chromium.org/259863007/diff/390001/components/bookmarks_en... components/bookmarks_enhanced/test_image_store.h:5: #ifndef COMPONENTS_BOOKMARKS_ENHANCED_MEMORY_IMAGE_STORE_H_ TEST_IMAGE_STORE
https://codereview.chromium.org/259863007/diff/390001/components/bookmarks_en... File components/bookmarks_enhanced/test_image_store.h (right): https://codereview.chromium.org/259863007/diff/390001/components/bookmarks_en... components/bookmarks_enhanced/test_image_store.h:5: #ifndef COMPONENTS_BOOKMARKS_ENHANCED_MEMORY_IMAGE_STORE_H_ On 2014/05/08 19:13:35, sky wrote: > TEST_IMAGE_STORE Done.
https://codereview.chromium.org/259863007/diff/430001/components/bookmarks_en... File components/bookmarks_enhanced.gypi (right): https://codereview.chromium.org/259863007/diff/430001/components/bookmarks_en... components/bookmarks_enhanced.gypi:46: 'bookmarks_enhanced/image_store_unittest.cc', No. The unittest.cc goes into the components_unittests target
https://codereview.chromium.org/259863007/diff/430001/components/bookmarks_en... File components/bookmarks_enhanced.gypi (right): https://codereview.chromium.org/259863007/diff/430001/components/bookmarks_en... components/bookmarks_enhanced.gypi:46: 'bookmarks_enhanced/image_store_unittest.cc', On 2014/05/08 19:57:19, tfarina wrote: > No. The unittest.cc goes into the components_unittests target Oh, I see. Done.
OWNER +blundell components/components.gyp components/bookmarks_enhanced/DEPS
https://codereview.chromium.org/259863007/diff/450001/components/bookmarks_en... File components/bookmarks_enhanced/image_store_unittest.cc (right): https://codereview.chromium.org/259863007/diff/450001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:62: scoped_refptr<ImageStore> createStore(base::ScopedTempDir& folder); CamelCase for all these functions. https://codereview.chromium.org/259863007/diff/450001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:88: virtual void SetUp() { OVERRIDE https://codereview.chromium.org/259863007/diff/450001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:94: void TearDown() OVERRIDE { virtual https://codereview.chromium.org/259863007/diff/450001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:99: bool usePersistentStore() { return shouldPersist<T>(); } bool use_persistent_store() const { ... } https://codereview.chromium.org/259863007/diff/450001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:100: void resetStore() { store_ = createStore<T>(tempDir_); } ResetStore() https://codereview.chromium.org/259863007/diff/450001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:118: TYPED_TEST(ImageStoreUnitTest, startsEmpty) { s/startsEmpty/StartsEmpty all test case names CamelCase.
https://codereview.chromium.org/259863007/diff/450001/components/bookmarks_en... File components/bookmarks_enhanced/image_store_unittest.cc (right): https://codereview.chromium.org/259863007/diff/450001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:62: scoped_refptr<ImageStore> createStore(base::ScopedTempDir& folder); On 2014/05/08 20:37:25, tfarina wrote: > CamelCase for all these functions. Done. https://codereview.chromium.org/259863007/diff/450001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:88: virtual void SetUp() { On 2014/05/08 20:37:25, tfarina wrote: > OVERRIDE Done. https://codereview.chromium.org/259863007/diff/450001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:94: void TearDown() OVERRIDE { On 2014/05/08 20:37:25, tfarina wrote: > virtual Done. https://codereview.chromium.org/259863007/diff/450001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:99: bool usePersistentStore() { return shouldPersist<T>(); } On 2014/05/08 20:37:25, tfarina wrote: > bool use_persistent_store() const { ... } Done. https://codereview.chromium.org/259863007/diff/450001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:100: void resetStore() { store_ = createStore<T>(tempDir_); } On 2014/05/08 20:37:25, tfarina wrote: > ResetStore() Done. https://codereview.chromium.org/259863007/diff/450001/components/bookmarks_en... components/bookmarks_enhanced/image_store_unittest.cc:118: TYPED_TEST(ImageStoreUnitTest, startsEmpty) { On 2014/05/08 20:37:25, tfarina wrote: > s/startsEmpty/StartsEmpty > > all test case names CamelCase. Done.
LGTM I assume that you're going to add the iOS-specific support needed in a followup? https://codereview.chromium.org/259863007/diff/530001/components/bookmarks_en... File components/bookmarks_enhanced.gypi (right): https://codereview.chromium.org/259863007/diff/530001/components/bookmarks_en... components/bookmarks_enhanced.gypi:8: 'target_name': 'bookmarks_enhanced', Nit: out of curiosity, why bookmarks_enhanced instead of enhanced_bookmarks? https://codereview.chromium.org/259863007/diff/530001/components/bookmarks_en... components/bookmarks_enhanced.gypi:30: 'bookmarks_enhanced/image_store_util.cc', nit: If this is just for Android and iOS, should this file just be named image_store_util_android.*? then you wouldn't need to do this in the gypfile, plus it would be more clear when looking at the file. https://codereview.chromium.org/259863007/diff/530001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/259863007/diff/530001/components/components_t... components/components_tests.gyp:210: 'components.gyp:bookmarks_enhanced_test_support', Add components.gyp:bookmarks_enhanced as well https://codereview.chromium.org/259863007/diff/530001/components/components_t... components/components_tests.gyp:340: ['include', '^bookmarks/'], Add bookmarks_enhanced here. https://codereview.chromium.org/259863007/diff/530001/components/components_t... components/components_tests.gyp:355: 'bookmarks_enhanced/image_store_unittest.cc', Why is this file being excluded on iOS? Should there be a TODO and bug reference here?
https://codereview.chromium.org/259863007/diff/530001/components/bookmarks_en... File components/bookmarks_enhanced.gypi (right): https://codereview.chromium.org/259863007/diff/530001/components/bookmarks_en... components/bookmarks_enhanced.gypi:8: 'target_name': 'bookmarks_enhanced', On 2014/05/09 08:17:48, blundell wrote: > Nit: out of curiosity, why bookmarks_enhanced instead of enhanced_bookmarks? Done. https://codereview.chromium.org/259863007/diff/530001/components/bookmarks_en... components/bookmarks_enhanced.gypi:30: 'bookmarks_enhanced/image_store_util.cc', On 2014/05/09 08:17:48, blundell wrote: > nit: If this is just for Android and iOS, should this file just be named > image_store_util_android.*? then you wouldn't need to do this in the gypfile, > plus it would be more clear when looking at the file. image_store_util.cc itself is not Android specific and can be used other platforms except ios, (I will add 'image_store_util.mm' in another CL). I once named image_store_util_android.cc but changed. https://codereview.chromium.org/259863007/diff/530001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/259863007/diff/530001/components/components_t... components/components_tests.gyp:210: 'components.gyp:bookmarks_enhanced_test_support', On 2014/05/09 08:17:48, blundell wrote: > Add components.gyp:bookmarks_enhanced as well Done. https://codereview.chromium.org/259863007/diff/530001/components/components_t... components/components_tests.gyp:340: ['include', '^bookmarks/'], On 2014/05/09 08:17:48, blundell wrote: > Add bookmarks_enhanced here. I'll do as a follow-up ios CL. This CL doesn't assume ios usage. https://codereview.chromium.org/259863007/diff/530001/components/components_t... components/components_tests.gyp:355: 'bookmarks_enhanced/image_store_unittest.cc', On 2014/05/09 08:17:48, blundell wrote: > Why is this file being excluded on iOS? Should there be a TODO and bug reference > here? Done.
SLGTM Thanks for the bookmarks_enhanced -> enhanced_bookmarks change; it reads much better to me. https://codereview.chromium.org/259863007/diff/570001/components/bookmarks_en... File components/bookmarks_enhanced.gypi (right): https://codereview.chromium.org/259863007/diff/570001/components/bookmarks_en... components/bookmarks_enhanced.gypi:3: # found in the LICENSE file. This file should be enhanced_bookmarks.gypi https://codereview.chromium.org/259863007/diff/570001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/259863007/diff/570001/components/components_t... components/components_tests.gyp:357: 'enhanced_bookmarks/image_store_unittest.cc', You don't actually need the exclusion here if you're not doing the inclusion of enhanced_bookmarks above fwiw.
thanks for all the reviews so far! https://codereview.chromium.org/259863007/diff/570001/components/components_t... File components/components_tests.gyp (right): https://codereview.chromium.org/259863007/diff/570001/components/components_t... components/components_tests.gyp:357: 'enhanced_bookmarks/image_store_unittest.cc', On 2014/05/09 09:49:33, blundell wrote: > You don't actually need the exclusion here if you're not doing the inclusion of > enhanced_bookmarks above fwiw. Done.
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/259863007/600001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by kkimlabs@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
components/enhanced_bookmarks/DEPS +bsalomon for adding '+third_party/skia', to DEPS +shess for adding '+sql', to DEPS
https://codereview.chromium.org/259863007/diff/600001/components/enhanced_boo... File components/enhanced_bookmarks/image_store_unittest.cc (right): https://codereview.chromium.org/259863007/diff/600001/components/enhanced_boo... components/enhanced_bookmarks/image_store_unittest.cc:19: bitmap.setConfig(SkBitmap::kARGB_8888_Config, width, height); Can you use SkBitmap::allocN32Pixels() here instead of setConfig and allocPixels? The latter two are on the way out. Otherwise, lgtm.
https://codereview.chromium.org/259863007/diff/600001/components/enhanced_boo... File components/enhanced_bookmarks/image_store_unittest.cc (right): https://codereview.chromium.org/259863007/diff/600001/components/enhanced_boo... components/enhanced_bookmarks/image_store_unittest.cc:19: bitmap.setConfig(SkBitmap::kARGB_8888_Config, width, height); On 2014/05/09 14:17:19, bsalomon wrote: > Can you use SkBitmap::allocN32Pixels() here instead of setConfig and > allocPixels? The latter two are on the way out. Otherwise, lgtm. Done.
sql/ dependency LGTM. I made some comments, but if I understand the review thread correctly, they may-or-may-not be actionable, and even if they are they can probably be handled fine in a follow-up review. So feel free to not address them now, I don't want to derail things this far into a thorough review. https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... File components/bookmarks_enhanced/persistent_image_store.cc (right): https://codereview.chromium.org/259863007/diff/210001/components/bookmarks_en... components/bookmarks_enhanced/persistent_image_store.cc:169: OpenDatabase(); On 2014/05/08 17:56:33, Kibeom Kim wrote: > On 2014/05/08 15:33:08, sky wrote: > > On 2014/05/07 00:09:46, Kibeom Kim wrote: > > > On 2014/05/06 16:14:20, sky wrote: > > > > Shouldn't you check the status every where? > > > > > > Actually, we just crash on OpenDatabase failure (#195) so I changed the > return > > > type to void. > > > > Isn't it possible that openning the database can fail? Why do you want to > crash? > > Done. (I will add failure returns for public functions as caller needs them in > the future) Random peanut-gallery comment: When adding in error returns, be mindful of what you expect the caller to do. A common pattern our code unfortunately follows is to delegate decisions to the caller, who delegates to their caller, who has no idea what to do and the database fails to open forever. Stuff stored on disk breaks because of external factors, and when you have hundreds of millions of users you just have to deal with it. https://codereview.chromium.org/259863007/diff/620001/components/enhanced_boo... File components/enhanced_bookmarks/persistent_image_store.cc (right): https://codereview.chromium.org/259863007/diff/620001/components/enhanced_boo... components/enhanced_bookmarks/persistent_image_store.cc:19: " image_url LONGVARCHAR NOT NULL," Anything matching .*CHAR.* is converted to TEXT internally. You can feel free to use variants, they'll work, but I encourage just calling a TEXT a TEXT so that people don't read assumptions into things. http://www.sqlite.org/datatype3.html https://codereview.chromium.org/259863007/diff/620001/components/enhanced_boo... components/enhanced_bookmarks/persistent_image_store.cc:31: "CREATE INDEX IF NOT EXISTS images_by_url_idx ON images_by_url(page_url)"; I cannot tell from the code if you actually want this index to contain multiple identical page_url rows, or if you want it to be UNIQUE. Most of the code looks like it's taking the first one it finds? If it's meant to be UNIQUE, it should be UNIQUE - or you could even make it the PRIMARY KEY in the table definition and not have a separate index. https://codereview.chromium.org/259863007/diff/620001/components/enhanced_boo... components/enhanced_bookmarks/persistent_image_store.cc:41: db.set_histogram_tag("BookmarkImages"); Good! The main reason I stopped in here was to make sure something like this was in here. You'll need to add a BookmarkImages line to SqliteDatabases in tools/metrics/histograms/histograms.xml, but since that will loop in Yet Another Owner, I am find if you push that to a follow-up CL. This will generate the data, the histograms.xml change is needed to see the data. https://codereview.chromium.org/259863007/diff/620001/components/enhanced_boo... components/enhanced_bookmarks/persistent_image_store.cc:89: return !!count; Another option would be query "SELECT 1 FROM images_by_url WHERE page_url = ? LIMIT 1", then return statement.Step(). I don't think it matters WRT efficiency. https://codereview.chromium.org/259863007/diff/620001/components/enhanced_boo... components/enhanced_bookmarks/persistent_image_store.cc:103: " VALUES (?, ?, ?, ?, ?)")); If you really want these indented, put it in the C++ level not the query level. It won't affect the query at all, but the extra bytes in the string don't have to be shipped to everyone in the world.
The CQ bit was checked by kkimlabs@chromium.org
The CQ bit was unchecked by kkimlabs@chromium.org
I'll do other things as a follow-up cls thanks! https://codereview.chromium.org/259863007/diff/620001/components/enhanced_boo... File components/enhanced_bookmarks/persistent_image_store.cc (right): https://codereview.chromium.org/259863007/diff/620001/components/enhanced_boo... components/enhanced_bookmarks/persistent_image_store.cc:103: " VALUES (?, ?, ?, ?, ?)")); On 2014/05/09 17:51:36, shess wrote: > If you really want these indented, put it in the C++ level not the query level. > It won't affect the query at all, but the extra bytes in the string don't have > to be shipped to everyone in the world. Done.
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/259863007/640001
+jar tools/metrics/histograms/histograms.xml
https://codereview.chromium.org/259863007/diff/620001/components/enhanced_boo... File components/enhanced_bookmarks/persistent_image_store.cc (right): https://codereview.chromium.org/259863007/diff/620001/components/enhanced_boo... components/enhanced_bookmarks/persistent_image_store.cc:41: db.set_histogram_tag("BookmarkImages"); On 2014/05/09 17:51:36, shess wrote: > Good! The main reason I stopped in here was to make sure something like this > was in here. You'll need to add a BookmarkImages line to SqliteDatabases in > tools/metrics/histograms/histograms.xml, but since that will loop in Yet Another > Owner, I am find if you push that to a follow-up CL. This will generate the > data, the histograms.xml change is needed to see the data. Done.
histograms.xml LGTM ...also.... nit below as drive by (but I didn't read the other code carefully). https://codereview.chromium.org/259863007/diff/650001/components/enhanced_boo... File components/enhanced_bookmarks/image_store.h (right): https://codereview.chromium.org/259863007/diff/650001/components/enhanced_boo... components/enhanced_bookmarks/image_store.h:63: }; nit: DISALLOW_COPY_AND_ASSIGN (...or is there a special reason not to??)
https://codereview.chromium.org/259863007/diff/650001/components/enhanced_boo... File components/enhanced_bookmarks/image_store.h (right): https://codereview.chromium.org/259863007/diff/650001/components/enhanced_boo... components/enhanced_bookmarks/image_store.h:63: }; On 2014/05/10 00:07:08, jar wrote: > nit: DISALLOW_COPY_AND_ASSIGN (...or is there a special reason not to??) Done.
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/259863007/670001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/259863007/670001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) win_chromium_compile_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/259863007/690001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
The CQ bit was checked by kkimlabs@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kkimlabs@chromium.org/259863007/710001
Message was sent while issue was closed.
Change committed as 269703 |
