Bug 146056

Agree with your proposal.

(In reply to comment #0) > 1) Merge the optional jhbuild-wayland.modules into the default I don't think we should merge all optional modules at this point, but just the ones we need for Wayland. If we want to talk about merging all optional modules, we should do it in another bug, because I think it's unrelated to this issue.

I think we should go straight to the latest stable release, 3.16.4, since it's closer to what our users will actually have. There's less value in testing with 3.12.x since it's already quite old. (In reply to comment #2) > I don't think we should merge all optional modules at this point, but just > the ones we need for Wayland. If we want to talk about merging all optional > modules, we should do it in another bug, because I think it's unrelated to > this issue. Agreed; see also bug #145696

(In reply to comment #3) > I think we should go straight to the latest stable release, 3.16.4, since > it's closer to what our users will actually have. There's less value in > testing with 3.12.x since it's already quite old. I also agree with Michael here.

(In reply to comment #4) > (In reply to comment #3) > > I think we should go straight to the latest stable release, 3.16.4, since > > it's closer to what our users will actually have. There's less value in > > testing with 3.12.x since it's already quite old. > > I also agree with Michael here. Everytime I have proposed that, for exactly that same reason, the response has been that the rule is to not bump anything unless required by tests. I guess it applies here. Anyway, I never liked that rule, so I, of course, agree on *always* testing with the version as close as possible to what most users have.

(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > I think we should go straight to the latest stable release, 3.16.4, since > > > it's closer to what our users will actually have. There's less value in > > > testing with 3.12.x since it's already quite old. > > > > I also agree with Michael here. > > Everytime I have proposed that, for exactly that same reason, the response > has been that the rule is to not bump anything unless required by tests. I > guess it applies here. Anyway, I never liked that rule, so I, of course, > agree on *always* testing with the version as close as possible to what most > users have. I think we shouldn't bump unless we absolutely need to, as it introduces a lot of churn in test results and forces every developer to recompile the *entire* JHBuild moduleset. It's a lot of work to update many results and the JHBuild compilation time is nothing to sneeze at. On the other hand, if we are forced to update, I don't think there is any issue updating to a recent version. Still perhaps it is time to reexamine the rule. One idea is that it is fine to update JHBuild moduleset if the updater personally takes responsibility for carefully updating all pixel and layout test results that change as a result.

(In reply to comment #3) > I think we should go straight to the latest stable release, 3.16.4, since > it's closer to what our users will actually have. There's less value in > testing with 3.12.x since it's already quite old. > I think that we may be risking to break compatibility with older versions without noticing. If we put 3.16.x on the JHBuild, we won't longer test with older versions. The possibility that a patch inadvertently introduces some change that breaks with < 3.16.x is real. But, the same will happen with an update to 3.12 (is only a matter of limiting the hypothetical breakage to <3.12 instead of <3.16). In any case, I'm fine with updating GTK to 3.16 if you all are also ok. I'm wonder if along with the update to GTK-3.16 we should also (or we want to) update any other library that GTK may depend on like glib, cairo, etc?

(In reply to comment #7) > I think that we may be risking to break compatibility with older versions > without noticing. > > If we put 3.16.x on the JHBuild, we won't longer test with older versions. > The possibility that a patch inadvertently introduces some change that > breaks with < 3.16.x is real. Hm... I was thinking that we used GDK_VERSION_MAX_ALLOWED to protect against this, but we don't. And we probably can't, since we often have to use newer things in conditional compilation blocks. So you have a good point.

(In reply to comment #7) > (In reply to comment #3) > > I think we should go straight to the latest stable release, 3.16.4, since > > it's closer to what our users will actually have. There's less value in > > testing with 3.12.x since it's already quite old. > > > > I think that we may be risking to break compatibility with older versions > without noticing. However, what usually happens is the opposite, things stop working or are broken with recent versions of GTK, and we don't notice it, but our users do. Remember the huge scrollbars and other theming issues, the GMutexLocker build issue, etc. > If we put 3.16.x on the JHBuild, we won't longer test with older versions. > The possibility that a patch inadvertently introduces some change that > breaks with < 3.16.x is real. We just need to be careful to use conditional compilation when using newer GTK+ API. It's not that common. The thing is that what we are testing is far from real. > But, the same will happen with an update to 3.12 (is only a matter of > limiting the hypothetical breakage to <3.12 instead of <3.16). > > In any case, I'm fine with updating GTK to 3.16 if you all are also ok. > > > I'm wonder if along with the update to GTK-3.16 we should also (or we want > to) update any other library that GTK may depend on like glib, cairo, etc? Yes, indeed. We need to check if newer GTK+ also requires newer versions of other deps, I bet it requires newer glib, for example, but also ATK, pango, etc. The other important dependency that affects the layout tests output is cairo. So, I would take advantage that we will have to rebaseline to also update cairo. With recent enough cairo and GTK+ we have HiDPI support, that we have never tested because we were using an old GTK.

(In reply to comment #7) > In any case, I'm fine with updating GTK to 3.16 if you all are also > ok. Debian sid has just upgraded to GTK 3.16 so I don't think it will cause any problem there.

Created attachment 255754 [details] Patch Requesting only review for the patch affecting the jhbuild code. Once this is r+, I will prepare to land it followed by an unreviewed gardening patch rebasing all the affected tests.

Created attachment 255920 [details] Patch Upload again to get the GTK EWS green now that it has libepoxy installed.

Reviewers please? Any comment regarding this patch?

Comment on attachment 255920 [details] Patch I think this is a good idea, but I'm surprised this changes no results at all (even non-pixel ones).

(In reply to comment #14) > Comment on attachment 255920 [details] > Patch > > I think this is a good idea, but I'm surprised this changes no results at > all (even non-pixel ones). (In reply to comment #14) > Comment on attachment 255920 [details] > Patch > > I think this is a good idea, but I'm surprised this changes no results at > all (even non-pixel ones). Thanks for the review. Without pixel tests this is a summary that I extracted after running three times (to discard flaky tests) the full Layout tests before and after applying this patch: New failures after applying this patch: fast/backgrounds/hidpi-background-image-contain-cover-scale-needs-more-precision.html [ ImageOnlyFailure ] fast/css/input-search-padding.html [ Failure ] fast/events/touch/tap-highlight-color.html [ Timeout ] fast/events/touch/touch-inside-iframe-scrolled.html [ Timeout ] fast/events/touch/touch-scaled-scrolled.html [ Timeout ] fast/events/touch/touch-stale-node-crash.html [ Timeout ] fast/hidpi/filters-blur.html [ ImageOnlyFailure ] fast/hidpi/filters-hue-rotate.html [ ImageOnlyFailure ] fast/hidpi/filters-invert.html [ ImageOnlyFailure ] fast/hidpi/filters-multiple.html [ ImageOnlyFailure ] fast/hidpi/filters-reference.html [ ImageOnlyFailure ] fast/selectors/matches-backtracking.html [ Timeout ] inspector/css/matched-style-properties.html [ Timeout ] inspector/css/stylesheet-with-mutations.html [ Timeout ] New passes after applying this patch: fast/canvas/canvas-fillPath-shadow.html fast/dom/HTMLImageElement/sizes/image-sizes-2x.html fast/dom/HTMLImageElement/sizes/image-sizes-js-change.html fast/events/touch/touch-inside-iframe.html fast/hidpi/image-srcset-change-dynamically-from-js-2x.html fast/hidpi/image-srcset-fraction-1.5x.html fast/hidpi/image-srcset-fraction.html fast/hidpi/image-srcset-invalid-inputs-except-one.html fast/hidpi/image-srcset-simple-2x.html fast/hidpi/image-srcset-src-selection-2x.html With pixel tests enabled, I'm working on identifying the new failures, but is a bit complicated since there are already like ~2000 image failures even before applying this patch. This are the gross numbers are: Right now, before applying this patch: => Tests to be fixed (7036): 5 crashes ( 0.1%) 85 timeouts ( 1.2%) 886 missing results (12.6%) 1990 image-only failures (28.3%) After applying this patch: => Tests to be fixed (7061): 8 crashes ( 0.1%) 94 timeouts ( 1.3%) 887 missing results (12.6%) 2012 image-only failures (28.5%) So, we have at least 22 new image-only failures that need to be rebased. Probably more. My idea is that before landing this patch, I will work on preparing a gardening patch that will be landed just after landing this one. I will do my best to check and rebase the tests that I identify as affected by this upgrade of libraries. The non-pixel test case is manageable and I have already identified the tests affected. However, for the pixel test case, I don't think I can check all failures, as there are already ~2000 failures before landing this. But, I will try to do my best and rebase/report the tests that I detect as new failures by comparing the previous test failure list with the new one.

(In reply to comment #15) > fast/backgrounds/hidpi-background-image-contain-cover-scale-needs-more- > precision.html [ ImageOnlyFailure ] > fast/css/input-search-padding.html [ Failure ] > fast/events/touch/tap-highlight-color.html [ Timeout ] > fast/events/touch/touch-inside-iframe-scrolled.html [ Timeout ] > fast/events/touch/touch-scaled-scrolled.html [ Timeout ] > fast/events/touch/touch-stale-node-crash.html [ Timeout ] > fast/hidpi/filters-blur.html [ ImageOnlyFailure ] > fast/hidpi/filters-hue-rotate.html [ ImageOnlyFailure ] > fast/hidpi/filters-invert.html [ ImageOnlyFailure ] > fast/hidpi/filters-multiple.html [ ImageOnlyFailure ] > fast/hidpi/filters-reference.html [ ImageOnlyFailure ] > fast/selectors/matches-backtracking.html [ Timeout ] > inspector/css/matched-style-properties.html [ Timeout ] > inspector/css/stylesheet-with-mutations.html [ Timeout ] These new timeouts worry me quite a bit. Is it possible to figure out what is causing them? The lone failure might just need to be rebased.

(In reply to comment #16) > These new timeouts worry me quite a bit. Is it possible to figure out what > is causing them? The lone failure might just need to be rebased. Running the layout tests only with the 7 tests that now timeout instead of the full site makes some of them work back, so maybe some of this tests have become slow. => Results: 4/7 tests passed (57.1%) => Tests to be fixed (3): 3 timeouts (100.0%) => Tests that will only be fixed if they crash (WONTFIX) (0): Expected to fail, but passed: (1) fast/events/touch/touch-inside-iframe-scrolled.html Regressions: Unexpected timeouts (3) fast/events/touch/tap-highlight-color.html [ Timeout ] fast/events/touch/touch-scaled-scrolled.html [ Timeout ] fast/events/touch/touch-stale-node-crash.html [ Timeout ] ------------------------------------------------------------------------------ This new 3 timeouts, are indeed new timeout as they still timeout if you pass the flag --no-timeout. I have no idea at this moment why they timeout with the new set of libraries.

Comment on attachment 255920 [details] Patch Clearing flags on attachment: 255920 Committed r186500: <http://trac.webkit.org/changeset/186500>

All reviewed patches have been landed. Closing bug.

(In reply to comment #18) > Comment on attachment 255920 [details] > Patch > > Clearing flags on attachment: 255920 > > Committed r186500: <http://trac.webkit.org/changeset/186500> Do the timeouts persist on the bots. I didn't see TestExpectations updates in the patch that landed?

(In reply to comment #20) > (In reply to comment #18) > > Comment on attachment 255920 [details] > > Patch > > > > Clearing flags on attachment: 255920 > > > > Committed r186500: <http://trac.webkit.org/changeset/186500> > > Do the timeouts persist on the bots. I didn't see TestExpectations updates > in the patch that landed? The bot is still processing. I'm waiting to check the results from the bot. I will update this bug once I land the gardening patch

(In reply to comment #21) > (In reply to comment #20) > > (In reply to comment #18) > > > Comment on attachment 255920 [details] > > > Patch > > > > > > Clearing flags on attachment: 255920 > > > > > > Committed r186500: <http://trac.webkit.org/changeset/186500> > > > > Do the timeouts persist on the bots. I didn't see TestExpectations updates > > in the patch that landed? > > The bot is still processing. I'm waiting to check the results from the bot. > I will update this bug once I land the gardening patch I have updated the expectations file and rebaselined one test on http://trac.webkit.org/changeset/186513

Also I reported here all the new failures/timeouts that I have identified as part of this upgrade of libraries: https://bugs.webkit.org/show_bug.cgi?id=146731