[Offline pages] Fixing restoring bookmarks filter, when offline pages disabled

[Offline pages] Fixing restoring bookmarks filter, when offline pages disabled

Ensures that when enhanced bookmarks UI is restoring the past filter
for content saved offline, and offline pages are disabled, it is treating
the state as invalid and reverts to the all bookmarks state.

BUG=581976
Committed: https://crrev.com/d1b363e7940c0b5cbc5eebd333af929b0d60032a
Cr-Commit-Position: refs/heads/master@{#372983}

[fgorski, 2016-02-01 23:44:30]

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

[fgorski, 2016-02-01 23:44:31]

This would be merge candidate. PTAL

[Ian Wen, 2016-02-01 23:51:48]

One minor suggestion in test.

https://codereview.chromium.org/1656103002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkTest.java
(right):

https://codereview.chromium.org/1656103002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkTest.java:248:
EnhancedBookmarkUtils.setLastUsedUrl(getActivity(), UrlConstants.BOOKMARKS_URL);
#248 and #253 are not necessary.

You could either:
1. setLastUsedUrl to offline page url, and then call openBookmarkManager without
parameter.
2. Or you could directly call openBookmarkManager() with offline page url.

The second option seems less flaky.

[fgorski, 2016-02-02 00:00:17]

updated

https://codereview.chromium.org/1656103002/diff/1/chrome/android/javatests/sr...
File
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkTest.java
(right):

https://codereview.chromium.org/1656103002/diff/1/chrome/android/javatests/sr...
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkTest.java:248:
EnhancedBookmarkUtils.setLastUsedUrl(getActivity(), UrlConstants.BOOKMARKS_URL);
On 2016/02/01 23:51:48, Ian Wen wrote:
> #248 and #253 are not necessary.
> 
> You could either:
> 1. setLastUsedUrl to offline page url, and then call openBookmarkManager
without
> parameter.
> 2. Or you could directly call openBookmarkManager() with offline page url.
> 
> The second option seems less flaky.

Done.

[Ian Wen, 2016-02-02 00:03:43]

lgtm with one comment.

Test code needs a separate reviewer.

https://codereview.chromium.org/1656103002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkTest.java
(right):

https://codereview.chromium.org/1656103002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkTest.java:251:
assertEquals(
Remove #251. This test should not depend on whether a pervious url was
persisted.

[fgorski, 2016-02-02 17:14:03]

Fixed the test and rebased.

Looks like you've meanwhile fixed the test ownership.

Thanks.

https://codereview.chromium.org/1656103002/diff/20001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkTest.java
(right):

https://codereview.chromium.org/1656103002/diff/20001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/enhancedbookmarks/EnhancedBookmarkTest.java:251:
assertEquals(
On 2016/02/02 00:03:43, Ian Wen wrote:
> Remove #251. This test should not depend on whether a pervious url was
> persisted.

Done.

[fgorski, 2016-02-02 17:54:13]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-02-02 17:54:13]

The patchset sent to the CQ was uploaded after l-g-t-m from ianwen@chromium.org
Link to the patchset: https://codereview.chromium.org/1656103002/#ps40001
(title: "Rebasing and removing the extra verification from the test")

[commit-bot: I haz the power, 2016-02-02 17:54:38]

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

[commit-bot: I haz the power, 2016-02-02 18:01:35]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-02 18:03:35]

Description was changed from

==========
[Offline pages] Fixing restoring bookmarks filter, when offline pages disabled

Ensures that when enhanced bookmarks UI is restoring the past filter
for content saved offline, and offline pages are disabled, it is treating
the state as invalid and reverts to the all bookmarks state.

BUG=581976
==========

to

==========
[Offline pages] Fixing restoring bookmarks filter, when offline pages disabled

Ensures that when enhanced bookmarks UI is restoring the past filter
for content saved offline, and offline pages are disabled, it is treating
the state as invalid and reverts to the all bookmarks state.

BUG=581976

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

[commit-bot: I haz the power, 2016-02-02 18:03:36]

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