El lun, 29-09-2014 a las 23:51 -0700, Alexey Proskuryakov escribió: > 29 сент. 2014 г., в 23:12, Carlos Garcia Campos <[email protected]> > написал(а): > > >> Please try it out, and let me know if something breaks, or is not as > >> good as it could be! > > > > Looks great! thanks. I've noticed that now we use red also for patches > > that don't apply. I liked that there was a different color for that > > case, since it's common when a patch depends on another one, and the > > patch itself is not necessarily bad. > > I agree that this is a non-obvious case. I chose red because EWS can't > say anything good about the patch (or anything at all), so it needs > extra careful attention from a reviewer. > > Other options that I considered were: > > - Keep it purple, as it was. I restricted purple to indicating > internal errors, so it should pretty much never happen. Using the same > color for the entirely different case of a patch that does not apply > seems wrong.
Agree. > - Make it white. That seems to make sense logically, but it has the > same problem of using a single color for multiple purposes as red > does. Also, I expect it to be surprising (I posted my patch several > hours ago, why is it still white?) Agree. > - Add a new color. I'm not sure if this case is common enough for the > color to become generally recognizable. Well, I guess it's a matter of getting used to it :-) > - Instead of per-queue bubbles, show text like "Patch does not apply > to trunk". Not workable, because queues start at different times, and > only some of them could fail to apply the patch. Right. > Perhaps most importantly, this case is really no different from > failing to build due to a dependency on another patch, or to failing > some tests for the same reason. So I don't think that we can represent > dependency on another patch with color. Fair enough, I haven't thought about those cases, I agree it's indeed the same case and we will see red bubbles. Ideally, the EWS might apply the patches following the dependency chain, but I guess that's not easy to do. > Thinking about this now, we could replace bubbles with text when no > queue has results (i.e. at least one queue has failed to apply, and > those that didn't still haven't started processing - we can reasonably > expect that they will fail, too, and even if they apply cleanly, we > can just revert to showing bubbles then). I can't think of any > situation where this would be misleading or difficult to comprehend. > Would this resolve your concern? Would you be willing to post a patch > implementing this? Well, I was just surprised to see the bubbles red because I was used to seeing them purple, but I agree the case it's not different to when a patch fails to build because of a dependency. So, I'll get used to the red :-) > - Alexey > > _______________________________________________ webkit-dev mailing list [email protected] https://lists.webkit.org/mailman/listinfo/webkit-dev
