All users were logged out of Bugzilla on October 13th, 2018

Add icons to places rocketbar results

RESOLVED FIXED in 1.4 S1 (14feb)

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: daleharvey, Assigned: daleharvey)

Tracking

unspecified
1.4 S1 (14feb)
x86
Mac OS X

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [systemsfe], [p=8])

Attachments

(1 attachment)

Comment hidden (empty)
(Assignee)

Updated

5 years ago
Assignee: nobody → dale
(Assignee)

Comment 1

5 years ago
Created attachment 8362929 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15560

There was one change that got intertwined in this commit and its a bit annoying to unwind, so I left it in, Datastore does have change events, so we now use them instead of manually triggering changes via the rocketbar interapp api, this is far more reliable

Apart from that its a fairly simple change, currently as with the rest of the history we just maintain a list of currently active urls + icons, these will have to be persisted however doing it this way gives us freedom to experiment with the implementation without breaking things going forward.

There is a test added along with this, it currently intermittently fails, very likely due to the same issue that has the other tests intermittently fail so I have added it as a skip and currently working on enabling them (in https://bugzilla.mozilla.org/show_bug.cgi?id=961137)
Attachment #8362929 - Flags: review?(kgrandon)
(Assignee)

Comment 2

5 years ago
It looks like this is a clean marge against the rocketbar branch and is working
(Assignee)

Comment 3

5 years ago
s/marge/merge
Comment on attachment 8362929 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15560

Dale - this looks good. I think we mainly just need to move the location of the test file.

I was also having problems actually getting icons to load in my search app. I'm seeing a few icons, but there's a bunch of sites that probably should have icons that don't (cnn.com) for example.
Attachment #8362929 - Flags: review?(kgrandon) → review+
(Assignee)

Comment 5

5 years ago
I wasnt quite sure what you were aiming for with the generic class names so I landed as is, it seems the places results arent gonna be particularly reusable styles, but as said can fix as a follow up.

I think I have seen issues with favicons not being set as well, the test added was a lot more flakey than the others, but working to reenable the tests now
(Assignee)

Comment 6

5 years ago
https://github.com/mozilla-b2g/gaia/commit/a1e985d3a045daa796812712dfb81486becc5a9b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Blocks: 964476
Whiteboard: [systemsfe]
Target Milestone: --- → 1.4 S1 (14feb)
(Assignee)

Updated

5 years ago
Whiteboard: [systemsfe] → [systemsfe], [p=8]
You need to log in before you can comment on or make changes to this bug.