Convert the RSS extension to event pages to avoid a dedicated background process.

Convert the RSS extension to event pages to avoid a dedicated background process.
Also convert from deprecated message passing APIs (on/sendRequest to on/sendMessage).

BUG=None
TEST=None, everything should work as before.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192810

[not at google - send to devlin, 2013-03-26 15:25:38]

https://codereview.chromium.org/12843037/diff/1/chrome/test/data/extensions/s...
File chrome/test/data/extensions/subscribe_page_action/background.js (right):

https://codereview.chromium.org/12843037/diff/1/chrome/test/data/extensions/s...
chrome/test/data/extensions/subscribe_page_action/background.js:7: var feedData
= {};
when the event page unloads this will be lost, but what is it used for?

[Finnur, 2013-03-26 15:43:13]

Good catch. It worked in my test because the background page doesn't go away
immediately. If I wait, this breaks.

I'll need to do more work here...

This variable is used in the popup code (which is not part of this changelist).

[Finnur, 2013-03-27 12:01:51]

How about this?

[not at google - send to devlin, 2013-03-28 00:35:05]

lgtm but consider storage API

https://codereview.chromium.org/12843037/diff/13001/chrome/test/data/extensio...
File chrome/test/data/extensions/subscribe_page_action/popup.js (right):

https://codereview.chromium.org/12843037/diff/13001/chrome/test/data/extensio...
chrome/test/data/extensions/subscribe_page_action/popup.js:14: var feeds =
JSON.parse(localStorage.getItem(tab.id));
I don't fully understand what this is for, but we're encouraging the storage API
over localstorage - and this will be especially useful if it makes sense for
this to be sync'd.

[Finnur, 2013-03-28 21:31:28]

It actually does not make sense to sync this.

Here's a little background on this variable: The background page gets notified
whenever there's a feed on the page being visited. *If* the user is interested,
they'll click the feed and the popup will ask the background page: Give me all
the feeds that were found on the page, so I can show it in the popup.

Before, this used to be a memory variable, now it is saved in localStorage, so
the popup page can look it up on its own.

I guess my questions are:
1) Given that this data is only valid for the lifetime of the tab (and not
useful for sync), do you still recommend using the storage API? 
2) Does that API require 'storage' as a permission and if so, will the extension
get disabled for just under 1M users as it gets updated (requesting more
permissions).

[not at google - send to devlin, 2013-04-02 02:29:38]

On 2013/03/28 21:31:28, Finnur wrote:
> It actually does not make sense to sync this.
> 
> Here's a little background on this variable: The background page gets notified
> whenever there's a feed on the page being visited. *If* the user is
interested,
> they'll click the feed and the popup will ask the background page: Give me all
> the feeds that were found on the page, so I can show it in the popup.

Why can't the popup do the request? It seems like the background page doesn't
necessarily need to get involved here.

> 
> Before, this used to be a memory variable, now it is saved in localStorage, so
> the popup page can look it up on its own.
> 
> I guess my questions are:
> 1) Given that this data is only valid for the lifetime of the tab (and not
> useful for sync), do you still recommend using the storage API?

Insofar as it's "better" than localStorage, yes. The question is more whether
persistent storage suitable at all. See above question, perhaps the answer to
that will help me understand the problem.

> 2) Does that API require 'storage' as a permission and if so, will the
extension
> get disabled for just under 1M users as it gets updated (requesting more
> permissions).

The storage API doesn't have a permission warning, so hopefully it won't result
in disabling and re-enabling the extension. If so it'd be a bug.

[Finnur, 2013-04-04 13:46:44]

> Why can't the popup do the request? 

There's no technical reason why it couldn't, I suppose, but I'm trying to limit
the scope of the changes given that this extension isn't really under active
development (except the bare necessities to keep it running -- like converting
it from the deprecated Messaging API, which prompted all this). I suckered
myself into also converting the background page to be non-permanent (because I
had always meant to do that anyway and it looked like it wouldn't be any effort
at all), but it turned out to be the change that took the most time for this CL.
:)

However, I've converted it to use the storage API. I know you've green-lighted
it, but could you take a quick look at the changes to sanity check?

Thanks!

[Finnur, 2013-04-05 13:05:34]

ping

[not at google - send to devlin, 2013-04-08 04:39:55]

lgtm

https://codereview.chromium.org/12843037/diff/31001/chrome/test/data/extensio...
File chrome/test/data/extensions/subscribe_page_action/background.js (right):

https://codereview.chromium.org/12843037/diff/31001/chrome/test/data/extensio...
chrome/test/data/extensions/subscribe_page_action/background.js:25:
feeds[sender.tab.id] = input;
initialize directly? var feeds = {...} (in fact you don't even really need to
declare a variable for it)

https://codereview.chromium.org/12843037/diff/31001/chrome/test/data/extensio...
chrome/test/data/extensions/subscribe_page_action/background.js:54:
chrome.storage.local.remove(tabId.toString());
I think (hope) the toString() is unnecessary.

https://codereview.chromium.org/12843037/diff/31001/chrome/test/data/extensio...
File chrome/test/data/extensions/subscribe_page_action/popup.js (right):

