Chromium Code Reviews
|
|
Chromium Code Reviews
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 Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionDescription and Demo:
https://sites.google.com/a/chromium.org/dev/developers/design-documents/extensions/offscreen-tabs
Presentation:
https://docs.google.com/a/google.com/present/view?id=dgjqt449_3gxsnm3qj
Spec:
http://www.corp.google.com/~alexbost/no_crawl/docs/experimental.offscreenTabs.html
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=101678
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 : '' #Messages
Total messages: 36 (0 generated)
Great work overall. Some relatively minor comments and questions. Once these are addressed, let's ask the extensions team for review. http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module.cc:61: OffscreenTabsMap* offscreen_tabs_map = new OffscreenTabsMap(); It's not OK to have a nontrivial initializer for this variable; it must be initialized to NULL. Also, it needs to be in the anonymous namespace below. Add a function like GetOffscreenTabsMap() which tests it for NULL, instantiates it if so, and returns the new value, and call that function everywhere in this file to fetch the map. http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module.cc:63: OffscreenTabsEventManager* event_manager; Must go in the anonymous namespace as well. Also must be initialized to NULL. http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module.cc:207: DCHECK(tab_contents)? http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module.cc:228: return false; Do you need to SendResponse(false) here, and elsewhere in this function where false is returned? http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module.cc:280: offscreen_tab_contents->view()->SizeContents(*new gfx::Size(width, height)); This is a memory leak. Just use: ->SizeContents(gfx::Size(width, height)); http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module.cc:321: return false; Is a SendReponse(false) needed here? If so, also elsewhere in this file where functions end with SendResponse(true) but can abort early. Also would be ideal to test some of the failure cases in a unit test -- for example, by passing an obviously invalid offscreen tab ID to this function. (Actually, I see now that you do have a negative test for this API, which is good.) http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module.cc:399: return false; Here and below, is any SendResponse(false) needed? http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_offscreen_tabs_module.h (right): http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module.h:40: virtual ~CreateOffscreenTabFunction() {} Virtual function definitions in headers, in particular the first virtual function for a class, are very problematic. Looking through more of the extension code, it seems that extension_tab_api.h's use of inlined virtuals is an outlier. Please follow the pattern in extension_downloads_api.cc and move all of the definitions of these virtual functions to the .cc file. http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_offscreen_tabs_module_constants.cc (right): http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module_constants.cc:7: namespace extension_offscreen_tabs_module_constants { It would be better to share many of these with extension_tabs_module_constants.cc. Minimally, add a TODO to refactor them. http://codereview.chromium.org/7720002/diff/2006/chrome/common/extensions/doc... File chrome/common/extensions/docs/experimental.infobars.html (right): http://codereview.chromium.org/7720002/diff/2006/chrome/common/extensions/doc... chrome/common/extensions/docs/experimental.infobars.html:602: <span class="optional">optional</span> It looks like the changes to this file are unrelated to yours and that you should manually revert or bring this file up to date. http://codereview.chromium.org/7720002/diff/2006/chrome/common/extensions/doc... File chrome/common/extensions/docs/samples.html (right): http://codereview.chromium.org/7720002/diff/2006/chrome/common/extensions/doc... chrome/common/extensions/docs/samples.html:326: "7f4d3fac7ae1ad4d514a15cd0b2c48c57a58c55e": "CATBLOCK I CANT HAS CHEEZBURGER! BACKGROUND_PAGE EXPERIMENTAL CHROME.EXPERIMENTAL.WEBREQUEST.ONBEFOREREQUEST", It looks like the changes to this file are unrelated to yours and that you need to either sync your workspace or revert changes to this file. http://codereview.chromium.org/7720002/diff/2006/chrome/common/extensions/doc... File chrome/common/extensions/docs/samples.json (right): http://codereview.chromium.org/7720002/diff/2006/chrome/common/extensions/doc... chrome/common/extensions/docs/samples.json:458: "zip_path": "examples\/extensions\/catblock.zip" This block of changes, and those below, look unrelated to yours. http://codereview.chromium.org/7720002/diff/2006/chrome/renderer/resources/ex... File chrome/renderer/resources/extension_process_bindings.js (right): http://codereview.chromium.org/7720002/diff/2006/chrome/renderer/resources/ex... chrome/renderer/resources/extension_process_bindings.js:1021: updateArgumentsPreValidate = function() { Actually, you can write this as: apiFunctions["experimental.offscreenTabs.sendKeyboardEvent"].updateArgumentsPreValidate = apiFunctions["experimental.offscreenTabs.sendMouseEvent"].updateArgumentsPreValidate; http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/offscreen_tabs/test.js (right): http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:29: // TODO listen for onUpdated once its implemented in the API TODO(alexbost):. Also, "it's". http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:30: var sleepTime = 100; In general the use of setTimeout and arbitrary delays results in flaky tests. Implementing onUpdated() and getting rid of this delay should be the top priority once this lands. Or, if it isn't that difficult to implement onUpdated(), let's do it now. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:44: } Missing ";". http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:47: var y = 11; It seems pretty delicate to aim for some text on the test pages with the synthesized clicks in these tests. Would it be possible to create a div element with a known width and height and attach the mouse event handlers to it? http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:59: } This could be a local variable in the "keyPress" function below. Also, needs a trailing ";". http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:84: func(true); I don't understand what this function is supposed to do. If you're just trying to delay once and then do the callback, you can just write: function waitToNavigate(tabId, callback) { setTimeout(function() { chrome.experimental.offscreenTabs.get(tabId, pass(function(tab) { callback(tab); })); }, sleepTime, false); } However in this case I'd suggest increasing the delay time to reduce potential flakiness -- perhaps to 250 ms. If you're trying to do something more sophisticated, like wait until the call to chrome.experimental.offscreenTabs.get succeeds, then more logic is needed. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:118: tabs[i].id = tabsResult[i].id; This looks wrong. If you are trying to make sure that the tabs are in the same order, sort both arrays with the same function, e.g.: function sortTab(tabA, tabB) { return tabB.id - tabA.id; } tabs.sort(sortTab); tabsResult.sort(sortTab); Then compare each tab. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:135: get(-1, fail("No offscreen tab with id: -1.")); This negative test is nice. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:187: tabMouse = tab; This looks unnecessary. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:198: mouseEvent.wheelDeltaY = -100; See question at top about using a div or something similar in the test pages instead of aiming for a line of text, to make the test more robust. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:200: sendMouseEvent(tabMouse.id, mouseEvent, null, null, pass(function(tab) { Pass 0 instead of null since the arguments are supposed to be numbers. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:232: chrome.experimental.offscreenTabs.remove(tabMouse.id); Indentation is off. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:271: chrome.experimental.offscreenTabs.remove(tabKeyboard.id); Indentation is off. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:349: chrome.test.runTests([ Indentation.
http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module.cc:61: OffscreenTabsMap* offscreen_tabs_map = new OffscreenTabsMap(); On 2011/08/23 22:46:53, kbr wrote: > It's not OK to have a nontrivial initializer for this variable; it must be > initialized to NULL. Also, it needs to be in the anonymous namespace below. Add > a function like GetOffscreenTabsMap() which tests it for NULL, instantiates it > if so, and returns the new value, and call that function everywhere in this file > to fetch the map. Done. http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module.cc:63: OffscreenTabsEventManager* event_manager; On 2011/08/23 22:46:53, kbr wrote: > Must go in the anonymous namespace as well. Also must be initialized to NULL. Done. http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module.cc:207: On 2011/08/23 22:46:53, kbr wrote: > DCHECK(tab_contents)? Done. http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module.cc:228: return false; On 2011/08/23 22:46:53, kbr wrote: > Do you need to SendResponse(false) here, and elsewhere in this function where > false is returned? Looking at extension_tabs_module.cc, they do not call SendResponse(false) in RunImpl() methods. http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module.cc:280: offscreen_tab_contents->view()->SizeContents(*new gfx::Size(width, height)); On 2011/08/23 22:46:53, kbr wrote: > This is a memory leak. Just use: ->SizeContents(gfx::Size(width, height)); Done. http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module.cc:321: return false; On 2011/08/23 22:46:53, kbr wrote: > Is a SendReponse(false) needed here? If so, also elsewhere in this file where > functions end with SendResponse(true) but can abort early. Also would be ideal > to test some of the failure cases in a unit test -- for example, by passing an > obviously invalid offscreen tab ID to this function. (Actually, I see now that > you do have a negative test for this API, which is good.) See above. http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module.cc:399: return false; On 2011/08/23 22:46:53, kbr wrote: > Here and below, is any SendResponse(false) needed? See above. http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_offscreen_tabs_module.h (right): http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module.h:40: virtual ~CreateOffscreenTabFunction() {} On 2011/08/23 22:46:53, kbr wrote: > Virtual function definitions in headers, in particular the first virtual > function for a class, are very problematic. Looking through more of the > extension code, it seems that extension_tab_api.h's use of inlined virtuals is > an outlier. Please follow the pattern in extension_downloads_api.cc and move all > of the definitions of these virtual functions to the .cc file. Done. http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... File chrome/browser/extensions/extension_offscreen_tabs_module_constants.cc (right): http://codereview.chromium.org/7720002/diff/2006/chrome/browser/extensions/ex... chrome/browser/extensions/extension_offscreen_tabs_module_constants.cc:7: namespace extension_offscreen_tabs_module_constants { On 2011/08/23 22:46:53, kbr wrote: > It would be better to share many of these with > extension_tabs_module_constants.cc. Minimally, add a TODO to refactor them. Done. http://codereview.chromium.org/7720002/diff/2006/chrome/common/extensions/doc... File chrome/common/extensions/docs/experimental.infobars.html (right): http://codereview.chromium.org/7720002/diff/2006/chrome/common/extensions/doc... chrome/common/extensions/docs/experimental.infobars.html:602: <span class="optional">optional</span> On 2011/08/23 22:46:53, kbr wrote: > It looks like the changes to this file are unrelated to yours and that you > should manually revert or bring this file up to date. Done. http://codereview.chromium.org/7720002/diff/2006/chrome/common/extensions/doc... File chrome/common/extensions/docs/samples.html (right): http://codereview.chromium.org/7720002/diff/2006/chrome/common/extensions/doc... chrome/common/extensions/docs/samples.html:326: "7f4d3fac7ae1ad4d514a15cd0b2c48c57a58c55e": "CATBLOCK I CANT HAS CHEEZBURGER! BACKGROUND_PAGE EXPERIMENTAL CHROME.EXPERIMENTAL.WEBREQUEST.ONBEFOREREQUEST", On 2011/08/23 22:46:53, kbr wrote: > It looks like the changes to this file are unrelated to yours and that you need > to either sync your workspace or revert changes to this file. Done. http://codereview.chromium.org/7720002/diff/2006/chrome/common/extensions/doc... File chrome/common/extensions/docs/samples.json (right): http://codereview.chromium.org/7720002/diff/2006/chrome/common/extensions/doc... chrome/common/extensions/docs/samples.json:458: "zip_path": "examples\/extensions\/catblock.zip" On 2011/08/23 22:46:53, kbr wrote: > This block of changes, and those below, look unrelated to yours. Done. http://codereview.chromium.org/7720002/diff/2006/chrome/renderer/resources/ex... File chrome/renderer/resources/extension_process_bindings.js (right): http://codereview.chromium.org/7720002/diff/2006/chrome/renderer/resources/ex... chrome/renderer/resources/extension_process_bindings.js:1021: updateArgumentsPreValidate = function() { On 2011/08/23 22:46:53, kbr wrote: > Actually, you can write this as: > apiFunctions["experimental.offscreenTabs.sendKeyboardEvent"].updateArgumentsPreValidate > = > apiFunctions["experimental.offscreenTabs.sendMouseEvent"].updateArgumentsPreValidate; Done. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/offscreen_tabs/test.js (right): http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:29: // TODO listen for onUpdated once its implemented in the API On 2011/08/23 22:46:53, kbr wrote: > TODO(alexbost):. Also, "it's". Done. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:30: var sleepTime = 100; On 2011/08/23 22:46:53, kbr wrote: > In general the use of setTimeout and arbitrary delays results in flaky tests. > Implementing onUpdated() and getting rid of this delay should be the top > priority once this lands. Or, if it isn't that difficult to implement > onUpdated(), let's do it now. Agreed. Putting it on top of my list. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:44: } On 2011/08/23 22:46:53, kbr wrote: > Missing ";". Done. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:47: var y = 11; On 2011/08/23 22:46:53, kbr wrote: > It seems pretty delicate to aim for some text on the test pages with the > synthesized clicks in these tests. Would it be possible to create a div element > with a known width and height and attach the mouse event handlers to it? I think aiming for the text is OK. It has absolute position (10,10) so the (11,11) pixel should always be part of the text no matter what the text is. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:59: } On 2011/08/23 22:46:53, kbr wrote: > This could be a local variable in the "keyPress" function below. Also, needs a > trailing ";". Done. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:84: func(true); On 2011/08/23 22:46:53, kbr wrote: > I don't understand what this function is supposed to do. If you're just trying > to delay once and then do the callback, you can just write: > > function waitToNavigate(tabId, callback) { > setTimeout(function() { > chrome.experimental.offscreenTabs.get(tabId, pass(function(tab) { > callback(tab); > })); > }, sleepTime, false); > } > > However in this case I'd suggest increasing the delay time to reduce potential > flakiness -- perhaps to 250 ms. > > If you're trying to do something more sophisticated, like wait until the call to > chrome.experimental.offscreenTabs.get succeeds, then more logic is needed. Done. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:118: tabs[i].id = tabsResult[i].id; On 2011/08/23 22:46:53, kbr wrote: > This looks wrong. If you are trying to make sure that the tabs are in the same > order, sort both arrays with the same function, e.g.: > > function sortTab(tabA, tabB) { return tabB.id - tabA.id; } > tabs.sort(sortTab); > tabsResult.sort(sortTab); > > Then compare each tab. Good point. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:135: get(-1, fail("No offscreen tab with id: -1.")); On 2011/08/23 22:46:53, kbr wrote: > This negative test is nice. Done. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:187: tabMouse = tab; On 2011/08/23 22:46:53, kbr wrote: > This looks unnecessary. This is just to set tabMouse.id which is used later. I will make it explicit: tabMouse.id = tab.id; http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:198: mouseEvent.wheelDeltaY = -100; On 2011/08/23 22:46:53, kbr wrote: > See question at top about using a div or something similar in the test pages > instead of aiming for a line of text, to make the test more robust. See above. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:200: sendMouseEvent(tabMouse.id, mouseEvent, null, null, pass(function(tab) { On 2011/08/23 22:46:53, kbr wrote: > Pass 0 instead of null since the arguments are supposed to be numbers. Done. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:232: chrome.experimental.offscreenTabs.remove(tabMouse.id); On 2011/08/23 22:46:53, kbr wrote: > Indentation is off. Done. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:271: chrome.experimental.offscreenTabs.remove(tabKeyboard.id); On 2011/08/23 22:46:53, kbr wrote: > Indentation is off. Done. http://codereview.chromium.org/7720002/diff/2006/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:349: chrome.test.runTests([ On 2011/08/23 22:46:53, kbr wrote: > Indentation. This looks right according to: http://src.chromium.org/viewvc/chrome/trunk/src/chrome/test/data/extensions/a...
The structure of the code generally looks good. We'll need to wait to land this until the API design review. A few more issues in the test case. http://codereview.chromium.org/7720002/diff/8044/chrome/common/extensions/doc... File chrome/common/extensions/docs/samples.json (right): http://codereview.chromium.org/7720002/diff/8044/chrome/common/extensions/doc... chrome/common/extensions/docs/samples.json:851: "source_hash": "fa560de6c20aa58332872918a330b4df00ab55cd", Why did this change? I don't think it should. http://codereview.chromium.org/7720002/diff/8044/chrome/common/extensions/doc... chrome/common/extensions/docs/samples.json:1202: "source_hash": "672f49ed8edbe0829c7ba5a1d890b4440b157991", Ditto. http://codereview.chromium.org/7720002/diff/8044/chrome/renderer/resources/ex... File chrome/renderer/resources/extension_process_bindings.js (right): http://codereview.chromium.org/7720002/diff/8044/chrome/renderer/resources/ex... chrome/renderer/resources/extension_process_bindings.js:1015: delete arguments[1][prop]; How about using a local variable for arguments[1]? http://codereview.chromium.org/7720002/diff/8044/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/offscreen_tabs/test.js (right): http://codereview.chromium.org/7720002/diff/8044/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:64: assertEq(tabA.height, tabB.height); Also assert that the IDs are equal. http://codereview.chromium.org/7720002/diff/8044/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:113: tabs[i].id = tabsResult[i].id; This should be unnecessary and is skewing the results. Shouldn't the IDs of the fetched tabs be the same as the IDs when they were created? http://codereview.chromium.org/7720002/diff/8044/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:178: tabMouse.id = tab.id; You should never reassign the IDs within the offscreen tab objects in this test. That information comes back from the browser and should be considered immutable. If you need to store this off for use in the next test, use a different variable.
http://codereview.chromium.org/7720002/diff/8044/chrome/common/extensions/doc... File chrome/common/extensions/docs/samples.json (right): http://codereview.chromium.org/7720002/diff/8044/chrome/common/extensions/doc... chrome/common/extensions/docs/samples.json:851: "source_hash": "fa560de6c20aa58332872918a330b4df00ab55cd", On 2011/08/26 00:57:48, kbr wrote: > Why did this change? I don't think it should. Not sure. http://codereview.chromium.org/7720002/diff/8044/chrome/common/extensions/doc... chrome/common/extensions/docs/samples.json:1202: "source_hash": "672f49ed8edbe0829c7ba5a1d890b4440b157991", On 2011/08/26 00:57:48, kbr wrote: > Ditto. Done. http://codereview.chromium.org/7720002/diff/8044/chrome/renderer/resources/ex... File chrome/renderer/resources/extension_process_bindings.js (right): http://codereview.chromium.org/7720002/diff/8044/chrome/renderer/resources/ex... chrome/renderer/resources/extension_process_bindings.js:1015: delete arguments[1][prop]; On 2011/08/26 00:57:48, kbr wrote: > How about using a local variable for arguments[1]? Done. http://codereview.chromium.org/7720002/diff/8044/chrome/test/data/extensions/... File chrome/test/data/extensions/api_test/offscreen_tabs/test.js (right): http://codereview.chromium.org/7720002/diff/8044/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:64: assertEq(tabA.height, tabB.height); On 2011/08/26 00:57:48, kbr wrote: > Also assert that the IDs are equal. Done. http://codereview.chromium.org/7720002/diff/8044/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:113: tabs[i].id = tabsResult[i].id; On 2011/08/26 00:57:48, kbr wrote: > This should be unnecessary and is skewing the results. Shouldn't the IDs of the > fetched tabs be the same as the IDs when they were created? Done. http://codereview.chromium.org/7720002/diff/8044/chrome/test/data/extensions/... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:178: tabMouse.id = tab.id; On 2011/08/26 00:57:48, kbr wrote: > You should never reassign the IDs within the offscreen tab objects in this test. > That information comes back from the browser and should be considered immutable. > If you need to store this off for use in the next test, use a different > variable. Done.
Interesting API! We should definitely figure out how to reconcile it with the regular tabs API. I'm still getting through the review, but this should get you started. Most of the comments are nits so far, except the one about having an OffscreenTab class. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:63: TabIdToTabMap* tabs_map = NULL; nit: add 1 line of whitespace to make it easier to read... after the offscreen_tabs_map declaration too http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:63: TabIdToTabMap* tabs_map = NULL; actually, I don't think you need TabIdToTabMap at all if you use ExtensionTabsUtil::GetTabById http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:66: TabIdToOffscreenTabsMap* offscreen_tabs_map = NULL; how about calling this map OffscreenTabsMap and updating the method names throughout? http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:73: OffscreenTabsEventManager* offscreen_tabs_event_manager = NULL; move these to ExtensionService? http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:115: TabContents* GetBackgroundPageTabContents(Profile* profile = NULL) { We usually try to stay way from default arguments. See this: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:123: int GetTabId(const TabContents* tab_contents) { can you get rid of this method and call ExtensionTabUtil::GetTabId directly? http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:143: std::string* error_message) { Could you group the input and output arguments together? I think right now it's in, out, in, out? in, in, out, out would probably be easier to understand and follows our style guidelines http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:144: std::vector<TabContents*> offscreen_tab_contents_vector = nit: renaming this offscreen_tabs might make same you some space below. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:145: (*GetTabIdToOffscreenTabsMap())[GetTabId(tab_contents)]; I think it'd be a little more readable to do: GetTabIdToOffscreenTabsMap()->at(GetTabId(tab_contents)); (in the other places below too) http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:148: it_offscreen_tab_contents = offscreen_tab_contents_vector.begin(); you could probably rename this to just "it" or "offscreen_tab" to save more space http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:215: std::string GetTabUrl(const TabContents* tab_contents) { Not sure you really need this method... http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:226: return -1; under what circumstances would tab this return -1? (when would you be getting the width or height of a tab that doesn't exist?) http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:265: result->SetInteger(keys::kIdKey, GetTabId(tab->tab_contents())); How about you put the TabContents in a local variable? TabContents* contents = tab->tab_contents(); http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:317: return; You can add a NOTREACHED() here since you should only be getting one of those two notifications. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:324: std::vector<TabContents*> offscreen_tab_contents_vector = can you shorten these variable names a bit too? offscreen_tabs and it or offscreen_tab? http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:332: GetTabsEventManager()->UnregisterForTabNotifications(tab_contents); this can just be UnregisterForTabNotifications(tab_contents) since you're calling it from within TabEventsManager http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:400: // Send a routed event directly the tab that spawned the offscreen tab. nit: "directly to the tab" http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:427: // Url Implementation comments should usually be complete sentences, here's the reference in the style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem... http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:457: width = window_bounds.width(); Can you initialize width with window_bounds.width() rather than set it in the else? http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:464: height = window_bounds.height(); ditto http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:466: // New offscreen tab fix these comments too http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:631: TabContents* current_tab_contents; initialize to NULL http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:639: DictionaryValue* dict; ditto http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:901: bool ToDataUrlOffscreenTabFunction::RunImpl() { How do you know how often to call this method? It looks like we trigger onUpdated whenever an offscreen tab navigates to a new page, but it looks like your demo updates after each keypress too. Do we trigger events for those things too? http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.h (right): http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.h:28: class TabsEventManager : public NotificationObserver { Can you put a little comment above TabsEventManager and OffscreenTabsEventManager describing what they do? http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.h:28: class TabsEventManager : public NotificationObserver { It's a little difficult to follow the life cycle of an offscreen tab, since it's spread throughout the whole file. What do you think of having an OffscreenTab class that manages the life cycle of an offscreen tab? - creates the new TabContents, and configures it to be offscreen - registers for notifications of the TabContents during construction - unregisters for notifications during deconstruction - holds pointers to its TabContents* and the extension that created it You should also be able to get rid of some of those maps then, like TabIdToTabMap and TabIdToExtensionidMap, since each OffscreenTab can hold the relevant data. http://codereview.chromium.org/7720002/diff/26001/chrome/common/extensions/ap... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7720002/diff/26001/chrome/common/extensions/ap... chrome/common/extensions/api/extension_api.json:8544: "name": "toDataUrl", How do you know how often to call this method? It looks like we can grab a new screenshot whenever onUpdated fires, but in the demo the screenshot updated after each keypress too.
I didn't get very far. I suggested some changes that will hopefully make for easier to follow, test and maintain code. -Scott http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:56: const int ToDataUrlOffscreenTabFunction::kDefaultQuality = 90; Can you assign the value in the definition? If not, put // static on the previous line. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:69: TabIdToExtensionIdMap* extension_id_map = NULL; Do you really need so many mappings? Could you instead map from id to a structure that contains all the data you care about? http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:116: if (profile && background_page_tab_contents == NULL) What if the profile differs from the profile you created the background_page_tab_contents with? Shouldn't you create one TabContents per profile? http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:130: *tab_contents = (*GetTabIdToTabMap())[tab_id]; Don't you want to use find so that you don't create an entry in the map if one isn't there already? http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:132: if (tab_contents) Shouldn't this be *tab_contents? http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:419: if (!browser) { Why do many of your run methods require a browser? For example, this one only requires it if width/height isn't needed. And GetOffscreenTabFunction::RunImpl doesn't even use the browser it looks for. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:482: return false; If you return here you leak the tabs you just created. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:494: if ((*GetTabIdToOffscreenTabsMap())[tab_contents_id]. Rather than using a bunch of static maps and what not, could you move all the state (GetTabIdToTabMap, GetTabIdToExtensionIdMap ...) into some object (maybe TabsEventManager). This should make for saner lifetime and easier to follow code. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.h (right): http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.h:37: const NotificationDetails& details); Use OVERRIDE where appropriate. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.h:40: class OffscreenTabsEventManager : public NotificationObserver { newlines between each class, DISALLOW_COPY_AND_ASSIGN for each class, and newline between sections (33/34 above). http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.h:57: DECLARE_EXTENSION_FUNCTION_NAME("experimental.offscreenTabs.create") Should the DECLARE macro be in the private section?
Hi guys, Thanks for the good suggestions! I added some comments and reorganized the code so it is more readable. You will notice a few new classes: OffscreenTab, Tab, and Map. Thanks for your help, Alex http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:56: const int ToDataUrlOffscreenTabFunction::kDefaultQuality = 90; On 2011/09/08 18:12:26, sky wrote: > Can you assign the value in the definition? If not, put // static on the > previous line. Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:63: TabIdToTabMap* tabs_map = NULL; On 2011/09/08 16:58:20, jstritar wrote: > nit: add 1 line of whitespace to make it easier to read... after the > offscreen_tabs_map declaration too Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:63: TabIdToTabMap* tabs_map = NULL; On 2011/09/08 16:58:20, jstritar wrote: > actually, I don't think you need TabIdToTabMap at all if you use > ExtensionTabsUtil::GetTabById I have combined all maps into a Map class. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:66: TabIdToOffscreenTabsMap* offscreen_tabs_map = NULL; On 2011/09/08 16:58:20, jstritar wrote: > how about calling this map OffscreenTabsMap and updating the method names > throughout? Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:69: TabIdToExtensionIdMap* extension_id_map = NULL; On 2011/09/08 18:12:26, sky wrote: > Do you really need so many mappings? Could you instead map from id to a > structure that contains all the data you care about? Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:73: OffscreenTabsEventManager* offscreen_tabs_event_manager = NULL; On 2011/09/08 16:58:20, jstritar wrote: > move these to ExtensionService? I would leave this for a future refactoring patch. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:115: TabContents* GetBackgroundPageTabContents(Profile* profile = NULL) { On 2011/09/08 16:58:20, jstritar wrote: > We usually try to stay way from default arguments. See this: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:116: if (profile && background_page_tab_contents == NULL) On 2011/09/08 18:12:26, sky wrote: > What if the profile differs from the profile you created the > background_page_tab_contents with? Shouldn't you create one TabContents per > profile? We are assuming that offscreen tabs will not be created by background pages with the exception of the API test. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:123: int GetTabId(const TabContents* tab_contents) { On 2011/09/08 16:58:20, jstritar wrote: > can you get rid of this method and call ExtensionTabUtil::GetTabId directly? Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:130: *tab_contents = (*GetTabIdToTabMap())[tab_id]; On 2011/09/08 18:12:26, sky wrote: > Don't you want to use find so that you don't create an entry in the map if one > isn't there already? Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:132: if (tab_contents) On 2011/09/08 18:12:26, sky wrote: > Shouldn't this be *tab_contents? Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:143: std::string* error_message) { On 2011/09/08 16:58:20, jstritar wrote: > Could you group the input and output arguments together? I think right now it's > in, out, in, out? in, in, out, out would probably be easier to understand and > follows our style guidelines Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:144: std::vector<TabContents*> offscreen_tab_contents_vector = On 2011/09/08 16:58:20, jstritar wrote: > nit: renaming this offscreen_tabs might make same you some space below. Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:145: (*GetTabIdToOffscreenTabsMap())[GetTabId(tab_contents)]; On 2011/09/08 16:58:20, jstritar wrote: > I think it'd be a little more readable to do: > GetTabIdToOffscreenTabsMap()->at(GetTabId(tab_contents)); > > (in the other places below too) Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:148: it_offscreen_tab_contents = offscreen_tab_contents_vector.begin(); On 2011/09/08 16:58:20, jstritar wrote: > you could probably rename this to just "it" or "offscreen_tab" to save more > space Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:215: std::string GetTabUrl(const TabContents* tab_contents) { On 2011/09/08 16:58:20, jstritar wrote: > Not sure you really need this method... Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:226: return -1; On 2011/09/08 16:58:20, jstritar wrote: > under what circumstances would tab this return -1? (when would you be getting > the width or height of a tab that doesn't exist?) I did this as a precaution so we don't segfault on a non-existing tab for example. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:265: result->SetInteger(keys::kIdKey, GetTabId(tab->tab_contents())); On 2011/09/08 16:58:20, jstritar wrote: > How about you put the TabContents in a local variable? > > TabContents* contents = tab->tab_contents(); Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:317: return; On 2011/09/08 16:58:20, jstritar wrote: > You can add a NOTREACHED() here since you should only be getting one of those > two notifications. Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:324: std::vector<TabContents*> offscreen_tab_contents_vector = On 2011/09/08 16:58:20, jstritar wrote: > can you shorten these variable names a bit too? offscreen_tabs and it or > offscreen_tab? Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:332: GetTabsEventManager()->UnregisterForTabNotifications(tab_contents); On 2011/09/08 16:58:20, jstritar wrote: > this can just be UnregisterForTabNotifications(tab_contents) since you're > calling it from within TabEventsManager Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:400: // Send a routed event directly the tab that spawned the offscreen tab. On 2011/09/08 16:58:20, jstritar wrote: > nit: "directly to the tab" Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:419: if (!browser) { On 2011/09/08 18:12:26, sky wrote: > Why do many of your run methods require a browser? For example, this one only > requires it if width/height isn't needed. And GetOffscreenTabFunction::RunImpl > doesn't even use the browser it looks for. Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:427: // Url On 2011/09/08 16:58:20, jstritar wrote: > Implementation comments should usually be complete sentences, here's the > reference in the style guide: > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Implem... Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:457: width = window_bounds.width(); On 2011/09/08 16:58:20, jstritar wrote: > Can you initialize width with window_bounds.width() rather than set it in the > else? Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:464: height = window_bounds.height(); On 2011/09/08 16:58:20, jstritar wrote: > ditto Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:466: // New offscreen tab On 2011/09/08 16:58:20, jstritar wrote: > fix these comments too Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:482: return false; On 2011/09/08 18:12:26, sky wrote: > If you return here you leak the tabs you just created. Good catch. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:494: if ((*GetTabIdToOffscreenTabsMap())[tab_contents_id]. On 2011/09/08 18:12:26, sky wrote: > Rather than using a bunch of static maps and what not, could you move all the > state (GetTabIdToTabMap, GetTabIdToExtensionIdMap ...) into some object (maybe > TabsEventManager). This should make for saner lifetime and easier to follow > code. Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:631: TabContents* current_tab_contents; On 2011/09/08 16:58:20, jstritar wrote: > initialize to NULL Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:639: DictionaryValue* dict; On 2011/09/08 16:58:20, jstritar wrote: > ditto Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:901: bool ToDataUrlOffscreenTabFunction::RunImpl() { On 2011/09/08 16:58:20, jstritar wrote: > How do you know how often to call this method? It looks like we trigger > onUpdated whenever an offscreen tab navigates to a new page, but it looks like > your demo updates after each keypress too. Do we trigger events for those things > too? We let the application explicitly call this method. In our demos we use a loop in order to get updates all the time. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.h (right): http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.h:28: class TabsEventManager : public NotificationObserver { On 2011/09/08 16:58:20, jstritar wrote: > Can you put a little comment above TabsEventManager and > OffscreenTabsEventManager describing what they do? Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.h:28: class TabsEventManager : public NotificationObserver { On 2011/09/08 16:58:20, jstritar wrote: > It's a little difficult to follow the life cycle of an offscreen tab, since it's > spread throughout the whole file. What do you think of having an OffscreenTab > class that manages the life cycle of an offscreen tab? > > - creates the new TabContents, and configures it to be offscreen > - registers for notifications of the TabContents during construction > - unregisters for notifications during deconstruction > - holds pointers to its TabContents* and the extension that created it > > You should also be able to get rid of some of those maps then, like > TabIdToTabMap and TabIdToExtensionidMap, since each OffscreenTab can hold the > relevant data. Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.h:37: const NotificationDetails& details); On 2011/09/08 18:12:26, sky wrote: > Use OVERRIDE where appropriate. Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.h:40: class OffscreenTabsEventManager : public NotificationObserver { On 2011/09/08 18:12:26, sky wrote: > newlines between each class, DISALLOW_COPY_AND_ASSIGN for each class, and > newline between sections (33/34 above). Done. http://codereview.chromium.org/7720002/diff/26001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.h:57: DECLARE_EXTENSION_FUNCTION_NAME("experimental.offscreenTabs.create") On 2011/09/08 18:12:26, sky wrote: > Should the DECLARE macro be in the private section? Done. http://codereview.chromium.org/7720002/diff/26001/chrome/common/extensions/ap... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7720002/diff/26001/chrome/common/extensions/ap... chrome/common/extensions/api/extension_api.json:8544: "name": "toDataUrl", On 2011/09/08 16:58:20, jstritar wrote: > How do you know how often to call this method? It looks like we can grab a new > screenshot whenever onUpdated fires, but in the demo the screenshot updated > after each keypress too. Answered in extension_offscreen_tabs_module.cc.
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/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:66: void Init(const GURL& url, const int width, const int height, when you wrap, each param goes on its own line. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:71: DictionaryValue* CreateValue(); Description? And document return value. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:82: NotificationRegistrar registrar_; newline between 81 and 82 http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:84: TabContents* contents_; Document ownership. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:102: const std::vector<OffscreenTab*> GetOffscreenTabs(); this should return a reference. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:116: TabContents* contents_; Document ownerhsip. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:118: const std::string* extension_id_; Does this really need to store a raw pointer? A value is much safer. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:132: ExtensionFunctionDispatcher* dispatcher, each param on its own line, here and other functions in this class. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:166: Map* GetMap() { Add descriptions on all your functions. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:174: if (profile && background_page_tab_contents == NULL) What happens in a multi-profile world? Don't you need a background tab contents per profile? http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:191: if (tab_contents) *tab_contents ? http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:210: url = extension->GetResourceURL(url_string); This seems like it may have security implications I don't quite know. You should talk with the extensions guy (Aaron, or Antony..) to make sure it's ok. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:219: URLFixerUpper::FixupURL(url.possibly_invalid_spec(), std::string()); Why do we need to fix this up? http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:225: int GetTabWidth(const TabContents* tab_contents) { This should take a TabCONtentsWrapper, not a tab_contents. Also, how about a single method: GetTabSize that returns a gfx::Size. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:247: OffscreenTab::OffscreenTab() {} initialize contents_. Although once you convert to scoped_ptr you won't need it. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:251: UnregisterForTabNotifications(); This is done by virtue of the registrar_ being destroyed. You shouldn't have to explicitly do it. In fact, you should probably move register into into and nuke both register/unregister. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:262: contents_ = new TabContents(profile, NULL, MSG_ROUTING_NONE, NULL, NULL); contents_ should be a scoped_ptr<TabContentsWrapper> http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:289: void OffscreenTab::SetSize(int width, int height) { newline between 288 and 289 http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:308: DCHECK(type == content::NOTIFICATION_NAV_ENTRY_COMMITTED); DCHECK_EQ http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:322: if (!GetMap()->GetTabOfOffscreenTab(contents_, &tab, &error_)) { Should this be a DCHECK? http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:344: tab->GetContents()->render_view_host()->routing_id(), indent 2 more. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:364: TabContents* Tab::GetContents() { inline and name contents() http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:368: const std::vector<OffscreenTab*> Tab::GetOffscreenTabs() { inline and name offscreen_tabs(), also return a ref. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:381: for (TabIterator it_tab = offscreen_tabs_.begin(); Use std::find http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:418: delete *it; STLDeleteElements http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:421: if (!GetMap()->RemoveTab(ExtensionTabUtil::GetTabId(contents_), &error_)) { Again, shouldn't this be a DCHECK? http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:426: UnregisterForTabNotifications(); Remove this, it's not needed. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:428: delete this; This should be first, and everything in this method should be moved to the destructor. That way if you delete a Tab, everything is cleaned up correctly. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:455: remove this line. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:465: if (map[tab_id] == NULL) { This inserts a NULL if tab_id isn't in the map. Instead use find. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.h (right): http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.h:27: class CreateOffscreenTabFunction : public SyncExtensionFunction { Add descriptions for these classes.
http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:66: void Init(const GURL& url, const int width, const int height, On 2011/09/13 23:42:23, sky wrote: > when you wrap, each param goes on its own line. Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:71: DictionaryValue* CreateValue(); On 2011/09/13 23:42:23, sky wrote: > Description? And document return value. Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:82: NotificationRegistrar registrar_; On 2011/09/13 23:42:23, sky wrote: > newline between 81 and 82 Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:84: TabContents* contents_; On 2011/09/13 23:42:23, sky wrote: > Document ownership. Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:102: const std::vector<OffscreenTab*> GetOffscreenTabs(); On 2011/09/13 23:42:23, sky wrote: > this should return a reference. Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:116: TabContents* contents_; On 2011/09/13 23:42:23, sky wrote: > Document ownerhsip. Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:118: const std::string* extension_id_; On 2011/09/13 23:42:23, sky wrote: > Does this really need to store a raw pointer? A value is much safer. Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:132: ExtensionFunctionDispatcher* dispatcher, On 2011/09/13 23:42:23, sky wrote: > each param on its own line, here and other functions in this class. Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:166: Map* GetMap() { On 2011/09/13 23:42:23, sky wrote: > Add descriptions on all your functions. Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:174: if (profile && background_page_tab_contents == NULL) On 2011/09/13 23:42:23, sky wrote: > What happens in a multi-profile world? Don't you need a background tab contents > per profile? This method is intended only for testing purposes. We are assuming that offscreen tabs will not be created by background pages with the exception of the API test background page. I will add a TODO for the multi-profile case. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:191: if (tab_contents) On 2011/09/13 23:42:23, sky wrote: > *tab_contents ? Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:210: url = extension->GetResourceURL(url_string); On 2011/09/13 23:42:23, sky wrote: > This seems like it may have security implications I don't quite know. You should > talk with the extensions guy (Aaron, or Antony..) to make sure it's ok. I copied this function verbatim from the tabs API. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:219: URLFixerUpper::FixupURL(url.possibly_invalid_spec(), std::string()); On 2011/09/13 23:42:23, sky wrote: > Why do we need to fix this up? I copied this function verbatim from the tabs API. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:225: int GetTabWidth(const TabContents* tab_contents) { On 2011/09/13 23:42:23, sky wrote: > This should take a TabCONtentsWrapper, not a tab_contents. Also, how about a > single method: GetTabSize that returns a gfx::Size. Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:247: OffscreenTab::OffscreenTab() {} On 2011/09/13 23:42:23, sky wrote: > initialize contents_. Although once you convert to scoped_ptr you won't need it. Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:251: UnregisterForTabNotifications(); On 2011/09/13 23:42:23, sky wrote: > This is done by virtue of the registrar_ being destroyed. You shouldn't have to > explicitly do it. In fact, you should probably move register into into and nuke > both register/unregister. Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:262: contents_ = new TabContents(profile, NULL, MSG_ROUTING_NONE, NULL, NULL); On 2011/09/13 23:42:23, sky wrote: > contents_ should be a scoped_ptr<TabContentsWrapper> Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:289: void OffscreenTab::SetSize(int width, int height) { On 2011/09/13 23:42:23, sky wrote: > newline between 288 and 289 Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:308: DCHECK(type == content::NOTIFICATION_NAV_ENTRY_COMMITTED); On 2011/09/13 23:42:23, sky wrote: > DCHECK_EQ Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:322: if (!GetMap()->GetTabOfOffscreenTab(contents_, &tab, &error_)) { On 2011/09/13 23:42:23, sky wrote: > Should this be a DCHECK? Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:344: tab->GetContents()->render_view_host()->routing_id(), On 2011/09/13 23:42:23, sky wrote: > indent 2 more. Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:364: TabContents* Tab::GetContents() { On 2011/09/13 23:42:23, sky wrote: > inline and name contents() Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:368: const std::vector<OffscreenTab*> Tab::GetOffscreenTabs() { On 2011/09/13 23:42:23, sky wrote: > inline and name offscreen_tabs(), also return a ref. Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:381: for (TabIterator it_tab = offscreen_tabs_.begin(); On 2011/09/13 23:42:23, sky wrote: > Use std::find Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:418: delete *it; On 2011/09/13 23:42:23, sky wrote: > STLDeleteElements Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:421: if (!GetMap()->RemoveTab(ExtensionTabUtil::GetTabId(contents_), &error_)) { On 2011/09/13 23:42:23, sky wrote: > Again, shouldn't this be a DCHECK? Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:426: UnregisterForTabNotifications(); On 2011/09/13 23:42:23, sky wrote: > Remove this, it's not needed. Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:428: delete this; On 2011/09/13 23:42:23, sky wrote: > This should be first, and everything in this method should be moved to the > destructor. That way if you delete a Tab, everything is cleaned up correctly. Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:455: On 2011/09/13 23:42:23, sky wrote: > remove this line. Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:465: if (map[tab_id] == NULL) { On 2011/09/13 23:42:23, sky wrote: > This inserts a NULL if tab_id isn't in the map. Instead use find. Done. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.h (right): http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.h:27: class CreateOffscreenTabFunction : public SyncExtensionFunction { On 2011/09/13 23:42:23, sky wrote: > Add descriptions for these classes. Done.
http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:174: if (profile && background_page_tab_contents == NULL) On 2011/09/14 21:45:30, alexbost wrote: > On 2011/09/13 23:42:23, sky wrote: > > What happens in a multi-profile world? Don't you need a background tab > contents > > per profile? > > This method is intended only for testing purposes. We are assuming that > offscreen tabs will not be created by background pages with the exception of the > API test background page. I will add a TODO for the multi-profile case. It looks like GetCurrentTab and OffscreenTab::Observe call into this, and they don't look testing specific. Or maybe I'm missing something? http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:210: url = extension->GetResourceURL(url_string); On 2011/09/14 21:45:30, alexbost wrote: > On 2011/09/13 23:42:23, sky wrote: > > This seems like it may have security implications I don't quite know. You > should > > talk with the extensions guy (Aaron, or Antony..) to make sure it's ok. > > I copied this function verbatim from the tabs API. Expose a method that both places can use rather copying. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:219: URLFixerUpper::FixupURL(url.possibly_invalid_spec(), std::string()); On 2011/09/14 21:45:30, alexbost wrote: > On 2011/09/13 23:42:23, sky wrote: > > Why do we need to fix this up? > > I copied this function verbatim from the tabs API. Expose a method that both places can use rather copying. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:65: ~OffscreenTab(); virtual http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:73: inline TabContents* contents(); Sorry, I didn't mean use the inline keyword, instead inline the implementation, eg: TabContents* contents() { return tab_.get(); } But note that the name should match the variable name. So, either rename to tab or rename tab_ to contents_. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:74: DictionaryValue* CreateValue(); // Creates an appropriate offscreen tab Document caller owns return value. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:98: // This class holds info about a tab that has spawned at least one offscreen tab 'tab' -> 'tab.' http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:105: ~Tab(); virtual http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:109: inline TabVector* offscreen_tabs(); const TabVector& http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:112: void AddOffscreenTab(OffscreenTab *tab); Document ownership for these two. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:122: TabContentsWrapper* tab_; // TabContentsWrapper associated with this tab. Your description says 'We will call such tab the 'parent'', so I would assume this variable is named parent too. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:253: return tab->view()->GetContainerSize(); At this point this function isn't saving you much. How about using tab->view()->GetContainSize() were appropriate. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:264: const Extension* extension, It doesn't look like you use extension or dispatcher. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:324: DCHECK(GetMap()->GetTabOfOffscreenTab(contents(), &tab, new std::string())); DCHECKs aren't compiled in release build, which means this whole line would not be compiled. Instead have DCHECK(*tab) on the next line. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:335: Profile* profile = Profile::FromBrowserContext( Get the profile from tab->tab_->profile(). http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:339: // Send a routed event directly to the parent tab. Move to line 341, and indent. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:353: Tab::Tab() {} initialize tab_ http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:358: DCHECK(GetMap()->RemoveTab( assign to a boolean, then DCHECK(removed). http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:363: tab_ = TabContentsWrapper::GetCurrentWrapperForContents(tab_contents); How do you know there is TabContentsWrapper associated with tab_contents? Might a background page use this api? http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:428: it != offscreen_tabs->end(); ++it) add {} here http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:522: if (map[tab_id] == NULL) { Use find to avoid insertion. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:565: Browser* browser = GetCurrentBrowser(); How do you know current browser is the one you want? http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:617: keys::kOffscreenTabNotFoundError, base::IntToString(tab_id)); indent 2 more.
http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:174: if (profile && background_page_tab_contents == NULL) On 2011/09/14 23:34:44, sky wrote: > On 2011/09/14 21:45:30, alexbost wrote: > > On 2011/09/13 23:42:23, sky wrote: > > > What happens in a multi-profile world? Don't you need a background tab > > contents > > > per profile? > > > > This method is intended only for testing purposes. We are assuming that > > offscreen tabs will not be created by background pages with the exception of > the > > API test background page. I will add a TODO for the multi-profile case. > > It looks like GetCurrentTab and OffscreenTab::Observe call into this, and they > don't look testing specific. Or maybe I'm missing something? The API tests (chrome/test/data/extensions/api_test/offscreen_tabs/test.html) call all API methods so GetCurrentTab and OffscreenTab::Observe get called during the tests. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:210: url = extension->GetResourceURL(url_string); On 2011/09/14 23:34:44, sky wrote: > On 2011/09/14 21:45:30, alexbost wrote: > > On 2011/09/13 23:42:23, sky wrote: > > > This seems like it may have security implications I don't quite know. You > > should > > > talk with the extensions guy (Aaron, or Antony..) to make sure it's ok. > > > > I copied this function verbatim from the tabs API. > > Expose a method that both places can use rather copying. At the extension API review meeting we decided that additional meetings are needed for long term redesign of the way extensions and tabs work and how that would integrate offscreen tabs. In the short term, however, we decided to move forward with checking the Offscreen Tabs API into experimental as it is. All methods that need refactoring are marked with TODOs, and we decided. http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:219: URLFixerUpper::FixupURL(url.possibly_invalid_spec(), std::string()); On 2011/09/14 23:34:44, sky wrote: > On 2011/09/14 21:45:30, alexbost wrote: > > On 2011/09/13 23:42:23, sky wrote: > > > Why do we need to fix this up? > > > > I copied this function verbatim from the tabs API. > > Expose a method that both places can use rather copying. See above. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:65: ~OffscreenTab(); On 2011/09/14 23:34:44, sky wrote: > virtual Done. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:73: inline TabContents* contents(); On 2011/09/14 23:34:44, sky wrote: > Sorry, I didn't mean use the inline keyword, instead inline the implementation, > eg: > > TabContents* contents() { return tab_.get(); } > > But note that the name should match the variable name. So, either rename to tab > or rename tab_ to contents_. I would like to have a method in this class to return TabContents instead of TabContentsWrapper since TabContents are needed to call various methods throughout the code. So maybe I should change the name back to GetContents()? This applies to the Tab class as well. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:74: DictionaryValue* CreateValue(); // Creates an appropriate offscreen tab On 2011/09/14 23:34:44, sky wrote: > Document caller owns return value. Done. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:98: // This class holds info about a tab that has spawned at least one offscreen tab On 2011/09/14 23:34:44, sky wrote: > 'tab' -> 'tab.' Done. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:105: ~Tab(); On 2011/09/14 23:34:44, sky wrote: > virtual Done. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:109: inline TabVector* offscreen_tabs(); On 2011/09/14 23:34:44, sky wrote: > const TabVector& Done. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:112: void AddOffscreenTab(OffscreenTab *tab); On 2011/09/14 23:34:44, sky wrote: > Document ownership for these two. Done. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:122: TabContentsWrapper* tab_; // TabContentsWrapper associated with this tab. On 2011/09/14 23:34:44, sky wrote: > Your description says 'We will call such tab the 'parent'', so I would assume > this variable is named parent too. Done. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:253: return tab->view()->GetContainerSize(); On 2011/09/14 23:34:44, sky wrote: > At this point this function isn't saving you much. How about using > tab->view()->GetContainSize() were appropriate. Done. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:264: const Extension* extension, On 2011/09/14 23:34:44, sky wrote: > It doesn't look like you use extension or dispatcher. Done. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:324: DCHECK(GetMap()->GetTabOfOffscreenTab(contents(), &tab, new std::string())); On 2011/09/14 23:34:44, sky wrote: > DCHECKs aren't compiled in release build, which means this whole line would not > be compiled. > Instead have DCHECK(*tab) on the next line. Done. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:335: Profile* profile = Profile::FromBrowserContext( On 2011/09/14 23:34:44, sky wrote: > Get the profile from tab->tab_->profile(). Done. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:339: // Send a routed event directly to the parent tab. On 2011/09/14 23:34:44, sky wrote: > Move to line 341, and indent. Done. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:353: Tab::Tab() {} On 2011/09/14 23:34:44, sky wrote: > initialize tab_ Done. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:358: DCHECK(GetMap()->RemoveTab( On 2011/09/14 23:34:44, sky wrote: > assign to a boolean, then DCHECK(removed). Done. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:363: tab_ = TabContentsWrapper::GetCurrentWrapperForContents(tab_contents); On 2011/09/14 23:34:44, sky wrote: > How do you know there is TabContentsWrapper associated with tab_contents? Might > a background page use this api? I create an artificial TabContentsWrapper in GetBackgroundPageTabContents. We are assuming the only background page that will use the API is the API test's one. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:428: it != offscreen_tabs->end(); ++it) On 2011/09/14 23:34:44, sky wrote: > add {} here Done. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:522: if (map[tab_id] == NULL) { On 2011/09/14 23:34:44, sky wrote: > Use find to avoid insertion. Done. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:565: Browser* browser = GetCurrentBrowser(); On 2011/09/14 23:34:44, sky wrote: > How do you know current browser is the one you want? What would you use instead? Notice, we only use the browser to set default width and height when they are not provided by the application. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:617: keys::kOffscreenTabNotFoundError, base::IntToString(tab_id)); On 2011/09/14 23:34:44, sky wrote: > indent 2 more. Done.
http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... 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/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:73: void SetUrl(const GURL& url); nit: We tend to use SetURL rather than SetUrl http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:105: const std::string& GetExtensionId() const { return extension_id_; } GetExtensionId --> extension_id since it's inline http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:302: DCHECK_EQ(content::NOTIFICATION_NAV_ENTRY_COMMITTED, type); You can convert these to CHECKs and CHECK_EQs if there's no performance problems. If these only get triggered from programming errors, then we might as well fail quickly in release builds too rather than having them show up as obscure crashes later. http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:316: GetMap()->GetTabOfOffscreenTab(contents(), &tab, new std::string()); Can you just have each OffscreenTab hold a pointer to its parent instead of having a separate map? http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:324: nit: remove new line? http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:682: DictionaryValue* dict = NULL; nit: maybe rename dict to something more informative? like event_info or js_event? (same goes for the mouse events) http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:921: // hash map in order to get offscreen tabs in O(1). Do you know how fast the actual TabContentsWrapper::CaptureSnapshot() method is? How many offscreen tabs do you expect to iterate over? http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:926: nit: delete new line http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:1207: const std::string& error) { nit: indent http://codereview.chromium.org/7720002/diff/53019/chrome/common/extensions/ap... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7720002/diff/53019/chrome/common/extensions/ap... chrome/common/extensions/api/extension_api.json:8691: "description": "When format is 'jpeg', controls the quality of the resulting image. This value is ignored for PNG images. As quality is decreased, the resulting image will have more visual artifacts, and the number of bytes needed to store it will decrease." Are the jpeg images produced any faster? How should an extension decide which image type and quality to use?
http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:64: ~OffscreenTab(); On 2011/09/15 18:52:01, jstritar wrote: > The destructors should be virtual (not Init). Done. http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:73: void SetUrl(const GURL& url); On 2011/09/15 18:52:01, jstritar wrote: > nit: We tend to use SetURL rather than SetUrl Done. http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:105: const std::string& GetExtensionId() const { return extension_id_; } On 2011/09/15 18:52:01, jstritar wrote: > GetExtensionId --> extension_id since it's inline Done. http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:302: DCHECK_EQ(content::NOTIFICATION_NAV_ENTRY_COMMITTED, type); On 2011/09/15 18:52:01, jstritar wrote: > You can convert these to CHECKs and CHECK_EQs if there's no performance > problems. If these only get triggered from programming errors, then we might as > well fail quickly in release builds too rather than having them show up as > obscure crashes later. Done. http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:316: GetMap()->GetTabOfOffscreenTab(contents(), &tab, new std::string()); On 2011/09/15 18:52:01, jstritar wrote: > Can you just have each OffscreenTab hold a pointer to its parent instead of > having a separate map? Done. http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:324: On 2011/09/15 18:52:01, jstritar wrote: > nit: remove new line? Done. http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:682: DictionaryValue* dict = NULL; On 2011/09/15 18:52:01, jstritar wrote: > nit: maybe rename dict to something more informative? like event_info or > js_event? (same goes for the mouse events) Done. http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:921: // hash map in order to get offscreen tabs in O(1). On 2011/09/15 18:52:01, jstritar wrote: > Do you know how fast the actual TabContentsWrapper::CaptureSnapshot() method is? > How many offscreen tabs do you expect to iterate over? I haven't tested it but it depends on the image width and height, and quality in the case of jpeg). http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:926: On 2011/09/15 18:52:01, jstritar wrote: > nit: delete new line Done. http://codereview.chromium.org/7720002/diff/53019/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:1207: const std::string& error) { On 2011/09/15 18:52:01, jstritar wrote: > nit: indent Done. http://codereview.chromium.org/7720002/diff/53019/chrome/common/extensions/ap... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7720002/diff/53019/chrome/common/extensions/ap... chrome/common/extensions/api/extension_api.json:8691: "description": "When format is 'jpeg', controls the quality of the resulting image. This value is ignored for PNG images. As quality is decreased, the resulting image will have more visual artifacts, and the number of bytes needed to store it will decrease." On 2011/09/15 18:52:01, jstritar wrote: > Are the jpeg images produced any faster? How should an extension decide which > image type and quality to use? I need to test this further but from my observations a jpeg with low quality gets to the application faster because the dataUrl is smaller.
http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:174: if (profile && background_page_tab_contents == NULL) On 2011/09/15 03:29:49, alexbost wrote: > On 2011/09/14 23:34:44, sky wrote: > > On 2011/09/14 21:45:30, alexbost wrote: > > > On 2011/09/13 23:42:23, sky wrote: > > > > What happens in a multi-profile world? Don't you need a background tab > > > contents > > > > per profile? > > > > > > This method is intended only for testing purposes. We are assuming that > > > offscreen tabs will not be created by background pages with the exception of > > the > > > API test background page. I will add a TODO for the multi-profile case. > > > > It looks like GetCurrentTab and OffscreenTab::Observe call into this, and they > > don't look testing specific. Or maybe I'm missing something? > > The API tests (chrome/test/data/extensions/api_test/offscreen_tabs/test.html) > call all API methods so GetCurrentTab and OffscreenTab::Observe get called > during the tests. Aren't GetCurrentTab and OffscreenTab::Observe used by non-testing code paths too though? http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:73: inline TabContents* contents(); On 2011/09/15 03:29:49, alexbost wrote: > On 2011/09/14 23:34:44, sky wrote: > > Sorry, I didn't mean use the inline keyword, instead inline the > implementation, > > eg: > > > > TabContents* contents() { return tab_.get(); } > > > > But note that the name should match the variable name. So, either rename to > tab > > or rename tab_ to contents_. > > I would like to have a method in this class to return TabContents instead of > TabContentsWrapper since TabContents are needed to call various methods > throughout the code. So maybe I should change the name back to GetContents()? > This applies to the Tab class as well. The names only need match if you're returning a field. If you're not returning a field, you can use something else. Maybe: TabContents* contents() { return tab_->tab_contents(); } http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:363: tab_ = TabContentsWrapper::GetCurrentWrapperForContents(tab_contents); On 2011/09/15 03:29:49, alexbost wrote: > On 2011/09/14 23:34:44, sky wrote: > > How do you know there is TabContentsWrapper associated with tab_contents? > Might > > a background page use this api? > > I create an artificial TabContentsWrapper in GetBackgroundPageTabContents. We > are assuming the only background page that will use the API is the API test's > one. By background page I meant one of these: http://code.google.com/chrome/extensions/background_pages.html . I'm not sure if such background pages always have a TabContentsWrapper associated with them. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:565: Browser* browser = GetCurrentBrowser(); On 2011/09/15 03:29:49, alexbost wrote: > On 2011/09/14 23:34:44, sky wrote: > > How do you know current browser is the one you want? > > What would you use instead? Notice, we only use the browser to set default width > and height when they are not provided by the application. Maybe the size of the tab?
http://codereview.chromium.org/7720002/diff/58001/chrome/common/extensions/ap... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7720002/diff/58001/chrome/common/extensions/ap... chrome/common/extensions/api/extension_api.json:8602: "type": "any", How about you define this as an object and specify the valid properties? This would get us some more validation and documentation for free. You could also do this for the keyboard events.
http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:174: if (profile && background_page_tab_contents == NULL) On 2011/09/15 20:05:57, sky wrote: > On 2011/09/15 03:29:49, alexbost wrote: > > On 2011/09/14 23:34:44, sky wrote: > > > On 2011/09/14 21:45:30, alexbost wrote: > > > > On 2011/09/13 23:42:23, sky wrote: > > > > > What happens in a multi-profile world? Don't you need a background tab > > > > contents > > > > > per profile? > > > > > > > > This method is intended only for testing purposes. We are assuming that > > > > offscreen tabs will not be created by background pages with the exception > of > > > the > > > > API test background page. I will add a TODO for the multi-profile case. > > > > > > It looks like GetCurrentTab and OffscreenTab::Observe call into this, and > they > > > don't look testing specific. Or maybe I'm missing something? > > > > The API tests (chrome/test/data/extensions/api_test/offscreen_tabs/test.html) > > call all API methods so GetCurrentTab and OffscreenTab::Observe get called > > during the tests. > > Aren't GetCurrentTab and OffscreenTab::Observe used by non-testing code paths > too though? They are. I am not sure what you are suggesting though. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:73: inline TabContents* contents(); On 2011/09/15 20:05:57, sky wrote: > On 2011/09/15 03:29:49, alexbost wrote: > > On 2011/09/14 23:34:44, sky wrote: > > > Sorry, I didn't mean use the inline keyword, instead inline the > > implementation, > > > eg: > > > > > > TabContents* contents() { return tab_.get(); } > > > > > > But note that the name should match the variable name. So, either rename to > > tab > > > or rename tab_ to contents_. > > > > I would like to have a method in this class to return TabContents instead of > > TabContentsWrapper since TabContents are needed to call various methods > > throughout the code. So maybe I should change the name back to GetContents()? > > This applies to the Tab class as well. > > The names only need match if you're returning a field. If you're not returning a > field, you can use something else. Maybe: > > TabContents* contents() { return tab_->tab_contents(); } Done. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:363: tab_ = TabContentsWrapper::GetCurrentWrapperForContents(tab_contents); On 2011/09/15 20:05:57, sky wrote: > On 2011/09/15 03:29:49, alexbost wrote: > > On 2011/09/14 23:34:44, sky wrote: > > > How do you know there is TabContentsWrapper associated with tab_contents? > > Might > > > a background page use this api? > > > > I create an artificial TabContentsWrapper in GetBackgroundPageTabContents. We > > are assuming the only background page that will use the API is the API test's > > one. > > By background page I meant one of these: > http://code.google.com/chrome/extensions/background_pages.html . I'm not sure if > such background pages always have a TabContentsWrapper associated with them. I think we are talking about the same background page concept. I create a TabContentsWrapper in GetBackgroundPageTabContents (line 199). http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:565: Browser* browser = GetCurrentBrowser(); On 2011/09/15 20:05:57, sky wrote: > On 2011/09/15 03:29:49, alexbost wrote: > > On 2011/09/14 23:34:44, sky wrote: > > > How do you know current browser is the one you want? > > > > What would you use instead? Notice, we only use the browser to set default > width > > and height when they are not provided by the application. > > Maybe the size of the tab? Good catch. http://codereview.chromium.org/7720002/diff/58001/chrome/common/extensions/ap... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7720002/diff/58001/chrome/common/extensions/ap... chrome/common/extensions/api/extension_api.json:8602: "type": "any", On 2011/09/15 20:09:34, jstritar wrote: > How about you define this as an object and specify the valid properties? This > would get us some more validation and documentation for free. You could also do > this for the keyboard events. I am not sure if that is simple enough to do. One problem is that some of the properties of MouseEvent and KeyboardEvent are objects themselves. Currently, I do pre-validation to get rid of those objects in order to be able to serialize the event object. See extension_process_bindings.js.
http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:174: if (profile && background_page_tab_contents == NULL) On 2011/09/15 20:57:57, alexbost wrote: > On 2011/09/15 20:05:57, sky wrote: > > On 2011/09/15 03:29:49, alexbost wrote: > > > On 2011/09/14 23:34:44, sky wrote: > > > > On 2011/09/14 21:45:30, alexbost wrote: > > > > > On 2011/09/13 23:42:23, sky wrote: > > > > > > What happens in a multi-profile world? Don't you need a background tab > > > > > contents > > > > > > per profile? > > > > > > > > > > This method is intended only for testing purposes. We are assuming that > > > > > offscreen tabs will not be created by background pages with the > exception > > of > > > > the > > > > > API test background page. I will add a TODO for the multi-profile case. > > > > > > > > It looks like GetCurrentTab and OffscreenTab::Observe call into this, and > > they > > > > don't look testing specific. Or maybe I'm missing something? > > > > > > The API tests > (chrome/test/data/extensions/api_test/offscreen_tabs/test.html) > > > call all API methods so GetCurrentTab and OffscreenTab::Observe get called > > > during the tests. > > > > Aren't GetCurrentTab and OffscreenTab::Observe used by non-testing code paths > > too though? > > They are. I am not sure what you are suggesting though. GetBackgroundTabContents creates and returns a TabContents all the time with a specific profile. You said the TabContents is only used for testing, yet it seems as though it's created all the time and leaked. If it's only used for testing it should be made sure that it's only created then.
http://codereview.chromium.org/7720002/diff/58001/chrome/common/extensions/ap... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7720002/diff/58001/chrome/common/extensions/ap... chrome/common/extensions/api/extension_api.json:8602: "type": "any", On 2011/09/15 20:57:57, alexbost wrote: > On 2011/09/15 20:09:34, jstritar wrote: > > How about you define this as an object and specify the valid properties? This > > would get us some more validation and documentation for free. You could also > do > > this for the keyboard events. > > I am not sure if that is simple enough to do. One problem is that some of the > properties of MouseEvent and KeyboardEvent are objects themselves. Currently, I > do pre-validation to get rid of those objects in order to be able to serialize > the event object. See extension_process_bindings.js. Do you think extensions should create MouseEvents and pass them in, or just pass in a property object like you did in the tests?
http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:363: tab_ = TabContentsWrapper::GetCurrentWrapperForContents(tab_contents); On 2011/09/15 20:57:57, alexbost wrote: > On 2011/09/15 20:05:57, sky wrote: > > On 2011/09/15 03:29:49, alexbost wrote: > > > On 2011/09/14 23:34:44, sky wrote: > > > > How do you know there is TabContentsWrapper associated with tab_contents? > > > Might > > > > a background page use this api? > > > > > > I create an artificial TabContentsWrapper in GetBackgroundPageTabContents. > We > > > are assuming the only background page that will use the API is the API > test's > > > one. > > > > By background page I meant one of these: > > http://code.google.com/chrome/extensions/background_pages.html . I'm not sure > if > > such background pages always have a TabContentsWrapper associated with them. > > I think we are talking about the same background page concept. I create a > TabContentsWrapper in GetBackgroundPageTabContents (line 199). If you're saying that all background pages are routed through you're new API, than that's fine. But if the existing extensions API is creating a background TabContents some where else that doesn't have a TabContentsWrapper associated with it, there would be problems.
http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/39001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:174: if (profile && background_page_tab_contents == NULL) On 2011/09/15 21:39:17, sky wrote: > On 2011/09/15 20:57:57, alexbost wrote: > > On 2011/09/15 20:05:57, sky wrote: > > > On 2011/09/15 03:29:49, alexbost wrote: > > > > On 2011/09/14 23:34:44, sky wrote: > > > > > On 2011/09/14 21:45:30, alexbost wrote: > > > > > > On 2011/09/13 23:42:23, sky wrote: > > > > > > > What happens in a multi-profile world? Don't you need a background > tab > > > > > > contents > > > > > > > per profile? > > > > > > > > > > > > This method is intended only for testing purposes. We are assuming > that > > > > > > offscreen tabs will not be created by background pages with the > > exception > > > of > > > > > the > > > > > > API test background page. I will add a TODO for the multi-profile > case. > > > > > > > > > > It looks like GetCurrentTab and OffscreenTab::Observe call into this, > and > > > they > > > > > don't look testing specific. Or maybe I'm missing something? > > > > > > > > The API tests > > (chrome/test/data/extensions/api_test/offscreen_tabs/test.html) > > > > call all API methods so GetCurrentTab and OffscreenTab::Observe get called > > > > during the tests. > > > > > > Aren't GetCurrentTab and OffscreenTab::Observe used by non-testing code > paths > > > too though? > > > > They are. I am not sure what you are suggesting though. > > GetBackgroundTabContents creates and returns a TabContents all the time with a > specific profile. You said the TabContents is only used for testing, yet it > seems as though it's created all the time and leaked. If it's only used for > testing it should be made sure that it's only created then. GetBackgroundTabContents is (now) only called from GetCurrentTab when there are no associated TabContents, which should make things more obvious. About leaking, notice that the API test removes all offscreen tabs at the end which causes the fake TabContents for the background page to be deleted too. http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/44006/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:363: tab_ = TabContentsWrapper::GetCurrentWrapperForContents(tab_contents); On 2011/09/15 21:41:36, sky wrote: > On 2011/09/15 20:57:57, alexbost wrote: > > On 2011/09/15 20:05:57, sky wrote: > > > On 2011/09/15 03:29:49, alexbost wrote: > > > > On 2011/09/14 23:34:44, sky wrote: > > > > > How do you know there is TabContentsWrapper associated with > tab_contents? > > > > Might > > > > > a background page use this api? > > > > > > > > I create an artificial TabContentsWrapper in GetBackgroundPageTabContents. > > We > > > > are assuming the only background page that will use the API is the API > > test's > > > > one. > > > > > > By background page I meant one of these: > > > http://code.google.com/chrome/extensions/background_pages.html . I'm not > sure > > if > > > such background pages always have a TabContentsWrapper associated with them. > > > > I think we are talking about the same background page concept. I create a > > TabContentsWrapper in GetBackgroundPageTabContents (line 199). > > If you're saying that all background pages are routed through you're new API, > than that's fine. But if the existing extensions API is creating a background > TabContents some where else that doesn't have a TabContentsWrapper associated > with it, there would be problems. If a background page ends up calling this method, then we will already have created not only a fake TabContentsWrapper for the background page but also a fake TabContents. See GetBackgroundPageTabContents. I've added CHECKs to verify this logic. http://codereview.chromium.org/7720002/diff/58001/chrome/common/extensions/ap... File chrome/common/extensions/api/extension_api.json (right): http://codereview.chromium.org/7720002/diff/58001/chrome/common/extensions/ap... chrome/common/extensions/api/extension_api.json:8602: "type": "any", On 2011/09/15 21:40:33, jstritar wrote: > On 2011/09/15 20:57:57, alexbost wrote: > > On 2011/09/15 20:09:34, jstritar wrote: > > > How about you define this as an object and specify the valid properties? > This > > > would get us some more validation and documentation for free. You could also > > do > > > this for the keyboard events. > > > > I am not sure if that is simple enough to do. One problem is that some of the > > properties of MouseEvent and KeyboardEvent are objects themselves. Currently, > I > > do pre-validation to get rid of those objects in order to be able to serialize > > the event object. See extension_process_bindings.js. > > Do you think extensions should create MouseEvents and pass them in, or just pass > in a property object like you did in the tests? We have actually had long discussions about this :) We thought passing JavaScript Mouse/KeyboardEvent objects would be more convenient for the application developer since we can directly use the properties of the event objects (with the exception of x and y for the mouse).
http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_apitest.cc (right): http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_apitest.cc:14: } I remember having problems setting the experimental flag like this before- maybe the flag not propagating to the renderers... don't remember exactly. We often use a pattern like this: class ExperimentalApiTest : public ExtensionApiTest { public: void SetUpCommandLine(CommandLine* command_line) { ExtensionApiTest::SetUpCommandLine(command_line); command_line->AppendSwitch(switches::kEnableExperimentalExtensionApis); } }; IN_PROC_BROWSER_TEST_F(ExperimentalApiTest, OffscreenTabs) { ASSERT_TRUE(RunExtensionTest("offscreen_tabs")) << message_; } http://codereview.chromium.org/7720002/diff/65001/chrome/renderer/resources/e... File chrome/renderer/resources/extension_process_bindings.js (right): http://codereview.chromium.org/7720002/diff/65001/chrome/renderer/resources/e... chrome/renderer/resources/extension_process_bindings.js:1028: for (prop in arg1) nit: not required but might be easier to read to use {} in the for loop http://codereview.chromium.org/7720002/diff/65001/chrome/renderer/resources/e... chrome/renderer/resources/extension_process_bindings.js:1038: updateArgumentsPreValidate; apiFunctions["experimental.offscreenTabs.sendMouseEvent"]. updateArgumentsPreValidate = apiFunctions["experimental.offscreenTabs.sendKeyboardEvent"]. updateArgumentsPreValidate; http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/offscreen_tabs/test.js (right): http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:6: // browser_tests.exe --gtest_filter=ExtensionApiTest.OffscreenTabs ExperimentalApiTest http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:20: }, style nits: spaces after the ':' { "url": extensionPath + "a.html", "width": 200, "height": 200 }, ... same for the other places. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:31: // Tab management can you make the comment more useful? or remove it? http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:35: remove new line http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:39: "button":0, "button": 0, etc.. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:49: ditto on these comments. they don't add much right now. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:88: var j = i; indent the function body 2 spaces less http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:104: assertEq(tabs.length, tabsResult.length); indent 2 spaces less http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:155: chrome.experimental.offscreenTabs.onUpdated.addListener(listener = We have a helper method, chrome.test.listenOnce, to make this pattern easier (it also fixes the race condition). See this for an example of it used in the tabs API test: http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/test/data/exte... http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:227: function removeMouse() { see comments about removeKeyboard http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:236: chrome.experimental.offscreenTabs.onUpdated.addListener(listener = chrome.test.listenOnce http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:236: chrome.experimental.offscreenTabs.onUpdated.addListener(listener = Why do you wait for onUpdated here? Why not just call keyPress from inside create's callback? Are you testing that onUpdated is invoked when creating an offscreen tab? http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:250: ); I'm not sure about this but it's probably safer to just have a callback here to make sure the test waits properly. chrome.experimental.offscreenTabs.create({}, pass(function() { assertTrue(true) })); http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:254: chrome.experimental.offscreenTabs.onUpdated.addListener(listener = ditto on chrome.test.listenOnce http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:276: function removeKeyboard() { I don't think this needs to be its own method since it's only called from one place and the API call is pretty self explanatory. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:279: verifyTabDoesNotExist(tabKeyboard.id); There's a race condition here. The tab may not really be removed until the callback is invoked. chrome.experimental.offscreenTabs.remove(tabKeyboard.id, pass(function() { verifyTabDoesNotExist(tabKeyboard.id); })); http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:350: function run() { You don't need the run() method-- the chrome.test.runTests part will start the test. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:352: function tabManagement() { I think you have race conditions in these tests- the test runner will move on to the next test while you're waiting for the callbacks to fire. We have a helper method, chrome.test.callbackPass, to handle this situation. You wrap your callback functions with it and the test runner will count how many callbacks you have and wait for them to be executed before moving on to the next test. They would look like this since you've assigned callbackPass to pass: chrome.experimental.offscreenTabs.toDataUrl(..., pass(function(dataUrl) { })); http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:353: startTabManagement(); I think these tests would be easier to follow if you call the offscreenTab APIs directly from these methods, rather than jumping to the helpers. (I may be wrong though since you might get too much indentation. It's up to you!) You can see examples of our test style in these tab tests (each of those html files is a little test suite, so there are lots of examples there): http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/test/data/exte... http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:355: , nit: comma right after the '}' like this }, http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:356: function mouseEvents() { Can you add comments to each of these to make clear what you're testing?
http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:63: class OffscreenTab : public NotificationObserver { You have way too many things named tab in this file to the point that it makes it hard to follow. See Map::RemoveOffscreenTab as an example. Here is what it has: tab_id: the id off the offscreen tab. offscreen_tab: yes, this name is perfect! tab: the parent tab, doesn't relate to tab_id. In other places you use 'tab' to mean OffscreenTab or even TabContentsWrapper. Oy! Be consistent. The following would make it a lot easier to read this code: OffscreenTab (great name!), use variables offscreen_tab and offscreen_tab_id to match. Tab: very ambiguous name, maybe it should be ParentTab to match the SetParentTab method you have. Then use variables named parent_tab and parent_tab_id. TabContents/TabContentsWrapper I'm fine with using tab for these, as long as you don't use tab for Tab or OffscreenTab. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:74: DictionaryValue* CreateValue(); // The caller owns the returned value. Document what this does too. Write comments assuming someone is looking at this code for the first time. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:162: bool RemoveTab(const int tab_id, std::string* error_message); Document ownership. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:206: bool GetCurrentTab(ExtensionFunctionDispatcher* dispatcher, Name GetCurrentTabContents to match GetBackgroundPageTabContents. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:214: *tab_contents = GetBackgroundPageTabContents(profile); Are we sure this is only going to get hit for testing? Can you add a CHECK to that effect? http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:252: OffscreenTab::OffscreenTab() {} member initialize parent_tab_. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:295: void OffscreenTab::SetParentTab(Tab* tab) { Change this to set_parent_tab and inline it (like you did for parent_tab()). http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:329: parent_tab_->contents()->render_view_host()->process()->Send( I'm not sure what happens if the tab crashes. The process() may be null in that case. You'll have to try it and make sure this code doesn't crash if that happens. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:342: tab_ = NULL; Use member initializer list. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:458: offscreen_tab->SetParentTab(tab); Shouldn't the parent be passed to Init? Otherwise it implies the parent can change, which you don't really want, right? http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:480: return true; Who takes ownership of offscreen_tab?
http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_apitest.cc (right): http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_apitest.cc:14: } On 2011/09/16 16:03:50, jstritar wrote: > I remember having problems setting the experimental flag like this before- maybe > the flag not propagating to the renderers... don't remember exactly. We often > use a pattern like this: > > class ExperimentalApiTest : public ExtensionApiTest { > public: > void SetUpCommandLine(CommandLine* command_line) { > ExtensionApiTest::SetUpCommandLine(command_line); > command_line->AppendSwitch(switches::kEnableExperimentalExtensionApis); > } > }; > > IN_PROC_BROWSER_TEST_F(ExperimentalApiTest, OffscreenTabs) { > ASSERT_TRUE(RunExtensionTest("offscreen_tabs")) << message_; > } I had that problem and fixed it by adding the API name here: chrome/renderer/resources/renderer_extension_bindings.js http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:63: class OffscreenTab : public NotificationObserver { On 2011/09/16 16:29:21, sky wrote: > You have way too many things named tab in this file to the point that it makes > it hard to follow. See Map::RemoveOffscreenTab as an example. Here is what it > has: > > tab_id: the id off the offscreen tab. > offscreen_tab: yes, this name is perfect! > tab: the parent tab, doesn't relate to tab_id. > > In other places you use 'tab' to mean OffscreenTab or even TabContentsWrapper. > Oy! Be consistent. The following would make it a lot easier to read this code: > > OffscreenTab (great name!), use variables offscreen_tab and offscreen_tab_id to > match. > Tab: very ambiguous name, maybe it should be ParentTab to match the SetParentTab > method you have. Then use variables named parent_tab and parent_tab_id. > TabContents/TabContentsWrapper I'm fine with using tab for these, as long as you > don't use tab for Tab or OffscreenTab. I completely agree :) I was trying to keep names short - not a good trade-off in this case. Done. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:74: DictionaryValue* CreateValue(); // The caller owns the returned value. On 2011/09/16 16:29:21, sky wrote: > Document what this does too. Write comments assuming someone is looking at this > code for the first time. Done. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:162: bool RemoveTab(const int tab_id, std::string* error_message); On 2011/09/16 16:29:21, sky wrote: > Document ownership. Done. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:206: bool GetCurrentTab(ExtensionFunctionDispatcher* dispatcher, On 2011/09/16 16:29:21, sky wrote: > Name GetCurrentTabContents to match GetBackgroundPageTabContents. Done. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:214: *tab_contents = GetBackgroundPageTabContents(profile); On 2011/09/16 16:29:21, sky wrote: > Are we sure this is only going to get hit for testing? Can you add a CHECK to > that effect? It does get hit because the test runs in a background page which does not have associated tab contents. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:252: OffscreenTab::OffscreenTab() {} On 2011/09/16 16:29:21, sky wrote: > member initialize parent_tab_. Done. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:295: void OffscreenTab::SetParentTab(Tab* tab) { On 2011/09/16 16:29:21, sky wrote: > Change this to set_parent_tab and inline it (like you did for parent_tab()). Done. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:329: parent_tab_->contents()->render_view_host()->process()->Send( On 2011/09/16 16:29:21, sky wrote: > I'm not sure what happens if the tab crashes. The process() may be null in that > case. You'll have to try it and make sure this code doesn't crash if that > happens. Done. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:342: tab_ = NULL; On 2011/09/16 16:29:21, sky wrote: > Use member initializer list. Done. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:458: offscreen_tab->SetParentTab(tab); On 2011/09/16 16:29:21, sky wrote: > Shouldn't the parent be passed to Init? Otherwise it implies the parent can > change, which you don't really want, right? Done. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:480: return true; On 2011/09/16 16:29:21, sky wrote: > Who takes ownership of offscreen_tab? Done. http://codereview.chromium.org/7720002/diff/65001/chrome/renderer/resources/e... File chrome/renderer/resources/extension_process_bindings.js (right): http://codereview.chromium.org/7720002/diff/65001/chrome/renderer/resources/e... chrome/renderer/resources/extension_process_bindings.js:1028: for (prop in arg1) On 2011/09/16 16:03:50, jstritar wrote: > nit: not required but might be easier to read to use {} in the for loop Done. http://codereview.chromium.org/7720002/diff/65001/chrome/renderer/resources/e... chrome/renderer/resources/extension_process_bindings.js:1038: updateArgumentsPreValidate; On 2011/09/16 16:03:50, jstritar wrote: > apiFunctions["experimental.offscreenTabs.sendMouseEvent"]. > updateArgumentsPreValidate = > apiFunctions["experimental.offscreenTabs.sendKeyboardEvent"]. > updateArgumentsPreValidate; Done. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/offscreen_tabs/test.js (right): http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:6: // browser_tests.exe --gtest_filter=ExtensionApiTest.OffscreenTabs On 2011/09/16 16:03:50, jstritar wrote: > ExperimentalApiTest Done. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:20: }, On 2011/09/16 16:03:50, jstritar wrote: > style nits: spaces after the ':' > > { > "url": extensionPath + "a.html", > "width": 200, > "height": 200 > }, > ... > > same for the other places. Done. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:31: // Tab management On 2011/09/16 16:03:50, jstritar wrote: > can you make the comment more useful? or remove it? Done. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:35: On 2011/09/16 16:03:50, jstritar wrote: > remove new line Done. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:39: "button":0, On 2011/09/16 16:03:50, jstritar wrote: > "button": 0, etc.. Done. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:49: On 2011/09/16 16:03:50, jstritar wrote: > ditto on these comments. they don't add much right now. Done. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:88: var j = i; On 2011/09/16 16:03:50, jstritar wrote: > indent the function body 2 spaces less Done. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:104: assertEq(tabs.length, tabsResult.length); On 2011/09/16 16:03:50, jstritar wrote: > indent 2 spaces less Done. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:155: chrome.experimental.offscreenTabs.onUpdated.addListener(listener = On 2011/09/16 16:03:50, jstritar wrote: > We have a helper method, chrome.test.listenOnce, to make this pattern easier (it > also fixes the race condition). See this for an example of it used in the tabs > API test: > > http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/test/data/exte... Bingo! (Not completely sure if it will work in my case but I will give it a better try later.) http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:227: function removeMouse() { On 2011/09/16 16:03:50, jstritar wrote: > see comments about removeKeyboard Done. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:236: chrome.experimental.offscreenTabs.onUpdated.addListener(listener = On 2011/09/16 16:03:50, jstritar wrote: > chrome.test.listenOnce Done. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:236: chrome.experimental.offscreenTabs.onUpdated.addListener(listener = On 2011/09/16 16:03:50, jstritar wrote: > Why do you wait for onUpdated here? Why not just call keyPress from inside > create's callback? Are you testing that onUpdated is invoked when creating an > offscreen tab? Because create might return before the page has navigated. We should probably return once it has navigated. I will add a TODO in the module .cc file. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:250: ); On 2011/09/16 16:03:50, jstritar wrote: > I'm not sure about this but it's probably safer to just have a callback here to > make sure the test waits properly. > > chrome.experimental.offscreenTabs.create({}, pass(function() { > assertTrue(true) > })); I had some trouble using the pass function with the way my tests are set up, so I left it out. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:254: chrome.experimental.offscreenTabs.onUpdated.addListener(listener = On 2011/09/16 16:03:50, jstritar wrote: > ditto on chrome.test.listenOnce Done. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:276: function removeKeyboard() { On 2011/09/16 16:03:50, jstritar wrote: > I don't think this needs to be its own method since it's only called from one > place and the API call is pretty self explanatory. Done. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:279: verifyTabDoesNotExist(tabKeyboard.id); On 2011/09/16 16:03:50, jstritar wrote: > There's a race condition here. The tab may not really be removed until the > callback is invoked. > > chrome.experimental.offscreenTabs.remove(tabKeyboard.id, pass(function() { > verifyTabDoesNotExist(tabKeyboard.id); > })); Done. BTW, I think there is a bug in the tabs API - remove doesn't return a callback. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:350: function run() { On 2011/09/16 16:03:50, jstritar wrote: > You don't need the run() method-- the chrome.test.runTests part will start the > test. Done. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:352: function tabManagement() { On 2011/09/16 16:03:50, jstritar wrote: > I think you have race conditions in these tests- the test runner will move on to > the next test while you're waiting for the callbacks to fire. > > We have a helper method, chrome.test.callbackPass, to handle this situation. You > wrap your callback functions with it and the test runner will count how many > callbacks you have and wait for them to be executed before moving on to the next > test. > > They would look like this since you've assigned callbackPass to pass: > > chrome.experimental.offscreenTabs.toDataUrl(..., pass(function(dataUrl) { > > })); Unfortunately, I haven't been able to use the pass function successfully in my tests. I actually get synchronicity problems if I do use it. I.e. I would get "All tests succeeded" multiple times or even when tests fail. Maybe there is something wrong with my general setup? But, right now the tests seem to run one after the other as opposed to in parallel, so I don't think there are race conditions. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:353: startTabManagement(); On 2011/09/16 16:03:50, jstritar wrote: > I think these tests would be easier to follow if you call the offscreenTab APIs > directly from these methods, rather than jumping to the helpers. (I may be wrong > though since you might get too much indentation. It's up to you!) > > You can see examples of our test style in these tab tests (each of those html > files is a little test suite, so there are lots of examples there): > > http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/test/data/exte... > Yes, I tried to stay away from the multiple levels of indentation. For example the Tab Management would have 5 levels. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:355: , On 2011/09/16 16:03:50, jstritar wrote: > nit: comma right after the '}' like this }, Done. http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:356: function mouseEvents() { On 2011/09/16 16:03:50, jstritar wrote: > Can you add comments to each of these to make clear what you're testing? Done.
http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:214: *tab_contents = GetBackgroundPageTabContents(profile); On 2011/09/16 20:18:14, alexbost wrote: > On 2011/09/16 16:29:21, sky wrote: > > Are we sure this is only going to get hit for testing? Can you add a CHECK to > > that effect? > > It does get hit because the test runs in a background page which does not have > associated tab contents. What prevents a non-test background page from doing this?
Looking much better. http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:116: // Caller assumes ownership of OffscreenTab. Update comment. This deletes the OffscreenTab. http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:330: Profile* profile = TabContentsWrapper::GetCurrentWrapperForContents( Only use GetCurrentWrapperForContents if necessary. Either expose the TabContentsWrapper in ParentTab or the profile directly. http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:337: && parent_tab_->contents()->render_view_host()->process()) && on the previous line and fix indenting. http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:360: CHECK(removed); We generally only use CHECK until there is a security issue, or we're trying to narrow down a crasher. I don't think that fits any of the cases for you in this file. http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:422: TabVector offscreen_tabs = parent_tab->offscreen_tabs(); const TabVector& http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:425: it != offscreen_tabs.end(); ++it) { indent one more space http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:469: parent_tab = map[offscreen_tab_id] = new ParentTab(); Separate this into two assignments. http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:492: parent_tab->RemoveOffscreenTab(offscreen_tab); nit: set offscreen_tab = NULL here as it's been deleted. http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:558: EXTENSION_FUNCTION_VALIDATE( Do these need to make sure the width/height is > 0 ? http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:627: TabVector offscreen_tabs = parent_tab->offscreen_tabs(); const TabVector& http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:985: TabContentsWrapper* tab_wrapper = TabContentsWrapper:: See comment earlier about using GetCurrentWrapperForContents here. It isn't necessary.
http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/offscreen_tabs/test.js (right): http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:352: function tabManagement() { On 2011/09/16 20:18:14, alexbost wrote: > On 2011/09/16 16:03:50, jstritar wrote: > > I think you have race conditions in these tests- the test runner will move on > to > > the next test while you're waiting for the callbacks to fire. > > > > We have a helper method, chrome.test.callbackPass, to handle this situation. > You > > wrap your callback functions with it and the test runner will count how many > > callbacks you have and wait for them to be executed before moving on to the > next > > test. > > > > They would look like this since you've assigned callbackPass to pass: > > > > chrome.experimental.offscreenTabs.toDataUrl(..., pass(function(dataUrl) { > > > > })); > > Unfortunately, I haven't been able to use the pass function successfully in my > tests. I actually get synchronicity problems if I do use it. I.e. I would get > "All tests succeeded" multiple times or even when tests fail. Maybe there is > something wrong with my general setup? But, right now the tests seem to run one > after the other as opposed to in parallel, so I don't think there are race > conditions. I do think you need them. The timing issue could show up as intermittent failures. You can get the 'all tests succeed' multiple times if one of the callbacks wrapped in pass() get's called multiple times-- basically each time you call pass, the callback should be invoked once. You don't wrap the event handler function with a pass when using chrome.test.listenOnce - maybe that was the problem (listenOnce will handle it for you)? Have you tried running the test like 10 or so times and see if it fails at all? I just patched it in and every now and then got this failure: [32445:32445:0916/165218:193549294384:INFO:CONSOLE(51)] "[FAIL] undefined: FAIL (no message) at Object.assertBool (chrome/extensionapitest:121:19) at chrome/extensionapitest:105:17 at Object.callback (chrome-extension://eeaadflfhcmafcpahlhlnkpommkklcch/test.js:322:13) at Object.<anonymous> (chrome/ExtensionProcessBindings:120:19)", source: chrome/extensionapitest (51) [32445:32445:0916/165218:193549294541:INFO:CONSOLE(23)] "Uncaught completed", source: chrome/extensionapitest (23) ../../chrome/browser/extensions/extension_offscreen_tabs_apitest.cc:18: Failure Value of: RunExtensionTest("offscreen_tabs") Actual: false Expected: true FAIL (no message) at Object.assertBool (chrome/extensionapitest:121:19) at chrome/extensionapitest:105:17 at Object.callback (chrome-extension://eeaadflfhcmafcpahlhlnkpommkklcch/test.js:322:13) at Object.<anonymous> (chrome/ExtensionProcessBindings:120:19) I think this is pretty important because these will be run on the main waterfall even if it's behind the experimental flag. http://codereview.chromium.org/7720002/diff/69002/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/offscreen_tabs/test.js (right): http://codereview.chromium.org/7720002/diff/69002/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:17: { "url": extensionPath + "a.html", nit: can you put the first property on the next line? { "url" ... here and the other places? http://codereview.chromium.org/7720002/diff/69002/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:346: }, nit: new lines between these methods http://codereview.chromium.org/7720002/diff/69002/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:348: function() { I think you got rid of these function names just now, but you'll want to keep them b/c they're used in the log output.
http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:214: *tab_contents = GetBackgroundPageTabContents(profile); On 2011/09/16 20:23:28, sky wrote: > On 2011/09/16 20:18:14, alexbost wrote: > > On 2011/09/16 16:29:21, sky wrote: > > > Are we sure this is only going to get hit for testing? Can you add a CHECK > to > > > that effect? > > > > It does get hit because the test runs in a background page which does not have > > associated tab contents. > > What prevents a non-test background page from doing this? Nothing right now. How can I distinguish between the two cases? http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/offscreen_tabs/test.js (right): http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:352: function tabManagement() { On 2011/09/16 21:02:41, jstritar wrote: > On 2011/09/16 20:18:14, alexbost wrote: > > On 2011/09/16 16:03:50, jstritar wrote: > > > I think you have race conditions in these tests- the test runner will move > on > > to > > > the next test while you're waiting for the callbacks to fire. > > > > > > We have a helper method, chrome.test.callbackPass, to handle this situation. > > You > > > wrap your callback functions with it and the test runner will count how many > > > callbacks you have and wait for them to be executed before moving on to the > > next > > > test. > > > > > > They would look like this since you've assigned callbackPass to pass: > > > > > > chrome.experimental.offscreenTabs.toDataUrl(..., pass(function(dataUrl) { > > > > > > })); > > > > Unfortunately, I haven't been able to use the pass function successfully in my > > tests. I actually get synchronicity problems if I do use it. I.e. I would get > > "All tests succeeded" multiple times or even when tests fail. Maybe there is > > something wrong with my general setup? But, right now the tests seem to run > one > > after the other as opposed to in parallel, so I don't think there are race > > conditions. > > I do think you need them. The timing issue could show up as intermittent > failures. You can get the 'all tests succeed' multiple times if one of the > callbacks wrapped in pass() get's called multiple times-- basically each time > you call pass, the callback should be invoked once. You don't wrap the event > handler function with a pass when using chrome.test.listenOnce - maybe that was > the problem (listenOnce will handle it for you)? > > Have you tried running the test like 10 or so times and see if it fails at all? > I just patched it in and every now and then got this failure: > > > [32445:32445:0916/165218:193549294384:INFO:CONSOLE(51)] "[FAIL] undefined: FAIL > (no message) > at Object.assertBool (chrome/extensionapitest:121:19) > at chrome/extensionapitest:105:17 > at Object.callback > (chrome-extension://eeaadflfhcmafcpahlhlnkpommkklcch/test.js:322:13) > at Object.<anonymous> (chrome/ExtensionProcessBindings:120:19)", source: > chrome/extensionapitest (51) > [32445:32445:0916/165218:193549294541:INFO:CONSOLE(23)] "Uncaught completed", > source: chrome/extensionapitest (23) > ../../chrome/browser/extensions/extension_offscreen_tabs_apitest.cc:18: Failure > Value of: RunExtensionTest("offscreen_tabs") > Actual: false > Expected: true > FAIL (no message) > at Object.assertBool (chrome/extensionapitest:121:19) > at chrome/extensionapitest:105:17 > at Object.callback > (chrome-extension://eeaadflfhcmafcpahlhlnkpommkklcch/test.js:322:13) > at Object.<anonymous> (chrome/ExtensionProcessBindings:120:19) > > I think this is pretty important because these will be run on the main waterfall > even if it's behind the experimental flag. This error is caused by create returning before the offscreen tab has navigated. I have added a TODO in the module .cc file. The toDataUrl is (now) called in an onUpdated event. We still need to figure out the callbackPass... http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:116: // Caller assumes ownership of OffscreenTab. On 2011/09/16 20:35:34, sky wrote: > Update comment. This deletes the OffscreenTab. Done. http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:330: Profile* profile = TabContentsWrapper::GetCurrentWrapperForContents( On 2011/09/16 20:35:34, sky wrote: > Only use GetCurrentWrapperForContents if necessary. Either expose the > TabContentsWrapper in ParentTab or the profile directly. Done. http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:337: && parent_tab_->contents()->render_view_host()->process()) On 2011/09/16 20:35:34, sky wrote: > && on the previous line and fix indenting. Done. http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:360: CHECK(removed); On 2011/09/16 20:35:34, sky wrote: > We generally only use CHECK until there is a security issue, or we're trying to > narrow down a crasher. I don't think that fits any of the cases for you in this > file. Done. http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:422: TabVector offscreen_tabs = parent_tab->offscreen_tabs(); On 2011/09/16 20:35:34, sky wrote: > const TabVector& Done. http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:425: it != offscreen_tabs.end(); ++it) { On 2011/09/16 20:35:34, sky wrote: > indent one more space Done. http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:469: parent_tab = map[offscreen_tab_id] = new ParentTab(); On 2011/09/16 20:35:34, sky wrote: > Separate this into two assignments. Done. http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:492: parent_tab->RemoveOffscreenTab(offscreen_tab); On 2011/09/16 20:35:34, sky wrote: > nit: set offscreen_tab = NULL here as it's been deleted. Done. http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:558: EXTENSION_FUNCTION_VALIDATE( On 2011/09/16 20:35:34, sky wrote: > Do these need to make sure the width/height is > 0 ? I think that's checked in the JavaScript pre-validation. I.e. the extension_api.json that specifies the API contains this: "width": { "type": "integer", "optional": true, "minimum": 0, "description": "Width of the window." } http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:627: TabVector offscreen_tabs = parent_tab->offscreen_tabs(); On 2011/09/16 20:35:34, sky wrote: > const TabVector& Done. http://codereview.chromium.org/7720002/diff/69002/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:985: TabContentsWrapper* tab_wrapper = TabContentsWrapper:: On 2011/09/16 20:35:34, sky wrote: > See comment earlier about using GetCurrentWrapperForContents here. It isn't > necessary. Done. http://codereview.chromium.org/7720002/diff/69002/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/offscreen_tabs/test.js (right): http://codereview.chromium.org/7720002/diff/69002/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:17: { "url": extensionPath + "a.html", On 2011/09/16 21:02:41, jstritar wrote: > nit: can you put the first property on the next line? > { > "url" ... > here and the other places? Done. http://codereview.chromium.org/7720002/diff/69002/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:346: }, On 2011/09/16 21:02:41, jstritar wrote: > nit: new lines between these methods Done. http://codereview.chromium.org/7720002/diff/69002/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:348: function() { On 2011/09/16 21:02:41, jstritar wrote: > I think you got rid of these function names just now, but you'll want to keep > them b/c they're used in the log output. Done.
This is my last concern. http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... File chrome/browser/extensions/extension_offscreen_tabs_module.cc (right): http://codereview.chromium.org/7720002/diff/65001/chrome/browser/extensions/e... chrome/browser/extensions/extension_offscreen_tabs_module.cc:214: *tab_contents = GetBackgroundPageTabContents(profile); On 2011/09/16 21:46:53, alexbost wrote: > On 2011/09/16 20:23:28, sky wrote: > > On 2011/09/16 20:18:14, alexbost wrote: > > > On 2011/09/16 16:29:21, sky wrote: > > > > Are we sure this is only going to get hit for testing? Can you add a CHECK > > to > > > > that effect? > > > > > > It does get hit because the test runs in a background page which does not > have > > > associated tab contents. > > > > What prevents a non-test background page from doing this? > > Nothing right now. How can I distinguish between the two cases? I don't know enough about background extensions and TabContents to know for sure. You'll need to talk with someone on the extensions team to resolve this.
LGTM with a TODO about figuring out background pages and as long as Jon understands this needs to be changed.
LGTM. Just in time! http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/offscreen_tabs/test.js (right): http://codereview.chromium.org/7720002/diff/65001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/offscreen_tabs/test.js:352: function tabManagement() { On 2011/09/16 21:46:53, alexbost wrote: > This error is caused by create returning before the offscreen tab has navigated. > I have added a TODO in the module .cc file. The toDataUrl is (now) called in an > onUpdated event. We still need to figure out the callbackPass... Okay, as long as the tests are passing now, I can figure out the callbackPass part later.
Thanks Jon and Scott for your reviews and Alex for pushing this through. Trying to submit through the commit bots now.
Change committed as 101678
Unfortunately this change was reverted in http://src.chromium.org/viewvc/chrome?view=rev&revision=101685 due to regressing the linux "sizes" step. Will have to try this again under a different CL.
FTW, this api could solve a whole host of problems with iframes and introduce .... da da da ... I would kill for this API!!
This essentially makes a Sidebar API. Depending on what sort of issues you run into, you could also think of this as an "HTML container" which could then be manipulate in a variety of ways. Perhaps an exclusive <hframe> element for Chrome could be made to fix the <iframe>, because well, you could totally use iframes for that demo I watched, iframes just suck. Also, I've had tab crashes with using dataUrl's in iframes... some pages on the web are just tooo heavy. Noticed other issues around dataUrl's for images and links too. (only super large dataUrl's cause problems)
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... |
