Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2746343002:
Phase III Step 1: Make ImageResourceContent manage its own ResourceStatus (Closed)
DescriptionMake ImageResourceContent manage its own ResourceStatus
Design doc:
https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8bE/edit#
1. This CL
- Adds ImageResourceContent::content_status_,
- Renames ImageResourceContent::GetStatus() to GetContentStatus(), and
- Makes them updated independently from Resource::status_/GetStatus().
This is to decouple GetStatus() timing dependencies
between ImageResource and ImageResourceContent, particularly to allow
ImageResourceContent::GetContentStatus() to be changed to kCached
(in https://codereview.chromium.org/2613853002/) when SVG image loading
is really finished, not necessarily when ResourceLoader is finished.
2. In order to change the content state to kPending when loading is
started, NotifyStartLoad() notifications are introduced.
3. Stricter assertions are added to |content_status_| transitions.
|SetStatus(kPending)| and NotifyStartLoad() calls are added to
non-ResourceLoader paths to make the state transition consistent.
4. This CL makes ImageResourceContent::GetContentStatus() to be kCached
before ImageNotifyFinished() is called in successful loading,
while Resource::GetStatus() is kPending at that time.
BUG=667641, 382170, 690480
Review-Url: https://codereview.chromium.org/2746343002
Cr-Commit-Position: refs/heads/master@{#470104}
Committed: https://chromium.googlesource.com/chromium/src/+/ebd2fa73e3046f3d6397545285a41fa1d78c4be8
Patch Set 1 #Patch Set 2 : Resurrect notifyStartLoad(); should be failing #Patch Set 3 : ImageResourceContent::notifyLoad() #Patch Set 4 : Rewind #
Total comments: 17
Patch Set 5 : Reflect comments #Patch Set 6 : Rebase after Blink Renaming #Patch Set 7 : Fix tests, asserts #Patch Set 8 : Refine, rename #Patch Set 9 : minor #Patch Set 10 : Refine #Patch Set 11 : Refine #Patch Set 12 : fix #Patch Set 13 : Rebase #
Total comments: 6
Patch Set 14 : Reflect comments #Dependent Patchsets: Messages
Total messages: 69 (58 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 unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Description was changed from ========== Make ImageResourceContent to manage its own ResourceStatus BUG= ========== to ========== Make ImageResourceContent to manage its own ResourceStatus BUG= ==========
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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 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 ========== Make ImageResourceContent to manage its own ResourceStatus BUG= ========== to ========== Make ImageResourceContent to manage its own ResourceStatus BUG=667641 ==========
Description was changed from ========== Make ImageResourceContent to manage its own ResourceStatus BUG=667641 ========== to ========== Make ImageResourceContent to manage its own ResourceStatus BUG=667641, 382170 ==========
Description was changed from ========== Make ImageResourceContent to manage its own ResourceStatus BUG=667641, 382170 ========== to ========== Make ImageResourceContent to manage its own ResourceStatus This CL adds ImageResourceContent::m_status and makes it updated in ImageResourceContent::updateImage(), independently from Resource::m_status. This is to decouple getStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::getStatus() to be changed to Cached (in a subsequent CL) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. BUG=667641, 382170 ==========
Description was changed from ========== Make ImageResourceContent to manage its own ResourceStatus This CL adds ImageResourceContent::m_status and makes it updated in ImageResourceContent::updateImage(), independently from Resource::m_status. This is to decouple getStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::getStatus() to be changed to Cached (in a subsequent CL) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. BUG=667641, 382170 ========== to ========== Make ImageResourceContent to manage its own ResourceStatus This CL adds ImageResourceContent::m_status and makes it updated in ImageResourceContent::updateImage(), independently from Resource::m_status. This is to decouple getStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::getStatus() to be changed to Cached (in a subsequent CL) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. This CL makes ImageResourceContent::getStatus() to be Cached when imageNotifyFinished() is called for successful loading. Previously getStatus() has been Pending in such cases. BUG=667641, 382170 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Make ImageResourceContent to manage its own ResourceStatus This CL adds ImageResourceContent::m_status and makes it updated in ImageResourceContent::updateImage(), independently from Resource::m_status. This is to decouple getStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::getStatus() to be changed to Cached (in a subsequent CL) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. This CL makes ImageResourceContent::getStatus() to be Cached when imageNotifyFinished() is called for successful loading. Previously getStatus() has been Pending in such cases. BUG=667641, 382170 ========== to ========== Make ImageResourceContent to manage its own ResourceStatus This CL adds ImageResourceContent::m_status and makes it updated independently from Resource::m_status. This is to decouple getStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::getStatus() to be changed to Cached (in a subsequent CL) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. Resource::notifyStartLoad() and ImageResourceContent::notifyStartLoad() are introduced to update ImageResourceContent::m_status to Pending when actual loading is started. This CL makes ImageResourceContent::getStatus() to be Cached when imageNotifyFinished() is called for successful loading. Previously getStatus() has been Pending in such cases. BUG=667641, 382170 ==========
Description was changed from ========== Make ImageResourceContent to manage its own ResourceStatus This CL adds ImageResourceContent::m_status and makes it updated independently from Resource::m_status. This is to decouple getStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::getStatus() to be changed to Cached (in a subsequent CL) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. Resource::notifyStartLoad() and ImageResourceContent::notifyStartLoad() are introduced to update ImageResourceContent::m_status to Pending when actual loading is started. This CL makes ImageResourceContent::getStatus() to be Cached when imageNotifyFinished() is called for successful loading. Previously getStatus() has been Pending in such cases. BUG=667641, 382170 ========== to ========== Make ImageResourceContent to manage its own ResourceStatus Design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... This CL adds ImageResourceContent::m_status and makes it updated independently from Resource::m_status. This is to decouple getStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::getStatus() to be changed to Cached (in a subsequent CL) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. Resource::notifyStartLoad() and ImageResourceContent::notifyStartLoad() are introduced to update ImageResourceContent::m_status to Pending when actual loading is started. This CL makes ImageResourceContent::getStatus() to be Cached when imageNotifyFinished() is called for successful loading. Previously getStatus() has been Pending in such cases. BUG=667641, 382170 ==========
hiroshige@chromium.org changed reviewers: + japhet@chromium.org, kouhei@chromium.org, yhirano@chromium.org
PTAL.
https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/resource/MockImageResourceObserver.cpp (right): https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/MockImageResourceObserver.cpp:44: DCHECK(image->isLoaded()); This CL enables this DCHECK(). (Previously this fails on successful loads)
https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp (right): https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:298: // When |ShouldNotifyFinish|, we set |m_status| to a loaded Can we have switch(status) here to ensure all status is covered? (w/ NOTREACHED() where needed) https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:314: if (m_status == ResourceStatus::NotStarted) Ditto https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:396: m_status = ResourceStatus::Pending; Can we always update status via updateStatus? https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/loader/fetch/Resource.h (right): https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/loader/fetch/Resource.h:343: // Do not cause any JavaScript calls or client/observer notifications. Would you also document what the hook is for? Also, "JavaScript calls or client/observer notifications are disallowed inside callback." https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:1272: resource->notifyStartLoad(); m_isAddRemoveClientProhibited? ScriptForbiddenScope?
https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp (right): https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:314: if (m_status == ResourceStatus::NotStarted) Is |status| arbitrary here? Can we put some DCHECKs? https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:314: if (m_status == ResourceStatus::NotStarted) Can you tell me why this is needed? Setting the status to Pending is basically done by notifyStartLoad(), right? https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:395: void ImageResourceContent::notifyStartLoad() { DCHECK_EQ(m_status, NotStarted)?
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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 ========== Make ImageResourceContent to manage its own ResourceStatus Design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... This CL adds ImageResourceContent::m_status and makes it updated independently from Resource::m_status. This is to decouple getStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::getStatus() to be changed to Cached (in a subsequent CL) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. Resource::notifyStartLoad() and ImageResourceContent::notifyStartLoad() are introduced to update ImageResourceContent::m_status to Pending when actual loading is started. This CL makes ImageResourceContent::getStatus() to be Cached when imageNotifyFinished() is called for successful loading. Previously getStatus() has been Pending in such cases. BUG=667641, 382170 ========== to ========== Make ImageResourceContent to manage its own ResourceStatus Design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... This CL adds ImageResourceContent::content_status_/GetContentStatus() and makes it updated independently from Resource::status_/GetStatus(). This is to decouple GetStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::GetContentStatus() to be changed to Cached (in a subsequent CL) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. This CL makes ImageResourceContent::GetContentStatus() to be Cached when imageNotifyFinished() is called for successful loading. Previously getStatus() has been Pending in such cases. This CL also adds stricter assertions on the state transition of |content_status_|, and causes more SetStatus(kPending) and NotifyStartLoad() calls to non-ResourceLoader paths to make the state transition consistent. Resource::NotifyStartLoad() and ImageResourceContent::NotifyStartLoad() are introduced to update ImageResourceContent::content_status_ to Pending when actual loading is started. BUG=667641, 382170 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Make ImageResourceContent to manage its own ResourceStatus Design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... This CL adds ImageResourceContent::content_status_/GetContentStatus() and makes it updated independently from Resource::status_/GetStatus(). This is to decouple GetStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::GetContentStatus() to be changed to Cached (in a subsequent CL) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. This CL makes ImageResourceContent::GetContentStatus() to be Cached when imageNotifyFinished() is called for successful loading. Previously getStatus() has been Pending in such cases. This CL also adds stricter assertions on the state transition of |content_status_|, and causes more SetStatus(kPending) and NotifyStartLoad() calls to non-ResourceLoader paths to make the state transition consistent. Resource::NotifyStartLoad() and ImageResourceContent::NotifyStartLoad() are introduced to update ImageResourceContent::content_status_ to Pending when actual loading is started. BUG=667641, 382170 ========== to ========== Make ImageResourceContent to manage its own ResourceStatus Design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... 1. This CL - Adds ImageResourceContent::content_status_, - Renames ImageResourceContent::GetStatus() to GetContentStatus(), and - Makes them updated independently from Resource::status_/GetStatus(). This is to decouple GetStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::GetContentStatus() to be changed to kCached (in https://codereview.chromium.org/2613853002/) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. 2. In order to change the content state to kPending when loading is started, NotifyStartLoad() notifications are introduced. 3. This CL also adds stricter assertions on the state transition of |content_status_|, and causes more SetStatus(kPending) and NotifyStartLoad() calls to non-ResourceLoader paths to make the state transition consistent. This CL makes ImageResourceContent::GetContentStatus() to be Cached when imageNotifyFinished() is called for successful loading. Previously getStatus() has been Pending in such cases. BUG=667641, 382170 ==========
Description was changed from ========== Make ImageResourceContent to manage its own ResourceStatus Design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... 1. This CL - Adds ImageResourceContent::content_status_, - Renames ImageResourceContent::GetStatus() to GetContentStatus(), and - Makes them updated independently from Resource::status_/GetStatus(). This is to decouple GetStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::GetContentStatus() to be changed to kCached (in https://codereview.chromium.org/2613853002/) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. 2. In order to change the content state to kPending when loading is started, NotifyStartLoad() notifications are introduced. 3. This CL also adds stricter assertions on the state transition of |content_status_|, and causes more SetStatus(kPending) and NotifyStartLoad() calls to non-ResourceLoader paths to make the state transition consistent. This CL makes ImageResourceContent::GetContentStatus() to be Cached when imageNotifyFinished() is called for successful loading. Previously getStatus() has been Pending in such cases. BUG=667641, 382170 ========== to ========== Make ImageResourceContent to manage its own ResourceStatus Design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... 1. This CL - Adds ImageResourceContent::content_status_, - Renames ImageResourceContent::GetStatus() to GetContentStatus(), and - Makes them updated independently from Resource::status_/GetStatus(). This is to decouple GetStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::GetContentStatus() to be changed to kCached (in https://codereview.chromium.org/2613853002/) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. 2. In order to change the content state to kPending when loading is started, NotifyStartLoad() notifications are introduced. 3. Stricter assertions are added to |content_status_| transitions. |SetStatus(kPending)| and NotifyStartLoad() calls are added to non-ResourceLoader paths to make the state transition consistent. 4. This CL makes ImageResourceContent::GetContentStatus() to be kCached before ImageNotifyFinished() is called in successful loading, while Resource::GetStatus() is kPending at that time. BUG=667641, 382170 ==========
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...
Restarting the activity. Main changes since last review comments: - Renamed ImageResourceContent::status_/GetStatus() to content_status_/GetContentStatus(). - Changed the state transition diagram and added the new diagram to the design doc and as comments in ImageResourceContent.h. content_status_ is now changed only when load starts (in NotifyStartLoad()) and when load finishes (in UpdateToLoadedContentStatus()). - Added stricter assertions on state transitions. PTAL again. https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp (right): https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:298: // When |ShouldNotifyFinish|, we set |m_status| to a loaded On 2017/03/15 11:18:53, kouhei (on vacation) wrote: > Can we have switch(status) here to ensure all status is covered? (w/ > NOTREACHED() where needed) Done. https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:314: if (m_status == ResourceStatus::NotStarted) On 2017/03/17 13:47:25, yhirano (away May 3 - May 5) wrote: > Can you tell me why this is needed? Setting the status to Pending is basically > done by notifyStartLoad(), right? Removed. https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:314: if (m_status == ResourceStatus::NotStarted) On 2017/03/17 13:47:25, yhirano (away May 3 - May 5) wrote: > Is |status| arbitrary here? Can we put some DCHECKs? |status| shouldn't be kNotStarted. Also, |status| should be kPending, except for one callsite of UpdateImage() in ReloadIfLoFiOrPlaceholderImage(). We might consider adding DCHECK_EQ(status, kPending) if it is not called from ReloadIfLoFiOrPlaceholderImage(), but this complicates the concept (and code) of state transition a little bit, and I don't have a clear reason to forbid updating the image data after the state changed to e.g. kCached. Therefore, I only added a CHECK() for kNotStarted (in UpdateImage()) in the latest Patch Set. https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:314: if (m_status == ResourceStatus::NotStarted) On 2017/03/17 13:47:25, yhirano (away May 3 - May 5) wrote: > Can you tell me why this is needed? Setting the status to Pending is basically > done by notifyStartLoad(), right? Right. I removed the transition in DoNotNotifyFinish. https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:395: void ImageResourceContent::notifyStartLoad() { On 2017/03/17 13:47:25, yhirano wrote: > DCHECK_EQ(m_status, NotStarted)? Not necessarily NotStarted, e.g. in the case of revalidation. https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.cpp:396: m_status = ResourceStatus::Pending; On 2017/03/15 11:18:53, kouhei wrote: > Can we always update status via updateStatus? I removed UpdateStatus(kDoNotNotifyFinish) case and made NotifyStartLoad() here the single point of the state transision to kPending. https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/loader/fetch/Resource.h (right): https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/loader/fetch/Resource.h:343: // Do not cause any JavaScript calls or client/observer notifications. On 2017/03/15 11:18:53, kouhei wrote: > Would you also document what the hook is for? > Also, "JavaScript calls or client/observer notifications are disallowed inside > callback." Done. https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2746343002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:1272: resource->notifyStartLoad(); On 2017/03/15 11:18:53, kouhei wrote: > m_isAddRemoveClientProhibited? > ScriptForbiddenScope? Done.
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 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 "Make ImageResourceContent to manage its own ResourceStatus" "to" is not needed? https://codereview.chromium.org/2746343002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right): https://codereview.chromium.org/2746343002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:354: CHECK_EQ(GetStatus(), ResourceStatus::kPending); Is it better to have this CHECK at the beginning of the function? https://codereview.chromium.org/2746343002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h (right): https://codereview.chromium.org/2746343002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h:106: // ImageResourceContent::getContentStatus() can be different from CamelCase
lgtm https://codereview.chromium.org/2746343002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h (right): https://codereview.chromium.org/2746343002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h:107: // ImageResource::getStatus(). Use ImageResourceContent::getContentStatus(). I'd also add link to https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8...
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 ========== Make ImageResourceContent to manage its own ResourceStatus Design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... 1. This CL - Adds ImageResourceContent::content_status_, - Renames ImageResourceContent::GetStatus() to GetContentStatus(), and - Makes them updated independently from Resource::status_/GetStatus(). This is to decouple GetStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::GetContentStatus() to be changed to kCached (in https://codereview.chromium.org/2613853002/) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. 2. In order to change the content state to kPending when loading is started, NotifyStartLoad() notifications are introduced. 3. Stricter assertions are added to |content_status_| transitions. |SetStatus(kPending)| and NotifyStartLoad() calls are added to non-ResourceLoader paths to make the state transition consistent. 4. This CL makes ImageResourceContent::GetContentStatus() to be kCached before ImageNotifyFinished() is called in successful loading, while Resource::GetStatus() is kPending at that time. BUG=667641, 382170 ========== to ========== Make ImageResourceContent manage its own ResourceStatus Design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... 1. This CL - Adds ImageResourceContent::content_status_, - Renames ImageResourceContent::GetStatus() to GetContentStatus(), and - Makes them updated independently from Resource::status_/GetStatus(). This is to decouple GetStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::GetContentStatus() to be changed to kCached (in https://codereview.chromium.org/2613853002/) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. 2. In order to change the content state to kPending when loading is started, NotifyStartLoad() notifications are introduced. 3. Stricter assertions are added to |content_status_| transitions. |SetStatus(kPending)| and NotifyStartLoad() calls are added to non-ResourceLoader paths to make the state transition consistent. 4. This CL makes ImageResourceContent::GetContentStatus() to be kCached before ImageNotifyFinished() is called in successful loading, while Resource::GetStatus() is kPending at that time. BUG=667641, 382170 ==========
> "Make ImageResourceContent to manage its own ResourceStatus" > "to" is not needed? Done, https://codereview.chromium.org/2746343002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResource.cpp (right): https://codereview.chromium.org/2746343002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResource.cpp:354: CHECK_EQ(GetStatus(), ResourceStatus::kPending); On 2017/05/08 05:28:51, yhirano wrote: > Is it better to have this CHECK at the beginning of the function? Done. Actually the if statement is not needed here and thus I removed it. (It will be needed after LoFi reload refactoring is done, not now.) https://codereview.chromium.org/2746343002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h (right): https://codereview.chromium.org/2746343002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h:106: // ImageResourceContent::getContentStatus() can be different from On 2017/05/08 05:28:51, yhirano wrote: > CamelCase Done. https://codereview.chromium.org/2746343002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/resource/ImageResourceContent.h:107: // ImageResource::getStatus(). Use ImageResourceContent::getContentStatus(). On 2017/05/08 05:36:22, kouhei (on transit) wrote: > I'd also add link to > https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... Done.
The CQ bit was unchecked by hiroshige@chromium.org
The CQ bit was checked by hiroshige@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org, yhirano@chromium.org Link to the patchset: https://codereview.chromium.org/2746343002/#ps260001 (title: "Reflect comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Make ImageResourceContent manage its own ResourceStatus Design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... 1. This CL - Adds ImageResourceContent::content_status_, - Renames ImageResourceContent::GetStatus() to GetContentStatus(), and - Makes them updated independently from Resource::status_/GetStatus(). This is to decouple GetStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::GetContentStatus() to be changed to kCached (in https://codereview.chromium.org/2613853002/) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. 2. In order to change the content state to kPending when loading is started, NotifyStartLoad() notifications are introduced. 3. Stricter assertions are added to |content_status_| transitions. |SetStatus(kPending)| and NotifyStartLoad() calls are added to non-ResourceLoader paths to make the state transition consistent. 4. This CL makes ImageResourceContent::GetContentStatus() to be kCached before ImageNotifyFinished() is called in successful loading, while Resource::GetStatus() is kPending at that time. BUG=667641, 382170 ========== to ========== Make ImageResourceContent manage its own ResourceStatus Design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... 1. This CL - Adds ImageResourceContent::content_status_, - Renames ImageResourceContent::GetStatus() to GetContentStatus(), and - Makes them updated independently from Resource::status_/GetStatus(). This is to decouple GetStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::GetContentStatus() to be changed to kCached (in https://codereview.chromium.org/2613853002/) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. 2. In order to change the content state to kPending when loading is started, NotifyStartLoad() notifications are introduced. 3. Stricter assertions are added to |content_status_| transitions. |SetStatus(kPending)| and NotifyStartLoad() calls are added to non-ResourceLoader paths to make the state transition consistent. 4. This CL makes ImageResourceContent::GetContentStatus() to be kCached before ImageNotifyFinished() is called in successful loading, while Resource::GetStatus() is kPending at that time. BUG=667641, 382170, 690480 ==========
The CQ bit was unchecked by hiroshige@chromium.org
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": 260001, "attempt_start_ts": 1494271566140140,
"parent_rev": "a4b69e228383bae8fb9c44ce9b11e7800766ed55", "commit_rev":
"ebd2fa73e3046f3d6397545285a41fa1d78c4be8"}
Message was sent while issue was closed.
Description was changed from ========== Make ImageResourceContent manage its own ResourceStatus Design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... 1. This CL - Adds ImageResourceContent::content_status_, - Renames ImageResourceContent::GetStatus() to GetContentStatus(), and - Makes them updated independently from Resource::status_/GetStatus(). This is to decouple GetStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::GetContentStatus() to be changed to kCached (in https://codereview.chromium.org/2613853002/) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. 2. In order to change the content state to kPending when loading is started, NotifyStartLoad() notifications are introduced. 3. Stricter assertions are added to |content_status_| transitions. |SetStatus(kPending)| and NotifyStartLoad() calls are added to non-ResourceLoader paths to make the state transition consistent. 4. This CL makes ImageResourceContent::GetContentStatus() to be kCached before ImageNotifyFinished() is called in successful loading, while Resource::GetStatus() is kPending at that time. BUG=667641, 382170, 690480 ========== to ========== Make ImageResourceContent manage its own ResourceStatus Design doc: https://docs.google.com/document/d/1O-fB83mrE0B_V8gzXNqHgmRLCvstTB4MMi3RnVLr8... 1. This CL - Adds ImageResourceContent::content_status_, - Renames ImageResourceContent::GetStatus() to GetContentStatus(), and - Makes them updated independently from Resource::status_/GetStatus(). This is to decouple GetStatus() timing dependencies between ImageResource and ImageResourceContent, particularly to allow ImageResourceContent::GetContentStatus() to be changed to kCached (in https://codereview.chromium.org/2613853002/) when SVG image loading is really finished, not necessarily when ResourceLoader is finished. 2. In order to change the content state to kPending when loading is started, NotifyStartLoad() notifications are introduced. 3. Stricter assertions are added to |content_status_| transitions. |SetStatus(kPending)| and NotifyStartLoad() calls are added to non-ResourceLoader paths to make the state transition consistent. 4. This CL makes ImageResourceContent::GetContentStatus() to be kCached before ImageNotifyFinished() is called in successful loading, while Resource::GetStatus() is kPending at that time. BUG=667641, 382170, 690480 Review-Url: https://codereview.chromium.org/2746343002 Cr-Commit-Position: refs/heads/master@{#470104} Committed: https://chromium.googlesource.com/chromium/src/+/ebd2fa73e3046f3d6397545285a4... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/ebd2fa73e3046f3d6397545285a4... |
