Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8387010:
Close all panels originated by the extension when extension unloads. (Closed)
|
Created:
9 years, 2 months ago by prasadt Modified:
9 years, 1 month ago CC:
chromium-reviews, jennb, jianli Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionClose all panels originated by the extension when extension unloads.
This fixes the bug where you're able to click on the wrench of panels whose originating extension has been unloaded, resulting in a crash.
BUG=101118
TEST=PanelBrowserTest.NonExtensionDomainPanelsCloseOnUninstall
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107837
Patch Set 1 #Patch Set 2 : Fix build errors from sync and merge. #
Total comments: 4
Patch Set 3 : Replace RunAllPending in loop with waiting for notifications. #
Total comments: 5
Patch Set 4 : Add extension browser test and other code review feedback. #
Total comments: 12
Patch Set 5 : Change ASSERT_EQ to EXPECT_EQ. #
Total comments: 16
Patch Set 6 : Code review feedback. #Messages
Total messages: 28 (0 generated)
Uploaded a new patch that fixes build errors from sync/merge.
Drive-by. http://codereview.chromium.org/8387010/diff/1004/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/8387010/diff/1004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:1364: MessageLoopForUI::current()->RunAllPending(); NOOOOOO! Don't do that please, it's known to be bad. Please wait for an event instead. Don't RunAllPending in a loop.
http://codereview.chromium.org/8387010/diff/1004/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/8387010/diff/1004/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:1364: MessageLoopForUI::current()->RunAllPending(); On 2011/10/25 09:01:38, Paweł Hajdan Jr. wrote: > NOOOOOO! Spooky response for something that seems to be done all over the code. Halloween? :-) git grep -B 4 RunAllPending | less > Don't do that please, it's known to be bad. Please wait for an event > instead. Don't RunAllPending in a loop. Done. As it turns out, this is easy enough to do this with wait on existing notifications. Thanks.
http://codereview.chromium.org/8387010/diff/1004/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/8387010/diff/1004/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:4290: if (!close_tab_contents) { Should be a way to do what you want without adding panel-specific logic each time through the loop for every tab in every browser. Another thought: Should popups opened by an extension to a different domain also be forced to close when extension is unloaded? What about tabs?
http://codereview.chromium.org/8387010/diff/7001/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/8387010/diff/7001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:1319: NonExtensionDomainPanelsCloseOnUninstall) { Could you make the test load a real extension that opens panels+popups+tabs to a different domain, unload the extension, then verify that everything that should be closed is closed? Seems like an extension browser test would be a more thorough test.
On 2011/10/25 17:29:28, prasadt wrote: > http://codereview.chromium.org/8387010/diff/1004/chrome/browser/ui/panels/pan... > File chrome/browser/ui/panels/panel_browsertest.cc (right): > > http://codereview.chromium.org/8387010/diff/1004/chrome/browser/ui/panels/pan... > chrome/browser/ui/panels/panel_browsertest.cc:1364: > MessageLoopForUI::current()->RunAllPending(); > On 2011/10/25 09:01:38, Paweł Hajdan Jr. wrote: > > NOOOOOO! > > git grep -B 4 RunAllPending | less I think Pawel meant that doing that repeatedly in a while loop is especially bad.
On 2011/10/25 17:40:57, Dmitry Titov wrote: > On 2011/10/25 17:29:28, prasadt wrote: > > > http://codereview.chromium.org/8387010/diff/1004/chrome/browser/ui/panels/pan... > > File chrome/browser/ui/panels/panel_browsertest.cc (right): > > > > > http://codereview.chromium.org/8387010/diff/1004/chrome/browser/ui/panels/pan... > > chrome/browser/ui/panels/panel_browsertest.cc:1364: > > MessageLoopForUI::current()->RunAllPending(); > > On 2011/10/25 09:01:38, Paweł Hajdan Jr. wrote: > > > NOOOOOO! > > > > git grep -B 4 RunAllPending | less > > I think Pawel meant that doing that repeatedly in a while loop is especially > bad. That's correct, which is why I put the -B 4 option which shows 4 context before the line with the text. Search for "while" in the output and you'll see a whole bunch of tests running this in a while loop. Pawel - What's the rationale for considering this such a bad thing (in tests only)? Curious to know.
On 2011/10/25 17:37:47, jennb wrote: > http://codereview.chromium.org/8387010/diff/7001/chrome/browser/ui/panels/pan... > File chrome/browser/ui/panels/panel_browsertest.cc (right): > > http://codereview.chromium.org/8387010/diff/7001/chrome/browser/ui/panels/pan... > chrome/browser/ui/panels/panel_browsertest.cc:1319: > NonExtensionDomainPanelsCloseOnUninstall) { > Could you make the test load a real extension that opens panels+popups+tabs to a > different domain, unload the extension, then verify that everything that should > be closed is closed? Seems like an extension browser test would be a more > thorough test. I like to keep the test more targeted where I can, that makes it easier to write, maintain and debug. I'd say what you're describing belongs with the extension tests. I'll write one if you like but it seems outside the scope of this CL.
I don't think this belongs in a browser test. You're mocking out a lot of the extension machinery which is subject to change. If you think this is the best way to do it though, I have a nit =) http://codereview.chromium.org/8387010/diff/7001/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/8387010/diff/7001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:1344: Panel* panel1 =CreatePanelWithParams(params1); Space.
http://codereview.chromium.org/8387010/diff/1004/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/8387010/diff/1004/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:4290: if (!close_tab_contents) { On 2011/10/25 17:32:16, jennb wrote: > Should be a way to do what you want without adding panel-specific logic each > time through the loop for every tab in every browser. What I'm doing is panels specific though. I want to close all non-extension panels when the extension that opened them is uninstalled. In the non-panel case, unloading an extension does not close non-extension tabs/windows opened by another extension. Makes sense, as these are indistinguishable from other normal tabs/windows and it would be strange for them to close on the user when an extension is uninstalled. > Another thought: Should popups opened by an extension to a different domain also > be forced to close when extension is unloaded? What about tabs? What I said above. Also if we were to do it, it would be a big enough change in extensions behavior that it'll need design input from the extensions team.
Code I commented in the drive-by LGTM, thank you. I'm not sure how many RunAllPending in a loop examples we have, but sometimes things slip past the drive-by reviews. That's one more reason I appreciate the fix.
http://codereview.chromium.org/8387010/diff/7001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/8387010/diff/7001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:4290: if (!close_tab_contents) { nit: could it be more readable like this: close_tab_contents = close_tab_contents || ....; ?
^^^ That was drive-by.
The current test provides good unit test coverage. Please also add an integration test that covers actually uninstalling an extension. If extension uninstall process stopped sending the _UNLOAD notification, the current test would still continue to pass, so we need more coverage. Have you tried having PanelManager register for the _UNLOAD notification to close panels that belong to the unloaded extension but whose URL is a non-extension domain? This would not interfere with the logic in browser.cc, and keeps panel-specific logic in the panel code. Thanks, Jenn
On 2011/10/26 02:17:33, jennb wrote: > The current test provides good unit test coverage. Please also add an > integration test that covers actually uninstalling an extension. If extension > uninstall process stopped sending the _UNLOAD notification, the current test > would still continue to pass, so we need more coverage. Done. > Have you tried having PanelManager register for the _UNLOAD notification to > close panels that belong to the unloaded extension but whose URL is a > non-extension domain? This would not interfere with the logic in browser.cc, and > keeps panel-specific logic in the panel code. If we did that, we'll need to keep the logic to handle _UNLOAD in sync between browser.cc and panel_manager.cc, that would be fragile. Alternately we'll need to introduce a common helper function in Browser class to be invoked by both the handlers and that seems unnecessary to me. It seems cleaner to keep this in one place. Considering we don't have a panel specific class for Browser, I'd say Browser class applies as much to panels as it does to any other type of window, say popup. And we shouldn't try to keep panel specific out of it just because its panel specific. > Thanks, > Jenn
http://codereview.chromium.org/8387010/diff/7001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/8387010/diff/7001/chrome/browser/ui/browser.cc... chrome/browser/ui/browser.cc:4290: if (!close_tab_contents) { On 2011/10/25 20:29:15, Dmitry Titov wrote: > nit: could it be more readable like this: > close_tab_contents = close_tab_contents || ....; > ? Done. http://codereview.chromium.org/8387010/diff/7001/chrome/browser/ui/panels/pan... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/8387010/diff/7001/chrome/browser/ui/panels/pan... chrome/browser/ui/panels/panel_browsertest.cc:1344: Panel* panel1 =CreatePanelWithParams(params1); On 2011/10/25 18:06:19, dcheng wrote: > Space. Done.
Thanks for adding the extension test. http://codereview.chromium.org/8387010/diff/12001/chrome/browser/extensions/w... File chrome/browser/extensions/window_open_apitest.cc (right): http://codereview.chromium.org/8387010/diff/12001/chrome/browser/extensions/w... chrome/browser/extensions/window_open_apitest.cc:38: if (BrowserList::GetBrowserCount(browser->profile()) == num_browsers && Why did this change from >= to ==? It seems safer to use >= since otherwise we may end up in a situation where this loop does not terminate. http://codereview.chromium.org/8387010/diff/12001/chrome/browser/extensions/w... chrome/browser/extensions/window_open_apitest.cc:62: ASSERT_EQ(num_popups, num_popups_seen); I would use EXPECT_EQ so the rest of the tests can continue running. http://codereview.chromium.org/8387010/diff/12001/chrome/browser/extensions/w... chrome/browser/extensions/window_open_apitest.cc:223: // Uninstall the extension. Comment not needed. http://codereview.chromium.org/8387010/diff/12001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/background.html (right): http://codereview.chromium.org/8387010/diff/12001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/background.html:4: chrome.windows.create({url: "about:blank", type: "panel"}); about:blank has special behavior. I would create a test page that's not part of the extension and use that instead.
http://codereview.chromium.org/8387010/diff/12001/chrome/browser/extensions/w... File chrome/browser/extensions/window_open_apitest.cc (right): http://codereview.chromium.org/8387010/diff/12001/chrome/browser/extensions/w... chrome/browser/extensions/window_open_apitest.cc:38: if (BrowserList::GetBrowserCount(browser->profile()) == num_browsers && On 2011/10/28 19:48:17, dcheng wrote: > Why did this change from >= to ==? It seems safer to use >= since otherwise we > may end up in a situation where this loop does not terminate. I think the situation is the same before too, as you could have a test that creates one few tabs. Yes, it could be a problem is some test consider more tabs etc than what's passed in to be a success. But I don't see that to be the case. I had to make the change because for the uninstall case, I'm looking for the count to go down to the expected number and not up. So >= will bail out right away which is not what I want. http://codereview.chromium.org/8387010/diff/12001/chrome/browser/extensions/w... chrome/browser/extensions/window_open_apitest.cc:62: ASSERT_EQ(num_popups, num_popups_seen); On 2011/10/28 19:48:17, dcheng wrote: > I would use EXPECT_EQ so the rest of the tests can continue running. Done. This won't actually affect other tests as these are browser tests and each runs in its own process. http://codereview.chromium.org/8387010/diff/12001/chrome/browser/extensions/w... chrome/browser/extensions/window_open_apitest.cc:223: // Uninstall the extension. On 2011/10/28 19:48:17, dcheng wrote: > Comment not needed. Done. http://codereview.chromium.org/8387010/diff/12001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/background.html (right): http://codereview.chromium.org/8387010/diff/12001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/background.html:4: chrome.windows.create({url: "about:blank", type: "panel"}); On 2011/10/28 19:48:17, dcheng wrote: > about:blank has special behavior. I would create a test page that's not part of > the extension and use that instead. about:blank meets the requirement for this test as its not considered part of the extension and that's pretty much what the test needs. I tried to do it with a test page that's not part of the extension. But I couldn't find a way to communicate back to the test code that the page loaded as I'm doing for the extension pages with chrome.test.sendMessage, that mechanism seems to work only for extension pages. If there is an equivalent of ExtensionTestMessageListener class for non-extension pages, I'll be happy to use it. But writing a new listener just for this test seems unnecessary. Its true that I'm not able to communicate back with about:blank either but its loading fast enough because: 1) The original browser window that the browser tests create by default is created with about:blank and so no new process is getting created. 2) about:blank does not involve loading file from disk and parsing html. There is potential for flakiness but looks like a low risk.
http://codereview.chromium.org/8387010/diff/12001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/background.html (right): http://codereview.chromium.org/8387010/diff/12001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/background.html:4: chrome.windows.create({url: "about:blank", type: "panel"}); On 2011/10/28 20:19:35, prasadt wrote: > On 2011/10/28 19:48:17, dcheng wrote: > > about:blank has special behavior. I would create a test page that's not part > of > > the extension and use that instead. > > about:blank meets the requirement for this test as its not considered part of > the extension and that's pretty much what the test needs. > > I tried to do it with a test page that's not part of the extension. But I > couldn't find a way to communicate back to the test code that the page loaded as > I'm doing for the extension pages with chrome.test.sendMessage, that mechanism > seems to work only for extension pages. If there is an equivalent of > ExtensionTestMessageListener class for non-extension pages, I'll be happy to use > it. But writing a new listener just for this test seems unnecessary. > > Its true that I'm not able to communicate back with about:blank either but its > loading fast enough because: > 1) The original browser window that the browser tests create by default is > created with about:blank and so no new process is getting created. > 2) about:blank does not involve loading file from disk and parsing html. > > There is potential for flakiness but looks like a low risk. There's some ongoing work to fix the current issues with extensions and security origins. Per http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#abo...: The origin of the about:blank Document is set when the Document is created. If the new browsing context has a creator browsing context, then the origin of the about:blank Document is the origin of the creator Document. Otherwise, the origin of the about:blank Document is a globally unique identifier assigned when the new browsing context is created. So it is quite possible that in the future, about:blank pages opened by an extension will be considered part of the extension.
http://codereview.chromium.org/8387010/diff/12001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/background.html (right): http://codereview.chromium.org/8387010/diff/12001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/background.html:4: chrome.windows.create({url: "about:blank", type: "panel"}); On 2011/10/28 20:27:08, dcheng wrote: > On 2011/10/28 20:19:35, prasadt wrote: > > On 2011/10/28 19:48:17, dcheng wrote: > > > about:blank has special behavior. I would create a test page that's not part > > of > > > the extension and use that instead. > > > > about:blank meets the requirement for this test as its not considered part of > > the extension and that's pretty much what the test needs. > > > > I tried to do it with a test page that's not part of the extension. But I > > couldn't find a way to communicate back to the test code that the page loaded > as > > I'm doing for the extension pages with chrome.test.sendMessage, that mechanism > > seems to work only for extension pages. If there is an equivalent of > > ExtensionTestMessageListener class for non-extension pages, I'll be happy to > use > > it. But writing a new listener just for this test seems unnecessary. > > > > Its true that I'm not able to communicate back with about:blank either but its > > loading fast enough because: > > 1) The original browser window that the browser tests create by default is > > created with about:blank and so no new process is getting created. > > 2) about:blank does not involve loading file from disk and parsing html. > > > > There is potential for flakiness but looks like a low risk. > > There's some ongoing work to fix the current issues with extensions and security > origins. Per > http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#abo...: > The origin of the about:blank Document is set when the Document is created. If > the new browsing context has a creator browsing context, then the origin of the > about:blank Document is the origin of the creator Document. Otherwise, the > origin of the about:blank Document is a globally unique identifier assigned when > the new browsing context is created. > > So it is quite possible that in the future, about:blank pages opened by an > extension will be considered part of the extension. As it stands now, this may not be an issue for this test as the first about:blank is getting created before extension is loaded. I could use chrome://extensions instead of about:blank and that works. But there is a higher chance of running into a timing issue with that causing flakiness, as it involves creating a new process. I suggest we stick with about:blank for now and deal with it in the unlikely case it starts failing sometime in the future.
http://codereview.chromium.org/8387010/diff/12001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/background.html (right): http://codereview.chromium.org/8387010/diff/12001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/background.html:4: chrome.windows.create({url: "about:blank", type: "panel"}); On 2011/10/28 20:53:23, prasadt wrote: > On 2011/10/28 20:27:08, dcheng wrote: > > On 2011/10/28 20:19:35, prasadt wrote: > > > On 2011/10/28 19:48:17, dcheng wrote: > > > > about:blank has special behavior. I would create a test page that's not > part > > > of > > > > the extension and use that instead. > > > > > > about:blank meets the requirement for this test as its not considered part > of > > > the extension and that's pretty much what the test needs. > > > > > > I tried to do it with a test page that's not part of the extension. But I > > > couldn't find a way to communicate back to the test code that the page > loaded > > as > > > I'm doing for the extension pages with chrome.test.sendMessage, that > mechanism > > > seems to work only for extension pages. If there is an equivalent of > > > ExtensionTestMessageListener class for non-extension pages, I'll be happy to > > use > > > it. But writing a new listener just for this test seems unnecessary. > > > > > > Its true that I'm not able to communicate back with about:blank either but > its > > > loading fast enough because: > > > 1) The original browser window that the browser tests create by default is > > > created with about:blank and so no new process is getting created. > > > 2) about:blank does not involve loading file from disk and parsing html. > > > > > > There is potential for flakiness but looks like a low risk. > > > > There's some ongoing work to fix the current issues with extensions and > security > > origins. Per > > > http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#abo...: > > The origin of the about:blank Document is set when the Document is created. If > > the new browsing context has a creator browsing context, then the origin of > the > > about:blank Document is the origin of the creator Document. Otherwise, the > > origin of the about:blank Document is a globally unique identifier assigned > when > > the new browsing context is created. > > > > So it is quite possible that in the future, about:blank pages opened by an > > extension will be considered part of the extension. > > As it stands now, this may not be an issue for this test as the first > about:blank is getting created before extension is loaded. > > I could use chrome://extensions instead of about:blank and that works. But there > is a higher chance of running into a timing issue with that causing flakiness, > as it involves creating a new process. > There's no guarantee that about:blank pages open in the same process, so about:blank isn't really any guarantee that the timing will work. What is the timing issue anyway for non-extension pages? I'm trying to understand where the source of flakiness is coming from, and why WaitForTabsAndPopups isn't enough to avoid it. > I suggest we stick with about:blank for now and deal with it in the unlikely > case it starts failing sometime in the future.
http://codereview.chromium.org/8387010/diff/16001/chrome/browser/extensions/w... File chrome/browser/extensions/window_open_apitest.cc (right): http://codereview.chromium.org/8387010/diff/16001/chrome/browser/extensions/w... chrome/browser/extensions/window_open_apitest.cc:38: if (BrowserList::GetBrowserCount(browser->profile()) == num_browsers && Better to leave as >= for both of these. It's a common coding technique to use >= when you need to abort at == as a safeguard. http://codereview.chromium.org/8387010/diff/16001/chrome/browser/extensions/w... chrome/browser/extensions/window_open_apitest.cc:194: host_resolver()->AddRule("a.com", "127.0.0.1"); Could you add a comment explaining why this is necessary? It's not obvious from reading the code. http://codereview.chromium.org/8387010/diff/16001/chrome/browser/extensions/w... chrome/browser/extensions/window_open_apitest.cc:227: WaitForTabsAndPopups(browser(), 1, 1, 0); I see why you changed '>=' to '==' now. WaitForTabsAndPopups() is meant to wait for the browser count to increase. Could you wait on browser close notifications or something here rather than change the intent of WaitForTabsAndPopups? http://codereview.chromium.org/8387010/diff/16001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/8387010/diff/16001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4280: ((web_app::GetExtensionIdFromApplicationName(app_name_) == I would reverse the check to have type_ == TYPE_PANEL first. It's less expensive than working with the app_name. http://codereview.chromium.org/8387010/diff/16001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4280: ((web_app::GetExtensionIdFromApplicationName(app_name_) == Will this work correctly if there is no app name? http://codereview.chromium.org/8387010/diff/16001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/8387010/diff/16001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browsertest.cc:1319: NonExtensionDomainPanelsCloseOnUninstall) { IsClosed or CloseNonExtensionDomainPanelOnUninstall http://codereview.chromium.org/8387010/diff/16001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browsertest.cc:1355: Extra blank line. http://codereview.chromium.org/8387010/diff/16001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/background.html (right): http://codereview.chromium.org/8387010/diff/16001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/background.html:5: window.open("about:blank", Suggest changing these to chrome.windows.create and chrome.tabs.create to make it analogous to the code below for extension content.
http://codereview.chromium.org/8387010/diff/16001/chrome/browser/extensions/w... File chrome/browser/extensions/window_open_apitest.cc (right): http://codereview.chromium.org/8387010/diff/16001/chrome/browser/extensions/w... chrome/browser/extensions/window_open_apitest.cc:38: if (BrowserList::GetBrowserCount(browser->profile()) == num_browsers && On 2011/10/28 21:12:27, jennb wrote: > Better to leave as >= for both of these. It's a common coding technique to use > >= when you need to abort at == as a safeguard. >= does not work for the uninstall case. In this case I'm starting with two browsers, two tabs and two panels, and waiting for the count to go down to 1, 1 and 0. If the check is >=, then it'll bail out right away which is not what I want. http://codereview.chromium.org/8387010/diff/16001/chrome/browser/extensions/w... chrome/browser/extensions/window_open_apitest.cc:194: host_resolver()->AddRule("a.com", "127.0.0.1"); On 2011/10/28 21:12:27, jennb wrote: > Could you add a comment explaining why this is necessary? It's not obvious from > reading the code. Left over. Removed it. I added it when I was working on making it work with non-extension pages that are not about:blank. http://codereview.chromium.org/8387010/diff/16001/chrome/browser/extensions/w... chrome/browser/extensions/window_open_apitest.cc:227: WaitForTabsAndPopups(browser(), 1, 1, 0); On 2011/10/28 21:12:27, jennb wrote: > I see why you changed '>=' to '==' now. WaitForTabsAndPopups() is meant to wait > for the browser count to increase. Could you wait on browser close notifications > or something here rather than change the intent of WaitForTabsAndPopups? Its possible but not as black boxy as we want this to be. WaitForTabsAndPopups() is used in just this file and shouldn't affect anything. If I must, I'd just introduce a new function and call it WaitForTabsAndPopupsToGoDownTo() or something like that, that'd be simpler. http://codereview.chromium.org/8387010/diff/16001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/8387010/diff/16001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4280: ((web_app::GetExtensionIdFromApplicationName(app_name_) == On 2011/10/28 21:12:27, jennb wrote: > I would reverse the check to have type_ == TYPE_PANEL first. It's less expensive > than working with the app_name. Done. http://codereview.chromium.org/8387010/diff/16001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4280: ((web_app::GetExtensionIdFromApplicationName(app_name_) == On 2011/10/28 21:12:27, jennb wrote: > Will this work correctly if there is no app name? Yes. GetExtensionIdFromApplicationName just returns an empty string. Also the test covers it, the first browser window created does not have an app_name_. http://codereview.chromium.org/8387010/diff/16001/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel_browsertest.cc (right): http://codereview.chromium.org/8387010/diff/16001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browsertest.cc:1319: NonExtensionDomainPanelsCloseOnUninstall) { On 2011/10/28 21:12:27, jennb wrote: > IsClosed or CloseNonExtensionDomainPanelOnUninstall This tests that panels close, hence the choice PanelsClose. I'm sure there are lot of reasonable alternatives :-) http://codereview.chromium.org/8387010/diff/16001/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel_browsertest.cc:1355: On 2011/10/28 21:12:27, jennb wrote: > Extra blank line. Done. http://codereview.chromium.org/8387010/diff/16001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/background.html (right): http://codereview.chromium.org/8387010/diff/16001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/background.html:5: window.open("about:blank", On 2011/10/28 21:12:27, jennb wrote: > Suggest changing these to chrome.windows.create and chrome.tabs.create to make > it analogous to the code below for extension content. I chose to keep them different to cover the two slightly different cases.
lgtm
http://codereview.chromium.org/8387010/diff/12001/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/background.html (right): http://codereview.chromium.org/8387010/diff/12001/chrome/test/data/extensions... chrome/test/data/extensions/api_test/window_open/close_panels_on_uninstall/background.html:4: chrome.windows.create({url: "about:blank", type: "panel"}); On 2011/10/28 21:11:57, dcheng wrote: > On 2011/10/28 20:53:23, prasadt wrote: > > On 2011/10/28 20:27:08, dcheng wrote: > > > On 2011/10/28 20:19:35, prasadt wrote: > > > > On 2011/10/28 19:48:17, dcheng wrote: > > > > > about:blank has special behavior. I would create a test page that's not > > part > > > > of > > > > > the extension and use that instead. > > > > > > > > about:blank meets the requirement for this test as its not considered part > > of > > > > the extension and that's pretty much what the test needs. > > > > > > > > I tried to do it with a test page that's not part of the extension. But I > > > > couldn't find a way to communicate back to the test code that the page > > loaded > > > as > > > > I'm doing for the extension pages with chrome.test.sendMessage, that > > mechanism > > > > seems to work only for extension pages. If there is an equivalent of > > > > ExtensionTestMessageListener class for non-extension pages, I'll be happy > to > > > use > > > > it. But writing a new listener just for this test seems unnecessary. > > > > > > > > Its true that I'm not able to communicate back with about:blank either but > > its > > > > loading fast enough because: > > > > 1) The original browser window that the browser tests create by default is > > > > created with about:blank and so no new process is getting created. > > > > 2) about:blank does not involve loading file from disk and parsing html. > > > > > > > > There is potential for flakiness but looks like a low risk. > > > > > > There's some ongoing work to fix the current issues with extensions and > > security > > > origins. Per > > > > > > http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#abo...: > > > The origin of the about:blank Document is set when the Document is created. > If > > > the new browsing context has a creator browsing context, then the origin of > > the > > > about:blank Document is the origin of the creator Document. Otherwise, the > > > origin of the about:blank Document is a globally unique identifier assigned > > when > > > the new browsing context is created. > > > > > > So it is quite possible that in the future, about:blank pages opened by an > > > extension will be considered part of the extension. > > > > As it stands now, this may not be an issue for this test as the first > > about:blank is getting created before extension is loaded. > > > > I could use chrome://extensions instead of about:blank and that works. But > there > > is a higher chance of running into a timing issue with that causing flakiness, > > as it involves creating a new process. > > > > There's no guarantee that about:blank pages open in the same process, so > about:blank isn't really any guarantee that the timing will work. What is the > timing issue anyway for non-extension pages? I'm trying to understand where the > source of flakiness is coming from, and why WaitForTabsAndPopups isn't enough to > avoid it. Discussed offline. Yes, about:blank does not guarantee that there won't be a timing issue. Its just my judgement call that it makes it far less likely. > > I suggest we stick with about:blank for now and deal with it in the unlikely > > case it starts failing sometime in the future.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/prasadt@chromium.org/8387010/14002
Change committed as 107837 |