https://codereview.chromium.org/12843037/diff/31001/chrome/test/data/extensio...
chrome/test/data/extensions/subscribe_page_action/popup.js:13:
chrome.tabs.getSelected(null, function(tab) {
null should be unnecessary now

https://codereview.chromium.org/12843037/diff/31001/chrome/test/data/extensio...
chrome/test/data/extensions/subscribe_page_action/popup.js:14:
chrome.storage.local.get(tab.id.toString(), function(result) {
ditto

[Finnur, 2013-04-08 12:59:02]

Committed patchset #4 manually as r192810 (presubmit successful).

[Finnur, 2013-04-08 12:59:24]

https://codereview.chromium.org/12843037/diff/31001/chrome/test/data/extensio...
File chrome/test/data/extensions/subscribe_page_action/background.js (right):

https://codereview.chromium.org/12843037/diff/31001/chrome/test/data/extensio...
chrome/test/data/extensions/subscribe_page_action/background.js:25:
feeds[sender.tab.id] = input;
This is actually the only way I could get it to work.

var feeds = {sender.tab.id: input};
 ... and ...
var feeds = {sender.tab.id.toString(): input};

gives me...

Uncaught SyntaxError: Unexpected token . 

Not a really helpful error message. I looked around for solutions and someone
suggested this and it worked like a charm.

https://codereview.chromium.org/12843037/diff/31001/chrome/test/data/extensio...
chrome/test/data/extensions/subscribe_page_action/background.js:54:
chrome.storage.local.remove(tabId.toString());
Nope, toString is needed. Otherwise I get:

Error in event handler for 'tabs.onRemoved': Invocation of form remove(integer)
doesn't match definition remove(string or array keys, optional function
callback) Error: Invocation of form remove(integer) doesn't match definition
remove(string or array keys, optional function callback)
    at normalizeArgumentsAndValidate (schemaUtils:120:11)
    at CustomBindingsObject.self.(anonymous function) [as remove]
(StorageArea:29:14)
    at chrome-extension://bmedchljhkolpfhojfonijnlhcdphapm/background.js:54:24
    at Event.dispatchToListener (event_bindings:356:21)
    at Event.dispatch_ (event_bindings:342:27)
    at dispatchArgs (event_bindings:214:26)
    at Object.chromeHidden.Event.dispatchEvent (event_bindings:223:7)
event_bindings:346

Overall I found the Storage API to be a lot more finicky than just localStorage.

https://codereview.chromium.org/12843037/diff/31001/chrome/test/data/extensio...
File chrome/test/data/extensions/subscribe_page_action/popup.js (right):

https://codereview.chromium.org/12843037/diff/31001/chrome/test/data/extensio...
chrome/test/data/extensions/subscribe_page_action/popup.js:13:
chrome.tabs.getSelected(null, function(tab) {
On 2013/04/08 04:39:55, kalman wrote:
> null should be unnecessary now

Done.

[not at google - send to devlin, 2013-04-08 22:01:27]

https://codereview.chromium.org/12843037/diff/31001/chrome/test/data/extensio...
File chrome/test/data/extensions/subscribe_page_action/background.js (right):

https://codereview.chromium.org/12843037/diff/31001/chrome/test/data/extensio...
chrome/test/data/extensions/subscribe_page_action/background.js:25:
feeds[sender.tab.id] = input;
On 2013/04/08 12:59:24, Finnur wrote:
> This is actually the only way I could get it to work.
> 
> var feeds = {sender.tab.id: input};
>  ... and ...
> var feeds = {sender.tab.id.toString(): input};
> 
> gives me...
> 
> Uncaught SyntaxError: Unexpected token . 
> 
> Not a really helpful error message. I looked around for solutions and someone
> suggested this and it worked like a charm.

Doh. I've been writing too much python.

https://codereview.chromium.org/12843037/diff/31001/chrome/test/data/extensio...
chrome/test/data/extensions/subscribe_page_action/background.js:54:
chrome.storage.local.remove(tabId.toString());
On 2013/04/08 12:59:24, Finnur wrote:
> Nope, toString is needed. Otherwise I get:
> 
> Error in event handler for 'tabs.onRemoved': Invocation of form
remove(integer)
> doesn't match definition remove(string or array keys, optional function
> callback) Error: Invocation of form remove(integer) doesn't match definition
> remove(string or array keys, optional function callback)
>     at normalizeArgumentsAndValidate (schemaUtils:120:11)
>     at CustomBindingsObject.self.(anonymous function) [as remove]
> (StorageArea:29:14)
>     at chrome-extension://bmedchljhkolpfhojfonijnlhcdphapm/background.js:54:24
>     at Event.dispatchToListener (event_bindings:356:21)
>     at Event.dispatch_ (event_bindings:342:27)
>     at dispatchArgs (event_bindings:214:26)
>     at Object.chromeHidden.Event.dispatchEvent (event_bindings:223:7)
> event_bindings:346
> 
> Overall I found the Storage API to be a lot more finicky than just
localStorage.
> 

Dang. Could you file bugs? I'm fixing the int key thing.

[Finnur, 2013-04-09 10:59:21]

So, I've already started to forget some of the details, but here's what I
remember of the problems, and looking at this list now I realize I was wrong to
blame the storage API for most of them:

1) The docs need improving. This I think is actually the main problem with the
Storage API. I'll file a bug.
2) The storage API is async, which means I have to restructure the code a tiny
bit. It's just the nature of the API beast. Not really an issue, just a minor
annoyance. :)
3) Not supporting int as key - this you know all about.
4) Getting the object stored with the Storage API took a bit of fiddling around.
I now realize this is more to do with Javascript not working the way I expected
it to, than the Storage API -- which I blamed at the time. The old localStorage
approach was easier, but that was because it didn't support an object so I had
to convert the object to (and from) JSON -- dead simple but really sub-par.
5) There were some problems when reading the object from the other side
(popup.js) but I think those fall in the same category as 4).

So. Two issues really (int as key and the docs).

[Finnur, 2013-04-09 11:37:51]

Docs issue filed here:
https://code.google.com/p/chromium/issues/detail?id=229359
Holy Duncan Ferguson! Are we up to close to 230K already? :)

[not at google - send to devlin, 2013-04-09 23:08:46]

Duncan Ferguson? Anyway, thanks. Totally agree.