PDFs viewed inside a <webview> should navigate the same as PDFs in tabs.

PDFs viewed inside a <webview> should navigate the same as PDFs in tabs.

This CL is (sort-of) a reland of

(1) https://codereview.chromium.org/1234403005/

as well as a revert of

(2) https://codereview.chromium.org/1275173004

CL (2) broke the CL (1), in that chrome.tabs.update will always
update the active tab in the browser window. If a user left-clicks on
a link in the PDF viewer, the expectation is that the PDF contents will
be replaced by the contents at the link, but that cannot happen
if the PDF is displayed in an app and chrome.tabs.update is used.

Also, https://codereview.chromium.org/1275173004 suggests that the
advantage of chrome.tabs.update is to navigate from one file:// url
to another, but based on other discussions,

https://code.google.com/p/chromium/issues/detail?id=528505

this may not be appropriate.

Finally, CL (1) broke context-menu navigations for links within webviews,
namely "open in new tab", "open in new window" and "open in incognito
window". This CL restricts itself to CURRENT_TAB navigations in order to
fix that.

BUG=532621
Committed: https://crrev.com/cfe0de44de3d2916c2d7254027cb70e0c7c14e0d
Cr-Commit-Position: refs/heads/master@{#351423}

[wjmaclean, 2015-09-17 17:47:24]

wjmaclean@chromium.org changed reviewers:
+ creis@chromium.org, sammc@google.com, thestig@chromium.org

[wjmaclean, 2015-09-17 17:47:49]

Please take a look.

[wjmaclean, 2015-09-17 17:47:56]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-17 17:48:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1350073002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1350073002/1

[commit-bot: I haz the power, 2015-09-17 18:42:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-17 18:42:53]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2015-09-17 22:54:00]

This CL would cause https://crbug.com/517780 to reopen.

If we have file:///tmp/foo.html, and it has a relative link to "bar.html",
simply clicking the link results in a successful navigation to
file:///tmp/bar.html

If we have file:///tmp/foo.pdf, and it has a relative link to "bar.html", this
CL causes it to navigate to about:blank.

[Sam McNally, 2015-09-17 23:36:32]

sammc@chromium.org changed reviewers:
+ sammc@chromium.org
- sammc@google.com

[Sam McNally, 2015-09-18 01:03:47]

https://codereview.chromium.org/1350073002/diff/1/chrome/browser/resources/pd...
File chrome/browser/resources/pdf/pdf.js (left):

https://codereview.chromium.org/1350073002/diff/1/chrome/browser/resources/pd...
chrome/browser/resources/pdf/pdf.js:49: if (chrome.tabs)
Could we change this to
  if (chrome.tabs && this.browserApi_.getStreamInfo().tabId != -1)
    chrome.tabs.update(this.browserApi_.getStreamInfo().tabId, {url: url});
  else
    window.location.href = url;

This would allow file url links to work outside webviews and webview navigations
to not leak to the last-active tab. As far as I can tell, webviews can't
navigate to file urls so this should cover everything.

[wjmaclean, 2015-09-18 16:20:30]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[wjmaclean, 2015-09-18 16:20:45]

https://codereview.chromium.org/1350073002/diff/1/chrome/browser/resources/pd...
File chrome/browser/resources/pdf/pdf.js (left):

https://codereview.chromium.org/1350073002/diff/1/chrome/browser/resources/pd...
chrome/browser/resources/pdf/pdf.js:49: if (chrome.tabs)
On 2015/09/18 01:03:47, Sam McNally wrote:
> Could we change this to
>   if (chrome.tabs && this.browserApi_.getStreamInfo().tabId != -1)
>     chrome.tabs.update(this.browserApi_.getStreamInfo().tabId, {url: url});
>   else
>     window.location.href = url;
> 
> This would allow file url links to work outside webviews and webview
navigations
> to not leak to the last-active tab. As far as I can tell, webviews can't
> navigate to file urls so this should cover everything.

Yes, but in this context |this| is a "Navigator" and does not have a
|browserApi_| member. I've plumbed one through, though a tad willy-nilly, as I'm
not sure what other implications this may have. Is there anything else that
needs to be done?

It does seem to solve both our cases.

I had originally attempted to look at chrome.tabs.getTabId, which didn't work
(for fairly obvious reasons), but I wasn't aware of browserApi, so thanks for
pointing it out.

[commit-bot: I haz the power, 2015-09-18 16:21:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1350073002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1350073002/20001

[commit-bot: I haz the power, 2015-09-18 17:08:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-18 17:08:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Lei Zhang, 2015-09-19 01:05:51]

Patch set 2 has broken tests, BTW.

[wjmaclean, 2015-09-21 14:02:13]

On 2015/09/19 01:05:51, Lei Zhang wrote:
> Patch set 2 has broken tests, BTW.

Yes, I noticed.

Apparently adding the browserApi object to the Navigator has some additional
steps. I won't get to this today, but will likely look at it tomorrow.

[wjmaclean, 2015-09-22 19:19:49]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-22 19:20:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1350073002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1350073002/40001

[commit-bot: I haz the power, 2015-09-22 21:06:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-22 21:06:26]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2015-09-23 13:35:02]

