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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 486903002: Moving app_view to extensions. (Closed)

Created:
6 years, 4 months ago by lfg
Modified:
6 years, 4 months ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, wjmaclean, Xi Han
Project:
chromium
Visibility:
Public.

Description

Moving app_view to extensions. Bug=352290 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291138

Patch Set 1 #

Total comments: 8

Patch Set 2 : #

Total comments: 3

Patch Set 3 : Fix formatting #

Patch Set 4 : Rebasing. #

Patch Set 5 : Fixing flags when getting extension. #

Total comments: 5

Patch Set 6 : #

Total comments: 7

Patch Set 7 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+154 lines, -439 lines) Patch
M chrome/browser/about_flags.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/apps/app_view_browsertest.cc View 3 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 2 chunks +1 line, -1 line 0 comments Download
M chrome/browser/extensions/api/chrome_extensions_api_client.h View 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/extensions/api/chrome_extensions_api_client.cc View 1 2 3 4 5 6 2 chunks +7 lines, -1 line 0 comments Download
D chrome/browser/guest_view/app_view/app_view_constants.h View 1 chunk +0 lines, -25 lines 0 comments Download
D chrome/browser/guest_view/app_view/app_view_constants.cc View 1 chunk +0 lines, -20 lines 0 comments Download
D chrome/browser/guest_view/app_view/app_view_guest.h View 1 chunk +0 lines, -86 lines 0 comments Download
D chrome/browser/guest_view/app_view/app_view_guest.cc View 1 chunk +0 lines, -259 lines 0 comments Download
A chrome/browser/guest_view/app_view/chrome_app_view_guest_delegate.h View 1 2 3 4 5 1 chunk +28 lines, -0 lines 0 comments Download
A chrome/browser/guest_view/app_view/chrome_app_view_guest_delegate.cc View 1 2 3 4 5 6 1 chunk +30 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 1 chunk +2 lines, -4 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 1 chunk +0 lines, -1 line 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc View 2 chunks +3 lines, -1 line 0 comments Download
M extensions/browser/api/extensions_api_client.h View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
M extensions/browser/api/extensions_api_client.cc View 1 2 3 4 5 6 2 chunks +6 lines, -2 lines 0 comments Download
A + extensions/browser/guest_view/app_view/app_view_constants.h View 2 chunks +3 lines, -3 lines 0 comments Download
A + extensions/browser/guest_view/app_view/app_view_constants.cc View 1 chunk +1 line, -1 line 0 comments Download
A + extensions/browser/guest_view/app_view/app_view_guest.h View 1 4 chunks +6 lines, -5 lines 0 comments Download
A + extensions/browser/guest_view/app_view/app_view_guest.cc View 1 2 3 4 5 6 5 chunks +18 lines, -20 lines 0 comments Download
A extensions/browser/guest_view/app_view/app_view_guest_delegate.h View 1 2 3 4 5 6 1 chunk +28 lines, -0 lines 0 comments Download
A + extensions/browser/guest_view/app_view/app_view_guest_delegate.cc View 1 2 3 4 5 1 chunk +3 lines, -4 lines 0 comments Download
M extensions/common/switches.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M extensions/common/switches.cc View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M extensions/extensions.gyp View 1 2 3 4 5 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 20 (0 generated)
lfg
6 years, 4 months ago (2014-08-18 20:48:56 UTC) #1
lfg
6 years, 4 months ago (2014-08-18 20:53:14 UTC) #2
Xi Han
This looks great! Please specify the bug in Bug=352290. https://codereview.chromium.org/486903002/diff/1/chrome/browser/guest_view/app_view/chrome_app_view_guest_delegate.cc File chrome/browser/guest_view/app_view/chrome_app_view_guest_delegate.cc (right): https://codereview.chromium.org/486903002/diff/1/chrome/browser/guest_view/app_view/chrome_app_view_guest_delegate.cc#newcode19 chrome/browser/guest_view/app_view/chrome_app_view_guest_delegate.cc:19: ...
6 years, 4 months ago (2014-08-18 21:29:11 UTC) #3
lfg
On 2014/08/18 21:29:11, hanxi wrote: > This looks great! Please specify the bug in Bug=352290. ...
6 years, 4 months ago (2014-08-18 21:52:36 UTC) #4
Xi Han
https://codereview.chromium.org/486903002/diff/20001/extensions/browser/guest_view/app_view/app_view_guest.cc File extensions/browser/guest_view/app_view/app_view_guest.cc (right): https://codereview.chromium.org/486903002/diff/20001/extensions/browser/guest_view/app_view/app_view_guest.cc#newcode105 extensions/browser/guest_view/app_view/app_view_guest.cc:105: ExtensionsAPIClient::Get()->CreateAppViewGuestDelegate()), I think app_view_guest_delegate_.reset(...) is needed when a new ...
6 years, 4 months ago (2014-08-19 13:29:31 UTC) #5
lfg
On 2014/08/19 13:29:31, hanxi wrote: > https://codereview.chromium.org/486903002/diff/20001/extensions/browser/guest_view/app_view/app_view_guest.cc > File extensions/browser/guest_view/app_view/app_view_guest.cc (right): > > https://codereview.chromium.org/486903002/diff/20001/extensions/browser/guest_view/app_view/app_view_guest.cc#newcode105 > ...
6 years, 4 months ago (2014-08-19 14:08:22 UTC) #6
Xi Han
lgtm
6 years, 4 months ago (2014-08-19 15:44:37 UTC) #7
lfg
kalman@chromium.org: Please review changes in chrome/browser/apps/app_view_browsertest.cc chrome/browser/extensions/api/chrome_extensions_api_client.h chrome/browser/extensions/api/chrome_extensions_api_client.cc chrome/chrome_browser.gypi chrome/common/chrome_switches.h chrome/common/chrome_switches.cc chrome/renderer/extensions/chrome_extensions_dispatcher_delegate.cc extensions/browser/api/extensions_api_client.h extensions/browser/api/extensions_api_client.cc extensions/common/switches.h ...
6 years, 4 months ago (2014-08-19 18:27:59 UTC) #8
not at google - send to devlin
I made a couple of comments, but I'd rather yoz look at this. https://codereview.chromium.org/486903002/diff/80001/extensions/browser/guest_view/app_view/app_view_guest.cc File ...
6 years, 4 months ago (2014-08-19 20:19:02 UTC) #9
Yoyo Zhou
https://chromiumcodereview.appspot.com/486903002/diff/80001/extensions/browser/api/extensions_api_client.h File extensions/browser/api/extensions_api_client.h (right): https://chromiumcodereview.appspot.com/486903002/diff/80001/extensions/browser/api/extensions_api_client.h#newcode73 extensions/browser/api/extensions_api_client.h:73: virtual AppViewGuestDelegate* CreateAppViewGuestDelegate() const; This could be simpler if ...
6 years, 4 months ago (2014-08-19 21:55:08 UTC) #10
lfg
On 2014/08/19 20:19:02, kalman wrote: > I made a couple of comments, but I'd rather ...
6 years, 4 months ago (2014-08-20 14:24:32 UTC) #11
lfg
On 2014/08/19 21:55:08, Yoyo Zhou wrote: > https://chromiumcodereview.appspot.com/486903002/diff/80001/extensions/browser/api/extensions_api_client.h > File extensions/browser/api/extensions_api_client.h (right): > > https://chromiumcodereview.appspot.com/486903002/diff/80001/extensions/browser/api/extensions_api_client.h#newcode73 ...
6 years, 4 months ago (2014-08-20 14:25:46 UTC) #12
Fady Samuel
https://codereview.chromium.org/486903002/diff/100001/chrome/browser/extensions/api/chrome_extensions_api_client.cc File chrome/browser/extensions/api/chrome_extensions_api_client.cc (right): https://codereview.chromium.org/486903002/diff/100001/chrome/browser/extensions/api/chrome_extensions_api_client.cc#newcode66 chrome/browser/extensions/api/chrome_extensions_api_client.cc:66: return new ChromeAppViewGuestDelegate; new ChromeAppViewGuestDelegate() tends to be the ...
6 years, 4 months ago (2014-08-20 14:33:26 UTC) #13
lfg
On 2014/08/20 14:33:26, Fady Samuel wrote: > https://codereview.chromium.org/486903002/diff/100001/chrome/browser/extensions/api/chrome_extensions_api_client.cc > File chrome/browser/extensions/api/chrome_extensions_api_client.cc (right): > > https://codereview.chromium.org/486903002/diff/100001/chrome/browser/extensions/api/chrome_extensions_api_client.cc#newcode66 ...
6 years, 4 months ago (2014-08-20 15:01:57 UTC) #14
Fady Samuel
lgtm
6 years, 4 months ago (2014-08-20 15:12:55 UTC) #15
Yoyo Zhou
LGTM
6 years, 4 months ago (2014-08-20 20:24:21 UTC) #16
not at google - send to devlin
lgtm, but TODO(yoz): be an apps owner.
6 years, 4 months ago (2014-08-21 17:04:06 UTC) #17
lfg
The CQ bit was checked by lfg@chromium.org
6 years, 4 months ago (2014-08-21 17:23:35 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/lfg@chromium.org/486903002/120001
6 years, 4 months ago (2014-08-21 17:27:10 UTC) #19
commit-bot: I haz the power
6 years, 4 months ago (2014-08-21 18:31:32 UTC) #20
Message was sent while issue was closed. 
Committed patchset #7 (120001) as 291138

Powered by Google App Engine
This is Rietveld 408576698