OfflinePageModel cleanups - improved CanSavePage() and moved static kInvalidOfflineId to impl

OfflinePageModel cleanups - improved CanSavePage() and moved static kInvalidOfflineId to impl.

[Tried to move CanSavePage to OfflinePageUtils but still want to use in components. In the process discovered other cleanup to do on that method.]

BUG=615165
Committed: https://crrev.com/128b7c0fff1d970e8c37e2a82b8cb4a168ba9d7f
Cr-Commit-Position: refs/heads/master@{#397519}

[dougarnett, 2016-06-01 15:44:26]

Description was changed from

==========
OfflinePageModel cleanups - move out statics CanSavePage() and kInvalidOfflineId

BUG=615165
==========

to

==========
OfflinePageModel cleanups - move out statics CanSavePage() and kInvalidOfflineId

BUG=615165
==========

[dougarnett, 2016-06-01 15:44:26]

dougarnett@chromium.org changed reviewers:
+ dewittj@chromium.org

[dewittj, 2016-06-01 16:05:04]

https://codereview.chromium.org/2023293002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_bridge.cc (right):

https://codereview.chromium.org/2023293002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_bridge.cc:158: return
OfflinePageUtils::CanSaveURL(url);
nit: just combine the two lines

https://codereview.chromium.org/2023293002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2023293002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:178: if (url.is_valid() &&
!url.SchemeIsHTTPOrHTTPS()) {
Why not CanSavePage?

[dougarnett, 2016-06-01 16:18:25]

https://codereview.chromium.org/2023293002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2023293002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:178: if (url.is_valid() &&
!url.SchemeIsHTTPOrHTTPS()) {
On 2016/06/01 16:05:04, dewittj wrote:
> Why not CanSavePage?

Good question here about where CanSaveURL() should live. Should we put in a
utils within components (so it is available to use here) rather than the utils
in browser/android? 

On the other hand, this is a little bit different of a test (is_valid and !) and
only used in one place - so wasn't clear to define a new method for it.

[dougarnett, 2016-06-01 19:27:09]

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

[dougarnett, 2016-06-01 19:27:09]

Adding Filip to review.

Filip, this CL is step toward the idea of making OfflinePageModel a pure
interface. It is moving static CanSavePage() method out into OfflinePageUtils -
which is fine for the API support but there is a check with the Model
implementation that was using this method and there is open question about
dealing with that.

https://codereview.chromium.org/2023293002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2023293002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:178: if (url.is_valid() &&
!url.SchemeIsHTTPOrHTTPS()) {
On 2016/06/01 16:18:25, dougarnett wrote:
> On 2016/06/01 16:05:04, dewittj wrote:
> > Why not CanSavePage?
> 
> Good question here about where CanSaveURL() should live. Should we put in a
> utils within components (so it is available to use here) rather than the utils
> in browser/android? 
> 
> On the other hand, this is a little bit different of a test (is_valid and !)
and
> only used in one place - so wasn't clear to define a new method for it.

Justin and I chatted about this offline wondering if we need this check here
anyway. In particular, why can't OfflinePageBridge::SavePage() do the CanSave*
check? If so, then the Bridge code might then need to run the callback with
SavePageResult::SKIPPED.

[dewittj, 2016-06-01 19:33:17]

On 2016/06/01 19:27:09, dougarnett wrote:
> Adding Filip to review.
> 
> Filip, this CL is step toward the idea of making OfflinePageModel a pure
> interface. It is moving static CanSavePage() method out into OfflinePageUtils
-
> which is fine for the API support but there is a check with the Model
> implementation that was using this method and there is open question about
> dealing with that.
> 
>
https://codereview.chromium.org/2023293002/diff/1/components/offline_pages/of...
> File components/offline_pages/offline_page_model_impl.cc (right):
> 
>
https://codereview.chromium.org/2023293002/diff/1/components/offline_pages/of...
> components/offline_pages/offline_page_model_impl.cc:178: if (url.is_valid() &&
> !url.SchemeIsHTTPOrHTTPS()) {
> On 2016/06/01 16:18:25, dougarnett wrote:
> > On 2016/06/01 16:05:04, dewittj wrote:
> > > Why not CanSavePage?
> > 
> > Good question here about where CanSaveURL() should live. Should we put in a
> > utils within components (so it is available to use here) rather than the
utils
> > in browser/android? 
> > 
> > On the other hand, this is a little bit different of a test (is_valid and !)
> and
> > only used in one place - so wasn't clear to define a new method for it.
> 
> Justin and I chatted about this offline wondering if we need this check here
> anyway. In particular, why can't OfflinePageBridge::SavePage() do the CanSave*
> check? If so, then the Bridge code might then need to run the callback with
> SavePageResult::SKIPPED.

To expand on this, it seems like the various archivers should know whether they
can serialize a given URL or not, the model doesn't have to care.

[fgorski, 2016-06-01 20:21:03]

https://codereview.chromium.org/2023293002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2023293002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:178: if (url.is_valid() &&
!url.SchemeIsHTTPOrHTTPS()) {
On 2016/06/01 19:27:09, dougarnett wrote:
> On 2016/06/01 16:18:25, dougarnett wrote:
> > On 2016/06/01 16:05:04, dewittj wrote:
> > > Why not CanSavePage?
> > 
> > Good question here about where CanSaveURL() should live. Should we put in a
> > utils within components (so it is available to use here) rather than the
utils
> > in browser/android? 
> > 
> > On the other hand, this is a little bit different of a test (is_valid and !)
> and
> > only used in one place - so wasn't clear to define a new method for it.

url.is_valid() was added recently, by jianli. We think there is a test that
depends on it. Perhaps updating the code to if (!CanSavePage) and making sure
the tests work is a better solution. 

If that works I think this method can still be a static on the model, until we
find a better place for it. I don't think this would violate the pure abstract
property of this class.

CanSaveURL can be a fall through to this method. That way we can also add more
conditions where saving URL should not be possible.

> 
> Justin and I chatted about this offline wondering if we need this check here
> anyway. In particular, why can't OfflinePageBridge::SavePage() do the CanSave*
> check? If so, then the Bridge code might then need to run the callback with
> SavePageResult::SKIPPED.
> 
> 

The reason I would be reluctant to do it is that we call InformSavePageDone
below, which collects UMA stats. If we short-circut that higher up, we will have
hard time tracking it properly. We already hit that problem in the past.

Additionally not all calls to SavePage will be done using the Bridge, which
justifies this check even more.

[dougarnett, 2016-06-01 22:08:40]

Description was changed from

==========
OfflinePageModel cleanups - move out statics CanSavePage() and kInvalidOfflineId

BUG=615165
==========

to

==========
OfflinePageModel cleanups - improved CanSavePage() and moved static
kInvalidOfflineId to impl.

[Tried to move CanSavePage to OfflinePageUtils but still want to use in
components. In the process discovered other cleanup to do on that method.]

BUG=615165
==========

[dougarnett, 2016-06-01 22:40:22]

Updated per Filip's comment.

Note: trybots seem to be having troubling patching currently.

https://codereview.chromium.org/2023293002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_bridge.cc (right):

https://codereview.chromium.org/2023293002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_bridge.cc:158: return
OfflinePageUtils::CanSaveURL(url);
On 2016/06/01 16:05:04, dewittj wrote:
> nit: just combine the two lines

Done.

https://codereview.chromium.org/2023293002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2023293002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:178: if (url.is_valid() &&
!url.SchemeIsHTTPOrHTTPS()) {
On 2016/06/01 20:21:03, fgorski wrote:
> On 2016/06/01 19:27:09, dougarnett wrote:
> > On 2016/06/01 16:18:25, dougarnett wrote:
> > > On 2016/06/01 16:05:04, dewittj wrote:
> > > > Why not CanSavePage?
> > > 
> > > Good question here about where CanSaveURL() should live. Should we put in
a
> > > utils within components (so it is available to use here) rather than the
> utils
> > > in browser/android? 
> > > 
> > > On the other hand, this is a little bit different of a test (is_valid and
!)
> > and
> > > only used in one place - so wasn't clear to define a new method for it.
> 
> url.is_valid() was added recently, by jianli. We think there is a test that
> depends on it. Perhaps updating the code to if (!CanSavePage) and making sure
> the tests work is a better solution. 
> 
> If that works I think this method can still be a static on the model, until we
> find a better place for it. I don't think this would violate the pure abstract
> property of this class.
> 
> CanSaveURL can be a fall through to this method. That way we can also add more
> conditions where saving URL should not be possible.
> 
> > 
> > Justin and I chatted about this offline wondering if we need this check here
> > anyway. In particular, why can't OfflinePageBridge::SavePage() do the
CanSave*
> > check? If so, then the Bridge code might then need to run the callback with
> > SavePageResult::SKIPPED.
> > 
> > 
> 
> The reason I would be reluctant to do it is that we call InformSavePageDone
> below, which collects UMA stats. If we short-circut that higher up, we will
have
> hard time tracking it properly. We already hit that problem in the past.
> 
> Additionally not all calls to SavePage will be done using the Bridge, which
> justifies this check even more.
> 

Ok put CanSave method back on the Model with url.is_valid() addition but did
keep the newer name for now.

[fgorski, 2016-06-02 16:41:56]

lgtm

https://codereview.chromium.org/2023293002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_bridge.cc (right):

https://codereview.chromium.org/2023293002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_bridge.cc:157: return
OfflinePageModel::CanSaveURL(ConvertJavaStringToUTF8(env, j_url));
As you've noticed compilers are not happy, because GURL as explicit constructor.
What dewittj@ meant was:
return OfflinePageModel::CanSaveURL(GURL(ConvertJavaStringToUTF8(env, j_url)));

[dewittj, 2016-06-02 18:08:41]

lgtm

[dougarnett, 2016-06-02 18:30:55]

https://codereview.chromium.org/2023293002/diff/60001/chrome/browser/android/...
File chrome/browser/android/offline_pages/offline_page_bridge.cc (right):

https://codereview.chromium.org/2023293002/diff/60001/chrome/browser/android/...
chrome/browser/android/offline_pages/offline_page_bridge.cc:157: return
OfflinePageModel::CanSaveURL(ConvertJavaStringToUTF8(env, j_url));
On 2016/06/02 16:41:56, fgorski wrote:
> As you've noticed compilers are not happy, because GURL as explicit
constructor.
> What dewittj@ meant was:
> return OfflinePageModel::CanSaveURL(GURL(ConvertJavaStringToUTF8(env,
j_url)));

Thanks - I do wonder now about the nit then - seems less readable to me so
putting back as I think the readability nit outweighs saving a line nit.

[dougarnett, 2016-06-02 20:09:04]

The CQ bit was checked by dougarnett@chromium.org

[dougarnett, 2016-06-02 20:09:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
fgorski@chromium.org, dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/2023293002/#ps100001
(title: "synced")

[commit-bot: I haz the power, 2016-06-02 20:10:01]

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

[commit-bot: I haz the power, 2016-06-02 21:52:37]

Description was changed from

==========
OfflinePageModel cleanups - improved CanSavePage() and moved static
kInvalidOfflineId to impl.

[Tried to move CanSavePage to OfflinePageUtils but still want to use in
components. In the process discovered other cleanup to do on that method.]

BUG=615165
==========

to

==========
OfflinePageModel cleanups - improved CanSavePage() and moved static
kInvalidOfflineId to impl.

[Tried to move CanSavePage to OfflinePageUtils but still want to use in
components. In the process discovered other cleanup to do on that method.]

BUG=615165
==========

[commit-bot: I haz the power, 2016-06-02 21:52:38]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-06-02 21:55:23]

Description was changed from

==========
OfflinePageModel cleanups - improved CanSavePage() and moved static
kInvalidOfflineId to impl.

[Tried to move CanSavePage to OfflinePageUtils but still want to use in
components. In the process discovered other cleanup to do on that method.]

BUG=615165
==========

to

==========
OfflinePageModel cleanups - improved CanSavePage() and moved static
kInvalidOfflineId to impl.

[Tried to move CanSavePage to OfflinePageUtils but still want to use in
components. In the process discovered other cleanup to do on that method.]

BUG=615165

Committed: https://crrev.com/128b7c0fff1d970e8c37e2a82b8cb4a168ba9d7f
Cr-Commit-Position: refs/heads/master@{#397519}
==========

[commit-bot: I haz the power, 2016-06-02 21:55:24]

Patchset 6 (id:??) landed as
https://crrev.com/128b7c0fff1d970e8c37e2a82b8cb4a168ba9d7f
Cr-Commit-Position: refs/heads/master@{#397519}