thestig@ & sammc@ ... is this ok now?

[raymes, 2015-09-24 00:09:42]

raymes@chromium.org changed reviewers:
+ raymes@chromium.org

[raymes, 2015-09-24 00:09:42]

https://codereview.chromium.org/1350073002/diff/40001/chrome/browser/resource...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/1350073002/diff/40001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:257: onNavigateInCurrentTab,
onNavigateInNewTab);
can we bind the tabId to onNavigateInCurrentTab rather than passing it into
navigator?

[wjmaclean, 2015-09-24 21:50:34]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-24 21:51:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1350073002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1350073002/60001

[commit-bot: I haz the power, 2015-09-24 22:34:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-24 22:34:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[wjmaclean, 2015-09-25 12:51:22]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-25 12:52:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1350073002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1350073002/80001

[wjmaclean, 2015-09-25 12:52:26]

raymes@ - is this ok?

There is some urgency to try and get this in and merged for 46, so I'm hoping
someone can take a look today.

[commit-bot: I haz the power, 2015-09-25 13:56:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-25 13:56:24]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2015-09-25 19:01:43]

https://codereview.chromium.org/1350073002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/1350073002/diff/80001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:63: if (chrome.tabs)
Do we need to add a tabId here as well?

[wjmaclean, 2015-09-25 19:04:12]

https://codereview.chromium.org/1350073002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/1350073002/diff/80001/chrome/browser/resource...
chrome/browser/resources/pdf/pdf.js:63: if (chrome.tabs)
On 2015/09/25 19:01:43, Lei Zhang wrote:
> Do we need to add a tabId here as well?

I don't think so, as it's going to attempt to open a new tab ...

[Lei Zhang, 2015-09-25 19:14:23]

The https://crbug.com/514387 test case is still broken with this CL. In the JS
code, |tabId| comes back as -1 for some reason.

[wjmaclean, 2015-09-25 19:58:35]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[wjmaclean, 2015-09-25 19:59:17]

My bad, apparently I misunderstood how to bind properly in JS (I'm really not a
JS guy ...). This version should be better.

[commit-bot: I haz the power, 2015-09-25 19:59:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1350073002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1350073002/100001

[commit-bot: I haz the power, 2015-09-25 20:52:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-25 20:52:32]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2015-09-25 20:57:31]

dbeam@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2015-09-25 20:57:32]

https://codereview.chromium.org/1350073002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/1350073002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf.js:44: * @param {int} tabId The id of the
current tab; -1 if no current tab.
there are no ints in javascript. this should be number

https://codereview.chromium.org/1350073002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf.js:251: var onNavigateBoundToCurrentTab =
function(browserApi, url) {
browserApi is already a local variable to this method, no need to bind it in
this method.  you can use browserApi from within the method without this param.

https://codereview.chromium.org/1350073002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf.js:252: var tabId = browserApi ?
browserApi.getStreamInfo().tabId : -1;
you're binding this.browserApi_ from outside, and it looks like that'll always
be truthy.  what is this ternary actually doing?

[raymes, 2015-09-27 23:02:02]

https://codereview.chromium.org/1350073002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/1350073002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf.js:261: onNavigateBoundToCurrentTab,
I think you should be able to delete onNavigateBoundToCurrentTab above and just
pass in:

onNavigateInCurrentTab.bind(undefined, browserApi.getStreamInfo().tabId)

[wjmaclean, 2015-09-28 13:29:22]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[wjmaclean, 2015-09-28 13:29:35]

https://codereview.chromium.org/1350073002/diff/100001/chrome/browser/resourc...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/1350073002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf.js:44: * @param {int} tabId The id of the
current tab; -1 if no current tab.
On 2015/09/25 20:57:31, Dan Beam wrote:
> there are no ints in javascript. this should be number

Done.

https://codereview.chromium.org/1350073002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf.js:251: var onNavigateBoundToCurrentTab =
function(browserApi, url) {
On 2015/09/25 20:57:31, Dan Beam wrote:
> browserApi is already a local variable to this method, no need to bind it in
> this method.  you can use browserApi from within the method without this
param.

Acknowledged.

https://codereview.chromium.org/1350073002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf.js:252: var tabId = browserApi ?
browserApi.getStreamInfo().tabId : -1;
On 2015/09/25 20:57:31, Dan Beam wrote:
> you're binding this.browserApi_ from outside, and it looks like that'll always
> be truthy.  what is this ternary actually doing?

Acknowledged.

https://codereview.chromium.org/1350073002/diff/100001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf.js:261: onNavigateBoundToCurrentTab,
On 2015/09/27 23:02:02, raymes wrote:
> I think you should be able to delete onNavigateBoundToCurrentTab above and
just
> pass in:
> 
> onNavigateInCurrentTab.bind(undefined, browserApi.getStreamInfo().tabId)

Ok.

I wasn't sure if it was safe to assume that the tabId value would never change,
hence why I originally wanted to check it each time the bound function was
called.

Done.

[commit-bot: I haz the power, 2015-09-28 13:29:41]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1350073002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1350073002/120001

[commit-bot: I haz the power, 2015-09-28 14:30:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-28 14:30:30]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2015-09-28 15:59:12]

https://codereview.chromium.org/1350073002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/1350073002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf.js:50: if (chrome.tabs && tabId != -1)
are you using tabId == -1 basically as a check to see if you're embedded in a
<webview>?  because if so...

  /**
   * @param {string} url ...
   * @param {boolean} inWebView ...
   */
  function onNavigateInCurrentTab(url, inWebView) {
    if (!inWebView && chrome.tabs)
      chrome.tabs.update({url: url});
    else
      window.location.href = url;
  }

https://codereview.chromium.org/1350073002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf.js:257:
this.browserApi_.getStreamInfo().tabId),
wait, don't you *want* this to use the current tabId if it changes?

