Fix server-side image map click location with "Open link in new tab"

Fix server-side image map click location with "Open link in new tab"

http://web.mit.edu/keithw/www/mapbug.html
Following a link that is part of a server-side image-map should include
the click coordinates in the query string, whether the user
(1) clicks normally, (2) middle-clicks, (3) selects "open link in new tab"
from the context menu.

But (3) choosing "open link in new tab" from the context menu does not
include the click location.

So I expanded ContextMenuClient::showContextMenu() function to include
Event. And then I made URL of context include mouse location information.

Last I made appendServerMapMousePosition function in HTMLAnchorElement to static.
for use this function instead of copying the code into two places

R=tkent@chromium.org,jinho.bang@samsung.com,limasdf@gmail.com, lazyboy@chromium.org
BUG=424721

[JakeYoon, 2016-02-04 14:14:34]

Please review @cbiesiner, @zino, @limasdf, @tkent

[JakeYoon, 2016-02-04 14:16:14]

Description was changed from

==========
Fix server-side image map click location with "Open link in new tab"

http://web.mit.edu/keithw/www/mapbug.html
Following a link that is part of a server-side image-map should include
the click coordinates in the query string, whether the user
(1) clicks normally, (2) middle-clicks, (3) selects "open link in new tab"
from the context menu.

But (3) choosing "open link in new tab" from the context menu does not
include the click location.

So I expanded ContextMenuClient::showContextMenu() function to include
Event. And then I made URL of context include mouse location information.
(When Event is mouse Event only)

R=cbiesiner@chromium.org,tkent@chromium.org,jinho.bang@samsung.com,limasdf@gm...
BUG=424721
==========

to

==========
Fix server-side image map click location with "Open link in new tab"

http://web.mit.edu/keithw/www/mapbug.html
Following a link that is part of a server-side image-map should include
the click coordinates in the query string, whether the user
(1) clicks normally, (2) middle-clicks, (3) selects "open link in new tab"
from the context menu.

But (3) choosing "open link in new tab" from the context menu does not
include the click location.

So I expanded ContextMenuClient::showContextMenu() function to include
Event. And then I made URL of context include mouse location information.
(When Event is mouse Event only)

R=cbiesiner@chromium.org,tkent@chromium.org,jinho.bang@samsung.com,limasdf@gm...
BUG=424721
==========

[JakeYoon, 2016-02-04 14:16:14]

yjaeseok@gmail.com changed reviewers:
+ cbiesinger@chromium.org
- cbiesiner@chromium.org

[cbiesinger, 2016-02-04 14:35:45]

I'm not an owner of this code, but it really seems like this function should be
shared between the two locations instead of copying the code into two places

[limasdf, 2016-02-04 14:55:51]

Couple of comments.

Thanks for the patch. We love it! However, There're a few rules and etiquette to
follow.

When asking someone to review it helps to set the expectations as to why you are
adding that person. 

Because you just need OWNER approval for the changed file, 
From patch set#1 of this CL, adding only tkent@ as a reviewer is enough. 
and CCing to cbiesinger@ or zino@ or limasdf@.(if you really want let us know
this CL).

If you add cbiesinger@ as a reviewer, he might think he has to review this CL,
and
it will review.  that means you use his time. When I look at this CL I get the
feeling that it isn't significant enough to warrant an extra set of eyes.

Thanks for the patch again!

[JakeYoon, 2016-02-04 15:24:23]

Description was changed from

==========
Fix server-side image map click location with "Open link in new tab"

http://web.mit.edu/keithw/www/mapbug.html
Following a link that is part of a server-side image-map should include
the click coordinates in the query string, whether the user
(1) clicks normally, (2) middle-clicks, (3) selects "open link in new tab"
from the context menu.

But (3) choosing "open link in new tab" from the context menu does not
include the click location.

So I expanded ContextMenuClient::showContextMenu() function to include
Event. And then I made URL of context include mouse location information.
(When Event is mouse Event only)

