Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1801933004:
Refactor SoftwareImageDecodeController (Closed)
DescriptionRefactor SoftwareImageDecodeController to make adding support for
medium quality more readable and natural.
BUG=594839
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
Committed: https://crrev.com/4c4a73d71ba30eca9fd9b8d17f8175f462631689
Cr-Commit-Position: refs/heads/master@{#387426}
Patch Set 1 : Break DecodeImageInternal into single responsibilities. #Patch Set 2 : Switching over image quality. #Patch Set 3 : Removing recursive image decode. #Patch Set 4 : Builds now. #
Total comments: 13
Patch Set 5 : Making CR changes. #Patch Set 6 : Hopefully fixing the unit test (which isn't building locally for me). #Patch Set 7 : Typo #Patch Set 8 : Bad return type. #
Total comments: 9
Patch Set 9 : Removing ImageDecodeControllerKey's can_use_original_decode_ #Patch Set 10 : Updating unit test. #Patch Set 11 : Adding can_use_original_decode back. #
Total comments: 2
Patch Set 12 : Fixing typo in variable name in constructor declaration. #
Total comments: 15
Patch Set 13 : Code review comments. #Patch Set 14 : Fixing error in high quality encoding. #Patch Set 15 : Rebasing again. #Patch Set 16 : Adding comments. #Patch Set 17 : More clear comment wording. #Patch Set 18 : Making single-line if not use brackets. #Patch Set 19 : Removing DecodeImageHighQuality (and MediumQuality) since the calls are small enough to just have b… #Patch Set 20 : Updating comment to reflect change in code. #
Messages
Total messages: 91 (42 generated)
Description was changed from ========== Break DecodeImageInternal into single responsibilities. BUG=594839 ========== to ========== Break DecodeImageInternal into single responsibilities. BUG=594839 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== Break DecodeImageInternal into single responsibilities. BUG=594839 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Refactor SoftwareImageDecodeController to make adding support for medium quality more readable and natural. BUG=594839 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
cblume@chromium.org changed reviewers: + vmpstr@chromium.org
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
vmpstr@chromium.org changed reviewers: + ericrk@chromium.org
https://codereview.chromium.org/1801933004/diff/60001/cc/tiles/software_image... File cc/tiles/software_image_decode_controller.cc (left): https://codereview.chromium.org/1801933004/diff/60001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:110: SkColorType SkColorTypeForDecoding(ResourceFormat format) { I think you kind of lost this patch in the merge? https://codereview.chromium.org/1801933004/diff/60001/cc/tiles/software_image... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1801933004/diff/60001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:338: SoftwareImageDecodeController::DecodeImageNoneLowQuality(const ImageKey& key, Maybe we can just skip this function and use GetOriginalDecode? Wdyt? https://codereview.chromium.org/1801933004/diff/60001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:359: if (!decoded_image_result.decoded_pixmap_.addr()) { nit: Usually for single line if and a single line statement, braces are optional. https://codereview.chromium.org/1801933004/diff/60001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:465: if (!image) { When would this be the case? https://codereview.chromium.org/1801933004/diff/60001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:466: return DecodedDrawImage(nullptr, kNone_SkFilterQuality); If this is a case that we need to handle, then it needs to be handled earlier. The problem with returning here is that the unlock object will be destroyed first, which will first acquire the mutex and then the lock on line 418 will be destroyed which will unlock it again. That's kind of just mutex churn :) https://codereview.chromium.org/1801933004/diff/60001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:518: SoftwareImageDecodeController::AttemptToUseOriginalImage(const ImageKey& key, nit: Can we call this "GetOriginalImageDecode" or something like that? https://codereview.chromium.org/1801933004/diff/60001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:523: if (!key.can_use_original_decode()) { Can we make this a DCHECK? Whatever code is calling this should only call it if key.can_use_original_decode is true.
I hope I have addressed the comments. I'm about to look into why the tests are failing. https://codereview.chromium.org/1801933004/diff/60001/cc/tiles/software_image... File cc/tiles/software_image_decode_controller.cc (left): https://codereview.chromium.org/1801933004/diff/60001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:110: SkColorType SkColorTypeForDecoding(ResourceFormat format) { On 2016/03/21 20:53:40, vmpstr wrote: > I think you kind of lost this patch in the merge? Whoops. Fixed. https://codereview.chromium.org/1801933004/diff/60001/cc/tiles/software_image... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1801933004/diff/60001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:338: SoftwareImageDecodeController::DecodeImageNoneLowQuality(const ImageKey& key, On 2016/03/21 20:53:40, vmpstr wrote: > Maybe we can just skip this function and use GetOriginalDecode? Wdyt? Done. https://codereview.chromium.org/1801933004/diff/60001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:359: if (!decoded_image_result.decoded_pixmap_.addr()) { On 2016/03/21 20:53:40, vmpstr wrote: > nit: Usually for single line if and a single line statement, braces are > optional. Done. https://codereview.chromium.org/1801933004/diff/60001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:466: return DecodedDrawImage(nullptr, kNone_SkFilterQuality); On 2016/03/21 20:53:40, vmpstr wrote: > If this is a case that we need to handle, then it needs to be handled earlier. > The problem with returning here is that the unlock object will be destroyed > first, which will first acquire the mutex and then the lock on line 418 will be > destroyed which will unlock it again. That's kind of just mutex churn :) I think you are right, this won't ever be the case. I needed to call draw_image.image(). From there I likely thought "I better check for null before I dereference the pointer." I failed to consider the mutex. https://codereview.chromium.org/1801933004/diff/60001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:518: SoftwareImageDecodeController::AttemptToUseOriginalImage(const ImageKey& key, On 2016/03/21 20:53:40, vmpstr wrote: > nit: Can we call this "GetOriginalImageDecode" or something like that? Done. https://codereview.chromium.org/1801933004/diff/60001/cc/tiles/software_image... cc/tiles/software_image_decode_controller.cc:523: if (!key.can_use_original_decode()) { On 2016/03/21 20:53:40, vmpstr wrote: > Can we make this a DCHECK? Whatever code is calling this should only call it if > key.can_use_original_decode is true. Done.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1801933004/diff/140001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1801933004/diff/140001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:488: if (key.can_use_original_decode()) { Can't we just call DecodeImageInternal here? Seems like the same logic is in both places? https://codereview.chromium.org/1801933004/diff/140001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:488: if (key.can_use_original_decode()) { If we unify the two by calling DecodeImageInternal, |can_use_original_decode| is only used in dchecks, so I think we can just remove this? https://codereview.chromium.org/1801933004/diff/140001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.h (right): https://codereview.chromium.org/1801933004/diff/140001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.h:199: const SkImage& image); nit: This function is not defined or used.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/180001
https://codereview.chromium.org/1801933004/diff/140001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1801933004/diff/140001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:488: if (key.can_use_original_decode()) { On 2016/03/24 16:06:59, ericrk wrote: > Can't we just call DecodeImageInternal here? Seems like the same logic is in > both places? Done. https://codereview.chromium.org/1801933004/diff/140001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:488: if (key.can_use_original_decode()) { On 2016/03/24 16:06:59, ericrk wrote: > If we unify the two by calling DecodeImageInternal, |can_use_original_decode| is > only used in dchecks, so I think we can just remove this? I began removing it and noticed GetScaleAdjustment was using it to return a SkSize of 1.f, 1.f. I would imagine a floating point divide of x/x would return 1.f but I wonder if it happens to be just slightly off and causing a problem? (Otherwise, why have that shortcut?) Similarly, inside ImageDecodeControllerKey's constructor there is a special branch for if we can use the original decode. But I don't think it is required. So I made these changes and believe they should work. https://codereview.chromium.org/1801933004/diff/140001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.h (right): https://codereview.chromium.org/1801933004/diff/140001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.h:199: const SkImage& image); On 2016/03/24 16:06:59, ericrk wrote: > nit: This function is not defined or used. Done.
https://codereview.chromium.org/1801933004/diff/140001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1801933004/diff/140001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:488: if (key.can_use_original_decode()) { On 2016/03/24 16:06:59, ericrk wrote: > If we unify the two by calling DecodeImageInternal, |can_use_original_decode| is > only used in dchecks, so I think we can just remove this? Running the tests, the low quality test and none quality test are failing on EXPECT_TRUE(decoded_draw_image.is_scale_adjustment_identity()); So it looks like the floating point division of x/x isn't within our epsilon.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/1801933004/diff/140001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.h (right): https://codereview.chromium.org/1801933004/diff/140001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.h:41: can_use_original_decode_ == other.can_use_original_decode_ && I may have missed something here, was mainly looking at uses in the cc - with the can_use_original_decode_ check here, we will return true for operator== for two keys, one with kNone filter quality and one with kLow filter quality. With the new logic, KNone and kLow won't match... Maybe it is better to leave this in for readability here?
https://codereview.chromium.org/1801933004/diff/140001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.h (right): https://codereview.chromium.org/1801933004/diff/140001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.h:41: can_use_original_decode_ == other.can_use_original_decode_ && On 2016/03/24 20:01:21, ericrk wrote: > I may have missed something here, was mainly looking at uses in the cc - with > the can_use_original_decode_ check here, we will return true for operator== for > two keys, one with kNone filter quality and one with kLow filter quality. With > the new logic, KNone and kLow won't match... > > Maybe it is better to leave this in for readability here? I think I agree. There is a special case to shortcut if the qualities don't match (low/none) but they both use the original decode. Without it, this would have to be aware of if the qualities happen to use the original decode and then would have to make a special case for "Well, unless they happen to only differ by qualities which use the original decode." We could make it not *too* messy but it would still have to be aware of what the quality is doing, which I don't like. So yeah, I think maybe it might make sense to leave the can_use_original_decode_ in.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/200001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, but please wait for vmpstr's lgtm as well. https://codereview.chromium.org/1801933004/diff/200001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.h (right): https://codereview.chromium.org/1801933004/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.h:77: bool can_use_original_decode_); nit: remove the "_"
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/220001
vmpstr@ ping https://codereview.chromium.org/1801933004/diff/200001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.h (right): https://codereview.chromium.org/1801933004/diff/200001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.h:77: bool can_use_original_decode_); On 2016/03/29 23:23:10, ericrk wrote: > nit: remove the "_" Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:406: // fall through I don't think you really need this comment. That is, " case A: case B: ... " is pretty clear. If you want to keep it, then it has to be a complete sentence. https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:488: unnecessary change https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:529: SoftwareImageDecodeController::DecodedImageResult::DecodedImageResult( Move this function to be after all other SoftwareImageDecodeController functions are defined. https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:575: SoftwareImageDecodeController::DecodeImageOrUseCache(const ImageKey& key, I'm still a bit confused about DecodeImageOrUseCache vs GetDecodedImageForDrawInternal... or similar things. Can you explain the general flow of the code as it stands now? https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.h (right): https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.h:190: struct DecodedImageResult { Can you move types to the top (next to other types in this access specifier). Also, can you explain the general flow of logic here? Ie, why do we need this extra type? https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.h:195: SkPixmap decoded_pixmap_; Typically, struct members that are public are named without the trailing underscore in cc. https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.h:200: scoped_ptr<DecodedImage> DecodeImageMediumQuality(const ImageKey& key, Add comments for each of the functions please. Specifically, it should say if the function has any side effects such as caching. https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.h:208: DecodedImageResult DecodeImageOrUseCache(const ImageKey& key, This is named somewhat awkwardly. I don't really know what it returns? Is this different from GetOriginalImageDecode?
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/280001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.cc (right): https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:406: // fall through On 2016/03/31 19:54:32, vmpstr wrote: > I don't think you really need this comment. That is, " > > case A: > case B: > ... > > " is pretty clear. > > If you want to keep it, then it has to be a complete sentence. Done. https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:488: On 2016/03/31 19:54:32, vmpstr wrote: > unnecessary change Done. https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:529: SoftwareImageDecodeController::DecodedImageResult::DecodedImageResult( On 2016/03/31 19:54:32, vmpstr wrote: > Move this function to be after all other SoftwareImageDecodeController functions > are defined. Done. https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.cc:575: SoftwareImageDecodeController::DecodeImageOrUseCache(const ImageKey& key, On 2016/03/31 19:54:32, vmpstr wrote: > I'm still a bit confused about DecodeImageOrUseCache vs > GetDecodedImageForDrawInternal... or similar things. > > Can you explain the general flow of the code as it stands now? DecodeImageInternal's old body was split into 3 functions: - GetOriginalImageDecode, - DecodeImageOrUseCache, and - ScaleImage The qualities which require no scaling only called GetOriginalImageDecode. The qualities which required scaling would first call DecodeImageOrUseCache and then pass the results to ScaleImage. Vlad and I talked about it and feel that a more natural split would be: - GetOriginalImageDecode, and - GetScaledImageDecode https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... File cc/tiles/software_image_decode_controller.h (right): https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.h:190: struct DecodedImageResult { On 2016/03/31 19:54:32, vmpstr wrote: > Can you move types to the top (next to other types in this access specifier). > > Also, can you explain the general flow of logic here? Ie, why do we need this > extra type? Done. The extra type was just to facilitate the single responsibility of DecodeImageOrUseCache and ScaleImage. ScaleImage needed the information which DecodedImageResult holds. But after Vlad and I talked about it a more natural division is between getting the original image and the scaled image. The fact that this struct existed showed it wasn't a great dividing line. https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.h:195: SkPixmap decoded_pixmap_; On 2016/03/31 19:54:32, vmpstr wrote: > Typically, struct members that are public are named without the trailing > underscore in cc. Done. https://codereview.chromium.org/1801933004/diff/220001/cc/tiles/software_imag... cc/tiles/software_image_decode_controller.h:200: scoped_ptr<DecodedImage> DecodeImageMediumQuality(const ImageKey& key, On 2016/03/31 19:54:33, vmpstr wrote: > Add comments for each of the functions please. Specifically, it should say if > the function has any side effects such as caching. Done.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/300001
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/340001
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by cblume@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks!
The CQ bit was checked by cblume@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ericrk@chromium.org Link to the patchset: https://codereview.chromium.org/1801933004/#ps380001 (title: "Updating comment to reflect change in code.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1801933004/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1801933004/380001
Message was sent while issue was closed.
Description was changed from ========== Refactor SoftwareImageDecodeController to make adding support for medium quality more readable and natural. BUG=594839 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Refactor SoftwareImageDecodeController to make adding support for medium quality more readable and natural. BUG=594839 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== Refactor SoftwareImageDecodeController to make adding support for medium quality more readable and natural. BUG=594839 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Refactor SoftwareImageDecodeController to make adding support for medium quality more readable and natural. BUG=594839 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel Committed: https://crrev.com/4c4a73d71ba30eca9fd9b8d17f8175f462631689 Cr-Commit-Position: refs/heads/master@{#387426} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/4c4a73d71ba30eca9fd9b8d17f8175f462631689 Cr-Commit-Position: refs/heads/master@{#387426} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
