|
|
DescriptionExplicitly clear the image in the first updateImage() call
Previously, updateImage() called in appendData()/finish()/error()
creates a new blink::Image only when the given SharedBuffer is non-empty and
|m_image| is nullptr.
However, this requires |m_image| to be cleared to enforce a new image
creation in LoFi/placeholder image reloading and would complicate
refactoring of LoFi reloading in https://codereview.chromium.org/2527353002.
This CL makes ImageResource to explicitly use ClearAndUpdateImage when calling
updateImage() for the first time for a corresponding response (or a
corresponding part in case of a multipart image).
As a result, we no longer need to clear |m_image| when reloading LoFi images.
BUG=667641
Patch Set 1 #
Total comments: 1
Patch Set 2 : Rebase #Patch Set 3 : Rebase #Patch Set 4 : Rebase #Patch Set 5 : Rebase after toyoshim #Patch Set 6 : Rebase #Patch Set 7 : Rebase #Patch Set 8 : Rebase #
Total comments: 2
Patch Set 9 : Rebase #Patch Set 10 : Rename flag name #Patch Set 11 : Rebase #Patch Set 12 : Rebase #
Depends on Patchset: Messages
Total messages: 40 (30 generated)
The CQ bit was checked by hiroshige@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...
Description was changed from ========== Explicitly use ClearExistingImage for the first updateImage() call BUG= ========== to ========== Explicitly use ClearExistingImage for the first updateImage() call Previously, updateImage() called in appendData()/finish()/error() creates a new blink::Image only when the given SharedBuffer is non-empty and |m_image| is nullptr. However, this requires |m_image| to be cleared to enforce a new image creation in LoFi/placeholder image reloading and would complicate refactoring of LoFi reloading in https://codereview.chromium.org/2527353002. This CL makes ImageResource to explicitly use ClearExistingImage when calling updateImage() for the first time for a corresponding response (or a corresponding part in case of a multipart image). As a result, we no longer need to clear |m_image| when reloading LoFi images. BUG=667641 ==========
hiroshige@chromium.org changed reviewers: + hajimehoshi@chromium.org, yhirano@chromium.org
PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2552653002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ImageResource.h (right): https://codereview.chromium.org/2552653002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ImageResource.h:27: #include "core/fetch/ImageResourceContent.h" I couldn't find this file.
The CQ bit was checked by hiroshige@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: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by hiroshige@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.
lgtm
The CQ bit was checked by hiroshige@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 checked by hiroshige@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: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Explicitly use ClearExistingImage for the first updateImage() call Previously, updateImage() called in appendData()/finish()/error() creates a new blink::Image only when the given SharedBuffer is non-empty and |m_image| is nullptr. However, this requires |m_image| to be cleared to enforce a new image creation in LoFi/placeholder image reloading and would complicate refactoring of LoFi reloading in https://codereview.chromium.org/2527353002. This CL makes ImageResource to explicitly use ClearExistingImage when calling updateImage() for the first time for a corresponding response (or a corresponding part in case of a multipart image). As a result, we no longer need to clear |m_image| when reloading LoFi images. BUG=667641 ========== to ========== Explicitly clear the image in the first updateImage() call Previously, updateImage() called in appendData()/finish()/error() creates a new blink::Image only when the given SharedBuffer is non-empty and |m_image| is nullptr. However, this requires |m_image| to be cleared to enforce a new image creation in LoFi/placeholder image reloading and would complicate refactoring of LoFi reloading in https://codereview.chromium.org/2527353002. This CL makes ImageResource to explicitly use ClearAndUpdateImage when calling updateImage() for the first time for a corresponding response (or a corresponding part in case of a multipart image). As a result, we no longer need to clear |m_image| when reloading LoFi images. BUG=667641 ==========
Rebased after toyoshim's directory move and https://codereview.chromium.org/2587503002. PTAL again.
The CQ bit was checked by hiroshige@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.
kouhei@chromium.org changed reviewers: + kouhei@chromium.org
https://codereview.chromium.org/2552653002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResource.h (right): https://codereview.chromium.org/2552653002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResource.h:152: bool m_isUpdateImageCalled = false; This flag name is quite confusing, as onePartIn~() also updates this. Would inverting this flag & renaming this to "m_shouldEnforceClearImage" make sense? Would you document somewhere why we have this ImageResourceContent::ClearImageOption in the first place?
On 2016/12/19 01:32:04, kouhei wrote: > https://codereview.chromium.org/2552653002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/loader/resource/ImageResource.h (right): > > https://codereview.chromium.org/2552653002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/loader/resource/ImageResource.h:152: bool > m_isUpdateImageCalled = false; > This flag name is quite confusing, as onePartIn~() also updates this. > Would inverting this flag & renaming this to "m_shouldEnforceClearImage" make > sense? > > Would you document somewhere why we have this > ImageResourceContent::ClearImageOption in the first place? <intent-to-unblock> Feel free to ping me on Hangout/VC for this, or go ahead and land this CL given lgt yhirano@.
The CQ bit was checked by hiroshige@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...
https://codereview.chromium.org/2552653002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResource.h (right): https://codereview.chromium.org/2552653002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResource.h:152: bool m_isUpdateImageCalled = false; On 2016/12/19 01:32:04, kouhei wrote: > This flag name is quite confusing, as onePartIn~() also updates this. > Would inverting this flag & renaming this to "m_shouldEnforceClearImage" make > sense? Done in Patch Set 10. > Would you document somewhere why we have this > ImageResourceContent::ClearImageOption in the first place? Done (in https://codereview.chromium.org/2587503002/ Patch Set 6).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm
Thanks for reviewing. However, I intended to use this CL in order to change the timing of clearing m_image of ImageResourceContent in reloading, but I found a problem there, and I changed my mind. Now I'll keep the timing of clearing m_image there, and thus this CL is not needed by that CL. Therefore I close this CL uncommitted. |