Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2127383002:
Open hyperlinks in PDF in a new tab when middle mouse clicking. (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. |
DescriptionOpen hyperlinks in PDF in a new tab when middle mouse clicking.
BUG=71754
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/0e0acb5813a475fe4cfca72d99a0ea9a99e5d19d
Cr-Commit-Position: refs/heads/master@{#405430}
Patch Set 1 #
Total comments: 30
Patch Set 2 : Open hyperlinks in PDF in a new tab when middle mouse clicking. #
Total comments: 4
Patch Set 3 : Open hyperlinks in PDF in a new tab when middle mouse clicking. #
Total comments: 5
Patch Set 4 : Open hyperlinks in PDF in a new tab when middle mouse clicking. #
Total comments: 10
Patch Set 5 : Open hyperlinks in PDF in a new tab when middle mouse clicking. #
Total comments: 2
Patch Set 6 : Open hyperlinks in PDF in a new tab when middle mouse clicking. #
Total comments: 5
Patch Set 7 : Open hyperlinks in PDF in a new tab when middle mouse clicking. #
Messages
Total messages: 54 (24 generated)
Description was changed from ========== Open hyperlinks in PDF in a new tab when middle mouse clicking. BUG=71754 ========== to ========== Open hyperlinks in PDF in a new tab when middle mouse clicking. BUG=71754 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
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...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
jaepark@google.com changed reviewers: + thestig@chromium.org
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/2127383002/diff/1/chrome/browser/pdf/pdf_exte... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2127383002/diff/1/chrome/browser/pdf/pdf_exte... chrome/browser/pdf/pdf_extension_test.cc:619: " {type: 'getLinkPosition'});", It feels like a bit more work to have to post 2 getLinkPosition messages. Can we maybe post getLinkPosition onc, save the coordinates somewhere in JS somewhere, and make a couple calls to retrieve them? https://codereview.chromium.org/2127383002/diff/1/chrome/browser/pdf/pdf_exte... chrome/browser/pdf/pdf_extension_test.cc:635: ASSERT_EQ(std::string("https://www.google.com/"), url.spec()); Can we do example.com and add: host_resolver()->AddRule("example.com", "127.0.0.1"); so we don't hit the real example.com? https://codereview.chromium.org/2127383002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2127383002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:608: var visiblePage = this.viewport_.getMostVisiblePage(); Can you add a comment to say this translate page coordinates to viewport coordinates? https://codereview.chromium.org/2127383002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:613: x: message.data.x + visiblePageDimensions.x, What if the plugin fails to find the link? Is x equal to 0, or do we get a JS error? https://codereview.chromium.org/2127383002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/pdf_scripting_api.js (right): https://codereview.chromium.org/2127383002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf_scripting_api.js:241: * Get position for a link in the document. Can you give a little more detail here, since PDFs can have more than 1 link. Which one is being retrieved? https://codereview.chromium.org/2127383002/diff/1/pdf/out_of_process_instance.cc File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2127383002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:141: // Get link in document (Plugin -> Page) I think the direction is reversed. The page (JS) is telling the plugin (C++ code) to get the positions. https://codereview.chromium.org/2127383002/diff/1/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/2127383002/diff/1/pdf/pdf_engine.h#newcode299 pdf/pdf_engine.h:299: virtual std::vector<pp::Rect> GetLinkRects() = 0; Add a comment to explain what this does. https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_engine.cc... pdf/pdfium/pdfium_engine.cc:3676: return pages_[GetMostVisiblePage()]->GetLinkRects(); GetMostVisiblePage() can return -1. https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_engine.h#... pdf/pdfium/pdfium_engine.h:122: std::vector<pp::Rect> GetLinkRects(); Put this after GetMetadata() and add the override keyword? I'm surprised it worked without override. https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_page.cc File pdf/pdfium/pdfium_page.cc (right): https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_page.cc#n... pdf/pdfium/pdfium_page.cc:381: for (size_t i = 0; i < links_.size(); ++i) { nit: You can also do a range-based for loop here. https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_page.cc#n... pdf/pdfium/pdfium_page.cc:382: pp::Rect rect = UnionLinkCharRects(links_[i].rects); nit: No need for |rect| ? https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_page.cc#n... pdf/pdfium/pdfium_page.cc:390: if (rects.empty()) Can't you skip this check and get the same result? https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_page.h File pdf/pdfium/pdfium_page.h (right): https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_page.h#ne... pdf/pdfium/pdfium_page.h:99: std::vector<pp::Rect> GetLinkRects(); nit: Put this above the simple accessors, around line 89. https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_page.h#ne... pdf/pdfium/pdfium_page.h:117: pp::Rect UnionLinkCharRects(const std::vector<pp::Rect> rects); The parameter should be passed by const ref, not by value. https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_page.h#ne... pdf/pdfium/pdfium_page.h:117: pp::Rect UnionLinkCharRects(const std::vector<pp::Rect> rects); This method doesn't use any class members. Put it in pdfium_page.cc in an anonymous namespace? Thus you don't have to expose it all in via the header.
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...
https://codereview.chromium.org/2127383002/diff/1/chrome/browser/pdf/pdf_exte... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2127383002/diff/1/chrome/browser/pdf/pdf_exte... chrome/browser/pdf/pdf_extension_test.cc:619: " {type: 'getLinkPosition'});", On 2016/07/08 18:02:35, Lei Zhang wrote: > It feels like a bit more work to have to post 2 getLinkPosition messages. Can we > maybe post getLinkPosition onc, save the coordinates somewhere in JS somewhere, > and make a couple calls to retrieve them? Done. https://codereview.chromium.org/2127383002/diff/1/chrome/browser/pdf/pdf_exte... chrome/browser/pdf/pdf_extension_test.cc:635: ASSERT_EQ(std::string("https://www.google.com/"), url.spec()); On 2016/07/08 18:02:35, Lei Zhang wrote: > Can we do http://example.com and add: > > host_resolver()->AddRule("example.com", "127.0.0.1"); > > so we don't hit the real example.com? Done. https://codereview.chromium.org/2127383002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2127383002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:608: var visiblePage = this.viewport_.getMostVisiblePage(); On 2016/07/08 18:02:35, Lei Zhang wrote: > Can you add a comment to say this translate page coordinates to viewport > coordinates? I used viewportCoordinateX and viewportCoordinateY variables instead. https://codereview.chromium.org/2127383002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf.js:613: x: message.data.x + visiblePageDimensions.x, On 2016/07/08 18:02:35, Lei Zhang wrote: > What if the plugin fails to find the link? Is x equal to 0, or do we get a JS > error? I added linkExists field to check. https://codereview.chromium.org/2127383002/diff/1/chrome/browser/resources/pd... File chrome/browser/resources/pdf/pdf_scripting_api.js (right): https://codereview.chromium.org/2127383002/diff/1/chrome/browser/resources/pd... chrome/browser/resources/pdf/pdf_scripting_api.js:241: * Get position for a link in the document. On 2016/07/08 18:02:35, Lei Zhang wrote: > Can you give a little more detail here, since PDFs can have more than 1 link. > Which one is being retrieved? Modified to /** * Get position of the first link in the most visible page of the document. * @return {boolean} true if the function is successful, false if there is an * outstanding request that has not been answered. */ https://codereview.chromium.org/2127383002/diff/1/pdf/out_of_process_instance.cc File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2127383002/diff/1/pdf/out_of_process_instance... pdf/out_of_process_instance.cc:141: // Get link in document (Plugin -> Page) On 2016/07/08 18:02:35, Lei Zhang wrote: > I think the direction is reversed. The page (JS) is telling the plugin (C++ > code) to get the positions. Done. https://codereview.chromium.org/2127383002/diff/1/pdf/pdf_engine.h File pdf/pdf_engine.h (right): https://codereview.chromium.org/2127383002/diff/1/pdf/pdf_engine.h#newcode299 pdf/pdf_engine.h:299: virtual std::vector<pp::Rect> GetLinkRects() = 0; On 2016/07/08 18:02:36, Lei Zhang wrote: > Add a comment to explain what this does. Done. https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_engine.cc... pdf/pdfium/pdfium_engine.cc:3676: return pages_[GetMostVisiblePage()]->GetLinkRects(); On 2016/07/08 18:02:36, Lei Zhang wrote: > GetMostVisiblePage() can return -1. Done. https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_engine.h#... pdf/pdfium/pdfium_engine.h:122: std::vector<pp::Rect> GetLinkRects(); On 2016/07/08 18:02:36, Lei Zhang wrote: > Put this after GetMetadata() and add the override keyword? I'm surprised it > worked without override. Done. https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_page.cc File pdf/pdfium/pdfium_page.cc (right): https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_page.cc#n... pdf/pdfium/pdfium_page.cc:381: for (size_t i = 0; i < links_.size(); ++i) { On 2016/07/08 18:02:36, Lei Zhang wrote: > nit: You can also do a range-based for loop here. Done. https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_page.cc#n... pdf/pdfium/pdfium_page.cc:382: pp::Rect rect = UnionLinkCharRects(links_[i].rects); On 2016/07/08 18:02:36, Lei Zhang wrote: > nit: No need for |rect| ? Done. https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_page.cc#n... pdf/pdfium/pdfium_page.cc:390: if (rects.empty()) On 2016/07/08 18:02:36, Lei Zhang wrote: > Can't you skip this check and get the same result? Done. https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_page.h File pdf/pdfium/pdfium_page.h (right): https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_page.h#ne... pdf/pdfium/pdfium_page.h:99: std::vector<pp::Rect> GetLinkRects(); On 2016/07/08 18:02:36, Lei Zhang wrote: > nit: Put this above the simple accessors, around line 89. Done. https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_page.h#ne... pdf/pdfium/pdfium_page.h:117: pp::Rect UnionLinkCharRects(const std::vector<pp::Rect> rects); On 2016/07/08 18:02:36, Lei Zhang wrote: > The parameter should be passed by const ref, not by value. Done. https://codereview.chromium.org/2127383002/diff/1/pdf/pdfium/pdfium_page.h#ne... pdf/pdfium/pdfium_page.h:117: pp::Rect UnionLinkCharRects(const std::vector<pp::Rect> rects); On 2016/07/08 18:02:36, Lei Zhang wrote: > This method doesn't use any class members. Put it in pdfium_page.cc in an > anonymous namespace? Thus you don't have to expose it all in via the header. Done.
thestig@chromium.org changed reviewers: + raymes@chromium.org, tsergeant@chromium.org
+raymes/tsergeant to look over the JS bits too. https://codereview.chromium.org/2127383002/diff/20001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2127383002/diff/20001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:644: ASSERT_EQ(std::string("https://www.example.com/"), url.spec()); If the link is www.example.com, but the host resolver override is for example.com, do we end up hitting the real website? https://codereview.chromium.org/2127383002/diff/20001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2127383002/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:3678: if (page_index == -1) Really, anything less than 0 is out of bounds.
https://codereview.chromium.org/2127383002/diff/20001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2127383002/diff/20001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:644: ASSERT_EQ(std::string("https://www.example.com/"), url.spec()); On 2016/07/08 21:12:29, Lei Zhang wrote: > If the link is http://www.example.com, but the host resolver override is for > http://example.com, do we end up hitting the real website? Fixed. https://codereview.chromium.org/2127383002/diff/20001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2127383002/diff/20001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:3678: if (page_index == -1) On 2016/07/08 21:12:29, Lei Zhang wrote: > Really, anything less than 0 is out of bounds. 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.
thestig@chromium.org changed reviewers: + dbeam@chromium.org
+dbeam for JS changes as well.
how does this CL accomplish what your description says "Open hyperlinks in PDF in a new tab when middle mouse clicking."? does this work with the second or later link on the page? why can't we just send all the rectangles or something? also, where's the code that actually verifies that a mouse click is actually inside a link rectangle? https://codereview.chromium.org/2127383002/diff/40001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2127383002/diff/40001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:603: "var pos_x, pos_y;" these should probably be posX and posY as they're javascript (-ish) https://codereview.chromium.org/2127383002/diff/40001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf_scripting_api.js (right): https://codereview.chromium.org/2127383002/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/pdf_scripting_api.js:241: * Get position of the first link in the most visible page of the document. @param {function(number, number):void} callback https://codereview.chromium.org/2127383002/diff/40001/pdf/out_of_process_inst... File pdf/out_of_process_instance.cc (right): https://codereview.chromium.org/2127383002/diff/40001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:142: const char kJSGetLinkPositionType[] = "getLinkPosition"; please make this and all variants plural, i.e. getLinkPositions https://codereview.chromium.org/2127383002/diff/40001/pdf/out_of_process_inst... pdf/out_of_process_instance.cc:485: reply.Set(pp::Var(kJSGetLinkPositionY), pp::Var(rects[0].y())); wait, how does this work with multiple links?
Description was changed from ========== Open hyperlinks in PDF in a new tab when middle mouse clicking. BUG=71754 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Open hyperlinks in PDF in a new tab when middle mouse clicking. BUG=71754 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Hmm, how come we're adding a new thing to the scripting API? (I think I'm missing something :)
On 2016/07/11 23:57:20, raymes wrote: > Hmm, how come we're adding a new thing to the scripting API? (I think I'm > missing something :) It's only being added to make writing the new browser test possible. Is there a better way?
Let me know your thoughts of the suggestion below :) https://codereview.chromium.org/2127383002/diff/40001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2127383002/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:615: if (message.data.x !== undefined && message.data.y !== undefined) { The position of the first link in the test document should always be the same (in page/document coordinates). So we shouldn't need to go to the plugin. The only thing we have to do then is convert this into screen coordinates. We can avoid adding code here to do this by accessing the viewport_ directly from the test. Take a look at PdfZoomWithoutBubble for an example of accessing the viewport. This should hopefully simplify the test a lot.
On 2016/07/13 02:03:25, raymes wrote: > Let me know your thoughts of the suggestion below :) So just assume the link in the document never changes because it's not worth it to add all the extra code just for testing?
On 2016/07/13 02:05:51, Lei Zhang wrote: > On 2016/07/13 02:03:25, raymes wrote: > > Let me know your thoughts of the suggestion below :) > > So just assume the link in the document never changes because it's not worth it > to add all the extra code just for testing? That's my thought. It shouldn't change right?
On 2016/07/13 02:34:39, raymes wrote: > On 2016/07/13 02:05:51, Lei Zhang wrote: > > So just assume the link in the document never changes because it's not worth > it > > to add all the extra code just for testing? > > That's my thought. It shouldn't change right? I think this is right. If the link in the document changes, it will probably fail some pdfium corpus tests. Or should we add test-link.pdf in pdfium corpus test to make sure that it's always drawn in the same position?
On 2016/07/13 17:54:21, jaepark wrote: > On 2016/07/13 02:34:39, raymes wrote: > > On 2016/07/13 02:05:51, Lei Zhang wrote: > > > So just assume the link in the document never changes because it's not worth > > it > > > to add all the extra code just for testing? > > > > That's my thought. It shouldn't change right? > > I think this is right. If the link in the document changes, it will probably > fail some pdfium corpus tests. > Or should we add test-link.pdf in pdfium corpus test to make sure that it's > always drawn in the same position? Sure. I don't think it adds much more coverage, but it doesn't hurt either.
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...
https://codereview.chromium.org/2127383002/diff/60001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2127383002/diff/60001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:596: content::WebContents* web_contents = Move this down to line ~627, closer to where it's used, whenever possible. https://codereview.chromium.org/2127383002/diff/60001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:635: browser()->tab_strip_model()->GetActiveWebContents()->GetURL(); Does GetActiveWebContents() still return the same pointer as |web_contents| before? If yes, then this implies the PDF page has navigated to example.com, which is not right. If not, then that's not right either because middle clicking should load the link in a new tab, without changing the selected tab. https://codereview.chromium.org/2127383002/diff/60001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2127383002/diff/60001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1552: if (event.GetButton() == PP_INPUTEVENT_MOUSEBUTTON_MIDDLE) Add a comment to explain why we return here. https://codereview.chromium.org/2127383002/diff/60001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1555: if (area == PDFiumPage::DOCLINK_AREA) { What about middle clicks on DOCLINK_AREAs? How do they compare with (shift +) left clicks?
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/2127383002/diff/60001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2127383002/diff/60001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:596: content::WebContents* web_contents = On 2016/07/14 00:03:39, Lei Zhang wrote: > Move this down to line ~627, closer to where it's used, whenever possible. Done. https://codereview.chromium.org/2127383002/diff/60001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:635: browser()->tab_strip_model()->GetActiveWebContents()->GetURL(); On 2016/07/14 00:03:39, Lei Zhang wrote: > Does GetActiveWebContents() still return the same pointer as |web_contents| > before? Yes it does. > If yes, then this implies the PDF page has navigated to http://example.com, which is > not right. AFAIK, the url of the tab remains example.com even if it's actually navigated to 127.0.0.1 (I have checked this). > If not, then that's not right either because middle clicking should load the > link in a new tab, without changing the selected tab. Current ctrl + left click behavior is also opens a new tab and the new tab becomes the active web content. I know this is different from ctrl + left clicking (or middle clicking) link in a normal web page. But if we want that behavior, I think it's better to do it in another CL. https://codereview.chromium.org/2127383002/diff/60001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2127383002/diff/60001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1552: if (event.GetButton() == PP_INPUTEVENT_MOUSEBUTTON_MIDDLE) On 2016/07/14 00:03:39, Lei Zhang wrote: > Add a comment to explain why we return here. Done. https://codereview.chromium.org/2127383002/diff/60001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1555: if (area == PDFiumPage::DOCLINK_AREA) { On 2016/07/14 00:03:40, Lei Zhang wrote: > What about middle clicks on DOCLINK_AREAs? How do they compare with (shift +) > left clicks? I assume you mean ctrl + left (not shift + left) click which triggers the link to open in a new tab. For DOCLINK_AREAs, ctrl + left click behaves exactly the same as left click. It navigates within the tab. For middle mouse clicking, nothing happens when clicked on the DOCLINK AREAs.
https://codereview.chromium.org/2127383002/diff/60001/chrome/browser/pdf/pdf_... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2127383002/diff/60001/chrome/browser/pdf/pdf_... chrome/browser/pdf/pdf_extension_test.cc:635: browser()->tab_strip_model()->GetActiveWebContents()->GetURL(); On 2016/07/14 00:59:40, jaepark wrote: > On 2016/07/14 00:03:39, Lei Zhang wrote: > > Does GetActiveWebContents() still return the same pointer as |web_contents| > > before? > Yes it does. > > > If yes, then this implies the PDF page has navigated to http://example.com, > which is > > not right. > AFAIK, the url of the tab remains http://example.com even if it's actually navigated to > 127.0.0.1 (I have checked this). > > > If not, then that's not right either because middle clicking should load the > > link in a new tab, without changing the selected tab. > > Current ctrl + left click behavior is also opens a new tab and the new tab > becomes the active web content. > I know this is different from ctrl + left clicking (or middle clicking) link in > a normal web page. But if we want that behavior, I think it's better to do it in > another CL. Sure. Can you add a comment to here to note the existing behavior is wrong and reference https://crbug.com/628054 ? https://codereview.chromium.org/2127383002/diff/60001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2127383002/diff/60001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:1555: if (area == PDFiumPage::DOCLINK_AREA) { On 2016/07/14 00:59:40, jaepark wrote: > On 2016/07/14 00:03:40, Lei Zhang wrote: > > What about middle clicks on DOCLINK_AREAs? How do they compare with (shift +) > > left clicks? > > I assume you mean ctrl + left (not shift + left) click which triggers the link > to open in a new tab. > For DOCLINK_AREAs, ctrl + left click behaves exactly the same as left click. It > navigates within the tab. > > For middle mouse clicking, nothing happens when clicked on the DOCLINK AREAs. Yes, I meant ctrl, though I just discovered what shift + left click does on a regular webpage... and now we have https://crbug.com/628057, wee! https://codereview.chromium.org/2127383002/diff/100001/chrome/browser/pdf/pdf... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2127383002/diff/100001/chrome/browser/pdf/pdf... chrome/browser/pdf/pdf_extension_test.cc:636: browser()->tab_strip_model()->GetActiveWebContents()->GetURL(); Ok, so if we are testing the current "incorrect" behavior, then you'd want to do: // Check the current tab is the new tab. content::WebContents* new_web_contents = browser()->tab_strip_model()->GetActiveWebContents(); ASSERT_NE(web_contents, new_web_contents); const GURL& url = new_web_contents->GetURL();
https://codereview.chromium.org/2127383002/diff/100001/chrome/browser/pdf/pdf... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2127383002/diff/100001/chrome/browser/pdf/pdf... chrome/browser/pdf/pdf_extension_test.cc:636: browser()->tab_strip_model()->GetActiveWebContents()->GetURL(); On 2016/07/14 01:25:05, Lei Zhang wrote: > Ok, so if we are testing the current "incorrect" behavior, then you'd want to > do: > > // Check the current tab is the new tab. > content::WebContents* new_web_contents = > browser()->tab_strip_model()->GetActiveWebContents(); > ASSERT_NE(web_contents, new_web_contents); > const GURL& url = new_web_contents->GetURL(); Done. https://codereview.chromium.org/2127383002/diff/120001/chrome/browser/pdf/pdf... File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/2127383002/diff/120001/chrome/browser/pdf/pdf... chrome/browser/pdf/pdf_extension_test.cc:636: // the focus of the tab. See http://crbug.com/628054. Added the comment that existing behavior is wrong.
lgtm
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 https://codereview.chromium.org/2127383002/diff/120001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2127383002/diff/120001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1552: // Prevent middle mouse button from selecting texts nit: texts->texts. https://codereview.chromium.org/2127383002/diff/120001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1654: // Prevent middle mouse button from selecting texts nit: same here
https://codereview.chromium.org/2127383002/diff/120001/pdf/pdfium/pdfium_engi... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/2127383002/diff/120001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1552: // Prevent middle mouse button from selecting texts On 2016/07/14 02:53:41, raymes wrote: > nit: texts->texts. Done. https://codereview.chromium.org/2127383002/diff/120001/pdf/pdfium/pdfium_engi... pdf/pdfium/pdfium_engine.cc:1654: // Prevent middle mouse button from selecting texts On 2016/07/14 02:53:41, raymes wrote: > nit: same here Done.
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, raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2127383002/#ps140001 (title: "Open hyperlinks in PDF in a new tab when middle mouse clicking.")
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 ========== Open hyperlinks in PDF in a new tab when middle mouse clicking. BUG=71754 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Open hyperlinks in PDF in a new tab when middle mouse clicking. BUG=71754 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Open hyperlinks in PDF in a new tab when middle mouse clicking. BUG=71754 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== Open hyperlinks in PDF in a new tab when middle mouse clicking. BUG=71754 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/0e0acb5813a475fe4cfca72d99a0ea9a99e5d19d Cr-Commit-Position: refs/heads/master@{#405430} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/0e0acb5813a475fe4cfca72d99a0ea9a99e5d19d Cr-Commit-Position: refs/heads/master@{#405430} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
