Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2888953004:
Check that SVGImage is loaded in ImageNotifyFinished() (Closed)
|
Created:
3 years, 7 months ago by hiroshige Modified:
3 years, 7 months ago CC:
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. |
DescriptionCheck that SVGImage is loaded in ImageNotifyFinished()
After https://codereview.chromium.org/2613853002/, SVGImage should be
completely loaded when ImageNotifyFinished() is called.
This CL explicitly checks that assumption in ImageNotifyFinished().
This is a reland of https://codereview.chromium.org/1515963005/,
with a stronger assumption of load event completion.
BUG=382170, 690480, 667641
Review-Url: https://codereview.chromium.org/2888953004
Cr-Commit-Position: refs/heads/master@{#473608}
Committed: https://chromium.googlesource.com/chromium/src/+/eec54a3ad6f426699e9aa5754b0e553de13fe75e
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 2
Patch Set 3 : Rebase #Patch Set 4 : Comments #
Depends on Patchset: Messages
Total messages: 35 (24 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 ========== Check that SVGImage is loaded in ImageNotifyFinished() BUG= ========== to ========== Check that SVGImage is loaded in ImageNotifyFinished() Reland of https://codereview.chromium.org/1515963005/ BUG= ==========
Description was changed from ========== Check that SVGImage is loaded in ImageNotifyFinished() Reland of https://codereview.chromium.org/1515963005/ BUG= ========== to ========== Check that SVGImage is loaded in ImageNotifyFinished() After https://codereview.chromium.org/2613853002/, SVGImage should be completely loaded when ImageNotifyFinished() is called. This CL explicitly checks that assumption in ImageNotifyFinished(). This is a reland of https://codereview.chromium.org/1515963005/, with a stronger assumption of load event completion. BUG=382170, 690480, 667641 ==========
hiroshige@chromium.org changed reviewers: + fs@opera.com, kinuko@chromium.org, kouhei@chromium.org, yhirano@chromium.org
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...
hiroshige@chromium.org changed reviewers: + schenney@chromium.org
PTAL. This requires [1], because without [1] ImageNotifyFinished() is called just in the middle of Document::ImplicitClose() and LoadEventFinished() is about to become true, but not yet. [1] https://codereview.chromium.org/2897533002/
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
lgtm
On 2017/05/19 at 03:00:20, hiroshige wrote: > PTAL. > > This requires [1], because without [1] ImageNotifyFinished() > is called just in the middle of Document::ImplicitClose() and > LoadEventFinished() is about to become true, but not yet. > > > [1] https://codereview.chromium.org/2897533002/ I take it this should've been a dependent PS on top of [1]? Anyway, filtering out that CL from this (left comment there on those bits), this LGTM.
pdr@chromium.org changed reviewers: + pdr@chromium.org
LGTM https://codereview.chromium.org/2888953004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/2888953004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/ImageLoader.cpp:559: ToSVGImage(image_->GetImage())->CheckLoaded(); Can you add a comment here describing why it would be dangerous for this to fail? I'd just like to help a future engineer years from now who finds this line and thinks it can be removed.
Patchset #4 (id:60001) has been deleted
> I take it this should've been a dependent PS on top of [1]? Er, yes. Rebased on top of [1]. https://codereview.chromium.org/2888953004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right): https://codereview.chromium.org/2888953004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/ImageLoader.cpp:559: ToSVGImage(image_->GetImage())->CheckLoaded(); On 2017/05/19 17:16:42, pdr. wrote: > Can you add a comment here describing why it would be dangerous for this to > fail? I'd just like to help a future engineer years from now who finds this line > and thinks it can be removed. Added comments here and in CheckLoaded(). SVG people, do these comment look reasonable?
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 2017/05/19 at 20:51:44, hiroshige wrote: > > I take it this should've been a dependent PS on top of [1]? > Er, yes. Rebased on top of [1]. > > https://codereview.chromium.org/2888953004/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right): > > https://codereview.chromium.org/2888953004/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/ImageLoader.cpp:559: ToSVGImage(image_->GetImage())->CheckLoaded(); > On 2017/05/19 17:16:42, pdr. wrote: > > Can you add a comment here describing why it would be dangerous for this to > > fail? I'd just like to help a future engineer years from now who finds this line > > and thinks it can be removed. > > Added comments here and in CheckLoaded(). > SVG people, do these comment look reasonable? LGTM
On 2017/05/19 at 20:55:22, pdr wrote: > On 2017/05/19 at 20:51:44, hiroshige wrote: > > > I take it this should've been a dependent PS on top of [1]? > > Er, yes. Rebased on top of [1]. > > > > https://codereview.chromium.org/2888953004/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/loader/ImageLoader.cpp (right): > > > > https://codereview.chromium.org/2888953004/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/loader/ImageLoader.cpp:559: ToSVGImage(image_->GetImage())->CheckLoaded(); > > On 2017/05/19 17:16:42, pdr. wrote: > > > Can you add a comment here describing why it would be dangerous for this to > > > fail? I'd just like to help a future engineer years from now who finds this line > > > and thinks it can be removed. > > > > Added comments here and in CheckLoaded(). > > SVG people, do these comment look reasonable? > > LGTM LGTM*2
lgtm
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.
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/2888953004/#ps80001 (title: "Comments")
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": 80001, "attempt_start_ts": 1495473060578440,
"parent_rev": "af7b1e7969eb07a91bc75accb5e9af2daffd4790", "commit_rev":
"eec54a3ad6f426699e9aa5754b0e553de13fe75e"}
Message was sent while issue was closed.
Description was changed from ========== Check that SVGImage is loaded in ImageNotifyFinished() After https://codereview.chromium.org/2613853002/, SVGImage should be completely loaded when ImageNotifyFinished() is called. This CL explicitly checks that assumption in ImageNotifyFinished(). This is a reland of https://codereview.chromium.org/1515963005/, with a stronger assumption of load event completion. BUG=382170, 690480, 667641 ========== to ========== Check that SVGImage is loaded in ImageNotifyFinished() After https://codereview.chromium.org/2613853002/, SVGImage should be completely loaded when ImageNotifyFinished() is called. This CL explicitly checks that assumption in ImageNotifyFinished(). This is a reland of https://codereview.chromium.org/1515963005/, with a stronger assumption of load event completion. BUG=382170, 690480, 667641 Review-Url: https://codereview.chromium.org/2888953004 Cr-Commit-Position: refs/heads/master@{#473608} Committed: https://chromium.googlesource.com/chromium/src/+/eec54a3ad6f426699e9aa5754b0e... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/chromium/src/+/eec54a3ad6f426699e9aa5754b0e... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
