[Offline] Adding archiver interface

[Offline] Adding archiver interface

BUG=491352
Committed: https://crrev.com/d1c862afcb67b7079104536e09da669d1ec92290
Cr-Commit-Position: refs/heads/master@{#334608}

[fgorski, 2015-06-09 22:59:06]

fgorski@chromium.org changed reviewers:
+ dimich@chromium.org

[fgorski, 2015-06-10 18:27:48]

Patchset #2 (id:20001) has been deleted

[Dmitry Titov, 2015-06-11 18:35:32]

https://codereview.chromium.org/1174803002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_archiver.h (right):

https://codereview.chromium.org/1174803002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_archiver.h:33: virtual void
OnCreateArchiveFailure(const Request& request) = 0;
What about returning some sort of error? I'm thinking about the mock where we
will do "Could not save the page because there is no available memory on the
device", as opposite to other errors. Since there is a file operation involved,
I think at least a set of { DEVICE_IS_FULL, CANCELLED, UNKNOWN_ERROR } is
needed, likely grow in the future.

https://codereview.chromium.org/1174803002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_archiver.h:43: // passed in client.
It is a good place to add comment about 'suggested_file_name' (as for why it is
suggested)

https://codereview.chromium.org/1174803002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_archiver.h:44: virtual Request
CreateArchive(
I think you want to return a pointer or ref to Request... Also, who owns it?

[Dmitry Titov, 2015-06-11 20:24:18]

More thoughts: if the Request is an object that is created by Archiver, then it
likely should be opaque for the client of Archiver and only creatable by it. It
can include the data needed by Archiver to perform the Request (so Archiver
won't have to maintain yet another data structure to keep its data associated
with each Request). The Request then would have a Cancel() method, canceling it.

It also can have 'status()' to indicate where it is, currently - potentially
useful for UI of some progress indicator, something like % or
snapshotting/saving/finished/failed...
The Request::url() should be a getter, to enforce the fact that url can not be
changed by objects outside Archiver.
As written, one can pass Request by value, copy it, change url in it - perhaps
not desirable things.
In other words, it seems Request is an abstract interface rather then a
struct... Implementation can live in archiver.cc as local class...

OnSuccess/OnFailure would still come to Client. Client would use reference
equality to see which Request was done.
Happy to discuss if you want to.

[fgorski, 2015-06-11 23:30:02]

PTAL

Further changes to the Request can be made once we have some more details of
what we want to use it for.

https://codereview.chromium.org/1174803002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_archiver.h (right):

https://codereview.chromium.org/1174803002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_archiver.h:33: virtual void
OnCreateArchiveFailure(const Request& request) = 0;
On 2015/06/11 18:35:32, Dmitry Titov wrote:
> What about returning some sort of error? I'm thinking about the mock where we
> will do "Could not save the page because there is no available memory on the
> device", as opposite to other errors. Since there is a file operation
involved,
> I think at least a set of { DEVICE_IS_FULL, CANCELLED, UNKNOWN_ERROR } is
> needed, likely grow in the future.

Done.

https://codereview.chromium.org/1174803002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_archiver.h:43: // passed in client.
On 2015/06/11 18:35:32, Dmitry Titov wrote:
> It is a good place to add comment about 'suggested_file_name' (as for why it
is
> suggested)

Dropped the parameter per offline discussion.

https://codereview.chromium.org/1174803002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_archiver.h:44: virtual Request
CreateArchive(
On 2015/06/11 18:35:32, Dmitry Titov wrote:
> I think you want to return a pointer or ref to Request... Also, who owns it?

Done.

[Dmitry Titov, 2015-06-12 18:09:12]

Great, thanks for making changes! Some more comments. Did not look at tests yet.

https://codereview.chromium.org/1174803002/diff/40001/components/offline_page...
File components/offline_pages/offline_page_archiver.h (right):

https://codereview.chromium.org/1174803002/diff/40001/components/offline_page...
components/offline_pages/offline_page_archiver.h:19: explicit Request(const
GURL& url);
It's better to leave just an interface here, without ctor, all abstract, no data
members. Then, implement actual Request as static class in a corresponding .cc
file that will define OfflinePageArchiverImpl.
This way, details of implementation of Request are hidden and there is no
question about who can create one etc. For example, it might have a pointer to
some internal data structure of Archiver and pull url form there, etc.

https://codereview.chromium.org/1174803002/diff/40001/components/offline_page...
components/offline_pages/offline_page_archiver.h:49: protected:
No need to have this ctor here. It is only needed if you also add DISALLO..
macro here, since defining a private copy ctor (by macros) will prevent compiler
from generating a default ctor.
In general, doing all that in a pure abstract class does not make a lot of sense
- it's better to keep the interface clean.

https://codereview.chromium.org/1174803002/diff/40001/components/offline_page...
components/offline_pages/offline_page_archiver.h:53: OfflinePageArchiver();
This ctor can be omitted.

https://codereview.chromium.org/1174803002/diff/40001/components/offline_page...
components/offline_pages/offline_page_archiver.h:57: // passed in client. Caller
owns the returned request object.
This needs to say what happens to ongoing archiving operation if the Request is
deleted by the caller. Archiver impl will need a test for it.

https://codereview.chromium.org/1174803002/diff/40001/components/offline_page...
components/offline_pages/offline_page_archiver.h:58: virtual scoped_ptr<Request>
CreateArchive(const GURL& url,
Need a comment of what happens if the same URL is used when:
- archive already exists on disk because of previously finished operation
- there is another archiving operation still in progress
- there is no currently open WebContents with specified url
- there is a WebContents but the tab with it was killed by OS (as discovered by
Archiver when this call is received)
- the WebContent exists but it is still loading (main or other resources)

https://codereview.chromium.org/1174803002/diff/40001/components/offline_page...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/1174803002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:53: // Offline page archiver.
Outlives the model. Owned by the embedder.
I don't have a proposal here, but I'm curious why can't this be owned by the
Model?

[fgorski, 2015-06-12 20:37:35]

PTAL

https://codereview.chromium.org/1174803002/diff/40001/components/offline_page...
File components/offline_pages/offline_page_archiver.h (right):

https://codereview.chromium.org/1174803002/diff/40001/components/offline_page...
components/offline_pages/offline_page_archiver.h:19: explicit Request(const
GURL& url);
On 2015/06/12 18:09:11, Dmitry Titov wrote:
> It's better to leave just an interface here, without ctor, all abstract, no
data
> members. Then, implement actual Request as static class in a corresponding .cc
> file that will define OfflinePageArchiverImpl.
> This way, details of implementation of Request are hidden and there is no
> question about who can create one etc. For example, it might have a pointer to
> some internal data structure of Archiver and pull url form there, etc.

Done.

https://codereview.chromium.org/1174803002/diff/40001/components/offline_page...
components/offline_pages/offline_page_archiver.h:49: protected:
On 2015/06/12 18:09:11, Dmitry Titov wrote:
> No need to have this ctor here. It is only needed if you also add DISALLO..
> macro here, since defining a private copy ctor (by macros) will prevent
compiler
> from generating a default ctor.
> In general, doing all that in a pure abstract class does not make a lot of
sense
> - it's better to keep the interface clean.

Done.

https://codereview.chromium.org/1174803002/diff/40001/components/offline_page...
components/offline_pages/offline_page_archiver.h:53: OfflinePageArchiver();
On 2015/06/12 18:09:11, Dmitry Titov wrote:
> This ctor can be omitted.

Done.

https://codereview.chromium.org/1174803002/diff/40001/components/offline_page...
components/offline_pages/offline_page_archiver.h:57: // passed in client. Caller
owns the returned request object.
On 2015/06/12 18:09:11, Dmitry Titov wrote:
> This needs to say what happens to ongoing archiving operation if the Request
is
> deleted by the caller. Archiver impl will need a test for it.

Done.

https://codereview.chromium.org/1174803002/diff/40001/components/offline_page...
components/offline_pages/offline_page_archiver.h:58: virtual scoped_ptr<Request>
CreateArchive(const GURL& url,
On 2015/06/12 18:09:11, Dmitry Titov wrote:
> Need a comment of what happens if the same URL is used when:

> - archive already exists on disk because of previously finished operation
Archiver comes up with a name for the archive that is valid and not used.

> - there is another archiving operation still in progress
Archiver should be capable to start multiple requests in parallel.
Model should make a proper request. This is outside of scope of archiver.

> - there is no currently open WebContents with specified url
Added ArchiverError::ERROR_CONTENT_UNAVAILABLE

> - there is a WebContents but the tab with it was killed by OS (as discovered
by
> Archiver when this call is received)
Same error as above

> - the WebContent exists but it is still loading (main or other resources)
Operation will take long. On success we proceed, on canceled navigation, see
above.

https://codereview.chromium.org/1174803002/diff/40001/components/offline_page...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/1174803002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.h:53: // Offline page archiver.
Outlives the model. Owned by the embedder.
On 2015/06/12 18:09:11, Dmitry Titov wrote:
> I don't have a proposal here, but I'm curious why can't this be owned by the
> Model? 

Embedder will still talk to the archiver, and for that it will require to know
the concrete type. I think it is better for it to own it, since it knows most
about archiver. Model will simply have reference for it and will use its
iterface.

[Dmitry Titov, 2015-06-12 21:03:59]

> > Need a comment of what happens if the same URL is used when:
> 
> > - archive already exists on disk because of previously finished operation
> Archiver comes up with a name for the archive that is valid and not used.
Sorry, I was not clear (see how lack of info breeds miscommunication?).
I meant "archive of the same page at the same URL"
The archive on disk can be refreshed or another one created (then we need to
figure out how to delete the hanging archives on disk)

> 
> > - there is another archiving operation still in progress
> Archiver should be capable to start multiple requests in parallel.
> Model should make a proper request. This is outside of scope of archiver.
Same thing, I meant "with the same URL".


> > - the WebContent exists but it is still loading (main or other resources)
> Operation will take long. On success we proceed, on canceled navigation, see
> above.
We might either want to stop navigation and make snapshot (this is what
currently "Save As.." command is doing.
At any rate, it's useful to reflect this in the comment.

> > I don't have a proposal here, but I'm curious why can't this be owned by the
> > Model? 
> 
> Embedder will still talk to the archiver, and for that it will require to know
> the concrete type. I think it is better for it to own it, since it knows most
> about archiver. Model will simply have reference for it and will use its
> iterface.

Thanks, makes sense.

https://codereview.chromium.org/1174803002/diff/60001/components/offline_page...
File components/offline_pages/offline_page_archiver.cc (right):

https://codereview.chromium.org/1174803002/diff/60001/components/offline_page...
components/offline_pages/offline_page_archiver.cc:10:
OfflinePageArchiver::Request::~Request() {
The remaining empty dtors can just go into .h file and this file is not needed
then, unless you have a plan for it to grow...

[fgorski, 2015-06-12 22:16:27]

Added more documentation

https://codereview.chromium.org/1174803002/diff/60001/components/offline_page...
File components/offline_pages/offline_page_archiver.cc (right):

https://codereview.chromium.org/1174803002/diff/60001/components/offline_page...
components/offline_pages/offline_page_archiver.cc:10:
OfflinePageArchiver::Request::~Request() {
On 2015/06/12 21:03:59, Dmitry Titov wrote:
> The remaining empty dtors can just go into .h file and this file is not needed
> then, unless you have a plan for it to grow...

Done.

[Dmitry Titov, 2015-06-15 21:55:42]

LGTM, with some typos fixed:

https://codereview.chromium.org/1174803002/diff/80001/components/offline_page...
File components/offline_pages/offline_page_archiver.h (right):

https://codereview.chromium.org/1174803002/diff/80001/components/offline_page...
components/offline_pages/offline_page_archiver.h:21: // Archiver will be
responsible for naming the file that is being save (it has
save -> saved

https://codereview.chromium.org/1174803002/diff/80001/components/offline_page...
components/offline_pages/offline_page_archiver.h:25: // Archiver should be able
archive multiple pages in parallel, as these are
able -> able to

https://codereview.chromium.org/1174803002/diff/80001/components/offline_page...
components/offline_pages/offline_page_archiver.h:28: // If archiver get two
consecutive requests to archive the same page (may be run
get -> gets

https://codereview.chromium.org/1174803002/diff/80001/components/offline_page...
components/offline_pages/offline_page_archiver.h:34: // If the page is not
completely loaded, it is up to the implemetnation of the
type: implemetnation

https://codereview.chromium.org/1174803002/diff/80001/components/offline_page...
components/offline_pages/offline_page_archiver.h:37: // point in time (what the
uses sees).
uses -> user

[Dmitry Titov, 2015-06-15 22:30:03]

https://codereview.chromium.org/1174803002/diff/80001/components/offline_page...
File components/offline_pages/offline_page_archiver.h (right):

https://codereview.chromium.org/1174803002/diff/80001/components/offline_page...
components/offline_pages/offline_page_archiver.h:70: virtual void
OnCreateArchiveSuccess(Request* request,
Additional comment: consider merging both callbacks and adding a new value to
ArchiveError (SUCCESSFULLY_CREATED?) and renaming ArchiveError into
ArchiveResult...

[fgorski, 2015-06-16 15:18:10]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2015-06-16 15:18:10]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org
Link to the patchset: https://codereview.chromium.org/1174803002/#ps100001
(title: "Addressing feedback to documentation")

[commit-bot: I haz the power, 2015-06-16 15:18:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1174803002/100001

[fgorski, 2015-06-16 15:18:38]

all addressed.

https://codereview.chromium.org/1174803002/diff/80001/components/offline_page...
File components/offline_pages/offline_page_archiver.h (right):

https://codereview.chromium.org/1174803002/diff/80001/components/offline_page...
components/offline_pages/offline_page_archiver.h:21: // Archiver will be
responsible for naming the file that is being save (it has
On 2015/06/15 21:55:42, Dmitry Titov wrote:
> save -> saved

Done.

https://codereview.chromium.org/1174803002/diff/80001/components/offline_page...
components/offline_pages/offline_page_archiver.h:25: // Archiver should be able
archive multiple pages in parallel, as these are
On 2015/06/15 21:55:42, Dmitry Titov wrote:
> able -> able to

Done.

https://codereview.chromium.org/1174803002/diff/80001/components/offline_page...
components/offline_pages/offline_page_archiver.h:28: // If archiver get two
consecutive requests to archive the same page (may be run
On 2015/06/15 21:55:42, Dmitry Titov wrote:
> get -> gets

Done.

https://codereview.chromium.org/1174803002/diff/80001/components/offline_page...
components/offline_pages/offline_page_archiver.h:34: // If the page is not
completely loaded, it is up to the implemetnation of the
On 2015/06/15 21:55:42, Dmitry Titov wrote:
> type: implemetnation

Done.

https://codereview.chromium.org/1174803002/diff/80001/components/offline_page...
components/offline_pages/offline_page_archiver.h:37: // point in time (what the
uses sees).
On 2015/06/15 21:55:42, Dmitry Titov wrote:
> uses -> user

Done.

https://codereview.chromium.org/1174803002/diff/80001/components/offline_page...
components/offline_pages/offline_page_archiver.h:70: virtual void
OnCreateArchiveSuccess(Request* request,
On 2015/06/15 22:30:03, Dmitry Titov wrote:
> Additional comment: consider merging both callbacks and adding a new value to
> ArchiveError (SUCCESSFULLY_CREATED?) and renaming ArchiveError into
> ArchiveResult...

Done.

[commit-bot: I haz the power, 2015-06-16 15:36:33]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2015-06-16 15:37:28]

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