Summary: | BORDER attribute with the object tag, using percentage values not working. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mihnea Ovidenie <mihnea> | ||||||||
Component: | DOM | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, ddkilzer, simon.fraser, tabatkins, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Mihnea Ovidenie
2011-07-26 05:48:10 PDT
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. *** |