R=cbiesiner@chromium.org,tkent@chromium.org,jinho.bang@samsung.com,limasdf@gm...
BUG=424721
==========

to

==========
Fix server-side image map click location with "Open link in new tab"

http://web.mit.edu/keithw/www/mapbug.html
Following a link that is part of a server-side image-map should include
the click coordinates in the query string, whether the user
(1) clicks normally, (2) middle-clicks, (3) selects "open link in new tab"
from the context menu.

But (3) choosing "open link in new tab" from the context menu does not
include the click location.

So I expanded ContextMenuClient::showContextMenu() function to include
Event. And then I made URL of context include mouse location information.

Last I made appendServerMapMousePosition function in HTMLAnchorElement to
static.
for use this function instead of copying the code into two places

R=cbiesiner@chromium.org,tkent@chromium.org,jinho.bang@samsung.com,limasdf@gm...
BUG=424721
==========

[JakeYoon, 2016-02-04 21:05:10]

Thanks cbiesinger@, limasdf@.
I try to keep in mind.

I updated patch. !!

[tkent, 2016-02-05 02:05:47]

This change needs a test.

Do you support showContentMenu() call from
ContextMenuController::showContextMenuAtPoint()?

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:110: void
HTMLAnchorElement::appendServerMapMousePosition(StringBuilder& url, Event*
event)
The |event| argument should be |const Event*| or |const Event&|.

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ContextMenuController.cpp (right):

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ContextMenuController.cpp:176:
m_client->showContextMenu(m_contextMenu.get(), event);
|event| can be nullptr.  It causes a crash in appendServerMapMousePosition().

[JakeYoon, 2016-02-05 04:04:50]

Thanks. I will add a test for this change.
And I have some question.

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:110: void
HTMLAnchorElement::appendServerMapMousePosition(StringBuilder& url, Event*
event)
On 2016/02/05 02:05:47, tkent wrote:
> The |event| argument should be |const Event*| or |const Event&|.

Could you explain to me the reason that argument should be |const Event*| or
|const Event&|? 

In fact, I don't know difference argument |const *| and |*|

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ContextMenuController.cpp (right):

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ContextMenuController.cpp:176:
m_client->showContextMenu(m_contextMenu.get(), event);
On 2016/02/05 02:05:47, tkent wrote:
> |event| can be nullptr.  It causes a crash in appendServerMapMousePosition().

How do you think to add to check nullptr in appendServerMapMousePosition()
function?

[zino, 2016-02-05 05:47:34]

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ContextMenuController.cpp (right):

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ContextMenuController.cpp:176:
m_client->showContextMenu(m_contextMenu.get(), event);
On 2016/02/05 04:04:49, JakeYoon wrote:
> On 2016/02/05 02:05:47, tkent wrote:
> > |event| can be nullptr.  It causes a crash in
appendServerMapMousePosition().
> 
> How do you think to add to check nullptr in appendServerMapMousePosition()
> function?

I'm not owner but I think you could modify appendServerMapMousePosition() so
that the method takes mouse position instead of event object.
Then, you can get the current position from hit test result in
ContextMenuClientImpl::showContextMenu() without passing event object.

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ContextMenuClientImpl.cpp (right):

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:173: StringBuilder url;
Even if the target node is not image map link, This showContextMenu() method
will always be called when contextmenu event happen.
But we don't need to run these codes at all times.

[tkent, 2016-02-05 06:00:45]

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:110: void
HTMLAnchorElement::appendServerMapMousePosition(StringBuilder& url, Event*
event)
On 2016/02/05 at 04:04:49, JakeYoon wrote:
> On 2016/02/05 02:05:47, tkent wrote:
> > The |event| argument should be |const Event*| or |const Event&|.
> 
> Could you explain to me the reason that argument should be |const Event*| or
|const Event&|? 
> 
> In fact, I don't know difference argument |const *| and |*|

