Chromium Code Reviews
Help | Chromium Project | Sign in
(654)

Issue 9428018: Create BaseWindow and ExtensionWindowWrapper for extension API access to Panels (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 4 months ago by stevenjb
Modified:
3 years, 4 months ago
CC:
chromium-reviews, Aaron Boodman, mihaip+watch_chromium.org, rhashimoto
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Create BaseWindow and ExtensionWindowWrapper for extension API access to Panels Tests are being re-enabled in http://codereview.chromium.org/9372126/. There appears to already be pretty reasonable test coverage. BUG=113412, 98759 TEST=Ensure all tests (especially browser_tests --gtest_filter=ExtensionApiTest*) run on all platforms. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124054

Patch Set 1 #

Total comments: 34

Patch Set 2 : Rebase + support fullscreen. #

Patch Set 3 : Address comments, rename -> ExtensionWindowController #

Total comments: 10

Patch Set 4 : Address comments. #

Patch Set 5 : Address comments. #

Total comments: 17

Patch Set 6 : Address feedback and (hopefully) fix tests. #

Patch Set 7 : Rebase #

Patch Set 8 : Fix GetLastFocusedWindow test #

Patch Set 9 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+758 lines, -155 lines) Patch
A chrome/browser/extensions/browser_extension_window_controller.h View 1 2 1 chunk +31 lines, -0 lines 0 comments Download
A chrome/browser/extensions/browser_extension_window_controller.cc View 1 2 3 4 5 1 chunk +63 lines, -0 lines 0 comments Download
M chrome/browser/extensions/extension_browser_event_router.cc View 1 2 3 4 5 6 2 chunks +4 lines, -2 lines 0 comments Download
M chrome/browser/extensions/extension_tab_util.h View 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/extensions/extension_tab_util.cc View 1 2 chunks +2 lines, -32 lines 0 comments Download
M chrome/browser/extensions/extension_tabs_module.cc View 1 2 3 4 5 6 7 16 chunks +112 lines, -56 lines 0 comments Download
M chrome/browser/extensions/extension_tabs_test.cc View 1 2 3 4 5 6 1 chunk +5 lines, -3 lines 0 comments Download
A chrome/browser/extensions/extension_window_controller.h View 1 2 3 4 5 1 chunk +72 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_window_controller.cc View 1 2 3 4 5 1 chunk +58 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_window_list.h View 1 2 3 4 5 6 7 1 chunk +51 lines, -0 lines 0 comments Download
A chrome/browser/extensions/extension_window_list.cc View 1 2 3 4 5 6 7 1 chunk +67 lines, -0 lines 0 comments Download
M chrome/browser/infobars/infobar_extension_api.cc View 1 2 2 chunks +3 lines, -1 line 0 comments Download
A chrome/browser/ui/base_window.h View 1 2 1 chunk +70 lines, -0 lines 0 comments Download
M chrome/browser/ui/browser.h View 1 2 3 4 5 6 7 8 4 chunks +8 lines, -1 line 0 comments Download
M chrome/browser/ui/browser.cc View 1 2 3 4 5 6 7 8 3 chunks +7 lines, -2 lines 0 comments Download
M chrome/browser/ui/browser_window.h View 1 2 3 4 5 3 chunks +13 lines, -47 lines 0 comments Download
M chrome/browser/ui/fullscreen_controller.h View 1 2 chunks +1 line, -2 lines 0 comments Download
M chrome/browser/ui/fullscreen_controller.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/aura/panel_view_aura.h View 1 2 3 4 chunks +29 lines, -1 line 0 comments Download
M chrome/browser/ui/views/aura/panel_view_aura.cc View 1 2 3 4 5 5 chunks +152 lines, -4 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 8 3 chunks +7 lines, -0 lines 0 comments Download
Commit: CQ not working?

Messages

