Re-land alexbost's experimental offscreenTabs API.

Re-land alexbost's experimental offscreenTabs API.

Original code review: http://codereview.chromium.org/7720002/
A followup code review: http://chromiumcodereview.appspot.com/9150052/

This includes some refactoring to simplify and reduce the code size:
 - sharing more code between tabs and offscreenTabs
 - splitting up and fixing the browser tests
 - forbidding use of the API from a background page

BUG=110833
TEST=OffscreenTabsApiTest.*

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

[jstritar, 2012-01-26 19:55:24]

I did a bit more refactoring. I have a couple things left to do (... think I
broke toDataURL on Mac).

http://codereview.chromium.org/9234042/diff/4051/chrome/browser/extensions/ex...
File chrome/browser/extensions/execute_script_apitest.cc (left):

http://codereview.chromium.org/9234042/diff/4051/chrome/browser/extensions/ex...
chrome/browser/extensions/execute_script_apitest.cc:23: #endif  //
defined(OS_CHROMEOS)
I re-enabled all these to see if they're still failing...

http://codereview.chromium.org/9234042/diff/4051/chrome/browser/extensions/ex...
chrome/browser/extensions/execute_script_apitest.cc:86: #endif
This CL fixes issue 89731.

[Mihai Parparita -not on Chrome, 2012-01-26 20:21:02]

http://codereview.chromium.org/9234042/diff/4051/chrome/browser/extensions/ex...
File chrome/browser/extensions/execute_script_apitest.cc (left):

http://codereview.chromium.org/9234042/diff/4051/chrome/browser/extensions/ex...
chrome/browser/extensions/execute_script_apitest.cc:86: #endif
On 2012/01/26 19:55:25, jstritar wrote:
> This CL fixes issue 89731.

Any chance this fix could be moved to a separate CL?

[Ken Russell (switch to Gerrit), 2012-01-27 01:27:50]

This looks good to me overall but I am not an owner. Nice code cleanups and
refactorings.

The browser_tests seem to still be failing with the current patch. LGTM so you
aren't blocked on me, but happy to look at subsequent versions too.

http://codereview.chromium.org/9234042/diff/4051/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc (right):

http://codereview.chromium.org/9234042/diff/4051/chrome/browser/extensions/ap...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:177: // May
cause the Tab object associated with the parent to be deleted.
Tab -> OffscreenTab?

http://codereview.chromium.org/9234042/diff/4051/chrome/browser/extensions/ex...
File chrome/browser/extensions/execute_script_apitest.cc (left):

http://codereview.chromium.org/9234042/diff/4051/chrome/browser/extensions/ex...
chrome/browser/extensions/execute_script_apitest.cc:86: #endif
On 2012/01/26 20:21:02, Mihai Parparita wrote:
> On 2012/01/26 19:55:25, jstritar wrote:
> > This CL fixes issue 89731.
> 
> Any chance this fix could be moved to a separate CL?

Agree it would be nicer if that could be split off. Also, I don't see where in
this CL that other bug is actually fixed. Could this be why the new
OffscreenTabs API tests are still failing in the most recent Mac and Linux try
jobs?

[jstritar, 2012-01-27 21:45:45]

+cc alexbost

The toDataUrl function doesn't appear to work on Mac -- it always returns black
images. This also happens when I don't use the backing store (there was a note
in the original CL about them not working on Mac).

Did you run into this before Alex? I guess we can just support Windows and Linux
while its experimental.

[Ken Russell (switch to Gerrit), 2012-01-27 21:48:59]

On 2012/01/27 21:45:45, jstritar wrote:
> +cc alexbost
> 
> The toDataUrl function doesn't appear to work on Mac -- it always returns
black
> images. This also happens when I don't use the backing store (there was a note
> in the original CL about them not working on Mac).
> 
> Did you run into this before Alex? I guess we can just support Windows and
Linux
> while its experimental.

I don't think that alexbost's email works any more. He only interned last
summer.

