1117968 - Show multiple places results in list format

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Search, defect)

Comment 1

[Hermes Cheng (inactive after July 27, 2015)]

Does "multiple places results" mean there will be 3 parts of search results which are "app recommendation", "history results", and "search suggestions"?

Comment 2

[Ben Francis [:benfrancis]]

(In reply to Hermes Cheng[:hermescheng] from comment #1)
> Does "multiple places results" mean there will be 3 parts of search results
> which are "app recommendation", "history results", and "search suggestions"?

No, it means there will be a list of results from the places database (browsing history).

Please see the latest search app spec https://mozilla.app.box.com/s/fhlmfa4xyooqpwaaaczc

Comment 3

[Ben Francis [:benfrancis]]

Attachment: WIP Patch

As I'm not very familiar with this area of code, looking for some early feedback about the general direction of this patch.

Comment 4

[Dale Harvey (:daleharvey)]

Comment on attachment 8548261 [details] [review]
WIP Patch

This is a good start, we used to show the results in this format so reverting back to the old code is good, we share this between the start page as well and they should be matched rendering wise.

As we discussed on IRC we should go back to using `getIcon`, and preferably have everyone use that, we can move it into iconshelper but not as part of this patch since there are other implications.

Comment 5

[Ben Francis [:benfrancis]]

(In reply to Dale Harvey (:daleharvey) from comment #4)
> As we discussed on IRC we should go back to using `getIcon`

Thanks Dale. Do you mean like this?

https://github.com/benfrancis/gaia/commit/7b0400e7fb907afa462203f16d14ed8a2ff880db

Comment 6

[Dale Harvey (:daleharvey)]

Yup exactly, that looks good

Comment 7

[Ben Francis [:benfrancis]]

I've implemented the new visual design. I think I just need to figure out how to not show the HISTORY heading when there are no history results, then write tests.

Comment 8

[Ben Francis [:benfrancis]]

Update: I fixed the heading issue and unit tests are now passing (though we could probably do with some more). Integration tests are a mess (they all currently fail for me locally), so I'm going to have to tackle that next week.

Comment 9

[Ben Francis [:benfrancis]]

Attachment: Patch

It would be great to get this reviewed, although I think we probably need more tests.

I'd be grateful for some guidance on testing because I'm struggling to figure out how to test some of the existing code. There are several existing disabled integration tests in this area and I can't find any unit tests for places.js

Any idea why the integration tests were disabled? What tests should we add for this patch?

I think I've got the existing unit tests to pass but there may be a couple of broken integration tests, I'd like to see what Tree Herder says.

Comment 10

[Dale Harvey (:daleharvey)]

Comment on attachment 8551456 [details] [review]
Patch

Which integration test in particular? this is mostly a layout change so I dont think we need a large amount of new testing however this broke a lot of the existing integration tests (which is a good thing, ensures they are actually testing things), the failures look related, I restarted just in case but will want to take a look.

I would be happy with this commit given a green run and to ensure https://github.com/mozilla-b2g/gaia/blob/master/apps/search/test/marionette/places_search_test.js#L31 is running and passing.

Gonna clear the review till the integration tests are green, but once they are reflag, I am happy with the code as it is, looks great

Comment 11

[Ben Francis [:benfrancis]]

It seems like most of the integration test failures were caused by a bad build where you couldn't re-focus the Rocketbar after it had lost focus, ever.

I've rebased and modified the places search test for the new DOM structure, let's see what Treeherder makes of that.

Comment 12

[Ben Francis [:benfrancis]]

Comment on attachment 8551456 [details] [review]
Patch

TreeHerder looks green, there was an integration test failure but I re-ran three times and it seems pretty stable. What say you Mr Harvey?

Comment 13

[Dale Harvey (:daleharvey)]

Comment on attachment 8551456 [details] [review]
Patch

Great this is looking / working great. Going to merge as some of the styling stuff conflicts / is the same as the changes I have made so I can rebase on this now.

Thanks

Comment 14

[Dale Harvey (:daleharvey)]

Merged in https://github.com/mozilla-b2g/gaia/commit/3b45754df848336e0db645eca153754fb5d05ec0, thanks

Comment 15

[Hermes Cheng (inactive after July 27, 2015)]

is this feature going to land to v2.2?

Comment 16

[Ben Francis [:benfrancis]]

Comment on attachment 8551456 [details] [review]
Patch

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): n/a, this is a committed feature
[User impact] if declined: No new shiny search UI
[Testing completed]: Covered by integration tests
[Risk to taking this patch] (and alternatives if risky):
[String changes made]: none.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/7441e5dfa06a365033822eeddef67cedebe3025a

Comment 18

[Brogan Zumwalt [Inactive]]

Issue still occurs on Flame 3.0

All applicable places results from browsing history are shown in a vertical list displaying the site icon, title, and url. But if user presses the X button to clear the search term, the History and Default Search headers remain, showing no results. Filed new bug 1131408

Device: Flame 3.0 Master
Build ID: 20150209010211
Gaia: 0d7b35f23402c4cb29bca6b98280fec48a196dec
Gecko: 3436787a82d0
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 38.0a1 (3.0)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0

Comment 19

[Hermes Cheng (inactive after July 27, 2015)]

I believe we can close this bug now. Verified with below builds.

* 2.2
Build ID               20150329162502
Gaia Revision          473cd63f53c855299b719285d9b95e3f2910782f
Gaia Date              2015-03-27 20:14:43
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/4b13c4254e2f
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150329.200030
Firmware Date          Sun Mar 29 20:00:41 EDT 2015
Bootloader             L1TC000118D0

* master
Build ID               20150329160203
Gaia Revision          67ad91f3f660b1f16b354ee4c5159ddc5a74d149
Gaia Date              2015-03-28 10:02:40
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/385840329d91
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150329.191719
Firmware Date          Sun Mar 29 19:17:28 EDT 2015
Bootloader             L1TC100118D0