Sample Extension for chrome.devtools.inspectedWindow.reload preprocessor option

Sample Extension for chrome.devtools.inspectedWindow.reload preprocessor option

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223348

[johnjbarton, 2013-09-03 23:51:21]

This is my first attempt to add example code so please point me in the right
direction. (The extension api patch is in review:
https://codereview.chromium.org/23514035/ so this CL is blocked)

[Mike West, 2013-09-04 08:54:37]

Sample looks pretty reasonable, just a few comments below. Thanks for putting
this together!

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
File
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/InspectedWindowLoadMonitor.js
(right):

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/InspectedWindowLoadMonitor.js:8:
* @ return {function} A function to be injected into the inspected window.
Nit: space after @.

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/InspectedWindowLoadMonitor.js:10:

Nit: kill the newline.

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/InspectedWindowLoadMonitor.js:27:
setTimeout(checkForLoad, 200);
Hrm. Is there no event you could hook into? The polling logic is trivial, but
distracts from the point of the sample.

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
File
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/PreprocessorPanel.js
(right):

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/PreprocessorPanel.js:13:
var postfix = '\n//# sourceURL=' + url + '.js\n';
Do you need the '.js' postfix? If 'listenerName' is set, you'd end up with
'http://example.com/file.html#listener.js', which seems strange.

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/PreprocessorPanel.js:53:
function createCell(text) {
Somewhat irrelevant to the extension's point, I know, but why use a table here?
It seems like you're really just building a list?

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/PreprocessorPanel.js:55:
td.textContent = text;
Thank you for not using 'innerHTML'! :)

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
File
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/chrome-preprocessor.html
(right):

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/chrome-preprocessor.html:3:
<script src="chrome-preprocessor.js"></script>
Hrm. Is there nothing in devtools similar to the `'background': { 'script':
[...] }` bits? That seems like something we should add. :)

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
File
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/manifest.json
(right):

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/manifest.json:7:
"content_security_policy": "default-src 'self'  chrome-extension-resource:
'unsafe-eval'; style-src 'self' 'unsafe-inline'; script-src 'self'
'unsafe-eval';  connect-src *; frame-src *;",
'unsafe-eval' has no effect in the 'default-src' directive ince you already have
it in 'script-src'. You can safely drop it from 'default-src'.

[Addy Osmani, 2013-09-04 12:28:01]

In addition to the nits from Mike, added one tiny comment but otherwise looks
good.

Nice work!

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
File
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/InspectedWindowLoadMonitor.js
(right):

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/InspectedWindowLoadMonitor.js:27:
setTimeout(checkForLoad, 200);
On 2013/09/04 08:54:37, Mike West wrote:
> Hrm. Is there no event you could hook into? The polling logic is trivial, but
> distracts from the point of the sample.

Afaik there isn't an alternative hook that could be used here. It might be worth
adding a comment that clarifies this in case.

[johnjbarton, 2013-09-04 18:57:48]

Thanks, PTAL.

Adding Pavel since it looks like this will work for devrel.

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
File
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/InspectedWindowLoadMonitor.js
(right):

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/InspectedWindowLoadMonitor.js:8:
* @ return {function} A function to be injected into the inspected window.
On 2013/09/04 08:54:37, Mike West wrote:
> Nit: space after @.

Done.

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/InspectedWindowLoadMonitor.js:10:

On 2013/09/04 08:54:37, Mike West wrote:
> Nit: kill the newline.

Done.

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/InspectedWindowLoadMonitor.js:27:
setTimeout(checkForLoad, 200);
On 2013/09/04 12:28:02, Addy Osmani wrote:
> On 2013/09/04 08:54:37, Mike West wrote:
> > Hrm. Is there no event you could hook into? The polling logic is trivial,
but
> > distracts from the point of the sample.
> 
> Afaik there isn't an alternative hook that could be used here. It might be
worth
> adding a comment that clarifies this in case.

I added a fileoverview comment. See also
https://code.google.com/p/chromium/issues/detail?id=285295

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
File
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/PreprocessorPanel.js
(right):

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/PreprocessorPanel.js:13:
var postfix = '\n//# sourceURL=' + url + '.js\n';
On 2013/09/04 08:54:37, Mike West wrote:
> Do you need the '.js' postfix? 

The .js postfix helps distinguish the preprocessor result (eg .js.js) from
network inputs (eg .js). 
Of course developers can make other choices, but I think for the example it is
important to not 
confuse the two buffers.

> If 'listenerName' is set, you'd end up with
> 'http://example.com/file.html#listener.js', which seems strange.

Indeed. I changed # to _, eg 'http://example.com/file.html_listener.js'

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/PreprocessorPanel.js:53:
function createCell(text) {
On 2013/09/04 08:54:37, Mike West wrote:
> Somewhat irrelevant to the extension's point, I know, but why use a table
here?
> It seems like you're really just building a list?

Done.

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
File
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/chrome-preprocessor.html
(right):

https://codereview.chromium.org/23892003/diff/1/chrome/common/extensions/docs...
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/chrome-preprocessor.html:3:
<script src="chrome-preprocessor.js"></script>
On 2013/09/04 08:54:37, Mike West wrote:
> Hrm. Is there nothing in devtools similar to the `'background': { 'script':
> [...] }` bits? That seems like something we should add. :)

Not to the best of my knowledge.

[Mike West, 2013-09-08 08:41:03]

LGTM, thanks for taking another pass.

[johnjbarton, 2013-09-09 15:54:56]

On 2013/09/08 08:41:03, Mike West wrote:
> LGTM, thanks for taking another pass.

Thanks Mike.

Pavel, any concerns?

The final patch for this feature has landed. I'll wait for the dev channel to
pick it up, test the example against dev channel, then land it.

[pfeldman, 2013-09-12 15:08:54]

lgtm

https://chromiumcodereview.appspot.com/23892003/diff/22001/chrome/common/exte...
File
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/PreprocessorPanel.css
(right):

https://chromiumcodereview.appspot.com/23892003/diff/22001/chrome/common/exte...
chrome/common/extensions/docs/examples/api/devtools/inspectedWindow/chrome-preprocessor/Panel/PreprocessorPanel.css:11:
font-size: 12px;
These should be sorted alphabetically.

[commit-bot: I haz the power, 2013-09-13 18:45:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/johnjbarton@chromium.org/23892003/27001

[commit-bot: I haz the power, 2013-09-13 19:04:15]

Failed to trigger a try job on chromium_presubmit
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-09-13 19:04:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/johnjbarton@chromium.org/23892003/26001

[commit-bot: I haz the power, 2013-09-13 19:27:30]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...

[commit-bot: I haz the power, 2013-09-16 16:04:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/johnjbarton@chromium.org/23892003/26001

[commit-bot: I haz the power, 2013-09-16 16:26:16]

Change committed as 223348