|
|
DescriptionMerge clearImage() and clearImageAndNotifyObservers() into updateImage()
In order to make ImageResourceContent::updateImage() the single control point
of image updates from ImageResource, this CL merges clearImage() and
clearImageAndNotifyObservers() calls from ImageResource into updateImage().
BUG=667641
Review-Url: https://codereview.chromium.org/2587503002
Cr-Commit-Position: refs/heads/master@{#442351}
Committed: https://chromium.googlesource.com/chromium/src/+/5a597473adc7e1e3d0dd703d6d64dbe9364144ba
Patch Set 1 #Patch Set 2 : Rebase. #Patch Set 3 : Rebase #
Total comments: 3
Patch Set 4 : Rename ClearImageOnly #Patch Set 5 : Build fix #Patch Set 6 : comment #
Dependent Patchsets: Messages
Total messages: 41 (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...
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 ========== Merge clearImage() and clearImageAndNotifyObservers() into updateImage() In order to make ImageResourceContent::updateImage() the single control point of image updates from ImageResource, this CL merges clearImage() and clearImageAndNotifyObservers() calls from ImageResource into updateImage(). BUG= ========== to ========== Merge clearImage() and clearImageAndNotifyObservers() into updateImage() In order to make ImageResourceContent::updateImage() the single control point of image updates from ImageResource, this CL merges clearImage() and clearImageAndNotifyObservers() calls from ImageResource into updateImage(). BUG=667641 ==========
hiroshige@chromium.org changed reviewers: + japhet@chromium.org, yhirano@chromium.org
PTAL. Initially I thought calling clearImage() etc. is simpler rather than adding more options to updateImage(), but during coding of https://codereview.chromium.org/2527353002 I found having a single path for updating image makes LoFi reloading simpler, so this CL merges clearImage(), clearImageAndNotifyObservers() and updateImage() into one.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2587503002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right): https://codereview.chromium.org/2587503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:242: getContent()->updateImage(nullptr, ImageResourceContent::ClearAndUpdateImage, Isn't this ClearImageOnly?
https://codereview.chromium.org/2587503002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right): https://codereview.chromium.org/2587503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:242: getContent()->updateImage(nullptr, ImageResourceContent::ClearAndUpdateImage, On 2016/12/19 11:50:26, yhirano wrote: > Isn't this ClearImageOnly? No. Here, we want to clear the image without notifying observers, to keep the current behavior. - If we use ClearImageOnly, the image is cleared but the observers are notified. - If we use ClearAndUpdateImage, the image is cleared and the observers are not notified because of the following if statement in updateImage(): if (m_sizeAvailable == Image::SizeUnavailable && !allDataReceived) return; In another perspective, using ClearAndUpdateImage here lets ImageResourceContent to decide whether to notify observers (and in this case it doesn't), which might look reasonable.
https://codereview.chromium.org/2587503002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right): https://codereview.chromium.org/2587503002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:242: getContent()->updateImage(nullptr, ImageResourceContent::ClearAndUpdateImage, On 2016/12/19 22:51:50, hiroshige wrote: > On 2016/12/19 11:50:26, yhirano wrote: > > Isn't this ClearImageOnly? > > No. > Here, we want to clear the image without notifying observers, to keep the > current behavior. > - If we use ClearImageOnly, the image is cleared but the observers are notified. > - If we use ClearAndUpdateImage, the image is cleared and the observers are not > notified because of the following if statement in updateImage(): > if (m_sizeAvailable == Image::SizeUnavailable && !allDataReceived) > return; > > In another perspective, using ClearAndUpdateImage here lets ImageResourceContent > to decide whether to notify observers (and in this case it doesn't), which might > look reasonable. Then I think the enum naming is confusing - ClearImageOnly sounds more restrictive than ClearAndUpdateImage.
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...
On 2016/12/20 12:09:16, yhirano OOO till 01-04 wrote: > https://codereview.chromium.org/2587503002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right): > > https://codereview.chromium.org/2587503002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:242: > getContent()->updateImage(nullptr, ImageResourceContent::ClearAndUpdateImage, > On 2016/12/19 22:51:50, hiroshige wrote: > > On 2016/12/19 11:50:26, yhirano wrote: > > > Isn't this ClearImageOnly? > > > > No. > > Here, we want to clear the image without notifying observers, to keep the > > current behavior. > > - If we use ClearImageOnly, the image is cleared but the observers are > notified. > > - If we use ClearAndUpdateImage, the image is cleared and the observers are > not > > notified because of the following if statement in updateImage(): > > if (m_sizeAvailable == Image::SizeUnavailable && !allDataReceived) > > return; > > > > In another perspective, using ClearAndUpdateImage here lets > ImageResourceContent > > to decide whether to notify observers (and in this case it doesn't), which > might > > look reasonable. > > Then I think the enum naming is confusing - ClearImageOnly sounds more > restrictive than ClearAndUpdateImage. Looks reasonable. I renamed ClearImageOnly to ClearImageAndNotifyObservers and also updated the comments.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
This l-g-t-m, but I've been out of the loop. If yhirano's happy, I'm happy :)
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 unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by hiroshige@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483656939671730, "parent_rev": "0f9a13711bef4107aeeb4e15ee296b917228c141", "commit_rev": "783791d3f346098318401b61b46971e6bb1c1a3a"}
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483656939671730, "parent_rev": "0f9a13711bef4107aeeb4e15ee296b917228c141", "commit_rev": "783791d3f346098318401b61b46971e6bb1c1a3a"}
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483656939671730, "parent_rev": "0f9a13711bef4107aeeb4e15ee296b917228c141", "commit_rev": "783791d3f346098318401b61b46971e6bb1c1a3a"}
The CQ bit was unchecked by commit-bot@chromium.org
Failed to commit the patch.
The CQ bit was checked by hiroshige@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1483990196832670, "parent_rev": "a110d353ecca5d521ce7ec513e0493c98f0b37ed", "commit_rev": "5a597473adc7e1e3d0dd703d6d64dbe9364144ba"}
Message was sent while issue was closed.
Description was changed from ========== Merge clearImage() and clearImageAndNotifyObservers() into updateImage() In order to make ImageResourceContent::updateImage() the single control point of image updates from ImageResource, this CL merges clearImage() and clearImageAndNotifyObservers() calls from ImageResource into updateImage(). BUG=667641 ========== to ========== Merge clearImage() and clearImageAndNotifyObservers() into updateImage() In order to make ImageResourceContent::updateImage() the single control point of image updates from ImageResource, this CL merges clearImage() and clearImageAndNotifyObservers() calls from ImageResource into updateImage(). BUG=667641 Review-Url: https://codereview.chromium.org/2587503002 Cr-Commit-Position: refs/heads/master@{#442351} Committed: https://chromium.googlesource.com/chromium/src/+/5a597473adc7e1e3d0dd703d6d64... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/5a597473adc7e1e3d0dd703d6d64... |