[Offline pages] Expose offline pages URL related methods in java code.

[Offline pages] Expose offline pages URL related methods in java code.

This patch exposes necessary 2 methods for handling URLs related to offline pages with tests.

BUG=565219
Committed: https://crrev.com/8cb6c4f240200ba511423396646aafffce6d8a62
Cr-Commit-Position: refs/heads/master@{#368096}

[fgorski, 2015-12-15 20:01:28]

fgorski@chromium.org changed reviewers:
+ jianli@chromium.org

[fgorski, 2015-12-15 20:01:28]

PTAL. This is a follow up patch to tab_android refactoring, which also handles
permission prompt when opening an offline page in M (without storage permission)

[jianli, 2015-12-16 01:41:23]

https://codereview.chromium.org/1524293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1524293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:343:
* Gets the offline URL of based on the online url of an offline page, if one
exists.
Gets the offline URL of an offline page that is saved for the online URL.

https://codereview.chromium.org/1524293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:345:
* @return URL pointing to the offline copy or <code>null</code> in none exists.
in => if

https://codereview.chromium.org/1524293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:353:
* Checks whether a provided url belongs to an offline page.
Returns true if an offline page stored in the provided offline URL exists.

[fgorski, 2015-12-16 22:12:58]

Addressed

https://codereview.chromium.org/1524293002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1524293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:343:
* Gets the offline URL of based on the online url of an offline page, if one
exists.
On 2015/12/16 01:41:23, jianli wrote:
> Gets the offline URL of an offline page that is saved for the online URL.

Done.

https://codereview.chromium.org/1524293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:345:
* @return URL pointing to the offline copy or <code>null</code> in none exists.
On 2015/12/16 01:41:23, jianli wrote:
> in => if

Done.

https://codereview.chromium.org/1524293002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:353:
* Checks whether a provided url belongs to an offline page.
On 2015/12/16 01:41:23, jianli wrote:
> Returns true if an offline page stored in the provided offline URL exists.

Done.

[jianli, 2015-12-16 23:42:41]

lgtm

[fgorski, 2015-12-17 20:53:49]

fgorski@chromium.org changed reviewers:
+ newt@chromium.org

[fgorski, 2015-12-17 20:53:49]

newt@, could you please take a look at the external navigation code?

I was trying to add the test for it, but since it requires saving a page
offline, which is more of a Chrome Activity based test, it was not easy to add
to the ExternalNavigationHandlerTest.

Do you think it would be ok to do it in OfflinePageBridge test? Please suggest
something that would work.

[newt (away), 2015-12-21 18:41:17]

On 2015/12/17 20:53:49, fgorski wrote:
> newt@, could you please take a look at the external navigation code?
> 
> I was trying to add the test for it, but since it requires saving a page
> offline, which is more of a Chrome Activity based test, it was not easy to add
> to the ExternalNavigationHandlerTest.
> 
> Do you think it would be ok to do it in OfflinePageBridge test? Please suggest
> something that would work.

It would be nice to test this in ExternalNavigationHandlerTest, since that's the
centralized place for testing the (very complicated) ExternalNavigationHandler
logic.

Ideally, the test would:
 - Load the native library. To do this, make ExternalNavigationHandlerTest
extend NativeLibraryTestBase, which is a subclass of InstrumentationTestCase.
 - Call some method in OfflinePageBridge to save a dummy webpage offline.
Currently it looks like OfflinePageBridge.savePage() takes a WebContents. You'd
either need to create a WebContents in the test, or add an alternative method
OfflinePageBridge.savePageForTests() that takes an HTML string, or something
like that.
 - Check that loading an offline URL doesn't show an intent picker by mimicking
other tests in ExternalNavigationHandlerTest

[newt (away), 2015-12-21 18:41:28]

https://codereview.chromium.org/1524293002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
(right):

https://codereview.chromium.org/1524293002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:173:
// If accessing a file URL, which points to an offline copy of an offline page,
there
Does this only apply to file:// URLs? If so, I'd move this check inside the "if
(params.getUrl().startsWith("file:")" block, since this is a fairly expensive
check compared to a string prefix check.

https://codereview.chromium.org/1524293002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:178:
if (offlinePageBridge.isOfflinePageUrl(params.getUrl())) {
Perhaps isOfflinePageUrl() should be a static method so you don't have to create
and destroy the bridge object here.

[fgorski, 2015-12-22 23:02:54]

Description was changed from

==========
[Offline pages] Removing permission prompt shown when opening offline page

This patch makes ExternalNavigationHandler aware of Offline pages and
right before a check for handling file:// URLs, it makes sure that
offline pages are handled without a need to override navigation.

It also exposes necessary 2 methods for handling URLs related to offline
pages with tests.

BUG=565875
==========

to

==========
[Offline pages] Expose offline pages URL related methods in java code.

This patch has been trimmed in scope. ExternalNavigationHandler code has been
moved.

It also exposes necessary 2 methods for handling URLs related to offline
pages with tests.

BUG=565875
==========

[fgorski, 2015-12-22 23:07:27]

Description was changed from

==========
[Offline pages] Expose offline pages URL related methods in java code.

This patch has been trimmed in scope. ExternalNavigationHandler code has been
moved.

It also exposes necessary 2 methods for handling URLs related to offline
pages with tests.

BUG=565875
==========

to

==========
[Offline pages] Expose offline pages URL related methods in java code.

This patch exposes necessary 2 methods for handling URLs related to offline
pages with tests.

BUG=565219
==========

[fgorski, 2015-12-22 23:10:20]

On 2015/12/21 18:41:28, newt wrote:
>
https://codereview.chromium.org/1524293002/diff/40001/chrome/android/java/src...
> File
>
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java
> (right):
> 
>
https://codereview.chromium.org/1524293002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:173:
> // If accessing a file URL, which points to an offline copy of an offline
page,
> there
> Does this only apply to file:// URLs? If so, I'd move this check inside the
"if
> (params.getUrl().startsWith("file:")" block, since this is a fairly expensive
> check compared to a string prefix check.
> 
>
https://codereview.chromium.org/1524293002/diff/40001/chrome/android/java/src...
>
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationHandler.java:178:
> if (offlinePageBridge.isOfflinePageUrl(params.getUrl())) {
> Perhaps isOfflinePageUrl() should be a static method so you don't have to
create
> and destroy the bridge object here.

Newton, I moved the ExternalNavigationHandler code to a separate patch:
https://codereview.chromium.org/1542943006/

I think I addressed your concerns there, but I had to relax requirements a bit.
Let's continue the discussion in the other issue.

I'll check this one in.

Thanks.

[fgorski, 2016-01-05 22:35:39]

Newton, could you take a look at the tests? (I think you are one of the owners
for tests)

[newt (away), 2016-01-05 22:40:41]

java code lgtm

https://codereview.chromium.org/1524293002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java
(right):

https://codereview.chromium.org/1524293002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java:161:
public void testIsOfflniePageUrl() throws Exception {
s/Offlnie/Offline/

[jianli, 2016-01-06 00:37:01]

lgtm

[fgorski, 2016-01-07 17:11:52]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-01-07 17:11:53]

The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org,
newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1524293002/#ps120001
(title: "Setting a flag to enable offline pages in test")

[commit-bot: I haz the power, 2016-01-07 17:12:25]

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

[fgorski, 2016-01-07 17:12:49]

Addressed and found the issue with tests.

Enabling the feature helps apparently.

https://codereview.chromium.org/1524293002/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java
(right):

https://codereview.chromium.org/1524293002/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java:161:
public void testIsOfflniePageUrl() throws Exception {
On 2016/01/05 22:40:41, newt wrote:
> s/Offlnie/Offline/

Done.

[commit-bot: I haz the power, 2016-01-07 17:17:29]

Description was changed from

==========
[Offline pages] Expose offline pages URL related methods in java code.

This patch exposes necessary 2 methods for handling URLs related to offline
pages with tests.

BUG=565219
==========

to

==========
[Offline pages] Expose offline pages URL related methods in java code.

This patch exposes necessary 2 methods for handling URLs related to offline
pages with tests.

BUG=565219
==========

[commit-bot: I haz the power, 2016-01-07 17:17:30]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-01-07 17:18:32]

Description was changed from

==========
[Offline pages] Expose offline pages URL related methods in java code.

This patch exposes necessary 2 methods for handling URLs related to offline
pages with tests.

BUG=565219
==========

to

==========
[Offline pages] Expose offline pages URL related methods in java code.

This patch exposes necessary 2 methods for handling URLs related to offline
pages with tests.

BUG=565219

Committed: https://crrev.com/8cb6c4f240200ba511423396646aafffce6d8a62
Cr-Commit-Position: refs/heads/master@{#368096}
==========

[commit-bot: I haz the power, 2016-01-07 17:18:33]

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