[Offline pages] Adding capability to free up space used by Offline pages

[Offline pages] Adding capability to free up space used by Offline pages

In this patch:
* Offline Page Bridge is extended to delete multiple pages as well as
  pull out a list of candidate pages for clean up.
* Offline Page Model is extended to offer a list of pages to clean up
* A dialog to free up space is added
* A header is added in a Saved offline section of bookmarks UI to offer
  cleaning up space (through the dialog above). (Header is shown, when
  number of pages to delete reaches a certain threshold).

In order to test the functionality (which triggers with a month delay):
* bring thresholds to 0
* update code offering pages for clean up (e.g. half of the existing
  set).

TEST=tested manually after modifications applied.
BUG=491352
R=jianli@chromium.org,kkimlabs@chromium.org

Committed: https://crrev.com/683028f51e4aaae0782d1583f33c401436f9cd37
Cr-Commit-Position: refs/heads/master@{#345804}

[fgorski, 2015-08-21 01:53:58]

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

[fgorski, 2015-08-21 01:53:58]

Please take a look.

kkimlabs@: enhanced bookmark code
newt@: layout, strings and android side of offline_pages + bridge
jianli@: model and overall.

thanks.
Filip

[Kibeom Kim (inactive), 2015-08-21 20:19:39]

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/eb_offline_pages_storage_space_header.xml
(right):

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/eb_offline_pages_storage_space_header.xml:8:
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
eb_promo_header and this is quite similar so I think it's good to unify. but not
really that important, could you add this to eb_promo_header.xml file?

<!-- TODO(kkimlabs): Generalize and unify
eb_offline_pages_storage_space_header.xml -->

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/eb_offline_pages_storage_space_header.xml:16:
android:id="@+id/offline_pages_storage_title"
fyi (maybe you already knew, but just in case):

android:id doesn't need to be globally unique, as long as it's local to the View
where we call findViewById().

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

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:51:
private int mCurrentHeader = -1;
minor suggestion:

I think it's good to reduce states to keep track in general. How about using
mDelegate#getCurrentState() instead of this variable.

So |void setCurrentHeader(int ...)| will become |void updateHeader()| and just
check the state there. also at #253.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:278:
setCurrentHeader(PROMO_HEADER_VIEW);
Actually, we always set some state after
onEnhancedBookmarkDelegateInitialized(), so we don't need to update header here
as now we update promo header on state changes. Also below
updateDividerSections() weren't needed (my fault)

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:321:
return;
I think in general we don't handle assert fall back gracefully unless it's
related to a security issue, so just:

assert headerViewType == PROMO_HEADER_VIEW || headerViewType ==
OFFLINE_PAGES_STORAGE_VIEW : "Incorrect header view type.";

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java
(right):

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:28:
final FreeUpSpaceCallback mCallback;
nit: private for these variables?

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:40:
* is finished, it should be taken care of in the callback implementation.
Hmm... this is subtle because OfflinePageBridge is destroyed by
OfflinePageBridge#destroy() call and there is no guarantee that this dialog
instance is garbage collected before that. But I guess it should be fine as this
dialog will outlive the underlying enhanced bookmark UI.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:51:
}
Android tries to call an empty constructor on DialogFragmnet recreation(steps
mentioned below) so I believe this will crash in that case.

