search

mozilla / remote-newtab

Remotely-hosted New Tab Page
https://mozilla.github.io/remote-newtab/src/
Mozilla Public License 2.0
15 stars 7 forks source link

Blocked links #186

Closed emtwo closed 8 years ago

emtwo commented 8 years ago

Allow links to be blocked. Allow undoing a blocked link and restoring all blocked links.

emtwo commented 8 years ago

@k88hudson, I'm working on tests next (and checking out codeclimate failures) but otherwise, this code is ready for review

oyiptong commented 8 years ago

I've been thinking about blocklists a little.

I think it is an important enough feature to think this through:

The result of a blocked site coming back on-screen could be catastrophic for users and thus makes for a poor user experience.

There is a high value for blocked sites to not appear in the page. We've been using indexedDB to store blocked sites partly because we don't have access to the hashing algorithm present in-chrome.

We could implement a hashing API, and store the blocklist in prefs as it is. The reasoning is that people will switch between local and cached copies of the new tab page.

While this doesn't need to block this PR, @emtwo how much work do you think would it be to insert a blocklist coming from chrome in a message?

We'd need a few things:

  1. master block list (from a pref)
  2. hash API so we can hash sites individually or in bulk
  3. an API to send an item to block

The page would have to change its state, and send a signal to chrome to broadcast to all other newtab pages to get updated.

We also need a way to migrate the previously blocked sites to the new page.

Anything I'm missing?

oyiptong commented 8 years ago

It would be important to figure out the blocklist at some point. We'll need to factor it in a migration strategy eventually anyway. We could put some thought in this now, and file another issue.

emtwo commented 8 years ago

If I understand correctly, @oyiptong, your concern is that when we migrate to RNT we would reset blocked links and that's a big issue?

If so, why do we need to keep a master list in Firefox? Is the plan that users can switch between running newtab locally and remotely? So we can't just send the list once initially just to migrate?

Also, I'm not sure what you mean that we're using indexedDB because we don't have access to the hashing algorithm in chrome. Those seem like two orthogonal things. Aren't we just using indexedDB because it's "webby" and avoids extra unnecessary message passing?

In any case, implementing a master list with hashing and message passing for blocking doesn't sound like it should be a problem. I think it could be a follow-up to this PR too.

oyiptong commented 8 years ago

If so, why do we need to keep a master list in Firefox? Is the plan that users can switch between running newtab locally and remotely? So we can't just send the list once initially just to migrate?

Yes indeed. We had a different understanding of ServiceWorker before. We thought about it as an application cache, but we have then realized that we may not be able to control the eviction from that cache. The page or parts of it may fail to load.

In that case, and in the offline case, we need a fallback strategy. Eventually, the idea is perhaps to ship an addon with a potentially outdated, but functional copy of the newtab page. This would use the same code, though it will have gone through the "Firefox" gauntlet. This page will have a different origin, therefore cannot share indexedDB instances.

Also, I'm not sure what you mean that we're using indexedDB because we don't have access to the hashing algorithm in chrome. Those seem like two orthogonal things. Aren't we just using indexedDB because it's "webby" and avoids extra unnecessary message passing?

You are correct. They are two different things. We want to be as webby as possible. However, I think user-experience trumps webbiness. Let's see how that impacts message passing.

emtwo commented 8 years ago

@k88hudson r?

k88hudson commented 8 years ago

You have a few lint errors -- if you want to avoid warnings for intentional console.logs, use lib/log instead

This is looking awesome!

k88hudson commented 8 years ago

Also, it would be a good idea to combine undo and initdb into a blocked action/reducer so it's easier to find

emtwo commented 8 years ago

r? @k88hudson or @marcoscaceres

marcoscaceres commented 8 years ago

R+ from me, bar a couple of little nits. This was a lot of work! Nice one @emtwo!! :star2: :trophy:

@k88hudson, leaving the React bits to you to give a once over.

marcoscaceres commented 8 years ago

Note that the codeclimate are non-issues, but we should try to fix that. @k88hudson, any idea why ESLint is complaining about ES6 not being enabled? Maybe this code just needs a rebase?

marcoscaceres commented 8 years ago

@emtwo, you might want to check the coveralls report. It seems to be complaining about a couple of missing tests for two public functions. Should be easy to fix.

k88hudson commented 8 years ago

@marcoscaceres codeclimate is continuing to run jshint for some reason, even though i've set the configuration to disable it... however we can't actually go in and modify it since mozilla hasn't approved code-climate as an application -_-

basically, we'll need to email them.