Add check for file:-navigations from PDFs

Add check for file:-navigations from PDFs

BUG=654279
TEST=./browser_tests --gtest_filter=PDFExtensionTest.Navigator
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/374249e767a68d8da073a4ed3a4f29236451174c
Cr-Commit-Position: refs/heads/master@{#425287}

[robwu, 2016-10-09 16:05:19]

Description was changed from

==========
Add check for file:-navigations from PDFs

BUG=654279
TEST=Manually, using steps from bug report.
==========

to

==========
Add check for file:-navigations from PDFs

BUG=654279
TEST=Manually, using steps from bug report.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[robwu, 2016-10-09 16:05:22]

The CQ bit was checked by rob@robwu.nl to run a CQ dry run

[commit-bot: I haz the power, 2016-10-09 16:05:34]

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

[robwu, 2016-10-09 16:06:23]

Description was changed from

==========
Add check for file:-navigations from PDFs

BUG=654279
TEST=Manually, using steps from bug report.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add check for file:-navigations from PDFs

BUG=654279
TEST=Manually, using steps from bug report.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[robwu, 2016-10-09 16:06:23]

rob@robwu.nl changed reviewers:
+ thestig@chromium.org

[commit-bot: I haz the power, 2016-10-09 16:54:09]

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

[commit-bot: I haz the power, 2016-10-09 16:54:10]

Dry run: This issue passed the CQ dry run.

[robwu, 2016-10-09 16:59:29]

Lei, I hope you still have energy for this review after a weekend of plumbing ;)

[Lei Zhang, 2016-10-10 21:07:03]

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

[Lei Zhang, 2016-10-10 21:07:04]

+raymes for Javascript bits.

It turns out plumbing is hard work and I ran out of energy.

https://codereview.chromium.org/2402873002/diff/1/chrome/browser/resources/pd...
File chrome/browser/resources/pdf/navigator.js (right):

https://codereview.chromium.org/2402873002/diff/1/chrome/browser/resources/pd...
chrome/browser/resources/pdf/navigator.js:74: if (!this.isSourceFileUrl_ &&
/^file:/i.test(url))
Can we write a helper function and call it from multiple places? Should
navigateInCurrentTab() call it as well?

[raymes, 2016-10-11 06:37:42]

https://codereview.chromium.org/2402873002/diff/1/chrome/browser/resources/pd...
File chrome/browser/resources/pdf/navigator.js (right):

https://codereview.chromium.org/2402873002/diff/1/chrome/browser/resources/pd...
chrome/browser/resources/pdf/navigator.js:74: if (!this.isSourceFileUrl_ &&
/^file:/i.test(url))
On 2016/10/10 21:07:04, Lei Zhang wrote:
> Can we write a helper function and call it from multiple places? Should
> navigateInCurrentTab() call it as well?

+1

Could you also please add a test to this to navigator_test.js. We may also want
a test in pdf_extension_test.cc

[robwu, 2016-10-12 22:07:46]

Description was changed from

==========
Add check for file:-navigations from PDFs

BUG=654279
TEST=Manually, using steps from bug report.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add check for file:-navigations from PDFs

BUG=654279
TEST=./browser_tests --gtest_filter=PDFExtensionTest.Navigator
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[robwu, 2016-10-12 22:08:00]

The CQ bit was checked by rob@robwu.nl to run a CQ dry run

[robwu, 2016-10-12 22:08:01]

https://codereview.chromium.org/2402873002/diff/1/chrome/browser/resources/pd...
File chrome/browser/resources/pdf/navigator.js (right):

https://codereview.chromium.org/2402873002/diff/1/chrome/browser/resources/pd...
chrome/browser/resources/pdf/navigator.js:74: if (!this.isSourceFileUrl_ &&
/^file:/i.test(url))
On 2016/10/11 06:37:42, raymes wrote:
> On 2016/10/10 21:07:04, Lei Zhang wrote:
> > Can we write a helper function and call it from multiple places? Should
> > navigateInCurrentTab() call it as well?
> 
> +1
> 
> Could you also please add a test to this to navigator_test.js. We may also
want
> a test in pdf_extension_test.cc

Done.

[commit-bot: I haz the power, 2016-10-12 22:08:24]

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-10-12 23:57:15]

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

[commit-bot: I haz the power, 2016-10-12 23:57:17]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[raymes, 2016-10-13 00:11:34]

lgtm thanks for the cleanups too

https://codereview.chromium.org/2402873002/diff/20001/chrome/test/data/pdf/na...
File chrome/test/data/pdf/navigator_test.js (right):

https://codereview.chromium.org/2402873002/diff/20001/chrome/test/data/pdf/na...
chrome/test/data/pdf/navigator_test.js:53: return;
Can we assert 
navigatorDelegate.navigateInCurrentTabCalled
navigatorDelegate.navigateInNewTabCalled
navigatorDelegate.navigateInNewWindowCalled

are all false in this case?

[robwu, 2016-10-13 07:22:35]

https://codereview.chromium.org/2402873002/diff/20001/chrome/test/data/pdf/na...
File chrome/test/data/pdf/navigator_test.js (right):

https://codereview.chromium.org/2402873002/diff/20001/chrome/test/data/pdf/na...
chrome/test/data/pdf/navigator_test.js:53: return;
On 2016/10/13 00:11:34, raymes wrote:
> Can we assert 
> navigatorDelegate.navigateInCurrentTabCalled
> navigatorDelegate.navigateInNewTabCalled
> navigatorDelegate.navigateInNewWindowCalled
> 
> are all false in this case?

That is not needed, because url is undefined iff there is no navigation.
That is also why I modified the delegate above to include a default value for
url.

[robwu, 2016-10-14 09:28:17]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2016-10-14 09:28:27]

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-10-14 10:13:43]

Description was changed from

==========
Add check for file:-navigations from PDFs

BUG=654279
TEST=./browser_tests --gtest_filter=PDFExtensionTest.Navigator
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add check for file:-navigations from PDFs

BUG=654279
TEST=./browser_tests --gtest_filter=PDFExtensionTest.Navigator
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-10-14 10:13:44]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-14 10:15:06]

Description was changed from

==========
Add check for file:-navigations from PDFs

BUG=654279
TEST=./browser_tests --gtest_filter=PDFExtensionTest.Navigator
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Add check for file:-navigations from PDFs

BUG=654279
TEST=./browser_tests --gtest_filter=PDFExtensionTest.Navigator
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/374249e767a68d8da073a4ed3a4f29236451174c
Cr-Commit-Position: refs/heads/master@{#425287}
==========

[commit-bot: I haz the power, 2016-10-14 10:15:07]

Patchset 2 (id:??) landed as
https://crrev.com/374249e767a68d8da073a4ed3a4f29236451174c
Cr-Commit-Position: refs/heads/master@{#425287}