PDF Viewer - Show context menu for links on right click

PDF Viewer - Show context menu for links on right click

Currently context menu for page is shown when right clicked
on links in the pdf. This happens because an empty string is
returned as url to ContextMenuClientImpl::showContextMenu()
from plugin.

The reason for above is that GetLinkAtPosition() isn't passing
a point in page coordinates to GetCharIndex() and GetCharIndex()
expects the point to be in page coordinate. This means that when
the document is zoomed or scrolled and right click is done then
it doesn't work.

This patch updates the point passed to GetCharIndex() to be in
page coordinates by factoring in current position and zoom level.

BUG=148665
Committed: https://crrev.com/47ddc0fca1e5944965866ac55844515a28d2d022
Cr-Commit-Position: refs/heads/master@{#294256}

[Nikhil, 2014-09-06 10:06:39]

n.bansal@samsung.com changed reviewers:
+ raymes@chromium.org, thestig@chromium.org, vitalybuka@chromium.org

[Nikhil, 2014-09-06 10:17:16]

PTAL at my proposed solution to show context menu for links when right click is
done on links inside the pdf. If you've any review comments, then please let me
know. I'll make the changes accordingly.

A few details from my side -

[*] If this was ever working earlier, then maybe we should also check what broke
it.

[*] Please note that I see an error in console when I right click to see context
menu. Same error is also displayed when I try to follow steps of reproduction
for issue 404063 (https://code.google.com/p/chromium/issues/detail?id=404063)

[*] Error mentioned in above point is -
ERROR:context_menu_params_builder.cc(64)] Creating a ContextMenuParams for a
node that has a linkurl but is not an ElementNode or does not have anancestor
that is a link.

PTAL. Thanks!

[raymes, 2014-09-08 01:25:38]

It looks like the root of the problem here is that GetLinkAtPosition() isn't
passing a point in page coordinates to GetCharIndex(). This means that when the
document is scrolled all the way to the top and is at 100% zoom, everything
works fine but as soon as it is zoomed or scrolled, right-click breaks.

Could you update the CL description to say something like this?

https://codereview.chromium.org/553453002/diff/1/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/553453002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1248: PDFiumPage::Area PDFiumEngine::GetCharIndex(
It looks like this function expects |point| to be in page coordinates. I think
it's only called from 3 places:
1) From GetCharIndex which provides |point| in page coordinates.
2) Recursively, which also provides |point| in page coordinates.
3) From GetLinkAtPosition which seems not to provide |point| in page
coordinates.

I think the right fix is to change GetLinkAtPosition to provide |point| in page
coordinates. Currently the transformation to the point is being applied multiple
times if this function recurses which I think is incorrect.

[raymes, 2014-09-08 01:26:15]

BTW, thanks for looking at this! It's a pretty nasty bug and no one has fixed it
in years!

[Vitaly Buka (NO REVIEWS), 2014-09-08 01:33:26]

lgtm

[Nikhil, 2014-09-08 05:59:13]

Thanks for the quick review! I've updated both the patch and the CL description.

PTAL. Thanks.

https://codereview.chromium.org/553453002/diff/1/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/553453002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1248: PDFiumPage::Area PDFiumEngine::GetCharIndex(
On 2014/09/08 01:25:38, raymes wrote:
> It looks like this function expects |point| to be in page coordinates. I think
> it's only called from 3 places:
> 1) From GetCharIndex which provides |point| in page coordinates.
> 2) Recursively, which also provides |point| in page coordinates.
> 3) From GetLinkAtPosition which seems not to provide |point| in page
> coordinates.
> 
> I think the right fix is to change GetLinkAtPosition to provide |point| in
page
> coordinates. Currently the transformation to the point is being applied
multiple
> times if this function recurses which I think is incorrect.
> 

Done.

[Nikhil, 2014-09-08 06:01:49]

I forgot to update that I tested this manually and context menu is coming
properly. I still see the error that I mentioned in my earlier comment
(https://codereview.chromium.org/553453002#msg2). But, I think that is a
separate issue.

[Nikhil, 2014-09-10 05:55:03]

raymes@, thestig@ - Could you please take a look at this for OWNERS review?
Thanks!

[Lei Zhang, 2014-09-10 19:18:51]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2014-09-10 19:18:52]

lgtm

[commit-bot: I haz the power, 2014-09-10 19:20:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/n.bansal@samsung.com/553453002/20001

[commit-bot: I haz the power, 2014-09-10 19:33:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 19:33:07]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[Lei Zhang, 2014-09-10 21:29:33]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-09-10 21:31:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/n.bansal@samsung.com/553453002/20001

[raymes, 2014-09-10 23:20:50]

On 2014/09/10 21:31:36, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-status.appspot.com/cq/n.bansal@samsung.com/553453002/20001

lgtm

[commit-bot: I haz the power, 2014-09-10 23:26:08]

Committed patchset #2 (id:20001) as 97ff1b6eec06722188945c7446c161da4f249a26

[commit-bot: I haz the power, 2014-09-10 23:29:05]

Patchset 2 (id:??) landed as
https://crrev.com/47ddc0fca1e5944965866ac55844515a28d2d022
Cr-Commit-Position: refs/heads/master@{#294256}