65176 – BORDER attribute with the object tag, using percentage values not working.

RESOLVED FIXED Bug 65176

BORDER attribute with the object tag, using percentage values not working.

https://bugs.webkit.org/show_bug.cgi?id=65176

Summary BORDER attribute with the object tag, using percentage values not working.

Mihnea Ovidenie

Reported 2011-07-26 05:48:10 PDT

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.

Attachments
Patch (9.85 KB, patch) 2011-07-26 05:59 PDT, Mihnea Ovidenie	no flags	Details Formatted Diff Diff
Patch 2 (8.72 KB, patch) 2011-07-28 00:56 PDT, Mihnea Ovidenie	ap: commit-queue-	Details Formatted Diff Diff
Patch 3 (8.59 KB, patch) 2011-07-29 07:20 PDT, Mihnea Ovidenie	no flags	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Mihnea Ovidenie

Comment 1 2011-07-26 05:59:11 PDT

Created attachment 101990 [details] Patch

Alexey Proskuryakov

Comment 2 2011-07-26 12:14:32 PDT

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.

Mihnea Ovidenie

Comment 3 2011-07-26 13:06:02 PDT

(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?

Alexey Proskuryakov

Comment 4 2011-07-27 11:58:16 PDT

- 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).

Mihnea Ovidenie

Comment 5 2011-07-28 00:56:53 PDT

Created attachment 102239 [details] Patch 2 Created a single html file instead of the script test.

Alexey Proskuryakov

Comment 6 2011-07-28 11:46:49 PDT

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.

Mihnea Ovidenie

Comment 7 2011-07-29 07:20:36 PDT

Created attachment 102359 [details] Patch 3

Mihnea Ovidenie

Comment 8 2011-07-29 07:23:25 PDT

(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.

Hajime Morrita

Comment 9 2011-08-01 03:13:46 PDT

Comment on attachment 102359 [details] Patch 3 Looks ok. The hard part is already done at Bug 20226.

WebKit Review Bot

Comment 10 2011-08-01 04:18:28 PDT

Comment on attachment 102359 [details] Patch 3 Clearing flags on attachment: 102359 Committed r92118: <http://trac.webkit.org/changeset/92118>

WebKit Review Bot

Comment 11 2011-08-01 04:18:34 PDT

All reviewed patches have been landed. Closing bug.

Mihnea Ovidenie

Comment 12 2011-09-01 07:12:28 PDT

*** Bug 48595 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component DOM

Assignee

Nobody

Reported

2011-07-26 05:48 PDT

Modified

2011-09-01 07:12 PDT History

CC List

5 users Show

URL

Keywords

Duplicates (1)

48595 View as bug list

Depends on

Blocks