811445 - Change Add Search Engine to use new schema

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

We removed the images table, so Add Search Engine should instead pull the favicon from history_with_favicons.

Comment 1

[Brian Nicholson (:bnicholson)]

Attachment: Part 1: Use history_with_favicons view for Add Search Engine

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Test case for Add Search Engine

Test for Add Search Engine.

Comment 3

[Brian Nicholson (:bnicholson)]

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

Comment 4

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Test case for Add Search Engine, v2

Some minor cleanup.

Comment 5

[Geoff Brown [:gbrown]]

(In reply to Brian Nicholson (:bnicholson) from comment #3)
> Try: https://tbpl.mozilla.org/?tree=Try&rev=ae496e682234

It looks like the new test failed that try run.

Comment 6

[Geoff Brown [:gbrown]]

Comment on attachment 681383 [details] [diff] [review]
Part 2: Test case for Add Search Engine, v2

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

Good stuff here - thanks. Clearing the review for now, since the try run failed; I'll have a closer look when it's passing reliably.

::: mobile/android/base/tests/testAddSearchEngine.java.in
@@ +36,5 @@
> +        boolean contextMenuAppeared = waitForTest(new BooleanTest() {
> +            public boolean test() {
> +                MotionEventHelper meh = new MotionEventHelper(getInstrumentation(), mDriver.getGeckoLeft(), mDriver.getGeckoTop());
> +                long down = meh.down(0, 0);
> +                mSolo.sleep(LONG_TAP_DURATION);

Could you use mSolo.clickLongOnScreen or mSolo.clickLongOnView instead?

@@ +40,5 @@
> +                mSolo.sleep(LONG_TAP_DURATION);
> +                meh.up(down, 0, 0);
> +                return mSolo.waitForText(CONTEXT_MENU_LABEL, 0, CONTEXT_MENU_TIMEOUT);
> +            }
> +        }, TEST_TIMEOUT);

TEST_TIMEOUT=20 seconds seems wrong. If it takes 20 seconds to bring up a context menu, that's a visible problem and we don't want the test to pass.

Comment 7

[Brian Nicholson (:bnicholson)]

I'm still having trouble getting this test to pass. Looking at https://tbpl.mozilla.org/?tree=Try&rev=c05b3c216dec, it looks like the search service isn't getting initialized for some reason since I don't see ""BRN: (js) search service init'd" in the logs.

I'll keep looking at this, but I'll land the first patch here in the meantime so Add Search Engine isn't broken.

http://hg.mozilla.org/integration/mozilla-inbound/rev/883c59cef464

Comment 8

[Brian Nicholson (:bnicholson)]

Looking at https://tbpl.mozilla.org/?tree=Try&rev=33b8c547bc02, I don't see "BRN: (js) done creating favicon statement" in the logs, so it looks like we're actually failing to create the database statement.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/883c59cef464

Comment 10

[Brian Nicholson (:bnicholson)]

Attachment: Part 2: Test case for Add Search Engine, v3

The test was failing because of bug 768532, where browser.db is stored outside of the profile. Here's the try push with the patch in bug 768532 applied:

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

Comment 11

[Brian Nicholson (:bnicholson)]

(In reply to Geoff Brown [:gbrown] from comment #6)
> Comment on attachment 681383 [details] [diff] [review]
> Part 2: Test case for Add Search Engine, v2
> 
> Review of attachment 681383 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Good stuff here - thanks. Clearing the review for now, since the try run
> failed; I'll have a closer look when it's passing reliably.
> 
> ::: mobile/android/base/tests/testAddSearchEngine.java.in
> @@ +36,5 @@
> > +        boolean contextMenuAppeared = waitForTest(new BooleanTest() {
> > +            public boolean test() {
> > +                MotionEventHelper meh = new MotionEventHelper(getInstrumentation(), mDriver.getGeckoLeft(), mDriver.getGeckoTop());
> > +                long down = meh.down(0, 0);
> > +                mSolo.sleep(LONG_TAP_DURATION);
> 
> Could you use mSolo.clickLongOnScreen or mSolo.clickLongOnView instead?

I don't think so - these don't allow specifying coordinates on the screen/view to be clicked. We need to long press on the <input> element on the page, which should be anchored at (0,0) because of the set 0 margin. clickLongOnView() on the Gecko SurfaceView *may* work if the press happens to take place at (0,0), but I didn't want to rely on that since it isn't specified in the API.

Rather than have this down/sleep/up code here, I created a longPress() method in MotionEventHelper to make the test code a bit more concise.

> @@ +40,5 @@
> > +                mSolo.sleep(LONG_TAP_DURATION);
> > +                meh.up(down, 0, 0);
> > +                return mSolo.waitForText(CONTEXT_MENU_LABEL, 0, CONTEXT_MENU_TIMEOUT);
> > +            }
> > +        }, TEST_TIMEOUT);
> 
> TEST_TIMEOUT=20 seconds seems wrong. If it takes 20 seconds to bring up a
> context menu, that's a visible problem and we don't want the test to pass.

I noticed that for whatever reason, the long press at (0,0) doesn't always work on the first try (maybe some kind of race with setting the viewport or touch event handlers?). I wrapped that waitForText() call (which has a 5s timeout) inside of the waitForTest() call (with the 20s timeout) as a way of retrying if it didn't succeed, but I agree that it wasn't a great way to do it. I changed it to a for loop that executes up to 2 times; still not pretty, but I can't think of a better option.

Comment 12

[Geoff Brown [:gbrown]]

Comment on attachment 686431 [details] [diff] [review]
Part 2: Test case for Add Search Engine, v3

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

This looks fine. Some of the timeouts are a bit arbitrary and awkward, but that's probably the best we can do.

Comment 13

[Brian Nicholson (:bnicholson)]

Comment on attachment 681262 [details] [diff] [review]
Part 1: Use history_with_favicons view for Add Search Engine

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 784086 (Fx19)
User impact if declined: "Add Search Engine" context menu item doesn't work
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 14

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5971115a94d2

Comment 15

[Brian Nicholson (:bnicholson)]

Comment on attachment 681262 [details] [diff] [review]
Part 1: Use history_with_favicons view for Add Search Engine

Removing request; looks like this actually made Fx19.

Comment 16

[Brian Nicholson (:bnicholson)]

Marking as fixed since the fix itself landed in Fx19; part 2 is just the robocop test.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5971115a94d2

Comment 18

[Ed Morley [:emorley]]

(In reply to Brian Nicholson (:bnicholson) from comment #16)
> Marking as fixed since the fix itself landed in Fx19; part 2 is just the
> robocop test.

(In reply to Ryan VanderMeulen from comment #17)
> https://hg.mozilla.org/mozilla-central/rev/5971115a94d2

Test backed out for causing bug 817336.

Backout (test only):
https://hg.mozilla.org/integration/mozilla-inbound/rev/082810a8a1b5

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/082810a8a1b5

Comment 20

[Brian Nicholson (:bnicholson)]

Closing this bug since it was fix long ago. Filed bug 871792 for the test.