Cookie whitelist

[jeroenev]

PR for adding ability to whitelist cookies #30
i'm not a JS developer nor a chrome extension dev
so feedback is definitely welcome

TODO: fix button styling
TODO: change button icon if whitelisted
TODO: remove item from whitelist if clicked on a button of a whitelisted cookie
TODO: don't add duplicate whitelist items
TODO: make whitelist a dictionary instead of an array
TODO: rewriting logic to detect if cookie is in whitelist
TODO: add button to whitelist domains instead of specific cookies
TODO: reusing logic for delete all cookies button

[Manvel]

Thanks @jeroen7s for the PR I won't have a chance to review the PR earlier than weekend, but as I see there are still a lot of TODOs, I assume the PR is not yet ready for the review, right ?

[Manvel]

i'm not a JS developer nor a chrome extension dev
so feedback is definitely welcome

I don't have yet any linter, or contribution guide setup yet and probably there is a lot of refactoring I need to do one day, so I'll really appreciate if you keep the coding/naming consistent with current implementation. The more readable is code and consistent the easier for me would be to review it.

[jeroenev]

feedback is always appreciated
will try to complete those TODO's this weekend somewhere

[jeroenev]

small question
is it possible to add an additional parameter (like name, storeId, httpOnly, secure etc...) called whitelist to a cookie, or would that create a security issue?
not sure if sites would be able to get or alter that parameter if added
then we don't need a separate array of arrays (or dictionary) to store the whitelist,
EDIT: nvm, that wouldn't allow for domain-wide whitelisting

[jeroenev]

welp, will not have much time this weekend after all, guess it will take a bit longer

[Manvel]

@jeroen7s No worries - take your time, thanks a lot for providing PR though.

Currently I'm busy with another project so I'll not annoy you with the Merge conflicts soon :)

[jeroenev]

I think most of the work is done now
I only need to figure out how to let the UI know which cookies/domains have been white-listed
and replace the current ugly broken icon with a better one, with an checked/unchecked state icon
but that will be for this weekend I think
or maybe tomorrow or friday-evening
you should be able to review most of the code already :)

[Manvel]

you should be able to review most of the code already :)

I'll try to have a skim before the weekend, otherwise I'll do full review, on weekend.

[jeroenev]

updated the code now, implemented the feedback
now the main thing that remains is updating the UI and changing the icons
however the logic for that seems a bit complicated atm (at least from a non-JS / non-UI dev's perspective)
and i'm not sure how I should go about it
maybe you could take a look at it or point me in the right direction?

[Manvel]

updated the code now, implemented the feedback

I'll again try to review that during the week, otherwise will do complete review over the weekend.

however the logic for that seems a bit complicated atm (at least from a non-JS / non-UI dev's perspective)

I see, yes it might seem to be complicated, here are some thoughts, hope they will make the task simple:

• Differentiate the states
• Reflect list item whitelist state when loaded
• Detect local storage change
• Reflect change on the UI accordingly

Differentiate the states

Currently the differentiation is done using data attributes for example data-expanded="true" , maybe you can use something like data-whitelisted="true" and create CSS accordingly, smth like:

.tableList > li[data-whitelisted="true"] > div .icon.whitelist
{
  background-image: url(...);
}

.tableList li > ul li[data-whitelisted="true"] .icon.whitelist
{
  background-image: url(...);
}

Reflect list item whitelist state when loaded

We have two places where we are loading cookies, first when we are loading Domain cookies and when we are loading Cookies for domain.

if (whitelisted)
  list.dataset.whitelisted = "true";

Detect local storage change

Chrome storage API does have onchange event, this can be used for detecting whitelisting changes and update the table accordingly.

We are already detecting cookies change and act accordingly here. So we might need something like onWhitelistChange and act accordingly when the state in the local storage is changed.

Reflect change on the UI accordingly

Reflecting changes on the UI I think can be done similarly as in the onCookiesChange, there is updateItem method which accept modified element and accessor, this way should be possible to update the Cookie list state.

const accessor = domain;
domainElement.dataset.whitelisted = state;
tableList.updateItem(domainElement, accessor);

// Similar way is done for sublist

Hope this helps.

[jeroenev]

thanks, I updated the code, with the workaround.
also added code to update the whitelist status on loading the cookie tab and when loading domain cookies.
so everything is basically functional, though it can still be improved.
the UI styling needs some work
as it currently looks like this:

[Manvel]

as it currently looks like this:

I thought you plan to use icon rather than "toggle switch", "wl", "bl" does only say something to the English speaking user, but that's not a good idea to label the switch states, in general the switch needs to be updated to use no label and have improved accessibility.

Suggestion: I think if you keep consistent with your initial idea of the custom icon, that will probably work better for now, rather than updating a label that will need to be translated as result. For accessibility reasons a native tooltip can be used, as there is no characters limitation for that and might be more descriptive.

[Manvel]

@jeroen7s I'm Sorry for being slow this weekend (Traveling), I might have not much time next week either, but will try to do full review again, otherwise I'll do the week after.

[jeroenev]

should pretty much be done now
let me know if theres anything left to be done

[Manvel]

@jeroen7s Thanks, I'll review the changes over the weekend.

[Manvel]

We are almost there.

Besides of 2 comments I have left, there are still some nits that needs to be fixed.

Regarding comments: If you are fine with the comments, can you please add those two fixes as well ?

Regarding Nit picking: I think they shouldn't be discussed in the review, but rather should be test implemented for detecting them. So let's keep them outside of the scope of current PR, I'll followup and fix them separately and setup a Travis CI which will spot inconsistencies automatically, after we can adapt the linters if any disagreement regarding styling.

Basically: Please fix 2 comments when you have time, I'll take care for the rest.

[Manvel]

That was fast :) LGTM and thanks a lot for the PR 🎉

[jeroenev]

awesome 🎉 thanks a lot