|
|
Created:
5 years, 3 months ago by Xianzhu Modified:
5 years, 3 months ago CC:
blink-reviews, dpranke Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionChange output method of paint invalidation failure message
Previously we remove all contents of the test html and insert message.
This makes the actual result invisible.
Put a mesage box at the top-right corner to indicate paint invalidation
objects mismatch. pdr@ contributed the idea and also he svg support
code.
BUG=524134
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=202425
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add pdr's svg code #Patch Set 3 : #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 16 (7 generated)
wangxianzhu@chromium.org changed reviewers: + pdr@chromium.org
LGTM https://codereview.chromium.org/1340423004/diff/1/LayoutTests/paint/invalidat... File LayoutTests/paint/invalidation/spv2/resources/paint-invalidation-test.js (right): https://codereview.chromium.org/1340423004/diff/1/LayoutTests/paint/invalidat... LayoutTests/paint/invalidation/spv2/resources/paint-invalidation-test.js:49: // TODO(wangxianzhu): The following works for HTML only. Should support SVG. Lets support SVG: function finishPaintInvalidationTest() { if (!window.testRunner || !window.internals) return; testRunner.layoutAndPaintAsyncThen(function() { var isSvg = document.rootElement && document.rootElement.nodeName == "svg"; var paintInvalidationObjects = JSON.stringify(internals.trackedPaintInvalidationObjects()); var expectedPaintInvalidationObjects = JSON.stringify(window.expectedPaintInvalidationObjects); internals.stopTrackingPaintInvalidationObjects(); if (paintInvalidationObjects != expectedPaintInvalidationObjects) { if (isSvg) { var svgns = "http://www.w3.org/2000/svg" var rect = document.createElementNS(svgns, 'rect'); rect.setAttribute('width', '30%'); rect.setAttribute('height', '10%'); rect.setAttribute('x', '70%'); rect.setAttribute('fill', 'white'); rect.setAttribute('stroke', 'black'); rect.setAttribute('stroke-width', '1px'); document.documentElement.appendChild(rect); var text = document.createElementNS(svgns, 'text'); text.setAttribute('x', '100%'); text.setAttribute('y', '5%'); text.setAttribute('text-anchor', 'end'); text.textContent = "Paint invalidation objects mismatch."; document.documentElement.appendChild(text); } else { var message = 'Paint invalidation objects mismatch.' + '\nExpected:\n' + expectedPaintInvalidationObjects.replace(/","/g, '",\n "') + '\nActual:\n' + paintInvalidationObjects.replace(/","/g, '",\n "'); testRunner.logToStderr(message); var div = document.createElement('div'); div.style.zIndex = '2147483647'; div.style.visibility = 'visible'; div.style.position = 'fixed'; div.style.top = '0'; div.style.right = '0'; div.style.width = '300px'; div.style.height = '36px'; div.style.color = 'red'; div.style.fontSize = '14px'; div.style.lineHeight = '16px'; div.style.fontFamily = 'Arial'; div.style.border = '1px solid red'; div.style.background = 'white'; div.appendChild(document.createTextNode('Paint invalidation objects mismatch.')); div.appendChild(document.createElement('br')); div.appendChild(document.createTextNode('See stderr for details.')); document.body.appendChild(div); } } testRunner.notifyDone(); }); https://codereview.chromium.org/1340423004/diff/1/LayoutTests/paint/invalidat... LayoutTests/paint/invalidation/spv2/resources/paint-invalidation-test.js:68: div.style.background = 'none'; Lets set the backgroundColor to white so the text is readable above any text.
On 2015/09/16 at 21:27:02, pdr wrote: Note that I removed the force layout in the above code. I think that'll be safe, but we should check (svg doesn't have document.body). Just for your information, a paint invalidation test for svg looks like: <?xml version="1.0" standalone="no"?> <!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd"> <svg width="100%" height="100%" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> <script xlink:href="resources/paint-invalidation-test.js"/> <script> window.expectedPaintInvalidationObjects = [ ]; function paintInvalidationTest() { document.getElementById('rect').style.fill = "green"; } onload = runPaintInvalidationTest; </script> <rect id="rect" width="100px" height="100px" fill="blue"/> </svg>
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/1340423004/#ps20001 (title: "Add pdr's svg code")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1340423004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1340423004/20001
The CQ bit was unchecked by wangxianzhu@chromium.org
The CQ bit was checked by wangxianzhu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org Link to the patchset: https://codereview.chromium.org/1340423004/#ps40001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1340423004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1340423004/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://src.chromium.org/viewvc/blink?view=rev&revision=202425
Message was sent while issue was closed.
With this patch, the experience of checking failure results is better, but still not good enough because it's still time-consuming: - I must carefully examine the each pixel result to see if there is real pixel mismatch, to distinguish paint failure or paint invalidation mismatch; - For each paint invalidation mismatch, I must look at the stderr output, and if it should be rebaselined, I must manually copy the actual output into the test. Neither auto-rebaseline nor --reset-results works for these tests. I thought of two ways to save the time for this: 1. Allow text expectations for ref tests (https://codereview.chromium.org/1346673003/); 2. Convert this tests to pixel tests. I would like to do 2 if we could not agree upon 1. Wdyt?
Message was sent while issue was closed.
On 2015/09/17 17:00:39, Xianzhu wrote: > With this patch, the experience of checking failure results is better, but still > not good enough because it's still time-consuming: > > - I must carefully examine the each pixel result to see if there is real pixel > mismatch, to distinguish paint failure or paint invalidation mismatch; > > - For each paint invalidation mismatch, I must look at the stderr output, and if > it should be rebaselined, I must manually copy the actual output into the test. > Neither auto-rebaseline nor --reset-results works for these tests. > > I thought of two ways to save the time for this: > 1. Allow text expectations for ref tests > (https://codereview.chromium.org/1346673003/); > 2. Convert this tests to pixel tests. > > I would like to do 2 if we could not agree upon 1. > > Wdyt? I mostly defer to pdr@ here. I don't like the idea of having to create more pixel tesets, but I like the idea of having tests that are reftests that *also* have -expected.txt output even less. I'm not really familiar with how the invalidation tests need to work, so I don't really know if there might be better options, sorry.
Message was sent while issue was closed.
ojan@chromium.org changed reviewers: + ojan@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1340423004/diff/40001/LayoutTests/paint/inval... File LayoutTests/paint/invalidation/spv2/resources/paint-invalidation-test.js (left): https://codereview.chromium.org/1340423004/diff/40001/LayoutTests/paint/inval... LayoutTests/paint/invalidation/spv2/resources/paint-invalidation-test.js:43: document.body.offsetTop; Doesn't removing this make your tests flaky because you'll get different tracked objects depending on whether a layout happened to happen or not? Can you just do document.documentElement.offsetTop? Does that work in SVG? |