Re: [webkit-dev] About USE(CROSS_PLATFORM_CONTEXT_MENUS)
I was having a look at our Context Menu design when this USE flag got my attention. Can someone help me clarify the motivation for it? The motivation is explained pretty well in the ChangeLog for r73802, which introduced this flag. I'll try to give a little explanation here too. CROSS_PLATFORM_CONTEXT_MENUS changes WebCore::ContextMenu and WebCore::ContextMenuItem from being thin wrappers around a platform-specific context menu and context menu item, to being full cross-platform representations of a menu and menu item. In this new model, most code will only ever deal with the cross-platform types; platform-specific menus and menu items should only be created as needed (e.g., when actually showing the menu on screen, or when passing a native menu up to the embedding application, as happens in the Apple WebKit[2] APIs). Providing a true cross-platform representation of menus and menu items makes many things simpler, including being able to serialize menus and send them across process boundaries. This last point was the immediate motivation for CROSS_PLATFORM_CONTEXT_MENUS: we needed to send context menus from the web process to the UI process to support context menus in WebKit2 on Windows. But the hope was that all ports would switch over to this model eventually. It seems that only PLATFORM(WIN) is using it, but I'm not sure if for both WK1 and WK2... WIN uses it for both WK1 and WK2. Also, is there any other port using it? I don't believe so. But if you look at ContextMenu[Item].h, it looks like CHROMIUM and EFL could very easily switch over to it; they've already implemented something very similar by abusing the PlatformMenuItemDescription typedef. -Adam ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
[webkit-dev] Best practices for landing new/changed layout test expectations?
I've noticed as of late several different approaches being used when adding/changing LayoutTests which need rebaselining on other platforms. Obviously we cannot expect developers to test/rebaseline on all platforms before landing given our current infrastructure. But what should we expect them to do? Currently some folks add failing expectations to other ports TestExpectations. Some add [skip]. Some even add them to the (new) global TestExpectations file. What's the proper course of action? I checked: http://trac.webkit.org/wiki/Keeping%20the%20Tree%20Green http://trac.webkit.org/wiki/Creating%20and%20Submitting%20Layout%20Tests%20and%20Patches http://trac.webkit.org/wiki/TestExpectations and didn't see the I expect this to fail/need rebaseline on other ports case discussed. I remember some discussion of a [rebaseline] keyword in TestExpectations, but I'm not sure that ever made it in? ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Best practices for landing new/changed layout test expectations?
On Mon, Feb 25, 2013 at 12:38 PM, Eric Seidel esei...@google.com wrote: I've noticed as of late several different approaches being used when adding/changing LayoutTests which need rebaselining on other platforms. Obviously we cannot expect developers to test/rebaseline on all platforms before landing given our current infrastructure. But what should we expect them to do? Currently some folks add failing expectations to other ports TestExpectations. Some add [skip]. Some even add them to the (new) global TestExpectations file. What's the proper course of action? I checked: http://trac.webkit.org/wiki/Keeping%20the%20Tree%20Green http://trac.webkit.org/wiki/Creating%20and%20Submitting%20Layout%20Tests%20and%20Patches http://trac.webkit.org/wiki/TestExpectations and didn't see the I expect this to fail/need rebaseline on other ports case discussed. Memory says that the last time this was raised [1], the consensus was to land the change, turn the tree red, notify as many gardeners / port maintainers as possible, and rebaseline as quickly as possible. I.e., don't add entries to TestExpectations. Of course, such a policy wouldn't play nicely w/ the EWS bots, and I think this discussion predated everyone switching over to TestExpectations (and certainly predated the generic expectations file). I don't find the above entirely satisfactory, but I also don't have any great alternatives to endorse given the existing tooling. Also, if the test fails generically (all ports), it probably shouldn't be landed. I'm not sure why you couldn't at least update the port you're developing on; if you don't, how do you know your fix is working at all? (Of course, there could be a case where a high-priority fix might break some low-priority tests). I remember some discussion of a [rebaseline] keyword in TestExpectations, but I'm not sure that ever made it in? The [ NeedsRebaseline ] enhancement is, as of yet, unimplemented and unclaimed [2]. It shouldn't be too hard for someone to try it if they're looking for a reason to explore the NRWT code :) -- Dirk [1] https://lists.webkit.org/pipermail/webkit-dev/2012-April/020250.html [2] https://bugs.webkit.org/show_bug.cgi?id=100415 ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Best practices for landing new/changed layout test expectations?
On Mon, Feb 25, 2013 at 12:52 PM, Dirk Pranke dpra...@google.com wrote: On Mon, Feb 25, 2013 at 12:38 PM, Eric Seidel esei...@google.com wrote: I've noticed as of late several different approaches being used when adding/changing LayoutTests which need rebaselining on other platforms. Obviously we cannot expect developers to test/rebaseline on all platforms before landing given our current infrastructure. But what should we expect them to do? Currently some folks add failing expectations to other ports TestExpectations. Some add [skip]. Some even add them to the (new) global TestExpectations file. What's the proper course of action? I checked: http://trac.webkit.org/wiki/Keeping%20the%20Tree%20Green http://trac.webkit.org/wiki/Creating%20and%20Submitting%20Layout%20Tests%20and%20Patches http://trac.webkit.org/wiki/TestExpectations and didn't see the I expect this to fail/need rebaseline on other ports case discussed. Memory says that the last time this was raised [1], the consensus was to land the change, turn the tree red, notify as many gardeners / port maintainers as possible, and rebaseline as quickly as possible. I.e., don't add entries to TestExpectations. Of course, such a policy wouldn't play nicely w/ the EWS bots, and I think this discussion predated everyone switching over to TestExpectations (and certainly predated the generic expectations file). I don't find the above entirely satisfactory, but I also don't have any great alternatives to endorse given the existing tooling. Also, if the test fails generically (all ports), it probably shouldn't be landed. I'm not sure why you couldn't at least update the port you're developing on; if you don't, how do you know your fix is working at all? (Of course, there could be a case where a high-priority fix might break some low-priority tests). Adding a new feature behind a build flag disabled everywhere by default. I remember some discussion of a [rebaseline] keyword in TestExpectations, but I'm not sure that ever made it in? The [ NeedsRebaseline ] enhancement is, as of yet, unimplemented and unclaimed [2]. It shouldn't be too hard for someone to try it if they're looking for a reason to explore the NRWT code :) I object to adding such a thing. People add and forget about these entries way too often: # Needs rebaseline fast/sub-pixel/inline-block-with-padding.html [ Failure ] # Rebaseline required after https://bugs.webkit.org/show_bug.cgi?id=101177 webkit.org/b/101177svg/dynamic-updates/SVGUseElement-dom-requiredFeatures.html [ ImageOnlyFailure Pass ] webkit.org/b/101177 svg/repaint/inner-svg-change-viewBox.svg [ ImageOnlyFailure Pass ] # These tests need to be rebaselined after https://bugs.webkit.org/show_bug.cgi?id=99984 webkit.org/b/99984 svg/as-image/image-preserveAspectRatio-all.svg [ ImageOnlyFailure Pass ] webkit.org/b/99984 svg/filters/feImage-preserveAspectRatio-all.svg [ ImageOnlyFailure Pass ] webkit.org/b/99984svg/dynamic-updates/SVGFEImageElement-dom-preserveAspectRatio-attr.html [ ImageOnlyFailure Pass ] webkit.org/b/99984svg/dynamic-updates/SVGFEImageElement-svgdom-preserveAspectRatio-prop.html [ ImageOnlyFailure Pass ] # Needs rebaseline after bug https://bugs.webkit.org/show_bug.cgi?id=104462 webkit.org/b/98687fast/css-generated-content/table-row-group-to-inline.html [ Failure ] webkit.org/b/104595 fast/css/empty-generated-content.html [ Failure ] # Needs a rebaseline after WK108429 lands. webkit.org/b/108429 svg/custom/text-ctm.svg [ Pass Failure ] # Transient. Needs rebaseline. webkit.org/b/103955 fast/repaint/caret-with-transformation.html [ Missing ] # Needs rebaseline. webkit.org/b/105574 editing/input/caret-at-the-edge-of-contenteditable.html [ Failure ] webkit.org/b/105574editing/input/reveal-caret-of-multiline-contenteditable.html [ Failure ] webkit.org/b/105574 editing/input/reveal-caret-of-multiline-input.html [ Failure ] webkit.org/b/105574fast/spatial-navigation/snav-div-overflow-scrol-hidden.html [ Failure ] # Rebaseline required after https://webkit.org/b/31397 webkit.org/b/107567 svg/batik/text/xmlSpace.svg [ Failure ] webkit.org/b/107567 fast/text/capitalize-empty-generated-string.html [ Failure ] webkit.org/b/107567 fast/text/whitespace/007.html [ Failure ] webkit.org/b/107567 fast/text/whitespace/006.html [ Failure ] webkit.org/b/107567 fast/inline/drawStyledEmptyInlinesWithWS.html [ Failure ] webkit.org/b/107567 tables/mozilla/bugs/bug1318.html [ Failure ] webkit.org/b/107567 fast/inline/drawStyledEmptyInlines.html [ Failure ] webkit.org/b/107567 tables/mozilla/bugs/bug113235-3.html [ Failure ] webkit.org/b/107567 css2.1/t0505-c16-descendant-01-e.html [ Failure ] webkit.org/b/107567 tables/mozilla/bugs/bug1188.html [ Failure ] webkit.org/b/107567 editing/selection/extend-by-sentence-001.html [ Failure ] webkit.org/b/107567 svg/carto.net/combobox.svg [ Failure ] # Needs rebaseline after https://bugs.webkit.org/show_bug.cgi?id=70634
Re: [webkit-dev] Best practices for landing new/changed layout test expectations?
On Mon, Feb 25, 2013 at 1:22 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Feb 25, 2013 at 12:52 PM, Dirk Pranke dpra...@google.com wrote: On Mon, Feb 25, 2013 at 12:38 PM, Eric Seidel esei...@google.com wrote: I've noticed as of late several different approaches being used when adding/changing LayoutTests which need rebaselining on other platforms. Obviously we cannot expect developers to test/rebaseline on all platforms before landing given our current infrastructure. But what should we expect them to do? Currently some folks add failing expectations to other ports TestExpectations. Some add [skip]. Some even add them to the (new) global TestExpectations file. What's the proper course of action? I checked: http://trac.webkit.org/wiki/Keeping%20the%20Tree%20Green http://trac.webkit.org/wiki/Creating%20and%20Submitting%20Layout%20Tests%20and%20Patches http://trac.webkit.org/wiki/TestExpectations and didn't see the I expect this to fail/need rebaseline on other ports case discussed. Memory says that the last time this was raised [1], the consensus was to land the change, turn the tree red, notify as many gardeners / port maintainers as possible, and rebaseline as quickly as possible. I.e., don't add entries to TestExpectations. Of course, such a policy wouldn't play nicely w/ the EWS bots, and I think this discussion predated everyone switching over to TestExpectations (and certainly predated the generic expectations file). I don't find the above entirely satisfactory, but I also don't have any great alternatives to endorse given the existing tooling. Also, if the test fails generically (all ports), it probably shouldn't be landed. I'm not sure why you couldn't at least update the port you're developing on; if you don't, how do you know your fix is working at all? (Of course, there could be a case where a high-priority fix might break some low-priority tests). Adding a new feature behind a build flag disabled everywhere by default. I remember some discussion of a [rebaseline] keyword in TestExpectations, but I'm not sure that ever made it in? The [ NeedsRebaseline ] enhancement is, as of yet, unimplemented and unclaimed [2]. It shouldn't be too hard for someone to try it if they're looking for a reason to explore the NRWT code :) I object to adding such a thing. People add and forget about these entries way too often: The rationale for adding the keyword was precisely this ... it would make it easier to programmatically identify tests needing updated baselines and alert people :). Do you have an alternative you'd suggest for keeping people from forgetting (particularly in the case where we have an expected failure waiting on a bug fix)? -- Dirk ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Best practices for landing new/changed layout test expectations?
On Mon, Feb 25, 2013 at 1:29 PM, Dirk Pranke dpra...@google.com wrote: On Mon, Feb 25, 2013 at 1:22 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Feb 25, 2013 at 12:52 PM, Dirk Pranke dpra...@google.com wrote: I remember some discussion of a [rebaseline] keyword in TestExpectations, but I'm not sure that ever made it in? The [ NeedsRebaseline ] enhancement is, as of yet, unimplemented and unclaimed [2]. It shouldn't be too hard for someone to try it if they're looking for a reason to explore the NRWT code :) I object to adding such a thing. People add and forget about these entries way too often: The rationale for adding the keyword was precisely this ... it would make it easier to programmatically identify tests needing updated baselines and alert people :). Do you have an alternative you'd suggest for keeping people from forgetting (particularly in the case where we have an expected failure waiting on a bug fix)? Just turn bots red. People maintaining bots will notice and rebaseline them as needed. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Best practices for landing new/changed layout test expectations?
On Mon, Feb 25, 2013 at 1:33 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Feb 25, 2013 at 1:29 PM, Dirk Pranke dpra...@google.com wrote: On Mon, Feb 25, 2013 at 1:22 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Feb 25, 2013 at 12:52 PM, Dirk Pranke dpra...@google.com wrote: I remember some discussion of a [rebaseline] keyword in TestExpectations, but I'm not sure that ever made it in? The [ NeedsRebaseline ] enhancement is, as of yet, unimplemented and unclaimed [2]. It shouldn't be too hard for someone to try it if they're looking for a reason to explore the NRWT code :) I object to adding such a thing. People add and forget about these entries way too often: The rationale for adding the keyword was precisely this ... it would make it easier to programmatically identify tests needing updated baselines and alert people :). Do you have an alternative you'd suggest for keeping people from forgetting (particularly in the case where we have an expected failure waiting on a bug fix)? Just turn bots red. People maintaining bots will notice and rebaseline them as needed. If there's a significant time lapse between failing test and bug fix, it seems like a bad idea to leave the tree red. Are you suggesting we should land a failling baseline in the meantime? -- Dirk ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Best practices for landing new/changed layout test expectations?
On Mon, Feb 25, 2013 at 1:39 PM, Dirk Pranke dpra...@google.com wrote: On Mon, Feb 25, 2013 at 1:33 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Feb 25, 2013 at 1:29 PM, Dirk Pranke dpra...@google.com wrote: On Mon, Feb 25, 2013 at 1:22 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Feb 25, 2013 at 12:52 PM, Dirk Pranke dpra...@google.com wrote: I remember some discussion of a [rebaseline] keyword in TestExpectations, but I'm not sure that ever made it in? The [ NeedsRebaseline ] enhancement is, as of yet, unimplemented and unclaimed [2]. It shouldn't be too hard for someone to try it if they're looking for a reason to explore the NRWT code :) I object to adding such a thing. People add and forget about these entries way too often: The rationale for adding the keyword was precisely this ... it would make it easier to programmatically identify tests needing updated baselines and alert people :). Do you have an alternative you'd suggest for keeping people from forgetting (particularly in the case where we have an expected failure waiting on a bug fix)? Just turn bots red. People maintaining bots will notice and rebaseline them as needed. If there's a significant time lapse between failing test and bug fix, it seems like a bad idea to leave the tree red. I don't think that's necessarily bad. If the port maintainer is not available on IRC/Bugzilla and can't assist you in rebaselining tests, I don't think you should be accountable for keeping bots green for them in practice. On the other hand, if all it takes to keep bots green is to commit new baselines, then why don't patch authors just do that? Other than people who are not yet committers, it doesn't make much sense for patch authors to be adding test expectations when they could simply commit new baselines after the fact unless you're landing a patch that requires a rebaseline of mass scale, in which case, adding test expectations temporarily might make sense. All too often, people add test expectations and forget about them because bots are green. It's the issue with incentives. People know they shouldn't leave bots red so they have a much higher inceptive to fix them on time. If they have added test expectations, then bots are green and other people won't be nagging them to fix it so they have much less inceptive to spend their time fixing bots. Are you suggesting we should land a failling baseline in the meantime? No. I'm suggesting patch authors perform their due diligence and either ask port maintainers to rebaseline or rebaseline tests themselves. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Best practices for landing new/changed layout test expectations?
On Mon, Feb 25, 2013 at 2:21 PM, Silvia Pfeiffer silvi...@chromium.orgwrote: On Tue, Feb 26, 2013 at 8:22 AM, Ryosuke Niwa rn...@webkit.org wrote: I object to adding such a thing. People add and forget about these entries way too often: # Needs rebaseline fast/sub-pixel/inline-block-with-padding.html [ Failure ] # Rebaseline required after https://bugs.webkit.org/show_bug.cgi?id=101177 webkit.org/b/101177svg/dynamic-updates/SVGUseElement-dom-requiredFeatures.html [ ImageOnlyFailure Pass ] webkit.org/b/101177 svg/repaint/inner-svg-change-viewBox.svg [ ImageOnlyFailure Pass ] # These tests need to be rebaselined after https://bugs.webkit.org/show_bug.cgi?id=99984 webkit.org/b/99984 svg/as-image/image-preserveAspectRatio-all.svg [ ImageOnlyFailure Pass ] webkit.org/b/99984 svg/filters/feImage-preserveAspectRatio-all.svg [ ImageOnlyFailure Pass ] webkit.org/b/99984svg/dynamic-updates/SVGFEImageElement-dom-preserveAspectRatio-attr.html [ ImageOnlyFailure Pass ] webkit.org/b/99984svg/dynamic-updates/SVGFEImageElement-svgdom-preserveAspectRatio-prop.html [ ImageOnlyFailure Pass ] # Needs rebaseline after bug https://bugs.webkit.org/show_bug.cgi?id=104462 webkit.org/b/98687fast/css-generated-content/table-row-group-to-inline.html [ Failure ] webkit.org/b/104595 fast/css/empty-generated-content.html [ Failure ] # Needs a rebaseline after WK108429 lands. webkit.org/b/108429 svg/custom/text-ctm.svg [ Pass Failure ] # Transient. Needs rebaseline. webkit.org/b/103955 fast/repaint/caret-with-transformation.html [ Missing ] # Needs rebaseline. webkit.org/b/105574editing/input/caret-at-the-edge-of-contenteditable.html [ Failure ] webkit.org/b/105574editing/input/reveal-caret-of-multiline-contenteditable.html [ Failure ] webkit.org/b/105574 editing/input/reveal-caret-of-multiline-input.html [ Failure ] webkit.org/b/105574fast/spatial-navigation/snav-div-overflow-scrol-hidden.html [ Failure ] # Rebaseline required after https://webkit.org/b/31397 webkit.org/b/107567 svg/batik/text/xmlSpace.svg [ Failure ] webkit.org/b/107567 fast/text/capitalize-empty-generated-string.html [ Failure ] webkit.org/b/107567 fast/text/whitespace/007.html [ Failure ] webkit.org/b/107567 fast/text/whitespace/006.html [ Failure ] webkit.org/b/107567 fast/inline/drawStyledEmptyInlinesWithWS.html [ Failure ] webkit.org/b/107567 tables/mozilla/bugs/bug1318.html [ Failure ] webkit.org/b/107567 fast/inline/drawStyledEmptyInlines.html [ Failure ] webkit.org/b/107567 tables/mozilla/bugs/bug113235-3.html [ Failure ] webkit.org/b/107567 css2.1/t0505-c16-descendant-01-e.html [ Failure ] webkit.org/b/107567 tables/mozilla/bugs/bug1188.html [ Failure ] webkit.org/b/107567 editing/selection/extend-by-sentence-001.html [ Failure ] webkit.org/b/107567 svg/carto.net/combobox.svg [ Failure ] # Needs rebaseline after https://bugs.webkit.org/show_bug.cgi?id=70634 webkit.org/b/109507compositing/contents-opaque/contents-opaque-background-color.html [ Failure ] webkit.org/b/109507compositing/contents-opaque/contents-opaque-layer-opacity.html [ Failure ] webkit.org/b/109507compositing/contents-opaque/contents-opaque-layer-transform.html [ Failure ] webkit.org/b/109507 compositing/rtl/rtl-fixed.html [ Failure ] webkit.org/b/109507 compositing/rtl/rtl-fixed-overflow.html [ Failure ] # Rebaseline required after https://webkit.org/b/95772 webkit.org/b/109209fast/text/international/bidi-ignored-for-first-child-inline.html [ Failure ] I think it's great to be able to create such a list! The bug at https://bugs.webkit.org/show_bug.cgi?id=100415 points to the idea of extending the garden-o-matic tool to create rebaselines based on the keyword. I like this idea - it would help everyone, including non-committers, deal with updating testexpectations and keep the tree green longer. I don't believe in making bots go read - that's a productivity stopper. I don't like the idea that now I have to either use this tool or manually read through TestExpectations to find tests that need rebaseline. It's also annoying if we had all contributors add new lines to TestExpectations because those changes will almost always conflict when applying patches. In general, I don't like any solution that requires modifying TestExpectations files a lot. It's unneeded svn churn. It should be fairly straight forward to create a tool that analyzes files changed in each commit and deduce which tests' expected results have been changed. The tool can then fetch results from each port' bot for those tests and automatically land them. It can then comment on the bug automatically about these rebaseline commits. There is no need to add remove entries from TestExpectation files. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Best practices for landing new/changed layout test expectations?
On Mon, Feb 25, 2013 at 2:36 PM, Rouslan Solomakhin rous...@chromium.orgwrote: On Mon, Feb 25, 2013 at 2:05 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Feb 25, 2013 at 1:39 PM, Dirk Pranke dpra...@google.com wrote: Are you suggesting we should land a failling baseline in the meantime? No. I'm suggesting patch authors perform their due diligence and either ask port maintainers to rebaseline or rebaseline tests themselves. How should one go about finding the port maintainers? There're 200 people on http://trac.webkit.org/wiki/WebKit%20Team and 400 people on #webkit IRC. :-) That page clearly indicates who works on which port. We can also create a mechanism by which we find port maintainers who are currently online. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Best practices for landing new/changed layout test expectations?
On Mon, Feb 25, 2013 at 2:48 PM, Dirk Pranke dpra...@google.com wrote: On Mon, Feb 25, 2013 at 2:05 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Feb 25, 2013 at 1:39 PM, Dirk Pranke dpra...@google.com wrote: Are you suggesting we should land a failling baseline in the meantime? No. I'm suggesting patch authors perform their due diligence and either ask port maintainers to rebaseline or rebaseline tests themselves. I think either you misunderstood my question, or I am misunderstanding your answer. I'm not asking who, I'm asking what ... If we know some tests are failing, and when we fix a bug the tests will start passing again (but that patch might not land for quite some time), what should we (anyone) do in the meantime? Leave the tree red, land incorrect -expected baselines so that we can catch changes in behavior, or add lines to TestExpectations? Many of the lines you cited fell into the last category. Either one of those two solutions would work (although I strictly advice we do the latter) when there are failing tests and we need a fix in order for those tests to pass but I'm not interested in discussing that matter at the moment. I'm specifically opposed to adding new entries to TestExpectations for the sole purpose of rebaselining them later. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Best practices for landing new/changed layout test expectations?
On Mon, Feb 25, 2013 at 3:53 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Feb 25, 2013 at 2:48 PM, Dirk Pranke dpra...@google.com wrote: On Mon, Feb 25, 2013 at 2:05 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Feb 25, 2013 at 1:39 PM, Dirk Pranke dpra...@google.com wrote: Are you suggesting we should land a failling baseline in the meantime? No. I'm suggesting patch authors perform their due diligence and either ask port maintainers to rebaseline or rebaseline tests themselves. I think either you misunderstood my question, or I am misunderstanding your answer. I'm not asking who, I'm asking what ... If we know some tests are failing, and when we fix a bug the tests will start passing again (but that patch might not land for quite some time), what should we (anyone) do in the meantime? Leave the tree red, land incorrect -expected baselines so that we can catch changes in behavior, or add lines to TestExpectations? Many of the lines you cited fell into the last category. Either one of those two solutions would work (although I strictly advice we do the latter) when there are failing tests and we need a fix in order for those tests to pass but I'm not interested in discussing that matter at the moment. I'm specifically opposed to adding new entries to TestExpectations for the sole purpose of rebaselining them later. One of the modes that the new generic TE file permits is to specify a generic Skip and overriding this with per-port Pass. This provides (IMO) a more manageable way to land a patch that you expect will require per-port mods to fully get the patch working on the primary ports. It also allows one to land a patch containing tests where a subsequent patch will provide the functionality to be tested. I recently used both of these modes to sub-divide a large patch and to incrementally verify (landing individual per-port fixes as needed) individual ports. Personally, I prefer this over the just turn bots red mode. I suspect the turn bots red approach results in greater churn, due to rollouts and re-landing, than the finer grain approach I outlined above. I agree it requires the committer to follow through on other ports and eventually remove the generic Skip and Pass rules (once it works everywhere), but we have to assume here (IMO) that committers will do the right thing and take responsibility for the process. [And if not, then remind them.] So I for one, would vote against the turn the bots red approach. Overall, I think that approach produces a higher interrupt load on our limited committer and reviewer resources. I think it would be useful to give other procedures an opportunity to be tried out so we can have more data for choosing between alternative approaches. Regards, Glenn ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Best practices for landing new/changed layout test expectations?
On Mon, Feb 25, 2013 at 3:36 PM, Glenn Adams gl...@skynav.com wrote: On Mon, Feb 25, 2013 at 3:53 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Feb 25, 2013 at 2:48 PM, Dirk Pranke dpra...@google.com wrote: On Mon, Feb 25, 2013 at 2:05 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Feb 25, 2013 at 1:39 PM, Dirk Pranke dpra...@google.com wrote: Are you suggesting we should land a failling baseline in the meantime? No. I'm suggesting patch authors perform their due diligence and either ask port maintainers to rebaseline or rebaseline tests themselves. I think either you misunderstood my question, or I am misunderstanding your answer. I'm not asking who, I'm asking what ... If we know some tests are failing, and when we fix a bug the tests will start passing again (but that patch might not land for quite some time), what should we (anyone) do in the meantime? Leave the tree red, land incorrect -expected baselines so that we can catch changes in behavior, or add lines to TestExpectations? Many of the lines you cited fell into the last category. Either one of those two solutions would work (although I strictly advice we do the latter) when there are failing tests and we need a fix in order for those tests to pass but I'm not interested in discussing that matter at the moment. I'm specifically opposed to adding new entries to TestExpectations for the sole purpose of rebaselining them later. One of the modes that the new generic TE file permits is to specify a generic Skip and overriding this with per-port Pass. This provides (IMO) a more manageable way to land a patch that you expect will require per-port mods to fully get the patch working on the primary ports. It also allows one to land a patch containing tests where a subsequent patch will provide the functionality to be tested. I recently used both of these modes to sub-divide a large patch and to incrementally verify (landing individual per-port fixes as needed) individual ports. Personally, I prefer this over the just turn bots red mode. I suspect the turn bots red approach results in greater churn, due to rollouts and re-landing, than the finer grain approach I outlined above. I agree it requires the committer to follow through on other ports and eventually remove the generic Skip and Pass rules (once it works everywhere), but we have to assume here (IMO) that committers will do the right thing and take responsibility for the process. [And if not, then remind them.] So I for one, would vote against the turn the bots red approach. Overall, I think that approach produces a higher interrupt load on our limited committer and reviewer resources. I think it would be useful to give other procedures an opportunity to be tried out so we can have more data for choosing between alternative approaches. This has been tried as many people including yourself are using this approach in landing and rebaselining patches. In particular, a lot of new contributors from Chromium port use this strategy. When I used to work for Google, I periodically went through all entries in TestExpectations files that needed rebaselines and manually rebaselined them myself because people just leave entries like I've cited in another email all the time. Quite frankly, I don't want it to be my (or anyone but patch author's) job to take care of all these stale entries people add. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Best practices for landing new/changed layout test expectations?
On Mon, Feb 25, 2013 at 5:27 PM, Ryosuke Niwa rn...@webkit.org wrote: Quite frankly, I don't want it to be my (or anyone but patch author's) job to take care of all these stale entries people add. I agree, but I think that sloppy follow-up doesn't mean the approach is necessarily bad. We have to police our own work and others all the time. I don't see that going away. In any case, it wouldn't be hard to add a command to webkitpy that reports extraneous/questionable entries in TE files. [Or perhaps there is already a tool that I haven't ran across.] ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Best practices for landing new/changed layout test expectations?
On Feb 25, 2013, at 4:27 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Feb 25, 2013 at 3:36 PM, Glenn Adams gl...@skynav.com wrote: On Mon, Feb 25, 2013 at 3:53 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Feb 25, 2013 at 2:48 PM, Dirk Pranke dpra...@google.com wrote: On Mon, Feb 25, 2013 at 2:05 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Feb 25, 2013 at 1:39 PM, Dirk Pranke dpra...@google.com wrote: Are you suggesting we should land a failling baseline in the meantime? No. I'm suggesting patch authors perform their due diligence and either ask port maintainers to rebaseline or rebaseline tests themselves. I think either you misunderstood my question, or I am misunderstanding your answer. I'm not asking who, I'm asking what ... If we know some tests are failing, and when we fix a bug the tests will start passing again (but that patch might not land for quite some time), what should we (anyone) do in the meantime? Leave the tree red, land incorrect -expected baselines so that we can catch changes in behavior, or add lines to TestExpectations? Many of the lines you cited fell into the last category. Either one of those two solutions would work (although I strictly advice we do the latter) when there are failing tests and we need a fix in order for those tests to pass but I'm not interested in discussing that matter at the moment. I'm specifically opposed to adding new entries to TestExpectations for the sole purpose of rebaselining them later. One of the modes that the new generic TE file permits is to specify a generic Skip and overriding this with per-port Pass. This provides (IMO) a more manageable way to land a patch that you expect will require per-port mods to fully get the patch working on the primary ports. It also allows one to land a patch containing tests where a subsequent patch will provide the functionality to be tested. I recently used both of these modes to sub-divide a large patch and to incrementally verify (landing individual per-port fixes as needed) individual ports. Personally, I prefer this over the just turn bots red mode. I suspect the turn bots red approach results in greater churn, due to rollouts and re-landing, than the finer grain approach I outlined above. I agree it requires the committer to follow through on other ports and eventually remove the generic Skip and Pass rules (once it works everywhere), but we have to assume here (IMO) that committers will do the right thing and take responsibility for the process. [And if not, then remind them.] So I for one, would vote against the turn the bots red approach. Overall, I think that approach produces a higher interrupt load on our limited committer and reviewer resources. I think it would be useful to give other procedures an opportunity to be tried out so we can have more data for choosing between alternative approaches. This has been tried as many people including yourself are using this approach in landing and rebaselining patches. In particular, a lot of new contributors from Chromium port use this strategy. When I used to work for Google, I periodically went through all entries in TestExpectations files that needed rebaselines and manually rebaselined them myself because people just leave entries like I've cited in another email all the time. Quite frankly, I don't want it to be my (or anyone but patch author's) job to take care of all these stale entries people add. The problem of Chromium in the early days was that all tests were rebaselined when they failed. Too many tests turned the bots red at this time and the Chromium folks didn't (couldn't?) take attention of all these failing tests. We lost the chance to fix a lot of these bugs immediately during this time. It took months to cover these tests in bug reports and fix them finally. Some tests may still report that everything is ok, even if they are failing - untracked. I see the current needs rebaseline approach better, since the tests are at least logged. I do not have an opinion how they should be logged. Greetings, Dirk - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Best practices for landing new/changed layout test expectations?
On Mon, Feb 25, 2013 at 6:17 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Feb 25, 2013 at 5:06 PM, Glenn Adams gl...@skynav.com wrote: On Mon, Feb 25, 2013 at 5:27 PM, Ryosuke Niwa rn...@webkit.org wrote: Quite frankly, I don't want it to be my (or anyone but patch author's) job to take care of all these stale entries people add. I agree, but I think that sloppy follow-up doesn't mean the approach is necessarily bad. We have to police our own work and others all the time. If we need to constantly remind people to do X, then X needs to be removed from the list of things we need to remember to do. Or, we need to automate it in the process, like having style bot serve as a gatekeeper for people that forget to run check-webkit-style on their own. I'm sure that most of us have forgotten to run it ourselves, but that doesn't mean we should remove the tool or the style bot. ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Best practices for landing new/changed layout test expectations?
On Mon, Feb 25, 2013 at 5:38 PM, Glenn Adams gl...@skynav.com wrote: On Mon, Feb 25, 2013 at 6:17 PM, Ryosuke Niwa rn...@webkit.org wrote: On Mon, Feb 25, 2013 at 5:06 PM, Glenn Adams gl...@skynav.com wrote: On Mon, Feb 25, 2013 at 5:27 PM, Ryosuke Niwa rn...@webkit.org wrote: Quite frankly, I don't want it to be my (or anyone but patch author's) job to take care of all these stale entries people add. I agree, but I think that sloppy follow-up doesn't mean the approach is necessarily bad. We have to police our own work and others all the time. If we need to constantly remind people to do X, then X needs to be removed from the list of things we need to remember to do. Or, we need to automate it in the process, like having style bot serve as a gatekeeper for people that forget to run check-webkit-style on their own. I'm sure that most of us have forgotten to run it ourselves, but that doesn't mean we should remove the tool or the style bot. Right. Although I do run check-webkit-style by virtue of my running webkit-patch upload to upload patches. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
Re: [webkit-dev] Best practices for landing new/changed layout test expectations?
On Mon, Feb 25, 2013 at 6:28 PM, Silvia Pfeiffer silvi...@chromium.orgwrote: On Tue, Feb 26, 2013 at 9:34 AM, Ryosuke Niwa rn...@webkit.org wrote: It should be fairly straight forward to create a tool that analyzes files changed in each commit and deduce which tests' expected results have been changed. The tool can then fetch results from each port' bot for those tests and automatically land them. It can then comment on the bug automatically about these rebaseline commits. There is no need to add remove entries from TestExpectation files. The consequence of this is that it's possible that rebaselines get committed that may actually indicate a new bug. Since your suggestion is to attach this to the bug through which the patch landed, I think that's ok and the original author (or the reviewer) should feel obliged to follow up on the newly introduced bug by creating a new bug. If we can do this in a way that avoids the bots from going red, I think it's a good solution. I don't like a dogmatic approach like keeping bots green at all cost. We need to weigh costs of bots turning red versus asking every contributor to do extra things in order to keep bots green while noticing that some of our 400 contributors are bound to make mistakes every now and then. After all, those people who get affected by red bots are also people who submit patches and (hopefully) maintain bots. I'm also saddened by the fact that the first thing some people do when they see builds failing or some tests failing is to blame others or complain instead of trying to fix those problems themselves. - R. Niwa ___ webkit-dev mailing list webkit-dev@lists.webkit.org https://lists.webkit.org/mailman/listinfo/webkit-dev