We definitely had this working on Mac OS but I think it might have required a
hack deep in WebKit that we didn't make a principled change. Let's just disable
any failing tests and iterate on it rather than trying to get everything working
the first time.

[jstritar, 2012-01-27 22:01:18]

I commented out the test that fails on Mac and disabled all the tests on
ChromeOS.

http://codereview.chromium.org/9234042/diff/4051/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc (right):

http://codereview.chromium.org/9234042/diff/4051/chrome/browser/extensions/ap...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:177: // May
cause the Tab object associated with the parent to be deleted.
On 2012/01/27 01:27:50, kbr wrote:
> Tab -> OffscreenTab?

Done.

http://codereview.chromium.org/9234042/diff/4051/chrome/browser/extensions/ex...
File chrome/browser/extensions/execute_script_apitest.cc (left):

http://codereview.chromium.org/9234042/diff/4051/chrome/browser/extensions/ex...
chrome/browser/extensions/execute_script_apitest.cc:86: #endif
I created a new CL for the fix, http://codereview.chromium.org/9225010/. I'll
need to merge that into this CL once it lands.

[svetlo_gmail.com, 2012-01-30 09:44:00]

Glad to hear work on the Offscreen Tabs API has been revived!


I think the problem is likely to be in the offscreen tab initialization.
One hacky thing that comes to mind: I relied on setting size of the view to
start the renderer, so I would look into what changed there. Your comment
about ChromeOS in OffscreenTab::SetSize points into the same direction.


Good luck!


On Fri, Jan 27, 2012 at 2:01 PM, <jstritar@chromium.org> wrote:

