Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2163953002:
rebaseline huge-image-viewport-scale.html (Closed)
|
Created:
4 years, 5 months ago by Mostyn Bramley-Moore Modified:
4 years ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionrebaseline huge-image-viewport-scale.html
This test was not being run with correctly initialized
preferences, and an invalid baseline. This fixes the
initialization, and rebaselines the test.
This is a prerequisite of https://codereview.chromium.org/2155273002/
BUG=331654, 464295
Committed: https://crrev.com/a99d1d19132186b09c0b5b5fe9efc5ec2a5c447e
Cr-Commit-Position: refs/heads/master@{#406679}
Patch Set 1 : trigger the problem layout test #Patch Set 2 : rebaseline huge-image-viewport-scale.html #Patch Set 3 : mac doesn't use aura, needs an equivalent change #
Total comments: 4
Patch Set 4 : use ignore_result() #
Messages
Total messages: 26 (16 generated)
Description was changed from ========== rebaseline and/or fix some unreliable layout tests This is a prerequisite of https://codereview.chromium.org/2155273002/ BUG=331654 ========== to ========== rebaseline and/or fix some unreliable layout tests This is a prerequisite of https://codereview.chromium.org/2155273002/ BUG=331654, 464295 ==========
The CQ bit was checked by mostynb@opera.com 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...
mostynb@opera.com changed reviewers: + pdr@chromium.org, wangxianzhu@chromium.org
@Xianzhu, pdr: please take a look, I suspect that this test has been invalid since it landed. I am not sure why calling GetWebkitPreferences should alter the layout test results (I assume that this forces an earlier initialization of something), but at least this will flag any future regressions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
The CQ bit was checked by mostynb@opera.com 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 2016/07/20 12:44:18, Mostyn Bramley-Moore wrote: > @Xianzhu, pdr: please take a look, I suspect that this test has been invalid > since it landed. > > I am not sure why calling GetWebkitPreferences should alter the layout test > results (I assume that this forces an earlier initialization of something), but > at least this will flag any future regressions. I guess this works by forcing synchronization of blink settings/flags and chrome preferences. Without this, blink code executed earlier than the first RVH::GetWebkitPreferences() call will use the default blink settings/flags which may be different from those synchronized with chromium preferences. I wonder if crbug.com/627798 is of the similar situation (if not the same). For that bug, the tests sometimes use incorrect scrollbar themes. SGTM. Please add content/browser owners for approval.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mostynb@opera.com changed reviewers: + avi@chromium.org
If I can reproduce crbug.com/627798 on linux, I will take a look to see if it's related. @Avi: can you please give this content/* approval?
lgtm https://codereview.chromium.org/2163953002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2163953002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:500: (void)prefs; ignore_result() from base/macros.h. https://codereview.chromium.org/2163953002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2163953002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:530: (void)prefs; ignore_result() from base/macros.h?
Thanks. https://codereview.chromium.org/2163953002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2163953002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:500: (void)prefs; On 2016/07/20 18:25:36, Avi wrote: > ignore_result() from base/macros.h. Done. https://codereview.chromium.org/2163953002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_mac.mm (right): https://codereview.chromium.org/2163953002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_mac.mm:530: (void)prefs; On 2016/07/20 18:25:36, Avi wrote: > ignore_result() from base/macros.h? Done.
Description was changed from ========== rebaseline and/or fix some unreliable layout tests This is a prerequisite of https://codereview.chromium.org/2155273002/ BUG=331654, 464295 ========== to ========== rebaseline huge-image-viewport-scale.html This test was not being run with correctly initialized preferences, and an invalid baseline. This fixes the initialization, and rebaselines the test. This is a prerequisite of https://codereview.chromium.org/2155273002/ BUG=331654, 464295 ==========
The CQ bit was checked by mostynb@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org Link to the patchset: https://codereview.chromium.org/2163953002/#ps60001 (title: "use ignore_result()")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== rebaseline huge-image-viewport-scale.html This test was not being run with correctly initialized preferences, and an invalid baseline. This fixes the initialization, and rebaselines the test. This is a prerequisite of https://codereview.chromium.org/2155273002/ BUG=331654, 464295 ========== to ========== rebaseline huge-image-viewport-scale.html This test was not being run with correctly initialized preferences, and an invalid baseline. This fixes the initialization, and rebaselines the test. This is a prerequisite of https://codereview.chromium.org/2155273002/ BUG=331654, 464295 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== rebaseline huge-image-viewport-scale.html This test was not being run with correctly initialized preferences, and an invalid baseline. This fixes the initialization, and rebaselines the test. This is a prerequisite of https://codereview.chromium.org/2155273002/ BUG=331654, 464295 ========== to ========== rebaseline huge-image-viewport-scale.html This test was not being run with correctly initialized preferences, and an invalid baseline. This fixes the initialization, and rebaselines the test. This is a prerequisite of https://codereview.chromium.org/2155273002/ BUG=331654, 464295 Committed: https://crrev.com/a99d1d19132186b09c0b5b5fe9efc5ec2a5c447e Cr-Commit-Position: refs/heads/master@{#406679} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/a99d1d19132186b09c0b5b5fe9efc5ec2a5c447e Cr-Commit-Position: refs/heads/master@{#406679}
Message was sent while issue was closed.
On 2016/07/20 21:44:59, commit-bot: I haz the power wrote: > Patchset 4 (id:??) landed as > https://crrev.com/a99d1d19132186b09c0b5b5fe9efc5ec2a5c447e > Cr-Commit-Position: refs/heads/master@{#406679} This CL changes RWHV{Aura,Mac}, with the intention of allowing for a follow-up CL: https://codereview.chromium.org/2155273002/. Given that that CL hasn't landed, can we revert the changes from this CL?
Message was sent while issue was closed.
On 2016/12/15 16:49:08, erikchen wrote:
> This CL changes RWHV{Aura,Mac}, with the intention of allowing for a follow-up
> CL: https://codereview.chromium.org/2155273002/. Given that that CL hasn't
> landed, can we revert the changes from this CL?
Wouldn't that just make the test flaky again? I don't see what good that would
do.
(I should dust off CL2155273002 and try to get it landed...)
-Mostyn.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
