PDF Viewer - Links should open on mouse up

PDF links should open on mouse up

Currently links in pdf open on mouse down. This doesn't
give user any chance to cancel the action by moving the mouse
without releasing the click.

This patch adds the fix to make sure pdf opens link on mouse up
and not on mouse down.

BUG=409753
Committed: https://crrev.com/408ab9799677a63fb8b83076f847e9f5302be813
Cr-Commit-Position: refs/heads/master@{#294564}

[Nikhil, 2014-09-06 07:19:29]

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

[Nikhil, 2014-09-06 07:21:46]

PTAL at my proposed solution to fix the issue that links in pdf open on mouse
down instead of mouse up.

If you've any review comments, then please let me know. I'll incorporate them.

Thanks!

[raymes, 2014-09-08 01:52:57]

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

https://codereview.chromium.org/553433002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1293: client_->FormTextFieldFocusChange(false);
Would it be better to store the information related to the mouse down in a
struct:

mouse_down_state_ = MouseDownState(area, target);

(this could be done outside of this if-statement)

Then in OnMouseUp, you could say:

if (mouse_down_state_ == MouseDownState(area, target)) {
  if (area == PDFiumPage::WEBLINK_AREA) {
    // open the link
  }
  // Possibly handle the scroll case too.
}

Then in OnMouseMove:

if (mouse_down_state_ != MouseDownState(area, target)) {
  mouse_down_state_ = MouseDownState();
}

What do you think?

[Nikhil, 2014-09-08 09:44:53]

Thanks for the quick review, Raymes! I've updated this CL.

PTAL. Thanks!

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

https://codereview.chromium.org/553433002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:1293: client_->FormTextFieldFocusChange(false);
On 2014/09/08 01:52:57, raymes wrote:
> Would it be better to store the information related to the mouse down in a
> struct:
> 
> mouse_down_state_ = MouseDownState(area, target);
> 
> (this could be done outside of this if-statement)
> 
> Then in OnMouseUp, you could say:
> 
> if (mouse_down_state_ == MouseDownState(area, target)) {
>   if (area == PDFiumPage::WEBLINK_AREA) {
>     // open the link
>   }
>   // Possibly handle the scroll case too.
> }
> 
> Then in OnMouseMove:
> 
> if (mouse_down_state_ != MouseDownState(area, target)) {
>   mouse_down_state_ = MouseDownState();
> }
> 
> What do you think?

Thanks for your suggestion, I agree that it could be useful to store mouse down
information. I've updated this CL accordingly. PTAL.

[Vitaly Buka (NO REVIEWS), 2014-09-08 16:45:51]

lgtm

[raymes, 2014-09-11 01:23:02]

lgtm

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

https://codereview.chromium.org/553433002/diff/20001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:1410: mouse_down_state_ = MouseDownState();
nit: Maybe just add a note of what's happening here:
// If the mouse moves away from where a mouse down happened, we clear
|mouse_down_state_|

[Nikhil, 2014-09-12 07:44:14]

Thanks for reviewing this! I've rebased the patch and made the change for nit
comment. I'm going to try to submit this now :)

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

https://codereview.chromium.org/553433002/diff/20001/pdf/pdfium/pdfium_engine...
pdf/pdfium/pdfium_engine.cc:1410: mouse_down_state_ = MouseDownState();
On 2014/09/11 01:23:01, raymes wrote:
> nit: Maybe just add a note of what's happening here:
> // If the mouse moves away from where a mouse down happened, we clear
> |mouse_down_state_|

Done.

[Nikhil, 2014-09-12 07:44:42]

The CQ bit was checked by n.bansal@samsung.com

[commit-bot: I haz the power, 2014-09-12 07:45:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/553433002/40001

[commit-bot: I haz the power, 2014-09-12 09:48:57]

Committed patchset #3 (id:40001) as 87d0fca4c48050fe6d55dea75c8bdfeb4f1921ae

[commit-bot: I haz the power, 2014-09-12 09:51:32]

Patchset 3 (id:??) landed as
https://crrev.com/408ab9799677a63fb8b83076f847e9f5302be813
Cr-Commit-Position: refs/heads/master@{#294564}