Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1099553002:
extensions: windows: list all windows from the current profile (Closed)
|
Created:
5 years, 8 months ago by llandwerlin-old Modified:
5 years, 4 months ago CC:
dcheng, asvitkine+watch_chromium.org, chromium-apps-reviews_chromium.org, chromium-reviews, Dmitry Titov, extensions-reviews_chromium.org, jennb, jianli, dmazzoni Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionextensions: windows: list all windows from the current profile
This change allows extensions to get access to a broader set of
chromium windows (namely windows created by other
applications/extensions) as long as they belong to the same profile.
BUG=394341
TEST=browser_tests --gtest_filter=ExtensionTabsTest.*
Committed: https://crrev.com/56b2a727992f8caf2141f89bf0073793f5cbe3d8
Cr-Commit-Position: refs/heads/master@{#342016}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Limit application resizing to window's constraints #Patch Set 3 : Add application windows resize constraint test #
Total comments: 8
Patch Set 4 : Update after dcheng's review #Patch Set 5 : Improve DevTools resizing test #Patch Set 6 : split out AppBaseWindow from app_window_controller.cc/h #Patch Set 7 : Fix tests running on X11 #Patch Set 8 : Use windowTypes as filter instead of allWindowTypes #Patch Set 9 : Workaround older stl on MacOSX #Patch Set 10 : Fix static variable usage on MacOSX #Patch Set 11 : More details in ExtensionTabsTest.FilteredEvents for MacOSX #Patch Set 12 : AppWindowController extracted #
Total comments: 13
Patch Set 13 : Update after stevenjb's review #Patch Set 14 : Disable focus event tests on MacOSX #
Total comments: 24
Patch Set 15 : Update after kalman's review #Patch Set 16 : Fix bitmask issue #
Total comments: 8
Patch Set 17 : Move filters into window_controller.cc/h #
Total comments: 9
Patch Set 18 : More filters completely into WindowController #
Total comments: 2
Patch Set 19 : nit #Messages
Total messages: 84 (35 generated)
lionel.g.landwerlin@intel.com changed reviewers: + kalman@chromium.org, pkasting@chromium.org
pkasting@, kalman@: This is one of the prerequisite change to enable extensions, through a this new permission, to list all the windows within a given session. Currently extensions using the chrome.windows.* API can only access tabs. Given that of the extensions using this API are providing some kind of window management, it would be nice these extensions to have access to all the windows. This is something we discussed earlier this year here : https://docs.google.com/a/intel.com/document/d/1_nSJrH0B9VizoudyfCSMCaAWc_X5R... Thank you.
The unit tests don't make me believe that this works for anything other than parsing permissions, and really, we typically don't test that anymore. We need tests that exercise window management - and, fairly complex window management at that. I presume this is only expected to work on ChromeOS. https://codereview.chromium.org/1099553002/diff/1/chrome/common/extensions/pe... File chrome/common/extensions/permissions/chrome_api_permissions.cc (right): https://codereview.chromium.org/1099553002/diff/1/chrome/common/extensions/pe... chrome/common/extensions/permissions/chrome_api_permissions.cc:196: IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ, Extensions can usually enumerate Chrome tabs/windows without any permission. The tabs/windows permissions just give them access to sensitive data like the URL. It makes sense to me to continue that behavior, which is to say, don't make tabs.global/windows.global require a permission warning, and don't make it grant access to data like the URL. Also I think that windows.global is enough, no reason to have the tabs.global alias.
If you list multiple reviewers, always say what you want each to review. I'm assuming you need my stamp for c/b/ui/panel.cc and kalman's doesn't suffice? If so, LGTM on that file. https://codereview.chromium.org/1099553002/diff/1/chrome/browser/ui/panels/pa... File chrome/browser/ui/panels/panel.cc (right): https://codereview.chromium.org/1099553002/diff/1/chrome/browser/ui/panels/pa... chrome/browser/ui/panels/panel.cc:156: if (extension->permissions_data()->HasAPIPermission( Nit: Shorter: return (extension->id() == panel_->extension_id()) || extension->permissions_data()->HasAPIPermission( extensions::APIPermission::kTabGlobal);
PTAL. Sorry for the long delay on updating this. I've added the code allowing extensions to list/update application windows. https://codereview.chromium.org/1099553002/diff/1/chrome/common/extensions/pe... File chrome/common/extensions/permissions/chrome_api_permissions.cc (right): https://codereview.chromium.org/1099553002/diff/1/chrome/common/extensions/pe... chrome/common/extensions/permissions/chrome_api_permissions.cc:196: IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ, On 2015/04/20 16:47:43, kalman wrote: > Extensions can usually enumerate Chrome tabs/windows without any permission. The > tabs/windows permissions just give them access to sensitive data like the URL. > > It makes sense to me to continue that behavior, which is to say, don't make > tabs.global/windows.global require a permission warning, and don't make it grant > access to data like the URL. > > Also I think that windows.global is enough, no reason to have the tabs.global > alias. Done.
Some drive-by thoughts. https://codereview.chromium.org/1099553002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/app_window_controller.cc (right): https://codereview.chromium.org/1099553002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/app_window_controller.cc:167: base::DictionaryValue* AppWindowController::CreateTabValue( Please use scoped_ptr here and elsewhere. https://codereview.chromium.org/1099553002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/1099553002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:267: const size_t NUM_WINDOWS = 5; Nit: kNumWindows is the naming convention for constants. https://codereview.chromium.org/1099553002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_windows_api.cc (right): https://codereview.chromium.org/1099553002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_windows_api.cc:47: windows_event_router(); Construct this directly in the initializer list instead of depending on the previous lazy-init path to do it. https://codereview.chromium.org/1099553002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_event_router.h (right): https://codereview.chromium.org/1099553002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_event_router.h:48: void DispatchEvents(); The name of this member makes it sound like this actually dispatches events. I'd suggest renaming it.
Thanks for the comments. https://codereview.chromium.org/1099553002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/app_window_controller.cc (right): https://codereview.chromium.org/1099553002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/app_window_controller.cc:167: base::DictionaryValue* AppWindowController::CreateTabValue( On 2015/06/29 18:15:49, dcheng wrote: > Please use scoped_ptr here and elsewhere. These methods are overridden from extension::WindowController : https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... I would have to change this interfaces to be able to do this. https://codereview.chromium.org/1099553002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/1099553002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:267: const size_t NUM_WINDOWS = 5; On 2015/06/29 18:15:49, dcheng wrote: > Nit: kNumWindows is the naming convention for constants. Done. https://codereview.chromium.org/1099553002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_windows_api.cc (right): https://codereview.chromium.org/1099553002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_windows_api.cc:47: windows_event_router(); On 2015/06/29 18:15:49, dcheng wrote: > Construct this directly in the initializer list instead of depending on the > previous lazy-init path to do it. Done. https://codereview.chromium.org/1099553002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_event_router.h (right): https://codereview.chromium.org/1099553002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_event_router.h:48: void DispatchEvents(); On 2015/06/29 18:15:49, dcheng wrote: > The name of this member makes it sound like this actually dispatches events. I'd > suggest renaming it. Done.
https://codereview.chromium.org/1099553002/diff/1/chrome/common/extensions/pe... File chrome/common/extensions/permissions/chrome_api_permissions.cc (right): https://codereview.chromium.org/1099553002/diff/1/chrome/common/extensions/pe... chrome/common/extensions/permissions/chrome_api_permissions.cc:196: IDS_EXTENSION_PROMPT_WARNING_HISTORY_READ, On 2015/06/26 11:26:14, llandwerlin wrote: > On 2015/04/20 16:47:43, kalman wrote: > > Extensions can usually enumerate Chrome tabs/windows without any permission. > The > > tabs/windows permissions just give them access to sensitive data like the URL. > > > > It makes sense to me to continue that behavior, which is to say, don't make > > tabs.global/windows.global require a permission warning, and don't make it > grant > > access to data like the URL. > > > > Also I think that windows.global is enough, no reason to have the tabs.global > > alias. > > Done. Mm actually I need to think about this a little bit more. It's the right thing to do to (a) force extensions to opt-in to getting global windows (otherwise it's not backwards compatible), and (b) not require an install warning just to do so. However, it's also useful to read the URLs (like the extensions) of windows, which does require an install warning. My current thinking is to actually use an entirely different namespace for this, like chrome.windows.global.query/remove/onCreated/onUpdated/..., then to have the windows.global permission give access to the URLs and so on. That would be both backwards compatible and fairly clean. But that's probably a much larger change, and I'm sure we can carve out plenty of code to submit before making that decision (like all of the event routing and querying stuff, most of the app window router). Sorry to drop this on you now. Another option would be to have 2 separate permissions but instinctively that sounds confusing. https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/app_window_controller.h (right): https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/app_window_controller.h:26: class AppBaseWindow : public ui::BaseWindow { I need to look into this more closely, but for now sending out the comment in the other files. https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:353: If the only change to a file is removing a blank line, it's not worth changing the file. https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:266: IN_PROC_BROWSER_TEST_F(ExtensionTabsTest, GetAllWindowsGlobal) { This test is basically identical to the one above. It doesn't really need to be: - either factor it into a common test, though I don't think that all the extra tests add that much, so - I'd simplify this test with the assumption that many of the assertions here (like testing the undocked devtools, but others as well) are already tested and no need to test them again. https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_windows_api.cc (right): https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_windows_api.cc:49: windows_event_router(); What does this do? https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_event_router.cc (right): https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_event_router.cc:32: dispatch_events_(false) { Can you handle all the decisions about whether to dispatch events internally? This class has access to its Profile, you can check the event router whether listeners exist or whatever. Also some of these changes are orthogonal to what you're doing, right? It'd be nice to submit those separately to - keep the revision history cleaner - make reviewing a bit easier - be able to pinpoint regressions more easily https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_util.cc (right): https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.cc:41: same comment re empty line. https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/ui/pane... File chrome/browser/ui/panels/panel.cc (right): https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/ui/pane... chrome/browser/ui/panels/panel.cc:158: return true; Without me looking into it: could you explain (to me now) why you need this check both here *and* in ash_panel_contents.cc? Are those classes simply badly factored to require you to do it twice? https://codereview.chromium.org/1099553002/diff/200001/chrome/common/extensio... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/1099553002/diff/200001/chrome/common/extensio... chrome/common/extensions/api/_api_features.json:802: "windows.global": { You shouldn't need to change API features (this file) at all. You're not adding an API, only a permission.
https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:353: On 2015/06/30 17:33:53, kalman wrote: > If the only change to a file is removing a blank line, it's not worth changing > the file. Sorry, removing. https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:266: IN_PROC_BROWSER_TEST_F(ExtensionTabsTest, GetAllWindowsGlobal) { On 2015/06/30 17:33:53, kalman wrote: > This test is basically identical to the one above. It doesn't really need to be: > - either factor it into a common test, though I don't think that all the extra > tests add that much, so > - I'd simplify this test with the assumption that many of the assertions here > (like testing the undocked devtools, but others as well) are already tested and > no need to test them again. Will do, Thanks. https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_windows_api.cc (right): https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_windows_api.cc:49: windows_event_router(); On 2015/06/30 17:33:53, kalman wrote: > What does this do? Sorry for that, removing. https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_event_router.cc (right): https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_event_router.cc:32: dispatch_events_(false) { On 2015/06/30 17:33:53, kalman wrote: > Can you handle all the decisions about whether to dispatch events internally? > This class has access to its Profile, you can check the event router whether > listeners exist or whatever. > > Also some of these changes are orthogonal to what you're doing, right? It'd be > nice to submit those separately to > - keep the revision history cleaner > - make reviewing a bit easier > - be able to pinpoint regressions more easily The reason for this WindowEventRouter change is that we need to insert WindowController s into the WindowControllerList for application windows, even if the extension isn't listening to any event. So we can have a Get* methods working without a particular code path for getting the list of application windows. I'll submit another CL just for the change in the event dispatch. https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_util.cc (right): https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.cc:41: On 2015/06/30 17:33:53, kalman wrote: > same comment re empty line. Acknowledged. https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/ui/pane... File chrome/browser/ui/panels/panel.cc (right): https://codereview.chromium.org/1099553002/diff/200001/chrome/browser/ui/pane... chrome/browser/ui/panels/panel.cc:158: return true; On 2015/06/30 17:33:53, kalman wrote: > Without me looking into it: could you explain (to me now) why you need this > check both here *and* in ash_panel_contents.cc? Are those classes simply badly > factored to require you to do it twice? Thanks for pointing this out. I forgot to fix this. This is wrong. My current thought is to remove the PanelExtensionWindowController from this file and rely on the AppWindowController instead. https://codereview.chromium.org/1099553002/diff/200001/chrome/common/extensio... File chrome/common/extensions/api/_api_features.json (right): https://codereview.chromium.org/1099553002/diff/200001/chrome/common/extensio... chrome/common/extensions/api/_api_features.json:802: "windows.global": { On 2015/06/30 17:33:53, kalman wrote: > You shouldn't need to change API features (this file) at all. You're not adding > an API, only a permission. Acknowledged.
Ok, I thought about it. I don't think we need any sort of permission, just make sure that these windows are only surfaced through the chrome.windows API (not chrome.tabs), and set the WindowType properly - it already has "app". Extensions should already be dealing with this in theory. The missing piece is surfacing additional data about the window which is usually reserved for tabs, like its URL, but we can worry about that later if ever.
On 2015/07/01 20:00:11, kalman wrote: > Ok, I thought about it. I don't think we need any sort of permission, just make > sure that these windows are only surfaced through the chrome.windows API (not > chrome.tabs), and set the WindowType properly - it already has "app". Extensions > should already be dealing with this in theory. > > The missing piece is surfacing additional data about the window which is usually > reserved for tabs, like its URL, but we can worry about that later if ever. PTAL. I might split the chromevox changes into another change, let me know what you prefer. Thanks.
https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/devtool... File chrome/browser/devtools/devtools_window.h (right): https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/devtool... chrome/browser/devtools/devtools_window.h:127: Browser* browser() { return browser_; } It would be nice not to expose this. https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/app_window_controller.h (right): https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/app_window_controller.h:29: virtual ~AppBaseWindow(); use "~AppBaseWindow() override" instead of virtual. https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:347: if (!this->IsFromSameProfile(*iter)) I still think we should be filtering out devtools, so a function call like "CanOperateOnWindow" is correct. However, the implementations of IsVisibleToExtension should remove references to whether the extension IDs match or what the extension type is. That belongs inside extension code, not in the WindowController implementations. In fact, I would do this: 1. Rename IsVisibleToExtension(extension) to IsHiddenFromExtensions(). 2. Provide a default implementation of IsHiddenFromExtensions which return false by default. 3. Remove most IsVisibleToExtension implementations, apart from that one is_devtools check. If at some point we do want to go back to checking IDs, or extension types, then it's better for WindowController to expose a GetExtension() function. https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_event_router.cc (right): https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_event_router.cc:53: for (AppWindow* app_window : registry->app_windows()) { Could you just have AppWindows have their own AppWindowControllers, rather than maintaining this map yourself here? https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_event_router.cc:55: app_window, make_scoped_ptr(new AppBaseWindow(app_window)).Pass(), You don't need to .Pass() something that is already an rvalue (and make_scoped_ptr returns an rvalue). https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... File chrome/browser/extensions/chrome_extension_function.h (right): https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... chrome/browser/extensions/chrome_extension_function.h:35: bool CanOperateOnWindow(const extensions::WindowController* window_controller) I think this method should be moved to windows_util (chrome/browser/extensions/api/tabs/windows_util.*) and out of ExtensionFunction/ExtensionFunctionDetails. That makes much more sense to me, and you can pull extra logic into there. https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/chromevox/background/background.js (right): https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/chromevox/background/background.js:123: // Avoid application windows, popup windows and panel windows. This change makes me worry. Well, it's a wake-up call that I should worry. I think that we should add a new parameter to the windows getters called "allTypes", which if true, returns all types of windows. Otherwise, it only returns normal windows, or whatever the old behavior was. I've put together a CL which will make this change trivial: https://codereview.chromium.org/1219983006/ A bit more complex, but backwards compatible. The comment about pulling the logic out of those "IsVisibleToExtension" functions and into the API implementation still applies.
Thanks for your time on reviewing this. Before I upload a new iteration with IsVisibleToExtension/IsHiddenFromExtensions, I have a question below about events on the chrome.windows API. https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/devtool... File chrome/browser/devtools/devtools_window.h (right): https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/devtool... chrome/browser/devtools/devtools_window.h:127: Browser* browser() { return browser_; } On 2015/07/07 21:42:23, kalman wrote: > It would be nice not to expose this. Thanks, just realized I can get through DevToolsWindowTesting. https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/app_window_controller.h (right): https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/app_window_controller.h:29: virtual ~AppBaseWindow(); On 2015/07/07 21:42:23, kalman wrote: > use "~AppBaseWindow() override" instead of virtual. ui::BaseWindow doesn't have a destructor so I have to leave it virtual. https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_event_router.cc (right): https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_event_router.cc:53: for (AppWindow* app_window : registry->app_windows()) { On 2015/07/07 21:42:23, kalman wrote: > Could you just have AppWindows have their own AppWindowControllers, rather than > maintaining this map yourself here? I wanted to do that first, but WindowController is part of chrome/browser/ whereas AppWindow is part of extensions/. There are dependencies that prevent me from doing this. https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_event_router.cc:55: app_window, make_scoped_ptr(new AppBaseWindow(app_window)).Pass(), On 2015/07/07 21:42:23, kalman wrote: > You don't need to .Pass() something that is already an rvalue (and > make_scoped_ptr returns an rvalue). Thanks, will change. https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/chromevox/background/background.js (right): https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/chromevox/background/background.js:123: // Avoid application windows, popup windows and panel windows. On 2015/07/07 21:42:23, kalman wrote: > This change makes me worry. Well, it's a wake-up call that I should worry. > > I think that we should add a new parameter to the windows getters called > "allTypes", which if true, returns all types of windows. Otherwise, it only > returns normal windows, or whatever the old behavior was. > > I've put together a CL which will make this change trivial: > https://codereview.chromium.org/1219983006/ > > A bit more complex, but backwards compatible. The comment about pulling the > logic out of those "IsVisibleToExtension" functions and into the API > implementation still applies. What about onCreated/onRemoved/onFocusChanged events?
https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/app_window_controller.h (right): https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/app_window_controller.h:29: virtual ~AppBaseWindow(); On 2015/07/08 13:07:58, llandwerlin wrote: > On 2015/07/07 21:42:23, kalman wrote: > > use "~AppBaseWindow() override" instead of virtual. > > ui::BaseWindow doesn't have a destructor so I have to leave it virtual. Huh, weird. ui::BaseWindow should really have a virtual destructor. Apparently the compiler will complain if it doesn't though, so technically it's safe. https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_event_router.cc (right): https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_event_router.cc:53: for (AppWindow* app_window : registry->app_windows()) { On 2015/07/08 13:07:58, llandwerlin wrote: > On 2015/07/07 21:42:23, kalman wrote: > > Could you just have AppWindows have their own AppWindowControllers, rather > than > > maintaining this map yourself here? > > I wanted to do that first, but WindowController is part of chrome/browser/ > whereas AppWindow is part of extensions/. There are dependencies that prevent me > from doing this. Damn. Ok. https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/chromevox/background/background.js (right): https://codereview.chromium.org/1099553002/diff/370001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/chromevox/background/background.js:123: // Avoid application windows, popup windows and panel windows. On 2015/07/08 13:07:59, llandwerlin wrote: > On 2015/07/07 21:42:23, kalman wrote: > > This change makes me worry. Well, it's a wake-up call that I should worry. > > > > I think that we should add a new parameter to the windows getters called > > "allTypes", which if true, returns all types of windows. Otherwise, it only > > returns normal windows, or whatever the old behavior was. > > > > I've put together a CL which will make this change trivial: > > https://codereview.chromium.org/1219983006/ > > > > A bit more complex, but backwards compatible. The comment about pulling the > > logic out of those "IsVisibleToExtension" functions and into the API > > implementation still applies. > > What about onCreated/onRemoved/onFocusChanged events? Damn. I guess what I said isn't possible then, except with a permission, and I don't want to do that. So never mind. Let's just announce it and leave it at that. +dmazzoni FYI.
PTAL. I've left the access to devtools windows, it would be nice to have the ability to move all windows. Otherwise the tiling extension I'm using just leaves these windows unaffected. Maybe I can write additional tests to address potential issues you can think of. Thanks.
On 2015/07/10 10:12:10, llandwerlin wrote: > PTAL. > I've left the access to devtools windows, it would be nice to have the ability > to move all windows. I agree it would be nice to manage devtools, but there was also https://code.google.com/p/chromium/issues/detail?id=427226. This doesn't reintroduce that, I hope? > Otherwise the tiling extension I'm using just leaves these windows unaffected. > Maybe I can write additional tests to address potential issues you can think of. > Thanks.
kalman@chromium.org changed reviewers: + benwells@chromium.org
lgtm but come to think of it I would feel a lot more comfortable if an app window owner looks at it as well, so wait for a review from benwells or delegate. ben - could you have a look at this change? https://codereview.chromium.org/1099553002/diff/410001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/app_window_controller.cc (right): https://codereview.chromium.org/1099553002/diff/410001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/app_window_controller.cc:96: // We constraint the given size to the min/max sizes of the constrain https://codereview.chromium.org/1099553002/diff/410001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/app_window_controller.cc:176: base::DictionaryValue* tab_value = new base::DictionaryValue(); You should be able to use https://code.google.com/p/chromium/codesearch#chromium/src/out/Debug/gen/chro... to save all of this manual base::Value wrangling. https://codereview.chromium.org/1099553002/diff/410001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_event_router.cc (right): https://codereview.chromium.org/1099553002/diff/410001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_event_router.cc:56: app_windows_.set(app_window->session_id().id(), controller.Pass()); Perhaps have an "AddControllerForWindow" function? then call just like AddControllerForWindow(app_window) here and below. https://codereview.chromium.org/1099553002/diff/410001/chrome/browser/extensi... File chrome/browser/extensions/browser_extension_window_controller.cc (right): https://codereview.chromium.org/1099553002/diff/410001/chrome/browser/extensi... chrome/browser/extensions/browser_extension_window_controller.cc:90: return browser_->is_devtools(); (weren't you going to remove this?)
benwells@chromium.org changed reviewers: + stevenjb@chromium.org
+stevenjb who is probably the best reviewer for this.
On 2015/07/10 17:34:59, kalman wrote: > On 2015/07/10 10:12:10, llandwerlin wrote: > > PTAL. > > I've left the access to devtools windows, it would be nice to have the ability > > to move all windows. > > I agree it would be nice to manage devtools, but there was also > https://code.google.com/p/chromium/issues/detail?id=427226. This doesn't > reintroduce that, I hope? > It doesn't reintroduce that problem, because devtools are still not accessible via the chrome.tabs API.
On 2015/07/13 10:17:58, llandwerlin wrote: > On 2015/07/10 17:34:59, kalman wrote: > > On 2015/07/10 10:12:10, llandwerlin wrote: > > > PTAL. > > > I've left the access to devtools windows, it would be nice to have the > ability > > > to move all windows. > > > > I agree it would be nice to manage devtools, but there was also > > https://code.google.com/p/chromium/issues/detail?id=427226. This doesn't > > reintroduce that, I hope? > > > > It doesn't reintroduce that problem, because devtools are still not accessible > via the chrome.tabs API. Ah sorry, misread the steps to reproduce, it still causes a crash. Looking into this.
On 2015/07/13 10:44:37, llandwerlin wrote: > On 2015/07/13 10:17:58, llandwerlin wrote: > > On 2015/07/10 17:34:59, kalman wrote: > > > On 2015/07/10 10:12:10, llandwerlin wrote: > > > > PTAL. > > > > I've left the access to devtools windows, it would be nice to have the > > ability > > > > to move all windows. > > > > > > I agree it would be nice to manage devtools, but there was also > > > https://code.google.com/p/chromium/issues/detail?id=427226. This doesn't > > > reintroduce that, I hope? > > > > > > > It doesn't reintroduce that problem, because devtools are still not accessible > > via the chrome.tabs API. > > Ah sorry, misread the steps to reproduce, it still causes a crash. > Looking into this. https://codereview.chromium.org/1228863006/ should fix this problem, I also have an additional test for this CL to ensure this scenario doesn't trigger a crash.
I haven't been in this code in a very long time, but I also realize that there may not be many (or any) devs who have. I would be much more comfortable if we added a parameter to windows.getAll() rather than potentially breaking existing extensions. (The fact that this CL includes changes to Chrome component extensions suggests to me that other extensions may break). That said, if the extensions team is comfortable with this change in behavior, I am happy to review the code changes. https://codereview.chromium.org/1099553002/diff/430001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/app_window_controller.h (right): https://codereview.chromium.org/1099553002/diff/430001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/app_window_controller.h:26: class AppBaseWindow : public ui::BaseWindow { This should really be in a separate file. https://codereview.chromium.org/1099553002/diff/430001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_event_router.h (right): https://codereview.chromium.org/1099553002/diff/430001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_event_router.h:91: base::ScopedPtrHashMap<int, scoped_ptr<AppWindowController>> app_windows_; This is really a map of window controllers, not the windows themselves, right? (The name is a bit confusing). https://codereview.chromium.org/1099553002/diff/430001/chrome/browser/resourc... File chrome/browser/resources/chromeos/chromevox/chromevox/background/background.js (right): https://codereview.chromium.org/1099553002/diff/430001/chrome/browser/resourc... chrome/browser/resources/chromeos/chromevox/chromevox/background/background.js:125: continue; Has there been sufficient discussion about whether or not it is OK to possibly break other extensions that use this API? Have we considered adding a parameter to windows.getAll, e.g. 'appWindows': true instead?
It would be nice to be able to control this with an argument to getAll. However,
it introduces some inconsistent behavior, like:
// There is no way to signal to onCreated that we don't
// want app windows, without adding filtered events, and this
// is a non-trivial change.
chrome.windows.onCreated.addListener(function(win) {
// It would be weird if you needed to supply an "app:true"
// option to this, given you just got it.
chrome.window.get(win.id, function() {
});
chrome.window.getAll(function(wins) {
// It would be weird if this function didn't return
// the window that you were just given.
});
});
This behavior would be closer to backwards compatible, but not fully backwards
compatible, and would introduce some funky corner case complexity.
The consistent way to fix this would be by adding a permission, or by adding a
field to the manifest, but this seems overly complex.
The other option would be to make the chrome.windows (and chrome.tabs?) events
take filters which by default filter out anything but browser tabs/windows, and
you must specify an additional filter to get other tabs. For example:
chrome.windows.onCreated.addListener(function(win) {
...
}, {
windowTypes: ['normal', 'app', 'devtools']
// or maybe
allWindowTypes: true
// or maybe
windowTypes: 'all'
});
I do think that's worth looking into. It serves an ulterior motive where I've
been wanting to introduce filters into the windows/tabs events anyway (in my
case, for URLs).
On 2015/07/20 21:56:58, kalman wrote:
> It would be nice to be able to control this with an argument to getAll.
However,
> it introduces some inconsistent behavior, like:
>
> // There is no way to signal to onCreated that we don't
> // want app windows, without adding filtered events, and this
> // is a non-trivial change.
> chrome.windows.onCreated.addListener(function(win) {
> // It would be weird if you needed to supply an "app:true"
> // option to this, given you just got it.
> chrome.window.get(win.id, function() {
> });
> chrome.window.getAll(function(wins) {
> // It would be weird if this function didn't return
> // the window that you were just given.
> });
> });
>
> This behavior would be closer to backwards compatible, but not fully backwards
> compatible, and would introduce some funky corner case complexity.
>
> The consistent way to fix this would be by adding a permission, or by adding a
> field to the manifest, but this seems overly complex.
>
> The other option would be to make the chrome.windows (and chrome.tabs?) events
> take filters which by default filter out anything but browser tabs/windows,
and
> you must specify an additional filter to get other tabs. For example:
>
> chrome.windows.onCreated.addListener(function(win) {
> ...
> }, {
> windowTypes: ['normal', 'app', 'devtools']
> // or maybe
> allWindowTypes: true
> // or maybe
> windowTypes: 'all'
> });
>
> I do think that's worth looking into. It serves an ulterior motive where I've
> been wanting to introduce filters into the windows/tabs events anyway (in my
> case, for URLs).
stevenjb@: Unless you have more comments, I'm going to try add these filters.
On 2015/07/23 12:54:08, llandwerlin wrote:
> On 2015/07/20 21:56:58, kalman wrote:
> > It would be nice to be able to control this with an argument to getAll.
> However,
> > it introduces some inconsistent behavior, like:
> >
> > // There is no way to signal to onCreated that we don't
> > // want app windows, without adding filtered events, and this
> > // is a non-trivial change.
> > chrome.windows.onCreated.addListener(function(win) {
> > // It would be weird if you needed to supply an "app:true"
> > // option to this, given you just got it.
> > chrome.window.get(win.id, function() {
> > });
> > chrome.window.getAll(function(wins) {
> > // It would be weird if this function didn't return
> > // the window that you were just given.
> > });
> > });
> >
> > This behavior would be closer to backwards compatible, but not fully
backwards
> > compatible, and would introduce some funky corner case complexity.
> >
> > The consistent way to fix this would be by adding a permission, or by adding
a
> > field to the manifest, but this seems overly complex.
> >
> > The other option would be to make the chrome.windows (and chrome.tabs?)
events
> > take filters which by default filter out anything but browser tabs/windows,
> and
> > you must specify an additional filter to get other tabs. For example:
> >
> > chrome.windows.onCreated.addListener(function(win) {
> > ...
> > }, {
> > windowTypes: ['normal', 'app', 'devtools']
> > // or maybe
> > allWindowTypes: true
> > // or maybe
> > windowTypes: 'all'
> > });
> >
> > I do think that's worth looking into. It serves an ulterior motive where
I've
> > been wanting to introduce filters into the windows/tabs events anyway (in my
> > case, for URLs).
>
> stevenjb@: Unless you have more comments, I'm going to try add these filters.
The filters sound good to me. If there is a path that allows us to get App
windows but not by default so that existing extensions do not change their
behavior, that sounds like a complete win to me.
lionel.g.landwerlin@intel.com changed reviewers: - benwells@chromium.org
Patchset #23 (id:430001) has been deleted
Patchset #22 (id:410001) has been deleted
Patchset #6 (id:100001) has been deleted
Patchset #4 (id:60001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:80001) has been deleted
Patchset #7 (id:200001) has been deleted
Patchset #12 (id:320001) has been deleted
Patchset #14 (id:370001) has been deleted
Patchset #12 (id:340001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
Patchset #6 (id:240001) has been deleted
Patchset #7 (id:280001) has been deleted
Patchset #12 (id:490001) has been deleted
Patchset #13 (id:530001) has been deleted
Patchset #9 (id:390001) has been deleted
Patchset #12 (id:550001) has been deleted
Patchset #7 (id:300001) has been deleted
Patchset #5 (id:220001) has been deleted
On 2015/07/23 17:20:29, stevenjb wrote:
>
> The filters sound good to me. If there is a path that allows us to get App
> windows but not by default so that existing extensions do not change their
> behavior, that sounds like a complete win to me.
I've made some progress on adding filters, but some problems remain. In
particular with regards to events.
The current implementation of event filters won't dispatch unless a filter is
given.
For example :
chrome.webNavigation.onCompleted.addListener(function() {});
will never fire the callback.
So right now adding filters to the chrome.windows APIs will break existing use
cases.
Also currently chrome.windows events do are emitted for at 'normal' 'popup' and
'app' types of windows. But in the case of 'app' windows, it only happens for
windows that have been created by the same extension. So if we want to exactly
reproduce the current behavior, while allowing other application windows,
filtering on window types is not possible.
Any thoughts?
(I'm currently looking at event rules, but I'm not sure whether if that will be
useful)
Thanks.
On 2015/07/29 15:38:31, llandwerlin wrote:
> On 2015/07/23 17:20:29, stevenjb wrote:
>
> >
> > The filters sound good to me. If there is a path that allows us to get App
> > windows but not by default so that existing extensions do not change their
> > behavior, that sounds like a complete win to me.
>
> I've made some progress on adding filters, but some problems remain. In
> particular with regards to events.
>
> The current implementation of event filters won't dispatch unless a filter is
> given.
> For example :
>
> chrome.webNavigation.onCompleted.addListener(function() {});
>
> will never fire the callback.
Really? That's very surprising because the webNavigation API predates filtered
events. If we'd made this change it would have been backwards incompatible, and
I would have expected more complaints. If this really is true could you file a
bug on it and we can fix it?
>
> So right now adding filters to the chrome.windows APIs will break existing use
> cases.
>
> Also currently chrome.windows events do are emitted for at 'normal' 'popup'
and
> 'app' types of windows. But in the case of 'app' windows, it only happens for
> windows that have been created by the same extension. So if we want to exactly
> reproduce the current behavior, while allowing other application windows,
> filtering on window types is not possible.
Whoa what is going on here. This all seems unintended. Do get() and getAll()
also return app windows that have been created by the extension? And...
extensions can't even create app windows, so how is that happening?
>
> Any thoughts?
>
> (I'm currently looking at event rules, but I'm not sure whether if that will
be
> useful)
>
> Thanks.
On 2015/07/29 18:33:38, kalman wrote:
> On 2015/07/29 15:38:31, llandwerlin wrote:
> > On 2015/07/23 17:20:29, stevenjb wrote:
> >
> > >
> > > The filters sound good to me. If there is a path that allows us to get App
> > > windows but not by default so that existing extensions do not change their
> > > behavior, that sounds like a complete win to me.
> >
> > I've made some progress on adding filters, but some problems remain. In
> > particular with regards to events.
> >
> > The current implementation of event filters won't dispatch unless a filter
is
> > given.
> > For example :
> >
> > chrome.webNavigation.onCompleted.addListener(function() {});
> >
> > will never fire the callback.
>
> Really? That's very surprising because the webNavigation API predates filtered
> events. If we'd made this change it would have been backwards incompatible,
and
> I would have expected more complaints. If this really is true could you file a
> bug on it and we can fix it?
Sorry, I just realized I got that wrong because of a bug I introduced into the
event_filtering_info.cc.
Dismiss this completely, but I think the second point still stands.
>
> >
> > So right now adding filters to the chrome.windows APIs will break existing
use
> > cases.
> >
> > Also currently chrome.windows events do are emitted for at 'normal' 'popup'
> and
> > 'app' types of windows. But in the case of 'app' windows, it only happens
for
> > windows that have been created by the same extension. So if we want to
exactly
> > reproduce the current behavior, while allowing other application windows,
> > filtering on window types is not possible.
>
> Whoa what is going on here. This all seems unintended. Do get() and getAll()
> also return app windows that have been created by the extension? And...
> extensions can't even create app windows, so how is that happening?
Sorry again, I meant 'panel', I don't think we can differentiate 2 panel window
created by 2 different extension based on their type.
> > Whoa what is going on here. This all seems unintended. Do get() and getAll() > > also return app windows that have been created by the extension? And... > > extensions can't even create app windows, so how is that happening? > > Sorry again, I meant 'panel', I don't think we can differentiate 2 panel window > created by 2 different extension based on their type. What about the get() and getAll() question? Do they include panels iff they are created by the same extension? This still does seems weird to me, and I'm ok breaking this so that they return all panels. Panels are still associated with a browser; the new idiom is that app windows aren't.
On 2015/07/29 19:47:05, kalman wrote: > > > Whoa what is going on here. This all seems unintended. Do get() and getAll() > > > also return app windows that have been created by the extension? And... > > > extensions can't even create app windows, so how is that happening? > > > > Sorry again, I meant 'panel', I don't think we can differentiate 2 panel > window > > created by 2 different extension based on their type. > > What about the get() and getAll() question? Do they include panels iff they are > created by the same extension? There doesn't seem to be a test actually verifying that it does (I'll write one to confirm), but I'm pretty sure they do since the filtering is based on extension id : https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... ) > > This still does seems weird to me, and I'm ok breaking this so that they return > all panels. Panels are still associated with a browser; the new idiom is that > app windows aren't. Ok, I have filtering mostly working now, but I used something like this : chrome.windows.getAll({ allWindowTypes: true}, function(windows) { ... }); chrome.windows.onCreated.addListener(function(win) { ... }, { allWindowTypes: true}); Would you prefer something more like { windowTypes: ['normal', 'app', 'devtools'] }?
On 2015/07/29 20:56:36, llandwerlin wrote: > On 2015/07/29 19:47:05, kalman wrote: > > > > Whoa what is going on here. This all seems unintended. Do get() and > getAll() > > > > also return app windows that have been created by the extension? And... > > > > extensions can't even create app windows, so how is that happening? > > > > > > Sorry again, I meant 'panel', I don't think we can differentiate 2 panel > > window > > > created by 2 different extension based on their type. > > > > What about the get() and getAll() question? Do they include panels iff they > are > > created by the same extension? > > There doesn't seem to be a test actually verifying that it does (I'll write one > to confirm), but I'm pretty sure they do since the filtering is based on > extension id : > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > ) > > > > > This still does seems weird to me, and I'm ok breaking this so that they > return > > all panels. Panels are still associated with a browser; the new idiom is that > > app windows aren't. > > Ok, I have filtering mostly working now, but I used something like this : > > chrome.windows.getAll({ allWindowTypes: true}, function(windows) { ... }); > chrome.windows.onCreated.addListener(function(win) { ... }, { allWindowTypes: > true}); > > Would you prefer something more like { windowTypes: ['normal', 'app', > 'devtools'] }? Yeah that looks better; and defaulting to ['normal', 'panels'] right?
On 2015/07/29 20:58:15, kalman wrote: > On 2015/07/29 20:56:36, llandwerlin wrote: > > On 2015/07/29 19:47:05, kalman wrote: > > > > > Whoa what is going on here. This all seems unintended. Do get() and > > getAll() > > > > > also return app windows that have been created by the extension? And... > > > > > extensions can't even create app windows, so how is that happening? > > > > > > > > Sorry again, I meant 'panel', I don't think we can differentiate 2 panel > > > window > > > > created by 2 different extension based on their type. > > > > > > What about the get() and getAll() question? Do they include panels iff they > > are > > > created by the same extension? > > > > There doesn't seem to be a test actually verifying that it does (I'll write > one > > to confirm), but I'm pretty sure they do since the filtering is based on > > extension id : > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext... > > ) > > > > > > > > This still does seems weird to me, and I'm ok breaking this so that they > > return > > > all panels. Panels are still associated with a browser; the new idiom is > that > > > app windows aren't. > > > > Ok, I have filtering mostly working now, but I used something like this : > > > > chrome.windows.getAll({ allWindowTypes: true}, function(windows) { ... }); > > chrome.windows.onCreated.addListener(function(win) { ... }, { allWindowTypes: > > true}); > > > > Would you prefer something more like { windowTypes: ['normal', 'app', > > 'devtools'] }? > > Yeah that looks better; and defaulting to ['normal', 'panels'] right? Almost, ['normal', 'panel', 'popup'] : https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
Patchset #11 (id:590001) has been deleted
Patchset #9 (id:510001) has been deleted
Patchset #10 (id:610001) has been deleted
Patchset #11 (id:650001) has been deleted
Patchset #11 (id:650001) has been deleted
Patchset #11 (id:670001) has been deleted
Patchset #8 (id:470001) has been deleted
Patchset #5 (id:260001) has been deleted
PTAL, thanks.
This is a pretty large CL; would it be reasonable to separate out the AppWindowController re-factoring (which seems like a good thing regardless of any extension API changes)? That way, if we run into any problems with the API changes, we have a much smaller LC to revert (and review). Thanks!
What diff are we up to now?
Patchset #12 (id:750001) has been deleted
Here is a version without the AppWindowController change. Sorry it took so long. At this point I could close this and reupload a new CL, because it's fairly different from what was there even 2 weeks ago.
Thanks for separating the CLs, much easier to review now. https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:331: } Given that we seem to copy/paste the same code three times, maybe wrap this in a helper function, e.g. GetCommonParams(&populate_tabs, &type_filter) ? That could also set type_filter to windows_util::GetDefaultWindowTypeFilters() if null and simplify the rest of the code a bit. https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:368: } Four times... :) https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_event_router.cc (right): https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_event_router.cc:203: event->filter_info = info; nit: I think this can be just: event->filter_info.SetWindowType(...) since we just created a new Event? https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_event_router.cc:217: event->filter_info = info; Same here https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_util.h (right): https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.h:27: const std::vector<windows::WindowType>& GetNoWindowTypeFilters(); We should typedef std::vector<windows::WindowType> as something like WindowTypeFilter. Also, is there any particular reason to use a vector<> instead of a set<>?
https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:331: } On 2015/07/30 19:00:37, stevenjb wrote: > Given that we seem to copy/paste the same code three times, maybe wrap this in a > helper function, e.g. GetCommonParams(&populate_tabs, &type_filter) ? That could > also set type_filter to windows_util::GetDefaultWindowTypeFilters() if null and > simplify the rest of the code a bit. > I can't really use a helper function because the params is a different type for each extension function. Added a template class to extract the parameters. https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:368: } On 2015/07/30 19:00:37, stevenjb wrote: > Four times... :) Done. https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_event_router.cc (right): https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_event_router.cc:203: event->filter_info = info; On 2015/07/30 19:00:37, stevenjb wrote: > nit: I think this can be just: event->filter_info.SetWindowType(...) since we > just created a new Event? Done. https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_event_router.cc:217: event->filter_info = info; On 2015/07/30 19:00:37, stevenjb wrote: > Same here Done. https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_util.h (right): https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.h:27: const std::vector<windows::WindowType>& GetNoWindowTypeFilters(); On 2015/07/30 19:00:37, stevenjb wrote: > We should typedef std::vector<windows::WindowType> as something like > WindowTypeFilter. > > Also, is there any particular reason to use a vector<> instead of a set<>? Done in WindowControllerList. I kept vector<> because it prevents a conversion from the api parameters (which is vector<>) to something else. That way I can just hand over the received filter. But maybe it makes sense to copy anyway?
lgtm! Ideally someone more familiar with the event filtering should take a closer look at that than I did. https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_api.cc (right): https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_api.cc:331: } On 2015/07/31 13:58:32, llandwerlin wrote: > On 2015/07/30 19:00:37, stevenjb wrote: > > Given that we seem to copy/paste the same code three times, maybe wrap this in > a > > helper function, e.g. GetCommonParams(&populate_tabs, &type_filter) ? That > could > > also set type_filter to windows_util::GetDefaultWindowTypeFilters() if null > and > > simplify the rest of the code a bit. > > > > I can't really use a helper function because the params is a different type for > each extension function. > Added a template class to extract the parameters. Ugh, you're right, I missed that. It's too bad we don't (and maybe can't) share the struct across the API. A template function works though, thanks. https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_util.h (right): https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.h:27: const std::vector<windows::WindowType>& GetNoWindowTypeFilters(); On 2015/07/31 13:58:32, llandwerlin wrote: > On 2015/07/30 19:00:37, stevenjb wrote: > > We should typedef std::vector<windows::WindowType> as something like > > WindowTypeFilter. > > > > Also, is there any particular reason to use a vector<> instead of a set<>? > > Done in WindowControllerList. > > I kept vector<> because it prevents a conversion from the api parameters (which > is vector<>) to something else. That way I can just hand over the received > filter. > But maybe it makes sense to copy anyway? I think a copy might be slightly cheaper than the vector lookups that would become trivial with a set, but the number of types is so small it really doesn't matter. I would go with whatever simplifies the code the most myself, but it's fine either way. The typedef helps a lot for readability regardless.
https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_util.cc (right): https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.cc:46: windows::WINDOW_TYPE_POPUP}; There are so few of these, you could model it as a bitmask? The whole static-local-return-reference thing is more complicated than it needs to be, I think. I'd also put the WindowMatchesTypeFilter function as a method on WindowTypeFilter: class WindowTypeFilter { public: bool MatchesType(WindowType window_type) { return filter_ & (1 << window_type); } ... whatever. private: // Bitmask of WindowTypes, or -1 to match all types. int filter_; }; After that, just copy it everywhere rather than using references. https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.cc:52: const extensions::WindowTypeFilter& GetNoWindowTypeFilters() { I don't understand this naming... looks like it matches all window types, not "no"? https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.cc:64: int window_type = extensions::api::windows::ParseWindowType( this should be a WindowType not an int. https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.cc:117: if (!extensions::util::CanCrossIncognito(extension, browser_context)) The other implementation uses function->include_incognito() which eventually comes from a call to ChromeExtensionsBrowserClient::CanExtensionCrossIncognito (trace through to the call site of set_incognito_enabled). https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_util.h (right): https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.h:26: const extensions::WindowTypeFilter& GetNoWindowTypeFilters(); Could you comment all of these new functions? https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.h:52: content::BrowserContext* browser_context, I'd just change CanOperateOnWindow to take a BrowserContext/Extension rather than having 2 separate versions. There are only a few call sites. It would be nice if GetWindowFromWindowID were given the same treatment, but that has too many dependencies unfortunately. (also, nitty, but prefer arguments to be passed in in order of generality, so BrowserContext would go before Extension). https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... File chrome/browser/extensions/window_controller_list.cc (right): https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... chrome/browser/extensions/window_controller_list.cc:72: windows_util::WindowMatchesTypeFilter(*iter, filter)) To share code with this and FindWindowById, and once these filters are especially cheap, I'd make FindWindowId directly call FindWindowByIdWithFilter with a filter that matches everything. Similar for other functions. https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... chrome/browser/extensions/window_controller_list.cc:75: return NULL; NULL --> nullptr https://codereview.chromium.org/1099553002/diff/810001/chrome/common/extensio... File chrome/common/extensions/api/windows.json (right): https://codereview.chromium.org/1099553002/diff/810001/chrome/common/extensio... chrome/common/extensions/api/windows.json:279: "description": "Conditions that the window's type being created must satisfy." "By default it will satisfy...." and same for the others. https://codereview.chromium.org/1099553002/diff/810001/extensions/common/even... File extensions/common/event_filtering_info.h (right): https://codereview.chromium.org/1099553002/diff/810001/extensions/common/even... extensions/common/event_filtering_info.h:36: bool has_window_type() const { return has_window_type_; } Could you add a comment: Note: window type is a Chrome concept, so arguably doesn't belong in the extensions module. If the number of Chrome concept grows, consider a delegation model with a ChromeEventFilteringInfo class. https://codereview.chromium.org/1099553002/diff/810001/extensions/common/even... File extensions/common/event_matcher.cc (right): https://codereview.chromium.org/1099553002/diff/810001/extensions/common/even... extensions/common/event_matcher.cc:84: base::ListValue* window_type_filters = NULL; NULL --> nullptr https://codereview.chromium.org/1099553002/diff/810001/extensions/common/even... extensions/common/event_matcher.cc:94: } else if (i >= 0 && i < static_cast<int>(arraysize(kDefaultWindowTypes))) { In this case I think it's clearer if you don't put this in an "else". It's redundant anyway since the previous line is a "return". Also, I'm surprised you need the static_cast, but... ok.
https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_util.h (right): https://codereview.chromium.org/1099553002/diff/770001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.h:27: const std::vector<windows::WindowType>& GetNoWindowTypeFilters(); On 2015/07/31 17:27:16, stevenjb wrote: > On 2015/07/31 13:58:32, llandwerlin wrote: > > On 2015/07/30 19:00:37, stevenjb wrote: > > > We should typedef std::vector<windows::WindowType> as something like > > > WindowTypeFilter. > > > > > > Also, is there any particular reason to use a vector<> instead of a set<>? > > > > Done in WindowControllerList. > > > > I kept vector<> because it prevents a conversion from the api parameters > (which > > is vector<>) to something else. That way I can just hand over the received > > filter. > > But maybe it makes sense to copy anyway? > > I think a copy might be slightly cheaper than the vector lookups that would > become trivial with a set, but the number of types is so small it really doesn't > matter. I would go with whatever simplifies the code the most myself, but it's > fine either way. The typedef helps a lot for readability regardless. > Switching to converting the array to a bit field. https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_util.cc (right): https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.cc:46: windows::WINDOW_TYPE_POPUP}; On 2015/07/31 21:48:03, kalman wrote: > There are so few of these, you could model it as a bitmask? The whole > static-local-return-reference thing is more complicated than it needs to be, I > think. I'd also put the WindowMatchesTypeFilter function as a method on > WindowTypeFilter: > > class WindowTypeFilter { > public: > bool MatchesType(WindowType window_type) { > return filter_ & (1 << window_type); > } > > ... whatever. > > private: > // Bitmask of WindowTypes, or -1 to match all types. > int filter_; > }; > > After that, just copy it everywhere rather than using references. Switching to a bitmask. https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.cc:52: const extensions::WindowTypeFilter& GetNoWindowTypeFilters() { On 2015/07/31 21:48:03, kalman wrote: > I don't understand this naming... looks like it matches all window types, not > "no"? Done. https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.cc:64: int window_type = extensions::api::windows::ParseWindowType( On 2015/07/31 21:48:03, kalman wrote: > this should be a WindowType not an int. Done. https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.cc:117: if (!extensions::util::CanCrossIncognito(extension, browser_context)) On 2015/07/31 21:48:03, kalman wrote: > The other implementation uses function->include_incognito() which eventually > comes from a call to ChromeExtensionsBrowserClient::CanExtensionCrossIncognito > (trace through to the call site of set_incognito_enabled). It looks like the remaining of a previous iteration of this CL. Removing as it's not used anymore. https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_util.h (right): https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.h:26: const extensions::WindowTypeFilter& GetNoWindowTypeFilters(); On 2015/07/31 21:48:03, kalman wrote: > Could you comment all of these new functions? Done. https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.h:52: content::BrowserContext* browser_context, On 2015/07/31 21:48:03, kalman wrote: > I'd just change CanOperateOnWindow to take a BrowserContext/Extension rather > than having 2 separate versions. There are only a few call sites. > > It would be nice if GetWindowFromWindowID were given the same treatment, but > that has too many dependencies unfortunately. > > (also, nitty, but prefer arguments to be passed in in order of generality, so > BrowserContext would go before Extension). Sorry, this isn't used anymore. Removing. https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... File chrome/browser/extensions/window_controller_list.cc (right): https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... chrome/browser/extensions/window_controller_list.cc:72: windows_util::WindowMatchesTypeFilter(*iter, filter)) On 2015/07/31 21:48:03, kalman wrote: > To share code with this and FindWindowById, and once these filters are > especially cheap, I'd make FindWindowId directly call FindWindowByIdWithFilter > with a filter that matches everything. > > Similar for other functions. Done. https://codereview.chromium.org/1099553002/diff/810001/chrome/browser/extensi... chrome/browser/extensions/window_controller_list.cc:75: return NULL; On 2015/07/31 21:48:03, kalman wrote: > NULL --> nullptr Done. https://codereview.chromium.org/1099553002/diff/810001/chrome/common/extensio... File chrome/common/extensions/api/windows.json (right): https://codereview.chromium.org/1099553002/diff/810001/chrome/common/extensio... chrome/common/extensions/api/windows.json:279: "description": "Conditions that the window's type being created must satisfy." On 2015/07/31 21:48:03, kalman wrote: > "By default it will satisfy...." > > and same for the others. Done. https://codereview.chromium.org/1099553002/diff/810001/extensions/common/even... File extensions/common/event_filtering_info.h (right): https://codereview.chromium.org/1099553002/diff/810001/extensions/common/even... extensions/common/event_filtering_info.h:36: bool has_window_type() const { return has_window_type_; } On 2015/07/31 21:48:03, kalman wrote: > Could you add a comment: > > Note: window type is a Chrome concept, so arguably doesn't belong in the > extensions module. If the number of Chrome concept grows, consider a delegation > model with a ChromeEventFilteringInfo class. Done. https://codereview.chromium.org/1099553002/diff/810001/extensions/common/even... File extensions/common/event_matcher.cc (right): https://codereview.chromium.org/1099553002/diff/810001/extensions/common/even... extensions/common/event_matcher.cc:84: base::ListValue* window_type_filters = NULL; On 2015/07/31 21:48:03, kalman wrote: > NULL --> nullptr Done. https://codereview.chromium.org/1099553002/diff/810001/extensions/common/even... extensions/common/event_matcher.cc:94: } else if (i >= 0 && i < static_cast<int>(arraysize(kDefaultWindowTypes))) { On 2015/07/31 21:48:03, kalman wrote: > In this case I think it's clearer if you don't put this in an "else". It's > redundant anyway since the previous line is a "return". > > Also, I'm surprised you need the static_cast, but... ok. Done, fyi arraysize is defined using sizeof which returns a size_t. On Linux 64bits int=4bytes, size_t=8bytes.
https://codereview.chromium.org/1099553002/diff/850001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_util.h (right): https://codereview.chromium.org/1099553002/diff/850001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.h:26: extensions::WindowTypeFilter GetDefaultWindowTypeFilter(); It would be better for the enum and the utils that deal with the enum were in the same file, whether that means moving WindowTypeFilter/WindowTypeFilterValue into here, or moving the utilities out of here. I think the latter is better. As I mention below you could move WindowMatchesTypeFilter into a WindowController::MatchesFilter method, and then GetAllWindowTypeFilter/WindowTypeFilterFromWindowTypes into static functions on that class. https://codereview.chromium.org/1099553002/diff/850001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.h:30: extensions::WindowTypeFilter GetAllWindowTypeFilter(); GetAllWindowTypesFilter. https://codereview.chromium.org/1099553002/diff/850001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.h:34: extensions::WindowTypeFilter filter); A method on extensions::WindowController called "MatchesFilter" would be better? https://codereview.chromium.org/1099553002/diff/850001/chrome/browser/extensi... File chrome/browser/extensions/window_controller_list.h (right): https://codereview.chromium.org/1099553002/diff/850001/chrome/browser/extensi... chrome/browser/extensions/window_controller_list.h:22: enum WindowTypeFilterValues : uint32 { Hm, I've never seen ": uint32" be needed before, is it actually necessary?
https://codereview.chromium.org/1099553002/diff/850001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/windows_util.h (right): https://codereview.chromium.org/1099553002/diff/850001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.h:26: extensions::WindowTypeFilter GetDefaultWindowTypeFilter(); On 2015/08/04 18:05:03, kalman wrote: > It would be better for the enum and the utils that deal with the enum were in > the same file, whether that means moving WindowTypeFilter/WindowTypeFilterValue > into here, or moving the utilities out of here. > > I think the latter is better. As I mention below you could move > WindowMatchesTypeFilter into a WindowController::MatchesFilter method, and then > GetAllWindowTypeFilter/WindowTypeFilterFromWindowTypes into static functions on > that class. Done, much simpler indeed. Thanks. https://codereview.chromium.org/1099553002/diff/850001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.h:30: extensions::WindowTypeFilter GetAllWindowTypeFilter(); On 2015/08/04 18:05:03, kalman wrote: > GetAllWindowTypesFilter. Since you proposed MatchesFilter, I renamed these to GetDefaultWindowFilter() and GetAllWindowFilter(). https://codereview.chromium.org/1099553002/diff/850001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/windows_util.h:34: extensions::WindowTypeFilter filter); On 2015/08/04 18:05:03, kalman wrote: > A method on extensions::WindowController called "MatchesFilter" would be better? Done. https://codereview.chromium.org/1099553002/diff/850001/chrome/browser/extensi... File chrome/browser/extensions/window_controller_list.h (right): https://codereview.chromium.org/1099553002/diff/850001/chrome/browser/extensi... chrome/browser/extensions/window_controller_list.h:22: enum WindowTypeFilterValues : uint32 { On 2015/08/04 18:05:03, kalman wrote: > Hm, I've never seen ": uint32" be needed before, is it actually necessary? dropped the : uint32 and moved the enum inside window_controller.cc as nobody needs access to these values.
https://codereview.chromium.org/1099553002/diff/870001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/1099553002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1272: // platform. Disable focus event tests for now. Old indentation was correct. https://codereview.chromium.org/1099553002/diff/870001/chrome/browser/extensi... File chrome/browser/extensions/window_controller.cc (right): https://codereview.chromium.org/1099553002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/window_controller.cc:22: enum WindowTypeFilterValues { Actually why do you even need this; can you use the api::windows::WINDOW_TYPE... values directly? https://codereview.chromium.org/1099553002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/window_controller.cc:37: Update_WindowControllerList_to_match_WindowType); I think this COMPILE_ASSERT can be left out. This code is generated. https://codereview.chromium.org/1099553002/diff/870001/chrome/browser/extensi... File chrome/browser/extensions/window_controller.h (right): https://codereview.chromium.org/1099553002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/window_controller.h:34: typedef uint32 WindowTypeFilter; We're supposed to use "using" these days. I'd also prefer for it to be declared inside WindowController.
https://codereview.chromium.org/1099553002/diff/870001/chrome/browser/extensi... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/1099553002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/api/tabs/tabs_test.cc:1272: // platform. Disable focus event tests for now. On 2015/08/05 17:33:25, kalman wrote: > Old indentation was correct. Done. https://codereview.chromium.org/1099553002/diff/870001/chrome/browser/extensi... File chrome/browser/extensions/window_controller.cc (right): https://codereview.chromium.org/1099553002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/window_controller.cc:22: enum WindowTypeFilterValues { On 2015/08/05 17:33:25, kalman wrote: > Actually why do you even need this; can you use the api::windows::WINDOW_TYPE... > values directly? Done. https://codereview.chromium.org/1099553002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/window_controller.cc:37: Update_WindowControllerList_to_match_WindowType); On 2015/08/05 17:33:25, kalman wrote: > I think this COMPILE_ASSERT can be left out. This code is generated. I want make sure that if someone adds a new window type they will notice that this file needs to be updated. You're right, replacing with == 5. https://codereview.chromium.org/1099553002/diff/870001/chrome/browser/extensi... File chrome/browser/extensions/window_controller.h (right): https://codereview.chromium.org/1099553002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/window_controller.h:34: typedef uint32 WindowTypeFilter; On 2015/08/05 17:33:25, kalman wrote: > We're supposed to use "using" these days. I'd also prefer for it to be declared > inside WindowController. Done.
lgtm https://codereview.chromium.org/1099553002/diff/870001/chrome/browser/extensi... File chrome/browser/extensions/window_controller.cc (right): https://codereview.chromium.org/1099553002/diff/870001/chrome/browser/extensi... chrome/browser/extensions/window_controller.cc:37: Update_WindowControllerList_to_match_WindowType); On 2015/08/05 18:12:02, llandwerlin wrote: > On 2015/08/05 17:33:25, kalman wrote: > > I think this COMPILE_ASSERT can be left out. This code is generated. > > I want make sure that if someone adds a new window type they will notice that > this file needs to be updated. > You're right, replacing with == 5. That makes sense. https://codereview.chromium.org/1099553002/diff/890001/chrome/browser/extensi... File chrome/browser/extensions/window_controller.cc (right): https://codereview.chromium.org/1099553002/diff/890001/chrome/browser/extensi... chrome/browser/extensions/window_controller.cc:43: for (unsigned int i = 0; i < types.size(); i++) size_t not unsigned int, but then again, range-based for loop would be even better: for (auto& window_type : types) filter |= 1 << window_type;
https://codereview.chromium.org/1099553002/diff/890001/chrome/browser/extensi... File chrome/browser/extensions/window_controller.cc (right): https://codereview.chromium.org/1099553002/diff/890001/chrome/browser/extensi... chrome/browser/extensions/window_controller.cc:43: for (unsigned int i = 0; i < types.size(); i++) On 2015/08/05 18:17:39, kalman wrote: > size_t not unsigned int, but then again, range-based for loop would be even > better: > > for (auto& window_type : types) > filter |= 1 << window_type; Done.
The CQ bit was checked by lionel.g.landwerlin@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, stevenjb@chromium.org, kalman@chromium.org Link to the patchset: https://codereview.chromium.org/1099553002/#ps910001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1099553002/910001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1099553002/910001
Message was sent while issue was closed.
Committed patchset #19 (id:910001)
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/56b2a727992f8caf2141f89bf0073793f5cbe3d8 Cr-Commit-Position: refs/heads/master@{#342016} |
