Bug 77450

Created attachment 124786 [details] Patch

(In reply to comment #1) > Created an attachment (id=124786) [details] > Patch Ossy, it would be cool if you could review this. I didn't include the whole "git mv qt-wk2 qt-5.0-wk2" on this patch because its size was getting to 6.2 MB. If I get the r+ on this I will land it all together manually.

Attachment 124786 [details] did not pass style-queue: Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9 Updating OpenSource First, rewinding head to replay your work on top of it... Applying: Fix compilation errors on build-webkit --debug --no-workers on mac. Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Auto-merging LayoutTests/ChangeLog CONFLICT (content): Merge conflict in LayoutTests/ChangeLog Auto-merging LayoutTests/platform/qt/Skipped CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped Auto-merging Source/WebCore/ChangeLog CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog Auto-merging Source/WebCore/WebCore.exp.in Auto-merging Source/WebKit/mac/ChangeLog CONFLICT (content): Merge conflict in Source/WebKit/mac/ChangeLog Failed to merge in the changes. Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac. When you have resolved this problem run "git rebase --continue". If you would prefer to skip this patch, instead run "git rebase --skip". To restore the original branch and stop rebasing run "git rebase --abort". rebase refs/remotes/origin/master: command returned error: 1 Died at Tools/Scripts/update-webkit line 164. If any of these errors are false positives, please file a bug against check-webkit-style.

Now we can skip tests on different ways: - qt-4.8: Qt4.8-WK1 - qt-5.0: Qt5.0-WK1 and Qt5.0-WK2 - qt-wk1: Qt4.8-WK1 and Qt5.0-WK1 - qt-wk2: Qt5.0-WK2 Which is missing that we can't skip a test for only Qt5.0-WK1. With your change we can skip tests on different ways: - qt-4.8: Qt4.8-WK1 - qt-5.0: Qt5.0-WK1 and Qt5.0-WK2 - qt-5.0-wk1: Qt5.0-WK1 - qt-5.0-wk2: Qt5.0-WK2 But now we can't skip tests for Qt4.8-WK1 and Qt5.0-WK1 (recent qt-wk1), and you added duplicated entries to qt-4.8 and qt-5.0-wk1 skipped lists. What if we add qt-5.0-wk1 and qt-5.0-wk2 and don't remove the general qt-wk1?

Otherwise I think it would be better if we - remove obsolete (4 month old) png files (introduced in http://trac.webkit.org/changeset/95412) from qt-wk2 before this patch lands and reland updated png files after https://bugs.webkit.org/show_bug.cgi?id=70484 fixed. - land the future r+ -ed patch from svn to preserv the Skipped list histories. (use svn cp, move, etc.)

(In reply to comment #4) > But now we can't skip tests for Qt4.8-WK1 and Qt5.0-WK1 (recent qt-wk1), > and you added duplicated entries to qt-4.8 and qt-5.0-wk1 skipped lists. > What if we add qt-5.0-wk1 and qt-5.0-wk2 and don't remove the general qt-wk1? I know it might sound weird but from my brief experience with gardening I'd rather have to replicate this tests in qt-4.8 and qt-5.0-wk1. It seemed to me more natural just to skip or unskip tests directly from a list that actually tells what it is dealing with... (In reply to comment #5) > - remove obsolete (4 month old) png files (introduced in http://trac.webkit.org/changeset/95412) from qt-wk2 before this patch lands and reland updated png files after https://bugs.webkit.org/show_bug.cgi?id=70484 fixed. > - land the future r+ -ed patch from svn to preserv the Skipped list histories. > (use svn cp, move, etc.) Not sure I'm following you here... Why do need to remove the png files and add new ones? Should I do that or one of you guys will deal with it when https://bugs.webkit.org/show_bug.cgi?id=70484 is fixed ? Thanks for the review!

(In reply to comment #6) > (In reply to comment #4) > > But now we can't skip tests for Qt4.8-WK1 and Qt5.0-WK1 (recent qt-wk1), > > and you added duplicated entries to qt-4.8 and qt-5.0-wk1 skipped lists. > > What if we add qt-5.0-wk1 and qt-5.0-wk2 and don't remove the general qt-wk1? > > I know it might sound weird but from my brief experience with gardening I'd rather have to replicate this tests in qt-4.8 and qt-5.0-wk1. It seemed to me more natural just to skip or unskip tests directly from a list that actually tells what it is dealing with... It's all the same for me, I can agree with you if there are only a couple of duplicated tests. > (In reply to comment #5) > > - remove obsolete (4 month old) png files (introduced in http://trac.webkit.org/changeset/95412) from qt-wk2 before this patch lands and reland updated png files after https://bugs.webkit.org/show_bug.cgi?id=70484 fixed. > > - land the future r+ -ed patch from svn to preserv the Skipped list histories. > > (use svn cp, move, etc.) > > Not sure I'm following you here... Why do need to remove the png files and add new ones? Should I do that or one of you guys will deal with it when https://bugs.webkit.org/show_bug.cgi?id=70484 is fixed ? Because these png files are absolutely obsolete ... And I don't know when will https://bugs.webkit.org/show_bug.cgi?id=70484 be fixed and when can we enable pixel tests. But I don't think if in the near future. So I prefer removing obsolete png files instead of moving them with a huge patch.

Balázs, have you got any objection against removing these obsolete png's?

(In reply to comment #8) > Balázs, have you got any objection against removing these obsolete png's? Please, do that! :)

(In reply to comment #9) > (In reply to comment #8) > > Balázs, have you got any objection against removing these obsolete png's? > > Please, do that! :) Done. ;)

Created attachment 126312 [details] updated patch with svn cp/mv to keep the history

Comment on attachment 126312 [details] updated patch with svn cp/mv to keep the history r=me and I'll land the patch manually from svn.

(In reply to comment #12) > (From update of attachment 126312 [details]) > r=me and I'll land the patch manually from svn. Thank you, Ossy! I'm rushing a few things here but please let me know if there is anything else I need to do after this, ok? Thanks again!

Comment on attachment 126312 [details] updated patch with svn cp/mv to keep the history Landed in http://trac.webkit.org/changeset/107397 and http://trac.webkit.org/changeset/107398