Total messages: 21 (0 generated)
stevenjb
This is still a work-in-progress but is ready for initial feedback. Any comments are welcome, ...
3 years, 4 months ago (2012-02-22 02:20:20 UTC) #1
sky
http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/browser_extension_window_wrapper.cc File chrome/browser/extensions/browser_extension_window_wrapper.cc (right): http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/browser_extension_window_wrapper.cc#newcode27 chrome/browser/extensions/browser_extension_window_wrapper.cc:27: DictionaryValue* result = new DictionaryValue(); There's a lot of ...
3 years, 4 months ago (2012-02-22 21:46:25 UTC) #2
sky
One more comment. ExtensionWindowWrapper seems a bit misnamed to me. Maybe ExtensionWindowController? ExtensionWindowHost?
3 years, 4 months ago (2012-02-22 21:48:03 UTC) #3
stevenjb
On 2012/02/22 21:48:03, sky wrote: > One more comment. ExtensionWindowWrapper seems a bit misnamed to ...
3 years, 4 months ago (2012-02-23 01:09:19 UTC) #4
sky
ExtensionWindowController SGTM On Wed, Feb 22, 2012 at 5:09 PM, <stevenjb@chromium.org> wrote: > On 2012/02/22 ...
3 years, 4 months ago (2012-02-23 01:30:21 UTC) #5
stevenjb
http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/browser_extension_window_wrapper.cc File chrome/browser/extensions/browser_extension_window_wrapper.cc (right): http://codereview.chromium.org/9428018/diff/1/chrome/browser/extensions/browser_extension_window_wrapper.cc#newcode27 chrome/browser/extensions/browser_extension_window_wrapper.cc:27: DictionaryValue* result = new DictionaryValue(); On 2012/02/22 21:46:25, sky ...
3 years, 4 months ago (2012-02-23 02:19:15 UTC) #6
stevenjb
Note: patch set #3 includes changes from http://codereview.chromium.org/9372126/ so that tests will run. Will rebase ...
3 years, 4 months ago (2012-02-23 02:20:28 UTC) #7
Mihai Parparita -not on Chrome
Overall design seems fine to me too. http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/browser_extension_window_controller.cc File chrome/browser/extensions/browser_extension_window_controller.cc (right): http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/browser_extension_window_controller.cc#newcode27 chrome/browser/extensions/browser_extension_window_controller.cc:27: BrowserExtensionWindowController::CreateWindowValue() const ...
3 years, 4 months ago (2012-02-23 02:35:36 UTC) #8
sky
Ben needed to do a bit of research on this one, so I'm holding off ...
3 years, 4 months ago (2012-02-23 17:13:47 UTC) #9
stevenjb
http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/browser_extension_window_controller.cc File chrome/browser/extensions/browser_extension_window_controller.cc (right): http://codereview.chromium.org/9428018/diff/8001/chrome/browser/extensions/browser_extension_window_controller.cc#newcode27 chrome/browser/extensions/browser_extension_window_controller.cc:27: BrowserExtensionWindowController::CreateWindowValue() const { On 2012/02/23 02:35:36, Mihai Parparita wrote: ...
3 years, 4 months ago (2012-02-23 19:57:51 UTC) #10
Ben Goodger (Google)
For the record, I talked to Steven and this seems like a worthwhile improvement. -Ben ...
3 years, 4 months ago (2012-02-24 00:15:03 UTC) #11
sky
http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/browser_extension_window_controller.cc File chrome/browser/extensions/browser_extension_window_controller.cc (right): http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/browser_extension_window_controller.cc#newcode70 chrome/browser/extensions/browser_extension_window_controller.cc:70: bool is_fullscreen, const GURL& extension_url) const { nit: each ...
3 years, 4 months ago (2012-02-24 04:38:23 UTC) #12
jennb
Nice. Wish we had BaseWindow for non-Aura Panels. Would have made for much easier unit ...
3 years, 4 months ago (2012-02-24 05:18:43 UTC) #13
stevenjb
http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/browser_extension_window_controller.cc File chrome/browser/extensions/browser_extension_window_controller.cc (right): http://codereview.chromium.org/9428018/diff/12003/chrome/browser/extensions/browser_extension_window_controller.cc#newcode70 chrome/browser/extensions/browser_extension_window_controller.cc:70: bool is_fullscreen, const GURL& extension_url) const { On 2012/02/24 ...
3 years, 4 months ago (2012-02-24 19:25:06 UTC) #14
sky
LGTM
3 years, 4 months ago (2012-02-24 20:21:00 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevenjb@chromium.org/9428018/22011
3 years, 4 months ago (2012-02-28 01:43:12 UTC) #16
commit-bot: I haz the power
Try job failure for 9428018-22011 (retry) (retry) on linux_rel for step "compile" (clobber build). It's ...
3 years, 4 months ago (2012-02-28 02:55:38 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevenjb@chromium.org/9428018/22011
3 years, 4 months ago (2012-02-28 16:34:20 UTC) #18
commit-bot: I haz the power
Try job failure for 9428018-22011 (retry) (retry) on linux_rel for step "check_deps". It's a second ...
3 years, 4 months ago (2012-02-28 17:50:17 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevenjb@chromium.org/9428018/36001
3 years, 4 months ago (2012-02-28 18:11:28 UTC) #20
commit-bot: I haz the power
3 years, 4 months ago (2012-02-28 23:46:08 UTC) #21
Change committed as 124054
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1f9106d