Use Distilled path instead of DistilledURL.

Use Distilled path instead of DistilledURL.

It does not make sense to store the sandbox path for the offline files.
Instead, store the path relative to the offline folder.
Also rename method to make clear distinction between absolute file path
and relative path that can be used in files path or URLs.

DistilledURL change semantics.
It was the file URL to the local file, it is now a URL that can be loaded to load the file.

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

[Olivier, 2016-11-10 15:40:13]

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

[jif-google, 2016-11-10 15:48:24]

jif@google.com changed reviewers:
+ jif@google.com

[jif-google, 2016-11-10 15:48:25]

Great, this bothered me as well.

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

https://codereview.chromium.org/2491383002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_download_service.h:83: const
base::FilePath& distilled_url,
s/distilled_url/distilled_path/

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

https://codereview.chromium.org/2491383002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_entry.cc:10: // URL used to open
offline pages.
To be more exact:
// Scheme and prefix used to open offline pages.

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

https://codereview.chromium.org/2491383002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_entry.h:41: // The local file URL
for the distilled version of the page. This should only
s/file URL/filepath/

https://codereview.chromium.org/2491383002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_entry.h:61: void
SetDistilledPath(const base::FilePath& url);
s/url/path/

Also, the comment needs to be updated.

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

https://codereview.chromium.org/2491383002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_model_storage_defaults.mm:43:
base::FilePath(base::SysNSStringToUTF8(distilled_path));
[distilled_path UTF8String]


(I kid)

[jif-google, 2016-11-10 15:51:35]

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

https://codereview.chromium.org/2491383002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_entry.cc:10: // URL used to open
offline pages.
On 2016/11/10 15:48:24, jif-google wrote:
> To be more exact:
> // Scheme and prefix used to open offline pages.

I'm wrong, "chrome://offline/" is actually a URL.

[gambard, 2016-11-10 16:07:08]

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

https://codereview.chromium.org/2491383002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_entry.h:45: const GURL
DistilledURL() const;
Are we still using this?

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

