Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2407573002:
Wait to notify completion until after a Lo-Fi image is reloaded. (Closed)
|
Created:
4 years, 2 months ago by sclittle Modified:
4 years, 2 months ago Reviewers:
Nate Chapin CC:
Nate Chapin, blink-reviews, chromium-reviews, gavinp+loader_chromium.org, loading-reviews+fetch_chromium.org, megjablon, tyoshino+watch_chromium.org, Yoav Weiss Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWait to notify completion until after a Lo-Fi image is reloaded.
Currently, if ImageResource::reloadIfLoFi() is called while that
ImageResource is still loading, the resource's clients and observers are
notified of completion when the current load is cancelled.
After this CL, the ImageResource waits to notify of completion until the
reload completes.
BUG=654055
Committed: https://crrev.com/1e457676004f8af16e0086280b73d87c1e5efba7
Cr-Commit-Position: refs/heads/master@{#424926}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Moved flag into ImageResource. #
Total comments: 2
Patch Set 3 : removed unnecessary m_isSchedulingReload check #
Messages
Total messages: 22 (8 generated)
sclittle@chromium.org changed reviewers: + japhet@chromium.org
n https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ImageResource.cpp:536: { I don't think you need to force this to have a narrower scope than this function. https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ImageResource.cpp:541: Resource::ProhibitNotifyClientsOfCompletionInScope AutoReset<bool> instead of a custom helper? https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ImageResource.cpp:547: loader()->cancel(); Is this the only case where you want to suppress callbacks? If so, the scoped suppressor should probably go in this if() block? https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:320: m_isNotifyClientsOfCompletionProhibited(false), I'm suspicious this can be on ImageResource instead of Resource. I think you can stop all these notifications by hooking ImageResource::checkNotify? Especially since clients shouldn't get added/removed during cancellation if callbacks are being surpressed. Is there a case I'm forgetting about?
https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ImageResource.cpp:536: { On 2016/10/10 23:47:49, Nate Chapin wrote: > I don't think you need to force this to have a narrower scope than this > function. AIUI we only want to prevent completion callbacks from being sent before we start the reload - we want to allow completion callbacks to be sent after the reload is started. Any new observers added after the call to setStatus(NotStarted) won't be immediately notified of completion, so that should be fine. The main reason it has a narrower scope is because I'm worried about fetcher->startLoad(). Right now, I don't know of any way that fetcher->startLoad() would complete the fetch synchronously (and thus want to call the completion callbacks because the fetch completed), but it's possible that a new codepath could be added in the future that does this. If you think this would be better at the function scope, then I'd be happy to move it. https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ImageResource.cpp:541: Resource::ProhibitNotifyClientsOfCompletionInScope On 2016/10/10 23:47:49, Nate Chapin wrote: > AutoReset<bool> instead of a custom helper? I don't see an easy way to do that, unless we want to expose a pointer to Resource::m_isNotifyClientsOfCompletionProhibited to subclasses. AutoReset doesn't allow copy or assign, so we can't just return an AutoReset<bool> from a method. https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ImageResource.cpp:547: loader()->cancel(); On 2016/10/10 23:47:49, Nate Chapin wrote: > Is this the only case where you want to suppress callbacks? If so, the scoped > suppressor should probably go in this if() block? loader()->cancel() is the main case I'm worried about, but we don't want other calls like clear() or notifyObservers() to cause completion callbacks to be sent either (e.g. if one of the observers notified in notifyObservers() adds a new client or observer or something). Only completion callbacks like ImageResourceObserver::imageNotifyFinished() and ResourceClient::notifyFinished() are suppressed, so other callbacks like ImageResourceObserver::imageChanged() can still run. https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:320: m_isNotifyClientsOfCompletionProhibited(false), On 2016/10/10 23:47:49, Nate Chapin wrote: > I'm suspicious this can be on ImageResource instead of Resource. I think you can > stop all these notifications by hooking ImageResource::checkNotify? Especially > since clients shouldn't get added/removed during cancellation if callbacks are > being surpressed. Is there a case I'm forgetting about? Only the completion callbacks are being suppressed, not others like ImageResourceObserver::imageChanged(), so IIUC it's possible that an imageChanged() or other kind of callback gets called and adds a new ResourceClient. We could try to suppress imageChanged callbacks too, but there are other ImageResourceObserver callbacks that probably shouldn't be suppressed during a reload and could still possibly add clients or observers, like ImageResourceObserver::computeResourcePriority(). I could be missing something, but AIUI it seems easier to just handle this in Resource than to enforce that no callbacks add observers or clients. WDYT?
https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ImageResource.cpp:536: { On 2016/10/11 at 02:56:56, sclittle wrote: > On 2016/10/10 23:47:49, Nate Chapin wrote: > > I don't think you need to force this to have a narrower scope than this > > function. > > AIUI we only want to prevent completion callbacks from being sent before we start the reload - we want to allow completion callbacks to be sent after the reload is started. Any new observers added after the call to setStatus(NotStarted) won't be immediately notified of completion, so that should be fine. > > The main reason it has a narrower scope is because I'm worried about fetcher->startLoad(). Right now, I don't know of any way that fetcher->startLoad() would complete the fetch synchronously (and thus want to call the completion callbacks because the fetch completed), but it's possible that a new codepath could be added in the future that does this. > > If you think this would be better at the function scope, then I'd be happy to move it. If startLoad() is someday allowed to complete the fetch synchronously, there will be lots of details the author of that change will have to work through. Feel free to make that hypothetical future author's life more difficult and save us the complexity in the more likely case that we never allow it :) https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ImageResource.cpp:541: Resource::ProhibitNotifyClientsOfCompletionInScope On 2016/10/11 at 02:56:56, sclittle wrote: > On 2016/10/10 23:47:49, Nate Chapin wrote: > > AutoReset<bool> instead of a custom helper? > > I don't see an easy way to do that, unless we want to expose a pointer to Resource::m_isNotifyClientsOfCompletionProhibited to subclasses. AutoReset doesn't allow copy or assign, so we can't just return an AutoReset<bool> from a method. Let's use AutoReset if and only if we settle on moving m_isNotifyClientsOfCompletionProhibited to ImageResource, then? https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ImageResource.cpp:547: loader()->cancel(); On 2016/10/11 at 02:56:56, sclittle wrote: > On 2016/10/10 23:47:49, Nate Chapin wrote: > > Is this the only case where you want to suppress callbacks? If so, the scoped > > suppressor should probably go in this if() block? > > loader()->cancel() is the main case I'm worried about, but we don't want other calls like clear() or notifyObservers() to cause completion callbacks to be sent either (e.g. if one of the observers notified in notifyObservers() adds a new client or observer or something). > > Only completion callbacks like ImageResourceObserver::imageNotifyFinished() and ResourceClient::notifyFinished() are suppressed, so other callbacks like ImageResourceObserver::imageChanged() can still run. clear() is trivial, but notifyObservers() could have unpredictable side effects. This scoping should be fine then. https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:320: m_isNotifyClientsOfCompletionProhibited(false), On 2016/10/11 at 02:56:56, sclittle wrote: > On 2016/10/10 23:47:49, Nate Chapin wrote: > > I'm suspicious this can be on ImageResource instead of Resource. I think you can > > stop all these notifications by hooking ImageResource::checkNotify? Especially > > since clients shouldn't get added/removed during cancellation if callbacks are > > being surpressed. Is there a case I'm forgetting about? > > Only the completion callbacks are being suppressed, not others like ImageResourceObserver::imageChanged(), so IIUC it's possible that an imageChanged() or other kind of callback gets called and adds a new ResourceClient. > > We could try to suppress imageChanged callbacks too, but there are other ImageResourceObserver callbacks that probably shouldn't be suppressed during a reload and could still possibly add clients or observers, like ImageResourceObserver::computeResourcePriority(). > > I could be missing something, but AIUI it seems easier to just handle this in Resource than to enforce that no callbacks add observers or clients. WDYT? I'm not certain how much of a risk this is in practice, but assuming it is: I think supressing in ImageResource::didAddClient and ImageResource::checkNotify should case all caseS?
https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ImageResource.cpp:536: { On 2016/10/12 22:56:17, Nate Chapin wrote: > On 2016/10/11 at 02:56:56, sclittle wrote: > > On 2016/10/10 23:47:49, Nate Chapin wrote: > > > I don't think you need to force this to have a narrower scope than this > > > function. > > > > AIUI we only want to prevent completion callbacks from being sent before we > start the reload - we want to allow completion callbacks to be sent after the > reload is started. Any new observers added after the call to > setStatus(NotStarted) won't be immediately notified of completion, so that > should be fine. > > > > The main reason it has a narrower scope is because I'm worried about > fetcher->startLoad(). Right now, I don't know of any way that > fetcher->startLoad() would complete the fetch synchronously (and thus want to > call the completion callbacks because the fetch completed), but it's possible > that a new codepath could be added in the future that does this. > > > > If you think this would be better at the function scope, then I'd be happy to > move it. > > If startLoad() is someday allowed to complete the fetch synchronously, there > will be lots of details the author of that change will have to work through. > Feel free to make that hypothetical future author's life more difficult and save > us the complexity in the more likely case that we never allow it :) Fair enough :) I've removed the dependency on AutoReset, and changed this to just set the m_isSchedulingReload to true and false directly, which should be fine since multiple calls to reloadIfLoFi should never be nested anyways. I'm still resetting m_isSchedulingReload before the call to fetcher->startLoad(), because there's no extra complexity to do that now, but I can move it after fetcher->startLoad() if you'd prefer. https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ImageResource.cpp:541: Resource::ProhibitNotifyClientsOfCompletionInScope On 2016/10/12 22:56:17, Nate Chapin wrote: > On 2016/10/11 at 02:56:56, sclittle wrote: > > On 2016/10/10 23:47:49, Nate Chapin wrote: > > > AutoReset<bool> instead of a custom helper? > > > > I don't see an easy way to do that, unless we want to expose a pointer to > Resource::m_isNotifyClientsOfCompletionProhibited to subclasses. AutoReset > doesn't allow copy or assign, so we can't just return an AutoReset<bool> from a > method. > > Let's use AutoReset if and only if we settle on moving > m_isNotifyClientsOfCompletionProhibited to ImageResource, then? I've removed the dependency on AutoReset and changed this to directly set/reset the bool, which should be fine because we should never schedule a reload while scheduling a reload. https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/ImageResource.cpp:547: loader()->cancel(); On 2016/10/12 22:56:17, Nate Chapin wrote: > On 2016/10/11 at 02:56:56, sclittle wrote: > > On 2016/10/10 23:47:49, Nate Chapin wrote: > > > Is this the only case where you want to suppress callbacks? If so, the > scoped > > > suppressor should probably go in this if() block? > > > > loader()->cancel() is the main case I'm worried about, but we don't want other > calls like clear() or notifyObservers() to cause completion callbacks to be sent > either (e.g. if one of the observers notified in notifyObservers() adds a new > client or observer or something). > > > > Only completion callbacks like ImageResourceObserver::imageNotifyFinished() > and ResourceClient::notifyFinished() are suppressed, so other callbacks like > ImageResourceObserver::imageChanged() can still run. > > clear() is trivial, but notifyObservers() could have unpredictable side effects. > This scoping should be fine then. Acknowledged. https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/fetch/Resource.cpp (right): https://codereview.chromium.org/2407573002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/fetch/Resource.cpp:320: m_isNotifyClientsOfCompletionProhibited(false), On 2016/10/12 22:56:17, Nate Chapin wrote: > On 2016/10/11 at 02:56:56, sclittle wrote: > > On 2016/10/10 23:47:49, Nate Chapin wrote: > > > I'm suspicious this can be on ImageResource instead of Resource. I think you > can > > > stop all these notifications by hooking ImageResource::checkNotify? > Especially > > > since clients shouldn't get added/removed during cancellation if callbacks > are > > > being surpressed. Is there a case I'm forgetting about? > > > > Only the completion callbacks are being suppressed, not others like > ImageResourceObserver::imageChanged(), so IIUC it's possible that an > imageChanged() or other kind of callback gets called and adds a new > ResourceClient. > > > > We could try to suppress imageChanged callbacks too, but there are other > ImageResourceObserver callbacks that probably shouldn't be suppressed during a > reload and could still possibly add clients or observers, like > ImageResourceObserver::computeResourcePriority(). > > > > I could be missing something, but AIUI it seems easier to just handle this in > Resource than to enforce that no callbacks add observers or clients. WDYT? > > I'm not certain how much of a risk this is in practice, but assuming it is: I > think supressing in ImageResource::didAddClient and ImageResource::checkNotify > should case all caseS? Good point, sorry, I forgot about ImageResource::didAddClient. I've moved this boolean to ImageResource, and renamed it to m_isSchedulingReload to be more specific.
LGTM with a nit https://codereview.chromium.org/2407573002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): https://codereview.chromium.org/2407573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ImageResource.cpp:114: if (isLoading() || m_isSchedulingReload) This should be redundant. checkNotify() is already covered, and the only other caller, onePartInMultipartReceived(), is only done in response to an appendData() callback, which shouldn't be possible inside reloadIfLofi().
https://codereview.chromium.org/2407573002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ImageResource.cpp (right): https://codereview.chromium.org/2407573002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ImageResource.cpp:114: if (isLoading() || m_isSchedulingReload) On 2016/10/12 23:45:23, Nate Chapin wrote: > This should be redundant. checkNotify() is already covered, and the only other > caller, onePartInMultipartReceived(), is only done in response to an > appendData() callback, which shouldn't be possible inside reloadIfLofi(). Done.
The CQ bit was checked by sclittle@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/2407573002/#ps40001 (title: "removed unnecessary m_isSchedulingReload check")
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by sclittle@chromium.org
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
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by sclittle@chromium.org
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.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Wait to notify completion until after a Lo-Fi image is reloaded. Currently, if ImageResource::reloadIfLoFi() is called while that ImageResource is still loading, the resource's clients and observers are notified of completion when the current load is cancelled. After this CL, the ImageResource waits to notify of completion until the reload completes. BUG=654055 ========== to ========== Wait to notify completion until after a Lo-Fi image is reloaded. Currently, if ImageResource::reloadIfLoFi() is called while that ImageResource is still loading, the resource's clients and observers are notified of completion when the current load is cancelled. After this CL, the ImageResource waits to notify of completion until the reload completes. BUG=654055 Committed: https://crrev.com/1e457676004f8af16e0086280b73d87c1e5efba7 Cr-Commit-Position: refs/heads/master@{#424926} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1e457676004f8af16e0086280b73d87c1e5efba7 Cr-Commit-Position: refs/heads/master@{#424926} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