It ensures the Event object is not modified in the function.

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ContextMenuController.cpp (right):

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ContextMenuController.cpp:176:
m_client->showContextMenu(m_contextMenu.get(), event);
On 2016/02/05 at 04:04:49, JakeYoon wrote:
> On 2016/02/05 02:05:47, tkent wrote:
> > |event| can be nullptr.  It causes a crash in
appendServerMapMousePosition().
> 
> How do you think to add to check nullptr in appendServerMapMousePosition()
function?

Check it in ContextMenuClientImpl::showContextMenu is cleaner, I think.

[JakeYoon, 2016-02-05 07:28:34]

I change some logics for server-side image click location.

1) Change event parameter of appendServerMapMousePosition function |*| to |const
*|
to ensure the Event object is not modified in the function.

2) Change call appendServerMapMousePosition
when event is not nullptr and link is for image map only

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:110: void
HTMLAnchorElement::appendServerMapMousePosition(StringBuilder& url, Event*
event)
On 2016/02/05 06:00:45, tkent wrote:
> On 2016/02/05 at 04:04:49, JakeYoon wrote:
> > On 2016/02/05 02:05:47, tkent wrote:
> > > The |event| argument should be |const Event*| or |const Event&|.
> > 
> > Could you explain to me the reason that argument should be |const Event*| or
> |const Event&|? 
> > 
> > In fact, I don't know difference argument |const *| and |*|
> 
> It ensures the Event object is not modified in the function.

Okay I change params type, |*| to |const *|

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/ContextMenuController.cpp (right):

https://codereview.chromium.org/1668553003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/ContextMenuController.cpp:176:
m_client->showContextMenu(m_contextMenu.get(), event);
On 2016/02/05 06:00:45, tkent wrote:
> On 2016/02/05 at 04:04:49, JakeYoon wrote:
> > On 2016/02/05 02:05:47, tkent wrote:
> > > |event| can be nullptr.  It causes a crash in
> appendServerMapMousePosition().
> > 
> > How do you think to add to check nullptr in appendServerMapMousePosition()
> function?
> 
> Check it in ContextMenuClientImpl::showContextMenu is cleaner, I think.

Okay, I think so too

[limasdf, 2016-02-09 04:36:11]

Great. However, you still need a test.

[cbiesinger, 2016-02-12 20:40:27]

cbiesinger@chromium.org changed reviewers:
- cbiesinger@chromium.org

[JakeYoon, 2016-02-21 12:26:41]

Description was changed from

==========
Fix server-side image map click location with "Open link in new tab"

http://web.mit.edu/keithw/www/mapbug.html
Following a link that is part of a server-side image-map should include
the click coordinates in the query string, whether the user
(1) clicks normally, (2) middle-clicks, (3) selects "open link in new tab"
from the context menu.

But (3) choosing "open link in new tab" from the context menu does not
include the click location.

So I expanded ContextMenuClient::showContextMenu() function to include
Event. And then I made URL of context include mouse location information.

Last I made appendServerMapMousePosition function in HTMLAnchorElement to
static.
for use this function instead of copying the code into two places

R=cbiesiner@chromium.org,tkent@chromium.org,jinho.bang@samsung.com,limasdf@gm...
BUG=424721
==========

to

==========
Fix server-side image map click location with "Open link in new tab"

http://web.mit.edu/keithw/www/mapbug.html
Following a link that is part of a server-side image-map should include
the click coordinates in the query string, whether the user
(1) clicks normally, (2) middle-clicks, (3) selects "open link in new tab"
from the context menu.

But (3) choosing "open link in new tab" from the context menu does not
include the click location.

So I expanded ContextMenuClient::showContextMenu() function to include
Event. And then I made URL of context include mouse location information.

Last I made appendServerMapMousePosition function in HTMLAnchorElement to
static.
for use this function instead of copying the code into two places

R=tkent@chromium.org,jinho.bang@samsung.com,limasdf@gmail.com,
lazyboy@chromium.org
BUG=424721
==========

