PDF viewer: Specify tabId when navigating with chrome.tabs.update.

PDF viewer: Specify tabId when navigating with chrome.tabs.update.

Currently, the use of chrome.tabs.update to navigate defaults to using
the selected tab. If another tab is selected before the navigation is
performed, this other tab will be navigated instead of the PDF viewer's
tab.

We pass the viewer's tabId to chrome.tabs.update to ensure the correct
tab is navigated.

BUG=672563
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/cf2bc970b5ee619700cc1f334b4c2145cf0a5ab5
Cr-Commit-Position: refs/heads/master@{#438563}

[Kevin McNee, 2016-12-12 18:24:24]

Description was changed from

==========
PDF viewer: Specify tabId when navigating with chrome.tabs.update.

Currently, the use of chrome.tabs.update to navigate defaults to using
the selected tab. If another tab is selected before the navigation is
performed, this other tab will be navigated instead of the PDF viewer's
tab.

We pass the viewer's tabId to chrome.tabs.update to ensure the correct
tab is navigated.

BUG=672563
==========

to

==========
PDF viewer: Specify tabId when navigating with chrome.tabs.update.

Currently, the use of chrome.tabs.update to navigate defaults to using
the selected tab. If another tab is selected before the navigation is
performed, this other tab will be navigated instead of the PDF viewer's
tab.

We pass the viewer's tabId to chrome.tabs.update to ensure the correct
tab is navigated.

BUG=672563
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Kevin McNee, 2016-12-12 18:40:20]

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

[Kevin McNee, 2016-12-12 18:40:21]

raymes@: Here's the fix for navigating the correct tab.

[Kevin McNee, 2016-12-12 18:44:14]

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

[commit-bot: I haz the power, 2016-12-12 18:45:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 23:52:39]

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

[commit-bot: I haz the power, 2016-12-12 23:52:40]

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_...)

[raymes, 2016-12-13 00:59:06]

Thanks Kevin!

We should also add a test. Have a look in pdf_extension_test.cc. This does look
a bit tricky to test but I think we can try do it:
1) Load a PDF using LoadPdfGetGuestContents to get a handle on the guest
WebContents
2) Open a new tab
3) content::ExecuteScript in the guest WebContents which fires a 'navigate'
event (look in pdf.js). 
4) Check that there is no pending navigation for the top level WebContents.

Would you be able to have a look at that?

[Kevin McNee, 2016-12-13 21:38:21]

On 2016/12/13 00:59:06, raymes wrote:
> Thanks Kevin!
> 
> We should also add a test. Have a look in pdf_extension_test.cc. This does
look
> a bit tricky to test but I think we can try do it:
> 1) Load a PDF using LoadPdfGetGuestContents to get a handle on the guest
> WebContents
> 2) Open a new tab
> 3) content::ExecuteScript in the guest WebContents which fires a 'navigate'
> event (look in pdf.js). 
> 4) Check that there is no pending navigation for the top level WebContents.
> 
> Would you be able to have a look at that?

Done.

[Kevin McNee, 2016-12-13 21:42:06]

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

[commit-bot: I haz the power, 2016-12-13 21:42:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-13 23:21:11]

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

[commit-bot: I haz the power, 2016-12-13 23:21:12]

Dry run: This issue passed the CQ dry run.

[raymes, 2016-12-14 01:43:28]

lgtm, thank you!

https://codereview.chromium.org/2568903002/diff/20001/chrome/browser/pdf/pdf_...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2568903002/diff/20001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:909:
IN_PROC_BROWSER_TEST_F(PDFExtensionTest, NavigationOnCorrectTab) {
nit: You may want to link to the bug above here and note that this makes sure
that even if a different tab is selected when a navigation occurs, the correct
tab still gets navigated.

https://codereview.chromium.org/2568903002/diff/20001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:922:
browser()->tab_strip_model()->GetActiveWebContents();
nit: may want to assert that active_web_contents is not equal to web_contents

https://codereview.chromium.org/2568903002/diff/20001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:936:
"plugin.dispatchEvent(navMessage);"));
nit: I was thinking we could just fire the navigate event on the document. On
second thought, I think we can just call viewer.navigator_.navigate(uri,
disposition)

