[Offline pages] Removing offline pages from Bookmarks UI

[Offline pages] Removing offline pages from Bookmarks UI

* Removing all Offline page UI from Bookmarks UI
* Removing all offline page code from bookmarks, except page snapshotting
* Removing alternative offline page strings
* Removing bookmarks filtering options

* Relevant UMA will be marked obsolete separately

BUG=603940
R=jianli@chromiu.org,ianwen@chromium.org

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

[fgorski, 2016-04-15 21:55:17]

fgorski@chromium.org changed reviewers:
+ jianli@chromium.org
- jianli@chromiu.org

[fgorski, 2016-04-15 21:55:18]

PTAL.

This is first step to making automagic offline bookmarks.

[jianli, 2016-04-15 23:11:34]

https://codereview.chromium.org/1894703002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java
(right):

https://codereview.chromium.org/1894703002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:167:
// Always use the callback here.
nit: update the comment like the following for better readbility
  // Invoke the callback now since it is for bookmark adding result.

https://codereview.chromium.org/1894703002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:189:
OfflinePageBridge offlinePageBridge =
OfflinePageBridge.getForProfile(getProfile());
I think we still need to check OfflinePageBridge.isEnabled.

https://codereview.chromium.org/1894703002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://codereview.chromium.org/1894703002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:215:
* If the given {@link WebContents} is null, an option to visit the page is shown
nit: update comment

[jianli, 2016-04-15 23:17:20]

Please also remove OfflinePageBridge.getFeatureMode. You can keep using
nativeGetFeatureMode which I will remove later.

[fgorski, 2016-04-18 16:53:45]

All addressed.

https://codereview.chromium.org/1894703002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java
(right):

https://codereview.chromium.org/1894703002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:167:
// Always use the callback here.
On 2016/04/15 at 23:11:34, jianli wrote:
> nit: update the comment like the following for better readbility
>   // Invoke the callback now since it is for bookmark adding result.

Done.

https://codereview.chromium.org/1894703002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:189:
OfflinePageBridge offlinePageBridge =
OfflinePageBridge.getForProfile(getProfile());
On 2016/04/15 at 23:11:34, jianli wrote:
> I think we still need to check OfflinePageBridge.isEnabled.

Done.

https://codereview.chromium.org/1894703002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://codereview.chromium.org/1894703002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:215:
* If the given {@link WebContents} is null, an option to visit the page is shown
On 2016/04/15 at 23:11:34, jianli wrote:
> nit: update comment

done.

[Ian Wen, 2016-04-18 18:33:14]