[JakeYoon, 2016-02-21 12:26:41]

yjaeseok@gmail.com changed reviewers:
+ lazyboy@chromium.org

[JakeYoon, 2016-02-21 12:35:23]

Finally, I added test case for this issue.
But I have some question,

#1. I just added test case for browser actual test only.
Is it Okay?

#2. In browser, actual mouse event are x = 20, y = 20.
But mouse event of image map are x = 12, y = 12.
(This size is always 8 smaller than actual mouse event.)
I think 8 is offset of browser.
So, I hard coding const 8.
Is it Okay?

Thanks

[tkent, 2016-02-22 02:06:19]

On 2016/02/21 at 12:35:23, yjaeseok wrote:
> Finally, I added test case for this issue.
> But I have some question,
> 
> #1. I just added test case for browser actual test only.
> Is it Okay?

Not good.  Testing code and tested code should be put nearby.
If I were you, I would move WebContenxtMenuData setup code out from
ContextMenuClientImpl::showContextMenu, and make a unit test for it.

> #2. In browser, actual mouse event are x = 20, y = 20.
> But mouse event of image map are x = 12, y = 12.
> (This size is always 8 smaller than actual mouse event.)
> I think 8 is offset of browser.
> So, I hard coding const 8.
> Is it Okay?

I think it's the default margin of <body>.  <body style="margin:0px"> resolves
it.

[JakeYoon, 2016-03-07 14:43:45]

I updated test to remove margin of <body>.

But, I do not add unit test for showContextMenu() func.
@tkent Could you advise to me how to add unit test for this function?...

I tried to call showContextMenu in Test System.
But... I cannot access ContextMenuClientImpl object.

So I would like to get some advise.
Thanks!

[tkent, 2016-03-07 23:50:02]

On 2016/03/07 at 14:43:45, yjaeseok wrote:
> I updated test to remove margin of <body>.
> 
> But, I do not add unit test for showContextMenu() func.
> @tkent Could you advise to me how to add unit test for this function?...
> 
> I tried to call showContextMenu in Test System.
> But... I cannot access ContextMenuClientImpl object.

I wrote:
> I would move WebContenxtMenuData setup code out from
ContextMenuClientImpl::showContextMenu, and make a unit test for it.

For example, adding a |static| function WebContextMenuData::setupMenuData(const
HitTestResult&, WebContextMenuData&).  You don't need to create a
ContextMenuClientImpl object in such approach.

[JakeYoon, 2016-03-13 14:32:35]

Hello, @tkent.

I added unit test for HTMLAnchorElement.
And I added static setupMenuData function like you mension.

But, I didn't add unit test for setupMenuData,
because I don't know how to make HitTestResult, Event params.

I tried to make params. But... I need your help!..!
Thanks

[tkent, 2016-03-14 00:06:34]

On 2016/03/13 at 14:32:35, yjaeseok wrote:
> because I don't know how to make HitTestResult, Event params.

HitTestResult doesn't have normal |new|.  However you should be able to make it
on stack.

HitTestResult hitTestResult;
hitTestResult.setInnerNode(...);
ContextMenuImpl::setupContextMenuData(hitTestResult, ...)

MouseEvent has setTarget() and setAbsolutePosition().

The HTMLAnchorElement logic needs layoutObject of an element.  So you'll need
DummyPageHolder too.

https://codereview.chromium.org/1668553003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp (right):

https://codereview.chromium.org/1668553003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLAnchorElement.cpp:155: void
HTMLAnchorElement::appendServerMapMousePosition(StringBuilder& url, IntPoint&
clampedPoint)
IntPoint& -> const IntPoint&

https://codereview.chromium.org/1668553003/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLAnchorElementTest.cpp (right):

https://codereview.chromium.org/1668553003/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLAnchorElementTest.cpp:1: // Copyright
2014 The Chromium Authors. All rights reserved.
2014 -> 2016