[wjmaclean, 2015-09-28 16:10:09]

https://codereview.chromium.org/1350073002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/1350073002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf.js:50: if (chrome.tabs && tabId != -1)
On 2015/09/28 15:59:11, Dan Beam wrote:
> are you using tabId == -1 basically as a check to see if you're embedded in a
> <webview>?  because if so...
> 
>   /**
>    * @param {string} url ...
>    * @param {boolean} inWebView ...
>    */
>   function onNavigateInCurrentTab(url, inWebView) {
>     if (!inWebView && chrome.tabs)
>       chrome.tabs.update({url: url});
>     else
>       window.location.href = url;
>   }

We could change it to 

@param {boolean} inTab

perhaps? I don't think we should limit ourselves to just WebView in this case.

https://codereview.chromium.org/1350073002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf.js:257:
this.browserApi_.getStreamInfo().tabId),
On 2015/09/28 15:59:12, Dan Beam wrote:
> wait, don't you *want* this to use the current tabId if it changes?

Well, yes, I think we would. However, it's not at all obvious that a Navigator,
once created, will ever change its tabId. I was assuming from Raymes suggestion
that we don't think it ever will.

[raymes, 2015-09-29 01:46:27]

chrome/browser/resources/pdf/pdf.js lgtm

https://codereview.chromium.org/1350073002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/1350073002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf.js:50: if (chrome.tabs && tabId != -1)
I agree - there might be other cases where we don't have a tabId. sammc@ would
know about what the other cases are.

https://codereview.chromium.org/1350073002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf.js:257:
this.browserApi_.getStreamInfo().tabId),
On 2015/09/28 16:10:08, wjmaclean wrote:
> On 2015/09/28 15:59:12, Dan Beam wrote:
> > wait, don't you *want* this to use the current tabId if it changes?
> 
> Well, yes, I think we would. However, it's not at all obvious that a
Navigator,
> once created, will ever change its tabId. I was assuming from Raymes
suggestion
> that we don't think it ever will.

Correct - the tabId will never change.

[wjmaclean, 2015-09-29 13:33:07]

The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-29 13:34:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1350073002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1350073002/140001

[wjmaclean, 2015-09-29 13:43:21]

dbeam@ - does this look ok to you?

[commit-bot: I haz the power, 2015-09-29 14:22:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-29 14:22:09]

Dry run: This issue passed the CQ dry run.

[wjmaclean, 2015-09-29 22:26:41]

The CQ bit was checked by wjmaclean@chromium.org

[wjmaclean, 2015-09-29 22:26:41]

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/1350073002/#ps140001
(title: "Pass boolean instead of tabId.")

[commit-bot: I haz the power, 2015-09-29 22:28:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1350073002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1350073002/140001

[Dan Beam, 2015-09-29 22:38:26]

this is more readable, IMO

https://codereview.chromium.org/1350073002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/1350073002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf.js:265: isInTab),
function() { onNavigateInCurrentTab(isInTab); },

[Dan Beam, 2015-09-29 22:38:59]

https://codereview.chromium.org/1350073002/diff/140001/chrome/browser/resourc...
File chrome/browser/resources/pdf/pdf.js (right):

https://codereview.chromium.org/1350073002/diff/140001/chrome/browser/resourc...
chrome/browser/resources/pdf/pdf.js:265: isInTab),
On 2015/09/29 22:38:26, Dan Beam wrote:
> function() { onNavigateInCurrentTab(isInTab); },

function(url) { onNavigateInCurrentTab(isInTab, url); },

**

[Dan Beam, 2015-09-29 22:39:12]

otherwise lgtm

[commit-bot: I haz the power, 2015-09-29 22:44:25]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-09-29 22:45:48]

Patchset 8 (id:??) landed as
https://crrev.com/cfe0de44de3d2916c2d7254027cb70e0c7c14e0d
Cr-Commit-Position: refs/heads/master@{#351423}