[chromium-dev] Re: seeking webkit reviewer for chrome-specific patches
Be sheriff that day :) Real advice: Once you have webkit patch R+'ed and chrome rebaselines ready, let the gardener know. Once the gardener is caught up, they can set commit-queue flag on your change, so it gets committed at a time when they are ready to deal with it and your follow up change will be there ready to fix everything. On Tue, Oct 13, 2009 at 4:06 PM, Evan Martin e...@chromium.org wrote: Both of these patches don't really have an obvious reviewer, but they're pretty simple. https://bugs.webkit.org/show_bug.cgi?id=30319 https://bugs.webkit.org/show_bug.cgi?id=30320 The latter one will require an epic amount of rebaselining, which I have volunteered to do. If anyone has advice on how to do that in a way that makes the webkit sheriffs happy, let me know. --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: seeking webkit reviewer for chrome-specific patches
I glanced at the other one. there were a few issues with it: 1. It does more than it says. It changes how the horizontal lines are drawn without explanation (nothing in the changelog and the bug only mentions the other direction). 2. It uses a lot of magic numbers and the same ones repeatedly. It would be nice to make constants out of some of them to help inform the reader where they come from. 3. It is unclear if there are any layout tests which cover this change. I would be happy to r- if you want :) but am uncomfortable with an r+ due to those reasons. On Tue, Oct 13, 2009 at 5:25 PM, Evan Martin e...@chromium.org wrote: dimich looked at one. Any reviewer for the other? It's like 8 obvious Linux-specific lines, no stress I promise. :) On Tue, Oct 13, 2009 at 4:06 PM, Evan Martin e...@chromium.org wrote: Both of these patches don't really have an obvious reviewer, but they're pretty simple. https://bugs.webkit.org/show_bug.cgi?id=30319 https://bugs.webkit.org/show_bug.cgi?id=30320 The latter one will require an epic amount of rebaselining, which I have volunteered to do. If anyone has advice on how to do that in a way that makes the webkit sheriffs happy, let me know. --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
[chromium-dev] Re: seeking webkit reviewer for chrome-specific patches
3. It is unclear if there are any layout tests which cover this change. I don't think layout tests can or should cover that change, except for pixel tests, which will just need to be rebaselined. --~--~-~--~~~---~--~~ Chromium Developers mailing list: chromium-dev@googlegroups.com View archives, change email options, or unsubscribe: http://groups.google.com/group/chromium-dev -~--~~~~--~~--~--~---
