Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2547463003:
Tweak the descriptions of the various types of layout tests. (Closed)
|
Created:
4 years ago by Dirk Pranke Modified:
3 years, 11 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTweak the descriptions of the various types of layout tests.
This CL adjusts the work in r434842 to better describe the
differences between pixel tests and render tree tests and when
you want one or the other. It also gets rid of the "DumpRenderTree"
terminology since we don't really use that any more.
R=pwnall@chromium.org, me@gsnedders.com
BUG=665494
Review-Url: https://codereview.chromium.org/2547463003
Cr-Commit-Position: refs/heads/master@{#443810}
Committed: https://chromium.googlesource.com/chromium/src/+/d2b7d6442f83d3259ef811b9257612b42a76d0a7
Patch Set 1 #
Total comments: 5
Patch Set 2 : rework, remove other DRT references #
Total comments: 4
Patch Set 3 : s/render tree/layout tree #Messages
Total messages: 18 (5 generated)
Thank you very much for making this document better! Your changes LGTM, as they make the document better than it currently is. A comment below points out an opportunity to make it even better, but don't feel blocked on it. https://codereview.chromium.org/2547463003/diff/1/docs/testing/writing_layout... File docs/testing/writing_layout_tests.md (right): https://codereview.chromium.org/2547463003/diff/1/docs/testing/writing_layout... docs/testing/writing_layout_tests.md:53: first two types, because the rendering of a page is influenced by Thanks for catching this inconsistency! https://codereview.chromium.org/2547463003/diff/1/docs/testing/writing_layout... docs/testing/writing_layout_tests.md:68: options. Most render tree tests also produce image results, and so these I think that the text starting at "Most render tree tests" belongs in a promo in the section dedicated to them. The text before it seems sufficient for the purpose of choosing between test types. https://codereview.chromium.org/2547463003/diff/1/docs/testing/writing_layout... docs/testing/writing_layout_tests.md:704: ## Dump Render Tree (DRT) Tests Can you please make the name here consistent with the new name above? There are a bunch of DRTs sprinkled over this section.
Updated. Please take another look? https://codereview.chromium.org/2547463003/diff/1/docs/testing/writing_layout... File docs/testing/writing_layout_tests.md (right): https://codereview.chromium.org/2547463003/diff/1/docs/testing/writing_layout... docs/testing/writing_layout_tests.md:68: options. Most render tree tests also produce image results, and so these On 2016/12/02 02:35:10, pwnall wrote: > I think that the text starting at "Most render tree tests" belongs in a promo in > the section dedicated to them. The text before it seems sufficient for the > purpose of choosing between test types. Okay. https://codereview.chromium.org/2547463003/diff/1/docs/testing/writing_layout... docs/testing/writing_layout_tests.md:704: ## Dump Render Tree (DRT) Tests On 2016/12/02 02:35:10, pwnall wrote: > Can you please make the name here consistent with the new name above? There are > a bunch of DRTs sprinkled over this section. Will do. It didn't even occur to me to check for multiple references :).
Still LGTM. Thank you very much for the update! https://codereview.chromium.org/2547463003/diff/20001/docs/testing/writing_la... File docs/testing/writing_layout_tests.md (right): https://codereview.chromium.org/2547463003/diff/20001/docs/testing/writing_la... docs/testing/writing_layout_tests.md:725: is still meaningful. There are actually many cases where the render tree Could "many cases" link to a crbug that tracks removing or updating these tests?
With the grammatical issue fixed, LGTM. Would be nice to fix the other issue, too, though! https://codereview.chromium.org/2547463003/diff/20001/docs/testing/writing_la... File docs/testing/writing_layout_tests.md (right): https://codereview.chromium.org/2547463003/diff/20001/docs/testing/writing_la... docs/testing/writing_layout_tests.md:717: results. Given the intro above says that by default "a pixel test will also dump the render tree", there should equally be a TODO above as to how to opt out of producing a render tree. https://codereview.chromium.org/2547463003/diff/20001/docs/testing/writing_la... docs/testing/writing_layout_tests.md:732: render tree tests are (grammar): needs a capital R
cbiesinger@chromium.org changed reviewers: + cbiesinger@chromium.org
https://codereview.chromium.org/2547463003/diff/20001/docs/testing/writing_la... File docs/testing/writing_layout_tests.md (right): https://codereview.chromium.org/2547463003/diff/20001/docs/testing/writing_la... docs/testing/writing_layout_tests.md:63: * *Render tree tests*, which output a textual representation of the render We call it Layout Tree now, fwiw
On 2016/12/02 22:15:22, cbiesinger wrote: > https://codereview.chromium.org/2547463003/diff/20001/docs/testing/writing_la... > File docs/testing/writing_layout_tests.md (right): > > https://codereview.chromium.org/2547463003/diff/20001/docs/testing/writing_la... > docs/testing/writing_layout_tests.md:63: * *Render tree tests*, which output a > textual representation of the render > We call it Layout Tree now, fwiw Good to know, I figured I was out of date.
On 2016/12/02 22:23:20, Dirk Pranke wrote: > On 2016/12/02 22:15:22, cbiesinger wrote: > > > https://codereview.chromium.org/2547463003/diff/20001/docs/testing/writing_la... > > File docs/testing/writing_layout_tests.md (right): > > > > > https://codereview.chromium.org/2547463003/diff/20001/docs/testing/writing_la... > > docs/testing/writing_layout_tests.md:63: * *Render tree tests*, which output a > > textual representation of the render > > We call it Layout Tree now, fwiw > > Good to know, I figured I was out of date. If you don't have the bandwidth to switch "render tree" -> "layout tree", I think it's still worth landing the fixes in this CL, and pursuing the rename in a follow-up CL.
On 2016/12/20 00:14:01, pwnall wrote: > If you don't have the bandwidth to switch "render tree" -> "layout tree", I > think it's still worth landing the fixes in this CL, and pursuing the rename in > a follow-up CL. Sorry, yes, this fell off my radar. I'll get it landed w/ the fixes tomorrow.
On 2016/12/21 02:48:23, Dirk Pranke wrote: > On 2016/12/20 00:14:01, pwnall wrote: > > If you don't have the bandwidth to switch "render tree" -> "layout tree", I > > think it's still worth landing the fixes in this CL, and pursuing the rename > in > > a follow-up CL. > > Sorry, yes, this fell off my radar. I'll get it landed w/ the fixes tomorrow. Okay, obviously this didn't happen "tomorrow" :). I will try to get to it this week, though. Sorry!
Finally updated; landing now.
The CQ bit was checked by dpranke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from me@gsnedders.com, pwnall@chromium.org Link to the patchset: https://codereview.chromium.org/2547463003/#ps40001 (title: "s/render tree/layout tree")
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": 40001, "attempt_start_ts": 1484451653321080,
"parent_rev": "b4dbf044c53ead8422a063e7c27ae9999cbecb4d", "commit_rev":
"d2b7d6442f83d3259ef811b9257612b42a76d0a7"}
Message was sent while issue was closed.
Description was changed from ========== Tweak the descriptions of the various types of layout tests. This CL adjusts the work in r434842 to better describe the differences between pixel tests and render tree tests and when you want one or the other. It also gets rid of the "DumpRenderTree" terminology since we don't really use that any more. R=pwnall@chromium.org, me@gsnedders.com BUG=665494 ========== to ========== Tweak the descriptions of the various types of layout tests. This CL adjusts the work in r434842 to better describe the differences between pixel tests and render tree tests and when you want one or the other. It also gets rid of the "DumpRenderTree" terminology since we don't really use that any more. R=pwnall@chromium.org, me@gsnedders.com BUG=665494 Review-Url: https://codereview.chromium.org/2547463003 Cr-Commit-Position: refs/heads/master@{#443810} Committed: https://chromium.googlesource.com/chromium/src/+/d2b7d6442f83d3259ef811b92576... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d2b7d6442f83d3259ef811b92576... |
