Functionality has been requested in the Extension API for Javascript to...

Functionality has been requested in the Extension API for Javascript to
take screenshots of the currently visible tab.  This changelist builds this
function, chrome.tabs.getVisibleScreenCapture.  This function takes a
single callback function and returns to that function a data URL of a JPEG
image of the current screen.  A simple sample extension is provided as a 
use case.

BUG=14760
TEST=There is an extension in chrome\test\data\extensions\samples\screenshot,
load this extension.  It creates a toolstrip button.  Click this button, you
should get a page with a screenshot of the active tab.  The API function
is found at chrome.tabs.getVisibleScreenCapture.

[adamhunter, 2009-06-23 00:29:23]

Hi Erik and Rafael, this is my screenshot checkin.  Please feel free to be
harsh.  Hope it's at least a good start.

[brettw, 2009-06-23 05:29:56]

I didn't look at this in detail, can you clarify some things for me?

I don't understand all of this plumbing. What does the screen capture stuff have
to do with the profile and all this other stuff? Can't you just get the current
backing store for the tab and return it?

(The backing store is off of the RenderWidgetHost which is the base class of
RenderViewHost which the TabContents has.)

Your function declarations need to be documented, especially in common global
classes like profile.

Use false instead of FALSE.

http://codereview.chromium.org/144019/diff/2014/2025
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/144019/diff/2014/2025#newcode641
Line 641: stream_as_string.reserve(jpeg_data->data.size());
Why reserve here?

http://codereview.chromium.org/144019/diff/2014/2025#newcode644
Line 644: jpeg_data->data.size());
Indent wrapped lines 4 spaces.

[Aaron Boodman, 2009-06-23 06:06:00]

Cool! I didn't realize someone was working on this.

