[Offline pages] Detecting missing offline copy

[Offline pages] Detecting missing offline copy

* Adding a check for offline page metadata consistency
  It's goal is to detect that some pages are missing an
  offline copy.
* Adding an event informing the observers of offline page
  model that a page was deleted.
* Adding 2 tests for the offline copy being deleted.
* Updating tests to not fail on memory bots
* Consuming the metadata checks and offline page deleted
  event from the enhanced bookmarks UI.

BUG=537085
Committed: https://crrev.com/9dac2866f1248d7dccccc90ecc84c7c1ee9d87b8
Cr-Commit-Position: refs/heads/master@{#355423}

[fgorski, 2015-10-12 23:07:14]

fgorski@chromium.org changed reviewers:
+ jianli@chromium.org, kkimlabs@chromium.org

[fgorski, 2015-10-12 23:08:38]

PTAL

[jianli, 2015-10-13 00:17:54]

https://codereview.chromium.org/1397233002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java
(right):

https://codereview.chromium.org/1397233002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:315:
if (mDelegate.getModel().getOfflinePageBridge() != null) {
nit: please cache OfflinePageBridge instance.

https://codereview.chromium.org/1397233002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:320:
if (mOfflineStorageHeader != null) {
Please move this piece to OfflinePageBridge block and you don't need to check
the nullability of mOfflineStorageHeader there.

https://codereview.chromium.org/1397233002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1397233002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:70:
public interface OfflinePageModelObserver {
Consider changing this from interface to avoid providing implementation for
every observer, like what BookmarkModelObserver does.

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

https://codereview.chromium.org/1397233002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.cc:70: void
ListPagesMissingOfflineCopy(
ValidateArchiveFiles?

https://codereview.chromium.org/1397233002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.cc:221: for (const auto&
id_page_pair : offline_pages_)
I think it might be better to combine this logic with the logic in line 157-163.
We can add a helper function to produce a list of pair of file path and
bookmark_id, like:
  void GetArchiveFiles(std::vector<std::pair<base::FilePath, int64>> files)

Change DeleteArchiveFiles to take this list and go through all files and return
those that are deleted successfully. Change OnDeleteArchiveFilesDone to update
the store accordingly.

Change ListPagesMissingOfflineCopy (better called as ValidateArchiveFiles) to
take this list.

https://codereview.chromium.org/1397233002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.cc:390: for (int64 bookmark_id :
bookmark_ids) {
nit: brackets are not needed

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

https://codereview.chromium.org/1397233002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.h:100: virtual void
OfflinePageDeleted(int64 bookmark_id) = 0;
nit: please add comment

https://codereview.chromium.org/1397233002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.h:165: void
CheckMetadataConsistency();
Since we're only checking if the file is still there, the name to indicate
checking all metadata seems to be too broad. How about
CheckForExternalFileDeletion?

https://codereview.chromium.org/1397233002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.h:219: void
OnListPagesMissingOfflineCopy(
It would be consistent with the naming "archive file" we use here. How about
OnValidateArchiveFilesDone?

[fgorski, 2015-10-13 17:39:57]

fgorski@chromium.org changed reviewers:
+ ianwen@chromium.org
- kkimlabs@chromium.org

[fgorski, 2015-10-13 17:39:58]

Addressing most of Jian's comments.

Ian, could you take a look at enhanced bookmarks stuff?

https://codereview.chromium.org/1397233002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java
(right):

https://codereview.chromium.org/1397233002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:315:
if (mDelegate.getModel().getOfflinePageBridge() != null) {
On 2015/10/13 00:17:53, jianli wrote:
> nit: please cache OfflinePageBridge instance.

Done.

https://codereview.chromium.org/1397233002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:320:
if (mOfflineStorageHeader != null) {
On 2015/10/13 00:17:53, jianli wrote:
> Please move this piece to OfflinePageBridge block and you don't need to check
> the nullability of mOfflineStorageHeader there.

Done.

https://codereview.chromium.org/1397233002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1397233002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:70:
public interface OfflinePageModelObserver {
On 2015/10/13 00:17:53, jianli wrote:
> Consider changing this from interface to avoid providing implementation for
> every observer, like what BookmarkModelObserver does.

Done.

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

https://codereview.chromium.org/1397233002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.cc:70: void
ListPagesMissingOfflineCopy(
On 2015/10/13 00:17:53, jianli wrote:
> ValidateArchiveFiles?

Changed to ListPagesMissingArchiveFile since that is the only thing we are doing
here.

https://codereview.chromium.org/1397233002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.cc:221: for (const auto&
id_page_pair : offline_pages_)
On 2015/10/13 00:17:54, jianli wrote:
> I think it might be better to combine this logic with the logic in line
157-163.
> We can add a helper function to produce a list of pair of file path and
> bookmark_id, like:
>   void GetArchiveFiles(std::vector<std::pair<base::FilePath, int64>> files)
> 
> Change DeleteArchiveFiles to take this list and go through all files and
return
> those that are deleted successfully. Change OnDeleteArchiveFilesDone to update
> the store accordingly.
> 
> Change ListPagesMissingOfflineCopy (better called as ValidateArchiveFiles) to
> take this list.

I don't understand what exactly is your expectation here. We can discuss this
when you are available.

https://codereview.chromium.org/1397233002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.cc:390: for (int64 bookmark_id :
bookmark_ids) {
On 2015/10/13 00:17:53, jianli wrote:
> nit: brackets are not needed

Not true:
a) FOR_EACH_OBSERVER is in fact multi-line after macro is expanded.
b) compilation fails on windows without {}

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

https://codereview.chromium.org/1397233002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.h:100: virtual void
OfflinePageDeleted(int64 bookmark_id) = 0;
On 2015/10/13 00:17:54, jianli wrote:
> nit: please add comment

Done.

https://codereview.chromium.org/1397233002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.h:165: void
CheckMetadataConsistency();
On 2015/10/13 00:17:54, jianli wrote:
> Since we're only checking if the file is still there, the name to indicate
> checking all metadata seems to be too broad. How about
> CheckForExternalFileDeletion?

I am ok with making that change in the model now, but I would rather not expose
such intimate knowledge of offline pages to the UI layer. I am only ok to make
such change on the backend side.

https://codereview.chromium.org/1397233002/diff/60001/components/offline_page...
components/offline_pages/offline_page_model.h:219: void
OnListPagesMissingOfflineCopy(
On 2015/10/13 00:17:54, jianli wrote:
> It would be consistent with the naming "archive file" we use here. How about
> OnValidateArchiveFilesDone?

Done.

[jianli, 2015-10-14 22:30:23]

https://codereview.chromium.org/1397233002/diff/100001/components/offline_pag...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1397233002/diff/100001/components/offline_pag...
components/offline_pages/offline_page_model.cc:71: void
ListPagesMissingArchiveFile(
FindMissingArchiveFiles?

https://codereview.chromium.org/1397233002/diff/100001/components/offline_pag...
components/offline_pages/offline_page_model.cc:77: if
(!base::PathExists(id_path.second))
I think we should also call base::DirectoryExists to find out if it is not for a
directory since it is likely that the same name could be used for a directory by
the external program.

https://codereview.chromium.org/1397233002/diff/100001/components/offline_pag...
components/offline_pages/offline_page_model.cc:408: const std::vector<int64>*
pages_missing_archive_file) {
pages_missing_archive_file => bookmark_ids_for_missing_archive_file?

https://codereview.chromium.org/1397233002/diff/100001/components/offline_pag...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/1397233002/diff/100001/components/offline_pag...
components/offline_pages/offline_page_model.h:165: // Checks that all of the
offline pages have corresponding offline copies.
Can you be a bit more specific in the comment? I think offline pages already
mean to save offline copies and we should try to avoid referring to offline copy
here since it is more UI term. Can we just say "Checks all offline pages to make
sure saved files are still present, i.e. not being deleted by external
programs"?

Could you please also comment the consequence of this, like metadata being
removed and OfflinePageDeleted being fired?

[Ian Wen, 2015-10-14 23:57:22]

https://chromiumcodereview.appspot.com/1397233002/diff/100001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
(right):

https://chromiumcodereview.appspot.com/1397233002/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:233:
private void updateOfflineSection() {
assert OfflinePageBridge.isEnabled() here? :)

https://chromiumcodereview.appspot.com/1397233002/diff/100001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java
(right):

https://chromiumcodereview.appspot.com/1397233002/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:284:
removeItem(deletedPosition);
Q: if an offline page is deleted, does it mean the bookmark should be deleted?
Is it possible that the user does not want the offline page stored, but would
like to treat it as a simple bookmark?

https://chromiumcodereview.appspot.com/1397233002/diff/100001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:316:
if (offlinePageBridge != null) {
Sometimes we use null checks, some times we use OfflinePageBridge#isEnabled().
Is there a best practice to prefer one against the other?

[jianli, 2015-10-15 00:24:36]

https://codereview.chromium.org/1397233002/diff/100001/components/offline_pag...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1397233002/diff/100001/components/offline_pag...
components/offline_pages/offline_page_model.cc:394: FOR_EACH_OBSERVER(Observer,
observers_, OfflinePageDeleted(bookmark_id));
Per the discussion with Ian, I realized that OnRemoveOfflinePagesDone could be
triggered from FinalizePageDeletion in my patch. I don't think we want to
trigger the delete event for the internal cleanup. We can work offline to see
how to resolve this.

[fgorski, 2015-10-15 19:12:29]

PTAL

https://codereview.chromium.org/1397233002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
(right):

https://codereview.chromium.org/1397233002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:233:
private void updateOfflineSection() {
On 2015/10/14 23:57:22, Ian Wen wrote:
> assert OfflinePageBridge.isEnabled() here? :)

Done.

https://codereview.chromium.org/1397233002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java
(right):

https://codereview.chromium.org/1397233002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:284:
removeItem(deletedPosition);
On 2015/10/14 23:57:22, Ian Wen wrote:
> Q: if an offline page is deleted, does it mean the bookmark should be deleted?

No. However, when user deletes bookmark, we make sure to remove the offline
page.
(that is the other patch that you're reviewing from Jian)

What is happening here is:
We know we are in "saved offline" view and we only show bookmarks with an
offline
copy, when we detect that a copy is missing, we remove the bookmark from the
view
without deleting the bookmark. It will be visible when you open "Mobile
bookmarks"
or "All bookmarks/saved pages".

> Is it possible that the user does not want the offline page stored, but would
> like to treat it as a simple bookmark?

Yes. That is our current implementation. If you go to edit bookmark view there
is
a row telling you what the size of the page is and a [remove] button, of course
for
bookmarks that have the offline copy.

https://codereview.chromium.org/1397233002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:316:
if (offlinePageBridge != null) {
On 2015/10/14 23:57:22, Ian Wen wrote:
> Sometimes we use null checks, some times we use OfflinePageBridge#isEnabled().
> Is there a best practice to prefer one against the other?

Here is a rule of thumb that we try to apply:
If we are going to be using offlinePageBridge, we check if it is not null,
otherwise, if we don't use it directly, we make a check for isEnabled.

https://codereview.chromium.org/1397233002/diff/100001/components/offline_pag...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1397233002/diff/100001/components/offline_pag...
components/offline_pages/offline_page_model.cc:71: void
ListPagesMissingArchiveFile(
On 2015/10/14 22:30:23, jianli wrote:
> FindMissingArchiveFiles?

No, we are not looking for missing archive files, we are looking for pages (that
are) missing archive files, hence "list pages missing archive files".

https://codereview.chromium.org/1397233002/diff/100001/components/offline_pag...
components/offline_pages/offline_page_model.cc:77: if
(!base::PathExists(id_path.second))
On 2015/10/14 22:30:23, jianli wrote:
> I think we should also call base::DirectoryExists to find out if it is not for
a
> directory since it is likely that the same name could be used for a directory
by
> the external program.

Done.

https://codereview.chromium.org/1397233002/diff/100001/components/offline_pag...
components/offline_pages/offline_page_model.cc:394: FOR_EACH_OBSERVER(Observer,
observers_, OfflinePageDeleted(bookmark_id));
On 2015/10/15 00:24:36, jianli wrote:
> Per the discussion with Ian, I realized that OnRemoveOfflinePagesDone could be
> triggered from FinalizePageDeletion in my patch. I don't think we want to
> trigger the delete event for the internal cleanup. We can work offline to see
> how to resolve this.

OK, let's discuss this.

https://codereview.chromium.org/1397233002/diff/100001/components/offline_pag...
components/offline_pages/offline_page_model.cc:408: const std::vector<int64>*
pages_missing_archive_file) {
On 2015/10/14 22:30:23, jianli wrote:
> pages_missing_archive_file => bookmark_ids_for_missing_archive_file?

Done. Went with ids_of_pages_missing_archive_file

https://codereview.chromium.org/1397233002/diff/100001/components/offline_pag...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/1397233002/diff/100001/components/offline_pag...
components/offline_pages/offline_page_model.h:165: // Checks that all of the
offline pages have corresponding offline copies.
On 2015/10/14 22:30:23, jianli wrote:
> Can you be a bit more specific in the comment? I think offline pages already
> mean to save offline copies and we should try to avoid referring to offline
copy
> here since it is more UI term. Can we just say "Checks all offline pages to
make
> sure saved files are still present, i.e. not being deleted by external
> programs"?
I disagree. See line 36 in this file.

> 
> Could you please also comment the consequence of this, like metadata being
> removed and OfflinePageDeleted being fired? 
Done.

[fgorski, 2015-10-20 15:59:25]

PTAL.
All comments from Jian Li addressed.
Ian, do you have any more suggestions?

Thanks.
Filip

[Ian Wen, 2015-10-20 20:52:01]

lgtm with nits. Thanks for answering the previous question I asked.

https://chromiumcodereview.appspot.com/1397233002/diff/160001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
(right):

https://chromiumcodereview.appspot.com/1397233002/diff/160001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:241:
mEnhancedBookmarksModel.getOfflinePageBridge().checkOfflinePageMetadata();
This looks like a sanity check, so let's move it to #235?

https://chromiumcodereview.appspot.com/1397233002/diff/160001/chrome/android/...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://chromiumcodereview.appspot.com/1397233002/diff/160001/chrome/android/...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:88:
* Called when an offline page is deleted.
Document "This can be called as a result of #checkOfflinePageMetaData()"

[jianli, 2015-10-20 21:56:49]

https://codereview.chromium.org/1397233002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1397233002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:73:
*  Base observer class listeners to be notified of changes to the offline page
model.
remove the extra space before 'B'

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

https://codereview.chromium.org/1397233002/diff/160001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.cc:107: if
(!offline_page_model_->is_loaded())
Is this likely?

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
components/offline_pages/offline_page_model.cc:77: void
ListPagesMissingArchiveFile(
I think FindPagesMissingArchiveFile sounds easier to understand than
ListPagesMissingArchiveFile.

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
components/offline_pages/offline_page_model.cc:84:
!base::DirectoryExists(id_path.second)) {
The not operator should be removed for the 2nd test since if the path denotes a
directory, it should be treated as an error.

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
components/offline_pages/offline_page_model.cc:357: Observer, observers_,
OfflinePageDeleted(offline_page_item.bookmark_id));
I think we should be firing both OfflinePageModelChanged and OfflinePageDeleted
events.

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
components/offline_pages/offline_page_model.cc:536: void
OfflinePageModel::OnRemovalOfPagesMissingArchiveFileDone(
The name OnRemovalOfPagesMissingArchiveFileDone seems to be confusing. How about
OnRemoveMetadataOfPagesMissingArchiveFileDone?

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
components/offline_pages/offline_page_model.cc:539: for (int64 bookmark_id :
bookmark_ids) {
nit: remove unneeded brackets

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
components/offline_pages/offline_page_model.cc:543: 
nit: remove extra blank line

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
components/offline_pages/offline_page_model.h:101: // Invoked when an offline
copy related to |bookmark_id| was deleted.
nit: add blank line

Also please move this method after OfflinePageModelChanged

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
File components/offline_pages/offline_page_model_unittest.cc (right):

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
components/offline_pages/offline_page_model_unittest.cc:289:
run_loop_->RunUntilIdle();
Will base::RunLoop().RunUntilIdle() work?

[fgorski, 2015-10-21 20:01:34]

PTAL

https://codereview.chromium.org/1397233002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java
(right):

https://codereview.chromium.org/1397233002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkEditActivity.java:241:
mEnhancedBookmarksModel.getOfflinePageBridge().checkOfflinePageMetadata();
On 2015/10/20 20:52:01, Ian Wen wrote:
> This looks like a sanity check, so let's move it to #235?

Done.

https://codereview.chromium.org/1397233002/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java
(right):

https://codereview.chromium.org/1397233002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:73:
*  Base observer class listeners to be notified of changes to the offline page
model.
On 2015/10/20 21:56:48, jianli wrote:
> remove the extra space before 'B'

Done.

https://codereview.chromium.org/1397233002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:88:
* Called when an offline page is deleted.
On 2015/10/20 20:52:01, Ian Wen wrote:
> Document "This can be called as a result of #checkOfflinePageMetaData()"

Done.

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

https://codereview.chromium.org/1397233002/diff/160001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.cc:107: if
(!offline_page_model_->is_loaded())
On 2015/10/20 21:56:48, jianli wrote:
> Is this likely?

Good catch.

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
components/offline_pages/offline_page_model.cc:77: void
ListPagesMissingArchiveFile(
On 2015/10/20 21:56:49, jianli wrote:
> I think FindPagesMissingArchiveFile sounds easier to understand than
> ListPagesMissingArchiveFile.

Done.

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
components/offline_pages/offline_page_model.cc:84:
!base::DirectoryExists(id_path.second)) {
On 2015/10/20 21:56:49, jianli wrote:
> The not operator should be removed for the 2nd test since if the path denotes
a
> directory, it should be treated as an error.

Done.

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
components/offline_pages/offline_page_model.cc:357: Observer, observers_,
OfflinePageDeleted(offline_page_item.bookmark_id));
On 2015/10/20 21:56:48, jianli wrote:
> I think we should be firing both OfflinePageModelChanged and
OfflinePageDeleted
> events.

I think triggering OfflinePageDeleted is enough and to the point. Plus it
carries specific information which helps observers react, while the other is
very generic and does not.

If we fire both events the UI listening to these might make a bookmark specific
update, followed by complete update, because you don't even know what just
happened.

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
components/offline_pages/offline_page_model.cc:536: void
OfflinePageModel::OnRemovalOfPagesMissingArchiveFileDone(
On 2015/10/20 21:56:49, jianli wrote:
> The name OnRemovalOfPagesMissingArchiveFileDone seems to be confusing. How
about
> OnRemoveMetadataOfPagesMissingArchiveFileDone?

How about: OnRemoveOfflinePagesMissingArchiveFileDone?

I disagree with Metadata part, because this is a callback to RemoveOfflinePages.

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
components/offline_pages/offline_page_model.cc:539: for (int64 bookmark_id :
bookmark_ids) {
On 2015/10/20 21:56:49, jianli wrote:
> nit: remove unneeded brackets

No, because it won't compile on Windows

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
components/offline_pages/offline_page_model.cc:543: 
On 2015/10/20 21:56:48, jianli wrote:
> nit: remove extra blank line

Done.

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
File components/offline_pages/offline_page_model.h (right):

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
components/offline_pages/offline_page_model.h:101: // Invoked when an offline
copy related to |bookmark_id| was deleted.
On 2015/10/20 21:56:49, jianli wrote:
> nit: add blank line
> 
> Also please move this method after OfflinePageModelChanged

Done.

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
File components/offline_pages/offline_page_model_unittest.cc (right):

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
components/offline_pages/offline_page_model_unittest.cc:289:
run_loop_->RunUntilIdle();
On 2015/10/20 21:56:49, jianli wrote:
> Will base::RunLoop().RunUntilIdle() work?

Done.

[jianli, 2015-10-21 21:21:12]

lgtm

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
File components/offline_pages/offline_page_model.cc (right):

https://codereview.chromium.org/1397233002/diff/160001/components/offline_pag...
components/offline_pages/offline_page_model.cc:357: Observer, observers_,
OfflinePageDeleted(offline_page_item.bookmark_id));
On 2015/10/21 20:01:33, fgorski wrote:
> On 2015/10/20 21:56:48, jianli wrote:
> > I think we should be firing both OfflinePageModelChanged and
> OfflinePageDeleted
> > events.
> 
> I think triggering OfflinePageDeleted is enough and to the point. Plus it
> carries specific information which helps observers react, while the other is
> very generic and does not.
> 
> If we fire both events the UI listening to these might make a bookmark
specific
> update, followed by complete update, because you don't even know what just
> happened.

Yes, we should avoid this. I now understand how bookmark model code handles
these events. I will fix this in another patch after you land this patch.

[fgorski, 2015-10-21 22:42:50]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2015-10-21 22:42:50]

The patchset sent to the CQ was uploaded after l-g-t-m from ianwen@chromium.org
Link to the patchset: https://codereview.chromium.org/1397233002/#ps170001
(title: "Updates based on the latest code review feedback")

[commit-bot: I haz the power, 2015-10-21 22:43:28]

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

[commit-bot: I haz the power, 2015-10-21 23:01:21]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2015-10-21 23:02:46]

Patchset 10 (id:??) landed as
https://crrev.com/9dac2866f1248d7dccccc90ecc84c7c1ee9d87b8
Cr-Commit-Position: refs/heads/master@{#355423}