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

ios/chrome/browser/reading_list/offline_url_utils.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef IOS_CHROME_BROWSER_READING_LIST_OFFLINE_URL_UTILS_H_
 #define IOS_CHROME_BROWSER_READING_LIST_OFFLINE_URL_UTILS_H_
 
 #include <string>
 
 #include "base/files/file_path.h"
 #include "base/strings/string16.h"
 #include "url/gurl.h"
 
 namespace reading_list {
 
 // The distilled URL chrome://offline/... that will load the file at |path|.
-GURL DistilledURLForPath(const base::FilePath& path, const GURL& virtual_url);
+// |entry_url| is the URL of the ReadingListEntry.
+// |virtual_url| is the URL to display in the omnibox.
+GURL OfflineURLForPath(const base::FilePath& distilled_path,

[gambard, 2017/01/20 14:49:34]

Can you change the function name? You obscure the |entry_url| and |virtual_url|
part.

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

Consider explaining why |entry_url| can be different from |virtual_url|.

[Olivier, 2017/01/20 16:09:14]

Done.

[Olivier, 2017/01/20 16:09:14]

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|.

+                       const GURL& entry_url,
+                       const GURL& virtual_url);
 
-// If |distilled_url| has a query "virtualURL" query params that is a URL,
-// returns it. If not, return |distilled_url|
-GURL VirtualURLForDistilledURL(const GURL& distilled_url);
+// If |offline_url| has a "virtualURL" query params that is a URL, returns it.
+// If not, return |EntryURLForOfflineURL(|offline_url|)|
+GURL VirtualURLForOfflineURL(const GURL& offline_url);
+
+// If |offline_url| has a "entryURL" query params that is a URL, returns it.
+// If not, return |offline_url|
+GURL EntryURLForOfflineURL(const GURL& offline_url);

[gambard, 2017/01/20 14:49:34]

Can you please set the same function order in .h and .cc?

[Olivier, 2017/01/20 16:09:14]

Done.

 
 // The file URL pointing to the local file to load to display |distilled_url|.
 // If |resources_root_url| is not nullptr, it is set to a file URL to the
 // directory conatining all the resources needed by |distilled_url|.
 // |offline_path| is the root path to the directory containing offline files.
 GURL FileURLForDistilledURL(const GURL& distilled_url,
                             const base::FilePath& offline_path,
                             GURL* resources_root_url);
 
 // Returns whether the URL points to a chrome offline URL.
 bool IsOfflineURL(const GURL& url);
 
 }  // namespace reading_list
 
 #endif  // IOS_CHROME_BROWSER_READING_LIST_OFFLINE_URL_UTILS_H_