Fix a bug in bookmark filtering, add a test

Fix a bug in bookmark filtering, add a test

BUG=592149
Committed: https://crrev.com/8c8dd590eb834e2cee23b159970803e34d669dd0
Cr-Commit-Position: refs/heads/master@{#379927}

[bburns, 2016-03-08 18:52:07]

bburns@chromium.org changed reviewers:
+ dtrainor@chromium.org, ianwen@chromium.org

[bburns, 2016-03-08 18:52:08]

ianwen: code review
dtrainor: OWNERS for chrome/android/BUILD.gn

[David Trainor- moved to gerrit, 2016-03-08 18:56:48]

chrome/android/BUILD.gn lgtm.

[Ian Wen, 2016-03-08 20:44:27]

looks good in general. One question about the test.

https://codereview.chromium.org/1774783002/diff/20001/chrome/android/junit/sr...
File
chrome/android/junit/src/org/chromium/chrome/browser/bookmarks/BookmarkModelTest.java
(right):

https://codereview.chromium.org/1774783002/diff/20001/chrome/android/junit/sr...
chrome/android/junit/src/org/chromium/chrome/browser/bookmarks/BookmarkModelTest.java:24:
public class BookmarkModelTest {
I don't think this test should be named as BookmarkModelTest, as it is only
about offline pages.

How about moving the tests to offlinepage test ?

[bburns, 2016-03-08 20:48:00]

On 2016/03/08 20:44:27, Ian Wen wrote:
> looks good in general. One question about the test.
> 
>
https://codereview.chromium.org/1774783002/diff/20001/chrome/android/junit/sr...
> File
>
chrome/android/junit/src/org/chromium/chrome/browser/bookmarks/BookmarkModelTest.java
> (right):
> 
>
https://codereview.chromium.org/1774783002/diff/20001/chrome/android/junit/sr...
>
chrome/android/junit/src/org/chromium/chrome/browser/bookmarks/BookmarkModelTest.java:24:
> public class BookmarkModelTest {
> I don't think this test should be named as BookmarkModelTest, as it is only
> about offline pages.
> 
> How about moving the tests to offlinepage test ?

I could if you really want to, but the function is in BookmarkModel.java so it
seems weird to test it in a different file.

Let me know if you feel strongly.

Thanks!
--brendan

[bburns, 2016-03-08 20:57:08]

On 2016/03/08 20:48:00, bburns wrote:
> On 2016/03/08 20:44:27, Ian Wen wrote:
> > looks good in general. One question about the test.
> > 
> >
>
https://codereview.chromium.org/1774783002/diff/20001/chrome/android/junit/sr...
> > File
> >
>
chrome/android/junit/src/org/chromium/chrome/browser/bookmarks/BookmarkModelTest.java
> > (right):
> > 
> >
>
https://codereview.chromium.org/1774783002/diff/20001/chrome/android/junit/sr...
> >
>
chrome/android/junit/src/org/chromium/chrome/browser/bookmarks/BookmarkModelTest.java:24:
> > public class BookmarkModelTest {
> > I don't think this test should be named as BookmarkModelTest, as it is only
> > about offline pages.
> > 
> > How about moving the tests to offlinepage test ?
> 
> I could if you really want to, but the function is in BookmarkModel.java so it
> seems weird to test it in a different file.
> 
> Let me know if you feel strongly.
> 
> Thanks!
> --brendan

Ok, renamed as discussed in offline conversation.

[Ian Wen, 2016-03-08 20:58:22]

lgtm. Thank you!

[bburns, 2016-03-08 21:54:39]

The CQ bit was checked by bburns@chromium.org

[bburns, 2016-03-08 21:54:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
dtrainor@chromium.org
Link to the patchset: https://codereview.chromium.org/1774783002/#ps40001
(title: "Fix a bug in bookmark filtering.  add tests.")

[commit-bot: I haz the power, 2016-03-08 21:55:13]

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

[commit-bot: I haz the power, 2016-03-08 22:04:13]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-08 22:05:30]

Description was changed from

==========
Fix a bug in bookmark filtering, add a test

BUG=592149
==========

to

==========
Fix a bug in bookmark filtering, add a test

BUG=592149

Committed: https://crrev.com/8c8dd590eb834e2cee23b159970803e34d669dd0
Cr-Commit-Position: refs/heads/master@{#379927}
==========

[commit-bot: I haz the power, 2016-03-08 22:05:31]

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