https://codereview.chromium.org/2491383002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_entry_unittest.cc:45:
TEST(ReadingListEntry, DistilledPath) {
Maybe add a test for DistilledURL?

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

https://codereview.chromium.org/2491383002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_model_storage_unittest.mm:88:
EXPECT_EQ(GURL("chrome://offline/distilled/page.html"),
Test the FilePath?
Which one will we be using most? FilePath or URL?

[Olivier, 2016-11-10 16:49:10]

Thanks

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

https://codereview.chromium.org/2491383002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_download_service.h:83: const
base::FilePath& distilled_url,
On 2016/11/10 15:48:24, jif-google wrote:
> s/distilled_url/distilled_path/

Done.

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

https://codereview.chromium.org/2491383002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_entry.h:41: // The local file URL
for the distilled version of the page. This should only
On 2016/11/10 15:48:24, jif-google wrote:
> s/file URL/filepath/

Done.

https://codereview.chromium.org/2491383002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_entry.h:45: const GURL
DistilledURL() const;
On 2016/11/10 16:07:08, gambard wrote:
> Are we still using this?

Yes, to load the page. The meaninq is different.
It was file://path/to/the/file
and now, it is chrome://offline/ID/page.html

https://codereview.chromium.org/2491383002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_entry.h:61: void
SetDistilledPath(const base::FilePath& url);
On 2016/11/10 15:48:24, jif-google wrote:
> s/url/path/
> 
> Also, the comment needs to be updated.

Done.

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

https://codereview.chromium.org/2491383002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_entry_unittest.cc:45:
TEST(ReadingListEntry, DistilledPath) {
On 2016/11/10 16:07:08, gambard wrote:
> Maybe add a test for DistilledURL?

I test DistilledURL in the DistilledPath test. Do you think there should be a
separated test?

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

https://codereview.chromium.org/2491383002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_model_storage_unittest.mm:88:
EXPECT_EQ(GURL("chrome://offline/distilled/page.html"),
On 2016/11/10 16:07:08, gambard wrote:
> Test the FilePath?
> Which one will we be using most? FilePath or URL?

The storage will store the path.
The loader will load the URL.

Here, the most relevant test is on FilePath

[Olivier, 2016-11-10 16:49:48]

Description was changed from

==========
Use Distilled path instead of DistilledURL.

It does not make sense to store the sandbox path for the offline files.
Instead, store the path relative to the offline folder.
Also rename method to make clear distinction between absolute file path
and relative path that can be used in files path or URLs.

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

to

==========
Use Distilled path instead of DistilledURL.

It does not make sense to store the sandbox path for the offline files.
Instead, store the path relative to the offline folder.
Also rename method to make clear distinction between absolute file path
and relative path that can be used in files path or URLs.

DistilledURL change semantics.
It was the file URL to the local file, it is now a URL that can be loaded to
load the file.

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

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

The CQ bit was checked by olivierrobin@chromium.org

[commit-bot: I haz the power, 2016-11-10 16:50:05]

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

[commit-bot: I haz the power, 2016-11-10 16:50:06]

This CL has an open dependency (Issue 2496553002 Patch 20001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[gambard, 2016-11-10 16:54:35]

lgtm with question

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

https://codereview.chromium.org/2491383002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_entry_unittest.cc:45:
TEST(ReadingListEntry, DistilledPath) {
On 2016/11/10 16:49:10, Olivier Robin wrote:
> On 2016/11/10 16:07:08, gambard wrote:
> > Maybe add a test for DistilledURL?
> 
> I test DistilledURL in the DistilledPath test. Do you think there should be a
> separated test?

No, it is OK I did not see it. Maybe update the test name then something like
"DistilledPathAndURL"?

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

https://codereview.chromium.org/2491383002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_unittest.cc:283:
EXPECT_EQ(GURL("chrome://offline/distilled/page.html"), entry.DistilledURL());
DistilledURL of DistilledPath?

[Olivier, 2016-11-14 16:43:23]

Done, thanks

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

https://codereview.chromium.org/2491383002/diff/1/ios/chrome/browser/reading_...
ios/chrome/browser/reading_list/reading_list_entry_unittest.cc:45:
TEST(ReadingListEntry, DistilledPath) {
On 2016/11/10 16:54:35, gambard wrote:
> On 2016/11/10 16:49:10, Olivier Robin wrote:
> > On 2016/11/10 16:07:08, gambard wrote:
> > > Maybe add a test for DistilledURL?
> > 
> > I test DistilledURL in the DistilledPath test. Do you think there should be
a
> > separated test?
> 
> No, it is OK I did not see it. Maybe update the test name then something like
> "DistilledPathAndURL"?

Done.

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

https://codereview.chromium.org/2491383002/diff/20001/ios/chrome/browser/read...
ios/chrome/browser/reading_list/reading_list_model_unittest.cc:283:
EXPECT_EQ(GURL("chrome://offline/distilled/page.html"), entry.DistilledURL());
On 2016/11/10 16:54:35, gambard wrote:
> DistilledURL of DistilledPath?

Done.

[Olivier, 2016-11-14 16:43:28]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2016-11-14 16:43:28]

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

[commit-bot: I haz the power, 2016-11-14 16:43:58]

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-14 16:54:12]

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

[commit-bot: I haz the power, 2016-11-14 16:54:13]

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

[Olivier, 2016-11-14 17:08:29]

The CQ bit was checked by olivierrobin@chromium.org

[Olivier, 2016-11-14 17:08:29]

The patchset sent to the CQ was uploaded after l-g-t-m from gambard@chromium.org
Link to the patchset: https://codereview.chromium.org/2491383002/#ps60001
(title: "fix compilation")

[commit-bot: I haz the power, 2016-11-14 17:09:15]

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-14 19:54:33]

Description was changed from

==========
Use Distilled path instead of DistilledURL.

It does not make sense to store the sandbox path for the offline files.
Instead, store the path relative to the offline folder.
Also rename method to make clear distinction between absolute file path
and relative path that can be used in files path or URLs.

DistilledURL change semantics.
It was the file URL to the local file, it is now a URL that can be loaded to
load the file.

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

to

==========
Use Distilled path instead of DistilledURL.

It does not make sense to store the sandbox path for the offline files.
Instead, store the path relative to the offline folder.
Also rename method to make clear distinction between absolute file path
and relative path that can be used in files path or URLs.

DistilledURL change semantics.
It was the file URL to the local file, it is now a URL that can be loaded to
load the file.

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

[commit-bot: I haz the power, 2016-11-14 19:54:34]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-14 20:10:27]

Description was changed from

==========
Use Distilled path instead of DistilledURL.

It does not make sense to store the sandbox path for the offline files.
Instead, store the path relative to the offline folder.
Also rename method to make clear distinction between absolute file path
and relative path that can be used in files path or URLs.

DistilledURL change semantics.
It was the file URL to the local file, it is now a URL that can be loaded to
load the file.

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

to

==========
Use Distilled path instead of DistilledURL.

It does not make sense to store the sandbox path for the offline files.
Instead, store the path relative to the offline folder.
Also rename method to make clear distinction between absolute file path
and relative path that can be used in files path or URLs.

DistilledURL change semantics.
It was the file URL to the local file, it is now a URL that can be loaded to
load the file.

BUG=664124

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

[commit-bot: I haz the power, 2016-11-14 20:10:28]

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