Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1775303003:
Display appropriate graphic for crashed out-of-process iframe. (Closed)
|
Created:
4 years, 9 months ago by lfg Modified:
4 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisplay appropriate graphic for crashed out-of-process iframe.
BUG=561006
Committed: https://crrev.com/cccffbea4b4f1d847e465f46db83eaeb9bc464c5
Cr-Commit-Position: refs/heads/master@{#380266}
Patch Set 1 #
Total comments: 7
Patch Set 2 : addressing comments #Patch Set 3 : make sure web_layer_ exists #Patch Set 4 : switching to skcolor_black #Patch Set 5 : sad_bitmap is not available in content_shell, only paint a black frame in that case #Messages
Total messages: 25 (9 generated)
lfg@chromium.org changed reviewers: + kenrb@chromium.org
PTAL.
fsamuel@chromium.org changed reviewers: + fsamuel@chromium.org
Drive by cleanup request: You can now delete BrowserPlugin::paint, I think! Yay! I've been wanting to delete this for AGES.
On 2016/03/09 17:01:03, Fady Samuel wrote: > Drive by cleanup request: > > You can now delete BrowserPlugin::paint, I think! Yay! I've been wanting to > delete this for AGES. Yes, will work in a follow-up patch.
Description was changed from ========== Display appropriate graphic for crashed out-of-process iframe. BUG=561006 ========== to ========== Display appropriate graphic for crashed out-of-process iframe. BUG=561006 ==========
lgtm with some minor comments. https://codereview.chromium.org/1775303003/diff/1/content/renderer/child_fram... File content/renderer/child_frame_compositing_helper.cc (right): https://codereview.chromium.org/1775303003/diff/1/content/renderer/child_fram... content/renderer/child_frame_compositing_helper.cc:135: (buffer_size_.height() - sad_bitmap->height()) / 2)); I think this is right but I'm not 100% sure... both buffer_size_ and SkBitmap dimensions are in physical pixels and not DIP, right? I would suggest making sure this positions correctly on a retina display if you haven't already. https://codereview.chromium.org/1775303003/diff/1/content/renderer/child_fram... content/renderer/child_frame_compositing_helper.cc:138: crashed_layer->AddChild(sad_layer); As discussed, we might want to not set this layer if the bitmap does not fit in the iframe rect?
lukasza@chromium.org changed reviewers: + lukasza@chromium.org
Drive-by comment / question :-) https://codereview.chromium.org/1775303003/diff/1/content/renderer/child_fram... File content/renderer/child_frame_compositing_helper.cc (right): https://codereview.chromium.org/1775303003/diff/1/content/renderer/child_fram... content/renderer/child_frame_compositing_helper.cc:138: crashed_layer->AddChild(sad_layer); On 2016/03/09 17:58:37, kenrb wrote: > As discussed, we might want to not set this layer if the bitmap does not fit in > the iframe rect? Can the iframe rect change its dimensions during the lifetime of the frame (and the lifetime of its associated sad layer)? (this is a drive-by comment from somebody not familiar with how this works - feel free to ignore).
On 2016/03/09 18:12:19, Łukasz Anforowicz wrote: > Can the iframe rect change its dimensions during the lifetime of the frame (and > the lifetime of its associated sad layer)? (this is a drive-by comment from > somebody not familiar with how this works - feel free to ignore). Yes it can. This patch doesn't handle that case, the sad graphic will be clipped if the iframe is resized to a size too small to fit the original size of the iframe. I looked into fixing that, but it's non-trivial. We would either need to have a hook and update the position every frame, or add a new type of layer to cc/ that allows for that to happen (nine_patch_layer is close, but doesn't quite solve it). An alternative is to scale the graphic to fit the entire iframe. That's possible, but for large iframes it may look a bit ugly, and for aspect ratios that are too different from the original image it would look stretched.
Ken, please take another look. https://codereview.chromium.org/1775303003/diff/1/content/renderer/child_fram... File content/renderer/child_frame_compositing_helper.cc (right): https://codereview.chromium.org/1775303003/diff/1/content/renderer/child_fram... content/renderer/child_frame_compositing_helper.cc:135: (buffer_size_.height() - sad_bitmap->height()) / 2)); On 2016/03/09 17:58:37, kenrb wrote: > I think this is right but I'm not 100% sure... both buffer_size_ and SkBitmap > dimensions are in physical pixels and not DIP, right? I would suggest making > sure this positions correctly on a retina display if you haven't already. Good catch. buffer_size_ is in DIPs, I switched to use the size of the current web_layer_, which is in physical pixels. The web_layer_ may not exist if the process crash before OnSetSurface, so I also added a check for web_layer_. https://codereview.chromium.org/1775303003/diff/1/content/renderer/child_fram... content/renderer/child_frame_compositing_helper.cc:138: crashed_layer->AddChild(sad_layer); On 2016/03/09 17:58:37, kenrb wrote: > As discussed, we might want to not set this layer if the bitmap does not fit in > the iframe rect? Done.
Great. lgtm
Great. lgtm
lfg@chromium.org changed reviewers: + piman@chromium.org
Hey piman, please take a look.
Drive by. https://codereview.chromium.org/1775303003/diff/1/content/renderer/child_fram... File content/renderer/child_frame_compositing_helper.cc (right): https://codereview.chromium.org/1775303003/diff/1/content/renderer/child_fram... content/renderer/child_frame_compositing_helper.cc:124: crashed_layer->SetBackgroundColor(SkColorSetARGBInline(255, 0, 0, 0)); Consider just using SK_ColorBLACK if that works.
lgtm
https://codereview.chromium.org/1775303003/diff/1/content/renderer/child_fram... File content/renderer/child_frame_compositing_helper.cc (right): https://codereview.chromium.org/1775303003/diff/1/content/renderer/child_fram... content/renderer/child_frame_compositing_helper.cc:124: crashed_layer->SetBackgroundColor(SkColorSetARGBInline(255, 0, 0, 0)); On 2016/03/09 21:06:24, dcheng wrote: > Consider just using SK_ColorBLACK if that works. Done.
The CQ bit was checked by lfg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1775303003/#ps80001 (title: "sad_bitmap is not available in content_shell, only paint a black frame in that case")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1775303003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1775303003/80001
Message was sent while issue was closed.
Description was changed from ========== Display appropriate graphic for crashed out-of-process iframe. BUG=561006 ========== to ========== Display appropriate graphic for crashed out-of-process iframe. BUG=561006 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Display appropriate graphic for crashed out-of-process iframe. BUG=561006 ========== to ========== Display appropriate graphic for crashed out-of-process iframe. BUG=561006 Committed: https://crrev.com/cccffbea4b4f1d847e465f46db83eaeb9bc464c5 Cr-Commit-Position: refs/heads/master@{#380266} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/cccffbea4b4f1d847e465f46db83eaeb9bc464c5 Cr-Commit-Position: refs/heads/master@{#380266} |
