Closed Bug 910316 Opened 11 years ago Closed 11 years ago

[e.me] Everything.me features in enhanced branch for Firefox OS 1.2

Categories

(Firefox OS Graveyard :: Gaia::Everything.me, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::Everything.me
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: sonmarce, Assigned: amirn)

References

Details

Attachments

(1 file, 2 obsolete files)

Patch - redirect to github PR
(deleted), text/html
crdlc
: review+
ranbena
: review+
zcampbell
: feedback+
Details
No description provided.
Blocks: 1.3-e.me
This is to group all features implemented in everything.me branch to start a review and land them on master branch. And this is the whole list: * Homescreen: * Convert landing page into a normal grid page (can contain apps) * New grid icon type - Collection * Enable dragging a grid icon (app/bookmark) onto a Collection * Add Collection via grid contextmenu * Enable grid-wide banner * Remove opacity change on panning * Collection: * Open/close animation * Static apps edit mode * Long tap Cloud app to pin to top * Add Collection settings - add apps, rename * Icon design update + icon for collection with less than 3 icons * Evme search results: * Dedup results * Improve Installed app categorization logic * Enable bookmarks in results as well * Add Marketplace download suggestions * Remove result click animation * Remove new result animation * Show browser app when query is URL * Allow saving a query as a collection
Cristian, do you think it's critical to have #910326 landed along with this patch?
Flags: needinfo?(crdlc)
IMHO it is not needed because it is available in v1-train right now, right? Am I wrong? Thanks
It is available but should be adapted to new Collection storage. So basically, search history will persist, but not Custom added shortcuts.
Attached file Patch - redirect to github PR (obsolete) (deleted) —
Attachment #797361 - Flags: review?(crdlc)
That should really be a meta-bug with dependencies. Dumping a huge patch like that is not ok. It looks like you even changed the firefox logo in the unofficial branding. Please don't.
Fabrice, this commit is far from ideal - believe me, we know. For multiple reasons we were forced to squash our commits into this PR. Crdlc was is the loop for the last 1.5 weeks, assisting us with coding and making sense of this feature-full commit. I will elaborate offline. And yes, the logo images were added by mistake and have been removed.
Broke off feature "Allow saving a query as a collection" Moved http://bugzil.la/911022
Comment on attachment 797361 [details] Patch - redirect to github PR Please Ran, could you review the evme part? Thanks
Attachment #797361 - Flags: review?(ran)
Please add the person working on it and change the status of the bug
Comment on attachment 797361 [details] Patch - redirect to github PR ><html><head><title>Redirect to pull request #11844</title><meta http-equiv="Refresh" content="2; url=https://github.com/mozilla-b2g/gaia/pull/11844"/></head><body>Redirect to pull request #11844 </body></html>
Attachment #797361 - Attachment mime type: text/plain → text/html
Assignee: nobody → amirn
Status: NEW → ASSIGNED
Broke off feature "Dedup results" Moved to http://bugzil.la/id=911180
Removed no longer relevant feature "Add Collection settings - add apps, rename"
Broke off feature "Add Collection via grid contextmenu" Moved to http://bugzil.la/id=911234
Broke off feature "Offline ready support" Moved to http://bugzil.la/id=911544
What is the current state of this? Thanks
Flags: needinfo?(crdlc) → needinfo?(amirn)
I've broken off as much features as I could to separate tickets. The diff hasn't changed substantially. Breaking off bigger pieces of code will set us back a week or more. We can't afford it. Apart from that the PR has been left untouched and it's dependencies are ready for review and merge as well. New features and bugs have also been created in separate tickets. Once we merge this into master, we can PR all the rest.
Flags: needinfo?(amirn)
When something can be reviewed, please tell me
All comments are on Github, thanks
Thanks a lot!! After Ran's review and all his comments will be addressed, we can start testing the code in our devices. If it works fine, r=+ from my side without doubts. Good work
Comment on attachment 797361 [details] Patch - redirect to github PR Awesome work as usual!!! All my comments were addressed so it is r+ from my side. Although we have to test the functionality before merging in our devices
Attachment #797361 - Flags: review?(crdlc) → review+
Attachment #797361 - Flags: review?(ran) → review+
blocking-b2g: --- → koi?
I'm going to backout this if you don't do it before me. This is not fine to land a at least 5mb memory regression as well as a fps regression in the homescreen. Sorry guys.
Vivien: Can you please back this out? QA would prefer to have to a new build to smoketest today.
Just making a note that it appears the patch was backed out here: https://github.com/mozilla-b2g/gaia/commit/32644e1564ee75e2a60ca455e12f092901c03fed
I was testing with different Gecko builds and this patch runs 30fps faster in 19-08 in compare to 05-09. The same happens with master. Any suggestion?
(In reply to Kevin Grandon :kgrandon from comment #26) > Just making a note that it appears the patch was backed out here: > https://github.com/mozilla-b2g/gaia/commit/ > 32644e1564ee75e2a60ca455e12f092901c03fed Yep I asked yesterday to back it out when I realized that this patch has landed. There are a number of issues we need to resolve before landing it again. Especially regressions on the homescreen couple with some big performance regressions.
No longer depends on: 910326
Attachment #806470 - Flags: review?(ran)
Attachment #806470 - Flags: review?(crdlc)
Depends on: 916827
Attachment #806470 - Flags: feedback?(zcampbell)
Zac, I left some explanations in the PR ui-test code.
Comment on attachment 806470 [details] Patch - redirect to github PR f+ on the condition that the sleep is removed in the follow-up pull request.
Attachment #806470 - Flags: feedback?(zcampbell) → feedback+
Comment on attachment 806470 [details] Patch - redirect to github PR Excellent work!
Attachment #806470 - Flags: review?(crdlc) → review+
Attachment #806470 - Flags: review?(ran) → review+
Attached file Patch - redirect to github PR (obsolete) (deleted) —
This is the same exact patch as the previous 806470, which had to be reissued due to the fact that previous was reverted (Gaia technical difficulties).
Attachment #797361 - Attachment is obsolete: true
Attachment #809668 - Attachment is obsolete: true
Please Ran take a look to all dependencies after landing https://github.com/mozilla-b2g/gaia/commit/62fff6a76a6c34471ed2f560287c7c64914e44c3
Flags: needinfo?(ran)
Depends on: 910331
Depends on: 920948
No longer depends on: 914875
All dependencies were resolved
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: needinfo?(ran)
Resolution: --- → FIXED
Blocks: 910331
No longer depends on: 910331
Blocks: 920948
No longer depends on: 920948
blocking-b2g: koi? → ---
blocking-b2g: --- → koi+
No longer blocks: 910331
No longer blocks: 911958
Depends on: 910331
No longer blocks: 922859
I was not able to uplift this bug to v1.2. If this bug has dependencies which are not marked in this bug, please comment on this bug. If this bug depends on patches that aren't approved for v1.2, we need to re-evaluate the approval. Otherwise, if this is just a merge conflict, you might be able to resolve it with: git checkout v1.2 git cherry-pick -x -m1 62fff6a76a6c34471ed2f560287c7c64914e44c3 <RESOLVE MERGE CONFLICTS> git commit
Flags: needinfo?(amirn)
Per a recent release drivers discussion, we need to hold off on uplifting this to 1.2 until we confirm a path forward post the planned e.me 1.2 status meeting tomorrow.
Whiteboard: [NO_UPLIFT]
Clearing nom - we're no longer taking e.me 1.2 feature changes to 1.2.
blocking-b2g: koi+ → ---
Flags: needinfo?(amirn)
Whiteboard: [NO_UPLIFT]
Jason - Does this mean that that Marketplace search is not part of e.me search as specified in the attachment and comment #1?
(In reply to David Bialer [:dbialer] from comment #41) > Jason - Does this mean that that Marketplace search is not part of e.me > search as specified in the attachment and comment #1? The marketplace integration into e.me search is currently not integrated 1.2, but it's still up for debate if it will be uplifted to 1.2 or not. Chris mentioned in the drivers meeting that there is still a desire to get that feature into 1.2. However, what is facilitating the debate was the fact the initial testing of the marketplace integration into e.me search on a custom branch had fundamental problems such as: 1. e.me search was only producing marketplace related results when the exact app name was matched - not any other case (e.g. marketplace category used producing relevant results to that category) 2. When selecting an app from marketplace suggestions on e.me search results, the marketplace app opens, but not to the app details page for that app 3. Whens selecting download more apps from marketplace suggestions on e.me search results, the marketplace app opens, but not to the search results page using the e.me search term provided I have an action item to test this again on the landed e.me changes on master to see if the same problems exist. If they do, I'll get bugs on file for these.
just some info about points 2&3 - we had an issue with the Marketplace API (moz activity) not opening the correct pages. To counter this, on master we now open a browser directly: https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/everything.me/js/etmmanager.js#L264 Though I'm unsure if is an acceptable solution, or if it's just a temporary fix until the marketplace API is fixed.
Guys, this feature is an inseparable part of Bug 910302. Code-wise it's not possible to detach and uplift.
(In reply to Evyatar 'Tron' Amitay (everything.me) from comment #43) > just some info about points 2&3 - we had an issue with the Marketplace API > (moz activity) not opening the correct pages. To counter this, on master we > now open a browser directly: > https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/everything. > me/js/etmmanager.js#L264 > > Though I'm unsure if is an acceptable solution, or if it's just a temporary > fix until the marketplace API is fixed. No that's not acceptable. Who reviewed that?
(In reply to Jason Smith [:jsmith] from comment #42) > (In reply to David Bialer [:dbialer] from comment #41) > > Jason - Does this mean that that Marketplace search is not part of e.me > > search as specified in the attachment and comment #1? > > The marketplace integration into e.me search is currently not integrated > 1.2, but it's still up for debate if it will be uplifted to 1.2 or not. > Chris mentioned in the drivers meeting that there is still a desire to get > that feature into 1.2. However, what is facilitating the debate was the fact > the initial testing of the marketplace integration into e.me search on a > custom branch had fundamental problems such as: > > 1. e.me search was only producing marketplace related results when the exact > app name was matched - not any other case (e.g. marketplace category used > producing relevant results to that category) > > 2. When selecting an app from marketplace suggestions on e.me search > results, the marketplace app opens, but not to the app details page for that > app > > 3. Whens selecting download more apps from marketplace suggestions on e.me > search results, the marketplace app opens, but not to the search results > page using the e.me search term provided > > I have an action item to test this again on the landed e.me changes on > master to see if the same problems exist. If they do, I'll get bugs on file > for these. Do you know if there bugs filed for these issues? I was not able to locate them. thanks.
Just did. Here's the relevant bugs. - bug 925804 - bug 925805 - bug 925806 - bug 925815
Blocks: 873167
Blocks: 920445
Depends on: 925461
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: