Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2176603002:
Simplify the condition for destroyDecodedData() (Closed)
|
Created:
4 years, 5 months ago by hajimehoshi Modified:
4 years, 4 months ago CC:
blink-reviews, chromium-reviews, gavinp+loader_chromium.org, Nate Chapin, loading-reviews+fetch_chromium.org, tyoshino+watch_chromium.org, Yoav Weiss Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSimplify the condition for destroyDecodedData()
This CL simplifies the condition for destroyDecodedData(). When an error
occurs, |m_image| must be null and the condition for |m_image| doesn't
make sense.
This CL also fixes another if condition and inserts DCHECK based on the
above assumption.
BUG=618623, 631321
TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.*
Committed: https://crrev.com/c3dc43ed9905a2225780ff466706cfd3044f62ce
Cr-Commit-Position: refs/heads/master@{#408358}
Patch Set 1 #Patch Set 2 : Address on hiroshige's review #Patch Set 3 : Remove calling prune() #Patch Set 4 : (rebase) #Messages
Total messages: 37 (24 generated)
The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hajimehoshi@chromium.org changed reviewers: + hiroshige@chromium.org, mkwst@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Code looks good. > This CL also removes calling prune() in clear() since the image is > cleared immediately and there is no use in pruning here. Could you make this a separate CL? > CL description > When an error occurs, |m_image| must be null Could you mention that this CL inserts |CHECK(!errorOccurred())| based on the assumption? ALso, how about: BUG=618623, 631321
On 2016/07/26 05:41:17, hiroshige wrote: > Code looks good. > > > This CL also removes calling prune() in clear() since the image is > > cleared immediately and there is no use in pruning here. > Could you make this a separate CL? > > > CL description > > When an error occurs, |m_image| must be null > Could you mention that this CL inserts |CHECK(!errorOccurred())| based on the > assumption? > > ALso, how about: > BUG=618623, 631321 Also, how about removing "Refactoring: " from the CL title, because this CL is not a pure refactoring: it inserts CHECK()s and potentially causes more crashes -- we don't expect so though. nit: Simply -> Simplify
Description was changed from ========== Refactoring: Simply the condition for destroyDecodedData() This CL simplifies the condition for destroyDecodedData(). When an error occurs, |m_image| must be null and the condition for |m_image| doesn't make sense. This CL also removes calling prune() in clear() since the image is cleared immediately and there is no use in pruning here. TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.* ========== to ========== Simplify the condition for destroyDecodedData() This CL simplifies the condition for destroyDecodedData(). When an error occurs, |m_image| must be null and the condition for |m_image| doesn't make sense. This CL also removes calling prune() in clear() since the image is cleared immediately and there is no use in pruning here. BUG=618623, 631321 TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.* ==========
Description was changed from ========== Simplify the condition for destroyDecodedData() This CL simplifies the condition for destroyDecodedData(). When an error occurs, |m_image| must be null and the condition for |m_image| doesn't make sense. This CL also removes calling prune() in clear() since the image is cleared immediately and there is no use in pruning here. BUG=618623, 631321 TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.* ========== to ========== Simplify the condition for destroyDecodedData() This CL simplifies the condition for destroyDecodedData(). When an error occurs, |m_image| must be null and the condition for |m_image| doesn't make sense. BUG=618623, 631321 TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.* ==========
The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/26 05:44:44, hiroshige wrote: > On 2016/07/26 05:41:17, hiroshige wrote: > > Code looks good. > > > > > This CL also removes calling prune() in clear() since the image is > > > cleared immediately and there is no use in pruning here. > > Could you make this a separate CL? > > > > > CL description > > > When an error occurs, |m_image| must be null > > Could you mention that this CL inserts |CHECK(!errorOccurred())| based on the > > assumption? > > > > ALso, how about: > > BUG=618623, 631321 > > Also, how about removing "Refactoring: " from the CL title, because this CL is > not a pure refactoring: it inserts CHECK()s and potentially causes more crashes > -- we don't expect so though. > > nit: Simply -> Simplify Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
lgtm! > CL description Could you mention that this CL inserts |CHECK(!errorOccurred())| based on the assumption?
Description was changed from ========== Simplify the condition for destroyDecodedData() This CL simplifies the condition for destroyDecodedData(). When an error occurs, |m_image| must be null and the condition for |m_image| doesn't make sense. BUG=618623, 631321 TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.* ========== to ========== Simplify the condition for destroyDecodedData() This CL simplifies the condition for destroyDecodedData(). When an error occurs, |m_image| must be null and the condition for |m_image| doesn't make sense. This CL also fixes another if condition and inserts DCHECK based on the above assumption. BUG=618623, 631321 TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.* ==========
On 2016/07/26 06:49:33, hiroshige wrote: > lgtm! > > > CL description > Could you mention that this CL inserts |CHECK(!errorOccurred())| based on the > assumption? Done.
The bots seem very unhappy with this patch.
On 2016/07/27 08:43:32, Mike West wrote: > The bots seem very unhappy with this patch. Oops, prune() calls destroyDecodedDataIfPossible() before m_image is cleared. Let's wait for https://codereview.chromium.org/2178073005/.
The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hajimehoshi@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM!
The CQ bit was checked by hajimehoshi@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from hiroshige@chromium.org Link to the patchset: https://codereview.chromium.org/2176603002/#ps60001 (title: "(rebase)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Simplify the condition for destroyDecodedData() This CL simplifies the condition for destroyDecodedData(). When an error occurs, |m_image| must be null and the condition for |m_image| doesn't make sense. This CL also fixes another if condition and inserts DCHECK based on the above assumption. BUG=618623, 631321 TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.* ========== to ========== Simplify the condition for destroyDecodedData() This CL simplifies the condition for destroyDecodedData(). When an error occurs, |m_image| must be null and the condition for |m_image| doesn't make sense. This CL also fixes another if condition and inserts DCHECK based on the above assumption. BUG=618623, 631321 TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.* ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Simplify the condition for destroyDecodedData() This CL simplifies the condition for destroyDecodedData(). When an error occurs, |m_image| must be null and the condition for |m_image| doesn't make sense. This CL also fixes another if condition and inserts DCHECK based on the above assumption. BUG=618623, 631321 TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.* ========== to ========== Simplify the condition for destroyDecodedData() This CL simplifies the condition for destroyDecodedData(). When an error occurs, |m_image| must be null and the condition for |m_image| doesn't make sense. This CL also fixes another if condition and inserts DCHECK based on the above assumption. BUG=618623, 631321 TEST=webkit_unit_tests --gtest_filter=ImageResourceTest.* Committed: https://crrev.com/c3dc43ed9905a2225780ff466706cfd3044f62ce Cr-Commit-Position: refs/heads/master@{#408358} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/c3dc43ed9905a2225780ff466706cfd3044f62ce Cr-Commit-Position: refs/heads/master@{#408358} |
