|
|
Created:
3 years, 7 months ago by hiroshige Modified:
3 years, 7 months ago CC:
fs, blink-reviews, chromium-reviews, krit, fmalita+watch_chromium.org, gyuyoung2, kouhei+svg_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Stephen Chennney Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake ImageObserver::AsyncLoadCompleted() called asynchronously
- To avoid complex things (i.e. ImageNotifyFinished() calls)
during Document::ImplicitClose() to prevent timing bugs, and
- To make LoadEventFinished() of SVG's document true when
ImageNotifyFinished() is called, and make the CHECK() in
https://codereview.chromium.org/2888953004/ hold.
BUG=382170, 690480, 667641
Review-Url: https://codereview.chromium.org/2897533002
Cr-Commit-Position: refs/heads/master@{#473593}
Committed: https://chromium.googlesource.com/chromium/src/+/8952091b786ce79e9e10810d2b047d39c420d4d7
Patch Set 1 #
Total comments: 6
Patch Set 2 : Reflect Comments #
Total comments: 2
Patch Set 3 : comment fix #
Total comments: 2
Patch Set 4 : Reflect kinuko's comment #
Messages
Total messages: 36 (23 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 ========== Make ImageObserver::AsyncLoadCompleted called asynchronously BUG= ========== to ========== Make ImageObserver::AsyncLoadCompleted() called asynchronously - To avoid complex things (i.e. ImageNotifyFinished() calls) during Document::ImplicitClose() to prevent timing bugs, and - To make LoadEventFinished() of SVG's document true when ImageNotifyFinished() is called, and make the CHECK() in https://codereview.chromium.org/2888953004/ hold. BUG=382170, 690480, 667641 ==========
hiroshige@chromium.org changed reviewers: + fs@opera.com, kinuko@chromium.org, kouhei@chromium.org, siyangxiong2010@gmail.com, yhirano@chromium.org
hiroshige@chromium.org changed reviewers: - siyangxiong2010@gmail.com
hiroshige@chromium.org changed reviewers: + schenney@chromium.org, tzik@chromium.org
WDYT? fs@ and schenney@ for SVG, kinuko@ and kouhei@ for loading, tzik@ for usage of RefPtr and PostTask(): this CL creates RefPtr<SVGImage> from a raw ptr (|this|), which is a little alarming to me.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:646: ToLocalFrame(page_->MainFrame())->GetDocument()) ->GetDocument() not needed. TaskRunnerHelper::Get also takes LocalFrame.
https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:650: WeakPersistent<ImageObserver>(GetImageObserver()), Why weak?
https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:645: TaskRunnerHelper::Get(TaskType::kDOMManipulation, Since this is a task posted on a task runner belonging to the frame of the SVGImage (not the client/initiator of the image load), I'm not sure how this should be interpreted. My feeling is that this task shouldn't go on the DOM Manipulation task source though - maybe kUnspecedLoading would be more appropriate? Also related, but I can't help feeling that maybe this should rather be posting the task to itself and check and notify the observer. I.e: load_state_ = kLoadCompleted; ...->PostTask(..., WTF::Bind(&SVGImage::NotifyAsyncLoadCompleted, RefPtr<SVGImage>(this))); void SVGImage::NotifyAsyncLoadCompleted() { if (ImageObserver* observer = GetImageObserver()) observer->AsyncLoadCompleted(this); } (Not overly enthused by the RefPtr, but...) This would compartmentalize things a bit better IMHO. WDYT?
hiroshige@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:645: TaskRunnerHelper::Get(TaskType::kDOMManipulation, On 2017/05/19 09:06:49, fs wrote: > Since this is a task posted on a task runner belonging to the > frame of the SVGImage (not the client/initiator of the image > load), I'm not sure how this should be interpreted. This is because - The frame etc. of the "client/initiator of the image load" is not accecible from SVGImage here, and - This is a kind of half-deferring the DispatchDidHandleOnloadEvents() callback tied to the Document of |page_|. > My feeling is that this task shouldn't go on the DOM > Manipulation task source though - > maybe kUnspecedLoading would be more appropriate? Done, but I'm also not sure. haraken@, any suggestions about TaskType and TaskRunner to use? > void SVGImage::NotifyAsyncLoadCompleted() ... Looks nice, done. https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:646: ToLocalFrame(page_->MainFrame())->GetDocument()) On 2017/05/19 05:20:54, kouhei (back in TOK) wrote: > ->GetDocument() not needed. TaskRunnerHelper::Get also takes LocalFrame. Done. https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:650: WeakPersistent<ImageObserver>(GetImageObserver()), On 2017/05/19 06:55:29, yhirano wrote: > Why weak? I think Persistent will also work well here. I made this WeakPersistent because I think AsyncLoadCompleted() is no longer needed if ImageObserver is destructed before invocation.
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...
lgtm https://codereview.chromium.org/2897533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2897533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:646: // Document::ImplcitClose(), we defer AsyncLoadCompleted() to avoid uNit: ImplicitClose
The CQ bit was checked by hiroshige@chromium.org to run a CQ dry run
https://codereview.chromium.org/2897533002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2897533002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:646: // Document::ImplcitClose(), we defer AsyncLoadCompleted() to avoid On 2017/05/19 20:38:43, fs wrote: > uNit: ImplicitClose Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2897533002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2897533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:666: } nit: no braces for single-line body for consistency
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
kUnspecedLoading looks reasonable. LGTM.
https://codereview.chromium.org/2897533002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2897533002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:666: } On 2017/05/19 22:29:58, kinuko wrote: > nit: no braces for single-line body for consistency Done.
On 2017/05/19 02:58:27, hiroshige wrote: > WDYT? > > fs@ and schenney@ for SVG, > kinuko@ and kouhei@ for loading, > tzik@ for usage of RefPtr and PostTask(): this CL creates RefPtr<SVGImage> from > a raw ptr (|this|), which is a little alarming to me. LGTM. Unlike base::RefCountedThreadSafe, WTF::ThreadSafeRefCount doesn't delay the destruction. So, creating RefPtr from this is safe unless it's called in the dtor of SVGImage. In this case, looks safe to me.
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
The patchset sent to the CQ was uploaded after l-g-t-m from kouhei@chromium.org, fs@opera.com, kinuko@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2897533002/#ps60001 (title: "Reflect kinuko's comment")
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": 60001, "attempt_start_ts": 1495469908421590, "parent_rev": "f04717fc7e1e19ed51028bab3b1008a20962a9dc", "commit_rev": "8952091b786ce79e9e10810d2b047d39c420d4d7"}
Message was sent while issue was closed.
Description was changed from ========== Make ImageObserver::AsyncLoadCompleted() called asynchronously - To avoid complex things (i.e. ImageNotifyFinished() calls) during Document::ImplicitClose() to prevent timing bugs, and - To make LoadEventFinished() of SVG's document true when ImageNotifyFinished() is called, and make the CHECK() in https://codereview.chromium.org/2888953004/ hold. BUG=382170, 690480, 667641 ========== to ========== Make ImageObserver::AsyncLoadCompleted() called asynchronously - To avoid complex things (i.e. ImageNotifyFinished() calls) during Document::ImplicitClose() to prevent timing bugs, and - To make LoadEventFinished() of SVG's document true when ImageNotifyFinished() is called, and make the CHECK() in https://codereview.chromium.org/2888953004/ hold. BUG=382170, 690480, 667641 Review-Url: https://codereview.chromium.org/2897533002 Cr-Commit-Position: refs/heads/master@{#473593} Committed: https://chromium.googlesource.com/chromium/src/+/8952091b786ce79e9e10810d2b04... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/8952091b786ce79e9e10810d2b04... |