[Reading List iOS] Display distilled URL instead of entry URL in omnibox

[Reading List iOS] Display distilled URL instead of entry URL in omnibox

When displaying an offline page, the URL must be the actual distilled
URL and not the Reading List entry URL (which can be a short URL like
goo.gl/...).

BUG=682666
Review-Url: https://codereview.chromium.org/2644293002
Cr-Commit-Position: refs/heads/master@{#445077}
Committed: https://chromium.googlesource.com/chromium/src/+/457f401daa5e0667546ab2a96afea821c6e5d88f

[Olivier, 2017-01-20 09:13:38]

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

[gambard, 2017-01-20 14:49:35]

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

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils.h:19: GURL
OfflineURLForPath(const base::FilePath& distilled_path,
Can you change the function name? You obscure the |entry_url| and |virtual_url|
part.

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils.h:29: GURL
EntryURLForOfflineURL(const GURL& offline_url);
Can you please set the same function order in .h and .cc?

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

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:24: //
chrome://offline/MD5/page.html?virtualURL=encorded%20URL;
Update comment

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:25:
TEST(OfflineURLUtilsTest, OfflineURLForPathWithEntryURLTest) {
s/OfflineURLForPathWithEntryURLTest/OfflineURLForPathWithEntryAndVirtualURLTest

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:38: // Checks the
distilled URL for the page is chrome://offline/MD5/page.html;
Update this comment.
You are checking that the entry url is the distilled url if there is no
?entryURL=... or I misunderstood the test?

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:46: //
chrome://offline/MD5/page.html?entryURL=encorded%20URL
WDYT of:
// Checks that the entry URL and the virtual URL of the page are the same if
"?virtualURL=" is not present.
or 
// Checks that the entry URL and the virtual URL of the page are the same if the
virtualURL parameter is not present.

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:57: //
chrome://offline/MD5/page.html?virtualURL=encorded%20URL
WDYT:
// Checks that if the only parameter present is virtualURL, the entry URL is the
offline url and the virtual url is the value of the virtualURL parameter.

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:68: //
chrome://offline/MD5/page.html?entryURL=...&virtualURL=...
WDYT:
// Checks that the entry URL and the virtual URL are equals to the value of
their corresponding parameter.

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_web_state_observer_unittest.mm
(right):

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_web_state_observer_unittest.mm:98:
GURL distilled_url = reading_list::OfflineURLForPath(
maybe remove the distilled_url as you are not using it?

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/ui/readi...
File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right):

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:97: _restored
= YES;
You will never reuse this object after restoring online URL?

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/ui/readi...
File ios/chrome/browser/ui/reading_list/offline_page_native_content_unittest.mm
(right):

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/offline_page_native_content_unittest.mm:30: 
Maybe add a test for the restoreOnlineURL through one of the public method? (The
best scenario would be to test all the implemented methods :) )

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/offline_page_native_content_unittest.mm:49:
TEST_F(OfflinePageNativeContentTest, BasicOfflinePageTestWithoutVirtualURL) {
Add comments

[jif, 2017-01-20 15:01:25]

lgtm

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

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils.h:19: GURL
OfflineURLForPath(const base::FilePath& distilled_path,
Consider explaining why |entry_url| can be different from |virtual_url|.

[Olivier, 2017-01-20 16:09:15]

Thanks

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

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils.h:19: GURL
OfflineURLForPath(const base::FilePath& distilled_path,
On 2017/01/20 15:01:25, jif wrote:
> Consider explaining why |entry_url| can be different from |virtual_url|.

Done.

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils.h:19: GURL
OfflineURLForPath(const base::FilePath& distilled_path,
On 2017/01/20 14:49:34, gambard wrote:
> Can you change the function name? You obscure the |entry_url| and
|virtual_url|
> part.

C++ function names don't always include parameters and distilled_path is the
only mandatory one.
I think the name reflects what the function do, give a chrome://offline URL that
allows to open |distilled_path|.

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils.h:29: GURL
EntryURLForOfflineURL(const GURL& offline_url);
On 2017/01/20 14:49:34, gambard wrote:
> Can you please set the same function order in .h and .cc?

Done.

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

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:24: //
chrome://offline/MD5/page.html?virtualURL=encorded%20URL;
On 2017/01/20 14:49:34, gambard wrote:
> Update comment

Done.

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:25:
TEST(OfflineURLUtilsTest, OfflineURLForPathWithEntryURLTest) {
On 2017/01/20 14:49:34, gambard wrote:
>
s/OfflineURLForPathWithEntryURLTest/OfflineURLForPathWithEntryAndVirtualURLTest

Done.

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:38: // Checks the
distilled URL for the page is chrome://offline/MD5/page.html;
On 2017/01/20 14:49:34, gambard wrote:
> Update this comment.
> You are checking that the entry url is the distilled url if there is no
> ?entryURL=... or I misunderstood the test?

Done.

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:46: //
chrome://offline/MD5/page.html?entryURL=encorded%20URL
On 2017/01/20 14:49:34, gambard wrote:
> WDYT of:
> // Checks that the entry URL and the virtual URL of the page are the same if
> "?virtualURL=" is not present.
> or 
> // Checks that the entry URL and the virtual URL of the page are the same if
the
> virtualURL parameter is not present.

Normalized all comments.

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:57: //
chrome://offline/MD5/page.html?virtualURL=encorded%20URL
On 2017/01/20 14:49:34, gambard wrote:
> WDYT:
> // Checks that if the only parameter present is virtualURL, the entry URL is
the
> offline url and the virtual url is the value of the virtualURL parameter.

Normalized all comments.

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:68: //
chrome://offline/MD5/page.html?entryURL=...&virtualURL=...
On 2017/01/20 14:49:34, gambard wrote:
> WDYT:
> // Checks that the entry URL and the virtual URL are equals to the value of
> their corresponding parameter.

Normalized all comments.

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
File ios/chrome/browser/reading_list/reading_list_web_state_observer_unittest.mm
(right):

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_web_state_observer_unittest.mm:98:
GURL distilled_url = reading_list::OfflineURLForPath(
On 2017/01/20 14:49:35, gambard wrote:
> maybe remove the distilled_url as you are not using it?

That is true :)
Done

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/ui/readi...
File ios/chrome/browser/ui/reading_list/offline_page_native_content.mm (right):

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/offline_page_native_content.mm:97: _restored
= YES;
On 2017/01/20 14:49:35, gambard wrote:
> You will never reuse this object after restoring online URL?

You should not, no.

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/ui/readi...
File ios/chrome/browser/ui/reading_list/offline_page_native_content_unittest.mm
(right):

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/offline_page_native_content_unittest.mm:30: 
On 2017/01/20 14:49:35, gambard wrote:
> Maybe add a test for the restoreOnlineURL through one of the public method?
(The
> best scenario would be to test all the implemented methods :) )

Done.

https://codereview.chromium.org/2644293002/diff/1/ios/chrome/browser/ui/readi...
ios/chrome/browser/ui/reading_list/offline_page_native_content_unittest.mm:49:
TEST_F(OfflinePageNativeContentTest, BasicOfflinePageTestWithoutVirtualURL) {
On 2017/01/20 14:49:35, gambard wrote:
> Add comments

Done.

[Olivier, 2017-01-20 16:43:26]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2017-01-20 16:43:27]

The patchset sent to the CQ was uploaded after l-g-t-m from jif@chromium.org
Link to the patchset: https://codereview.chromium.org/2644293002/#ps20001
(title: "feedback")

[commit-bot: I haz the power, 2017-01-20 16:43:38]

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-01-20 16:53:31]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484930606993420,
"parent_rev": "805390278cc4168505bc11540830707417a14244", "commit_rev":
"457f401daa5e0667546ab2a96afea821c6e5d88f"}

[commit-bot: I haz the power, 2017-01-20 16:54:02]

Description was changed from

==========
[Reading List iOS] Display distilled URL instead of entry URL in omnibox

When displaying an offline page, the URL must be the actual distilled
URL and not the Reading List entry URL (which can be a short URL like
goo.gl/...).

BUG=682666
==========

to

==========
[Reading List iOS] Display distilled URL instead of entry URL in omnibox

When displaying an offline page, the URL must be the actual distilled
URL and not the Reading List entry URL (which can be a short URL like
goo.gl/...).

BUG=682666

Review-Url: https://codereview.chromium.org/2644293002
Cr-Commit-Position: refs/heads/master@{#445077}
Committed:
https://chromium.googlesource.com/chromium/src/+/457f401daa5e0667546ab2a96afe...
==========

[commit-bot: I haz the power, 2017-01-20 16:54:03]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/457f401daa5e0667546ab2a96afe...