971268 - [B2G][Rocketbar][E.me] When searching for E.me apps via Rocketbar apps will be opened with the browser wrapper preventing from saving to the homescreen

Status: VERIFIED FIXED

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

Description

[Natalya Kot [:nkot]]

Attachment: logcat

Description:
When searching for E.me apps via Rocketbar applications will be opened with a browser wrapper which will prevent from saving apps to the homescreen. 

Repro Steps:
1) Updated Buri to BuildID: 20140211040200
2) Tap the Rocketbar
3) Enter in search - social
4) Select Facebook app
5) Once loaded, expand the bottom wrapper to add Facebook to the homescreen

Actual:
Unable to add, no such option

Expected:
Able to add Facebook to the homescreen

**Note:
The user is able to add Facebook (and other E.me apps) when launching them via E.me collections

Environmental Variables:
Device: Buri v1.4 master build, Mozilla RIL
BuildID: 20140211040200
Gaia: 9fc36dde3a4a3c5ca200275b68ffb56b4173bec3
Gecko: d812f80a0f1d
Version: 30.0a1
Firmware Version: v1.2-devices.cfg


Repro frequency: 100%
See attached logcat

Comment 1

[Jason Smith [:jsmith]]

Probably a regression from landing the system browser.

Comment 2

[Jason Smith [:jsmith]]

Talked with Kevin in IRC about this - what we're going to do to unblock smoketests & UI automation is disable the rocketbar for pvtbuilds until it's ready for general QA. As such, I'm pulling the flags here & instead, flagging bug 972034 as the smoketest blocker to disable it.

Comment 3

[Natalya Kot [:nkot]]

Hello Kevin, 
since bug 972034 is won't fix, can you please clarify if this bug here is a valid issue or it's expected behavior now with the current Rocketbar UX implementation.

It's still causing confusion to us as ability to add E.me apps to the homescreen is part of our manual E.me smoke test and I don't want to fail it when it's expected to not be working right now.

Thanks.

Comment 4

[Kevin Grandon :kgrandon]

Hi, so we are ready to QA E.me apps, but the flow is slightly changed. There is no longer an option to add apps to the homescreen from E.me results. What is the process from removing this test from the smoke test suite?

Is there any other confusion, or just this one item?

Comment 5

[Natalya Kot [:nkot]]

If you can link me a new stable flow, we'll update the test case and we can help testing it on a daily basis. 
If it's not stable yet, we can temporary disable E.me test case. Let me know please.

Thank you for your quick response.

Comment 6

[Kevin Grandon :kgrandon]

I can send the spec to you if you don't already have that, would that help? It's no longer a valid test case, so we just need to remove it.

Comment 7

[Jason Smith [:jsmith]]

(In reply to Kevin Grandon :kgrandon from comment #6)
> I can send the spec to you if you don't already have that, would that help?
> It's no longer a valid test case, so we just need to remove it.

We should not be disabling that test case until this bug gets resolved.

Comment 8

[Jason Smith [:jsmith]]

Under the rocketbar UX, in order to switch the smoketest to use that instead, we would need the following use case implemented:

1. Open rocketbar
2. Search for twitter
3. Select the e.me twitter app
4. After twitter loads, select to add twitter to the homescreen
5. Confirm to add to homescreen
6. Verify twitter is added to the homescreen
7. Repeat 2 - 6 with facebook

Comment 9

[Kevin Grandon :kgrandon]

Ok, I think I understand now. The new favoriting flow is a little bit different, and unfortunately not available yet. Implementing this should be fairly trivial, so we can likely have something landed in the next couple of days if that is ok? Alternatively we can consider going down the road of disabling rocketbar for now, but I'd rather just knock this bug out if this is the only QA blocker currently.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Kevin Grandon :kgrandon from comment #9)
> Ok, I think I understand now. The new favoriting flow is a little bit
> different, and unfortunately not available yet. Implementing this should be
> fairly trivial, so we can likely have something landed in the next couple of
> days if that is ok? Alternatively we can consider going down the road of
> disabling rocketbar for now, but I'd rather just knock this bug out if this
> is the only QA blocker currently.

That's not okay. We do not disable tests. The resolution path that was done in bug 972163 did not resolve the smoketest issue here & still keeps UI automation blocked, as the smoketests focus on adding an e.me app to the homescreen.

Comment 11

[Kevin Grandon :kgrandon]

Actually it looks like we can restore this by just passing a few parameters that we are missing today (which is easier than disabling the rocketbar). Patch in progress.

Comment 12

[Kevin Grandon :kgrandon]

Attachment: Pull Request - Enable bookmarking from Search App

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 8376753 [details] [review]
Pull Request - Enable bookmarking from Search App

Hey Guys - anyone available for a review? Although the final implementation will be different, this is a quick patch to unblock QA so they can perform their smoketests. This patch essentially allows for bookmarking to the homescreen from the web result provider. It is slightly different from master as we will only give the user the option to bookmark the app URL (not two different URLs). This will be closer to the final rocketbar implementation. Thanks!

Comment 14

[Ran Ben Aharon [:ranbena] EverythingMe]

Comment on attachment 8376753 [details] [review]
Pull Request - Enable bookmarking from Search App

Looks good to me (apart for the square icons being produced on the homescreen).

Comment 15

[Kevin Grandon :kgrandon]

Comment on attachment 8376753 [details] [review]
Pull Request - Enable bookmarking from Search App

I'll fix the homescreen styled as well if it's easy to slipstream in. Thanks guys!

Comment 16

[Kevin Grandon :kgrandon]

Landed: https://github.com/mozilla-b2g/gaia/commit/ac06cfbd2baf6494ffbb668cc599e3892cd5e17b

Will work on a follow-up for image rounding - I think this is homescreen specific code though. Thanks for the review.

Comment 17

[Ran Ben Aharon [:ranbena] EverythingMe]

IMO, the icon rounding should be done in search/js/eme/api.js so no extra rounding will be required later on (AND should ease scrolling perf). Is there any reason not to?

Comment 18

[Kevin Grandon :kgrandon]

(In reply to Ran Ben Aharon (Everything.me) from comment #17)
> IMO, the icon rounding should be done in search/js/eme/api.js so no extra
> rounding will be required later on (AND should ease scrolling perf). Is
> there any reason not to?

I want to run through the spec and check with Alive on the system side. I'm under the impression that we may want to move icon detection/formatting into the system app. Users will need to be able to bookmark any site inside of a browser chrome now - so it's important that we can generate the icon in the system app.

Comment 19

[Jason Smith [:jsmith]]

This is a smoketest blocker. It's completely out of the question that this could be a non-blocker.

Comment 20

[Amir Nissim (:amirn)]

We have a simple css rule for rounding the icons:
https://github.com/EverythingMe/gaia/blob/965711-redesign-collection-icon/apps/search/style/search.css#L120

Perhaps some class renaming broke it?

I agree the system app should do all icon processing (rounding included).
Handling icon appearance in 2 places proved to be difficult (as currently done in E.me and Homescreen code)

Comment 21

[Tony Nguyen]

This issue no longer occurs on the latest Buri Master M-C build. I'm now able to add E.me apps to the homescreen.

Environmental Variables:
Device: Buri Master M-C mozRIL
BuildID: 20140219040204
Gaia: ac06cfbd2baf6494ffbb668cc599e3892cd5e17b
Gecko: bf0e76f2a7d4
Version: 30.0a1
v1.2-device.cfg

Comment 22

[Ran Ben Aharon [:ranbena] EverythingMe]

Kevin, let's discuss the icon rounding in Bug 975130