Create offline_url_utils

Create offline_url_utils

This utils will handle all the conversions between offline file path
and URLs.

BUG=664124
Committed: https://crrev.com/5ab336ce9b177b0e70e9175633b8ebf07bff3a21
Cr-Commit-Position: refs/heads/master@{#432541}

[Olivier, 2016-11-16 10:30:54]

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

[gambard, 2016-11-16 12:44:20]

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/offline_url_utils.h (right):

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.h:16: base::FilePath
OfflineRootDirectoryPath(const base::FilePath& profile_path);
Add comments for the argument please.

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.h:21: const base::FilePath&
profile_path,
Same

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.h:26: base::FilePath
OfflinePageAbsolutePath(const base::FilePath& profile_path,
Same

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.h:29: // The relative path to
the distilled page. The result is relative to
Please add a logical link between "distilled page" and |url|. What about:
// The relative path to the offline webpage for the URL.

You can also add "The file may not exist", as above.

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.h:39: // The file URL pointing
to the local file to load whan displaying
s/whan/when/

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.h:40: // |distilled_url|. If
|resources_root_url| is not nullptr, it is set to a file
Maybe add where we are displaying |distilled_url|? To whom?

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/offline_url_utils_unittest.cc (right):

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:68: // Test the
resource root for chrome://offline/MD5/page.html is
s/Test/Checks/

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:74:
EXPECT_FALSE(file_url.is_valid());
Add empty lines between different tests please.

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/url_downloader_unittest.cc (right):

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/url_downloader_unittest.cc:62:
reading_list::OfflinePageAbsolutePath(base_directory_, url));
base_directory_ or base_path_?

[Olivier, 2016-11-16 13:36:28]

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/offline_url_utils.h (right):

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.h:16: base::FilePath
OfflineRootDirectoryPath(const base::FilePath& profile_path);
On 2016/11/16 12:44:20, gambard wrote:
> Add comments for the argument please.

Done.

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.h:21: const base::FilePath&
profile_path,
On 2016/11/16 12:44:19, gambard wrote:
> Same

Done.

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.h:26: base::FilePath
OfflinePageAbsolutePath(const base::FilePath& profile_path,
On 2016/11/16 12:44:20, gambard wrote:
> Same

Done.

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.h:29: // The relative path to
the distilled page. The result is relative to
On 2016/11/16 12:44:19, gambard wrote:
> Please add a logical link between "distilled page" and |url|. What about:
> // The relative path to the offline webpage for the URL.
> 
> You can also add "The file may not exist", as above.

Done.

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.h:39: // The file URL pointing
to the local file to load whan displaying
On 2016/11/16 12:44:20, gambard wrote:
> s/whan/when/

Done.

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils.h:40: // |distilled_url|. If
|resources_root_url| is not nullptr, it is set to a file
On 2016/11/16 12:44:19, gambard wrote:
> Maybe add where we are displaying |distilled_url|? To whom?

I don't think this utility function needs to know where it is displayed.

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/offline_url_utils_unittest.cc (right):

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:68: // Test the
resource root for chrome://offline/MD5/page.html is
On 2016/11/16 12:44:20, gambard wrote:
> s/Test/Checks/

Done.

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/offline_url_utils_unittest.cc:74:
EXPECT_FALSE(file_url.is_valid());
On 2016/11/16 12:44:20, gambard wrote:
> Add empty lines between different tests please.

Done.

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
File ios/chrome/browser/reading_list/url_downloader_unittest.cc (right):

https://codereview.chromium.org/2506993002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/url_downloader_unittest.cc:62:
reading_list::OfflinePageAbsolutePath(base_directory_, url));
On 2016/11/16 12:44:20, gambard wrote:
> base_directory_ or base_path_?

Actually MockURLDownloader is friend of URLDownloader, so using base_directory_.

[gambard, 2016-11-16 15:32:50]

lgtm

[Olivier, 2016-11-16 15:58:03]

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2016-11-16 15:58:53]

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, 2016-11-16 17:40:26]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-16 17:49:08]

Description was changed from

==========
Create offline_url_utils

This utils will handle all the conversions between offline file path
and URLs.

BUG=664124
==========

to

==========
Create offline_url_utils

This utils will handle all the conversions between offline file path
and URLs.

BUG=664124

Committed: https://crrev.com/5ab336ce9b177b0e70e9175633b8ebf07bff3a21
Cr-Commit-Position: refs/heads/master@{#432541}
==========

[commit-bot: I haz the power, 2016-11-16 17:49:08]

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