http://codereview.chromium.org/144019/diff/2014/2025
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/144019/diff/2014/2025#newcode625
Line 625: bool GetVisibleScreenCaptureFunction::RunImpl() {
Naming this method RunImpl() is confusing because SyncFunction has a virtual
method also named that that all the sync functions implement.

http://codereview.chromium.org/144019/diff/2014/2025#newcode625
Line 625: bool GetVisibleScreenCaptureFunction::RunImpl() {
The structure of this class is confusing. It shouldn't contain a method called
RunImpl() because SyncFunction contains an abstract method named that which all
the synchronous functions implement, but this isn't.

It seems like this class can just have three methods Run(), Observe(), and
HandleResponse() with Run() and Observe() calling to HandleResponse().

http://codereview.chromium.org/144019/diff/2014/2025#newcode627
Line 627: if (screen_capture) {
Prefer to invert checks and return false early to reduce indentation.

http://codereview.chromium.org/144019/diff/2014/2039
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/144019/diff/2014/2039#newcode317
Line 317: chrome.tabs.getVisibleScreenCapture = function(callback) {
What region does the function capture? From the name it sounds like the entire
screen?

Should we stick this method on a new module, like chrome.capture, eg
chrome.capture.getScreen() ?

[Erik does not do reviews, 2009-06-23 17:03:44]

http://codereview.chromium.org/144019/diff/2014/2025
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/144019/diff/2014/2025#newcode626
Line 626: const SkBitmap *screen_capture = profile()->GetScreenCapture();
Hanging this off of profile seems wrong.  Why isn't it just passed along in the
notification itself?  Does it need to be cached for some reason?

http://codereview.chromium.org/144019/diff/2014/2025#newcode627
Line 627: if (screen_capture) {
so is the logic here if there isn't a screen capture that it should return true?
 that doesn't seem right.

http://codereview.chromium.org/144019/diff/2014/2025#newcode628
Line 628: RefCountedBytes* jpeg_data = new RefCountedBytes;
this is leaked.  wrap in a scoped_ptr.

http://codereview.chromium.org/144019/diff/2014/2025#newcode655
Line 655: profile()->InvalidateScreenCapture();
I don't understand this.  Why would you mark it invalid, but then proceed to
return it anyway?

http://codereview.chromium.org/144019/diff/2014/2025#newcode660
Line 660: Browser* browser = NULL;
combine these three lines into one

http://codereview.chromium.org/144019/diff/2014/2025#newcode669
Line 669: ->RequestVisibleScreenCapture();
rather than this weird line wrap, just add a local for TabContents or
RenderViewHost and split it into two statements.

http://codereview.chromium.org/144019/diff/2014/2025#newcode672
Line 672: NotificationService::AllSources());
If we have two browser windows taking captures at the same time, this will be
confusing and error-prone.  Since the notification is sent with the TabContents
as the source, why not just observe that?

http://codereview.chromium.org/144019/diff/2014/2025#newcode679
Line 679: const NotificationSource& source,
indent is wrong here

http://codereview.chromium.org/144019/diff/2014/2022
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/144019/diff/2014/2022#newcode91
Line 91: class GetVisibleScreenCaptureFunction : public AsyncExtensionFunction,
"screen capture" seems inaccurate.  It's really a tab capture.

http://codereview.chromium.org/144019/diff/2014/2022#newcode92
Line 92: public NotificationObserver {
indent is off here.  either 4 characters in from the left or aligned with the
public from the previous line.

http://codereview.chromium.org/144019/diff/2014/2027
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/144019/diff/2014/2027#newcode965
Line 965: screen_capture_ = bitmap;
how does this memory get freed?

http://codereview.chromium.org/144019/diff/2014/2027#newcode966
Line 966: contains_valid_screen_capture_ = TRUE;
use true/false rather than TRUE/FALSE

http://codereview.chromium.org/144019/diff/2014/2021
File chrome/browser/profile.h (right):

http://codereview.chromium.org/144019/diff/2014/2021#newcode197
Line 197: virtual bool ContainsValidScreenCapture() = 0;
I think we should get all of these out of profile.  However, any method
definition in a header file should have a comment above it unless it's an
implementation of another already-defined interface.

http://codereview.chromium.org/144019/diff/2014/2021#newcode464
Line 464: bool contains_valid_screen_capture_;
again, I think these shouldn't be here, but for future reference, they would
need to be above the DISALLOW_ macro, and would need to have comments.  It's
also quite likely that you'd want a scoped_ptr for screen_capture_.

http://codereview.chromium.org/144019/diff/2014/2031
File chrome/browser/renderer_host/render_view_host.cc (right):

http://codereview.chromium.org/144019/diff/2014/2031#newcode1015
Line 1015: const SkBitmap& bitmap) {
indent

http://codereview.chromium.org/144019/diff/2014/2032
File chrome/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/144019/diff/2014/2032#newcode440
Line 440: void RequestVisibleScreenCapture();
comment

http://codereview.chromium.org/144019/diff/2014/2030
File chrome/browser/renderer_host/render_view_host_delegate.h (right):

http://codereview.chromium.org/144019/diff/2014/2030#newcode212
Line 212: // This bitmap should be updated with the visible contents of the
screen.
not of the screen, but of the RenderView.

http://codereview.chromium.org/144019/diff/2014/2028
File chrome/browser/tab_contents/tab_contents.cc (right):

http://codereview.chromium.org/144019/diff/2014/2028#newcode1661
Line 1661: const SkBitmap& bitmap) {
indent

http://codereview.chromium.org/144019/diff/2014/2028#newcode1662
Line 1662: profile()->StoreScreenCapture(&bitmap);
This is really bad.  You're passing a pointer to a stack object which is then
being stored in a heap object.  You're just getting lucky in that the
notification is being delivered synchronously.  More reason to pass the bitmap
in the notification rather than trying to store it in the profile.

http://codereview.chromium.org/144019/diff/2014/2028#newcode1666
Line 1666: NotificationService::NoDetails());
Again, this is where you could pass the bitmap as the details of this
notification.

http://codereview.chromium.org/144019/diff/2014/2038
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/144019/diff/2014/2038#newcode469
Line 469: url, visible_screen));
indent 4 chars

http://codereview.chromium.org/144019/diff/2014/2038#newcode638
Line 638: bool RenderView::CaptureVisibleScreen(WebFrame* frame,
OK, so this is the biggest disconnect I have with this patch.  I thought the
whole reason you were doing this in the renderer process was that you needed
more than just the visible part of the view (at least that's what we discussed
originally).  If you just need the visible portion, then you can do it
completely in the browser process using the backing store cache.

http://codereview.chromium.org/144019/diff/2014/2038#newcode641
Line 641: double begin = time_util::GetHighResolutionTimeNow();
Rather than putting this timer code in an #ifdef, use base/histogram.h,
HISTOGRAM_TIMES.

http://codereview.chromium.org/144019/diff/2014/2038#newcode651
Line 651: if (!frame_view)
Do this before the call to CaptureFrame.

http://codereview.chromium.org/144019/diff/2014/2038#newcode659
Line 659: frame_view->GetSize().width, frame_view->GetSize().height);
4 char indent

http://codereview.chromium.org/144019/diff/2014/2038#newcode666
Line 666: OutputDebugStringA(buf);
irrelevant once you switch to histogram, but this function is windows-only and
shouldn't be used.  see base/logging.h for general logging.

http://codereview.chromium.org/144019/diff/2014/2036
File chrome/renderer/renderer_resources.grd (right):

http://codereview.chromium.org/144019/diff/2014/2036#newcode25
Line 25: 
the place to make these changes is in the comment at the top.  just add a
character inside the comment.

http://codereview.chromium.org/144019/diff/2014/2016
File chrome/test/data/extensions/samples/screenshot/functions.js (right):

http://codereview.chromium.org/144019/diff/2014/2016#newcode1
Line 1: //onclick="foo = chrome.tabs.getVisibleScreenCapture();
contentWindow.document.write('Hello, world!<p><IMG NAME=ss>');
contentWindow.document.images.ss = foo;"
removed unused code

http://codereview.chromium.org/144019/diff/2014/2016#newcode10
Line 10: console.log('hi2');
use similar coding style (indents, {}, etc. to C++) throughout this file

http://codereview.chromium.org/144019/diff/2014/2016#newcode10
Line 10: console.log('hi2');
remove unnecessary console.log calls (multiple in the file)

http://codereview.chromium.org/144019/diff/2014/2016#newcode36
Line 36: //	while(!child_loaded) ;
remove unused commented out code (multiple in the file)

http://codereview.chromium.org/144019/diff/2014/2018
File chrome/test/data/extensions/samples/screenshot/my_toolstrip.html (right):

http://codereview.chromium.org/144019/diff/2014/2018#newcode7
Line 7: <span>Hi2, World!</span>
A better label would be good.

[Aaron Boodman, 2009-06-23 17:37:16]

Stepping back from the code, thinking about design issues:

* We are returning JPEG data, but the user doesn't get to control the
compression level. Maybe there should be an option for this?

* Should we have an option to control the target size of the capture? Without
one, developers could use canvas, I think, to scale it down, so maybe this is
unnecessary. The only value I can imagine is that we may already have
specialized logic in Chrome for scaling tabs down very small (to thumbnails). I
remember at one time we were toying with the idea of trying to emphasize certain
features when thumbnail-ifying. But I'm not sure if this ever happened.

[adamhunter, 2009-06-23 18:28:55]

Wanted to send these out quickly because they are structural.

http://codereview.chromium.org/144019/diff/2014/2022
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/144019/diff/2014/2022#newcode91
Line 91: class GetVisibleScreenCaptureFunction : public AsyncExtensionFunction,
On 2009/06/23 17:03:44, Erik Kay wrote:
> "screen capture" seems inaccurate.  It's really a tab capture.

Makes sense.  GetVisibleTabCaptureFunction?

http://codereview.chromium.org/144019/diff/2014/2038
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/144019/diff/2014/2038#newcode638
Line 638: bool RenderView::CaptureVisibleScreen(WebFrame* frame,
It is true that the requirements shifted to be only the visible portion.  Now
that I have written the code to do the screenshot this way, is it worth
changing?  It would seem the advantage to having it this way is that a) it does
not rely on the backing store mechanism and b) if, in the future, we want more
than the visible region, it will be easier to make that shift.  Also, at one
point Rafael mentioned that he wasn't sure how long the backing store stayed
around or if it was always available.

http://codereview.chromium.org/144019/diff/2014/2039
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/144019/diff/2014/2039#newcode317
Line 317: chrome.tabs.getVisibleScreenCapture = function(callback) {
Because I'm new, I was trying to keep things simple, but if you'd like I can
create a new top-level chrome.capture.  It does capture only the current tab,
would it be better to call it something that reflects that?

[brettw, 2009-06-23 18:35:12]

http://codereview.chromium.org/144019/diff/2014/2038
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/144019/diff/2014/2038#newcode638
Line 638: bool RenderView::CaptureVisibleScreen(WebFrame* frame,
On 2009/06/23 18:28:55, adamhunter wrote:
> It is true that the requirements shifted to be only the visible portion.  Now
> that I have written the code to do the screenshot this way, is it worth
> changing?

Very much so. I don't want improperly designed (not your fault) unused code in
our codebase.

[Erik does not do reviews, 2009-06-23 18:38:08]

On 2009/06/23 18:28:55, adamhunter wrote:
> Wanted to send these out quickly because they are structural.
> 
> http://codereview.chromium.org/144019/diff/2014/2022
> File chrome/browser/extensions/extension_tabs_module.h (right):
> 
> http://codereview.chromium.org/144019/diff/2014/2022#newcode91
> Line 91: class GetVisibleScreenCaptureFunction : public
AsyncExtensionFunction,
> On 2009/06/23 17:03:44, Erik Kay wrote:
> > "screen capture" seems inaccurate.  It's really a tab capture.
> 
> Makes sense.  GetVisibleTabCaptureFunction?

Sure.


> http://codereview.chromium.org/144019/diff/2014/2038
> File chrome/renderer/render_view.cc (right):
> 
> http://codereview.chromium.org/144019/diff/2014/2038#newcode638
> Line 638: bool RenderView::CaptureVisibleScreen(WebFrame* frame,
> It is true that the requirements shifted to be only the visible portion.  Now
> that I have written the code to do the screenshot this way, is it worth
> changing?  It would seem the advantage to having it this way is that a) it
does
> not rely on the backing store mechanism and b) if, in the future, we want more
> than the visible region, it will be easier to make that shift.  Also, at one
> point Rafael mentioned that he wasn't sure how long the backing store stayed
> around or if it was always available.

For the active tab, the backing store is always available.  I agree with Brett
that we should change it to stay in browser.


> http://codereview.chromium.org/144019/diff/2014/2039
> File chrome/renderer/resources/extension_process_bindings.js (right):
> 
> http://codereview.chromium.org/144019/diff/2014/2039#newcode317
> Line 317: chrome.tabs.getVisibleScreenCapture = function(callback) {
> Because I'm new, I was trying to keep things simple, but if you'd like I can
> create a new top-level chrome.capture.  It does capture only the current tab,
> would it be better to call it something that reflects that?

Since it's tied to a tab, personally I prefer it being in the tab API directly.

[adamhunter, 2009-06-23 18:38:15]

http://codereview.chromium.org/144019/diff/2014/2038
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/144019/diff/2014/2038#newcode638
Line 638: bool RenderView::CaptureVisibleScreen(WebFrame* frame,
On 2009/06/23 18:35:12, brettw wrote:
> On 2009/06/23 18:28:55, adamhunter wrote:
> > It is true that the requirements shifted to be only the visible portion. 
Now
> > that I have written the code to do the screenshot this way, is it worth
> > changing?
> 
> Very much so. I don't want improperly designed (not your fault) unused code in
> our codebase.

You didn't respond to my implied question about the backing store.  Is it always
available and up to date?

[adamhunter, 2009-06-23 18:46:36]

On 2009/06/23 18:38:15, adamhunter wrote:
> http://codereview.chromium.org/144019/diff/2014/2038
> File chrome/renderer/render_view.cc (right):
> 
> http://codereview.chromium.org/144019/diff/2014/2038#newcode638
> Line 638: bool RenderView::CaptureVisibleScreen(WebFrame* frame,
> On 2009/06/23 18:35:12, brettw wrote:
> > On 2009/06/23 18:28:55, adamhunter wrote:
> > > It is true that the requirements shifted to be only the visible portion. 
> Now
> > > that I have written the code to do the screenshot this way, is it worth
> > > changing?
> > 
> > Very much so. I don't want improperly designed (not your fault) unused code
in
> > our codebase.
> 
> You didn't respond to my implied question about the backing store.  Is it
always
> available and up to date?


Sorry I just saw Erik's note.  OK, so in BackingStore* GetBackingStore(bool
force_create);, the comment leads to two questions: do i want to force_create? 
It seems the answer is yes.  Also, under what conditions does it return NULL? 
Is it just if we run out of memory?  Or are there times when the backing store
doesn't exist?  If the latter, are these anything I need to worry about?  What
do I return when GetBackingStore returns NULL?

[adamhunter, 2009-06-23 23:04:20]

I have now rewritten this change structurally to use the backing store, and also
tried to incorporate the stylistic suggestions.  The change is substantially
different.  Thank you very much for your help.  Let me know what else needs to
be done.  One unfortunate consequence is that, because the backing store is
platform specific, the first pass of this change is as well.  Also, I just
realized that I missed Aaron's good suggestion about configurable JPEG, I'd like
to put that in but I didn't want to hold up the review for that.

[brettw, 2009-06-23 23:26:41]

http://codereview.chromium.org/144019/diff/2081/2091
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/144019/diff/2081/2091#newcode651
Line 651: // Get the bitmap out of the canvas and resample it.
This comment is incorrect.

http://codereview.chromium.org/144019/diff/2081/2091#newcode653
Line 653: bmp.copyTo(&screen_capture, SkBitmap::kARGB_8888_Config);
Why do you need this copy?

http://codereview.chromium.org/144019/diff/2081/2091#newcode655
Line 655: UNREFERENCED_PARAMETER(backing_store);
Use NOTIMPLEMENTED so other platforms know this isn't done.

http://codereview.chromium.org/144019/diff/2081/2091#newcode671
Line 671: memcpy(&stream_as_string[0], (const char *)&jpeg_data->data[0],
Don't use C-style casts. Remove the space before the *.

http://codereview.chromium.org/144019/diff/2081/2087
File chrome/test/testing_profile.h (right):

http://codereview.chromium.org/144019/diff/2081/2087#newcode129
Line 129: virtual bool ContainsValidScreenCapture() {
I think you need to delete all of these.

[brettw, 2009-06-23 23:27:04]

> http://codereview.chromium.org/144019/diff/2081/2091#newcode655
> Line 655: UNREFERENCED_PARAMETER(backing_store);
> Use NOTIMPLEMENTED so other platforms know this isn't done.

In addition to UNREFERENCED_PARAMETER, I mean.

[Erik does not do reviews, 2009-06-23 23:27:27]

http://codereview.chromium.org/144019/diff/2081/2091
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/144019/diff/2081/2091#newcode7
Line 7: #include "base/file_util.h"
is this being used?

http://codereview.chromium.org/144019/diff/2081/2091#newcode9
Line 9: #include "base/gfx/png_encoder.h"
is this being used?

http://codereview.chromium.org/144019/diff/2081/2091#newcode11
Line 11: #include "base/thread.h"
is this being used?

http://codereview.chromium.org/144019/diff/2081/2091#newcode655
Line 655: UNREFERENCED_PARAMETER(backing_store);
add NOTREACHED() and move the TODO from above into this section.

http://codereview.chromium.org/144019/diff/2081/2093
File chrome/renderer/renderer_resources.grd (right):

http://codereview.chromium.org/144019/diff/2081/2093#newcode24
Line 24: </grit>
I think this might be missing a trailing newline

http://codereview.chromium.org/144019/diff/2081/2083
File chrome/test/data/extensions/samples/screenshot/functions.js (right):

http://codereview.chromium.org/144019/diff/2081/2083#newcode8
Line 8: button = document.getElementById("button");
clean up spacing, indent and {} position in this file

http://codereview.chromium.org/144019/diff/2081/2083#newcode29
Line 29: chrome.tabs.getVisibleTabCapture(function(img) { try{ have_screenshot =
true; screenshot_url = img; } catch(e){for(var f in e){console.log(f + ":" +
e[f]);}}});
reformat this function.  newlines are perfectly fine in the middle of a nested
function.

[rafaelw, 2009-06-24 22:02:31]

http://codereview.chromium.org/144019/diff/2101/2111
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/144019/diff/2101/2111#newcode628
Line 628: bool GetVisibleTabCaptureFunction::RunImpl() {
It seems like this function needs to take a windowId so that it's clear what
browser is returning bits for it's selected tab. It could default to the
"current" window as other tab functions do.

http://codereview.chromium.org/144019/diff/2101/2111#newcode628
Line 628: bool GetVisibleTabCaptureFunction::RunImpl() {
I'm not sure if 'Visible' here refers to the Tab that's targeted, or that it
only returns visible bits from the current scroll region. However the naming
gets sorted out, hopefully it'll be clear & consistent with respect to the
entities in play here (window, tab) and the behavior (visible vs. full
scrollable area).

http://codereview.chromium.org/144019/diff/2101/2111#newcode638
Line 638: // TODO(adamhunter): Write this for other platforms.
you may (also) want 'TODO(port): ' here for consistency.

http://codereview.chromium.org/144019/diff/2101/2111#newcode644
Line 644: return false;
set an error here.

http://codereview.chromium.org/144019/diff/2101/2111#newcode664
Line 664: return false;
set an error here.

http://codereview.chromium.org/144019/diff/2101/2111#newcode672
Line 672: net::Base64Encode(stream_as_string, &base64_result);
is there any danger of hitting a data url size limit here?

[adamhunter, 2009-06-25 18:48:57]

More changes, hopefully things are starting to look good.  Let me know what I've
missed.

http://codereview.chromium.org/144019/diff/2081/2091
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/144019/diff/2081/2091#newcode653
Line 653: bmp.copyTo(&screen_capture, SkBitmap::kARGB_8888_Config);
On 2009/06/23 23:26:42, brettw wrote:
> Why do you need this copy?

At one point when I didn't have it in there I was getting memory errors (but
that was many changes ago).  Am I guaranteed to be able to access that
SkBitmap's memory for the duration I need it?

http://codereview.chromium.org/144019/diff/2101/2111#newcode672
Line 672: jpeg_data->data.size());
On 2009/06/24 22:02:32, rafaelw wrote:
> is there any danger of hitting a data url size limit here?

Does anyone on this list know?  I tried to poke around but couldn't find one. 
I've tested with some very large (2500x2000) pixel images and it's been fine,
but I don't know if there is some limit.  (Remember that since this is Chrome
internal it doesn't matter if, say, another browser has a limit).

[brettw, 2009-06-25 19:23:11]

http://codereview.chromium.org/144019/diff/2081/2091
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/144019/diff/2081/2091#newcode653
Line 653: bmp.copyTo(&screen_capture, SkBitmap::kARGB_8888_Config);
On 2009/06/25 18:48:59, adamhunter wrote:
> On 2009/06/23 23:26:42, brettw wrote:
> > Why do you need this copy?
> 
> At one point when I didn't have it in there I was getting memory errors (but
> that was many changes ago).  Am I guaranteed to be able to access that
> SkBitmap's memory for the duration I need it?

Yes. You can access it here, so you can certainly access it a few lines down as
long as you don't modify the canvas that owns it.

http://codereview.chromium.org/144019/diff/4005/3087#newcode666
Line 666: return false;
brace posbiitoning.

http://codereview.chromium.org/144019/diff/4005/3084
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/144019/diff/4005/3084#newcode11
Line 11: #include "chrome/common/notification_registrar.h"
Why did you add this?

http://codereview.chromium.org/144019/diff/4005/3080
File chrome/test/data/extensions/samples/screenshot/functions.js (right):

http://codereview.chromium.org/144019/diff/4005/3080#newcode1
Line 1: var button = null;
I'm not a JS expert, but I can't imagine that this file has the correct
formatting.

[Aaron Boodman, 2009-06-30 21:29:17]

http://codereview.chromium.org/144019/diff/4005/3080
File chrome/test/data/extensions/samples/screenshot/functions.js (right):

http://codereview.chromium.org/144019/diff/4005/3080#newcode1
Line 1: var button = null;
On 2009/06/25 19:23:11, brettw wrote:
> I'm not a JS expert, but I can't imagine that this file has the correct
> formatting.

It doesn't. First of all it needs the standard Chromium file header. Also,
indent is two spaces, the braces should be 1 space after the parens, "else"
should be on same line as brace from "if", same with "catch" and "try".

[shinyobject, 2009-07-06 20:32:13]

Hi Aaron and Brett, I've made an effort to address your concerns, let me know if
there's any other concerns you might have.  Thanks!

[adamhunter, 2009-07-06 20:35:34]

My apologies, apparently I was logged in with my personal account at some point.

[Erik does not do reviews, 2009-07-06 20:51:06]

I have one small comment (the optional function parameter), the rest are style
nits.

http://codereview.chromium.org/144019/diff/5001/6009
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/144019/diff/5001/6009#newcode641
Line 641: Extension::kInternalVisibleTabCaptureError, "");
nit: 4 char indent

http://codereview.chromium.org/144019/diff/5001/6009#newcode665
Line 665: Extension::kInternalVisibleTabCaptureError, "");
nit: indent should be 4 characters from previous line

http://codereview.chromium.org/144019/diff/5001/6013
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/144019/diff/5001/6013#newcode323
Line 323: chrome.types.optFun
This should probably not be optional since it does nothing productive without
the function.

http://codereview.chromium.org/144019/diff/5001/6002
File chrome/test/data/extensions/samples/screenshot/functions.js (right):

http://codereview.chromium.org/144019/diff/5001/6002#newcode8
Line 8: button = document.getElementById("button");
spacing is still off throughout this file.  use 2 char indents.

http://codereview.chromium.org/144019/diff/5001/6002#newcode9
Line 9: button.addEventListener("click",handleClick);
space after ,

http://codereview.chromium.org/144019/diff/5001/6002#newcode18
Line 18: else {
combine with previous line

http://codereview.chromium.org/144019/diff/5001/6002#newcode19
Line 19: window.setTimeout("waitForChildLoaded()", 50);
use the function reference instead of a string:
window.setTimeout(waitforChildLoaded, 50);

http://codereview.chromium.org/144019/diff/5001/6002#newcode27
Line 27: try {
indent under the function (do you possibly have a mix of tabs and spaces here? 
it looks really odd.)

http://codereview.chromium.org/144019/diff/5001/6002#newcode31
Line 31: catch(e) {
combine with previous line

http://codereview.chromium.org/144019/diff/5001/6002#newcode38
Line 38: window.setTimeout("waitForChildLoaded()", 50);
use function reference instead of string

[adamhunter, 2009-07-07 21:48:15]

All done.

[adamhunter, 2009-07-07 21:50:46]

On 2009/07/07 21:48:15, adamhunter wrote:
> All done.

Looks like i have merge errors, i'll fix them up.

[Erik does not do reviews, 2009-07-09 21:47:47]

LGTM - so now brettw, aa, rafaelw?

[Aaron Boodman, 2009-07-10 00:56:48]

http://codereview.chromium.org/144019/diff/4030/7016
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/144019/diff/4030/7016#newcode317
Line 317: chrome.tabs.getVisibleTabCapture = function(callback) {
api nitpickery: Is it possible to capture a not-visible tab? Because if it is,
my preference would be for capture(tabid, callback).

If not, then how about captureSelected() ?

[adamhunter, 2009-07-10 01:11:22]

http://codereview.chromium.org/144019/diff/4005/3080
File chrome/test/data/extensions/samples/screenshot/functions.js (right):

http://codereview.chromium.org/144019/diff/4005/3080#newcode1
Line 1: var button = null;
On 2009/06/25 19:23:11, brettw wrote:
> I'm not a JS expert, but I can't imagine that this file has the correct
> formatting.

I tried to find a solid JS style guide and failed.  Can you help me and point me
towards one?

http://codereview.chromium.org/144019/diff/4030/7016
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/144019/diff/4030/7016#newcode317
Line 317: chrome.tabs.getVisibleTabCapture = function(callback) {
You can't get a non-visible tab, no.  I was trying to emphasize that it captured
the Visible region, not the whole tab.  And I wanted to name it the same as the
underlying native implementation (where the word "Tab" is important to identify
that it works on tabs).

[rafaelw, 2009-07-10 01:14:36]

http://codereview.chromium.org/144019/diff/4030/7012
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/144019/diff/4030/7012#newcode625
Line 625: bool GetVisibleTabCaptureFunction::RunImpl() {
I didn't see a reply to my earlier suggestion to have getVisibleTab() take an
optional windowId parameter. Simply grabbing the browser from the dispatcher
here may result in the "right" thing if called from a toolstrip (it'll return
the "host" browser), but if called from a background_page, it'll return the
LastActive browser, which may or may not be the right thing.

http://codereview.chromium.org/144019/diff/4030/7018
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/144019/diff/4030/7018#newcode133
Line 133: static const char* kInternalVisibleTabCaptureError;
The rest of these errors here are related to extension loading. This error
belongs with in the extension_tabs_module_constants.h

[darin (slow to review), 2009-07-14 05:34:43]

Apologies for going back in time....  but, I couldn't find where these
questions were addressed.  It seems to me that it may also be useful to give
people the ability to request a PNG instead of a JPEG in case they wish to
get a lossless screen capture.  I like the idea of the API allowing the
consumer to specify the requested image size.  It may also make sense for
their to be a scale quality parameter.  Chrome can do high quality image
scaling but that is computationally expensive compared to basic scaling.
-Darin


On Tue, Jun 23, 2009 at 10:37 AM, <aa@chromium.org> wrote:

> Stepping back from the code, thinking about design issues:
>
> * We are returning JPEG data, but the user doesn't get to control the
> compression level. Maybe there should be an option for this?
>
> * Should we have an option to control the target size of the capture?
> Without one, developers could use canvas, I think, to scale it down, so
> maybe this is unnecessary. The only value I can imagine is that we may
> already have specialized logic in Chrome for scaling tabs down very
> small (to thumbnails). I remember at one time we were toying with the
> idea of trying to emphasize certain features when thumbnail-ifying. But
> I'm not sure if this ever happened.
>
>
> http://codereview.chromium.org/144019
>

[Aaron Boodman, 2009-07-14 17:23:22]

Here was the answer:

<snip>
Also, I just realized that I missed Aaron's good
suggestion about configurable JPEG, I'd like to put that in but I didn't
want to hold up the review for that.
</snip>

Let's get a first cut in. But I do agree that:

* We should export PNG instead of JPEG if possible.
* People should be able to configure sizes of capture.

- a

On Mon, Jul 13, 2009 at 10:34 PM, Darin Fisher<darin@chromium.org> wrote:
> Apologies for going back in time.... =A0but, I couldn't find where these
> questions were addressed. =A0It seems to me that it may also be useful to=
 give
> people the ability to request a PNG instead of a JPEG in case they wish t=
o
> get a lossless screen capture. =A0I like the idea of the API allowing the
> consumer to specify the requested image size. =A0It may also make sense f=
or
> their to be a scale quality parameter. =A0Chrome can do high quality imag=
e
> scaling but that is computationally expensive compared to basic scaling.
> -Darin
>
> On Tue, Jun 23, 2009 at 10:37 AM, <aa@chromium.org> wrote:
>>
>> Stepping back from the code, thinking about design issues:
>>
>> * We are returning JPEG data, but the user doesn't get to control the
>> compression level. Maybe there should be an option for this?
>>
>> * Should we have an option to control the target size of the capture?
>> Without one, developers could use canvas, I think, to scale it down, so
>> maybe this is unnecessary. The only value I can imagine is that we may
>> already have specialized logic in Chrome for scaling tabs down very
>> small (to thumbnails). I remember at one time we were toying with the
>> idea of trying to emphasize certain features when thumbnail-ifying. But
>> I'm not sure if this ever happened.
>>
>> http://codereview.chromium.org/144019
>
>

[adamhunter, 2009-07-14 20:50:00]

All done.

[rafaelw, 2009-07-14 20:53:10]

LGTM. Really nice work on this.

[Mohamed Mansour, 2009-07-27 00:43:13]

Sorry for dropping by so late, would it be "better" if we do something like:
 chrome://image/http://www.google.com

To get the screenshot of a webpage rather than coupling this for chromium
extensions? Similar to:
 chrome://thumb/http://www.google.com

The reason why I am saying this is because for print preview and settings tab,
we would want to get the screenshot of the tab so we can  manipulate it for
printing. We could reuse same functionality for both areas if we implement an
"image" chrome url which could be used by anyone (extension dev or domui dev)

[erikkay, 2009-07-27 17:25:07]

On Sun, Jul 26, 2009 at 5:43 PM, <mhm@chromium.org> wrote:

> Sorry for dropping by so late, would it be "better" if we do something
> like:
>  chrome://image/http://www.google.com
>
> To get the screenshot of a webpage rather than coupling this for
> chromium extensions? Similar to:
>  chrome://thumb/http://www.google.com
>
> The reason why I am saying this is because for print preview and
> settings tab, we would want to get the screenshot of the tab so we can
> manipulate it for printing. We could reuse same functionality for both
> areas if we implement an "image" chrome url which could be used by
> anyone (extension dev or domui dev)
>
> http://codereview.chromium.org/144019
>

The way this code is currently written, it just grabs the backing store from
the current visible tab.  This means that it's the same size as the viewport
on the visible tab, no new layout happens.

Given this, I don't think this could be made to work with printing.  The two
obvious problems are the my browser width might not match paper width, and
that anything that's outside of the viewport (scrolled) won't be visible in
the preview.

If you wind up building a more generalized "render the specified page with
its current resources to a bitmap constrained to the following dimensions"
for print preview, I'm sure that would be useful in other situations as
well.

Erik

[Aaron Boodman, 2009-07-30 18:15:12]

LGTM, with a few nits.

http://codereview.chromium.org/144019/diff/7116/4106
File chrome/browser/extensions/extension_tabs_module.cc (right):

http://codereview.chromium.org/144019/diff/7116/4106#newcode662
Line 662: // TODO(adamhunter): TODO(port): Write this for other platforms.
What about the other platforms? What is the timeline for implementation? Do we
know how to do it?

http://codereview.chromium.org/144019/diff/7116/4106#newcode663
Line 663: NOTIMPLEMENTED();
Also, I forget what the behavior of NOTIMPLEMENTED() is in Release. We should
not crash. I would prefer to just return an error in platforms we don't support.

http://codereview.chromium.org/144019/diff/7116/4111
File chrome/common/extensions/extension.h (right):

http://codereview.chromium.org/144019/diff/7116/4111#newcode56
Line 56: 
remove no-op change.

http://codereview.chromium.org/144019/diff/7116/4110
File chrome/renderer/resources/extension_process_bindings.js (right):

http://codereview.chromium.org/144019/diff/7116/4110#newcode322
Line 322: chrome.tabs.getVisibleTabCapture = function(windowId, callback) {
Shouldn't this be in api.json now?

http://codereview.chromium.org/144019/diff/7116/4110#newcode322
Line 322: chrome.tabs.getVisibleTabCapture = function(windowId, callback) {
Can we change the name to captureVisibleTab()?