Update access info when an offline page is being visited

Update access info when an offline page is being visited

We need to update last access time and increase access count.

BUG=491352
Patch

Committed: https://crrev.com/cf6162c9d4b7eee6b58075079650467dbf6cebf7
Cr-Commit-Position: refs/heads/master@{#350425}

[jianli, 2015-09-16 20:05:57]

Patchset #1 (id:1) has been deleted

[jianli, 2015-09-16 20:07:14]

jianli@chromium.org changed reviewers:
+ dimich@chromium.org, ianwen@chromium.org

[jianli, 2015-09-16 20:07:15]

dimich for offline pages
ianwen for enhanced bookmarks

[Ian Wen, 2015-09-17 00:20:11]

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

https://codereview.chromium.org/1345043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:250:
public void onAccessPageDone(boolean success) {
Do you plan to use this interface/method in other places in future? In this CL I
only see it being called and used in tests.

If it is only a method to make testing easier, I would not enforce the caller of
accessPage to new a useless object.

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

https://codereview.chromium.org/1345043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:184:
public void accessPage(BookmarkId bookmarkId, AccessPageCallback callback) {
If the function means "mark the page have accessed", how about renaming it to
markPageAccessed(), and update the javadoc?

"Access a page" sounds a big awkward...

[Dmitry Titov, 2015-09-17 18:49:22]

LGTM for offline pages, +1 for Ian's comments about callback and naming.

[jianli, 2015-09-18 00:16:12]

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

https://codereview.chromium.org/1345043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:250:
public void onAccessPageDone(boolean success) {
On 2015/09/17 00:20:11, Ian Wen wrote:
> Do you plan to use this interface/method in other places in future? In this CL
I
> only see it being called and used in tests.
> 
> If it is only a method to make testing easier, I would not enforce the caller
of
> accessPage to new a useless object.

Changed to support not passing callback from a caller.

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

https://codereview.chromium.org/1345043002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:184:
public void accessPage(BookmarkId bookmarkId, AccessPageCallback callback) {
On 2015/09/17 00:20:11, Ian Wen wrote:
> If the function means "mark the page have accessed", how about renaming it to
> markPageAccessed(), and update the javadoc?
> 
> "Access a page" sounds a big awkward...

Done.

[Ian Wen, 2015-09-18 21:20:44]

lgtm after comments. :)

https://codereview.chromium.org/1345043002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java
(right):

https://codereview.chromium.org/1345043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:241:
if (page == null) {
Inline the return.

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

https://codereview.chromium.org/1345043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:184:
public void markPageAccessed(BookmarkId bookmarkId, MarkPageAccessedCallback
callback) {
How about add another public method called "markPageAccessedForTest(BookmarkId,
MarkPageAccessedCallback)", and change this method to not have the callback
parameter?

[jianli, 2015-09-18 21:41:23]

Patchset #5 (id:100001) has been deleted

[jianli, 2015-09-18 21:42:29]

https://codereview.chromium.org/1345043002/diff/80001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java
(right):

https://codereview.chromium.org/1345043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:241:
if (page == null) {
On 2015/09/18 21:20:44, Ian Wen wrote:
> Inline the return.

Done.

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

https://codereview.chromium.org/1345043002/diff/80001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:184:
public void markPageAccessed(BookmarkId bookmarkId, MarkPageAccessedCallback
callback) {
On 2015/09/18 21:20:44, Ian Wen wrote:
> How about add another public method called
"markPageAccessedForTest(BookmarkId,
> MarkPageAccessedCallback)", and change this method to not have the callback
> parameter?

Done.

[jianli, 2015-09-18 21:48:13]

The CQ bit was checked by jianli@chromium.org

[jianli, 2015-09-18 21:48:14]

The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org,
ianwen@chromium.org
Link to the patchset: https://codereview.chromium.org/1345043002/#ps120001
(title: "Address some more feedback")

[commit-bot: I haz the power, 2015-09-18 21:49:17]

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

[commit-bot: I haz the power, 2015-09-18 22:05:10]

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

[commit-bot: I haz the power, 2015-09-18 22:05:11]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jianli, 2015-09-18 22:18:18]

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

[jianli, 2015-09-18 22:18:18]

newt, could you please approve Java test change? Thanks.

[newt (away), 2015-09-21 23:02:05]

https://codereview.chromium.org/1345043002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java
(right):

https://codereview.chromium.org/1345043002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:233:
public String getBookmarkLaunchUrl(BookmarkId bookmarkId) {
Eek. I never want to program against an API where getter methods have
side-effects. This seems very prone to breakage (e.g. access counts being
updated twice).

See "Misleading names" in this wonderful document:
http://www.se.rit.edu/~tabeec/RIT_441/Resources_files/How%20To%20Write%20Unma...

Either rename this method to make it clear that it has side-effects, or split
this into two separate methods.

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

https://codereview.chromium.org/1345043002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:51:
* Callback used to marking an offline page accessed.
s/marking/mark/

https://codereview.chromium.org/1345043002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:53:
public interface MarkPageAccessedCallback {
If MarkPageAccessedCallback is only used for tests, I'd remove it to keep things
simpler, and adjust the test slightly (see my comment in the test file).

https://codereview.chromium.org/1345043002/diff/120001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java
(right):

https://codereview.chromium.org/1345043002/diff/120001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java:164:
final Semaphore semaphore = new Semaphore(0);
If you remove MarkPageAccessedCallback (which I'd suggest doing), then you can
replace this code with Chrome's CriteriaHelper class:

    assertTrue(CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
        @Override
        public boolean isSatisfied() {
            OfflinePageItem offlinePage =
mOfflinePageBridge.getPageByBookmarkId(BOOKMARK_ID);
            return offlinePage.getAccessCount() == 1;
        }
    }));

This is both less test code and less production code :)

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

https://codereview.chromium.org/1345043002/diff/120001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.cc:48: const
ScopedJavaGlobalRef<jobject>& j_callback_obj, bool sucecss) {
"success" typo

[jianli, 2015-09-23 00:11:01]

Patchset #6 (id:140001) has been deleted

[jianli, 2015-09-23 00:11:11]

https://codereview.chromium.org/1345043002/diff/120001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java
(right):

https://codereview.chromium.org/1345043002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:233:
public String getBookmarkLaunchUrl(BookmarkId bookmarkId) {
On 2015/09/21 23:02:05, newt wrote:
> Eek. I never want to program against an API where getter methods have
> side-effects. This seems very prone to breakage (e.g. access counts being
> updated twice).
> 
> See "Misleading names" in this wonderful document:
>
http://www.se.rit.edu/~tabeec/RIT_441/Resources_files/How%20To%20Write%20Unma...
> 
> Either rename this method to make it clear that it has side-effects, or split
> this into two separate methods.

Split into 2 methods.

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

https://codereview.chromium.org/1345043002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:51:
* Callback used to marking an offline page accessed.
On 2015/09/21 23:02:05, newt wrote:
> s/marking/mark/

Done.

https://codereview.chromium.org/1345043002/diff/120001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:53:
public interface MarkPageAccessedCallback {
On 2015/09/21 23:02:05, newt wrote:
> If MarkPageAccessedCallback is only used for tests, I'd remove it to keep
things
> simpler, and adjust the test slightly (see my comment in the test file).

Done.

https://codereview.chromium.org/1345043002/diff/120001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java
(right):

https://codereview.chromium.org/1345043002/diff/120001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java:164:
final Semaphore semaphore = new Semaphore(0);
On 2015/09/21 23:02:05, newt wrote:
> If you remove MarkPageAccessedCallback (which I'd suggest doing), then you can
> replace this code with Chrome's CriteriaHelper class:
> 
>     assertTrue(CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
>         @Override
>         public boolean isSatisfied() {
>             OfflinePageItem offlinePage =
> mOfflinePageBridge.getPageByBookmarkId(BOOKMARK_ID);
>             return offlinePage.getAccessCount() == 1;
>         }
>     }));
> 
> This is both less test code and less production code :)

Done.

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

https://codereview.chromium.org/1345043002/diff/120001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_bridge.cc:48: const
ScopedJavaGlobalRef<jobject>& j_callback_obj, bool sucecss) {
On 2015/09/21 23:02:05, newt wrote:
> "success" typo

This callback is not longer needed.

[newt (away), 2015-09-23 21:52:56]

https://codereview.chromium.org/1345043002/diff/160001/chrome/android/java/pr...
File chrome/android/java/proguard.flags (right):

https://codereview.chromium.org/1345043002/diff/160001/chrome/android/java/pr...
chrome/android/java/proguard.flags:138: #-keep public class
android.net.http.SslCertificate
why this change? remove this.

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

https://codereview.chromium.org/1345043002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:236:
public Pair<String, Boolean> getBookmarkLaunchUrl(BookmarkId bookmarkId) {
Hmm. Using Pair<> makes the API complicated and hard to understand at the call
site. How about just having one method like before but renaming it to
getLaunchUrlAndMarkAccessed().

https://codereview.chromium.org/1345043002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:283:
* * @param bookmarkId Bookmark ID for the offline page.
remove "* "

https://codereview.chromium.org/1345043002/diff/160001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java
(right):

https://codereview.chromium.org/1345043002/diff/160001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java:170:
try {
lines 170-180 should be outside of the inner class Runnable. e.g.

  ThreadUtils.runOnUiThreadBlocking(new Runnable() {
     ...
  });
  assertTrue(CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
     ...
  });

since you can't poll the UI thread from inside the UI thread.

But actually, in this case, it looks like polling isn't needed at all. The
access count will be updated synchronously, right? So you could just write:

  ThreadUtils.runOnUiThreadBlocking(new Runnable() {
     ...
     mOfflinePageBridge.markPageAccessed(bookmarkId);
     OfflinePageItem offlinePage = ...
     assertEquals(expectedAccessCount, offlinePage.getAccessCount());
  });

[jianli, 2015-09-23 23:20:40]

Patchset #7 (id:180001) has been deleted

[jianli, 2015-09-23 23:21:00]

Patchset #7 (id:200001) has been deleted

[jianli, 2015-09-23 23:21:07]

https://codereview.chromium.org/1345043002/diff/160001/chrome/android/java/pr...
File chrome/android/java/proguard.flags (right):

https://codereview.chromium.org/1345043002/diff/160001/chrome/android/java/pr...
chrome/android/java/proguard.flags:138: #-keep public class
android.net.http.SslCertificate
On 2015/09/23 21:52:56, newt wrote:
> why this change? remove this.

Sorry, this is to work around my local build problem and should not be here.

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

https://codereview.chromium.org/1345043002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:236:
public Pair<String, Boolean> getBookmarkLaunchUrl(BookmarkId bookmarkId) {
On 2015/09/23 21:52:56, newt wrote:
> Hmm. Using Pair<> makes the API complicated and hard to understand at the call
> site. How about just having one method like before but renaming it to
> getLaunchUrlAndMarkAccessed().

Ok. Previously I think we might have getLaunchUrl being called for some other
cases. Renamed to getLaunchUrlAndMarkAccessed. If we do have other case, I will
make change then.

https://codereview.chromium.org/1345043002/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarksModel.java:283:
* * @param bookmarkId Bookmark ID for the offline page.
On 2015/09/23 21:52:56, newt wrote:
> remove "* "

Removed since it is not longed needed.

https://codereview.chromium.org/1345043002/diff/160001/chrome/android/javates...
File
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java
(right):

https://codereview.chromium.org/1345043002/diff/160001/chrome/android/javates...
chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridgeTest.java:170:
try {
On 2015/09/23 21:52:56, newt wrote:
> lines 170-180 should be outside of the inner class Runnable. e.g.
> 
>   ThreadUtils.runOnUiThreadBlocking(new Runnable() {
>      ...
>   });
>   assertTrue(CriteriaHelper.pollForUIThreadCriteria(new Criteria() {
>      ...
>   });
> 
> since you can't poll the UI thread from inside the UI thread.
> 
> But actually, in this case, it looks like polling isn't needed at all. The
> access count will be updated synchronously, right? So you could just write:
> 
>   ThreadUtils.runOnUiThreadBlocking(new Runnable() {
>      ...
>      mOfflinePageBridge.markPageAccessed(bookmarkId);
>      OfflinePageItem offlinePage = ...
>      assertEquals(expectedAccessCount, offlinePage.getAccessCount());
>   });
> 

The access count should not be updated synchronously. We should only update the
cache upon successful store write. I have found out this problem when I was
working on making markPageAccessed test work but I reverted this fix when I
reverted the temporary. I  logging change. I now bring it back in
offline_page_model.cc. I changed to pull the polling part out.

[newt (away), 2015-09-23 23:23:36]

everything in chrome/ lgtm

[jianli, 2015-09-23 23:45:48]

The CQ bit was checked by jianli@chromium.org

[jianli, 2015-09-23 23:45:48]

The patchset sent to the CQ was uploaded after l-g-t-m from ianwen@chromium.org,
dimich@chromium.org
Link to the patchset: https://codereview.chromium.org/1345043002/#ps220001
(title: "Some more fixes")

[commit-bot: I haz the power, 2015-09-23 23:48:55]

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

[commit-bot: I haz the power, 2015-09-24 00:59:39]

Committed patchset #7 (id:220001)

[commit-bot: I haz the power, 2015-09-24 01:00:07]

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