[Kevin McNee, 2016-12-14 16:03:33]

https://codereview.chromium.org/2568903002/diff/20001/chrome/browser/pdf/pdf_...
File chrome/browser/pdf/pdf_extension_test.cc (right):

https://codereview.chromium.org/2568903002/diff/20001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:909:
IN_PROC_BROWSER_TEST_F(PDFExtensionTest, NavigationOnCorrectTab) {
On 2016/12/14 01:43:28, raymes wrote:
> nit: You may want to link to the bug above here and note that this makes sure
> that even if a different tab is selected when a navigation occurs, the correct
> tab still gets navigated.

Done.

https://codereview.chromium.org/2568903002/diff/20001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:922:
browser()->tab_strip_model()->GetActiveWebContents();
On 2016/12/14 01:43:28, raymes wrote:
> nit: may want to assert that active_web_contents is not equal to web_contents

Done.

https://codereview.chromium.org/2568903002/diff/20001/chrome/browser/pdf/pdf_...
chrome/browser/pdf/pdf_extension_test.cc:936:
"plugin.dispatchEvent(navMessage);"));
On 2016/12/14 01:43:28, raymes wrote:
> nit: I was thinking we could just fire the navigate event on the document. On
> second thought, I think we can just call viewer.navigator_.navigate(uri,
> disposition)

Done.

[Kevin McNee, 2016-12-14 16:05:11]

The CQ bit was checked by mcnee@chromium.org

[Kevin McNee, 2016-12-14 16:05:11]

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/2568903002/#ps40001
(title: "Fix nits.")

[commit-bot: I haz the power, 2016-12-14 16:05:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-14 18:57:58]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481731511423500,
"parent_rev": "0283d45c441d1ba91301f1d3c22ee0b042d295d4", "commit_rev":
"5d9784e9e76b85ebf3e4bdaa0e28640915c7b064"}

[commit-bot: I haz the power, 2016-12-14 18:58:28]

Description was changed from

==========
PDF viewer: Specify tabId when navigating with chrome.tabs.update.

Currently, the use of chrome.tabs.update to navigate defaults to using
the selected tab. If another tab is selected before the navigation is
performed, this other tab will be navigated instead of the PDF viewer's
tab.

We pass the viewer's tabId to chrome.tabs.update to ensure the correct
tab is navigated.

BUG=672563
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
PDF viewer: Specify tabId when navigating with chrome.tabs.update.

Currently, the use of chrome.tabs.update to navigate defaults to using
the selected tab. If another tab is selected before the navigation is
performed, this other tab will be navigated instead of the PDF viewer's
tab.

We pass the viewer's tabId to chrome.tabs.update to ensure the correct
tab is navigated.

BUG=672563
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2568903002
==========

[commit-bot: I haz the power, 2016-12-14 18:58:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-14 19:02:37]

Description was changed from

==========
PDF viewer: Specify tabId when navigating with chrome.tabs.update.

Currently, the use of chrome.tabs.update to navigate defaults to using
the selected tab. If another tab is selected before the navigation is
performed, this other tab will be navigated instead of the PDF viewer's
tab.

We pass the viewer's tabId to chrome.tabs.update to ensure the correct
tab is navigated.

BUG=672563
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Review-Url: https://codereview.chromium.org/2568903002
==========

to

==========
PDF viewer: Specify tabId when navigating with chrome.tabs.update.

Currently, the use of chrome.tabs.update to navigate defaults to using
the selected tab. If another tab is selected before the navigation is
performed, this other tab will be navigated instead of the PDF viewer's
tab.

We pass the viewer's tabId to chrome.tabs.update to ensure the correct
tab is navigated.

BUG=672563
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/cf2bc970b5ee619700cc1f334b4c2145cf0a5ab5
Cr-Commit-Position: refs/heads/master@{#438563}
==========

[commit-bot: I haz the power, 2016-12-14 19:02:38]

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