Re: [PATCH 0/3] mshtml: Image map support

[Alex Henrie]

2015-08-04 8:36 GMT-06:00 Alex Henrie :
> Hi,
>
> Could I get some feedback on the following changes before I submit them
> to wine-patches?

Shoot, I just sent these to wine-patches by accident instead of
wine-devel. Feedback would still be appreciated.

-Alex

[PATCH 0/3] mshtml: Image map support

[Jacek Caban]

Hi Alex,

On 08/04/15 16:39, Alex Henrie wrote:
> 2015-08-04 8:36 GMT-06:00 Alex Henrie :
> > Hi,
> >
> > Could I get some feedback on the following changes before I submit them
> > to wine-patches?
>
> Shoot, I just sent these to wine-patches by accident instead of
> wine-devel. Feedback would still be appreciated.

First two patches are fine on wine-patches. They look good to me.

The third patch needs more work. It would be nice to have event helper
functions more generic as in if we want to use them for yet another
element type, we wouldn't need to add another argument. Maybe callers
should call appropriate GetHref and GetTarget and pass strings to the
helper?

Also, it would be nice to have a trivial test case for area element. See
dom.c for an example. Just a call to test_elem_type for an area element
should be enough for the stub.

Thanks,
Jacek

[PATCH 0/3] mshtml: Image map support

[Alex Henrie]

2015-08-04 11:00 GMT-06:00 Jacek Caban :
> The third patch needs more work. It would be nice to have event helper
> functions more generic as in if we want to use them for yet another
> element type, we wouldn't need to add another argument. Maybe callers
> should call appropriate GetHref and GetTarget and pass strings to the
> helper?

I pushed a new version of patch 3 to
https://github.com/alexhenrie/wine/commits/master - this version
shares less code between <a> and <area>, but it will be easier to use
the code in other places now. Any thoughts?
> Also, it would be nice to have a trivial test case for area element. See
> dom.c for an example. Just a call to test_elem_type for an area element
> should be enough for the stub.

I would add this, but I don't know where exactly to insert the test.
Would you be willing to write and submit this test, or at least tell
me which function in dom.c to add it to?

Even so, that test would have gone with patch 2, and Alexandre already
accepted that patch without tests, so we shouldn't have to wait to
submit patch 3 which definitely doesn't need new tests.

-Alex

[PATCH 0/3] mshtml: Image map support

[Jacek Caban]

On 08/06/15 06:09, Alex Henrie wrote:
> 2015-08-04 11:00 GMT-06:00 Jacek Caban :
> > The third patch needs more work. It would be nice to have event helper
> > functions more generic as in if we want to use them for yet another
> > element type, we wouldn't need to add another argument. Maybe callers
> > should call appropriate GetHref and GetTarget and pass strings to the
> > helper?
>
> I pushed a new version of patch 3 to
> https://github.com/alexhenrie/wine/commits/master - this version
> shares less code between <a> and <area>, but it will be easier to use
> the code in other places now. Any thoughts?

It looks much better, thanks. However, I think we shouldn't call
GetHref() and GetTarget() for each event. It means that each element
handler needs to check event id instead of moving that to
handle_link_events (even in cost of sharing a bit less of code).

Also, please avoid moving eventid_t to mshtml_private.h. You may include
htmlevent.h in files that need it.

Cheers,
Jaeck

[PATCH 0/3] mshtml: Image map support

[Alex Henrie]

2015-08-06 6:58 GMT-06:00 Jacek Caban :
> It looks much better, thanks. However, I think we shouldn't call
> GetHref() and GetTarget() for each event. It means that each element
> handler needs to check event id instead of moving that to
> handle_link_events (even in cost of sharing a bit less of code).
>
> Also, please avoid moving eventid_t to mshtml_private.h. You may include
> htmlevent.h in files that need it.

Good points, thanks. I have revised the commit at
https://github.com/alexhenrie/wine/commits/master again to take into
account your feedback.

-Alex