906055 - Fix awesomescreen cache

Status: RESOLVED FIXED

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

Description

[Ben Francis [:benfrancis]]

Bug 826497 introduced a number of regressions. This bug is to track those so that when we re-factor that code we can see if they get fixed.

Comment 1

[ffos-browser]

A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/55556930

Comment 2

[Ben Francis [:benfrancis]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/11735

I was working on bug 833149 but the awesomescreen was so tangled and broken that I decided to re-factor it first. This patch fixes bug 902998, bug 903008, bug 905314 and bug 905844 so I've added it to this bug, but it removes the caching added in bug 826497.

It turns out that there's actually very little that we can cache. The time you really need the cache is on keypresses when typing in the awesomebar, but because we need to highlight matching text in the title and url of the result we need to modify it on every key press anyway. One thing we can cache is favicons, so I'd like to add that back in (not yet in this patch).

There are still some problems with this patch but I wanted to get some early feedback as this is a reasonably sized re-factor and I won't be able to continue with it until Tuesday.

Known issues:
* The bookmark star isn't unfilled when unbookmarking from the bookmark menu
* Settings styles look broken (caused by this patch?)

Comment 3

[Ben Francis [:benfrancis]]

Looks like the settings style breakage isn't this patch, I filed bug 909787 for that.

Comment 4

[Ben Francis [:benfrancis]]

Fixed known issues.

Comment 5

[Dale Harvey (:daleharvey)]

Comment on attachment 794981 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11735

Looking good so far, this is definitely the type of change that requires the introduction of basic integration tests though.

Comment 6

[Ben Francis [:benfrancis]]

I've added a couple more commits to the pull request (https://github.com/mozilla-b2g/gaia/pull/10102) to cache icons in memory instead of querying indexedDB for every icon on every key press, use cloneNode on template nodes instead of creating new ones all the time and use replaceNode instead of innerHTML=''... but you can still see the flicker that the original patch fixed.

That patch worked by caching entire list items from the DOM, but in doing so it broke keyword highlighting and all the context menu event listeners.

I'm struggling to find a caching strategy that can eliminate the flicker and not cause the 4 regressions the other patch caused. I think the next step is probably to do some profiling to figure out exactly what's taking the time but I'm reluctant to spend too much more time fixing this glitch when all I really wanted to do was unblock bug 833149.

I'll continue to work on this, but I'm open to suggestions on a better fix. Dale, do you have any suggestions?

Comment 7

[Ben Francis [:benfrancis]]

I think I'm going to try going back to caching entire list elements and just updating the text on each key press...

Comment 8

[Ben Francis [:benfrancis]]

Have updated the pull request with a proof of concept with no flicker and no regressions :)

Still needs a little more work and some tests.

Comment 9

[Ben Francis [:benfrancis]]

Comment on attachment 794981 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11735

OK, I've got the awesomescreen into a much happier state, with a working cache and no flicker and refactored into its own module :)

I'm afraid it took me several hours to figure out how to write my first extremely basic integration test and get it to pass so I haven't been able to get the test coverage to where we want it to be.

I'll leave it up to Dale whether we want to review and land this now so that I'm unblocked on working on search suggestions again, or wait until I slowly learn how to write more complex integration tests.

There are so many tests to write that I actually think we should start filing bugs to write unit tests and integration tests for each of the newly extracted modules in the browser app (toolbar, settings, awesomescreen etc.) to bootstrap our test coverage. I suspect we may need to do some work on making them more loosely coupled and testable as well.

Comment 10

[Dale Harvey (:daleharvey)]

In my experience backfilling tests isnt a great idea, there is just so many cases to test and its pretty much an infinitely long task without a filter, I find it best to require tests for new functionality and very importantly to add tests for any regressions, this give you a filter of tests for things that actually break.

So happy for this to have a very basic test, it can be incrementally added to as we work on it more, should get to this tonight

Comment 11

[Dale Harvey (:daleharvey)]

Comment on attachment 794981 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11735

Clearing review, the tests are currently failing, as far as I can tell the awesome screen is definitely not getting shown so I think previous passes may have been false positives

From a code perspective this is good though, and does fix a whole bunch of bugs which is sweet

if you DEBUG=* when running the tests you can see dump() output, debugging from there shouldnt be too much of a problem

Comment 12

[Dale Harvey (:daleharvey)]

Comment on attachment 794981 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/11735

Tests all fixed, cheers

Comment 13

[Dale Harvey (:daleharvey)]

https://github.com/mozilla-b2g/gaia/commit/e986740cba9d61a7320994439085d22faf07eb8f