Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15295018:
Continue bitmap fetching for notifications. (Closed)
|
Created:
7 years, 7 months ago by Pete Williamson Modified:
7 years, 6 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionContinue bitmap fetching for notifications.
This continues the bitmap fetching for notifications feature by adding the actual bitmap URL fetching and decoding.
This checkin adds a new class, NotificationBitmapFetcher,
which is responsible for fetching the URL of the bitmap to
get the bitmap bits, and decoding them into an SkBitmap for
eventual passing to the Rich Notifications system.
Along with the class I add unit tests (for testing the synchronous parts) and browser tests (for testing the async parts) of the checkin.
The current approach uses URLFetcher and ImageDecoder (on a different thread). The call flow is first to call the URLFetcher, and when it returns async, to call the ImageDecoder in a different process so that any bitmap decoding errors do not result in a security hole.
BUG=241829
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203566
Patch Set 1 #Patch Set 2 : Synced Notification bitmap fetching - implementation and tests #Patch Set 3 : Synced Notifications Bitmap Fetching improve unit tests #
Total comments: 15
Patch Set 4 : Synced Notification Bitmap Fetching - Address DCheng CR feedback #
Total comments: 18
Patch Set 5 : Synced Notification Bitmap Fetching - refactor to use delegates instead of notifications #
Total comments: 52
Patch Set 6 : Synced Notification Bitmap Fetching - refactor unit tests into browser test #
Total comments: 22
Patch Set 7 : Synced Notification Bitmap Fetching - more DCheng feedback, change OnURLFetchCompelte test to a Sta… #
Total comments: 6
Patch Set 8 : Synced Notification Bitmap Fetching - more DCheng feedback, change TestURLFetcher to FakeURLFetcher… #
Total comments: 23
Patch Set 9 : Synced Notification Bitmap Fetching - DCheng and MikeT feedback, make NotificationBitmapFetcher no … #
Total comments: 7
Patch Set 10 : Synced Notification Bitmap Fetching - minor CR feedback, added message loops for browser tests. #Patch Set 11 : Synced Notifications Bitmap Fetching - fix case of include file name. #Patch Set 12 : Synced Notification Bitmap Fetching - fix another case sensitive filename #Patch Set 13 : Synced Notification Bitmap Fetching - Linux Compile Fix #
Messages
Total messages: 30 (0 generated)
Reviewers: Look for TODO(reviewers) for a few questions on the proper approach as I am still learning the chrome way to do things. Dmitry, Mike, please look at everything. Later when you are satisfied, I will add thakis@ to look at chrome_notifications.h and the .gypi files.
https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc (right): https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:16: GURL& url, url doesn't seem to be used. https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:18: scoped_refptr<base::MessageLoopProxy> task_runner) : task_runner doesn't seem to be used. https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:21: bitmap_(NULL) {} Explicit initialization of scoped_ptr with NULL won't work for much longer. Prefer the default ctor instead. https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:91: // TODO(reviewers): what is the proper thread to use? I picked IO arbitrarily. When in doubt, look at how other code does it =) Most of the other code seems to just pass a proxy to the message loop for the UI thread. I'd probably just do that, since that means we don't need to bounce between threads when handling the result and it makes it a bit simpler. https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_unittest.cc (right): https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_unittest.cc:32: class StubURLFetcher : public net::URLFetcher { Is it possible to use TestURLFetcher instead? If it's not, there should be a comment why here. https://codereview.chromium.org/15295018/diff/6001/chrome/common/chrome_notif... File chrome/common/chrome_notification_types.h (right): https://codereview.chromium.org/15295018/diff/6001/chrome/common/chrome_notif... chrome/common/chrome_notification_types.h:707: NOTIFICATION_NOTIFY_BITMAP_FETCH_COMPLETE, Is it possible to make this change without adding a notification?
I'd mostly echo Daniel's feedback, so I'll remove myself from the list now... His nod is sufficient.
I'd mostly echo Daniel's feedback, so I'll remove myself from the list now... His nod is sufficient.
Address DCheng CR feedback. I also added back the DCHECK for correct threading in the NotificationBitmapFetcher, and fixed the Unit tests to accomodate it. We still need to identify the right thread to use for decoding bitmaps. Removed Dimich@ as a reviewer per his request https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc (right): https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:16: GURL& url, On 2013/05/22 19:38:35, dcheng wrote: > url doesn't seem to be used. url is assigned to url_, which we fetch. url_, however was not being used, which was a bug, fixed now. https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:18: scoped_refptr<base::MessageLoopProxy> task_runner) : On 2013/05/22 19:38:35, dcheng wrote: > task_runner doesn't seem to be used. removed. https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:21: bitmap_(NULL) {} On 2013/05/22 19:38:35, dcheng wrote: > Explicit initialization of scoped_ptr with NULL won't work for much longer. > Prefer the default ctor instead. Done. https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:91: // TODO(reviewers): what is the proper thread to use? I picked IO arbitrarily. On 2013/05/22 19:38:35, dcheng wrote: > When in doubt, look at how other code does it =) > > Most of the other code seems to just pass a proxy to the message loop for the UI > thread. I'd probably just do that, since that means we don't need to bounce > between threads when handling the result and it makes it a bit simpler. The whole point of this is to get off the Browser thread, on the theory that any crashing flaw in our bitmap decoders becomes an exploit in the browser. We would much prefer the exploit to happen in a sandboxed process if one should occur. https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_unittest.cc (right): https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_unittest.cc:32: class StubURLFetcher : public net::URLFetcher { On 2013/05/22 19:38:35, dcheng wrote: > Is it possible to use TestURLFetcher instead? If it's not, there should be a > comment why here. Done. https://codereview.chromium.org/15295018/diff/6001/chrome/common/chrome_notif... File chrome/common/chrome_notification_types.h (right): https://codereview.chromium.org/15295018/diff/6001/chrome/common/chrome_notif... chrome/common/chrome_notification_types.h:707: NOTIFICATION_NOTIFY_BITMAP_FETCH_COMPLETE, On 2013/05/22 19:38:35, dcheng wrote: > Is it possible to make this change without adding a notification? I will need to listen to this notification in the client code when I write it. I will definitely need some notification, let me know if you think it would be wise to reuse an existing notification for this, but my suspicion is that would be unwise.
https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc (right): https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:91: // TODO(reviewers): what is the proper thread to use? I picked IO arbitrarily. On 2013/05/23 16:47:29, Pete Williamson wrote: > On 2013/05/22 19:38:35, dcheng wrote: > > When in doubt, look at how other code does it =) > > > > Most of the other code seems to just pass a proxy to the message loop for the > UI > > thread. I'd probably just do that, since that means we don't need to bounce > > between threads when handling the result and it makes it a bit simpler. > > The whole point of this is to get off the Browser thread, on the theory that any > crashing flaw in our bitmap decoders becomes an exploit in the browser. We > would much prefer the exploit to happen in a sandboxed process if one should > occur. The security that this provides comes from the fact that it runs in a separate *process*. It's OK to handle the callbacks on the UI thread, as long as you're not doing anything blocking. https://codereview.chromium.org/15295018/diff/6001/chrome/common/chrome_notif... File chrome/common/chrome_notification_types.h (right): https://codereview.chromium.org/15295018/diff/6001/chrome/common/chrome_notif... chrome/common/chrome_notification_types.h:707: NOTIFICATION_NOTIFY_BITMAP_FETCH_COMPLETE, On 2013/05/23 16:47:29, Pete Williamson wrote: > On 2013/05/22 19:38:35, dcheng wrote: > > Is it possible to make this change without adding a notification? > > I will need to listen to this notification in the client code when I write it. > I will definitely need some notification, let me know if you think it would be > wise to reuse an existing notification for this, but my suspicion is that would > be unwise. I would suggest using the delegate or observer pattern instead, like URLFetcher does. As a whole, I believe we're trying our usage of notifications throughout the code base where it makes sense. https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc (right): https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:33: if (url_fetcher_ == NULL) Why check for NULL here? Is it permissible to call StartImageFetch() multiple times on the same instance? If so, why is line 36 not similarly protected? https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:104: DCHECK(source == url_fetcher_.get()); DCHECK_EQ https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:106: progress_current_ = current; It doesn't seem like anything uses these values outside debugging, so please remove before checking in. https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h (right): https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:30: explicit NotificationBitmapFetcher(GURL& url); Style: Chromium does not allow mutable references. https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:33: bool image_ready(); Style: Simple getters should be inlined into the header (and should be marked const). https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:38: SkBitmap* bitmap(); Prefer returning const pointers/references. Note that by using a delegate pattern, none of these getters will be needed though. https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:22: uint32_t kMaxGreen = 0xFF0000FF; Minor nit: Chrome prefers the use of the integer types defined in basictypes.h, without the _t suffix. In the long term, there will be a cleanup to use stdint.h (see https://groups.google.com/a/chromium.org/forum/?fromgroups#!topic/chromium-de... and http://crbug.com/138542), but that has not been done yet. https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:27: class NotificationBitmapFetcherBrowserTest : public InProcessBrowserTest { The convention is to use a typedef in this case. However, I'm not quite sure I understand why both a browser test and a unit test are required. https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_unittest.cc (right): https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_unittest.cc:23: class HttpRequestHeaders; Style: the contents of a namespace should not be indented.
Refactored to use delegates instead of notification observers as requested by DCheng. As part of the refactor, I moved some tests which no longer need to be async from browser tests to unit tests. https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc (right): https://codereview.chromium.org/15295018/diff/6001/chrome/browser/notificatio... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:91: // TODO(reviewers): what is the proper thread to use? I picked IO arbitrarily. On 2013/05/23 19:32:25, dcheng wrote: > On 2013/05/23 16:47:29, Pete Williamson wrote: > > On 2013/05/22 19:38:35, dcheng wrote: > > > When in doubt, look at how other code does it =) > > > > > > Most of the other code seems to just pass a proxy to the message loop for > the > > UI > > > thread. I'd probably just do that, since that means we don't need to bounce > > > between threads when handling the result and it makes it a bit simpler. > > > > The whole point of this is to get off the Browser thread, on the theory that > any > > crashing flaw in our bitmap decoders becomes an exploit in the browser. We > > would much prefer the exploit to happen in a sandboxed process if one should > > occur. > > The security that this provides comes from the fact that it runs in a separate > *process*. It's OK to handle the callbacks on the UI thread, as long as you're > not doing anything blocking. Oh, I thought this task runner was telling the ImageDecoder what thread to run on, not what thread to make the callback on. Changed it to UI since that is where I want the callback. https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc (right): https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:33: if (url_fetcher_ == NULL) On 2013/05/23 19:32:25, dcheng wrote: > Why check for NULL here? Is it permissible to call StartImageFetch() multiple > times on the same instance? If so, why is line 36 not similarly protected? This is to work with the SetURLFetcherForTest() function. Tests will pass in a TestURLFetcher, while production code will use a regular URLFetcher. To make it easier to use the function. I did not make a second check for NULL because we throw on out of memory, if I recall correctly. https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:104: DCHECK(source == url_fetcher_.get()); On 2013/05/23 19:32:25, dcheng wrote: > DCHECK_EQ Removed the check https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:106: progress_current_ = current; On 2013/05/23 19:32:25, dcheng wrote: > It doesn't seem like anything uses these values outside debugging, so please > remove before checking in. Done. https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h (right): https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:30: explicit NotificationBitmapFetcher(GURL& url); On 2013/05/23 19:32:25, dcheng wrote: > Style: Chromium does not allow mutable references. Done. https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:33: bool image_ready(); On 2013/05/23 19:32:25, dcheng wrote: > Style: Simple getters should be inlined into the header (and should be marked > const). removed. https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:38: SkBitmap* bitmap(); On 2013/05/23 19:32:25, dcheng wrote: > Prefer returning const pointers/references. Note that by using a delegate > pattern, none of these getters will be needed though. Removed. https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:22: uint32_t kMaxGreen = 0xFF0000FF; On 2013/05/23 19:32:25, dcheng wrote: > Minor nit: Chrome prefers the use of the integer types defined in basictypes.h, > without the _t suffix. In the long term, there will be a cleanup to use stdint.h > (see > https://groups.google.com/a/chromium.org/forum/?fromgroups#%21topic/chromium-... > and http://crbug.com/138542), but that has not been done yet. Done. https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:27: class NotificationBitmapFetcherBrowserTest : public InProcessBrowserTest { On 2013/05/23 19:32:25, dcheng wrote: > The convention is to use a typedef in this case. However, I'm not quite sure I > understand why both a browser test and a unit test are required. Changed to use a typedef I originally tried to write just a unit test, but I had to write a browser test because we have to wait for an async operation, and I couldn't find a way to do that from a unit test. While I could move functionality from the unit test to the browser test, it seems lower cost to not bring up the browser for tests that don't need it. Let me know if you disagree and think the unit test methods should become browser test methods. https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_unittest.cc (right): https://codereview.chromium.org/15295018/diff/14001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_unittest.cc:23: class HttpRequestHeaders; On 2013/05/23 19:32:25, dcheng wrote: > Style: the contents of a namespace should not be indented. Done.
https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:7: #include "chrome/common/chrome_notification_types.h" Nit: None of the content notification headers are required any more. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:75: bitmap_.reset(new SkBitmap()); Is it still necessary to make a copy of the bitmap? https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:80: delegate_->OnFetchComplete(image_ready_, bitmap_.get()); Rather than using image_ready_, is it possible to either: - Have the delegate have two callbacks (OnFetchSucceeeded, OnFetchFailed) - Or just use the fact that the bitmap will be NULL if it failed. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:29: virtual void OnFetchComplete(const bool success, SkBitmap* bitmap) = 0; Generally, it's not required to add a const to integer type parameters. However, the SkBitmap should probably be passed as a const pointer/ref. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:50: void HandleImageFailed(); This is declared but not defined anywhere. Perhaps this is leftover from an earlier version? https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:80: GURL url_; Nit: constify this. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:82: NotificationBitmapFetcherDelegate* delegate_; Nit: NotificationBitmapFetcherDelegate* const delegate_; https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:85: void SetURLFetcherForTest(scoped_ptr<net::URLFetcher>& url_fetcher); Just make this a public method. Also, is it intentional that url_fetcher is being passed as a mutable ref to a scoped_ptr? https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:87: FRIEND_TEST_ALL_PREFIXES(NotificationBitmapFetcherTest, ImageReadyTest); Are all these friend declarations required? https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:34: ~NotificationBitmapFetcherTestDelegate() {} The destructor should be explicitly marked virtual. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:37: void OnFetchComplete(const bool success, SkBitmap* bitmap) { Overridden methods should be marked virtual OVERRIDE (#include base/compiler_specific.h for OVERRIDE). https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:43: bool success() { return success_; } const. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:44: SkBitmap* bitmap() { return bitmap_; } Add const where appropriate. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:48: SkBitmap* bitmap_; It's probably safer to have this be SkBitmap bitmap_; to prevent potential lifetime issues in the future. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:73: EXPECT_EQ(kMaxGreen, image.getColor(0, 0)); These EXPECT statements seem out of place. I wouldn't expect a test to verify input data. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:88: ASSERT_TRUE(gfx::PNGCodec::Encode(&*data.begin(), Instead of doing this, just use gfx::PNGCodec::EncodeBGRASkBitmap. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:113: net::TestURLFetcher* stub_url_fetcher = Make this a scoped_ptr. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:121: fetcher->OnURLFetchComplete(stub_url_fetcher); Maybe just invoke Start() https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:129: SkBitmap* found_image = delegate.bitmap(); Consider just using BitmapsAreEqual from ui/gfx/skia_util.h. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_unittest.cc (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_unittest.cc:65: TEST_F(NotificationBitmapFetcherTest, OnImageDecodedTest) { It feels strange to have the functional tests split between unit tests and browser tests. Since the fetcher uses ImageDecoder (which requires a utility process), it seems logical to just make these all browser tests. In particular, the tests feel too tightly coupled since it directly invokes various URLFetcherDelegate and ImageDecoder::Delegate callbacks.
Addressed DCheng feedback. I've replaced the "success" parameter in the callback with a "URL" parameter when I realized that I needed a way to know which bitmap is being returned. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:7: #include "chrome/common/chrome_notification_types.h" On 2013/05/28 20:11:34, dcheng wrote: > Nit: None of the content notification headers are required any more. Done. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:75: bitmap_.reset(new SkBitmap()); On 2013/05/28 20:11:34, dcheng wrote: > Is it still necessary to make a copy of the bitmap? I think so. I don't know that ImageDecoder provides us any lifetime guarantees of the bitmap, and copying it is the only way I can see to make sure the bits are still valid later when we try to use them. Looking at the source code, ImageDecoder gets a callback when the image decoding is done with a bitmap reference on the callback, and it just passes along that reference to me. That implies that the data is coming from another thread asynchronously, and there is no guarantee that the other thread will keep the data alive longer than the span of the callback. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:80: delegate_->OnFetchComplete(image_ready_, bitmap_.get()); On 2013/05/28 20:11:34, dcheng wrote: > Rather than using image_ready_, is it possible to either: > - Have the delegate have two callbacks (OnFetchSucceeeded, OnFetchFailed) > - Or just use the fact that the bitmap will be NULL if it failed. Done (using NULL) https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:29: virtual void OnFetchComplete(const bool success, SkBitmap* bitmap) = 0; On 2013/05/28 20:11:34, dcheng wrote: > Generally, it's not required to add a const to integer type parameters. However, > the SkBitmap should probably be passed as a const pointer/ref. Done. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:50: void HandleImageFailed(); On 2013/05/28 20:11:34, dcheng wrote: > This is declared but not defined anywhere. Perhaps this is leftover from an > earlier version? Yep. Removed. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:80: GURL url_; On 2013/05/28 20:11:34, dcheng wrote: > Nit: constify this. Done. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:82: NotificationBitmapFetcherDelegate* delegate_; On 2013/05/28 20:11:34, dcheng wrote: > Nit: NotificationBitmapFetcherDelegate* const delegate_; Done. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:85: void SetURLFetcherForTest(scoped_ptr<net::URLFetcher>& url_fetcher); On 2013/05/28 20:11:34, dcheng wrote: > Just make this a public method. Also, is it intentional that url_fetcher is > being passed as a mutable ref to a scoped_ptr? Done. Yes, it is intentional. We need to override the url_fetcher member variable after construction, we can't do that if the member variable is const. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:87: FRIEND_TEST_ALL_PREFIXES(NotificationBitmapFetcherTest, ImageReadyTest); On 2013/05/28 20:11:34, dcheng wrote: > Are all these friend declarations required? Removed. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:34: ~NotificationBitmapFetcherTestDelegate() {} On 2013/05/28 20:11:34, dcheng wrote: > The destructor should be explicitly marked virtual. Done. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:37: void OnFetchComplete(const bool success, SkBitmap* bitmap) { On 2013/05/28 20:11:34, dcheng wrote: > Overridden methods should be marked virtual OVERRIDE (#include > base/compiler_specific.h for OVERRIDE). Done. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:43: bool success() { return success_; } On 2013/05/28 20:11:34, dcheng wrote: > const. Done. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:44: SkBitmap* bitmap() { return bitmap_; } On 2013/05/28 20:11:34, dcheng wrote: > Add const where appropriate. Done. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:48: SkBitmap* bitmap_; On 2013/05/28 20:11:34, dcheng wrote: > It's probably safer to have this be SkBitmap bitmap_; to prevent potential > lifetime issues in the future. Done. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:73: EXPECT_EQ(kMaxGreen, image.getColor(0, 0)); On 2013/05/28 20:11:34, dcheng wrote: > These EXPECT statements seem out of place. I wouldn't expect a test to verify > input data. Removed. My original thinking was that this was fragile code, so I wanted to be sure that it did what I expected it to before moving on. Now that it reliably does what I expect, I feel safe removing it. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:88: ASSERT_TRUE(gfx::PNGCodec::Encode(&*data.begin(), On 2013/05/28 20:11:34, dcheng wrote: > Instead of doing this, just use gfx::PNGCodec::EncodeBGRASkBitmap. Done. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:113: net::TestURLFetcher* stub_url_fetcher = On 2013/05/28 20:11:34, dcheng wrote: > Make this a scoped_ptr. If I do, won't it get double deleted, since url_fetcher is a scoped ptr which is cloned from this one? url_fetcher has to be a scoped ptr so I can pass it, but I need to have a reference to it anyway so I can pass it to OnURLFetchComplete(). I'm definitely open to changing this, but this is the simplest working design that I could see. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:121: fetcher->OnURLFetchComplete(stub_url_fetcher); On 2013/05/28 20:11:34, dcheng wrote: > Maybe just invoke Start() This test is not set up to be async, and Start is an async call. In this case the intent was to just test the OnURLFetchComplete in isolation. While I am moving this to a browser test which can be async, so I could call Start() instead, I feel that I get more solid testing by calling OnURLFetchComplete specifically, since verifying its behavior is the main point of this test. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:129: SkBitmap* found_image = delegate.bitmap(); On 2013/05/28 20:11:34, dcheng wrote: > Consider just using BitmapsAreEqual from ui/gfx/skia_util.h. Done. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_unittest.cc (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_unittest.cc:65: TEST_F(NotificationBitmapFetcherTest, OnImageDecodedTest) { On 2013/05/28 20:11:34, dcheng wrote: > It feels strange to have the functional tests split between unit tests and > browser tests. Since the fetcher uses ImageDecoder (which requires a utility > process), it seems logical to just make these all browser tests. > > In particular, the tests feel too tightly coupled since it directly invokes > various URLFetcherDelegate and ImageDecoder::Delegate callbacks. OK, I can easily make these browser tests. I'm not sure I understand the problem with coupling though, unit tests are often tightly coupled to the code to get effective and direct test coverage. In what way are you thinking it can be improved?
> I've replaced the "success" parameter in the callback with a "URL" parameter > when I realized that I needed a way to know which bitmap is being returned. I don't think that URL should be a callback parameter. A real implementation of the delegate should have some bound information so it knows which notification its associated with. Depending on the URL to map back to the right notification won't work well if multiple notifications specify the same image. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:75: bitmap_.reset(new SkBitmap()); On 2013/05/29 18:05:00, Pete Williamson wrote: > On 2013/05/28 20:11:34, dcheng wrote: > > Is it still necessary to make a copy of the bitmap? > > I think so. I don't know that ImageDecoder provides us any lifetime guarantees > of the bitmap, and copying it is the only way I can see to make sure the bits > are still valid later when we try to use them. > > Looking at the source code, ImageDecoder gets a callback when the image decoding > is done with a bitmap reference on the callback, and it just passes along that > reference to me. That implies that the data is coming from another thread > asynchronously, and there is no guarantee that the other thread will keep the > data alive longer than the span of the callback. > Can we just use SkBitmap's operator=? https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:85: void SetURLFetcherForTest(scoped_ptr<net::URLFetcher>& url_fetcher); On 2013/05/29 18:05:00, Pete Williamson wrote: > On 2013/05/28 20:11:34, dcheng wrote: > > Just make this a public method. Also, is it intentional that url_fetcher is > > being passed as a mutable ref to a scoped_ptr? > > Done. > > Yes, it is intentional. We need to override the url_fetcher member variable > after construction, we can't do that if the member variable is const. Yes, but why does the argument have to a mutable reference? https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:113: net::TestURLFetcher* stub_url_fetcher = On 2013/05/29 18:05:00, Pete Williamson wrote: > On 2013/05/28 20:11:34, dcheng wrote: > > Make this a scoped_ptr. > > If I do, won't it get double deleted, since url_fetcher is a scoped ptr which is > cloned from this one? url_fetcher has to be a scoped ptr so I can pass it, but > I need to have a reference to it anyway so I can pass it to > OnURLFetchComplete(). I'm definitely open to changing this, but this is the > simplest working design that I could see. If the test uses Start() instead of OnURLFetchComplete(), then this won't be a problem. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:121: fetcher->OnURLFetchComplete(stub_url_fetcher); On 2013/05/29 18:05:00, Pete Williamson wrote: > On 2013/05/28 20:11:34, dcheng wrote: > > Maybe just invoke Start() > > This test is not set up to be async, and Start is an async call. In this case > the intent was to just test the OnURLFetchComplete in isolation. While I am > moving this to a browser test which can be async, so I could call Start() > instead, I feel that I get more solid testing by calling OnURLFetchComplete > specifically, since verifying its behavior is the main point of this test. That means that Start() is never tested--and it might end up doing nothing. Now that the tests are browser tests, I think all the tests should be invoking Start(). Note that ImageDecoder isn't currently very testable; to fix that, we should ideally have something like what URLFetcher does. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_unittest.cc (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_unittest.cc:65: TEST_F(NotificationBitmapFetcherTest, OnImageDecodedTest) { On 2013/05/29 18:05:00, Pete Williamson wrote: > On 2013/05/28 20:11:34, dcheng wrote: > > It feels strange to have the functional tests split between unit tests and > > browser tests. Since the fetcher uses ImageDecoder (which requires a utility > > process), it seems logical to just make these all browser tests. > > > > In particular, the tests feel too tightly coupled since it directly invokes > > various URLFetcherDelegate and ImageDecoder::Delegate callbacks. > > OK, I can easily make these browser tests. I'm not sure I understand the > problem with coupling though, unit tests are often tightly coupled to the code > to get effective and direct test coverage. In what way are you thinking it can > be improved? I think there are two potential pitfalls with this approach: 1) The tests won't catch problems in Start() because they simply don't invoke it. 2) The code paths taken by the test differ from those of real code. If image decoding failed, the bitmap fetcher would receive a OnURLFetchComplete callback followed by a callback to OnDecodeImageFailed. But in the HandleImageFailedTest, the testing code simply invokes OnDecodeImageFailed directly. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc (right): https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:45: OnDecodeImageFailed(NULL); Return? https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:67: // This comes in on another thread, so we send the bitmap back to the UI No longer accurate. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h (right): https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:10: #include "base/message_loop/message_loop_proxy.h" This isn't used in the header. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:21: class NotificationBitmapFetcherBrowserTest; Not needed. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:23: const bool ASYNC_CALL = true; Note that naming convention for constants is kFoo. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:32: explicit NotificationBitmapFetcherTestDelegate(bool async = false) No default args. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:42: bitmap_ = *bitmap; I suspect operator= is sufficient. Invoking deepCopyTo would be redundant. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:89: std::string image_string; std::string image_string(compressed.begin(), compressed.end()) https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:155: EXPECT_EQ(8, delegate.bitmap().getSize()); Will gfx::BitmapsAreEqual work here?
More CR feedback and answers for DCheng. I removed the URL parameter from the callback, but I may re-introduce it or something like it in another checkin. While I could have one delegate per bitmap fetcher, I prefer to have a parent class be the delegate for all of them and disambiguate at the parent. (I may pass an array index instead so I don't have to do string matching). https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:75: bitmap_.reset(new SkBitmap()); On 2013/05/29 20:40:29, dcheng wrote: > On 2013/05/29 18:05:00, Pete Williamson wrote: > > On 2013/05/28 20:11:34, dcheng wrote: > > > Is it still necessary to make a copy of the bitmap? > > > > I think so. I don't know that ImageDecoder provides us any lifetime > guarantees > > of the bitmap, and copying it is the only way I can see to make sure the bits > > are still valid later when we try to use them. > > > > Looking at the source code, ImageDecoder gets a callback when the image > decoding > > is done with a bitmap reference on the callback, and it just passes along that > > reference to me. That implies that the data is coming from another thread > > asynchronously, and there is no guarantee that the other thread will keep the > > data alive longer than the span of the callback. > > > > Can we just use SkBitmap's operator=? I tried this without the additional call to deep copy, and it failed for me. operator= makes a shallow copy, which works well if something is holding the bits in memory until you need them. However, in this case, that object is in another process and there are no guarantees that I can find that it will always keep the bitmap as long as I want it. I took a look at the code, and found that operator= just does a memcpy of the skbitmap object, including the pointer to the allocated memory where the pixels are stored, which did not appear to me to be a shared_ptr. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:85: void SetURLFetcherForTest(scoped_ptr<net::URLFetcher>& url_fetcher); On 2013/05/29 20:40:29, dcheng wrote: > On 2013/05/29 18:05:00, Pete Williamson wrote: > > On 2013/05/28 20:11:34, dcheng wrote: > > > Just make this a public method. Also, is it intentional that url_fetcher is > > > being passed as a mutable ref to a scoped_ptr? > > > > Done. > > > > Yes, it is intentional. We need to override the url_fetcher member variable > > after construction, we can't do that if the member variable is const. > > Yes, but why does the argument have to a mutable reference? My thinking is that if this is a const reference, we will be unable to assign it to the non-const member variable without a const_cast, which I would rather avoid. I would also prefer to keep the existing test even when we add a new test for Start, I think the existing test provides value. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:113: net::TestURLFetcher* stub_url_fetcher = On 2013/05/29 20:40:29, dcheng wrote: > On 2013/05/29 18:05:00, Pete Williamson wrote: > > On 2013/05/28 20:11:34, dcheng wrote: > > > Make this a scoped_ptr. > > > > If I do, won't it get double deleted, since url_fetcher is a scoped ptr which > is > > cloned from this one? url_fetcher has to be a scoped ptr so I can pass it, > but > > I need to have a reference to it anyway so I can pass it to > > OnURLFetchComplete(). I'm definitely open to changing this, but this is the > > simplest working design that I could see. > > If the test uses Start() instead of OnURLFetchComplete(), then this won't be a > problem. We now use Start, but it is still a problem. It is still a problem because the TestURLFetcher doesn't actually call the callback (OnURLFetchComplete), so I must call it manually. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:121: fetcher->OnURLFetchComplete(stub_url_fetcher); On 2013/05/29 20:40:29, dcheng wrote: > On 2013/05/29 18:05:00, Pete Williamson wrote: > > On 2013/05/28 20:11:34, dcheng wrote: > > > Maybe just invoke Start() > > > > This test is not set up to be async, and Start is an async call. In this case > > the intent was to just test the OnURLFetchComplete in isolation. While I am > > moving this to a browser test which can be async, so I could call Start() > > instead, I feel that I get more solid testing by calling OnURLFetchComplete > > specifically, since verifying its behavior is the main point of this test. > > That means that Start() is never tested--and it might end up doing nothing. > Now that the tests are browser tests, I think all the tests should be invoking > Start(). Note that ImageDecoder isn't currently very testable; to fix that, we > should ideally have something like what URLFetcher does. Done. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc (right): https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:45: OnDecodeImageFailed(NULL); On 2013/05/29 20:40:29, dcheng wrote: > Return? Good catch! Done. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:67: // This comes in on another thread, so we send the bitmap back to the UI On 2013/05/29 20:40:29, dcheng wrote: > No longer accurate. Done. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h (right): https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:10: #include "base/message_loop/message_loop_proxy.h" On 2013/05/29 20:40:29, dcheng wrote: > This isn't used in the header. Done. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:21: class NotificationBitmapFetcherBrowserTest; On 2013/05/29 20:40:29, dcheng wrote: > Not needed. Done. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:23: const bool ASYNC_CALL = true; On 2013/05/29 20:40:29, dcheng wrote: > Note that naming convention for constants is kFoo. Done. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:32: explicit NotificationBitmapFetcherTestDelegate(bool async = false) On 2013/05/29 20:40:29, dcheng wrote: > No default args. Done. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:42: bitmap_ = *bitmap; On 2013/05/29 20:40:29, dcheng wrote: > I suspect operator= is sufficient. Invoking deepCopyTo would be redundant. My testing shows that operator= is not sufficient as explained elsewhere. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:89: std::string image_string; On 2013/05/29 20:40:29, dcheng wrote: > std::string image_string(compressed.begin(), compressed.end()) Done. Thanks! I didn't realize that string would have a constructor taking the other type of character. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:155: EXPECT_EQ(8, delegate.bitmap().getSize()); On 2013/05/29 20:40:29, dcheng wrote: > Will gfx::BitmapsAreEqual work here? Done. It turns out that it only works for ARGB_8888 bitmaps, but it is easy enough to convert my bitmap to that kind.
https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:75: bitmap_.reset(new SkBitmap()); On 2013/05/30 00:13:18, Pete Williamson wrote: > On 2013/05/29 20:40:29, dcheng wrote: > > On 2013/05/29 18:05:00, Pete Williamson wrote: > > > On 2013/05/28 20:11:34, dcheng wrote: > > > > Is it still necessary to make a copy of the bitmap? > > > > > > I think so. I don't know that ImageDecoder provides us any lifetime > > guarantees > > > of the bitmap, and copying it is the only way I can see to make sure the > bits > > > are still valid later when we try to use them. > > > > > > Looking at the source code, ImageDecoder gets a callback when the image > > decoding > > > is done with a bitmap reference on the callback, and it just passes along > that > > > reference to me. That implies that the data is coming from another thread > > > asynchronously, and there is no guarantee that the other thread will keep > the > > > data alive longer than the span of the callback. > > > > > > > Can we just use SkBitmap's operator=? > > I tried this without the additional call to deep copy, and it failed for me. > operator= makes a shallow copy, which works well if something is holding the > bits in memory until you need them. However, in this case, that object is in > another process and there are no guarantees that I can find that it will always > keep the bitmap as long as I want it. I took a look at the code, and found that > operator= just does a memcpy of the skbitmap object, including the pointer to > the allocated memory where the pixels are stored, which did not appear to me to > be a shared_ptr. I see. I misinterpreted the comment (which remarks that the pixels are shared) but not catch the part which describes that ownership remains with src. Thanks for investigating this. https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:85: void SetURLFetcherForTest(scoped_ptr<net::URLFetcher>& url_fetcher); On 2013/05/30 00:13:18, Pete Williamson wrote: > On 2013/05/29 20:40:29, dcheng wrote: > > On 2013/05/29 18:05:00, Pete Williamson wrote: > > > On 2013/05/28 20:11:34, dcheng wrote: > > > > Just make this a public method. Also, is it intentional that url_fetcher > is > > > > being passed as a mutable ref to a scoped_ptr? > > > > > > Done. > > > > > > Yes, it is intentional. We need to override the url_fetcher member variable > > > after construction, we can't do that if the member variable is const. > > > > Yes, but why does the argument have to a mutable reference? > > My thinking is that if this is a const reference, we will be unable to assign it > to the non-const member variable without a const_cast, which I would rather > avoid. I would also prefer to keep the existing test even when we add a new > test for Start, I think the existing test provides value. Smart pointers aren't generally meant to be passed by reference or pointer. I think this can just be passed by value. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:42: bitmap_ = *bitmap; On 2013/05/30 00:13:18, Pete Williamson wrote: > On 2013/05/29 20:40:29, dcheng wrote: > > I suspect operator= is sufficient. Invoking deepCopyTo would be redundant. > > My testing shows that operator= is not sufficient as explained elsewhere. It seems strange to use both operator= and deepCopyTo. Perhaps reset() + deepCopyTo() is sufficient? https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:89: std::string image_string; On 2013/05/30 00:13:18, Pete Williamson wrote: > On 2013/05/29 20:40:29, dcheng wrote: > > std::string image_string(compressed.begin(), compressed.end()) > > Done. Thanks! I didn't realize that string would have a constructor taking the > other type of character. I don't see the change. https://codereview.chromium.org/15295018/diff/37001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/37001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:113: // Since the TestURLFetcher doesn't do anything when Start() is called, Oops. Perhaps use FakeURLFetcher instead, which does do this? Sorry for the confusion. https://codereview.chromium.org/15295018/diff/37001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:159: OnURLFetchFailureTest) { This test should be able to use NotificationBitmapFetcher::Start() as well. https://codereview.chromium.org/15295018/diff/37001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:195: fetcher->SetURLFetcherForTest(url_fetcher); I think it should be pretty easy to make this test flow through Start() as well. Just pass something like "1234" to ImageDecoder. That way, none of the tests need to be manually invoke callbacks, making them more robust.
More CR changes per DCheng. Primary change is to make the tests go through the URLFetcher flow by using a FakeURLFetcher instead of a TestURLFetcher, and calling fetcher->Start(); https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h (right): https://codereview.chromium.org/15295018/diff/22001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:85: void SetURLFetcherForTest(scoped_ptr<net::URLFetcher>& url_fetcher); On 2013/05/30 00:38:14, dcheng wrote: > On 2013/05/30 00:13:18, Pete Williamson wrote: > > On 2013/05/29 20:40:29, dcheng wrote: > > > On 2013/05/29 18:05:00, Pete Williamson wrote: > > > > On 2013/05/28 20:11:34, dcheng wrote: > > > > > Just make this a public method. Also, is it intentional that url_fetcher > > is > > > > > being passed as a mutable ref to a scoped_ptr? > > > > > > > > Done. > > > > > > > > Yes, it is intentional. We need to override the url_fetcher member > variable > > > > after construction, we can't do that if the member variable is const. > > > > > > Yes, but why does the argument have to a mutable reference? > > > > My thinking is that if this is a const reference, we will be unable to assign > it > > to the non-const member variable without a const_cast, which I would rather > > avoid. I would also prefer to keep the existing test even when we add a new > > test for Start, I think the existing test provides value. > > Smart pointers aren't generally meant to be passed by reference or pointer. I > think this can just be passed by value. Done. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:42: bitmap_ = *bitmap; On 2013/05/30 00:38:14, dcheng wrote: > On 2013/05/30 00:13:18, Pete Williamson wrote: > > On 2013/05/29 20:40:29, dcheng wrote: > > > I suspect operator= is sufficient. Invoking deepCopyTo would be redundant. > > > > My testing shows that operator= is not sufficient as explained elsewhere. > > It seems strange to use both operator= and deepCopyTo. Perhaps reset() + > deepCopyTo() is sufficient? OK, using just deepCopyTo seems to work, Done. https://codereview.chromium.org/15295018/diff/30001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:89: std::string image_string; On 2013/05/30 00:38:14, dcheng wrote: > On 2013/05/30 00:13:18, Pete Williamson wrote: > > On 2013/05/29 20:40:29, dcheng wrote: > > > std::string image_string(compressed.begin(), compressed.end()) > > > > Done. Thanks! I didn't realize that string would have a constructor taking > the > > other type of character. > > I don't see the change. Oops, it got undone when undoing and redoing to fix something else. I put it back. https://codereview.chromium.org/15295018/diff/37001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/37001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:113: // Since the TestURLFetcher doesn't do anything when Start() is called, On 2013/05/30 00:38:14, dcheng wrote: > Oops. Perhaps use FakeURLFetcher instead, which does do this? Sorry for the > confusion. Done. https://codereview.chromium.org/15295018/diff/37001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:159: OnURLFetchFailureTest) { On 2013/05/30 00:38:14, dcheng wrote: > This test should be able to use NotificationBitmapFetcher::Start() as well. Done. https://codereview.chromium.org/15295018/diff/37001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:195: fetcher->SetURLFetcherForTest(url_fetcher); On 2013/05/30 00:38:14, dcheng wrote: > I think it should be pretty easy to make this test flow through Start() as well. > Just pass something like "1234" to ImageDecoder. That way, none of the tests > need to be manually invoke callbacks, making them more robust. Done.
https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc (right): https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:20: if (url_fetcher_ == NULL) Style nit: Use {}s since the statement takes multiple lines. https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h (right): https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:8: #include "base/gtest_prod_util.h" This include is no longer used. https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:34: public base::RefCountedThreadSafe<NotificationBitmapFetcher> { This no longer needs to be refcounted. https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:38: NotificationBitmapFetcherDelegate* delegate); Style nit: indent is wonky. https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:39: ~NotificationBitmapFetcher(); Nit: Explicitly mark destructor virtual. Note that since this (was) a refcounted object, it should have been private as well, but since that no longer needs to be the case, it can remain here. https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:75: scoped_ptr<SkBitmap> bitmap_; Note that you don't have to include SkBitmap here since it's only being used as a template parameter to scoped_ptr. A forward declaration would suffice. In the past, the preference was to forward declare when possible and only #include the header in the implementation file when needed. The reason for this was (and still is) compilation speed, since including a header ends up including more headers which include more headers... It probably doesn't matter here but just something to keep in mind. https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:35: explicit NotificationBitmapFetcherTestDelegate(bool async) Since all tests are now async, the async_ parameters and its associated constants are no longer needed. https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:87: // Copy the bits into the string, and put them into the StubURLFetcher. Nit: FakeURLFetcher. https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:97: url, fetcher, image_string, kSuccess)); This is a personal preference, but I don't think that aliasing true/false to constants is adding to the clarity of this code. It makes it unclear what type FakeURLFetcher takes in its constructor (bool? enum? int? string? something else?). If a bool parameter is unclear in its context (which is often the case), the general convention I've seen is to put its purpose in an inline comment before or after, e.g.: new net::FakeURLFetcher(url, fetcher, image_string, /*success=*/ true) Or something along those lines.
Good tests! LGTM. https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h (right): https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:36: explicit NotificationBitmapFetcher( This doesn't need to be explicit anymore now that there are two arguments.
Nico: Please do owners review of the .gypi files. DCheng and MikeT, this should address all your feedback. https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc (right): https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.cc:20: if (url_fetcher_ == NULL) On 2013/05/30 22:38:09, dcheng wrote: > Style nit: Use {}s since the statement takes multiple lines. Done. https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h (right): https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:34: public base::RefCountedThreadSafe<NotificationBitmapFetcher> { On 2013/05/30 22:38:09, dcheng wrote: > This no longer needs to be refcounted. Done. https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:36: explicit NotificationBitmapFetcher( On 2013/05/30 22:53:20, miket wrote: > This doesn't need to be explicit anymore now that there are two arguments. Done. https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher.h:38: NotificationBitmapFetcherDelegate* delegate); On 2013/05/30 22:38:09, dcheng wrote: > Style nit: indent is wonky. Done. https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:35: explicit NotificationBitmapFetcherTestDelegate(bool async) On 2013/05/30 22:38:09, dcheng wrote: > Since all tests are now async, the async_ parameters and its associated > constants are no longer needed. We still have one sync test (and I may add more someday) https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:97: url, fetcher, image_string, kSuccess)); On 2013/05/30 22:38:09, dcheng wrote: > This is a personal preference, but I don't think that aliasing true/false to > constants is adding to the clarity of this code. It makes it unclear what type > FakeURLFetcher takes in its constructor (bool? enum? int? string? something > else?). If a bool parameter is unclear in its context (which is often the case), > the general convention I've seen is to put its purpose in an inline comment > before or after, e.g.: > new net::FakeURLFetcher(url, fetcher, image_string, /*success=*/ true) > > Or something along those lines. I think that knowing the meaning (succeeded, failed) is more important than knowing the type in this case, I'd prefer to leave the code as is.
https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:35: explicit NotificationBitmapFetcherTestDelegate(bool async) On 2013/05/31 00:17:39, Pete Williamson wrote: > On 2013/05/30 22:38:09, dcheng wrote: > > Since all tests are now async, the async_ parameters and its associated > > constants are no longer needed. > > We still have one sync test (and I may add more someday) How is StartTest different from OnImageDecodedTest? https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:97: url, fetcher, image_string, kSuccess)); On 2013/05/31 00:17:39, Pete Williamson wrote: > On 2013/05/30 22:38:09, dcheng wrote: > > This is a personal preference, but I don't think that aliasing true/false to > > constants is adding to the clarity of this code. It makes it unclear what type > > FakeURLFetcher takes in its constructor (bool? enum? int? string? something > > else?). If a bool parameter is unclear in its context (which is often the > case), > > the general convention I've seen is to put its purpose in an inline comment > > before or after, e.g.: > > new net::FakeURLFetcher(url, fetcher, image_string, /*success=*/ true) > > > > Or something along those lines. > > I think that knowing the meaning (succeeded, failed) is more important than > knowing the type in this case, I'd prefer to leave the code as is. Right, that's why I suggested putting it in comments. This is pretty common throughout Chrome; defining local bool consts is much rarer and not used in this way. Consistency is nice to have in a codebase the size of Chrome's, since having constructs like this that are unique to your code make it that much harder (even if only a little, it adds up) for other developers to parse.
Nico@ Need owners review of the .gypi files. Answers for DCheng (the fixes are minor, I'll batch them together with the next set of changes). https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:35: explicit NotificationBitmapFetcherTestDelegate(bool async) On 2013/05/31 00:27:38, dcheng wrote: > On 2013/05/31 00:17:39, Pete Williamson wrote: > > On 2013/05/30 22:38:09, dcheng wrote: > > > Since all tests are now async, the async_ parameters and its associated > > > constants are no longer needed. > > > > We still have one sync test (and I may add more someday) > > How is StartTest different from OnImageDecodedTest? The primary difference is that we are testing a different callback. Instead of testing the URL fetch, which is the first stage of processing, here we are testing the results from the second stage of processing, the bitmap decoding. There are several philosophies of unit testing. This was originally written to be a unit test trying to test this function in isolation. I think there is value in testing functions in isolation, so I'd like to leave this test as is. If you are convinced that we also need a more end-to-end test, I can certainly add one later. https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:87: // Copy the bits into the string, and put them into the StubURLFetcher. On 2013/05/30 22:38:09, dcheng wrote: > Nit: FakeURLFetcher. Done. https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:97: url, fetcher, image_string, kSuccess)); On 2013/05/31 00:27:38, dcheng wrote: > On 2013/05/31 00:17:39, Pete Williamson wrote: > > On 2013/05/30 22:38:09, dcheng wrote: > > > This is a personal preference, but I don't think that aliasing true/false to > > > constants is adding to the clarity of this code. It makes it unclear what > type > > > FakeURLFetcher takes in its constructor (bool? enum? int? string? something > > > else?). If a bool parameter is unclear in its context (which is often the > > case), > > > the general convention I've seen is to put its purpose in an inline comment > > > before or after, e.g.: > > > new net::FakeURLFetcher(url, fetcher, image_string, /*success=*/ true) > > > > > > Or something along those lines. > > > > I think that knowing the meaning (succeeded, failed) is more important than > > knowing the type in this case, I'd prefer to leave the code as is. > > Right, that's why I suggested putting it in comments. This is pretty common > throughout Chrome; defining local bool consts is much rarer and not used in this > way. Consistency is nice to have in a codebase the size of Chrome's, since > having constructs like this that are unique to your code make it that much > harder (even if only a little, it adds up) for other developers to parse. Done.
https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:35: explicit NotificationBitmapFetcherTestDelegate(bool async) On 2013/05/31 01:01:44, Pete Williamson wrote: > On 2013/05/31 00:27:38, dcheng wrote: > > On 2013/05/31 00:17:39, Pete Williamson wrote: > > > On 2013/05/30 22:38:09, dcheng wrote: > > > > Since all tests are now async, the async_ parameters and its associated > > > > constants are no longer needed. > > > > > > We still have one sync test (and I may add more someday) > > > > How is StartTest different from OnImageDecodedTest? > > The primary difference is that we are testing a different callback. Instead of > testing the URL fetch, which is the first stage of processing, here we are > testing the results from the second stage of processing, the bitmap decoding. > > There are several philosophies of unit testing. This was originally written to > be a unit test trying to test this function in isolation. I think there is > value in testing functions in isolation, so I'd like to leave this test as is. > If you are convinced that we also need a more end-to-end test, I can certainly > add one later. I'm trying to think of a situation where StartTest will pass and OnImageDecodedTest will not, and I can't think of anything. This, combined with that the code flow being tested will never occur in practice in the browser, makes it hard for me to understand why OnImageDecodedTest is needed. https://codereview.chromium.org/15295018/diff/52001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/52001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:131: fetcher.SetURLFetcherForTest(url_fetcher.Pass()); If this test remains, note that url_fetcher is completely unused. https://codereview.chromium.org/15295018/diff/52001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:161: I think this test needs to run the message loop to work properly. https://codereview.chromium.org/15295018/diff/52001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:175: Ditto.
Answers and changes per DCheng. https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:35: explicit NotificationBitmapFetcherTestDelegate(bool async) On 2013/05/31 02:34:52, dcheng wrote: > On 2013/05/31 01:01:44, Pete Williamson wrote: > > On 2013/05/31 00:27:38, dcheng wrote: > > > On 2013/05/31 00:17:39, Pete Williamson wrote: > > > > On 2013/05/30 22:38:09, dcheng wrote: > > > > > Since all tests are now async, the async_ parameters and its associated > > > > > constants are no longer needed. > > > > > > > > We still have one sync test (and I may add more someday) > > > > > > How is StartTest different from OnImageDecodedTest? > > > > The primary difference is that we are testing a different callback. Instead > of > > testing the URL fetch, which is the first stage of processing, here we are > > testing the results from the second stage of processing, the bitmap decoding. > > > > There are several philosophies of unit testing. This was originally written > to > > be a unit test trying to test this function in isolation. I think there is > > value in testing functions in isolation, so I'd like to leave this test as is. > > > If you are convinced that we also need a more end-to-end test, I can certainly > > add one later. > > I'm trying to think of a situation where StartTest will pass and > OnImageDecodedTest will not, and I can't think of anything. This, combined with > that the code flow being tested will never occur in practice in the browser, > makes it hard for me to understand why OnImageDecodedTest is needed. If start test fails, I can quickly tell by whether ImageDecocedTest also fails whether the problem was in image decoding or URL fetching. https://codereview.chromium.org/15295018/diff/52001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/52001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:131: fetcher.SetURLFetcherForTest(url_fetcher.Pass()); On 2013/05/31 02:34:52, dcheng wrote: > If this test remains, note that url_fetcher is completely unused. Good point, removed. https://codereview.chromium.org/15295018/diff/52001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:161: On 2013/05/31 02:34:52, dcheng wrote: > I think this test needs to run the message loop to work properly. Surprisingly, it passed without the message loop, but I added the message loop anyway so it will be theoretically correct, and it still passes, so I'll submit it with the message loop. https://codereview.chromium.org/15295018/diff/52001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:175: On 2013/05/31 02:34:52, dcheng wrote: > Ditto. Same response.
On 2013/05/31 00:27:37, dcheng wrote: > https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... > File > chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc > (right): > > https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... > chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:35: > explicit NotificationBitmapFetcherTestDelegate(bool async) > On 2013/05/31 00:17:39, Pete Williamson wrote: > > On 2013/05/30 22:38:09, dcheng wrote: > > > Since all tests are now async, the async_ parameters and its associated > > > constants are no longer needed. > > > > We still have one sync test (and I may add more someday) > > How is StartTest different from OnImageDecodedTest? > > https://codereview.chromium.org/15295018/diff/44001/chrome/browser/notificati... > chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:97: > url, fetcher, image_string, kSuccess)); > On 2013/05/31 00:17:39, Pete Williamson wrote: > > On 2013/05/30 22:38:09, dcheng wrote: > > > This is a personal preference, but I don't think that aliasing true/false to > > > constants is adding to the clarity of this code. It makes it unclear what > type > > > FakeURLFetcher takes in its constructor (bool? enum? int? string? something > > > else?). If a bool parameter is unclear in its context (which is often the > > case), > > > the general convention I've seen is to put its purpose in an inline comment > > > before or after, e.g.: > > > new net::FakeURLFetcher(url, fetcher, image_string, /*success=*/ true) > > > > > > Or something along those lines. > > > > I think that knowing the meaning (succeeded, failed) is more important than > > knowing the type in this case, I'd prefer to leave the code as is. > > Right, that's why I suggested putting it in comments. This is pretty common > throughout Chrome; defining local bool consts is much rarer and not used in this > way. Consistency is nice to have in a codebase the size of Chrome's, since > having constructs like this that are unique to your code make it that much > harder (even if only a little, it adds up) for other developers to parse. Just a drive-by comment on common practices: Indeed, passing literal 'true' and 'false' is a practice in Chromium (but not in Webkit). At the same time, the inline comments are not popular I think, I had reviewers asking me to remove them and simple searches like 'git grep "true);"' shows practically no inline comments used. I don't ask for any further changes in this CL, it's just a comment.
lgtm https://codereview.chromium.org/15295018/diff/52001/chrome/browser/notificati... File chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc (right): https://codereview.chromium.org/15295018/diff/52001/chrome/browser/notificati... chrome/browser/notifications/sync_notifier/notification_bitmap_fetcher_browsertest.cc:161: On 2013/05/31 16:42:20, Pete Williamson wrote: > On 2013/05/31 02:34:52, dcheng wrote: > > I think this test needs to run the message loop to work properly. > > Surprisingly, it passed without the message loop, but I added the message loop > anyway so it will be theoretically correct, and it still passes, so I'll submit > it with the message loop. success() defaults to false, so that's why it passed, even though no code was actually running.
ping thakis@ - We still need OWNERS review of the .gypi files. Thanks!
gypi lgtm. It's fine to TBR gypi changes like this, see comment in chrome/OWNERS. (Sorry for the delay, I was already out by 5:17pm yesterday when this arrived, and I was busy with interviews today until now.)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petewil@chromium.org/15295018/58001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petewil@chromium.org/15295018/74005
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_chromeos_clang. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/petewil@chromium.org/15295018/80001
Message was sent while issue was closed.
Change committed as 203566 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
