Wired background loader component extension.

Wired background loader component extension.

TEST=make sure selected background pages are periodically loaded
BUG=141677
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=156137

[Mihai Parparita -not on Chrome, 2012-08-20 20:03:59]

https://chromiumcodereview.appspot.com/10855239/diff/1/chrome/browser/resourc...
File chrome/browser/resources/backloader/manifest.json (right):

https://chromiumcodereview.appspot.com/10855239/diff/1/chrome/browser/resourc...
chrome/browser/resources/backloader/manifest.json:14: "http://www.google.com/*"
Didn't catch this before, but do you actually need this permission?

[zel, 2012-08-20 21:01:10]

https://chromiumcodereview.appspot.com/10855239/diff/1/chrome/browser/resourc...
File chrome/browser/resources/backloader/manifest.json (right):

https://chromiumcodereview.appspot.com/10855239/diff/1/chrome/browser/resourc...
chrome/browser/resources/backloader/manifest.json:14: "http://www.google.com/*"
On 2012/08/20 20:03:59, Mihai Parparita wrote:
> Didn't catch this before, but do you actually need this permission?

Yeah, this was just a placeholder. I am still waiting for the list of actual
URLs here, will remove this for now. Done.

[Mihai Parparita -not on Chrome, 2012-08-20 21:24:03]

LGTM

https://chromiumcodereview.appspot.com/10855239/diff/1/chrome/browser/resourc...
File chrome/browser/resources/backloader/manifest.json (right):

https://chromiumcodereview.appspot.com/10855239/diff/1/chrome/browser/resourc...
chrome/browser/resources/backloader/manifest.json:14: "http://www.google.com/*"
On 2012/08/20 21:01:10, zel wrote:
> Yeah, this was just a placeholder. I am still waiting for the list of actual
> URLs here, will remove this for now. Done.

Note that unless you intent to actually inject script into these URLs, you don't
need permissions for them to just load them in an iframe.

[zel, 2012-08-20 22:15:44]

https://chromiumcodereview.appspot.com/10855239/diff/1/chrome/browser/resourc...
File chrome/browser/resources/backloader/manifest.json (right):

https://chromiumcodereview.appspot.com/10855239/diff/1/chrome/browser/resourc...
chrome/browser/resources/backloader/manifest.json:14: "http://www.google.com/*"
On 2012/08/20 21:24:03, Mihai Parparita wrote:
> On 2012/08/20 21:01:10, zel wrote:
> > Yeah, this was just a placeholder. I am still waiting for the list of actual
> > URLs here, will remove this for now. Done.
> 
> Note that unless you intent to actually inject script into these URLs, you
don't
> need permissions for them to just load them in an iframe.

How about postMessage business? Do I need to have this permission for that?

[Mihai Parparita -not on Chrome, 2012-08-20 22:23:09]

On 2012/08/20 22:15:44, zel wrote:
> How about postMessage business? Do I need to have this permission for that?

Nope, that's also meant to cross origins. Each endpoint can do its own
validation (as to whether it wants to receive a processed message) by checking
the origin that sent it the event:
https://developer.mozilla.org/en-US/docs/DOM/window.postMessage#Security_conc...

[James Hawkins, 2012-09-11 18:49:21]

LGTM with nits.

https://chromiumcodereview.appspot.com/10855239/diff/19001/chrome/browser/res...
File chrome/browser/resources/backloader/scripts/background.js (right):

https://chromiumcodereview.appspot.com/10855239/diff/19001/chrome/browser/res...
chrome/browser/resources/backloader/scripts/background.js:32: // Delayed closing
of the background page once when all iframes are loaded.
Optional nit: Blank line above this to separate it from the (supposed)
constants.

https://chromiumcodereview.appspot.com/10855239/diff/19001/chrome/browser/res...
chrome/browser/resources/backloader/scripts/background.js:33: max_delay_ms_:
10000,
nit: JS variable names use javascriptCase, e.g. maxDelayMs.

https://chromiumcodereview.appspot.com/10855239/diff/19001/chrome/browser/res...
chrome/browser/resources/backloader/scripts/background.js:74: getExtensionById_:
function(list, id) {
nit: Document method and params.  Here and elsewhere.

https://chromiumcodereview.appspot.com/10855239/diff/19001/chrome/browser/res...
chrome/browser/resources/backloader/scripts/background.js:76: if (list[i].id ==
id) {
nit: No braces for single-line blocks.

https://chromiumcodereview.appspot.com/10855239/diff/19001/chrome/browser/res...
chrome/browser/resources/backloader/scripts/background.js:90: var pages_to_load
= [];
nit: pagesToLoad.

https://chromiumcodereview.appspot.com/10855239/diff/19001/chrome/browser/res...
chrome/browser/resources/backloader/scripts/background.js:131:
loadLuncherFrame_: function(index, page_url) {
nit: pageUrl

[sky, 2012-09-11 19:25:51]

LGTM

[zel, 2012-09-11 19:59:40]

https://chromiumcodereview.appspot.com/10855239/diff/19001/chrome/browser/res...
File chrome/browser/resources/backloader/scripts/background.js (right):

https://chromiumcodereview.appspot.com/10855239/diff/19001/chrome/browser/res...
chrome/browser/resources/backloader/scripts/background.js:32: // Delayed closing
of the background page once when all iframes are loaded.
On 2012/09/11 18:49:22, James Hawkins wrote:
> Optional nit: Blank line above this to separate it from the (supposed)
> constants.

Done.

https://chromiumcodereview.appspot.com/10855239/diff/19001/chrome/browser/res...
chrome/browser/resources/backloader/scripts/background.js:33: max_delay_ms_:
10000,
On 2012/09/11 18:49:22, James Hawkins wrote:
> nit: JS variable names use javascriptCase, e.g. maxDelayMs.

Done.

https://chromiumcodereview.appspot.com/10855239/diff/19001/chrome/browser/res...
chrome/browser/resources/backloader/scripts/background.js:74: getExtensionById_:
function(list, id) {
On 2012/09/11 18:49:22, James Hawkins wrote:
> nit: Document method and params.  Here and elsewhere.

Done.

https://chromiumcodereview.appspot.com/10855239/diff/19001/chrome/browser/res...
chrome/browser/resources/backloader/scripts/background.js:76: if (list[i].id ==
id) {
On 2012/09/11 18:49:22, James Hawkins wrote:
> nit: No braces for single-line blocks.

Done.

https://chromiumcodereview.appspot.com/10855239/diff/19001/chrome/browser/res...
chrome/browser/resources/backloader/scripts/background.js:90: var pages_to_load
= [];
On 2012/09/11 18:49:22, James Hawkins wrote:
> nit: pagesToLoad.

Done.

https://chromiumcodereview.appspot.com/10855239/diff/19001/chrome/browser/res...
chrome/browser/resources/backloader/scripts/background.js:131:
loadLuncherFrame_: function(index, page_url) {
On 2012/09/11 18:49:22, James Hawkins wrote:
> nit: pageUrl

Done.

[commit-bot: I haz the power, 2012-09-11 19:59:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zelidrag@chromium.org/10855239/22001

[commit-bot: I haz the power, 2012-09-11 20:00:01]

Failed to apply patch for chrome/browser/resources/backloader/manifest.json:
While running patch -p1 --forward --force;
patching file chrome/browser/resources/backloader/manifest.json
Hunk #1 FAILED at 11.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/browser/resources/backloader/manifest.json.rej