Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2166193002:
Handle ctrl + shift + left click on links in PDF. (Closed)
|
Created:
4 years, 5 months ago by jaepark Modified:
4 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, arv+watch_chromium.org, extensions-reviews_chromium.org, dsinclair Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle ctrl + shift + left click on links in PDF.
Ctrl + shift + left (or shift + middle) click on links should open a new
foreground tab.
BUG=630075
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/1b44ac414765de1d882f295006cfe2687872320b
Cr-Commit-Position: refs/heads/master@{#407840}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Handle ctrl + shift + left click on links in PDF. #
Total comments: 1
Patch Set 3 : Handle ctrl + shift + left click on links in PDF. #
Total comments: 10
Patch Set 4 : Handle ctrl + shift + left click on links in PDF. #
Total comments: 8
Patch Set 5 : Handle ctrl + shift + left click on links in PDF. #
Messages
Total messages: 64 (44 generated)
Description was changed from ========== Handle ctrl + shift + left click on links in PDF. Ctrl + shift + left (or shift + middle) click on links should open a new foreground tab. BUG=630075 ========== to ========== Handle ctrl + shift + left click on links in PDF. Ctrl + shift + left (or shift + middle) click on links should open a new foreground tab. BUG=630075 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by jaepark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jaepark@google.com changed reviewers: + thestig@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
There are red trybots. You may also be interested in ui::DispositionFromClick(). https://codereview.chromium.org/2166193002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/navigator.js (right): https://codereview.chromium.org/2166193002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/navigator.js:30: * Represents options when navigating to a new url. You still need a comment to mention where the C++ version is and how the two needs to stay in sync. https://codereview.chromium.org/2166193002/diff/1/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/2166193002/diff/1/pdf/pdf_engine.h#newcode83 pdf/pdf_engine.h:83: virtual void NavigateTo(const std::string& url, int option) = 0; Can |option| be of type NavigateOption? https://codereview.chromium.org/2166193002/diff/1/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2166193002/diff/1/pdf/pdfium/pdfium_engine.cc... pdf/pdfium/pdfium_engine.cc:1646: NavigateOption option = OPEN_IN_CURRENT_TAB; No need to initialize since all the cases are covered below. Declare this closer to when it's used. https://codereview.chromium.org/2166193002/diff/1/pdf/pdfium/pdfium_engine.cc... pdf/pdfium/pdfium_engine.cc:1647: bool ctrl = !!(event.GetModifiers() & kDefaultKeyModifier) || Maybe control can stay as open_in_new_tab?
dsinclair@chromium.org changed reviewers: + dsinclair@chromium.org
https://codereview.chromium.org/2166193002/diff/1/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2166193002/diff/1/pdf/pdfium/pdfium_engine.h#... pdf/pdfium/pdfium_engine.h:52: enum NavigateOption { Can this be an enum class instead of a plain enum?
The CQ bit was checked by jaepark@google.com to run a CQ dry run
The CQ bit was unchecked by jaepark@google.com
The CQ bit was checked by jaepark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jaepark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by jaepark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Fixed red trybots. I looked into ui::DispositionFromClick(), but the expected behavior was different (such as behavior of left click using alt modifier). https://codereview.chromium.org/2166193002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/navigator.js (right): https://codereview.chromium.org/2166193002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/navigator.js:30: * Represents options when navigating to a new url. On 2016/07/21 05:34:28, Lei Zhang wrote: > You still need a comment to mention where the C++ version is and how the two > needs to stay in sync. Done. https://codereview.chromium.org/2166193002/diff/1/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/2166193002/diff/1/pdf/pdf_engine.h#newcode83 pdf/pdf_engine.h:83: virtual void NavigateTo(const std::string& url, int option) = 0; On 2016/07/21 05:34:28, Lei Zhang wrote: > Can |option| be of type NavigateOption? Done. https://codereview.chromium.org/2166193002/diff/1/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2166193002/diff/1/pdf/pdfium/pdfium_engine.cc... pdf/pdfium/pdfium_engine.cc:1646: NavigateOption option = OPEN_IN_CURRENT_TAB; On 2016/07/21 05:34:28, Lei Zhang wrote: > No need to initialize since all the cases are covered below. Declare this closer > to when it's used. Done. https://codereview.chromium.org/2166193002/diff/1/pdf/pdfium/pdfium_engine.cc... pdf/pdfium/pdfium_engine.cc:1647: bool ctrl = !!(event.GetModifiers() & kDefaultKeyModifier) || On 2016/07/21 05:34:28, Lei Zhang wrote: > Maybe control can stay as open_in_new_tab? Done. https://codereview.chromium.org/2166193002/diff/1/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2166193002/diff/1/pdf/pdfium/pdfium_engine.h#... pdf/pdfium/pdfium_engine.h:52: enum NavigateOption { On 2016/07/21 13:34:05, dsinclair wrote: > Can this be an enum class instead of a plain enum? Done. https://codereview.chromium.org/2166193002/diff/40001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2166193002/diff/40001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:985: message.Set(kJSNavigateOption, pp::Var(static_cast<int>(option))); To pass enum class from Plugin to Page, I used static_cast<int> since pp::Var doesn't support Enum Classes.
On 2016/07/21 22:34:33, jaepark wrote: > Fixed red trybots. > > I looked into ui::DispositionFromClick(), but the expected behavior was > different (such as behavior of left click using alt modifier). How is it different? On my Linux desktop, alt + clicking moves windows around because that's what my window manager is set to, and the window manager has priority over applications. But on Windows, alt + clicking does a download.
On 2016/07/21 23:00:53, Lei Zhang wrote: > How is it different? On my Linux desktop, alt + clicking moves windows around > because that's what my window manager is set to, and the window manager has > priority over applications. But on Windows, alt + clicking does a download. On my Linux desktop, alt + left clicking on a link in a web page does not do anything, and on my Chrome OS, it's the same as right clicking.
On 2016/07/21 23:00:53, Lei Zhang wrote:
> How is it different? On my Linux desktop, alt + clicking moves windows around
> because that's what my window manager is set to, and the window manager has
> priority over applications. But on Windows, alt + clicking does a download.
I'm trying to use ui::DispositionFromClick and WindowOpenDisposition, but it
seems like I can't include "ui/base/window_open_disposition.h" from
pdf/pdf_engine.h.
Presubmit gives me the following error.
You added one or more #includes that violate checkdeps rules.
pdf/pdf_engine.h
Illegal include: "ui/base/window_open_disposition.h"
Because of no rule applying.
I'm not sure if this is not allowed, or we just have to add a rule.
On 2016/07/22 05:35:05, jaepark wrote: > You added one or more #includes that violate checkdeps rules. > pdf/pdf_engine.h > Illegal include: "ui/base/window_open_disposition.h" > Because of no rule applying. > > I'm not sure if this is not allowed, or we just have to add a rule. You need to update pdf/DEPS and get ui/OWNERS approval.
The CQ bit was checked by jaepark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jaepark@google.com
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by jaepark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Modified to use ui::DispositionFromClick() and WindowOpenDisposition enum in ui/base/window_open_disposition.h.
https://codereview.chromium.org/2166193002/diff/80001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2166193002/diff/80001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:800: blink::WebInputEvent::MetaKey | blink::WebInputEvent::ShiftKey; You can add a helper function to return the right modifier per platform and use it in the test above as well. Here, you'd need to do: int modifiers = blink::WebInputEvent::ShiftKey | GetPlatformModifierKey(); https://codereview.chromium.org/2166193002/diff/80001/chrome/browser/resource... File chrome/browser/resources/pdf/navigator.js (right): https://codereview.chromium.org/2166193002/diff/80001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:77: // TODO (jaepark): It should navigate in a new window when It's weird that the first two cases fall through to the SAVE_TO_DISK case. It reads as though current tab will result in a save to disk, which is unexpected. https://codereview.chromium.org/2166193002/diff/80001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (left): https://codereview.chromium.org/2166193002/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1646: bool open_in_new_tab = !!(event.GetModifiers() & kDefaultKeyModifier) || I think this is the only use of |kDefaultKeyModifier|, so if you are removing it, remove |kDefaultKeyModifier| as well. https://codereview.chromium.org/2166193002/diff/80001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2166193002/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1647: !!(event.GetModifiers() & PP_INPUTEVENT_MODIFIER_MIDDLEBUTTONDOWN); BTW, GetModifiers() looks like it might be somewhat expensive to call. Maybe save the result to a local variable and use it all over? https://codereview.chromium.org/2166193002/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1656: ui::DispositionFromClick(middle_button, alt_key, ctrl_key, meta_key, Previously, pdf/ was only using values from headers in ui/, but now it is actually calling into ui/ code, so pdf/BUILD.gn and pdf/pdf.gyp need to explicitly depend on ui/. i.e. "//ui/base" in GN.
https://codereview.chromium.org/2166193002/diff/80001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2166193002/diff/80001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:800: blink::WebInputEvent::MetaKey | blink::WebInputEvent::ShiftKey; On 2016/07/23 00:39:38, Lei Zhang wrote: > You can add a helper function to return the right modifier per platform and use > it in the test above as well. Here, you'd need to do: > > int modifiers = blink::WebInputEvent::ShiftKey | GetPlatformModifierKey(); Done. I made a const variable for the default modifier. https://codereview.chromium.org/2166193002/diff/80001/chrome/browser/resource... File chrome/browser/resources/pdf/navigator.js (right): https://codereview.chromium.org/2166193002/diff/80001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:77: // TODO (jaepark): It should navigate in a new window when On 2016/07/23 00:39:38, Lei Zhang wrote: > It's weird that the first two cases fall through to the SAVE_TO_DISK case. It > reads as though current tab will result in a save to disk, which is unexpected. Done. I've changed it not to fall through in each cases. https://codereview.chromium.org/2166193002/diff/80001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (left): https://codereview.chromium.org/2166193002/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1646: bool open_in_new_tab = !!(event.GetModifiers() & kDefaultKeyModifier) || On 2016/07/23 00:39:38, Lei Zhang wrote: > I think this is the only use of |kDefaultKeyModifier|, so if you are removing > it, remove |kDefaultKeyModifier| as well. Done. https://codereview.chromium.org/2166193002/diff/80001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2166193002/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1647: !!(event.GetModifiers() & PP_INPUTEVENT_MODIFIER_MIDDLEBUTTONDOWN); On 2016/07/23 00:39:38, Lei Zhang wrote: > BTW, GetModifiers() looks like it might be somewhat expensive to call. Maybe > save the result to a local variable and use it all over? Done. https://codereview.chromium.org/2166193002/diff/80001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1656: ui::DispositionFromClick(middle_button, alt_key, ctrl_key, meta_key, On 2016/07/23 00:39:38, Lei Zhang wrote: > Previously, pdf/ was only using values from headers in ui/, but now it is > actually calling into ui/ code, so pdf/BUILD.gn and pdf/pdf.gyp need to > explicitly depend on ui/. i.e. "//ui/base" in GN. Done.
The CQ bit was checked by jaepark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2166193002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/pdf/navigator.js (right): https://codereview.chromium.org/2166193002/diff/100001/chrome/browser/resourc... chrome/browser/resources/pdf/navigator.js:49: * @param {boolean} newTab Whether to perform the navigation in a new tab or Doc is out of date. https://codereview.chromium.org/2166193002/diff/100001/chrome/browser/resourc... chrome/browser/resources/pdf/navigator.js:87: // TODO (jaepark): It should navigate in a new window when no space after "TODO" https://codereview.chromium.org/2166193002/diff/100001/chrome/browser/resourc... chrome/browser/resources/pdf/navigator.js:89: break; Previously these fell through to SAVE_TO_DISK, which then took the CURRENT_TAB behavior. The actual behavior was fine, but the way it was written looked a bit weird. Now these are no-ops, which seem slightly worse than if we at elast open the link. Can we put paste the CURRENT_TAB code here so these cases at least do something? https://codereview.chromium.org/2166193002/diff/100001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2166193002/diff/100001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1651: bool meta_key = !!(modifiers & PP_INPUTEVENT_MODIFIER_METAKEY); wonky indent
Patchset #5 (id:120001) has been deleted
https://codereview.chromium.org/2166193002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/pdf/navigator.js (right): https://codereview.chromium.org/2166193002/diff/100001/chrome/browser/resourc... chrome/browser/resources/pdf/navigator.js:49: * @param {boolean} newTab Whether to perform the navigation in a new tab or On 2016/07/25 22:10:02, Lei Zhang wrote: > Doc is out of date. Done. https://codereview.chromium.org/2166193002/diff/100001/chrome/browser/resourc... chrome/browser/resources/pdf/navigator.js:87: // TODO (jaepark): It should navigate in a new window when On 2016/07/25 22:10:02, Lei Zhang wrote: > no space after "TODO" Done. https://codereview.chromium.org/2166193002/diff/100001/chrome/browser/resourc... chrome/browser/resources/pdf/navigator.js:89: break; On 2016/07/25 22:10:02, Lei Zhang wrote: > Previously these fell through to SAVE_TO_DISK, which then took the CURRENT_TAB > behavior. The actual behavior was fine, but the way it was written looked a bit > weird. Now these are no-ops, which seem slightly worse than if we at elast open > the link. Can we put paste the CURRENT_TAB code here so these cases at least do > something? Done. https://codereview.chromium.org/2166193002/diff/100001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2166193002/diff/100001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1651: bool meta_key = !!(modifiers & PP_INPUTEVENT_MODIFIER_METAKEY); On 2016/07/25 22:10:02, Lei Zhang wrote: > wonky indent Done.
The CQ bit was checked by jaepark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jaepark@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
jaepark@google.com changed reviewers: + sky@chromium.org
+sky to review ui/base dependency.
On 2016/07/26 01:10:39, jaepark wrote: > +sky to review ui/base dependency. Historical context: pdf/ used to be its own DLL, so its dependencies were extremely limited. That's no longer the case and the pdf/ code just goes into the chrome target these days.
LGTM
The CQ bit was checked by jaepark@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Handle ctrl + shift + left click on links in PDF. Ctrl + shift + left (or shift + middle) click on links should open a new foreground tab. BUG=630075 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Handle ctrl + shift + left click on links in PDF. Ctrl + shift + left (or shift + middle) click on links should open a new foreground tab. BUG=630075 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/1b44ac414765de1d882f295006cfe2687872320b Cr-Commit-Position: refs/heads/master@{#407840} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/1b44ac414765de1d882f295006cfe2687872320b Cr-Commit-Position: refs/heads/master@{#407840} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
