[Gnash-commit] [patch #8904] Patches to fix special stroke size handling in `MovieClip.lineStyle()`

[Nutchanon Wetchasit]

URL:
<http://savannah.gnu.org/patch/?8904>

Summary: Patches to fix special stroke size handling in
`MovieClip.lineStyle()`
Project: Gnash - The GNU Flash player
Submitted by: nachanon
Submitted on: Fri 05 Feb 2016 01:42:25 PM ICT
Category: core
Priority: 5 - Normal
Status: None
Privacy: Public
Assigned to: None
Originator Email:
Open/Closed: Open
Discussion Lock: Any

_______________________________________________________

Details:

Current Gnash's implementation of `MovieClip.lineStyle()` has an incorrect
handling of `thickness` argument when it have special value like
`undefined` or `Infinity`, a.k.a. bug #45731.

Patch 1 of 2:
See the attached `0001_reset-line-style-on-undefined-stroke.patch`.

Original code of `MovieCLip.lineStyle()` implementation already resets
line style when the method was called without any argument
<http://git.savannah.gnu.org/cgit/gnash.git/tree/libcore/asobj/MovieClip_as.cpp?id=02ec1e61bf5b08a5db29472c268a2e448ac8772a#n1497>,
but it doesn't
reset line style when `thickness` argument is specified as `undefined`,
and left it to number conversion code's interpretation
<http://git.savannah.gnu.org/cgit/gnash.git/tree/libcore/asobj/MovieClip_as.cpp?id=02ec1e61bf5b08a5db29472c268a2e448ac8772a#n1610>,
which results
in hairline (width 0) stroke.

This patch extends the condition check
<http://git.savannah.gnu.org/cgit/gnash.git/tree/libcore/asobj/MovieClip_as.cpp?id=02ec1e61bf5b08a5db29472c268a2e448ac8772a#n1497>
to cover the case where
`thickness` argument is specified, but as `undefined`, which has
essentially the same meaning; partially fixing bug #45731.

Gnash: 0.8.11dev (patched against git 02ec1e6 3-Feb-2016)
System: Debian GNU/Linux 7.0 Wheezy i386




_______________________________________________________

File Attachments:


-------------------------------------------------------
Date: Fri 05 Feb 2016 01:42:25 PM ICT Name:
0001_reset-line-style-on-undefined-stroke.patch Size: 698B By: nachanon
Patch to make `MovieClip.lineStyle()` disable line drawing on `undefined` line
thickness
<http://savannah.gnu.org/patch/download.php?file_id=36250>

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?8904>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Update of patch #8904 (project gnash):

Status: None => Ready For Test
Assigned to: None => strk

_______________________________________________________

Follow-up Comment #1:

Pushed as commit 5dd3e016bef83cc0368fa1fdca8ffcb58d811e89

We have some drawing API tests in testsuite/misc-ming.all/DrawingApiTest.as,
and the corresponding DrawingApiTestRunner.cpp

I hope you'll find it easy to add tests to

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?8904>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Nutchanon Wetchasit]

Follow-up Comment #2, patch #8904 (project gnash):

Patch 2 of 2:
See the attached `0002_treat-impossible-line-size-as-zero.patch`.

Current Gnash's implementation `MovieClip.lineStyle()` treats line thickness
of `Infinity` as 255 <https://savannah.gnu.org/bugs/?45731#comment3>, while
Flash Player treats it (together as other
impossible line size like negative number, and NaN) as 0.

This patch adds a special-case check to `MovieClip.lineStyle()` which
explicitly set the line size to zero (hairline stroke) when `Infinity`,
`-Infinity` or `NaN` line size value is encountered.

Automated tests will follow as a separate patch entry.

Note:
* In original code, behavior of `NaN` line size is correct but it depends on
order of comparison used inside `clamp()` function
<http://git.savannah.gnu.org/cgit/gnash.git/tree/libbase/GnashNumeric.h?id=051aa9c34b69e1a0a1d2d75e3cf1db07fcea4006#n75>.
Explicit `NaN` check is added to ensure correct behavior regardless of how
`clamp()` was implemented.
* I'm not sure why `clamp()` function was used as `<float>` rather than
`<double>` despite `toNumber()` returning `double`
<http://git.savannah.gnu.org/cgit/gnash.git/tree/libcore/vm/VM.h?id=051aa9c34b69e1a0a1d2d75e3cf1db07fcea4006#n408>
and `pixelsToTwips()` accepting `double` argument
<http://git.savannah.gnu.org/cgit/gnash.git/tree/libbase/GnashNumeric.h?id=051aa9c34b69e1a0a1d2d75e3cf1db07fcea4006#n143>.
I left this part intact as nearby code seems to do similar thing.

Gnash: 0.8.11dev (patched against git 051aa9c 15-Feb-2016)
System: Debian GNU/Linux 7.0 Wheezy i386


(file #36380)
_______________________________________________________

Additional Item Attachment:

File name: 0002_treat-impossible-line-size-as-zero.patch Size:1 KB


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?8904>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Follow-up Comment #3, patch #8904 (project gnash):

Patch 2 pushed as commit 982270fff22b80f7d3872f3696f4636053894ca4


_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?8904>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/

[Sandro Santilli]

Follow-up Comment #4, patch #8904 (project gnash):

Chances to see tests for testsuite/misc-ming.all/DrawingApiTest.as, and the
corresponding DrawingApiTestRunner.cpp ?

_______________________________________________________

Reply to this item at:

<http://savannah.gnu.org/patch/?8904>

_______________________________________________
Message sent via/by Savannah
http://savannah.gnu.org/