863347 - Remove about:home from preloaded bookmarks

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Ian Barlow (:ibarlow)]

Now that we are going to make about:home so easy to get to (see bug 862813), it doesn't really make sense to keep it as a preloaded bookmark. 

Let's remove it as part of the about:home refresh work.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Patch

Note: Tested over this patch (https://bugzilla.mozilla.org/show_bug.cgi?id=888578#c28) as my build was otherwise broken. I assume this shouldn't affect the bookmarks setting.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Patch

Fixed patch - initially used ">>" instead of ">" to redirect the output. x_x

Comment 4

[Richard Newman [:rnewman]]

Note for me: Sync interplay.

Comment 5

[Wesley Johnston (:wesj)]

Make sure you update any tests that depend on this bookmark existing.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Patch w/ test

Didn't move r+ just to make sure I didn't forget anything - added test.

https://tbpl.mozilla.org/?tree=Try&rev=e39468bd765e

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 770924 [details] [diff] [review]
Patch w/ test

Review of attachment 770924 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, as soon as I saw this I remembered we have more tests that would be broken here too. Namely:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testBookmark.java.in#75
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testBookmark.java.in#90
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testBookmarksTab.java.in#46

Since these tests are alrady broken on Fig, its going to be really hard to do any meaningful test to make sure we don't break them even further. Also, we may end up just throwing them out to rewrite anyway. Can you update the numbers here, just because we know about them, and I'll try not to think of anything else that we're breaking :)

Comment 8

[Richard Newman [:rnewman]]

Don't you also need to touch:

http://mxr.mozilla.org/mozilla-central/source/mobile/locales/generic/profile/bookmarks.json.in
-- remove the bookmark itself

http://mxr.mozilla.org/mozilla-central/source/mobile/locales/en-US/profile/bookmarks.inc
-- remove the now-redundant string

?

Comment 9

[Richard Newman [:rnewman]]

See mxr search:

http://mxr.mozilla.org/mozilla-central/search?string=bookmarks_aboutHome

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 01c: Patch

Fixed tests as per comment 7 and removed strings as per comment 8. Also fixed one additional test that broke in my previous try build (in base/tests/testAllPagesTab.java.in).

https://tbpl.mozilla.org/?tree=Try&rev=b7e43e8dabbe

Also, I'm not too familiar with Fig/project branches and so have not been testing on it at all (i.e. my try push is from m-c). If I should be testing on Fig (which I assume I should), please let me know.

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 771076 [details] [diff] [review]
01c: Patch

Found bugs from my TBPL push; will update with a fixed patch.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 01d: Patch

https://tbpl.mozilla.org/?tree=Try&rev=b8a078d59a89

I fixed the issues which were related to another update of bookmark counts.

There should probably be a global constant specifying the number of default bookmarks so it only needs to be changed in a single location, but I wasn't sure where to put it. Do you feel either putting it in BaseTest or a new TestConstants.java file is appropriate? I prefer the latter.

---

Additionally, I noticed in testAllPagesTab.java.in that the following line is called multiple times (https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testAllPagesTab.java.in#51):

>ListView list = getAllPagesList(mBookmarks[0], some_int);

getAllPagesList is tested in TestList (https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/tests/testAllPagesTab.java.in#51), but later methods call this (e.g. testContextMenu & testClick) assuming it works (else it returns null and causes a NullPointerException).

This seems incorrect to me since if this method fails, those other tests will also fail which is against the philosophy of unit testing.

Ideally, TestList, in which getAllPagesList is tested, should be called first followed by the others who short-circuit. However, I could not find a way to specify testing order nor do I believe testing short-circuits. Alternatively, and perhaps more correctly, a mock all pages list could be constructed and used.

I chose to set some_int in getAllPagesList(mBookmarks[0], some_int) to -1 as the number of items are irrelevant when getting the list in these methods and specifying a number will cause the method to only return a list if the number matches the number of expected items which is more likely to return null and thus is more fragile.

Do you have any advice?

Comment 13

[Wesley Johnston (:wesj)]

Comment on attachment 772483 [details] [diff] [review]
01d: Patch

Review of attachment 772483 [details] [diff] [review]:
-----------------------------------------------------------------

I'd be fine putting a constant list of bookmarks and counts in BaseTest if we want, since we use them in multiple places.

::: mobile/android/base/tests/testAllPagesTab.java.in
@@ +61,5 @@
>          // This test fails, only when we're running tests
>          // mAsserter.is(host.getCurrentTab(), 0, "All pages tab is selected in tab strip");
>  
> +        mAsserter.isnot(list, null, "checking that all pages list exists and has " +
> +                defaultBookmarkCount + " children (the default bookmarks)");

We don't really have rules about line length in java.

@@ +108,5 @@
>          mActions.sendSpecialKey(Actions.SpecialKey.BACK);
>      }
>  
>      private void testContextMenu(String url) {
> +        ListView list = getAllPagesList(mBookmarks[0], -1);

I think the point of this number was to make sure the list had actually filled in i.e. to avoid races where we try to click on a view that hasn't been created yet (although, in reality it just checks that the cursor is filled in). I would err on the side of leaving it in.

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 01e: Patch

(In reply to Wesley Johnston (:wesj) from comment #13) 
> I'd be fine putting a constant list of bookmarks and counts in BaseTest if
> we want, since we use them in multiple places.

Done.

> We don't really have rules about line length in java.

Do you mind if I leave the code this way anyway? It's consistently formatted in a readable fashion (though by my opinion, of course) if I format it this way, as opposed to however my editor, Bugzilla., etc. decides to wrap it, which I generally find to be less readable.

> I think the point of this number was to make sure the list had actually
> filled in i.e. to avoid races where we try to click on a view that hasn't
> been created yet (although, in reality it just checks that the cursor is
> filled in). I would err on the side of leaving it in.

Re-added, though by use of the constants added above.

I also took the liberty of swapping hard-coded "about:firefox" with a generic "DEFAULT_BOOKMARKS_URL[0]" when called from getBookmarksList(...) and similar methods since they are just waiting for this text, no matter what it is, to appear. While it's unlikely we remove "about:firefox", this is less fragile.

As we spoke about in person, I still need to try this on actual fig. x_x Hopefully it'll apply cleanly, however, I think Margaret was cleaning up the tests so perhaps it will not.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

If you want to wait until it's on fig to review, that's fine with me.

Also, not as useful but: https://tbpl.mozilla.org/?tree=Try&rev=34ee529b6cf6

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

try is green. Margaret said to go ahead and push when it's ready so please review when you get the chance.

Comment 17

[Wesley Johnston (:wesj)]

Comment on attachment 774181 [details] [diff] [review]
01e: Patch

Review of attachment 774181 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. A little comment about variable names, but not that worried about it in tests.

::: mobile/android/base/tests/BaseTest.java.in
@@ +44,5 @@
>      private static final int MAX_LIST_ATTEMPTS = 3;
>      private static final int MAX_WAIT_ENABLED_TEXT_MS = 10000;
>      public static final int MAX_WAIT_MS = 3000;
>  
> +    // Note: DEFAULT_BOOKMARKS_TITLE.length == DEFAULT_BOOKMARKS_URL.length

I would typically name these in plural form i.e. DEFAULT_BOOKMARKS_TITLES

Comment 18

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 1f: Patch

(In reply to Wesley Johnston (:wesj) from comment #17)
> LGTM. A little comment about variable names, but not that worried about it
> in tests.
>
> I would typically name these in plural form i.e. DEFAULT_BOOKMARKS_TITLES

But I'm very picky so I did it anyway. :P Carried r+.

try: https://tbpl.mozilla.org/?tree=Try&rev=3a0ee591e431

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Updated try (the previous results were lost):

https://tbpl.mozilla.org/?tree=Try&rev=06d519ddaa46

Comment 20

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #19)
> Updated try (the previous results were lost):
> 
> https://tbpl.mozilla.org/?tree=Try&rev=06d519ddaa46

I realized these results are based on fig, where the tests didn't run before this patch (and still don't) so this try build is useless.

To be clear, this patch was tested by hand on fig and the tests run successfully on m-c.

Comment 21

[:Margaret Leibovic]

File that bug for level 3 commit access!

https://hg.mozilla.org/projects/fig/rev/3c46c005d676

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3c46c005d676