> I commented out the test that fails on Mac and disabled all the tests on
> ChromeOS.
>
>
> http://codereview.chromium.**org/9234042/diff/4051/chrome/**
>
browser/extensions/api/**offscreen_tabs/offscreen_tabs_**api.cc<http://codereview.chromium.org/9234042/diff/4051/chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc>
> File chrome/browser/extensions/api/**offscreen_tabs/offscreen_tabs_**
> api.cc
> (right):
>
> http://codereview.chromium.**org/9234042/diff/4051/chrome/**
>
browser/extensions/api/**offscreen_tabs/offscreen_tabs_**api.cc#newcode177<http://codereview.chromium.org/9234042/diff/4051/chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc#newcode177>
> chrome/browser/extensions/api/**offscreen_tabs/offscreen_tabs_**
> api.cc:177:
> // May cause the Tab object associated with the parent to be deleted.
> On 2012/01/27 01:27:50, kbr wrote:
>
>> Tab -> OffscreenTab?
>>
>
> Done.
>
> http://codereview.chromium.**org/9234042/diff/4051/chrome/**
>
browser/extensions/execute_**script_apitest.cc<http://codereview.chromium.org/9234042/diff/4051/chrome/browser/extensions/execute_script_apitest.cc>
> File chrome/browser/extensions/**execute_script_apitest.cc (left):
>
> http://codereview.chromium.**org/9234042/diff/4051/chrome/**
>
browser/extensions/execute_**script_apitest.cc#oldcode86<http://codereview.chromium.org/9234042/diff/4051/chrome/browser/extensions/execute_script_apitest.cc#oldcode86>
> chrome/browser/extensions/**execute_script_apitest.cc:86: #endif
> I created a new CL for the fix,
http://codereview.chromium.**org/9225010/<http://codereview.chromium.org/9225...
> .
> I'll need to merge that into this CL once it lands.
>
>
http://codereview.chromium.**org/9234042/<http://codereview.chromium.org/9234...
>

[asargent_no_longer_on_chrome, 2012-02-29 18:20:52]

Still working my way through the implementation of the API part of the review,
but here are some initial comments.

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.h (right):

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.h:21: // those
in tabs, there are few key differences that need to be resolved.
typo: "...there are few..." -> "...there are a few..."

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/e...
File chrome/browser/extensions/execute_script_apitest.cc (right):

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/e...
chrome/browser/extensions/execute_script_apitest.cc:19:
IN_PROC_BROWSER_TEST_F(ExecuteScriptApiTest, ExecuteScriptBasic) {
These changes look unrelated to offscreen tabs. Cruft from another CL or
something?

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.h:33: // can use them. We should
remove these once the APIs are combined.
Would it be appropriate for these to go in extension_tab_util.h?

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.offscreenTabs.json (right):

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.offscreenTabs.json:44: "description":
"The URL to navigate the offscreen tab to initially. Fully-qualified URLs must
include a scheme (i.e. 'http://www.google.com', not 'www.google.com'). Relative
URLs will be relative to the current page within the extension."
During the API review meeting we did for this many months ago, one of the
important points of discussion was whether the lifetime of the created offscreen
tab was scoped to the lifetime of the context of its caller (browser action
popup, etc.), or if we were going to rely on extension authors to remember to
call remove. 

I haven't looked at the implementation yet to see what the current behavior is,
but we should be sure to note in the description here something about that.

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.offscreenTabs.json:150: "type": "any",
Any reason the type here can't be "MouseEvent" instead of "any" to get the
benefits of type checking? Or maybe you want:

"type": "object",
"isInstanceOf": "MouseEvent"

(see some of the other .json files for examples)

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.offscreenTabs.json:174: "$ref":
"OffscreenTab",
It's unusual to be returning the OffscreenTab object here in the callback for
the sendMouseEvent function. I would have expected just the typical
success/failure information, and then the developer would rely on the onUpdated
event if they are interested in seeing changes in e.g. the url of an offscreen
tab.

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.offscreenTabs.json:194: "type": "any",
same question here about type

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.offscreenTabs.json:204: "$ref":
"OffscreenTab",
same thing here - weird to return the OffscreenTab to the callback for this
function

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.offscreenTabs.json:212: "name":
"toDataUrl",
Instead returning the tab contents as a data url, I wonder if we'd get better
performance returning the result as a Blob (or have an option for one or the
other). It might be worth looking into what jcivelli did for the
pageCapture.saveAsMHTML function. 

http://www.w3.org/TR/FileAPI/ defines Blobs.

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.offscreenTabs.json:297: "$ref":
"OffscreenTab",
again, weird to return OffscreenTab here

[Ken Russell (switch to Gerrit), 2012-02-29 18:36:06]

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.offscreenTabs.json (right):

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.offscreenTabs.json:212: "name":
"toDataUrl",
On 2012/02/29 18:20:52, Antony Sargent wrote:
> Instead returning the tab contents as a data url, I wonder if we'd get better
> performance returning the result as a Blob (or have an option for one or the
> other). It might be worth looking into what jcivelli did for the
> pageCapture.saveAsMHTML function. 
> 
> http://www.w3.org/TR/FileAPI/ defines Blobs.

The eventual goal is to avoid reading back the contents of the offscreen tab
altogether, and instead allow WebGL code in the extension using this API to
directly reference the GPU texture containing the page's rendered output. That's
the only way that high performance will be achieved. So it may not be worth
trying to further optimize this code path.

[asargent_no_longer_on_chrome, 2012-03-01 00:20:04]

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc (right):

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:192: //
TODO(jstritar): This isn't multi-profile aware.
lolwut? Do you just mean that this should be hung as a ProfileKeyedService or
something instead of using a LazyInstance?

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:203: // Note
that you can't create offscreen tabs from background pages, since they
Awesome, I like this restriction to help avoid leaks of offscreen tabs. Please
mention this in the documentation.

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:204: // don't
have an associated WebContents. The life time of offscreen tabs is tied
nit: "life time" -> "lifetime"

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:325:
STLDeleteContainerPairSecondPointers(map_.begin(), map_.end());
Optional suggestion: if you used linked_ptr<> instead of a raw pointer as the
value in the map, you get this for free. That would also let you get rid of the
RemoveParentTab method if favor of just calling erase directly.

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:508:
OffscreenTab* offscreen_tab = GetMap()->CreateOffscreenTab(
Optional suggestion: the fact that the map owns the OffscreenTab* (and hence we
aren't leaking it at the end of this function) could be more clear here if
CreateOffscreenTab returned a const * or const &.

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:651:
SendResponse(true);
Since this is a SyncExtensionFunction, you don't need to call SendResponse
manually right?

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:765:
SendResponse(true);
same thing here

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:836: 
nit: Can you include a comment here about where SendResponse is called in the
async case? I assume somewhere in our UpdateTabFunction parent class.

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.offscreenTabs.json (right):

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.offscreenTabs.json:212: "name":
"toDataUrl",
On 2012/02/29 18:36:07, kbr wrote:
> On 2012/02/29 18:20:52, Antony Sargent wrote:
> > Instead returning the tab contents as a data url, I wonder if we'd get
better
> > performance returning the result as a Blob (or have an option for one or the
> > other). It might be worth looking into what jcivelli did for the
> > pageCapture.saveAsMHTML function. 
> > 
> > http://www.w3.org/TR/FileAPI/ defines Blobs.
> 
> The eventual goal is to avoid reading back the contents of the offscreen tab
> altogether, and instead allow WebGL code in the extension using this API to
> directly reference the GPU texture containing the page's rendered output.
That's
> the only way that high performance will be achieved. So it may not be worth
> trying to further optimize this code path.
Ok, fine by me.

[jstritar, 2012-03-11 15:55:24]

Ah, thought I had another week until the branch point. I'll revise this ASAP.

[jstritar, 2012-03-12 14:53:06]

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc (right):

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:192: //
TODO(jstritar): This isn't multi-profile aware.
On 2012/03/01 00:20:05, Antony Sargent wrote:
> lolwut? Do you just mean that this should be hung as a ProfileKeyedService or
> something instead of using a LazyInstance?

Actually, there's no problem here. I was thinking we needed one per profile, but
we key off the tab ids which are unique.

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:203: // Note
that you can't create offscreen tabs from background pages, since they
On 2012/03/01 00:20:05, Antony Sargent wrote:
> Awesome, I like this restriction to help avoid leaks of offscreen tabs. Please
> mention this in the documentation.

I updated the create method documentation for now.

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:204: // don't
have an associated WebContents. The life time of offscreen tabs is tied
On 2012/03/01 00:20:05, Antony Sargent wrote:
> nit: "life time" -> "lifetime"

Done.

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:325:
STLDeleteContainerPairSecondPointers(map_.begin(), map_.end());
On 2012/03/01 00:20:05, Antony Sargent wrote:
> Optional suggestion: if you used linked_ptr<> instead of a raw pointer as the
> value in the map, you get this for free. That would also let you get rid of
the
> RemoveParentTab method if favor of just calling erase directly. 

Nice, done. I ended up still needing RemoveParentTab since it's called from
ParentTab, but now it just calls std::map::erase.

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:508:
OffscreenTab* offscreen_tab = GetMap()->CreateOffscreenTab(
On 2012/03/01 00:20:05, Antony Sargent wrote:
> Optional suggestion: the fact that the map owns the OffscreenTab* (and hence
we
> aren't leaking it at the end of this function) could be more clear here if
> CreateOffscreenTab returned a const * or const &.

Done.

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:651:
SendResponse(true);
On 2012/03/01 00:20:05, Antony Sargent wrote:
> Since this is a SyncExtensionFunction, you don't need to call SendResponse
> manually right?

Done.

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:765:
SendResponse(true);
On 2012/03/01 00:20:05, Antony Sargent wrote:
> same thing here

Done.

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.cc:836: 
On 2012/03/01 00:20:05, Antony Sargent wrote:
> nit: Can you include a comment here about where SendResponse is called in the
> async case? I assume somewhere in our UpdateTabFunction parent class. 

Done.

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
File chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.h (right):

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/a...
chrome/browser/extensions/api/offscreen_tabs/offscreen_tabs_api.h:21: // those
in tabs, there are few key differences that need to be resolved.
On 2012/02/29 18:20:52, Antony Sargent wrote:
> typo: "...there are few..." -> "...there are a few..."

Done.

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/e...
File chrome/browser/extensions/execute_script_apitest.cc (right):

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/e...
chrome/browser/extensions/execute_script_apitest.cc:19:
IN_PROC_BROWSER_TEST_F(ExecuteScriptApiTest, ExecuteScriptBasic) {
On 2012/02/29 18:20:52, Antony Sargent wrote:
> These changes look unrelated to offscreen tabs. Cruft from another CL or
> something?
> 

Done.

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tabs_module.h (right):

http://codereview.chromium.org/9234042/diff/23001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tabs_module.h:33: // can use them. We should
remove these once the APIs are combined.
On 2012/02/29 18:20:52, Antony Sargent wrote:
> Would it be appropriate for these to go in extension_tab_util.h?

Yeah, they can probably go there.

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.offscreenTabs.json (right):

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.offscreenTabs.json:44: "description":
"The URL to navigate the offscreen tab to initially. Fully-qualified URLs must
include a scheme (i.e. 'http://www.google.com', not 'www.google.com'). Relative
URLs will be relative to the current page within the extension."
On 2012/02/29 18:20:52, Antony Sargent wrote:
> During the API review meeting we did for this many months ago, one of the
> important points of discussion was whether the lifetime of the created
offscreen
> tab was scoped to the lifetime of the context of its caller (browser action
> popup, etc.), or if we were going to rely on extension authors to remember to
> call remove. 
> 
> I haven't looked at the implementation yet to see what the current behavior
is,
> but we should be sure to note in the description here something about that.

Added a note about offscreen tab lifetime.

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.offscreenTabs.json:150: "type": "any",
On 2012/02/29 18:20:52, Antony Sargent wrote:
> Any reason the type here can't be "MouseEvent" instead of "any" to get the
> benefits of type checking? Or maybe you want:
> 
> "type": "object",
> "isInstanceOf": "MouseEvent"
> 
> (see some of the other .json files for examples)

Think we can leave this as is for now? We've been creating events with simple
objects like { type: 'mousedown', button: 0, .... }, whereas there are many more
parameters involved when creating real events.

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.offscreenTabs.json:174: "$ref":
"OffscreenTab",
On 2012/02/29 18:20:52, Antony Sargent wrote:
> It's unusual to be returning the OffscreenTab object here in the callback for
> the sendMouseEvent function. I would have expected just the typical
> success/failure information, and then the developer would rely on the
onUpdated
> event if they are interested in seeing changes in e.g. the url of an offscreen
> tab. 

Done. Removed the OffscreenTab parameter from the sendKeyboardEvent,
sendMouseEvent and update methods. I originally avoided changing the interface,
since there are demos written against this. These are minimal and easy to
migrate though.

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.offscreenTabs.json:194: "type": "any",
On 2012/02/29 18:20:52, Antony Sargent wrote:
> same question here about type

Done.

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.offscreenTabs.json:204: "$ref":
"OffscreenTab",
On 2012/02/29 18:20:52, Antony Sargent wrote:
> same thing here - weird to return the OffscreenTab to the callback for this
> function

Done.

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.offscreenTabs.json:297: "$ref":
"OffscreenTab",
On 2012/02/29 18:20:52, Antony Sargent wrote:
> again, weird to return OffscreenTab here

Done.

[asargent_no_longer_on_chrome, 2012-03-13 00:06:02]

lgtm

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.offscreenTabs.json (right):

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.offscreenTabs.json:150: "type": "any",
On 2012/03/12 14:53:06, jstritar wrote:
> On 2012/02/29 18:20:52, Antony Sargent wrote:
> > Any reason the type here can't be "MouseEvent" instead of "any" to get the
> > benefits of type checking? Or maybe you want:
> > 
> > "type": "object",
> > "isInstanceOf": "MouseEvent"
> > 
> > (see some of the other .json files for examples)
> 
> Think we can leave this as is for now? We've been creating events with simple
> objects like { type: 'mousedown', button: 0, .... }, whereas there are many
more
> parameters involved when creating real events.

Ok - let's at least leave it as type "object" for now. BTW, I think you might be
able to specify a list of required and optional properties inline here for
object types, so you could still get the benefit of type checking. Or, maybe
create an full-fledged type (like Tab, Window, etc. for the tabs or windows
APIs).

[jstritar, 2012-03-15 22:31:40]

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental.offscreenTabs.json (right):

http://codereview.chromium.org/9234042/diff/23001/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental.offscreenTabs.json:150: "type": "any",
On 2012/03/13 00:06:02, Antony Sargent wrote:
>
> Ok - let's at least leave it as type "object" for now. BTW, I think you might
be
> able to specify a list of required and optional properties inline here for
> object types, so you could still get the benefit of type checking. Or, maybe
> create an full-fledged type (like Tab, Window, etc. for the tabs or windows
> APIs). 
>

We end up only using a fraction of the properties on the mouse and keyboard
events, so I defined MouseEvent and KeyboardEvents (like how OffscreenTab is
defined). This should be easier for extensions too since creating actual JS
mouse/keyboard events is kind of a pain.

[commit-bot: I haz the power, 2012-03-16 14:22:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/9234042/49001

[commit-bot: I haz the power, 2012-03-16 14:22:22]

Can't apply patch for file chrome/renderer/extensions/custom_bindings_util.cc.
While running patch -p1 --forward --force;
patching file chrome/renderer/extensions/custom_bindings_util.cc
Hunk #1 FAILED at 38.
Hunk #2 FAILED at 53.
2 out of 2 hunks FAILED -- saving rejects to file
chrome/renderer/extensions/custom_bindings_util.cc.rej

[commit-bot: I haz the power, 2012-03-16 16:42:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/9234042/53005

[commit-bot: I haz the power, 2012-03-16 22:47:58]

Try job failure for 9234042-53005 (retry) on mac_rel for steps "ui_tests,
net_unittests".
It's a second try, previously, steps "ui_tests, net_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-03-16 23:29:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/9234042/53005

[commit-bot: I haz the power, 2012-03-16 23:29:58]

Can't apply patch for file
chrome/browser/extensions/extension_function_registry.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/extension_function_registry.cc
Hunk #1 FAILED at 11.
Hunk #2 FAILED at 460.
2 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_registry.cc.rej

[commit-bot: I haz the power, 2012-03-16 23:43:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/9234042/58001

[commit-bot: I haz the power, 2012-03-17 00:54:16]

Try job failure for 9234042-58001 (retry) on linux_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-03-19 15:13:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/9234042/58001

[commit-bot: I haz the power, 2012-03-19 15:14:06]

Can't apply patch for file chrome/chrome_browser_extensions.gypi.
While running patch -p1 --forward --force;
patching file chrome/chrome_browser_extensions.gypi
Hunk #1 FAILED at 82.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/chrome_browser_extensions.gypi.rej

[commit-bot: I haz the power, 2012-03-19 18:05:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/9234042/63001

[commit-bot: I haz the power, 2012-03-19 19:48:23]

Try job failure for 9234042-63001 (retry) (retry) on linux_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-03-19 20:08:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jstritar@chromium.org/9234042/64041

[jstritar, 2012-03-19 20:09:46]

Sorry for the spam. I keep needing to merge and then unmerge (as the conflicting
CLs get reverted). I'll land manually if necessary

[commit-bot: I haz the power, 2012-03-19 22:46:26]

Can't apply patch for file
chrome/browser/extensions/extension_function_dispatcher.cc.
While running patch -p1 --forward --force;
patching file chrome/browser/extensions/extension_function_dispatcher.cc
Hunk #1 FAILED at 22.
Hunk #2 FAILED at 534.
2 out of 2 hunks FAILED -- saving rejects to file
chrome/browser/extensions/extension_function_dispatcher.cc.rej