Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2149153003:
Links in PDF should be opened in new background tabs when ctrl + left clicked. (Closed)
|
Created:
4 years, 5 months ago by jaepark Modified:
4 years, 5 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. |
DescriptionLinks in PDF should be opened in new background tabs when ctrl + left clicked.
BUG=628054
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/b8c0e4533fbd48c760be908803faded4b93e396d
Cr-Commit-Position: refs/heads/master@{#406058}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Links in PDF should be opened in new background tabs when ctrl + left clicked. #
Total comments: 2
Patch Set 3 : Links in PDF should be opened in new background tabs when ctrl + left clicked. #
Total comments: 2
Patch Set 4 : Links in PDF should be opened in new background tabs when ctrl + left clicked. #Patch Set 5 : Links in PDF should be opened in new background tabs when ctrl + left clicked. #
Total comments: 2
Messages
Total messages: 30 (15 generated)
Description was changed from ========== Links in PDF should be opened in new background tabs when ctrl + left clicked. BUG=628054 ========== to ========== Links in PDF should be opened in new background tabs when ctrl + left clicked. BUG=628054 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
jaepark@google.com changed reviewers: + raymes@chromium.org, thestig@chromium.org
In normal web pages, links are opened in new background tabs when ctrl + mouse left clicked. PDF should have the same behavior. https://codereview.chromium.org/2149153003/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2149153003/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:68: window.open(url); I wasn't able to find a way to create a new background tab without using chrome.tabs. Any ideas?
https://codereview.chromium.org/2149153003/diff/1/chrome/browser/pdf/pdf_exte... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2149153003/diff/1/chrome/browser/pdf/pdf_exte... chrome/browser/pdf/pdf_extension_test.cc:629: blink::WebMouseEvent::ButtonMiddle, point); Do you want to add another test for ctrl + left clicking? The two tests can probably share the most of this function. https://codereview.chromium.org/2149153003/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2149153003/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:68: window.open(url); On 2016/07/14 22:52:39, jaepark wrote: > I wasn't able to find a way to create a new background tab without using > chrome.tabs. Any ideas? I think this is just best effort.
https://codereview.chromium.org/2149153003/diff/1/chrome/browser/pdf/pdf_exte... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2149153003/diff/1/chrome/browser/pdf/pdf_exte... chrome/browser/pdf/pdf_extension_test.cc:629: blink::WebMouseEvent::ButtonMiddle, point); On 2016/07/14 22:57:52, Lei Zhang wrote: > Do you want to add another test for ctrl + left clicking? The two tests can > probably share the most of this function. Done. https://codereview.chromium.org/2149153003/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2149153003/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:68: window.open(url); On 2016/07/14 22:57:52, Lei Zhang wrote: > On 2016/07/14 22:52:39, jaepark wrote: > > I wasn't able to find a way to create a new background tab without using > > chrome.tabs. Any ideas? > > I think this is just best effort. Acknowledged.
https://codereview.chromium.org/2149153003/diff/20001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2149153003/diff/20001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:237: gfx::Point& screen_coord) { Can't pass by non-const ref. Pass in a pointer instead? Or just pass in a single pointer as an in-out parameter.
https://codereview.chromium.org/2149153003/diff/20001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2149153003/diff/20001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:237: gfx::Point& screen_coord) { On 2016/07/15 00:34:22, Lei Zhang wrote: > Can't pass by non-const ref. Pass in a pointer instead? Or just pass in a single > pointer as an in-out parameter. Done. I used a single pointer as an in-out parameter.
Description was changed from ========== Links in PDF should be opened in new background tabs when ctrl + left clicked. BUG=628054 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Links in PDF should be opened in new background tabs when ctrl + left clicked. BUG=628054 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
https://codereview.chromium.org/2149153003/diff/40001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2149153003/diff/40001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:246: " Math.floor(" + std::to_string(point->x()) + " + screenOffsetX);" https://chromium-cpp.appspot.com/ says we can't quite use std::to_string() yet. Use base::IntToString() instead?
https://codereview.chromium.org/2149153003/diff/40001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2149153003/diff/40001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:246: " Math.floor(" + std::to_string(point->x()) + " + screenOffsetX);" On 2016/07/15 00:51:28, Lei Zhang wrote: > https://chromium-cpp.appspot.com/ says we can't quite use std::to_string() yet. > Use base::IntToString() instead? Done.
lgtm, but please give raymes@ a chance to comment on the JS. Please do a CQ dry run and make sure all the tests pass.
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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 failing Mac test. https://codereview.chromium.org/2149153003/diff/80001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2149153003/diff/80001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:642: Mac test was failing. We need to use MetaKey instead of ControlKey in Mac.
https://codereview.chromium.org/2149153003/diff/80001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2149153003/diff/80001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:642: On 2016/07/15 15:24:42, jaepark wrote: > Mac test was failing. We need to use MetaKey instead of ControlKey in Mac. Acknowledged.
js lgtm
The CQ bit was checked by jaepark@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2149153003/#ps80001 (title: "Links in PDF should be opened in new background tabs when ctrl + left clicked.")
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.
Description was changed from ========== Links in PDF should be opened in new background tabs when ctrl + left clicked. BUG=628054 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Links in PDF should be opened in new background tabs when ctrl + left clicked. BUG=628054 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Links in PDF should be opened in new background tabs when ctrl + left clicked. BUG=628054 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Links in PDF should be opened in new background tabs when ctrl + left clicked. BUG=628054 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/b8c0e4533fbd48c760be908803faded4b93e396d Cr-Commit-Position: refs/heads/master@{#406058} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b8c0e4533fbd48c760be908803faded4b93e396d Cr-Commit-Position: refs/heads/master@{#406058} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
