Update flags used by offline pages and bookmarks

Update flags used by offline pages and bookmarks

Offline pages have been removed from bookmarks UI. We're updating flags used by various offline page features. For capability to save offline copy automatically for bookmarked pages, "OfflineBookmarks" is introduced. The old flag "OfflinePages" is removed. We keep IsOfflinePagesEnabled() as a way to tell if any of offline features is enabled.

BUG=603940
Committed: https://crrev.com/5e9585c1f10b97051840ffc0532922089b39877c
Cr-Commit-Position: refs/heads/master@{#388920}

[jianli, 2016-04-16 00:40:41]

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

[fgorski, 2016-04-18 17:09:37]

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

https://codereview.chromium.org/1899433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:187:
public static int getFeatureMode() {
This method is gone from my patch par your request.

https://codereview.chromium.org/1899433002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
(right):

https://codereview.chromium.org/1899433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:77:
public static int getStringId(int stringResId) {
This method is gone after my patch.

Also, you can probably have a one-liner here:

return stringResId;

https://codereview.chromium.org/1899433002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1899433002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:14435: +        Enable offline capability for
bookmarks.
Enable saving bookmarked pages for offline viewing.

https://codereview.chromium.org/1899433002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_utils.cc (right):

https://codereview.chromium.org/1899433002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_utils.cc:27: if
(!offline_pages::IsOfflineBookmarksEnabled())
This is a bit tricky. I don't think we should be checking for bookmarks being
enabled here, as these checks will be growing with lastN and others offline
things added.

How about we still have:
bool offline_pages::IsOfflinePagesEnbaled() {
  return IsOfflineBookmarksEnabled()
      || IsOfflineOtherFeatureEnabled() 
      || IsOfflineYetAnotherFeatureEnabled();
}

// Names of features are of course made up, but you know what I mean. Either one
of these features should make this code work regardless of the any other parts.

And this code will not have to change

https://codereview.chromium.org/1899433002/diff/1/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_model_factory.cc (right):

https://codereview.chromium.org/1899433002/diff/1/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_model_factory.cc:62: if
(offline_pages::IsOfflineBookmarksEnabled())
Same idea as above. (And applies below)

https://codereview.chromium.org/1899433002/diff/1/components/error_page/commo...
File components/error_page/common/localized_error.cc (right):

https://codereview.chromium.org/1899433002/diff/1/components/error_page/commo...
components/error_page/common/localized_error.cc:1066:
IDS_ERRORPAGES_BUTTON_SHOW_OFFLINE_PAGES_AS_BOOKMARKS);
I am not sure if we still want to point the users to bookmarks as there is no
saved offline section there. Could you check with Tal, if documentation is not
clear.

Or do you plan to revisit this area?

https://codereview.chromium.org/1899433002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_feature.cc (left):

https://codereview.chromium.org/1899433002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_feature.cc:79: bool
IsOfflinePagesEnabled() {
As mentioned above, keep this and make it an aggregation of individual checks.

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

https://codereview.chromium.org/1899433002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_feature.cc:31:
IsOfflineBookmarksEnabled();
No. These should be independent.

https://codereview.chromium.org/1899433002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_feature.cc:36: &&
IsOfflineBookmarksEnabled();
ditto.

[jianli, 2016-04-18 21:17:01]

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

https://codereview.chromium.org/1899433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageBridge.java:187:
public static int getFeatureMode() {
On 2016/04/18 17:09:36, fgorski wrote:
> This method is gone from my patch par your request.

Acknowledged.

https://codereview.chromium.org/1899433002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
(right):

https://codereview.chromium.org/1899433002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:77:
public static int getStringId(int stringResId) {
On 2016/04/18 17:09:36, fgorski wrote:
> This method is gone after my patch.
> 
> Also, you can probably have a one-liner here:
> 
> return stringResId;

I think we're going to remove this after I sync with your patch.

https://codereview.chromium.org/1899433002/diff/1/chrome/app/generated_resour...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/1899433002/diff/1/chrome/app/generated_resour...
chrome/app/generated_resources.grd:14435: +        Enable offline capability for
bookmarks.
On 2016/04/18 17:09:36, fgorski wrote:
> Enable saving bookmarked pages for offline viewing.

Done.

https://codereview.chromium.org/1899433002/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/offline_page_utils.cc (right):

https://codereview.chromium.org/1899433002/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/offline_page_utils.cc:27: if
(!offline_pages::IsOfflineBookmarksEnabled())
On 2016/04/18 17:09:36, fgorski wrote:
> This is a bit tricky. I don't think we should be checking for bookmarks being
> enabled here, as these checks will be growing with lastN and others offline
> things added.
> 
> How about we still have:
> bool offline_pages::IsOfflinePagesEnbaled() {
>   return IsOfflineBookmarksEnabled()
>       || IsOfflineOtherFeatureEnabled() 
>       || IsOfflineYetAnotherFeatureEnabled();
> }
> 
> // Names of features are of course made up, but you know what I mean. Either
one
> of these features should make this code work regardless of the any other
parts.
> 
> And this code will not have to change

Done.

https://codereview.chromium.org/1899433002/diff/1/chrome/browser/bookmarks/bo...
File chrome/browser/bookmarks/bookmark_model_factory.cc (right):

https://codereview.chromium.org/1899433002/diff/1/chrome/browser/bookmarks/bo...
chrome/browser/bookmarks/bookmark_model_factory.cc:62: if
(offline_pages::IsOfflineBookmarksEnabled())
On 2016/04/18 17:09:37, fgorski wrote:
> Same idea as above. (And applies below)

Done.

https://codereview.chromium.org/1899433002/diff/1/components/error_page/commo...
File components/error_page/common/localized_error.cc (right):

https://codereview.chromium.org/1899433002/diff/1/components/error_page/commo...
components/error_page/common/localized_error.cc:1066:
IDS_ERRORPAGES_BUTTON_SHOW_OFFLINE_PAGES_AS_BOOKMARKS);
On 2016/04/18 17:09:37, fgorski wrote:
> I am not sure if we still want to point the users to bookmarks as there is no
> saved offline section there. Could you check with Tal, if documentation is not
> clear.
> 
> Or do you plan to revisit this area?

I will do it in a separate patch to hide/remove offline button in error pages.

https://codereview.chromium.org/1899433002/diff/1/components/offline_pages/of...
File components/offline_pages/offline_page_feature.cc (left):

https://codereview.chromium.org/1899433002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_feature.cc:79: bool
IsOfflinePagesEnabled() {
On 2016/04/18 17:09:37, fgorski wrote:
> As mentioned above, keep this and make it an aggregation of individual checks.

Done.

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

https://codereview.chromium.org/1899433002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_feature.cc:31:
IsOfflineBookmarksEnabled();
On 2016/04/18 17:09:37, fgorski wrote:
> No. These should be independent.

Done.

https://codereview.chromium.org/1899433002/diff/1/components/offline_pages/of...
components/offline_pages/offline_page_feature.cc:36: &&
IsOfflineBookmarksEnabled();
On 2016/04/18 17:09:37, fgorski wrote:
> ditto.

Done.

[fgorski, 2016-04-18 22:40:02]

lgtm with 2 missed changes.

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

https://codereview.chromium.org/1899433002/diff/20001/components/offline_page...
components/offline_pages/offline_page_feature.cc:36:
IsOfflineBookmarksEnabled();
remove. the features should be independent.

https://codereview.chromium.org/1899433002/diff/20001/components/offline_page...
components/offline_pages/offline_page_feature.cc:41: &&
IsOfflineBookmarksEnabled();
ditto.

[jianli, 2016-04-18 23:09:05]

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

https://codereview.chromium.org/1899433002/diff/20001/components/offline_page...
components/offline_pages/offline_page_feature.cc:36:
IsOfflineBookmarksEnabled();
On 2016/04/18 22:40:02, fgorski wrote:
> remove. the features should be independent.

Done.

https://codereview.chromium.org/1899433002/diff/20001/components/offline_page...
components/offline_pages/offline_page_feature.cc:41: &&
IsOfflineBookmarksEnabled();
On 2016/04/18 22:40:02, fgorski wrote:
> ditto.

Done.

[jianli, 2016-04-20 00:43:29]

Patchset #4 (id:60001) has been deleted

[jianli, 2016-04-20 00:50:37]

Description was changed from

==========
Update flags used by offline pages and bookmarks

BUG=603940
==========

to

==========
Update flags used by offline pages and bookmarks

Offline pages have been removed from bookmarks UI. We're updating flags used by
various offline page features. For capability to save offline copy automatically
for bookmarked pages, "OfflineBookmarks" is introduced. The old flag
"OfflinePages" is removed. We keep IsOfflinePagesEnabled() as a way to tell if
any of offline features is enabled.

BUG=603940
==========

[jianli, 2016-04-20 00:52:42]

jianli@chromium.org changed reviewers:
+ mmenke@chromium.org, tedchoc@chromium.org

[jianli, 2016-04-20 00:52:43]

tedchoc, for clank changes.

mmenke, for error page changes. For now, we only remove the offline button. The
existing infrastructure is still kept since we very likely need to expose a
similar button to bring to the new portal.

[Ted C, 2016-04-20 16:44:11]

On 2016/04/20 00:52:43, jianli wrote:
> tedchoc, for clank changes.
> 
> mmenke, for error page changes. For now, we only remove the offline button.
The
> existing infrastructure is still kept since we very likely need to expose a
> similar button to bring to the new portal.

android - lgtm

[mmenke, 2016-04-21 16:37:40]

On 2016/04/20 16:44:11, Ted C wrote:
> On 2016/04/20 00:52:43, jianli wrote:
> > tedchoc, for clank changes.
> > 
> > mmenke, for error page changes. For now, we only remove the offline button.
> The
> > existing infrastructure is still kept since we very likely need to expose a
> > similar button to bring to the new portal.
> 
> android - lgtm

components/error_page LGTM

[mmenke, 2016-04-21 16:39:03]

[+edwardjung]: FYI

[jianli, 2016-04-21 21:57:52]

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

[jianli, 2016-04-21 21:57:53]

thestig, could you please approve chrome/browser changes? thanks

[Lei Zhang, 2016-04-21 22:01:34]

On 2016/04/21 21:57:53, jianli wrote:
> thestig, could you please approve chrome/browser changes? thanks

non-Android bits lgtm

[jianli, 2016-04-21 22:06:57]

The CQ bit was checked by jianli@chromium.org

[jianli, 2016-04-21 22:06:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
fgorski@chromium.org, tedchoc@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1899433002/#ps100001
(title: "Fix test")

[commit-bot: I haz the power, 2016-04-21 22:07:18]

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

[commit-bot: I haz the power, 2016-04-21 22:13:42]

Description was changed from

==========
Update flags used by offline pages and bookmarks

Offline pages have been removed from bookmarks UI. We're updating flags used by
various offline page features. For capability to save offline copy automatically
for bookmarked pages, "OfflineBookmarks" is introduced. The old flag
"OfflinePages" is removed. We keep IsOfflinePagesEnabled() as a way to tell if
any of offline features is enabled.

BUG=603940
==========

to

==========
Update flags used by offline pages and bookmarks

Offline pages have been removed from bookmarks UI. We're updating flags used by
various offline page features. For capability to save offline copy automatically
for bookmarked pages, "OfflineBookmarks" is introduced. The old flag
"OfflinePages" is removed. We keep IsOfflinePagesEnabled() as a way to tell if
any of offline features is enabled.

BUG=603940
==========

[commit-bot: I haz the power, 2016-04-21 22:13:43]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-04-22 19:40:21]

Description was changed from

==========
Update flags used by offline pages and bookmarks

Offline pages have been removed from bookmarks UI. We're updating flags used by
various offline page features. For capability to save offline copy automatically
for bookmarked pages, "OfflineBookmarks" is introduced. The old flag
"OfflinePages" is removed. We keep IsOfflinePagesEnabled() as a way to tell if
any of offline features is enabled.

BUG=603940
==========

to

==========
Update flags used by offline pages and bookmarks

Offline pages have been removed from bookmarks UI. We're updating flags used by
various offline page features. For capability to save offline copy automatically
for bookmarked pages, "OfflineBookmarks" is introduced. The old flag
"OfflinePages" is removed. We keep IsOfflinePagesEnabled() as a way to tell if
any of offline features is enabled.

BUG=603940

Committed: https://crrev.com/5e9585c1f10b97051840ffc0532922089b39877c
Cr-Commit-Position: refs/heads/master@{#388920}
==========

[commit-bot: I haz the power, 2016-04-22 19:40:23]

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