Chromium

[Empty comment from Monorail migration]

I have created a patch to allow to specify the format of the screenshot in second
argument of captureVisibleTab.
This patch adds an extra argument to specify the image format(and quality of jpeg)
that breaks API compatibility.
But I think supporting PNG is worth enought to change it. According to HTML5 canvas
element
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-canvas-element.html#dom-canvas-todataurl,
toDataURL reuqires supporting image/png. So I think it's preferable captureVisibleTab
supports PNG too.

Please consider this patch and changing captureVisibleTab API spec.

Thank you for the patch. I wonder if we should just change the format to always be PNG
instead of JPEG. I will try to apply this soon.

Thanks for your comment. I think it is reasonable that we change captureVisibleTab
signature to keep image format flexibility and extendability. If we add second
argument, it could contain a parameter for specifying the section to capture that is
needed for http://code.google.com/p/chromium/issues/detail?id=21375 and any other
expansions needed in future. So I hope change current signature and add second
argument for specifing various parameters.

In https://crbug.com/chromium/21072#c3, aa@chromium.org said:
> I wonder if we should just change the format to always be PNG
> instead of JPEG.

I think it is worth having both.  If an extension is taking screen shots regularly
and uploading them, low quality JPEGs could be a better choice than PNGs.  I know of
a firefox extension that wished to do this*.  The images would be used to figure out
the last time the page visibly changed.  

* http://code.google.com/p/page-speed

Sam

Great example Sam. I agree with your take.

[Empty comment from Monorail migration]

[Empty comment from Monorail migration]

[Empty comment from Monorail migration]

[Empty comment from Monorail migration]

Per discussion with rafaelw, the new signature for captureVisibleTab should be:

chrome.tabs.captureVisibleTab(optional windowId, optional object options, function callback);

To implement this without breaking existing users, we need to allow the options object to be omitted.  This will
require fixing https://crbug.com/chromium/21370.

Sam

I've created a bug for the required change to extend this function in a non-breaking
way: crbug.com/29215.

[Empty comment from Monorail migration]

I notice this keeps being pushed back even though a patch has been submitted for it...
Is that because it's blocked on https://crbug.com/chromium/29215? (Note: Blocked on refers to a merged issue
which makes it look like a fixed issue)

Could something temporary be put in place incorporating the patch, and a real fix come
when 29215 is fixed?

The provided patch is a breaking change. I'm not opposed to the patch, we just can't
break the API in the process.

The patch needs to be updated to understand the old signature. This doesn't
necessarily need to block on 21370 -- I'm happy to do a few one-offs before we get
the hang of a general solution the method overloading problem.

I've removed the mstone-x, which really means "patches for this likely won't be
accepted without further design or discussion"

[Empty comment from Monorail migration]

[Empty comment from Monorail migration]

[Empty comment from Monorail migration]

It seems to be difficult to add extra argument beacause current jsonschema
implementation could not handle multiple versions of signatures which differ in
numbers of arguments without spoiling argument validation. So in this patch, change
the signature to (choices [integer windowId, object params], function callback).

This new signature could be validated with current jsonschema implementation and
even if https://crbug.com/chromium/29215 was resolved in future and there is need to change the signature
to more intuitive format (optional integer windowId, optional object params, optional
function callback), new signature could be recognized as first argument is omitted
and sencond params is specified. that means there is no breaking change is required.

Please review and consider appling new patch.

[Comment Deleted]

[Comment Deleted]

Can we implement a captureTab function and deprecate captureVisibleTab?  The function
should ideally be capable of capturing any rectangle on at least the current tab in any
format specified.  Specifying a rectangle will reduce the need for compression and
resolve this issue leaving us with a more extendable function.  This should reduce the
time needed to get this functionality included, and the existing function could later
be implemented using the new one.  

[Empty comment from Monorail migration]

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=43985 

------------------------------------------------------------------------
r43985 | skerner@chromium.org | 2010-04-08 12:06:15 -0700 (Thu, 08 Apr 2010) | 6 lines
Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/base/values.h?r1=43985&r2=43984
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_function.cc?r1=43985&r2=43984
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_function.h?r1=43985&r2=43984
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_tabs_module.cc?r1=43985&r2=43984
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_tabs_module.h?r1=43985&r2=43984
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_tabs_module_constants.cc?r1=43985&r2=43984
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/extensions/extension_tabs_module_constants.h?r1=43985&r2=43984
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/api/extension_api.json?r1=43985&r2=43984
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/docs/tabs.html?r1=43985&r2=43984
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/renderer/extensions/extension_api_client_unittest.cc?r1=43985&r2=43984
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/renderer/resources/extension_process_bindings.js?r1=43985&r2=43984

Support PNG and quality control in chrome.tabs.captureVisibleTab().

BUG=21072
TEST=ExtensionAPIClientTest.CaptureVisibleTab

Review URL: http://codereview.chromium.org/1527015
------------------------------------------------------------------------

[Empty comment from Monorail migration]

I opened https://crbug.com/chromium/44758 regarding distortion/transparency within text when png format is
used.

This issue has been closed for some time. No one will pay attention to new comments.
If you are seeing this bug or have new data, please click New Issue to start a new bug.

[Empty comment from Monorail migration]

This issue was migrated from crbug.com/chromium/21072?no_tracker_redirect=1

[Multiple monorail components: Platform>Extensions, UI]
[Monorail mergedwith: crbug.com/chromium/32489]
[Monorail components added to Component Tags custom field.]