search

citp / news-disinformation-study

A research project on how web users consume, are exposed to, and share news online.
8 stars 2 forks source link

Add-on review rev 13d60a95b2e5298797e2d7ddde31b617fe317d5b #69

Closed wagnerand closed 3 years ago

wagnerand commented 3 years ago

I completed the add-on review of rev 13d60a95b2e5298797e2d7ddde31b617fe317d5b.

Overall, the code looks ok. There are a few issues we'd ask you to resolve for the final build:

1) Please remove all unused permissions from manifest.json. It looks like at least the following permissions are unused:

Thank you.

rhelmer commented 3 years ago

I suggested the management permission since we will likely want to have the add on self uninstall after the study ends to ensure everything is cleaned up, and I didn’t want users to have to get an extra upgrade prompt when the permissions change.

Maybe that’s not a concern or there’s a better way to do this though :)

akohlbre commented 3 years ago

Excellent, thanks! We will end up using the management perm for uninstall, but I think you're right about the others. I'll make these changes in my final push.

wagnerand commented 3 years ago

Sounds good, thank you!

kewisch commented 3 years ago

I am fairly certain the management.uninstallSelf method does not actually require the management permission. Good to test to be certain before you remove it.

akohlbre commented 3 years ago

Indeed, I can uninstall self without the management perm. Handy :)

I think what's pushed now is full functionality. @knowtheory and I have some UX improvements that we'll likely want to get through, but this is it for now. @wagnerand is it possible to get this version signed off on?

akohlbre commented 3 years ago

(sorry, I just pushed one last commit to fix a tracked domain, hopefully nobody had grabbed the code yet)