Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2300243004:
Links in PDF should open in a new window when shift + left clicked. (Closed)
DescriptionLinks in PDF should open in a new window when shift + left clicked.
BUG=628057
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/65033f1bda3b68880ed76c911bea8e12f2748e80
Cr-Commit-Position: refs/heads/master@{#417094}
Patch Set 1 : Links in PDF should open in a new window when shift + left clicked. #
Total comments: 6
Patch Set 2 : Links in PDF should open in a new window when shift + left clicked. #
Total comments: 20
Patch Set 3 : Links in PDF should open in a new window when shift + left clicked. #
Total comments: 12
Patch Set 4 : Links in PDF should open in a new window when shift + left clicked. #
Messages
Total messages: 37 (26 generated)
Description was changed from ========== Links in PDF should open in a new window when shift + left clicked. BUG=628057 ========== to ========== Links in PDF should open in a new window when shift + left clicked. BUG=628057 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...
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_...)
Patchset #1 (id:1) 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.
jaepark@google.com changed reviewers: + dsinclair@chromium.org, raymes@chromium.org, thestig@chromium.org
https://codereview.chromium.org/2300243004/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/navigator.js (right): https://codereview.chromium.org/2300243004/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:24: navigateInNewWindowCallback) { Now that we're passing 3 functions in here, we may want to consider passing in an object instead. https://codereview.chromium.org/2300243004/diff/20001/chrome/test/data/pdf/na... File chrome/test/data/pdf/navigator_test.js (right): https://codereview.chromium.org/2300243004/diff/20001/chrome/test/data/pdf/na... chrome/test/data/pdf/navigator_test.js:42: } I'm not sure if it's useful to define this 3 times, perhaps we can just have one? https://codereview.chromium.org/2300243004/diff/20001/chrome/test/data/pdf/na... chrome/test/data/pdf/navigator_test.js:69: function doNavigationUrlTestInCurrentTabAndNewTabAndNewWindow( Perhaps just change this to doNavigationUrlTest? Also the tests that call this don't seem to use the callbacks that get passed in, so perhaps we can just create them in here?
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/2300243004/diff/20001/chrome/browser/resource... File chrome/browser/resources/pdf/navigator.js (right): https://codereview.chromium.org/2300243004/diff/20001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:24: navigateInNewWindowCallback) { On 2016/09/05 04:29:32, raymes wrote: > Now that we're passing 3 functions in here, we may want to consider passing in > an object instead. Done. https://codereview.chromium.org/2300243004/diff/20001/chrome/test/data/pdf/na... File chrome/test/data/pdf/navigator_test.js (right): https://codereview.chromium.org/2300243004/diff/20001/chrome/test/data/pdf/na... chrome/test/data/pdf/navigator_test.js:42: } On 2016/09/05 04:29:32, raymes wrote: > I'm not sure if it's useful to define this 3 times, perhaps we can just have > one? Done. https://codereview.chromium.org/2300243004/diff/20001/chrome/test/data/pdf/na... chrome/test/data/pdf/navigator_test.js:69: function doNavigationUrlTestInCurrentTabAndNewTabAndNewWindow( On 2016/09/05 04:29:32, raymes wrote: > Perhaps just change this to doNavigationUrlTest? > > Also the tests that call this don't seem to use the callbacks that get passed > in, so perhaps we can just create them in here? Done.
Let me know if this makes sense. https://codereview.chromium.org/2300243004/diff/40001/chrome/browser/resource... File chrome/browser/resources/pdf/navigator.js (right): https://codereview.chromium.org/2300243004/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:16: function Navigator(originalUrl, viewport, paramsParser, navigateCallback) { I think it would be better to call it something like navigatorDelegate. https://codereview.chromium.org/2300243004/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:20: this.navigateInCurrentTabCallback_ = navigateCallback.currentTabCallback; this.navigatorDelegate_ = navigatorDelegate; https://codereview.chromium.org/2300243004/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:77: this.navigateInNewTabCallback_(url, false); this.navigatorDelegate_.navigateInNewTab(...); https://codereview.chromium.org/2300243004/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:80: this.navigateInNewTabCallback_(url, true); this.navigatorDelegate_.navigateInNewTab(...); https://codereview.chromium.org/2300243004/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:83: this.navigateInNewWindowCallback_(url); this.navigatorDelegate_.navigateInNewWindow(...); etc. https://codereview.chromium.org/2300243004/diff/40001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2300243004/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:49: function onNavigateInCurrentTab(isInTab, isSourceFileUrl, url) { Why not define these 3 functions inside a NavigatorDelegate class? https://codereview.chromium.org/2300243004/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:283: this.paramsParser_, onNavigateCallback); Then we could just pass in a new NavigatorDelegate here https://codereview.chromium.org/2300243004/diff/40001/chrome/test/data/pdf/na... File chrome/test/data/pdf/navigator_test.js (right): https://codereview.chromium.org/2300243004/diff/40001/chrome/test/data/pdf/na... chrome/test/data/pdf/navigator_test.js:5: function NavigateCallback() { We would make this into a MockNavigatorDelegate class too. The state could then be class-level state. We would need 3 separate bools to track whether each function had been called. https://codereview.chromium.org/2300243004/diff/40001/chrome/test/data/pdf/na... chrome/test/data/pdf/navigator_test.js:65: navigateCallback); We could just pass in a new MockNavigatorDelegate here (and below) https://codereview.chromium.org/2300243004/diff/40001/chrome/test/data/pdf/na... chrome/test/data/pdf/navigator_test.js:69: mockViewportChangedCallback, navigateInCurrentTabCallback); We could pass in the appropriate state of the MockNavigatorDelegate to check it in each of these function calls. Ideally we would check that only the correct function was called (and not others). This would require a little bit of reworking of the test.
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/2300243004/diff/40001/chrome/browser/resource... File chrome/browser/resources/pdf/navigator.js (right): https://codereview.chromium.org/2300243004/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:16: function Navigator(originalUrl, viewport, paramsParser, navigateCallback) { On 2016/09/06 04:36:07, raymes wrote: > I think it would be better to call it something like navigatorDelegate. Done. https://codereview.chromium.org/2300243004/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:20: this.navigateInCurrentTabCallback_ = navigateCallback.currentTabCallback; On 2016/09/06 04:36:07, raymes wrote: > this.navigatorDelegate_ = navigatorDelegate; Done. https://codereview.chromium.org/2300243004/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:77: this.navigateInNewTabCallback_(url, false); On 2016/09/06 04:36:07, raymes wrote: > this.navigatorDelegate_.navigateInNewTab(...); Done. https://codereview.chromium.org/2300243004/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:80: this.navigateInNewTabCallback_(url, true); On 2016/09/06 04:36:07, raymes wrote: > this.navigatorDelegate_.navigateInNewTab(...); Done. https://codereview.chromium.org/2300243004/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:83: this.navigateInNewWindowCallback_(url); On 2016/09/06 04:36:07, raymes wrote: > this.navigatorDelegate_.navigateInNewWindow(...); > > etc. Done. https://codereview.chromium.org/2300243004/diff/40001/chrome/browser/resource... File chrome/browser/resources/pdf/pdf.js (right): https://codereview.chromium.org/2300243004/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:49: function onNavigateInCurrentTab(isInTab, isSourceFileUrl, url) { On 2016/09/06 04:36:07, raymes wrote: > Why not define these 3 functions inside a NavigatorDelegate class? Done. https://codereview.chromium.org/2300243004/diff/40001/chrome/browser/resource... chrome/browser/resources/pdf/pdf.js:283: this.paramsParser_, onNavigateCallback); On 2016/09/06 04:36:07, raymes wrote: > Then we could just pass in a new NavigatorDelegate here Done. https://codereview.chromium.org/2300243004/diff/40001/chrome/test/data/pdf/na... File chrome/test/data/pdf/navigator_test.js (right): https://codereview.chromium.org/2300243004/diff/40001/chrome/test/data/pdf/na... chrome/test/data/pdf/navigator_test.js:5: function NavigateCallback() { On 2016/09/06 04:36:07, raymes wrote: > We would make this into a MockNavigatorDelegate class too. The state could then > be class-level state. We would need 3 separate bools to track whether each > function had been called. Done. https://codereview.chromium.org/2300243004/diff/40001/chrome/test/data/pdf/na... chrome/test/data/pdf/navigator_test.js:65: navigateCallback); On 2016/09/06 04:36:07, raymes wrote: > We could just pass in a new MockNavigatorDelegate here (and below) Done. https://codereview.chromium.org/2300243004/diff/40001/chrome/test/data/pdf/na... chrome/test/data/pdf/navigator_test.js:69: mockViewportChangedCallback, navigateInCurrentTabCallback); On 2016/09/06 04:36:07, raymes wrote: > We could pass in the appropriate state of the MockNavigatorDelegate to check it > in each of these function calls. > > Ideally we would check that only the correct function was called (and not > others). This would require a little bit of reworking of the test. Done.
Thanks this looks much better and is only +8 lines now :) lgtm https://codereview.chromium.org/2300243004/diff/60001/chrome/browser/resource... File chrome/browser/resources/pdf/navigator.js (right): https://codereview.chromium.org/2300243004/diff/60001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:24: * Creates a new NavigatorDelegate for calling callback funcions when nit: Creates a new NavigatorDelegate for calling browser-specific functions to do the actual navigating. https://codereview.chromium.org/2300243004/diff/60001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:30: function NavigatorDelegate(isInTab, isSourceFileUrl) { nit: perhaps move this class above "function Navigator" above https://codereview.chromium.org/2300243004/diff/60001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:38: * Called when navigation happens in the current tab. nit: Called when navigation should happen in the current tab. https://codereview.chromium.org/2300243004/diff/60001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:52: * Called when navigation happens in the new tab. nit: Called when navigation should happen in a new tab. https://codereview.chromium.org/2300243004/diff/60001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:67: * Called when navigation happens in the new window. nit: Called when navigation should happen in a new window. https://codereview.chromium.org/2300243004/diff/60001/chrome/test/data/pdf/na... File chrome/test/data/pdf/navigator_test.js (right): https://codereview.chromium.org/2300243004/diff/60001/chrome/test/data/pdf/na... chrome/test/data/pdf/navigator_test.js:36: * a new window depending on the value of |disposition|. nit: fill 80 chars
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.
Thanks for the review raymes@. thestig@ Would you please take a look at C++ part? Thanks! https://codereview.chromium.org/2300243004/diff/60001/chrome/browser/resource... File chrome/browser/resources/pdf/navigator.js (right): https://codereview.chromium.org/2300243004/diff/60001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:24: * Creates a new NavigatorDelegate for calling callback funcions when On 2016/09/07 06:41:19, raymes wrote: > nit: Creates a new NavigatorDelegate for calling browser-specific functions to > do the actual navigating. Done. https://codereview.chromium.org/2300243004/diff/60001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:30: function NavigatorDelegate(isInTab, isSourceFileUrl) { On 2016/09/07 06:41:19, raymes wrote: > nit: perhaps move this class above "function Navigator" above Done. https://codereview.chromium.org/2300243004/diff/60001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:38: * Called when navigation happens in the current tab. On 2016/09/07 06:41:19, raymes wrote: > nit: Called when navigation should happen in the current tab. Done. https://codereview.chromium.org/2300243004/diff/60001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:52: * Called when navigation happens in the new tab. On 2016/09/07 06:41:19, raymes wrote: > nit: Called when navigation should happen in a new tab. Done. https://codereview.chromium.org/2300243004/diff/60001/chrome/browser/resource... chrome/browser/resources/pdf/navigator.js:67: * Called when navigation happens in the new window. On 2016/09/07 06:41:19, raymes wrote: > nit: Called when navigation should happen in a new window. Done. https://codereview.chromium.org/2300243004/diff/60001/chrome/test/data/pdf/na... File chrome/test/data/pdf/navigator_test.js (right): https://codereview.chromium.org/2300243004/diff/60001/chrome/test/data/pdf/na... chrome/test/data/pdf/navigator_test.js:36: * a new window depending on the value of |disposition|. On 2016/09/07 06:41:19, raymes wrote: > nit: fill 80 chars Done.
lgtm
The CQ bit was checked by jaepark@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from raymes@chromium.org Link to the patchset: https://codereview.chromium.org/2300243004/#ps80001 (title: "Links in PDF should open in a new window when shift + 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.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Links in PDF should open in a new window when shift + left clicked. BUG=628057 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Links in PDF should open in a new window when shift + left clicked. BUG=628057 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/65033f1bda3b68880ed76c911bea8e12f2748e80 Cr-Commit-Position: refs/heads/master@{#417094} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/65033f1bda3b68880ed76c911bea8e12f2748e80 Cr-Commit-Position: refs/heads/master@{#417094} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
