Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(262)

Issue 7720002: Chrome Extensions chrome.experimental.offscreenTabs.* API implementation, docs, and test. (Closed)

Created:
9 years, 4 months ago by alexbost
Modified:
8 years, 8 months ago
CC:
chromium-reviews, Erik does not do reviews, brettw-cc_chromium.org, mihaip+watch_chromium.org, Aaron Boodman, darin-cc_chromium.org, Paweł Hajdan Jr., pam+watch_chromium.org
Visibility:
Public.

Description

Patch Set 1 #

Patch Set 2 : '' #

Patch Set 3 : '' #

Patch Set 4 : '' #

Patch Set 5 : '' #

Patch Set 6 : '' #

Total comments: 54

Patch Set 7 : '' #

Patch Set 8 : '' #

Patch Set 9 : '' #

Patch Set 10 : '' #

Patch Set 11 : '' #

Patch Set 12 : '' #

Patch Set 13 : '' #

Total comments: 12

Patch Set 14 : '' #

Patch Set 15 : '' #

Patch Set 16 : '' #

Patch Set 17 : '' #

Total comments: 76

Patch Set 18 : '' #

Patch Set 19 : '' #

Total comments: 72

Patch Set 20 : '' #

Patch Set 21 : '' #

Total comments: 48

Patch Set 22 : '' #

Patch Set 23 : '' #

Patch Set 24 : '' #

Patch Set 25 : '' #

Patch Set 26 : '' #

Patch Set 27 : '' #

Patch Set 28 : '' #

Total comments: 22

Patch Set 29 : '' #

Patch Set 30 : '' #

Total comments: 4

Patch Set 31 : '' #

Patch Set 32 : '' #

Total comments: 76

Patch Set 33 : '' #

Total comments: 28

Patch Set 34 : '' #

Patch Set 35 : '' #

Patch Set 36 : '' #

Patch Set 37 : '' #

Patch Set 38 : '' #

Patch Set 39 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+6391 lines, -2 lines) Patch
M chrome/browser/extensions/extension_function_dispatcher.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 2 chunks +11 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_offscreen_tabs_apitest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +20 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_offscreen_tabs_module.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +156 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_offscreen_tabs_module.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 1 chunk +1228 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_offscreen_tabs_module_constants.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +83 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_offscreen_tabs_module_constants.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +78 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/extensions/api/extension_api.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +336 lines, -0 lines 0 comments Download
M chrome/common/extensions/docs/experimental.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +1 line, -0 lines 0 comments Download
A chrome/common/extensions/docs/experimental.offscreenTabs.html View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +4028 lines, -0 lines 0 comments Download
M chrome/common/extensions/docs/samples.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 3 chunks +11 lines, -2 lines 0 comments Download
M chrome/renderer/resources/extension_process_bindings.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +18 lines, -0 lines 0 comments Download
M chrome/renderer/resources/renderer_extension_bindings.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +1 line, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/offscreen_tabs/a.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +16 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/offscreen_tabs/b.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +13 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/offscreen_tabs/c.html View 1 2 3 4 5 6 7 8 9 10 1 chunk +11 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/offscreen_tabs/manifest.json View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 1 chunk +10 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/offscreen_tabs/test.html View 17 18 19 20 21 22 23 24 1 chunk +1 line, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/offscreen_tabs/test.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 1 chunk +364 lines, -0 lines 0 comments Download

Messages

