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

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: benfrancis, Assigned: benfrancis)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: sprintready)

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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

5 years ago
A Pivotal Tracker story has been created for this Bug: http://www.pivotaltracker.com/story/show/55556930
(Assignee)

Comment 2

5 years ago
Created attachment 794981 [details] [review]
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?)
Attachment #794981 - Flags: feedback?(dale)
Summary: [Meta] Fix awesomescreen cache → Fix awesomescreen cache
Whiteboard: sprintready
(Assignee)

Comment 3

5 years ago
Looks like the settings style breakage isn't this patch, I filed bug 909787 for that.
(Assignee)

Comment 4

5 years ago
Fixed known issues.
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.
Attachment #794981 - Flags: feedback?(dale) → feedback+
(Assignee)

Updated

5 years ago
Assignee: nobody → bfrancis
(Assignee)

Comment 6

5 years ago
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?
Flags: needinfo?(dale)
(Assignee)

Comment 7

5 years ago
I think I'm going to try going back to caching entire list elements and just updating the text on each key press...
(Assignee)

Comment 8

5 years ago
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.
Flags: needinfo?(dale)
(Assignee)

Comment 9

5 years ago
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.
Attachment #794981 - Flags: review?(dale)
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 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
Attachment #794981 - Flags: review?(dale)
https://github.com/mozilla-b2g/gaia/commit/e986740cba9d61a7320994439085d22faf07eb8f
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Duplicate of this bug: 902998
Duplicate of this bug: 903008
Duplicate of this bug: 905314
Duplicate of this bug: 905844
(Assignee)

Updated

5 years ago
Depends on: 903019
Attachment mime type: text/plain → text/x-github-pull-request
You need to log in before you can comment on or make changes to this bug.