This is a follow up after bug 20226. <object id="OBJ" border="20%" data="red_200_200.png" type="image/png"></object> In the above case, border is not parsed to 20px but instead 0 is used, thus the object element does not have a border. Patch coming.
Created attachment 101990 [details] Patch
Comment on attachment 101990 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=101990&action=review > LayoutTests/ChangeLog:13 > + * fast/borders/script-tests/TEMPLATE.html: Added. Please don't add or use script test machinery. It's only for tests in fast/js, and using it elsewhere turned out to be a bad idea. I'm keeping this as r? for someone to look at actual code changes, but it's really r- for tests.
(In reply to comment #2) > (From update of attachment 101990 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=101990&action=review > > > LayoutTests/ChangeLog:13 > > + * fast/borders/script-tests/TEMPLATE.html: Added. > > Please don't add or use script test machinery. It's only for tests in fast/js, and using it elsewhere turned out to be a bad idea. > > I'm keeping this as r? for someone to look at actual code changes, but it's really r- for tests. Ok, was there a discussion on webkit-dev about this issue? Can you please explain me why it turned out to be a bad idea?
- it makes it unnecessarily difficult to reach test content from a bot results page, or when looking through tests in Finder; - it adds a file per test to svn, making checkouts and updates slower for everyone; - people unnecessarily use DOM manipulations to create content instead of simple markup. New tests shouldn't use separate .html and .js, except in rare cases (namely fast/js test, and when you actually share .js between multiple tests).
Created attachment 102239 [details] Patch 2 Created a single html file instead of the script test.
Comment on attachment 102239 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=102239&action=review I didn't attempt to think about the meaningful parts of this patch, just noting a few nitpicks about coding style. > LayoutTests/ChangeLog:8 > + Added the tests with border width with percentages in a new file. Removed them It might be slightly less confusing to say "moved" instead of "removed". > LayoutTests/fast/borders/border-width-percent.html:1 > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> It's marginally better to use an HTML5 doctype. > LayoutTests/fast/borders/border-width-percent.html:8 > +<p id="description">This tests the border width property with percent values for HTML elements.</p> Extremely small nit: no need for id on this element any more. > LayoutTests/fast/borders/border-width-percent.html:21 > + return borderBoxWidth / 2; Is it OK if the result is non-integer? Do we need rounding here? > Source/WebCore/html/HTMLElement.cpp:142 > + unsigned int borderWidth = 0; We don't say "unsigned int" as a matter of coding style. Please just use "unsigned", and it would be nice to file a bug against style checker. > Source/WebCore/html/HTMLElement.cpp:143 > + if (!attr->value().isEmpty() && !attr->value().isNull()) This check makes no sense - null string is also empty. > Source/WebCore/html/HTMLElement.h:93 > + unsigned int parseBorderWidthAttribute(Attribute*); Just unsigned.
Created attachment 102359 [details] Patch 3
(In reply to comment #6) > (From update of attachment 102239 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=102239&action=review > > I didn't attempt to think about the meaningful parts of this patch, just noting a few nitpicks about coding style. > > > LayoutTests/ChangeLog:8 > > + Added the tests with border width with percentages in a new file. Removed them > > It might be slightly less confusing to say "moved" instead of "removed". Done. > > > LayoutTests/fast/borders/border-width-percent.html:1 > > +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN"> > > It's marginally better to use an HTML5 doctype. Done. > > > LayoutTests/fast/borders/border-width-percent.html:8 > > +<p id="description">This tests the border width property with percent values for HTML elements.</p> > > Extremely small nit: no need for id on this element any more. Done. > > > LayoutTests/fast/borders/border-width-percent.html:21 > > + return borderBoxWidth / 2; > > Is it OK if the result is non-integer? Do we need rounding here? For the purpose of this test, this should be ok in my opinion. > > > Source/WebCore/html/HTMLElement.cpp:142 > > + unsigned int borderWidth = 0; > > We don't say "unsigned int" as a matter of coding style. Please just use "unsigned", and it would be nice to file a bug against style checker. Done. Filed https://bugs.webkit.org/show_bug.cgi?id=65375 > > > Source/WebCore/html/HTMLElement.cpp:143 > > + if (!attr->value().isEmpty() && !attr->value().isNull()) > > This check makes no sense - null string is also empty. Done. > > > Source/WebCore/html/HTMLElement.h:93 > > + unsigned int parseBorderWidthAttribute(Attribute*); > > Just unsigned. Done.
Comment on attachment 102359 [details] Patch 3 Looks ok. The hard part is already done at Bug 20226.
Comment on attachment 102359 [details] Patch 3 Clearing flags on attachment: 102359 Committed r92118: <http://trac.webkit.org/changeset/92118>
All reviewed patches have been landed. Closing bug.
*** Bug 48595 has been marked as a duplicate of this bug. ***