[iOS Reading List] Only allow offline URL when the offline file exists.

[iOS Reading List] Only allow offline URL when the offline file exists.

If the users taps an offline url that does not match an entry, an error
message should be displayed.

BUG=703393
Review-Url: https://codereview.chromium.org/2800673002
Cr-Commit-Position: refs/heads/master@{#462874}
Committed: https://chromium.googlesource.com/chromium/src/+/5c861c2d5bddd50d63c154c50d73ec9c3b308e33

[Olivier, 2017-04-05 11:26:20]

olivierrobin@chromium.org changed reviewers:
+ jif@chromium.org

[Olivier, 2017-04-05 11:26:26]

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

[commit-bot: I haz the power, 2017-04-05 11:26:37]

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, 2017-04-05 11:49:09]

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

[commit-bot: I haz the power, 2017-04-05 11:49:10]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Olivier, 2017-04-05 12:51:41]

Description was changed from

==========
[iOS Reaidng List] Only allow offline URL when the offline file exists.

If the users taps an offline url that does not match an entry, an error
message should be displayed.

BUG=703393
==========

to

==========
[iOS Reading List] Only allow offline URL when the offline file exists.

If the users taps an offline url that does not match an entry, an error
message should be displayed.

BUG=703393
==========

[Olivier, 2017-04-05 13:00:35]

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

[commit-bot: I haz the power, 2017-04-05 13:00:48]

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, 2017-04-05 13:10:44]

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

[commit-bot: I haz the power, 2017-04-05 13:10:45]

Dry run: This issue passed the CQ dry run.

[jif, 2017-04-05 13:19:32]

https://codereview.chromium.org/2800673002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/offline_url_utils.cc (right):

https://codereview.chromium.org/2800673002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils.cc:100: return url ==
reading_list::OfflineURLForPath(entry->DistilledPath(),
when can this test be false?

https://codereview.chromium.org/2800673002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/offline_url_utils.h (right):

https://codereview.chromium.org/2800673002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils.h:21: // from |entry_url| is
the distillation was done after a redirection.
s/is/if/

https://codereview.chromium.org/2800673002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils.h:22: // |distilled_path|,
|entry_url| and |virtual_url| are required and cannot be
s/cannot/must not/

[Olivier, 2017-04-05 14:30:54]

Thanks

PTAL

https://codereview.chromium.org/2800673002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/offline_url_utils.cc (right):

https://codereview.chromium.org/2800673002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils.cc:100: return url ==
reading_list::OfflineURLForPath(entry->DistilledPath(),
On 2017/04/05 13:19:32, jif wrote:
> when can this test be false?

If by chance a user type a URL that pass all the other tests.

https://codereview.chromium.org/2800673002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/offline_url_utils.h (right):

https://codereview.chromium.org/2800673002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils.h:21: // from |entry_url| is
the distillation was done after a redirection.
On 2017/04/05 13:19:32, jif wrote:
> s/is/if/

Done.

https://codereview.chromium.org/2800673002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils.h:22: // |distilled_path|,
|entry_url| and |virtual_url| are required and cannot be
On 2017/04/05 13:19:32, jif wrote:
> s/cannot/must not/

Done.

[jif, 2017-04-05 14:37:04]

lgtm

https://codereview.chromium.org/2800673002/diff/40001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/offline_url_utils.cc (right):

https://codereview.chromium.org/2800673002/diff/40001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.cc:100: return url ==
reading_list::OfflineURLForPath(entry->DistilledPath(),
Add a comment that looks like:

// Can be false if the offline URL was typed directly in the omnibox.

[Olivier, 2017-04-05 15:21:00]

olivierrobin@chromium.org changed reviewers:
+ eugenebut@chromium.org

[Olivier, 2017-04-07 08:19:22]

olivierrobin@chromium.org changed reviewers:
+ marq@chromium.org

[Olivier, 2017-04-07 08:19:23]

Hi Eugene, marq, can you take a look?

[marq (ping after 24h), 2017-04-07 09:50:37]

lgtm

[Eugene But (OOO till 7-30), 2017-04-07 15:17:52]

lgtm

https://codereview.chromium.org/2800673002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/offline_url_utils.cc (right):

https://codereview.chromium.org/2800673002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.cc:93: if
(!entry_url.is_valid() || !model || !model->loaded()) {
Is it ok to pass null model? Please document in header's comments

https://codereview.chromium.org/2800673002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.cc:101: // the tests above but
still is not the correct one.
What does "not the correct one" mean? URL which does not point to HTTP server
that is online?

https://codereview.chromium.org/2800673002/diff/60001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2800673002/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:3034: [self
hasControllerForURL:url]) {
Should |[self hasControllerForURL:url]| be inside  |else if (url_host ==
kChromeUIOfflineHost)| block? Or maybe instead of |if| check you want to use
DCHECK?

[Olivier, 2017-04-07 15:44:27]

Thanks

https://codereview.chromium.org/2800673002/diff/60001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/offline_url_utils.cc (right):

https://codereview.chromium.org/2800673002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.cc:93: if
(!entry_url.is_valid() || !model || !model->loaded()) {
On 2017/04/07 15:17:52, Eugene But wrote:
> Is it ok to pass null model? Please document in header's comments

Done.

https://codereview.chromium.org/2800673002/diff/60001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.cc:101: // the tests above but
still is not the correct one.
On 2017/04/07 15:17:52, Eugene But wrote:
> What does "not the correct one" mean? URL which does not point to HTTP server
> that is online?

The one that is returned by |OfflineURLForPath|.
Main issue is to point to another offline file or display a different
virtualURL.
 But as user has to type it, it is unlikely.

https://codereview.chromium.org/2800673002/diff/60001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2800673002/diff/60001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:3034: [self
hasControllerForURL:url]) {
On 2017/04/07 15:17:52, Eugene But wrote:
> Should |[self hasControllerForURL:url]| be inside  |else if (url_host ==
> kChromeUIOfflineHost)| block? Or maybe instead of |if| check you want to use
> DCHECK?

This is possible if the user taps a wrong URL.
In that case, we want to display a "Page unavailable" error handled in the else
case.

[Olivier, 2017-04-07 15:44:34]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-04-07 15:44:34]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org,
eugenebut@chromium.org, marq@chromium.org
Link to the patchset: https://codereview.chromium.org/2800673002/#ps80001
(title: "Update comments")

[commit-bot: I haz the power, 2017-04-07 15:44:56]

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, 2017-04-07 15:56:50]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491579874235630,
"parent_rev": "c528e3809ff5494f046b339937727bad8aae87c5", "commit_rev":
"5c861c2d5bddd50d63c154c50d73ec9c3b308e33"}

[commit-bot: I haz the power, 2017-04-07 15:57:33]

Description was changed from

==========
[iOS Reading List] Only allow offline URL when the offline file exists.

If the users taps an offline url that does not match an entry, an error
message should be displayed.

BUG=703393
==========

to

==========
[iOS Reading List] Only allow offline URL when the offline file exists.

If the users taps an offline url that does not match an entry, an error
message should be displayed.

BUG=703393

Review-Url: https://codereview.chromium.org/2800673002
Cr-Commit-Position: refs/heads/master@{#462874}
Committed:
https://chromium.googlesource.com/chromium/src/+/5c861c2d5bddd50d63c154c50d73...
==========

[commit-bot: I haz the power, 2017-04-07 15:57:35]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/5c861c2d5bddd50d63c154c50d73...