[Offline Pages on the NTP] Mark "file://" URLs as offline-available

[Offline Pages on the NTP] Mark "file://" URLs as offline-available

If we're showing offline-available indications, then "file://" links should be treated as available

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

[Marc Treib, 2016-01-12 10:54:10]

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

[fgorski, 2016-01-12 17:21:28]

Overall idea looks good. I have a suggestion to simplify things a bit though. 

Newton, does it make sense?

https://codereview.chromium.org/1579153002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/1579153002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:468:
private boolean isLocalUrl(String url) {
I think you can avoid the URI and exception handling if you do

return url.startsWith("file:");

Code is simpler and the practice is used by the clank team, e.g. in
ExternalNavigationHandler:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...

Even thou it is a one liner I would keep it in this method.

[newt (away), 2016-01-12 20:30:31]

https://codereview.chromium.org/1579153002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/1579153002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:468:
private boolean isLocalUrl(String url) {
On 2016/01/12 17:21:28, fgorski wrote:
> I think you can avoid the URI and exception handling if you do
> 
> return url.startsWith("file:");
> 
> Code is simpler and the practice is used by the clank team, e.g. in
> ExternalNavigationHandler:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> 
> Even thou it is a one liner I would keep it in this method.

Alternatively, you can use android.net.Uri, which is much lighter-weight than
java.net.URI. In particular, android.net.Uri does lazy parsing, so
Uri.parse(pageUrl).getScheme() is quite cheap.

I think either option is fine. I'd avoid java.net.URI though since it's heavier
weight and requires the exception handling boilerplate.

[Marc Treib, 2016-01-13 10:46:26]

Patchset #2 (id:20001) has been deleted

[Marc Treib, 2016-01-13 11:12:10]

https://codereview.chromium.org/1579153002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java
(right):

https://codereview.chromium.org/1579153002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPage.java:468:
private boolean isLocalUrl(String url) {
On 2016/01/12 20:30:31, newt wrote:
> On 2016/01/12 17:21:28, fgorski wrote:
> > I think you can avoid the URI and exception handling if you do
> > 
> > return url.startsWith("file:");
> > 
> > Code is simpler and the practice is used by the clank team, e.g. in
> > ExternalNavigationHandler:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/jav...
> > 
> > Even thou it is a one liner I would keep it in this method.
> 
> Alternatively, you can use android.net.Uri, which is much lighter-weight than
> java.net.URI. In particular, android.net.Uri does lazy parsing, so
> Uri.parse(pageUrl).getScheme() is quite cheap.
> 
> I think either option is fine. I'd avoid java.net.URI though since it's
heavier
> weight and requires the exception handling boilerplate.

Thanks for the suggestions! I've gone with Uri.parse - it's not any more code,
and I'm not a fan of manually doing string stuff on URLs (though in this case it
wouldn't really be a problem).

[fgorski, 2016-01-13 15:20:24]

lgtm

[newt (away), 2016-01-13 18:53:10]

lgtm

[Marc Treib, 2016-01-14 09:25:41]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-01-14 09:25:41]

The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org,
fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1579153002/#ps50001
(title: "rebase")

[commit-bot: I haz the power, 2016-01-14 09:26:10]

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

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

Committed patchset #3 (id:50001)

[commit-bot: I haz the power, 2016-01-14 10:08:46]

Description was changed from

==========
[Offline Pages on the NTP] Mark "file://" URLs as offline-available

If we're showing offline-available indications, then "file://" links should be
treated as available

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

to

==========
[Offline Pages on the NTP] Mark "file://" URLs as offline-available

If we're showing offline-available indications, then "file://" links should be
treated as available

BUG=565219

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

[commit-bot: I haz the power, 2016-01-14 10:08:47]

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