Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(326)

Issue 2897533002: Make ImageObserver::AsyncLoadCompleted() called asynchronously (Closed)

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.

Description

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/+/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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+17 lines, -2 lines) Patch
M third_party/WebKit/Source/core/svg/graphics/SVGImage.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp View 1 2 3 3 chunks +16 lines, -2 lines 0 comments Download

Messages

Total messages: 36 (23 generated)
hiroshige
WDYT? fs@ and schenney@ for SVG, kinuko@ and kouhei@ for loading, tzik@ for usage of ...
3 years, 7 months ago (2017-05-19 02:58:27 UTC) #7
kouhei (in TOK)
lgtm https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp#newcode646 third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:646: ToLocalFrame(page_->MainFrame())->GetDocument()) ->GetDocument() not needed. TaskRunnerHelper::Get also takes LocalFrame.
3 years, 7 months ago (2017-05-19 05:20:54 UTC) #10
yhirano
https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp#newcode650 third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:650: WeakPersistent<ImageObserver>(GetImageObserver()), Why weak?
3 years, 7 months ago (2017-05-19 06:55:29 UTC) #11
fs
https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp#newcode645 third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:645: TaskRunnerHelper::Get(TaskType::kDOMManipulation, Since this is a task posted on a ...
3 years, 7 months ago (2017-05-19 09:06:49 UTC) #12
hiroshige
https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2897533002/diff/1/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp#newcode645 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 ...
3 years, 7 months ago (2017-05-19 20:28:53 UTC) #14
fs
lgtm https://codereview.chromium.org/2897533002/diff/20001/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2897533002/diff/20001/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp#newcode646 third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:646: // Document::ImplcitClose(), we defer AsyncLoadCompleted() to avoid uNit: ...
3 years, 7 months ago (2017-05-19 20:38:43 UTC) #17
hiroshige
https://codereview.chromium.org/2897533002/diff/20001/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2897533002/diff/20001/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp#newcode646 third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:646: // Document::ImplcitClose(), we defer AsyncLoadCompleted() to avoid On 2017/05/19 ...
3 years, 7 months ago (2017-05-19 20:54:15 UTC) #19
kinuko
lgtm https://codereview.chromium.org/2897533002/diff/40001/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2897533002/diff/40001/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp#newcode666 third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:666: } nit: no braces for single-line body for ...
3 years, 7 months ago (2017-05-19 22:29:59 UTC) #21
haraken
kUnspecedLoading looks reasonable. LGTM.
3 years, 7 months ago (2017-05-19 23:08:08 UTC) #24
hiroshige
https://codereview.chromium.org/2897533002/diff/40001/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp File third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp (right): https://codereview.chromium.org/2897533002/diff/40001/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp#newcode666 third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp:666: } On 2017/05/19 22:29:58, kinuko wrote: > nit: no ...
3 years, 7 months ago (2017-05-20 01:06:26 UTC) #25
tzik
On 2017/05/19 02:58:27, hiroshige wrote: > WDYT? > > fs@ and schenney@ for SVG, > ...
3 years, 7 months ago (2017-05-22 02:07:54 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2897533002/60001
3 years, 7 months ago (2017-05-22 16:18:52 UTC) #33
commit-bot: I haz the power
3 years, 7 months ago (2017-05-22 16:24:26 UTC) #36
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8952091b786ce79e9e10810d2b04...

Powered by Google App Engine
This is Rietveld 408576698