New issue
Advanced search Search tips

Issue 868444 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 27
Cc:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug



Sign in to add a comment

virtual/scalefactor200/fast/hidpi/static/validation-bubble-appearance-hidpi.html failing on WebKit Mac10.13 (retina)

Project Member Reported by sky@chromium.org, Jul 27

Issue description

This started here: https://ci.chromium.org/buildbot/chromium.webkit/WebKit%20Mac10.13%20%28retina%29/263 .

Failure looks to be because of golden images not matching:

9:34:53.543 49202 "/b/s/w/ir/out/Release/image_diff --diff /b/s/w/itqSin9p/tmp_EMj8D/actual.png /b/s/w/itqSin9p/tmp_EMj8D/expected.png /b/s/w/itqSin9p/tmp_EMj8D/diff.png" took 0.20s
09:34:53.546 49154 [1087/5973] virtual/scalefactor200/fast/hidpi/static/validation-bubble-appearance-hidpi.html failed unexpectedly (image diff)
09:34:53.545 49202 worker/6 virtual/scalefactor200/fast/hidpi/static/validation-bubble-appearance-hidpi.html failed:

https://chromium-review.googlesource.com/c/chromium/src/+/1150673 changes around code related to ScreenInfo, which seems like it may have impacted this. I'm going to revert
 
Cc: a...@chromium.org
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ae08f33b34db0a95b188315f64f6bda747e77351

commit ae08f33b34db0a95b188315f64f6bda747e77351
Author: Scott Violet <sky@chromium.org>
Date: Fri Jul 27 17:37:23 2018

Revert "Move some of VisualProperties to RenderWidget::Init()."

This reverts commit 628bdce65237022927e4694d55cb4df8fd0af297.

Reason for revert: virtual/scalefactor200/fast/hidpi/static/validation-bubble-appearance-hidpi.html started failing on WebKit Mac10.13 (retina) and this patch is on the blame list and seems related. Output isn't that useful, other than to say golden images don't match:

9:34:53.543 49202 "/b/s/w/ir/out/Release/image_diff --diff /b/s/w/itqSin9p/tmp_EMj8D/actual.png /b/s/w/itqSin9p/tmp_EMj8D/expected.png /b/s/w/itqSin9p/tmp_EMj8D/diff.png" took 0.20s
09:34:53.546 49154 [1087/5973] virtual/scalefactor200/fast/hidpi/static/validation-bubble-appearance-hidpi.html failed unexpectedly (image diff)
09:34:53.545 49202 worker/6 virtual/scalefactor200/fast/hidpi/static/validation-bubble-appearance-hidpi.html failed:

Original change's description:
> Move some of VisualProperties to RenderWidget::Init().
> 
> VisualProperties are passed to the RenderViewImpl::Create() method via
> the IPC params, then are passed along to the RenderWidget, but in a
> lot of different steps.
> 
> 1st, screen info is passed to the RenderWidget constructor.
> 2nd, the display mode is set directly on the RenderWidget.
> [ a bunch of other initialization here ]
> 3rd, the whole set of properties is passed to
> OnSynchronizeVisualProperties like an IPC was just received.
> 
> The 3rd step sends the screen info and the display mode again, though
> they won't be changed of course.
> 
> This combines step 1 and 2 into one step where they are passed to the
> RenderWidget::Init() method. Then we can look at combining with step 3.
> 
> R=​ajwong@chromium.org, avi@chromium.org
> 
> Bug: 419087
> Change-Id: Ifcb8eaa0915bb235172e8c5b7bee188e863c18c3
> Reviewed-on: https://chromium-review.googlesource.com/1150673
> Commit-Queue: danakj <danakj@chromium.org>
> Reviewed-by: Albert J. Wong <ajwong@chromium.org>
> Reviewed-by: Avi Drissman <avi@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#578660}

TBR=avi@chromium.org,ajwong@chromium.org,danakj@chromium.org

Change-Id: Ife47ce2124a0a5baffaa2dd08b3dfe7de79cc568
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 419087,  868444 
Reviewed-on: https://chromium-review.googlesource.com/1153398
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578694}
[modify] https://crrev.com/ae08f33b34db0a95b188315f64f6bda747e77351/content/renderer/render_view_impl.cc
[modify] https://crrev.com/ae08f33b34db0a95b188315f64f6bda747e77351/content/renderer/render_widget.cc
[modify] https://crrev.com/ae08f33b34db0a95b188315f64f6bda747e77351/content/renderer/render_widget.h
[modify] https://crrev.com/ae08f33b34db0a95b188315f64f6bda747e77351/content/renderer/render_widget_fullscreen_pepper.cc
[modify] https://crrev.com/ae08f33b34db0a95b188315f64f6bda747e77351/content/renderer/render_widget_fullscreen_pepper.h
[modify] https://crrev.com/ae08f33b34db0a95b188315f64f6bda747e77351/content/renderer/render_widget_unittest.cc
[modify] https://crrev.com/ae08f33b34db0a95b188315f64f6bda747e77351/content/test/layouttest_support.cc

Labels: -Sheriff-Chromium
The bot did go green, so it looks like Avi's patch did trigger this. Removing Sheriff-Chromium.
Cc: -a...@chromium.org
Owner: a...@chromium.org
Status: Assigned (was: Untriaged)
As the bot is green, I'm also moving to Fixed.
Cc: a...@chromium.org
Owner: danakj@chromium.org
Status: Fixed (was: Assigned)
Um...

628bdce65237022927e4694d55cb4df8fd0af297 is not me. I reviewed it, but it was Dana.

Also, given the revert worked, let's call this fixed. Dana knows that it failed and is working out why (see https://chromium-review.googlesource.com/c/chromium/src/+/1153311).
Sorry about that Avi.

Sign in to add a comment