So making a static newInstance(...) constructor is convention.
http://developer.android.com/intl/ko/reference/android/app/DialogFragment.html

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:54:
public Dialog onCreateDialog(Bundle savedInstanceState) {
I believe that this is recreated when Chrome activity is recreated.

Steps:
1. Android settings -> Developer options -> Turn on "Don't keep activities"
2. Open this dialog
3. Switch to another app
4. Come back to Chrome

I don't think restoring this dialog is necessary so let's just dismiss.

FYI: I had to fix the same issue for another DialogFragment
https://codereview.chromium.org/1248373003/

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:60:
if (mOfflinePageBridge != null) {
I believe that this is not needed?

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java
(right):

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java:27:
private static final long MINIMUM_CEALNUP_SIZE_TO_OFFER = 5 * (1 << 20); // 5MB
suggestion: include unit (BYTE) in the constant name? Also for getSize private
functions below.

[jianli, 2015-08-21 21:06:27]

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

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:271:
mDelegate.getModel().getOfflinePageBridge(), new StorageSpaceHeaderCallback() {
I believe you will hit an assert in OfflinePageStorageSpaceHeader constructor
when offline pages feature is not enabled. Should we create
OfflinePageStorageSpaceHeader if it is not enabled?

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:318:
private void setCurrentHeader(int headerViewType) {
Please comment this method for better understanding its purpose.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java
(right):

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:22:
* Helper class showing the dialog to allowing to "Free up space" taken by
offline pages.
I don't think this is a helper class. How about:
  Shows a "Free up space" dialog to clean up storage for offline pages.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:35:
void onCleanUpSpaceDone();
add public

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:73:
if (id != AlertDialog.BUTTON_POSITIVE || mOfflinePageBridge == null) {
Since we already assert at line 48, the 2nd condition check should not be
needed.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java
(right):

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java:26:
private static final long MINIMUM_TOTAL_SIZE_TO_OFFER = 10 * (1 << 20); // 10MB
I found out the suffix "TO_OFFER" was hard to understand.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java:36:
void onStorageSpaceCleanUpDone();
add public

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

https://codereview.chromium.org/1307753002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:25: const base::TimeDelta
OLD_PAGE_INTERVAL = base::TimeDelta::FromDays(30);
We should use C++ style constant here. Also it is hard to tell what
OLD_PAGE_INTERVAL is used for. How about naming it like
kOldPageCleanupThreshold. Please also comment it.

https://codereview.chromium.org/1307753002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:151:
offline_pages.push_back(id_page.second);
nit: it is better to store Now time before for loop.

[Kibeom Kim (inactive), 2015-08-21 21:15:08]

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java
(right):

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:40:
* is finished, it should be taken care of in the callback implementation.
On 2015/08/21 20:19:39, Kibeom Kim wrote:
> Hmm... this is subtle because OfflinePageBridge is destroyed by
> OfflinePageBridge#destroy() call and there is no guarantee that this dialog
> instance is garbage collected before that. But I guess it should be fine as
this
> dialog will outlive the underlying enhanced bookmark UI.

Correction:
Said the last sentence backward, and what I meant to say was that this class
won't be using mOfflinePageBridge after the underlying enhanced bookmark UI even
if it's garbage collected later.

[fgorski, 2015-08-21 22:09:39]

Addressed. PTAL

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/res...
File chrome/android/java/res/layout/eb_offline_pages_storage_space_header.xml
(right):

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/eb_offline_pages_storage_space_header.xml:8:
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
On 2015/08/21 20:19:39, Kibeom Kim wrote:
> eb_promo_header and this is quite similar so I think it's good to unify. but
not
> really that important, could you add this to eb_promo_header.xml file?
> 
> <!-- TODO(kkimlabs): Generalize and unify
> eb_offline_pages_storage_space_header.xml -->

Done.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/res...
chrome/android/java/res/layout/eb_offline_pages_storage_space_header.xml:16:
android:id="@+id/offline_pages_storage_title"
On 2015/08/21 20:19:39, Kibeom Kim wrote:
> fyi (maybe you already knew, but just in case):
> 
> android:id doesn't need to be globally unique, as long as it's local to the
View
> where we call findViewById().

I suspected that. Thanks for confirming.
I decided to simplify the naming a bit.

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

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:51:
private int mCurrentHeader = -1;
On 2015/08/21 20:19:39, Kibeom Kim wrote:
> minor suggestion:
> 
> I think it's good to reduce states to keep track in general. How about using
> mDelegate#getCurrentState() instead of this variable.
> 
> So |void setCurrentHeader(int ...)| will become |void updateHeader()| and just
> check the state there. also at #253.

Done.
I made a relevant update, but i think for applying that solution long term, we
should also move UIState from EnhancedBookmarkManager (If this is the official
set of values for the EBDelegte.getCurrentState()

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:271:
mDelegate.getModel().getOfflinePageBridge(), new StorageSpaceHeaderCallback() {
On 2015/08/21 21:06:27, jianli wrote:
> I believe you will hit an assert in OfflinePageStorageSpaceHeader constructor
> when offline pages feature is not enabled. Should we create
> OfflinePageStorageSpaceHeader if it is not enabled?

Done.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:278:
setCurrentHeader(PROMO_HEADER_VIEW);
On 2015/08/21 20:19:39, Kibeom Kim wrote:
> Actually, we always set some state after
> onEnhancedBookmarkDelegateInitialized(), so we don't need to update header
here
> as now we update promo header on state changes. Also below
> updateDividerSections() weren't needed (my fault)

Done.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:318:
private void setCurrentHeader(int headerViewType) {
On 2015/08/21 21:06:27, jianli wrote:
> Please comment this method for better understanding its purpose.

It was renamed to updateHeader, and that is exactly what it is doing.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:321:
return;
On 2015/08/21 20:19:39, Kibeom Kim wrote:
> I think in general we don't handle assert fall back gracefully unless it's
> related to a security issue, so just:
> 
> assert headerViewType == PROMO_HEADER_VIEW || headerViewType ==
> OFFLINE_PAGES_STORAGE_VIEW : "Incorrect header view type.";

Done. And then updated to updateHeader approach.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java
(right):

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:22:
* Helper class showing the dialog to allowing to "Free up space" taken by
offline pages.
On 2015/08/21 21:06:27, jianli wrote:
> I don't think this is a helper class. How about:
>   Shows a "Free up space" dialog to clean up storage for offline pages.

Done.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:28:
final FreeUpSpaceCallback mCallback;
On 2015/08/21 20:19:39, Kibeom Kim wrote:
> nit: private for these variables?

Done. Thanks for catching that.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:35:
void onCleanUpSpaceDone();
On 2015/08/21 21:06:27, jianli wrote:
> add public

Interface methods are public by default.

http://docs.oracle.com/javase/specs/jls/se7/html/jls-9.html#jls-9.4

It is permitted, but discouraged as a matter of style, to redundantly
specify the public and/or abstract modifier for a method declared
in an interface.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:40:
* is finished, it should be taken care of in the callback implementation.
On 2015/08/21 21:15:08, Kibeom Kim wrote:
> On 2015/08/21 20:19:39, Kibeom Kim wrote:
> > Hmm... this is subtle because OfflinePageBridge is destroyed by
> > OfflinePageBridge#destroy() call and there is no guarantee that this dialog
> > instance is garbage collected before that. But I guess it should be fine as
> this
> > dialog will outlive the underlying enhanced bookmark UI.
> 
> Correction:
> Said the last sentence backward, and what I meant to say was that this class
> won't be using mOfflinePageBridge after the underlying enhanced bookmark UI
even
> if it's garbage collected later.

This dialog will also be shown after a snackbar:
1. Create a bookmark with offline copy
2. Wait, we are short on space, let's tell the user:
3. Show snackbar that says, you saved the page, but run low on space:
https://codereview.chromium.org/1305883002/
4. User clicks the snackbar and gets the "free up space" dialog, and that is
   where the EBModel with OPBridge might be kept for a little longer.

In the header scenario I agree with you.

Does that sound reasonable?

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:51:
}
On 2015/08/21 20:19:39, Kibeom Kim wrote:
> Android tries to call an empty constructor on DialogFragmnet recreation(steps
> mentioned below) so I believe this will crash in that case.
> 
> So making a static newInstance(...) constructor is convention.
> http://developer.android.com/intl/ko/reference/android/app/DialogFragment.html

Done.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:54:
public Dialog onCreateDialog(Bundle savedInstanceState) {
On 2015/08/21 20:19:39, Kibeom Kim wrote:
> I believe that this is recreated when Chrome activity is recreated.
> 
> Steps:
> 1. Android settings -> Developer options -> Turn on "Don't keep activities"
> 2. Open this dialog
> 3. Switch to another app
> 4. Come back to Chrome
> 
> I don't think restoring this dialog is necessary so let's just dismiss.
> 
> FYI: I had to fix the same issue for another DialogFragment
> https://codereview.chromium.org/1248373003/

Done.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:60:
if (mOfflinePageBridge != null) {
On 2015/08/21 20:19:39, Kibeom Kim wrote:
> I believe that this is not needed?

Done.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:73:
if (id != AlertDialog.BUTTON_POSITIVE || mOfflinePageBridge == null) {
On 2015/08/21 21:06:27, jianli wrote:
> Since we already assert at line 48, the 2nd condition check should not be
> needed.

Done.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java
(right):

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java:26:
private static final long MINIMUM_TOTAL_SIZE_TO_OFFER = 10 * (1 << 20); // 10MB
On 2015/08/21 21:06:27, jianli wrote:
> I found out the suffix "TO_OFFER" was hard to understand.

Done.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java:27:
private static final long MINIMUM_CEALNUP_SIZE_TO_OFFER = 5 * (1 << 20); // 5MB
On 2015/08/21 20:19:39, Kibeom Kim wrote:
> suggestion: include unit (BYTE) in the constant name? Also for getSize private
> functions below.

Done.

https://codereview.chromium.org/1307753002/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java:36:
void onStorageSpaceCleanUpDone();
On 2015/08/21 21:06:27, jianli wrote:
> add public

not necessary per previous comment.

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

https://codereview.chromium.org/1307753002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:25: const base::TimeDelta
OLD_PAGE_INTERVAL = base::TimeDelta::FromDays(30);
On 2015/08/21 21:06:27, jianli wrote:
> We should use C++ style constant here. Also it is hard to tell what
> OLD_PAGE_INTERVAL is used for. How about naming it like
> kOldPageCleanupThreshold. Please also comment it.

Done.

https://codereview.chromium.org/1307753002/diff/20001/components/offline_page...
components/offline_pages/offline_page_model.cc:151:
offline_pages.push_back(id_page.second);
On 2015/08/21 21:06:27, jianli wrote:
> nit: it is better to store Now time before for loop.

Done.

[jianli, 2015-08-21 23:39:22]

lgtm

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

https://codereview.chromium.org/1307753002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:271:
if (mDelegate.getModel().getOfflinePageBridge() != null) {
nit: cache mDelegate.getModel().getOfflinePageBridge() since you also need it
below

https://codereview.chromium.org/1307753002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java
(right):

https://codereview.chromium.org/1307753002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java:26:
/** Minim total size of all pages, before a header will show to offer freeing up
space. */
Minim?

will show => will be shown

https://codereview.chromium.org/1307753002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java:28:
/** Minim size of pages to clean up, before a header will show to offer freeing
up space. */
ditto

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

https://codereview.chromium.org/1307753002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.cc:25: const base::TimeDelta
kPageCleanUpThreshold = base::TimeDelta::FromDays(30);
nit: add comment like: pages which are created 30 days earlier will be deleted
in order to free up space.

https://codereview.chromium.org/1307753002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.cc:146: const
std::vector<OfflinePageItem> OfflinePageModel::GetPagesToCleanUp() const {
add test

[Kibeom Kim (inactive), 2015-08-24 18:30:38]

lgtm

[fgorski, 2015-08-24 18:47:14]

Addressed feedback from Jian Li.

Still working on bitness problems in compilation on arm64

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

https://codereview.chromium.org/1307753002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkItemsAdapter.java:271:
if (mDelegate.getModel().getOfflinePageBridge() != null) {
On 2015/08/21 23:39:21, jianli wrote:
> nit: cache mDelegate.getModel().getOfflinePageBridge() since you also need it
> below

Done.

https://codereview.chromium.org/1307753002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java
(right):

https://codereview.chromium.org/1307753002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java:26:
/** Minim total size of all pages, before a header will show to offer freeing up
space. */
On 2015/08/21 23:39:21, jianli wrote:
> Minim?
> 
> will show => will be shown

Done.

https://codereview.chromium.org/1307753002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java:28:
/** Minim size of pages to clean up, before a header will show to offer freeing
up space. */
On 2015/08/21 23:39:21, jianli wrote:
> ditto

Done.

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

https://codereview.chromium.org/1307753002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.cc:25: const base::TimeDelta
kPageCleanUpThreshold = base::TimeDelta::FromDays(30);
On 2015/08/21 23:39:21, jianli wrote:
> nit: add comment like: pages which are created 30 days earlier will be deleted
> in order to free up space.

Done.

https://codereview.chromium.org/1307753002/diff/40001/components/offline_page...
components/offline_pages/offline_page_model.cc:146: const
std::vector<OfflinePageItem> OfflinePageModel::GetPagesToCleanUp() const {
On 2015/08/21 23:39:21, jianli wrote:
> add test

Done.

[newt (away), 2015-08-24 22:37:09]

Note on naming: Java package names should not contain underscores. It should be
"org.chromium.chrome.browser.offlinepages."

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java
(right):

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:35:
void onCleanUpSpaceDone();
Use "free up" or "clean up" consistently. Don't mix them as that reduces
predictability.

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:38:
public static OfflinePageFreeUpSpaceDialog newInstance(
public methods need javadoc

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:74:
if (id != AlertDialog.BUTTON_POSITIVE) {
why "!=" ?? It seems clearer to write "if (id == AlertDialog.BUTTON_NEGATIVE)"

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:76:
if (mCallback != null) mCallback.onCleanUpSpaceDone();
Calling onCleanUpSpaceDone() doesn't seem right, since the space wasn't actually
cleaned up. If you want to differentiate between the cleaned-up cases and the
canceled case, you should add another method "onCanceled()" to
FreeUpSpaceCallback; otherwise, rename "onCleanUpSpaceDone()" to
"onDialogDismissed()" or something like that.

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:88:
/** @return A list of Bookmark IDs for which the offline pages will be deleted.
*/
nit: no need for "@return" for single-line javadocs. You can just write "Returns
a list..."

https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java
(right):

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java:40:
public interface StorageSpaceHeaderCallback {
StorageSpaceHeaderCallback is just a copy of FreeUpSpaceCallback. Why not just
use FreeUpSpaceCallback directly and delete StorageSpaceHeaderCallback?

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java:82:
mCallback.onStorageSpaceCleanUpDone();
Too much nesting. Please rearrange this a bit to reduce nesting.

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1860: <message
name="IDS_OFFLINE_PAGES_FREE_UP_SPACE_TITLE" desc="Title of the dialog offering
to remove offline pages, to clean up storage and text on a button triggering
that dialog. [CHAR-LIMIT=32]">
This description is quite confusing. How about "Title of the dialog offering to
remove offline pages to free up storage space. Also used as the label on a
button to show this dialog."

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

https://codereview.chromium.org/1307753002/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.cc:179:
scoped_ptr<ScopedJavaGlobalRef<jobject>> j_callback_obj_ptr(
Don't wrap ScopedJavaGlobalRef in a scoped_ptr. Just use ScopedJavaGlobalRef.
Please fix the other occurrences of this in this file.

[fgorski, 2015-08-25 18:25:25]

As for package naming. ianwen@ pointed me to that issue in our code few weeks
back and I put it on our list of things to fix. At the time we had plenty of
patches in progress and we didn't want to address it, but we will get down to
that in M47. Sounds good?

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java
(right):

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:35:
void onCleanUpSpaceDone();
On 2015/08/24 22:37:08, newt wrote:
> Use "free up" or "clean up" consistently. Don't mix them as that reduces
> predictability.

Done.

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:38:
public static OfflinePageFreeUpSpaceDialog newInstance(
On 2015/08/24 22:37:08, newt wrote:
> public methods need javadoc

Done. For some reason javadoc was misplaced.

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:74:
if (id != AlertDialog.BUTTON_POSITIVE) {
On 2015/08/24 22:37:08, newt wrote:
> why "!=" ?? It seems clearer to write "if (id == AlertDialog.BUTTON_NEGATIVE)"

Done.

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:76:
if (mCallback != null) mCallback.onCleanUpSpaceDone();
On 2015/08/24 22:37:08, newt wrote:
> Calling onCleanUpSpaceDone() doesn't seem right, since the space wasn't
actually
> cleaned up. If you want to differentiate between the cleaned-up cases and the
> canceled case, you should add another method "onCanceled()" to
> FreeUpSpaceCallback; otherwise, rename "onCleanUpSpaceDone()" to
> "onDialogDismissed()" or something like that.

Done.

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:88:
/** @return A list of Bookmark IDs for which the offline pages will be deleted.
*/
On 2015/08/24 22:37:08, newt wrote:
> nit: no need for "@return" for single-line javadocs. You can just write
"Returns
> a list..."
> 
> https://google.github.io/styleguide/javaguide.html#s7.2-summary-fragment

Done.

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java
(right):

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java:40:
public interface StorageSpaceHeaderCallback {
On 2015/08/24 22:37:08, newt wrote:
> StorageSpaceHeaderCallback is just a copy of FreeUpSpaceCallback. Why not just
> use FreeUpSpaceCallback directly and delete StorageSpaceHeaderCallback?

Done. You are correct. Extracted the interface to a separate file and added a
cancel case.

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageStorageSpaceHeader.java:82:
mCallback.onStorageSpaceCleanUpDone();
On 2015/08/24 22:37:08, newt wrote:
> Too much nesting. Please rearrange this a bit to reduce nesting.

Done. By removing the extra interface it took care of itself. Let me know if you
want more clean up.

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1860: <message
name="IDS_OFFLINE_PAGES_FREE_UP_SPACE_TITLE" desc="Title of the dialog offering
to remove offline pages, to clean up storage and text on a button triggering
that dialog. [CHAR-LIMIT=32]">
On 2015/08/24 22:37:08, newt wrote:
> This description is quite confusing. How about "Title of the dialog offering
to
> remove offline pages to free up storage space. Also used as the label on a
> button to show this dialog."

Done.

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

https://codereview.chromium.org/1307753002/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.cc:179:
scoped_ptr<ScopedJavaGlobalRef<jobject>> j_callback_obj_ptr(
On 2015/08/24 22:37:08, newt wrote:
> Don't wrap ScopedJavaGlobalRef in a scoped_ptr. Just use ScopedJavaGlobalRef.
> Please fix the other occurrences of this in this file.

Done.

[fgorski, 2015-08-25 18:28:27]

self review, noticed omission. Waiting for the tests to finish to fix the string
documentation

https://codereview.chromium.org/1307753002/diff/140001/chrome/android/java/st...
File chrome/android/java/strings/android_chrome_strings.grd (right):

https://codereview.chromium.org/1307753002/diff/140001/chrome/android/java/st...
chrome/android/java/strings/android_chrome_strings.grd:1860: <message
name="IDS_OFFLINE_PAGES_FREE_UP_SPACE_TITLE" desc="Text on a button and title of
the dialog offering to remove offline pages free up storage space.
[CHAR-LIMIT=32]">
noticed omission. will fix

[fgorski, 2015-08-27 00:26:15]

newt@, friendly ping ;)

[newt (away), 2015-08-27 03:13:27]

lgtm

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java
(right):

https://codereview.chromium.org/1307753002/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offline_pages/OfflinePageFreeUpSpaceDialog.java:38:
public static OfflinePageFreeUpSpaceDialog newInstance(
On 2015/08/25 18:25:25, fgorski wrote:
> On 2015/08/24 22:37:08, newt wrote:
> > public methods need javadoc
> 
> Done. For some reason javadoc was misplaced.

Oh weird. Better now :)

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

https://codereview.chromium.org/1307753002/diff/100001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.cc:179:
scoped_ptr<ScopedJavaGlobalRef<jobject>> j_callback_obj_ptr(
On 2015/08/25 18:25:25, fgorski wrote:
> On 2015/08/24 22:37:08, newt wrote:
> > Don't wrap ScopedJavaGlobalRef in a scoped_ptr. Just use
ScopedJavaGlobalRef.
> > Please fix the other occurrences of this in this file.
> 
> Done.

Thanks for this cleanup.

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

https://codereview.chromium.org/1307753002/diff/140001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.cc:138:
ScopedJavaGlobalRef<jobject> j_callback_ref;
You could just use the two-arg constructor, instead of calling Reset:

  ScopedJavaGlobalRef<jobject> j_callback_ref(env, j_callback_obj)

Doesn't really matter either way, though.

[fgorski, 2015-08-27 05:34:27]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2015-08-27 05:34:28]

The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org,
kkimlabs@chromium.org
Link to the patchset: https://codereview.chromium.org/1307753002/#ps170001
(title: "Converting jlongarray to std::vector<int64>")

[commit-bot: I haz the power, 2015-08-27 05:34:39]

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

[commit-bot: I haz the power, 2015-08-27 05:39:26]

Committed patchset #10 (id:170001)

[commit-bot: I haz the power, 2015-08-27 05:40:05]

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