Fixed Screenshot extension example

Fixed Screenshot extension example

BUG=

Committed: https://crrev.com/29cb00fdc723f85c108cc9cdf3c66c98332ee009
Cr-Commit-Position: refs/heads/master@{#291874}

[François Beaufort, 2014-08-21 08:33:47]

Since `captureVisibleTab` now requires <all_urls>, I've updated the Screenshot
Extension sample to use activeTab to reflect our best practises.

BUG=367283
NOTRY=true

[not at google - send to devlin, 2014-08-21 15:00:51]

https://codereview.chromium.org/490133002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js
(right):

https://codereview.chromium.org/490133002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js:19:
chrome.tabs.create({url: viewTabUrl}, function() {
You still do need to do the onUpdated dance here. Creating a tab doesn't
necessarily mean it's loaded, it just means the UI frame has been created.

https://codereview.chromium.org/490133002/diff/1/chrome/common/extensions/doc...
File chrome/common/extensions/docs/examples/api/tabs/screenshot/screenshot.html
(right):

https://codereview.chromium.org/490133002/diff/1/chrome/common/extensions/doc...
chrome/common/extensions/docs/examples/api/tabs/screenshot/screenshot.html:6:
<img id="target" src="white.png" height="480">
Fairly unimportant question:

Why this change? Did you only specify one to keep the scaling right? I would
have thought that specifying just the width would be more common, but I don't
really know what this is supposed to do.

[François Beaufort, 2014-08-22 12:03:32]

On 2014/08/21 15:00:51, kalman wrote:
>
https://codereview.chromium.org/490133002/diff/1/chrome/common/extensions/doc...
> File chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js
> (right):
> 
>
https://codereview.chromium.org/490133002/diff/1/chrome/common/extensions/doc...
> chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js:19:
> chrome.tabs.create({url: viewTabUrl}, function() {
> You still do need to do the onUpdated dance here. Creating a tab doesn't
> necessarily mean it's loaded, it just means the UI frame has been created.
> 

Thank you for this. As you can see, I've slightly updated the onUpdated dance
because the onUpdated 
event listener was never triggered the first time we click on the browser
action. 
I'm not sure why but this should address this issue.

>
https://codereview.chromium.org/490133002/diff/1/chrome/common/extensions/doc...
> File
chrome/common/extensions/docs/examples/api/tabs/screenshot/screenshot.html
> (right):
> 
>
https://codereview.chromium.org/490133002/diff/1/chrome/common/extensions/doc...
> chrome/common/extensions/docs/examples/api/tabs/screenshot/screenshot.html:6:
> <img id="target" src="white.png" height="480">
> Fairly unimportant question:
> 
> Why this change? Did you only specify one to keep the scaling right? I would
> have thought that specifying just the width would be more common, but I don't
> really know what this is supposed to do.

Yup, this is to keep the scaling intact. If I specify the "width" I may end up
with a very tall image if I'm in rotated mode.
Specifying the "height" only will make sure it fits into a regular tab most of
the time.

[not at google - send to devlin, 2014-08-22 14:20:51]

LGTM with a couple of cleanup suggestions. These comments could just as well
apply to the original code.

https://codereview.chromium.org/490133002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js
(right):

https://codereview.chromium.org/490133002/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js:46:
chrome.tabs.onUpdated.addListener(addSnapshotImageToTab);
Feel free not to do this, but I've seen a nice pattern of:

chrome.tabs.onUpdated.addListener(function listener() {
  /* check stuff... */

  chrome.tabs.onUpdated.removeListener(listener);

  /* do stuff... */
});

which would avoid needing to have that temporary addSnapshotImageToTab variable.

https://codereview.chromium.org/490133002/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js:47:
chrome.tabs.create({url: viewTabUrl}, function(tab) {
And also - you can avoid |targetId| if you nest things even more:

chrome.tabs.create({url: viewTabUrl}, function(tab) {
  chrome.tabs.onUpdated.addListener(function listener() {
    /* use tab.id rather than targetId */
  }
});

More nesting, but fewer variables escaping out of their necessary scope. If you
went back to having that separate takeScreenshot() function it might help the
nesting a little.

[François Beaufort, 2014-08-25 07:35:58]

https://codereview.chromium.org/490133002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js
(right):

https://codereview.chromium.org/490133002/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js:46:
chrome.tabs.onUpdated.addListener(addSnapshotImageToTab);
On 2014/08/22 14:20:51, kalman wrote:
> Feel free not to do this, but I've seen a nice pattern of:
> 
> chrome.tabs.onUpdated.addListener(function listener() {
>   /* check stuff... */
> 
>   chrome.tabs.onUpdated.removeListener(listener);
> 
>   /* do stuff... */
> });
> 
> which would avoid needing to have that temporary addSnapshotImageToTab
variable.

I love this pattern.
Thanks!

https://codereview.chromium.org/490133002/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js:47:
chrome.tabs.create({url: viewTabUrl}, function(tab) {
On 2014/08/22 14:20:51, kalman wrote:
> And also - you can avoid |targetId| if you nest things even more:
> 
> chrome.tabs.create({url: viewTabUrl}, function(tab) {
>   chrome.tabs.onUpdated.addListener(function listener() {
>     /* use tab.id rather than targetId */
>   }
> });
> 
> More nesting, but fewer variables escaping out of their necessary scope. If
you
> went back to having that separate takeScreenshot() function it might help the
> nesting a little.

Sadly, adding the listener chrome.tabs.onUpdated after chrome.tabs.create will
always fail the first time we load the extension whereas it works fine after.
I'm not sure why sadly ;(

[not at google - send to devlin, 2014-08-25 16:32:06]

https://codereview.chromium.org/490133002/diff/20001/chrome/common/extensions...
File chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js
(right):

https://codereview.chromium.org/490133002/diff/20001/chrome/common/extensions...
chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js:47:
chrome.tabs.create({url: viewTabUrl}, function(tab) {
On 2014/08/25 07:35:57, François Beaufort wrote:
> On 2014/08/22 14:20:51, kalman wrote:
> > And also - you can avoid |targetId| if you nest things even more:
> > 
> > chrome.tabs.create({url: viewTabUrl}, function(tab) {
> >   chrome.tabs.onUpdated.addListener(function listener() {
> >     /* use tab.id rather than targetId */
> >   }
> > });
> > 
> > More nesting, but fewer variables escaping out of their necessary scope. If
> you
> > went back to having that separate takeScreenshot() function it might help
the
> > nesting a little.
> 
> Sadly, adding the listener chrome.tabs.onUpdated after chrome.tabs.create will
> always fail the first time we load the extension whereas it works fine after.
> I'm not sure why sadly ;(

Ah ok. Bummer. I can see why.

[François Beaufort, 2014-08-26 06:56:33]

On 2014/08/25 16:32:06, kalman wrote:
>
https://codereview.chromium.org/490133002/diff/20001/chrome/common/extensions...
> File chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js
> (right):
> 
>
https://codereview.chromium.org/490133002/diff/20001/chrome/common/extensions...
> chrome/common/extensions/docs/examples/api/tabs/screenshot/background.js:47:
> chrome.tabs.create({url: viewTabUrl}, function(tab) {
> On 2014/08/25 07:35:57, François Beaufort wrote:
> > On 2014/08/22 14:20:51, kalman wrote:
> > > And also - you can avoid |targetId| if you nest things even more:
> > > 
> > > chrome.tabs.create({url: viewTabUrl}, function(tab) {
> > >   chrome.tabs.onUpdated.addListener(function listener() {
> > >     /* use tab.id rather than targetId */
> > >   }
> > > });
> > > 
> > > More nesting, but fewer variables escaping out of their necessary scope.
If
> > you
> > > went back to having that separate takeScreenshot() function it might help
> the
> > > nesting a little.
> > 
> > Sadly, adding the listener chrome.tabs.onUpdated after chrome.tabs.create
will
> > always fail the first time we load the extension whereas it works fine
after.
> > I'm not sure why sadly ;(
> 
> Ah ok. Bummer. I can see why.

Do you mind sharing why this happens?

[fbeaufortchromium, 2014-08-26 06:57:42]

The CQ bit was checked by fbeaufort@chromium.org

[commit-bot: I haz the power, 2014-08-26 06:59:17]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/fbeaufort@chromium.org/490133002/40001

[commit-bot: I haz the power, 2014-08-26 09:37:23]

Committed patchset #3 (40001) as 127547c2490ae93dfba41e3801a82b2a8ff787e3

[not at google - send to devlin, 2014-08-26 19:40:12]

> Do you mind sharing why this happens?

I think - when the callback returns, the tab has been created. By the time the
event listener is added the tab may have already loaded?

[commit-bot: I haz the power, 2014-09-10 02:41:42]

Patchset 3 (id:??) landed as
https://crrev.com/29cb00fdc723f85c108cc9cdf3c66c98332ee009
Cr-Commit-Position: refs/heads/master@{#291874}