[Offline Pages] Refactoring deletion of pages with same urls.

[Offline Pages] Refactoring deletion of pages with same urls.

Simple refactoring to make InformSavePageDone() doesn't do extra work.

BUG=624043
Committed: https://crrev.com/f258f19fe9c59c5f6445d069637c95a09efc926c
Cr-Commit-Position: refs/heads/master@{#404298}

[romax, 2016-07-01 01:58:07]

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

[romax, 2016-07-01 01:58:08]

PTAL, simple change. Thanks!

[fgorski, 2016-07-01 17:43:22]

Mostly look good.

2 comments.

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

https://codereview.chromium.org/2112183002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:743:
DeleteExistingPageWithSameURL(result, offline_page);
Did you consider this?

if (result == SavePageResult::SUCCESS) {
  DeleteExistingPageWithSameURL(offline_page);
} else {
  PostClearStorageIfNeededTask();
}

That way you don't have to propagate result to the next method, and the method
name matches its functionality.

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

https://codereview.chromium.org/2112183002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.h:217: void
DeleteExistingPageWithSameURL(SavePageResult result,
s/Page/Pages/

[romax, 2016-07-06 19:44:50]

addressed comments. PTAL

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

https://codereview.chromium.org/2112183002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.cc:743:
DeleteExistingPageWithSameURL(result, offline_page);
On 2016/07/01 17:43:22, fgorski wrote:
> Did you consider this?
> 
> if (result == SavePageResult::SUCCESS) {
>   DeleteExistingPageWithSameURL(offline_page);
> } else {
>   PostClearStorageIfNeededTask();
> }
> 
> That way you don't have to propagate result to the next method, and the method
> name matches its functionality.

Done.

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

https://codereview.chromium.org/2112183002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_model_impl.h:217: void
DeleteExistingPageWithSameURL(SavePageResult result,
On 2016/07/01 17:43:22, fgorski wrote:
> s/Page/Pages/

Done.

[fgorski, 2016-07-07 17:03:46]

lgtm

https://codereview.chromium.org/2112183002/diff/20001/components/offline_page...
File components/offline_pages/offline_page_model_impl.cc (right):

https://codereview.chromium.org/2112183002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model_impl.cc:825: if (pages_allowed !=
kUnlimitedPages) {
nit:

if (pages_allowed == kUnlimited...)
  return;

[romax, 2016-07-07 21:39:14]

The CQ bit was checked by romax@chromium.org

[romax, 2016-07-07 21:39:15]

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

[commit-bot: I haz the power, 2016-07-07 21:41:02]

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-07-07 23:43:33]

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

[commit-bot: I haz the power, 2016-07-07 23:43:35]

Try jobs failed on following builders:
  mac_chromium_gyp_rel on master.tryserver.chromium.mac (JOB_TIMED_OUT, no build
URL)

[romax, 2016-07-08 01:09:32]

The CQ bit was checked by romax@chromium.org

[commit-bot: I haz the power, 2016-07-08 01:10:12]

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-07-08 01:15:57]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-08 01:16:06]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 01:20:28]

Description was changed from

==========
[Offline Pages] Refactoring deletion of pages with same urls.

Simple refactoring to make InformSavePageDone() doesn't do extra work.

BUG=624043
==========

to

==========
[Offline Pages] Refactoring deletion of pages with same urls.

Simple refactoring to make InformSavePageDone() doesn't do extra work.

BUG=624043

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

[commit-bot: I haz the power, 2016-07-08 01:20:30]

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