Thanks for preparing such a large change!

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkActionBar.java
(right):

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkActionBar.java:58:
getMenu()
Reformoat #58 - #69? At lease getMenu() and findItem() should be on the same
line.

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkEditActivity.java
(right):

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkEditActivity.java:58:
setTitle(R.string.edit_bookmark);
Since we no longer need to modify the activity name programmatically, I would
remove #58 and #59, and in androidManifest.xml, add
android:label="@string/string.edit_bookmark".

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java
(right):

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:156:
public void addBookmarkAsync(BookmarkId parent, int index, String title, String
url,
This method is no longer async. I would rename it to
addBookmarkWithOfflinePage(). 

Also why do we still need AddBookmarkCallback?

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:185:
OfflinePageBridge offlinePageBridge =
OfflinePageBridge.getForProfile(getProfile());
You might want to use Profile#getLastUsedProfile().getOriginal()?

On Android there are only two profiles. A normal profile and its incognito
profile. Bookmarks are always stored in the normal profile.

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUIState.java
(right):

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUIState.java:66:
state.mShouldPersist = shouldPersist(uri);
mShouldPersist should be removed from this class, together with all the
processing related to it .

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:73:
bookmarkModel.addBookmarkAsync(parent, bookmarkModel.getChildCount(parent),
tab.getTitle(),
Since now bookmark and offline pages are decoupled, I suppose the cleanest
separation should be:
1. Remove addBookmarkAsync() and saveOfflinePage() in bookmark model.
2. Add one line in ChromeActivity#addOrEditBookmark() method to save the offline
page.

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:225:
* Opens a bookmark depending on connection status and reports UMA.
Update javadoc.

[fgorski, 2016-04-18 23:39:05]

PTAL.

I'll fix the missing documentation comment in a separate patch, because I
already started the build (and the new UI didn't show me the comment in time).
Already have it fixed locally.

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkActionBar.java
(right):

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkActionBar.java:58:
getMenu()
On 2016/04/18 at 18:33:13, Ian Wen wrote:
> Reformoat #58 - #69? At lease getMenu() and findItem() should be on the same
line.

Done.

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkEditActivity.java
(right):

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkEditActivity.java:58:
setTitle(R.string.edit_bookmark);
On 2016/04/18 at 18:33:13, Ian Wen wrote:
> Since we no longer need to modify the activity name programmatically, I would
remove #58 and #59, and in androidManifest.xml, add
android:label="@string/string.edit_bookmark".

Done.

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java
(right):

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:156:
public void addBookmarkAsync(BookmarkId parent, int index, String title, String
url,
On 2016/04/18 at 18:33:14, Ian Wen wrote:
> This method is no longer async. I would rename it to
addBookmarkWithOfflinePage(). 
> 
> Also why do we still need AddBookmarkCallback?

If you want me to change it to a synchronous version I can do that, but I
wouldn't mention the offline page in the method name as this is something that
may or may not happen and is really a side channel action for user convenience.

done.

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkModel.java:185:
OfflinePageBridge offlinePageBridge =
OfflinePageBridge.getForProfile(getProfile());
On 2016/04/18 at 18:33:13, Ian Wen wrote:
> You might want to use Profile#getLastUsedProfile().getOriginal()?
> 
> On Android there are only two profiles. A normal profile and its incognito
profile. Bookmarks are always stored in the normal profile.

I am hoping getProfile() will actually give me incognito here. Is that the case?

We deal with the problem you mentioned on the factory side. There is a chance we
will not want to save things from incognito profile, in which case I want to
know where the request came from. Does that make sense?

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUIState.java
(right):

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUIState.java:66:
state.mShouldPersist = shouldPersist(uri);
On 2016/04/18 at 18:33:14, Ian Wen wrote:
> mShouldPersist should be removed from this class, together with all the
processing related to it .

Good point. Done.

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java
(right):

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:73:
bookmarkModel.addBookmarkAsync(parent, bookmarkModel.getChildCount(parent),
tab.getTitle(),
On 2016/04/18 at 18:33:14, Ian Wen wrote:
> Since now bookmark and offline pages are decoupled, I suppose the cleanest
separation should be:
> 1. Remove addBookmarkAsync() and saveOfflinePage() in bookmark model.
> 2. Add one line in ChromeActivity#addOrEditBookmark() method to save the
offline page.

Done.

https://codereview.chromium.org/1894703002/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkUtils.java:225:
* Opens a bookmark depending on connection status and reports UMA.
On 2016/04/18 18:33:14, Ian Wen wrote:
> Update javadoc.

Done.

[Ian Wen, 2016-04-19 00:11:10]

Awesome change! The separation looks very clean to me. lgtm with some nits.

https://chromiumcodereview.appspot.com/1894703002/diff/60001/chrome/android/j...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://chromiumcodereview.appspot.com/1894703002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1062:
BookmarkId newBookmarkId =
Nit: It seems these 3 lines can be reformatted to be 2 lines instead.

https://chromiumcodereview.appspot.com/1894703002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkActionBar.java
(right):

https://chromiumcodereview.appspot.com/1894703002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkActionBar.java:60:
getMenu()
Nit: #60-62 and #63-65 can also be 2 lines instead of 3.

https://chromiumcodereview.appspot.com/1894703002/diff/60001/chrome/android/j...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageToolbar.java
(right):

https://chromiumcodereview.appspot.com/1894703002/diff/60001/chrome/android/j...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageToolbar.java:51:
((TextView) mBookmarksButton.getChildAt(0))
Nit: Let's keep it 2 lines instead of 3?

We might be using a different code formatter in Eclipse. Try using
https://raw.githubusercontent.com/android/platform_development/master/ide/ecl...
in eclipse instead for Clank?

[jianli, 2016-04-19 00:27:01]

lgtm

[fgorski, 2016-04-19 04:35:10]

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

[fgorski, 2016-04-19 04:35:11]

Ted, PTAL.

Ian approved bookmarks pull out, but there are some changes in Clank that I'd
like you to take a look at.

Thanks.

https://codereview.chromium.org/1894703002/diff/60001/chrome/android/java/src...
File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java
(right):

https://codereview.chromium.org/1894703002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1062:
BookmarkId newBookmarkId =
On 2016/04/19 00:11:09, Ian Wen wrote:
> Nit: It seems these 3 lines can be reformatted to be 2 lines instead.

I think it is one character too long.

https://codereview.chromium.org/1894703002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkActionBar.java
(right):

https://codereview.chromium.org/1894703002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/bookmarks/BookmarkActionBar.java:60:
getMenu()
On 2016/04/19 00:11:09, Ian Wen wrote:
> Nit: #60-62 and #63-65 can also be 2 lines instead of 3.

Done. git cl format changes it to this form though.

https://codereview.chromium.org/1894703002/diff/60001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageToolbar.java
(right):

https://codereview.chromium.org/1894703002/diff/60001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageToolbar.java:51:
((TextView) mBookmarksButton.getChildAt(0))
On 2016/04/19 00:11:09, Ian Wen wrote:
> Nit: Let's keep it 2 lines instead of 3?
> 
> We might be using a different code formatter in Eclipse. Try using
>
https://raw.githubusercontent.com/android/platform_development/master/ide/ecl...
> in eclipse instead for Clank?

Done. git cl format complains again here.

I don't code in eclipse.

[Ted C, 2016-04-19 21:07:26]

lgtm

[fgorski, 2016-04-19 21:13:32]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-04-19 21:13:33]

The patchset sent to the CQ was uploaded after l-g-t-m from ianwen@chromium.org,
jianli@chromium.org
Link to the patchset: https://codereview.chromium.org/1894703002/#ps180001
(title: "Marking more methods as visible for testing")

[commit-bot: I haz the power, 2016-04-19 21:14:00]

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

[commit-bot: I haz the power, 2016-04-19 21:29:30]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-04-22 19:16:37]

Description was changed from

==========
[Offline pages] Removing offline pages from Bookmarks UI

* Removing all Offline page UI from Bookmarks UI
* Removing all offline page code from bookmarks, except page snapshotting
* Removing alternative offline page strings
* Removing bookmarks filtering options

* Relevant UMA will be marked obsolete separately

BUG=603940
R=jianli@chromiu.org,ianwen@chromium.org
==========

to

==========
[Offline pages] Removing offline pages from Bookmarks UI

* Removing all Offline page UI from Bookmarks UI
* Removing all offline page code from bookmarks, except page snapshotting
* Removing alternative offline page strings
* Removing bookmarks filtering options

* Relevant UMA will be marked obsolete separately

BUG=603940
R=jianli@chromiu.org,ianwen@chromium.org

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

[commit-bot: I haz the power, 2016-04-22 19:16:39]

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