Total messages: 36 (0 generated)
Ken Russell (switch to Gerrit)
Great work overall. Some relatively minor comments and questions. Once these are addressed, let's ask ...
9 years, 4 months ago (2011-08-23 22:46:53 UTC) #1
alexbost
http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/extension_offscreen_tabs_module.cc File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/extension_offscreen_tabs_module.cc#newcode61 chrome/browser/extensions/extension_offscreen_tabs_module.cc:61: OffscreenTabsMap* offscreen_tabs_map = new OffscreenTabsMap(); On 2011/08/23 22:46:53, kbr ...
9 years, 4 months ago (2011-08-24 20:06:21 UTC) #2
Ken Russell (switch to Gerrit)
The structure of the code generally looks good. We'll need to wait to land this ...
9 years, 4 months ago (2011-08-26 00:57:47 UTC) #3
alexbost
http://codereview.chromium.org/7720002/diff/8044/chrome/common/extensions/docs/samples.json File chrome/common/extensions/docs/samples.json (right): http://codereview.chromium.org/7720002/diff/8044/chrome/common/extensions/docs/samples.json#newcode851 chrome/common/extensions/docs/samples.json:851: "source_hash": "fa560de6c20aa58332872918a330b4df00ab55cd", On 2011/08/26 00:57:48, kbr wrote: > Why ...
9 years, 4 months ago (2011-08-26 22:07:23 UTC) #4
jstritar
Interesting API! We should definitely figure out how to reconcile it with the regular tabs ...
9 years, 3 months ago (2011-09-08 16:58:20 UTC) #5
sky
I didn't get very far. I suggested some changes that will hopefully make for easier ...
9 years, 3 months ago (2011-09-08 18:12:26 UTC) #6
alexbost
Hi guys, Thanks for the good suggestions! I added some comments and reorganized the code ...
9 years, 3 months ago (2011-09-10 04:47:53 UTC) #7
sky
I made it further this time. Here's some comments. I'll add more later. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/extension_offscreen_tabs_module.cc File ...
9 years, 3 months ago (2011-09-13 23:42:23 UTC) #8
alexbost
http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/extension_offscreen_tabs_module.cc File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/extension_offscreen_tabs_module.cc#newcode66 chrome/browser/extensions/extension_offscreen_tabs_module.cc:66: void Init(const GURL& url, const int width, const int ...
9 years, 3 months ago (2011-09-14 21:45:29 UTC) #9
sky
http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/extension_offscreen_tabs_module.cc File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/extension_offscreen_tabs_module.cc#newcode174 chrome/browser/extensions/extension_offscreen_tabs_module.cc:174: if (profile && background_page_tab_contents == NULL) On 2011/09/14 21:45:30, ...
9 years, 3 months ago (2011-09-14 23:34:43 UTC) #10
alexbost
http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/extension_offscreen_tabs_module.cc File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/extension_offscreen_tabs_module.cc#newcode174 chrome/browser/extensions/extension_offscreen_tabs_module.cc:174: if (profile && background_page_tab_contents == NULL) On 2011/09/14 23:34:44, ...
9 years, 3 months ago (2011-09-15 03:29:49 UTC) #11
jstritar
http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/extension_offscreen_tabs_module.cc File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/extension_offscreen_tabs_module.cc#newcode64 chrome/browser/extensions/extension_offscreen_tabs_module.cc:64: ~OffscreenTab(); The destructors should be virtual (not Init). http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/extension_offscreen_tabs_module.cc#newcode73 ...
9 years, 3 months ago (2011-09-15 18:52:01 UTC) #12
alexbost
http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/extension_offscreen_tabs_module.cc File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/extension_offscreen_tabs_module.cc#newcode64 chrome/browser/extensions/extension_offscreen_tabs_module.cc:64: ~OffscreenTab(); On 2011/09/15 18:52:01, jstritar wrote: > The destructors ...
9 years, 3 months ago (2011-09-15 19:39:36 UTC) #13
sky
http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/extension_offscreen_tabs_module.cc File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/extension_offscreen_tabs_module.cc#newcode174 chrome/browser/extensions/extension_offscreen_tabs_module.cc:174: if (profile && background_page_tab_contents == NULL) On 2011/09/15 03:29:49, ...
9 years, 3 months ago (2011-09-15 20:05:57 UTC) #14
jstritar
http://codereview.chromium.org/7720002/diff/58001/chrome/common/extensions/api/extension_api.json File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7720002/diff/58001/chrome/common/extensions/api/extension_api.json#newcode8602 chrome/common/extensions/api/extension_api.json:8602: "type": "any", How about you define this as an ...
9 years, 3 months ago (2011-09-15 20:09:33 UTC) #15
alexbost
http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/extension_offscreen_tabs_module.cc File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/extension_offscreen_tabs_module.cc#newcode174 chrome/browser/extensions/extension_offscreen_tabs_module.cc:174: if (profile && background_page_tab_contents == NULL) On 2011/09/15 20:05:57, ...
9 years, 3 months ago (2011-09-15 20:57:57 UTC) #16
sky
http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/extension_offscreen_tabs_module.cc File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/extension_offscreen_tabs_module.cc#newcode174 chrome/browser/extensions/extension_offscreen_tabs_module.cc:174: if (profile && background_page_tab_contents == NULL) On 2011/09/15 20:57:57, ...
9 years, 3 months ago (2011-09-15 21:39:17 UTC) #17
jstritar
http://codereview.chromium.org/7720002/diff/58001/chrome/common/extensions/api/extension_api.json File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7720002/diff/58001/chrome/common/extensions/api/extension_api.json#newcode8602 chrome/common/extensions/api/extension_api.json:8602: "type": "any", On 2011/09/15 20:57:57, alexbost wrote: > On ...
9 years, 3 months ago (2011-09-15 21:40:33 UTC) #18
sky
http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/extension_offscreen_tabs_module.cc File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/extension_offscreen_tabs_module.cc#newcode363 chrome/browser/extensions/extension_offscreen_tabs_module.cc:363: tab_ = TabContentsWrapper::GetCurrentWrapperForContents(tab_contents); On 2011/09/15 20:57:57, alexbost wrote: > ...
9 years, 3 months ago (2011-09-15 21:41:35 UTC) #19
alexbost
http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/extension_offscreen_tabs_module.cc File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/extension_offscreen_tabs_module.cc#newcode174 chrome/browser/extensions/extension_offscreen_tabs_module.cc:174: if (profile && background_page_tab_contents == NULL) On 2011/09/15 21:39:17, ...
9 years, 3 months ago (2011-09-15 23:01:29 UTC) #20
jstritar
http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/extension_offscreen_tabs_apitest.cc File chrome/browser/extensions/extension_offscreen_tabs_apitest.cc (right): http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/extension_offscreen_tabs_apitest.cc#newcode14 chrome/browser/extensions/extension_offscreen_tabs_apitest.cc:14: } I remember having problems setting the experimental flag ...
9 years, 3 months ago (2011-09-16 16:03:49 UTC) #21
sky
http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/extension_offscreen_tabs_module.cc File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/extension_offscreen_tabs_module.cc#newcode63 chrome/browser/extensions/extension_offscreen_tabs_module.cc:63: class OffscreenTab : public NotificationObserver { You have way ...
9 years, 3 months ago (2011-09-16 16:29:20 UTC) #22
alexbost
http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/extension_offscreen_tabs_apitest.cc File chrome/browser/extensions/extension_offscreen_tabs_apitest.cc (right): http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/extension_offscreen_tabs_apitest.cc#newcode14 chrome/browser/extensions/extension_offscreen_tabs_apitest.cc:14: } On 2011/09/16 16:03:50, jstritar wrote: > I remember ...
9 years, 3 months ago (2011-09-16 20:18:13 UTC) #23
sky
http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/extension_offscreen_tabs_module.cc File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/extension_offscreen_tabs_module.cc#newcode214 chrome/browser/extensions/extension_offscreen_tabs_module.cc:214: *tab_contents = GetBackgroundPageTabContents(profile); On 2011/09/16 20:18:14, alexbost wrote: > ...
9 years, 3 months ago (2011-09-16 20:23:28 UTC) #24
sky
Looking much better. http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/extension_offscreen_tabs_module.cc File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/extension_offscreen_tabs_module.cc#newcode116 chrome/browser/extensions/extension_offscreen_tabs_module.cc:116: // Caller assumes ownership of OffscreenTab. ...
9 years, 3 months ago (2011-09-16 20:35:34 UTC) #25
jstritar
http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions/api_test/offscreen_tabs/test.js File chrome/test/data/extensions/api_test/offscreen_tabs/test.js (right): http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions/api_test/offscreen_tabs/test.js#newcode352 chrome/test/data/extensions/api_test/offscreen_tabs/test.js:352: function tabManagement() { On 2011/09/16 20:18:14, alexbost wrote: > ...
9 years, 3 months ago (2011-09-16 21:02:41 UTC) #26
alexbost
http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/extension_offscreen_tabs_module.cc File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/extension_offscreen_tabs_module.cc#newcode214 chrome/browser/extensions/extension_offscreen_tabs_module.cc:214: *tab_contents = GetBackgroundPageTabContents(profile); On 2011/09/16 20:23:28, sky wrote: > ...
9 years, 3 months ago (2011-09-16 21:46:53 UTC) #27
sky
This is my last concern. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/extension_offscreen_tabs_module.cc File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/extension_offscreen_tabs_module.cc#newcode214 chrome/browser/extensions/extension_offscreen_tabs_module.cc:214: *tab_contents = GetBackgroundPageTabContents(profile); On ...
9 years, 3 months ago (2011-09-16 21:51:45 UTC) #28
sky
LGTM with a TODO about figuring out background pages and as long as Jon understands ...
9 years, 3 months ago (2011-09-16 22:04:13 UTC) #29
jstritar
LGTM. Just in time! http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions/api_test/offscreen_tabs/test.js File chrome/test/data/extensions/api_test/offscreen_tabs/test.js (right): http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions/api_test/offscreen_tabs/test.js#newcode352 chrome/test/data/extensions/api_test/offscreen_tabs/test.js:352: function tabManagement() { On 2011/09/16 ...
9 years, 3 months ago (2011-09-16 22:14:22 UTC) #30
Ken Russell (switch to Gerrit)
Thanks Jon and Scott for your reviews and Alex for pushing this through. Trying to ...
9 years, 3 months ago (2011-09-16 23:18:53 UTC) #31
commit-bot: I haz the power
Change committed as 101678
9 years, 3 months ago (2011-09-18 01:02:19 UTC) #32
Ken Russell (switch to Gerrit)
Unfortunately this change was reverted in http://src.chromium.org/viewvc/chrome?view=rev&revision=101685 due to regressing the linux "sizes" step. Will ...
9 years, 3 months ago (2011-09-18 10:31:21 UTC) #33
devinrhode2
FTW, this api could solve a whole host of problems with iframes and introduce .... ...
8 years, 8 months ago (2012-04-11 08:16:31 UTC) #34
devinrhode2
This essentially makes a Sidebar API. Depending on what sort of issues you run into, ...
8 years, 8 months ago (2012-04-11 08:22:40 UTC) #35
devinrhode2
8 years, 8 months ago (2012-04-11 08:29:27 UTC) #36
Also, I can't help but mention that having an API to manually send mouse and
keyboard events sounds bad... what about focus events? webkitVisibility state?
etc...

Powered by Google App Engine
This is Rietveld 408576698