878968 - Improve update event handling in addons manager

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: Add-on Manager, defect)

Description

[Brian Nicholson (:bnicholson)]

In the addons manager, we clear and rebuild the entire list whenever there's a change to an addon/search engine [1]. We could be smarter about the way things are updated, and we could instead only add/update/remove items individually. The downloads manager [2] is one example of where we keep track of and update individual items. 

[1] http://hg.mozilla.org/mozilla-central/file/8b1bfcf0ce6e/mobile/android/chrome/content/aboutAddons.js#l563
[2] http://hg.mozilla.org/mozilla-central/file/8b1bfcf0ce6e/mobile/android/chrome/content/aboutDownloads.js

Comment 1

[Artem Tak]

I would be happy to work on this

Comment 2

[Brian Nicholson (:bnicholson)]

(In reply to Artem Tak from comment #1)
> I would be happy to work on this

Great! I'll go ahead and assign it to you. To start, I'd recommend looking into aboutDownloads.js as a way to access an element based on an ID (particularly, _getElementForDownload() [1]). Let me know if you have any questions.

[1] http://hg.mozilla.org/mozilla-central/file/8b1bfcf0ce6e/mobile/android/chrome/content/aboutDownloads.js#l428

Comment 3

[Brian Nicholson (:bnicholson)]

Artem, are you still looking at this?

Comment 4

[Adrian May [:adrianmay]]

If Artem wants to keep this fair enough, but otherwise I'll give it a whirl.

But on my phone, I can't get any details to pop up. I just get the context menu with Uninstall and Disable.

Comment 5

[Adrian May [:adrianmay]]

All being quiet on the Western front I started on this today. It looks like a total rewrite and might take all week. Hopefully something reusable spins off it. 

I have a problem that the code talks about search engines, but I didn't persuade my phone to exhibit that functionality, so I'm not sure how to test my new version. How do I get a search engine to show up in the add-ons list?

Comment 6

[:Margaret Leibovic]

You're likely experiencing bug 902932. Do you still see this problem if you update your tree?

Comment 7

[Adrian May [:adrianmay]]

> You're likely experiencing bug 902932.

That bug seems to describe something different. I can get a context menu by long pressing. 

My problem is that I don't know how to install anything like a "search engine" that would appear in the add-ons list. I tried installing the AOL and Inudu add-ons.

I noticed some slippery intermittent bugs in this area, but rather than file them I'll rewrite the whole of aboutAddons.js and thereby make those bugs obsolete, giving you other ones instead of course ;-)

BTW, does anybody else notice that Settings->Customise->Search is only populated first time you go there? Or that long clicking on Google or Bing's input box and selecting Install seems to have no effect? I'll file them if they're news.

Comment 8

[Adrian May [:adrianmay]]

Attachment: Addons.patch

This is very WIP but I figured I might as well start collecting comments sooner rather than later. Still no idea what's intended as regards search engines.

Comment 9

[Adrian May [:adrianmay]]

Attachment: Addons.patch

OK, I'm done with this so it's over to you folks for comments.

Comment 10

[Brian Nicholson (:bnicholson)]

Comment on attachment 790253 [details] [diff] [review]
Addons.patch

Review of attachment 790253 [details] [diff] [review]:
-----------------------------------------------------------------

I like how you used JS prototypes to separate the addons controller and the search engines controller; it's definitely a more OO approach than what we have now. However, as Margaret reminded me, we're going to be dropping about:addons altogether soon. A rewrite of this size is likely to introduce regressions, so I don't think worth the risk since this is all going away in the semi-near future.

In fact, this bug was created right before we decided the current about:addons would be dropped. This bug is only about some minor optimizations to about:addons, so at this point, it's probably not worth spending any more resources on at all due to the reasons noted above.

I'm sorry to tell you this only after you submitted this patch -- you put a lot of work into this! To save some time for future bugs, I suggest describing a rough overview of your plan in the bug before doing so much. While I think this rewrite would be a good one with changes, it's far beyond the scope of this particular bug.

P.S. Some of the variable names used here were, well, interesting ("meat", "bingo", etc). Although perhaps boring, it's best to follow the style of existing code when it comes to names, formatting, comments, etc. This helps ensure the code stays readable and maintainable for future devs.

Comment 11

[Brian Nicholson (:bnicholson)]

Since we're dropping the current about:addons page, I don't think it's worth spending any more time on this bug (see comment 10).

By the way, for those interested, when I say "dropping about:addons", what I really mean is that we're moving this logic (or at least heavily modifying it) to be in the Java-side UI code -- the same way we handle about:home. This will allow addons to be manipulated directly in our settings UI.

Comment 12

[Adrian May [:adrianmay]]

No problem. It was fun all the same. Learned a bit of JS along the way too. 

:-)