Re: [webkit-dev] About fixing old layout bugs
Hi, developers, I'd really like my patches to be reviewed and landed. Could any reviewer/commiter help me? The normal review/commit queue mechanisms aren't suitable for the the patches because the layout-tests in them are almost always out-dated. Thanks, Xianzhu 2010/8/1 Xianzhu Wang phnix...@gmail.com Hi, Now the status of the three old bugs are: * white space preceding br ( https://bugs.webkit.org/show_bug.cgi?id=37261): New patches uploaded * relative font-size (https://bugs.webkit.org/show_bug.cgi?id=18413): New patches uploaded * line breaking around some punctuations ( https://bugs.webkit.org/show_bug.cgi?id=37698): Fixed For the br bug, I still split the patch into following parts to ease review: 1. A patch containing only the changed code and the added test; 2. A patch containing all affected text-only layout tests that differ only about trailing spaces; 3. A patch containing all affected DRT layout tests that differ only about trailing spaces of text rendering nodes; 4. A patch containing other layout tests that have been manually adjusted; 5. A patch containing changed Skipped files of other platforms. The above 2 and 3 were auto generated by scripts. And instead of rebaslining tests of other platforms, I simply (temporarily) added the affected tests into Skipped files. I used the similar method to generate the patch for the second bug but didn't split it because the patch is much smaller. The patches also don't contain pixel tests to reduce the scope and size. Because of rapid changes of affected layout tests, the patches had already been out-dated even before I uploaded them. It seems to me that the normal process of review queue and commit queue doesn't work for such patches. Would any committer please help review and maybe commit the patches? Thanks, Xianzhu 2010/6/2 Xianzhu Wang phnix...@gmail.com Hi, I'm new to webkit development, and I'd like to hear opinions about the problems I met. Now I'm trying to fix some old layout bugs, for example: * white space preceding br ( https://bugs.webkit.org/show_bug.cgi?id=37261) * relative font-size (https://bugs.webkit.org/show_bug.cgi?id=18413) * line breaking around some punctuations ( https://bugs.webkit.org/show_bug.cgi?id=37698) Some people have warned me about the difficulty of fixing these bugs, and now I have realized it. Fixing the bugs themselves is not very difficult, for example, only 2 functional lines change can fix the first bug. However, the change will break more than 4000 existing layout tests mostly because trailing spaces preceding brs in current expectations of the tests (for example, PASS vs PASS). I tried to rebaseline all effected layout tests (for now on mac only), and the patch is about 6MB (Sorry I overlooked the Bigfile option when I submitted the patch, so I split it into 4 parts). My questions are: 1. The bugs violate the standards and cause some site compatibility issues. However, because the bugs are old, some web developers might treat them as features and depend on them, so fixing them might break some existing pages. Is there any existing policy about this problem? Are these bugs worth fixing? 2. What's the best practice to deal with a patche containing many updated layout test expectations? Is there a good way to rebaseline the effected tests on all platforms? Thanks, Xianzhu ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] About fixing old layout bugs
In fact the (really lousy) model I've employed in the past when this situation has arisen is that I hack the render tree dump to continue to dump the old rendering. The render tree dumping code is full of hacks as a result and is basically lying about many things at this point. It would be really nice to take the time to remove all of these hacks and just update every test, but we need a good solution for how to update many tests without breaking the tree. dave On Jun 3, 2010, at 6:16 PM, Ojan Vafai wrote: When there are only a couple tests that need new expectations, you can get away with committing your patch with the expectations for the platforms you have access to and then immediately grabbing the new expectations off the buildbots. There is currently no good way to address the cases where your patch causes many tests to need new results. There are ideas to make it better, but I don't think anyone is actively working on them. Specifically, the EWS bots could run the layout tests and give you access to the results. For now, with patches where you need to change many results and they're different on different platforms, you need to either get access to that platform, or get the help of someone who has access to it. Ojan On Wed, Jun 2, 2010 at 7:32 PM, Xianzhu Wang phnix...@gmail.com wrote: Hi, I'm still wondering what the best practice is to deal with many updated layout tests in a patch. It seems I must run the layout tests on all effected platforms by myself to ensure a green build after committing the patch, right? This is really difficult to me. Is there a easier way? Thanks, Xianzhu 2010/6/2 Xianzhu Wang phnix...@gmail.com Hi, I'm new to webkit development, and I'd like to hear opinions about the problems I met. Now I'm trying to fix some old layout bugs, for example: * white space preceding br (https://bugs.webkit.org/show_bug.cgi?id=37261) * relative font-size (https://bugs.webkit.org/show_bug.cgi?id=18413) * line breaking around some punctuations (https://bugs.webkit.org/show_bug.cgi?id=37698) Some people have warned me about the difficulty of fixing these bugs, and now I have realized it. Fixing the bugs themselves is not very difficult, for example, only 2 functional lines change can fix the first bug. However, the change will break more than 4000 existing layout tests mostly because trailing spaces preceding brs in current expectations of the tests (for example, PASS vs PASS). I tried to rebaseline all effected layout tests (for now on mac only), and the patch is about 6MB (Sorry I overlooked the Bigfile option when I submitted the patch, so I split it into 4 parts). My questions are: 1. The bugs violate the standards and cause some site compatibility issues. However, because the bugs are old, some web developers might treat them as features and depend on them, so fixing them might break some existing pages. Is there any existing policy about this problem? Are these bugs worth fixing? 2. What's the best practice to deal with a patche containing many updated layout test expectations? Is there a good way to rebaseline the effected tests on all platforms? Thanks, Xianzhu ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] About fixing old layout bugs
For this particular bug (the br one), even ref tests would be inadequate, since the reference renderings would have to change too. dave On Jun 4, 2010, at 2:17 PM, Dirk Pranke wrote: One admittedly painful way to do this would be to dump two render trees, an old format and a new format, and then build tooling to roll between the versions. Most of the pain would probably be in modifying the dump code to accept version flags and know whether to output old or new as necessary. Of course, you can't really do this for pixel tests, so you're still kind of stuck. ref tests might be a better answer in both situations. -- Dirk On Fri, Jun 4, 2010 at 10:36 AM, David Hyatt hy...@apple.com wrote: In fact the (really lousy) model I've employed in the past when this situation has arisen is that I hack the render tree dump to continue to dump the old rendering. The render tree dumping code is full of hacks as a result and is basically lying about many things at this point. It would be really nice to take the time to remove all of these hacks and just update every test, but we need a good solution for how to update many tests without breaking the tree. dave On Jun 3, 2010, at 6:16 PM, Ojan Vafai wrote: When there are only a couple tests that need new expectations, you can get away with committing your patch with the expectations for the platforms you have access to and then immediately grabbing the new expectations off the buildbots. There is currently no good way to address the cases where your patch causes many tests to need new results. There are ideas to make it better, but I don't think anyone is actively working on them. Specifically, the EWS bots could run the layout tests and give you access to the results. For now, with patches where you need to change many results and they're different on different platforms, you need to either get access to that platform, or get the help of someone who has access to it. Ojan On Wed, Jun 2, 2010 at 7:32 PM, Xianzhu Wang phnix...@gmail.com wrote: Hi, I'm still wondering what the best practice is to deal with many updated layout tests in a patch. It seems I must run the layout tests on all effected platforms by myself to ensure a green build after committing the patch, right? This is really difficult to me. Is there a easier way? Thanks, Xianzhu 2010/6/2 Xianzhu Wang phnix...@gmail.com Hi, I'm new to webkit development, and I'd like to hear opinions about the problems I met. Now I'm trying to fix some old layout bugs, for example: * white space preceding br (https://bugs.webkit.org/show_bug.cgi?id=37261) * relative font-size (https://bugs.webkit.org/show_bug.cgi?id=18413) * line breaking around some punctuations (https://bugs.webkit.org/show_bug.cgi?id=37698) Some people have warned me about the difficulty of fixing these bugs, and now I have realized it. Fixing the bugs themselves is not very difficult, for example, only 2 functional lines change can fix the first bug. However, the change will break more than 4000 existing layout tests mostly because trailing spaces preceding brs in current expectations of the tests (for example, PASS vs PASS). I tried to rebaseline all effected layout tests (for now on mac only), and the patch is about 6MB (Sorry I overlooked the Bigfile option when I submitted the patch, so I split it into 4 parts). My questions are: 1. The bugs violate the standards and cause some site compatibility issues. However, because the bugs are old, some web developers might treat them as features and depend on them, so fixing them might break some existing pages. Is there any existing policy about this problem? Are these bugs worth fixing? 2. What's the best practice to deal with a patche containing many updated layout test expectations? Is there a good way to rebaseline the effected tests on all platforms? Thanks, Xianzhu ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] About fixing old layout bugs
When there are only a couple tests that need new expectations, you can get away with committing your patch with the expectations for the platforms you have access to and then immediately grabbing the new expectations off the buildbots. There is currently no good way to address the cases where your patch causes many tests to need new results. There are ideas to make it better, but I don't think anyone is actively working on them. Specifically, the EWS bots could run the layout tests and give you access to the results. For now, with patches where you need to change many results and they're different on different platforms, you need to either get access to that platform, or get the help of someone who has access to it. Ojan On Wed, Jun 2, 2010 at 7:32 PM, Xianzhu Wang phnix...@gmail.com wrote: Hi, I'm still wondering what the best practice is to deal with many updated layout tests in a patch. It seems I must run the layout tests on all effected platforms by myself to ensure a green build after committing the patch, right? This is really difficult to me. Is there a easier way? Thanks, Xianzhu 2010/6/2 Xianzhu Wang phnix...@gmail.com Hi, I'm new to webkit development, and I'd like to hear opinions about the problems I met. Now I'm trying to fix some old layout bugs, for example: * white space preceding br ( https://bugs.webkit.org/show_bug.cgi?id=37261) * relative font-size (https://bugs.webkit.org/show_bug.cgi?id=18413) * line breaking around some punctuations ( https://bugs.webkit.org/show_bug.cgi?id=37698) Some people have warned me about the difficulty of fixing these bugs, and now I have realized it. Fixing the bugs themselves is not very difficult, for example, only 2 functional lines change can fix the first bug. However, the change will break more than 4000 existing layout tests mostly because trailing spaces preceding brs in current expectations of the tests (for example, PASS vs PASS). I tried to rebaseline all effected layout tests (for now on mac only), and the patch is about 6MB (Sorry I overlooked the Bigfile option when I submitted the patch, so I split it into 4 parts). My questions are: 1. The bugs violate the standards and cause some site compatibility issues. However, because the bugs are old, some web developers might treat them as features and depend on them, so fixing them might break some existing pages. Is there any existing policy about this problem? Are these bugs worth fixing? 2. What's the best practice to deal with a patche containing many updated layout test expectations? Is there a good way to rebaseline the effected tests on all platforms? Thanks, Xianzhu ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] About fixing old layout bugs
Thanks Dan for reply. 2010/6/2 Dan Bernstein m...@apple.com Hi Xianzhu, On Jun 1, 2010, at 9:37 PM, Xianzhu Wang wrote: Hi, I'm new to webkit development, and I'd like to hear opinions about the problems I met. Now I'm trying to fix some old layout bugs, for example: * white space preceding br ( https://bugs.webkit.org/show_bug.cgi?id=37261) * relative font-size (https://bugs.webkit.org/show_bug.cgi?id=18413) * line breaking around some punctuations ( https://bugs.webkit.org/show_bug.cgi?id=37698) Some people have warned me about the difficulty of fixing these bugs, and now I have realized it. Fixing the bugs themselves is not very difficult, for example, only 2 functional lines change can fix the first bug. However, the change will break more than 4000 existing layout tests mostly because trailing spaces preceding brs in current expectations of the tests (for example, PASS vs PASS). I tried to rebaseline all effected layout tests (for now on mac only), and the patch is about 6MB (Sorry I overlooked the Bigfile option when I submitted the patch, so I split it into 4 parts). My questions are: 1. The bugs violate the standards and cause some site compatibility issues. However, because the bugs are old, some web developers might treat them as features and depend on them, so fixing them might break some existing pages. Is there any existing policy about this problem? Not sure. I think WebKit should not make a policy decision to diverge from the standard (quirks mode withstanding) unless WebKit developers are actively advocating a change to the relevant standard. Are these bugs worth fixing? * white space preceding br ( https://bugs.webkit.org/show_bug.cgi?id=37261) Unless I’m missing something, there is no known downside to fixing this bug. Seems worthwhile. * relative font-size (https://bugs.webkit.org/show_bug.cgi?id=18413) Is this really a standards compliance issue or just compatibility with Firefox? If there is a standards issue, is there a subset of the change that’s sufficient for standards compliance? Unless WebKit is in violation of a standard here, I personally don’t consider this important, but I know that other contributors have agreed to this change in principle, and it’s just that the patch has been abandoned by the original author. Yes, this is a standard compliance issue ( http://www.w3.org/TR/CSS2/fonts.html#font-size-props). I have asked the original author and he'd like me to continue his work. * line breaking around some punctuations ( https://bugs.webkit.org/show_bug.cgi?id=37698) Seems worthwhile if it aligns WebKit with Firefox and IE. If moves away from IE compatibility, I wouldn’t do it. The main issue here is efficiency. I've uploaded another patch (for preview) that should have the same efficiency as the current algorithm and keep IE compatibility. I'll upload the whole patch soon. Personally, I think it’s good that you’re looking into such long-standing standards/compatibility issues! Regards, —Dan ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] About fixing old layout bugs
Hi, I'm still wondering what the best practice is to deal with many updated layout tests in a patch. It seems I must run the layout tests on all effected platforms by myself to ensure a green build after committing the patch, right? This is really difficult to me. Is there a easier way? Thanks, Xianzhu 2010/6/2 Xianzhu Wang phnix...@gmail.com Hi, I'm new to webkit development, and I'd like to hear opinions about the problems I met. Now I'm trying to fix some old layout bugs, for example: * white space preceding br ( https://bugs.webkit.org/show_bug.cgi?id=37261) * relative font-size (https://bugs.webkit.org/show_bug.cgi?id=18413) * line breaking around some punctuations ( https://bugs.webkit.org/show_bug.cgi?id=37698) Some people have warned me about the difficulty of fixing these bugs, and now I have realized it. Fixing the bugs themselves is not very difficult, for example, only 2 functional lines change can fix the first bug. However, the change will break more than 4000 existing layout tests mostly because trailing spaces preceding brs in current expectations of the tests (for example, PASS vs PASS). I tried to rebaseline all effected layout tests (for now on mac only), and the patch is about 6MB (Sorry I overlooked the Bigfile option when I submitted the patch, so I split it into 4 parts). My questions are: 1. The bugs violate the standards and cause some site compatibility issues. However, because the bugs are old, some web developers might treat them as features and depend on them, so fixing them might break some existing pages. Is there any existing policy about this problem? Are these bugs worth fixing? 2. What's the best practice to deal with a patche containing many updated layout test expectations? Is there a good way to rebaseline the effected tests on all platforms? Thanks, Xianzhu ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] About fixing old layout bugs
Hi, I'm new to webkit development, and I'd like to hear opinions about the problems I met. Now I'm trying to fix some old layout bugs, for example: * white space preceding br ( https://bugs.webkit.org/show_bug.cgi?id=37261) * relative font-size (https://bugs.webkit.org/show_bug.cgi?id=18413) * line breaking around some punctuations ( https://bugs.webkit.org/show_bug.cgi?id=37698) Some people have warned me about the difficulty of fixing these bugs, and now I have realized it. Fixing the bugs themselves is not very difficult, for example, only 2 functional lines change can fix the first bug. However, the change will break more than 4000 existing layout tests mostly because trailing spaces preceding brs in current expectations of the tests (for example, PASS vs PASS). I tried to rebaseline all effected layout tests (for now on mac only), and the patch is about 6MB (Sorry I overlooked the Bigfile option when I submitted the patch, so I split it into 4 parts). My questions are: 1. The bugs violate the standards and cause some site compatibility issues. However, because the bugs are old, some web developers might treat them as features and depend on them, so fixing them might break some existing pages. Is there any existing policy about this problem? Are these bugs worth fixing? 2. What's the best practice to deal with a patche containing many updated layout test expectations? Is there a good way to rebaseline the effected tests on all platforms? Thanks, Xianzhu ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] About fixing old layout bugs
Hi Xianzhu, On Jun 1, 2010, at 9:37 PM, Xianzhu Wang wrote: Hi, I'm new to webkit development, and I'd like to hear opinions about the problems I met. Now I'm trying to fix some old layout bugs, for example: * white space preceding br (https://bugs.webkit.org/show_bug.cgi?id=37261) * relative font-size (https://bugs.webkit.org/show_bug.cgi?id=18413) * line breaking around some punctuations (https://bugs.webkit.org/show_bug.cgi?id=37698) Some people have warned me about the difficulty of fixing these bugs, and now I have realized it. Fixing the bugs themselves is not very difficult, for example, only 2 functional lines change can fix the first bug. However, the change will break more than 4000 existing layout tests mostly because trailing spaces preceding brs in current expectations of the tests (for example, PASS vs PASS). I tried to rebaseline all effected layout tests (for now on mac only), and the patch is about 6MB (Sorry I overlooked the Bigfile option when I submitted the patch, so I split it into 4 parts). My questions are: 1. The bugs violate the standards and cause some site compatibility issues. However, because the bugs are old, some web developers might treat them as features and depend on them, so fixing them might break some existing pages. Is there any existing policy about this problem? Not sure. I think WebKit should not make a policy decision to diverge from the standard (quirks mode withstanding) unless WebKit developers are actively advocating a change to the relevant standard. Are these bugs worth fixing? * white space preceding br (https://bugs.webkit.org/show_bug.cgi?id=37261) Unless I’m missing something, there is no known downside to fixing this bug. Seems worthwhile. * relative font-size (https://bugs.webkit.org/show_bug.cgi?id=18413) Is this really a standards compliance issue or just compatibility with Firefox? If there is a standards issue, is there a subset of the change that’s sufficient for standards compliance? Unless WebKit is in violation of a standard here, I personally don’t consider this important, but I know that other contributors have agreed to this change in principle, and it’s just that the patch has been abandoned by the original author. * line breaking around some punctuations (https://bugs.webkit.org/show_bug.cgi?id=37698) Seems worthwhile if it aligns WebKit with Firefox and IE. If moves away from IE compatibility, I wouldn’t do it. The main issue here is efficiency. Personally, I think it’s good that you’re looking into such long-standing standards/compatibility issues! Regards, —Dan___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
