[webkit-reviews] review canceled: [Bug 83889] Crash in WebKit!WebCore::RenderBoxModelObject::paddingLeft+0x5c. : [Attachment 139602] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Takashi Sakamoto
<tasak at google.com>'s request for review:
Bug 83889: Crash in WebKit!WebCore::RenderBoxModelObject::paddingLeft+0x5c.
https://bugs.webkit.org/show_bug.cgi?id=83889

Attachment 139602: Patch
https://bugs.webkit.org/attachment.cgi?id=139602&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=139602&action=review


The code change is fine.

>> LayoutTests/scrollbars/scrollbar-percent-padding-crash.html:1
>> +<style>
> 
> I could not reproduce this ref-test failure without your patch. Did it work
for you ?.

For the record, I don't really like a ref-test that is really not one. Have the
2 files (test & reference) be exactly the same doesn't make a good ref-test as
any change would still make the test pass. This should be a dumpAsText(true)
output as it would at least catch up variation on the pixel output. If that
makes the output platform specific, we may need to find another way.

> LayoutTests/scrollbars/scrollbar-percent-padding-crash.html:13
> +</script>
> +<div style="height: 1000px;">

Test *should* have the following information:
* bug title
* bug id or URL (I prefer URL in a clickable format)
* explain what they expect

Without those information, the test is at most ambiguous and it will be
difficult for any maintainer to know what is expected and if any variation is
expected.