Move core web_view code to extensions.

Move core web_view code to extensions.


BUG=352290
Committed: https://chromium.googlesource.com/chromium/src/+/1218a23163baab9f53d70e9a63030db13ef04d04

Committed: https://crrev.com/0d0a1e97194c6e9ea93c0808b0d1e5ac567ba6b0
Cr-Commit-Position: refs/heads/master@{#291947}

[Fady Samuel, 2014-08-22 16:40:17]

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/DEPS (right):

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/DEPS:3:
"+chrome/browser/extensions/api/web_view/web_view_internal_api.h",
Are we OK with this? I worry this will make app_shell significantly larger.

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/web_view/web_view_guest_delegate.h (right):

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_guest_delegate.h:30:
WebViewGuestDelegate(WebViewGuest* web_view_guest);
explicit

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/web_view/web_view_permission_helper.h
(right):

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_permission_helper.h:149:
scoped_ptr<extensions::WebViewPermissionHelperDelegate>
Drop extensions::

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
File
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.cc
(right):

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.cc:12:
extensions::WebViewPermissionHelper* web_view_permission_helper)
Drop extensions::

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
File
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h
(right):

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h:20:
extensions::WebViewPermissionHelper* web_view_permission_helper);
Given we are already in the extensions namespace, I don't think we need
extensions::

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h:88:
extensions:: WebViewPermissionHelper* web_view_permission_helper() {
web_view_permission_helper() const

Also, I don't think we need extensions::

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h:93:
extensions::WebViewPermissionHelper* web_view_permission_helper_;
This indicates that the WebViewPermissionHelper cannot change.
WebViewPermissionHelper* const web_view_permission_helper_;

[Xi Han, 2014-08-22 18:04:53]

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/DEPS (right):

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/DEPS:3:
"+chrome/browser/extensions/api/web_view/web_view_internal_api.h",
On 2014/08/22 16:40:17, Fady Samuel wrote:
> Are we OK with this? I worry this will make app_shell significantly larger.

Eventually we will move web_view_internal_api.h to extensions, so app_shell will
build with this part. I add a TODO to remind me to remove these entries in the
DEPS files.

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/web_view/web_view_guest_delegate.h (right):

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_guest_delegate.h:30:
WebViewGuestDelegate(WebViewGuest* web_view_guest);
On 2014/08/22 16:40:17, Fady Samuel wrote:
> explicit

Done.

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
File extensions/browser/guest_view/web_view/web_view_permission_helper.h
(right):

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_permission_helper.h:149:
scoped_ptr<extensions::WebViewPermissionHelperDelegate>
On 2014/08/22 16:40:17, Fady Samuel wrote:
> Drop extensions::

Done.

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
File
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.cc
(right):

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.cc:12:
extensions::WebViewPermissionHelper* web_view_permission_helper)
On 2014/08/22 16:40:17, Fady Samuel wrote:
> Drop extensions::

Done.

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
File
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h
(right):

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h:20:
extensions::WebViewPermissionHelper* web_view_permission_helper);
On 2014/08/22 16:40:17, Fady Samuel wrote:
> Given we are already in the extensions namespace, I don't think we need
> extensions::


Good catch, thanks. I just forget to remove these extensions::.

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h:88:
extensions:: WebViewPermissionHelper* web_view_permission_helper() {
On 2014/08/22 16:40:17, Fady Samuel wrote:
> web_view_permission_helper() const
> 
> Also, I don't think we need extensions::

Done.

https://codereview.chromium.org/496873006/diff/1/extensions/browser/guest_vie...
extensions/browser/guest_view/web_view/web_view_permission_helper_delegate.h:93:
extensions::WebViewPermissionHelper* web_view_permission_helper_;
On 2014/08/22 16:40:17, Fady Samuel wrote:
> This indicates that the WebViewPermissionHelper cannot change.
> WebViewPermissionHelper* const web_view_permission_helper_;

Done.

[Fady Samuel, 2014-08-22 18:13:55]

lgtm with nits

https://codereview.chromium.org/496873006/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_guest_delegate.h (right):

https://codereview.chromium.org/496873006/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest_delegate.h:71:
WebViewGuest* web_view_guest_;
WebViewGuest* const web_view_guest_;

https://codereview.chromium.org/496873006/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_permission_helper.h
(right):

https://codereview.chromium.org/496873006/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_permission_helper.h:152:
WebViewGuest* web_view_guest_;
WebViewGuest* const web_view_guest_;

[Xi Han, 2014-08-22 18:25:43]

+kalman@chromium.org: Please review changes in:
-chrome/browser/extensions/api/chrome_extensions_api_client.h
-chrome/browser/extensions/api/chrome_extensions_api_client.cc
-chrome/browser/extensions/api/declarative/declarative_api.cc
-chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
-chrome/browser/extensions/api/web_request/web_request_api.cc
-chrome/browser/extensions/api/web_request/web_request_permissions.cc
-chrome/browser/extensions/menu_manager.cc
-chrome/browser/extensions/url_request_util.cc
-extensions/browser/api/extensions_api_client.h
-extensions/browser/api/extensions_api_client.cc
-extensions/extensions.gyp
Thanks!

https://codereview.chromium.org/496873006/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_guest_delegate.h (right):

https://codereview.chromium.org/496873006/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest_delegate.h:71:
WebViewGuest* web_view_guest_;
On 2014/08/22 18:13:55, Fady Samuel wrote:
> WebViewGuest* const web_view_guest_;

Done.

https://codereview.chromium.org/496873006/diff/20001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_permission_helper.h
(right):

https://codereview.chromium.org/496873006/diff/20001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_permission_helper.h:152:
WebViewGuest* web_view_guest_;
On 2014/08/22 18:13:55, Fady Samuel wrote:
> WebViewGuest* const web_view_guest_;

Done.

[not at google - send to devlin, 2014-08-22 20:21:13]

Yoyo is a better reviewer than me. I'm not sure about the delegate thing.

[Xi Han, 2014-08-22 21:25:51]

Fady: PTAL;

+yoz@chromium.org: Please review changes in:
-chrome/browser/extensions/api/chrome_extensions_api_client.h
-chrome/browser/extensions/api/chrome_extensions_api_client.cc
-chrome/browser/extensions/api/declarative/declarative_api.cc
-chrome/browser/extensions/api/declarative_webrequest/webrequest_action.cc
-chrome/browser/extensions/api/web_request/web_request_api.cc
-chrome/browser/extensions/api/web_request/web_request_permissions.cc
-chrome/browser/extensions/menu_manager.cc
-chrome/browser/extensions/url_request_util.cc
-extensions/browser/api/extensions_api_client.h
-extensions/browser/api/extensions_api_client.cc
-extensions/extensions.gyp
Thanks!

[Xi Han, 2014-08-22 21:27:23]

+sky@chromium.org: Please review changes in 
-chrome/browser/geolocation/geolocation_permission_context_extensions.cc
-chrome/browser/plugins/plugin_info_message_filter.cc
-chrome/browser/renderer_context_menu/context_menu_content_type_factory.cc
-chrome/browser/renderer_context_menu/render_view_context_menu.cc
-chrome/browser/renderer_host/chrome_render_message_filter.cc
-chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc
-chrome/browser/ui/cocoa/constrained_window/constrained_window_mac.mm
-chrome/browser/ui/views/constrained_window_views.cc

Thanks!

[Fady Samuel, 2014-08-22 21:35:47]

lgtm

[sky, 2014-08-22 22:34:39]

LGTM

[Yoyo Zhou, 2014-08-23 01:22:09]

LGTM

https://chromiumcodereview.appspot.com/496873006/diff/80001/extensions/browse...
File extensions/browser/api/extensions_api_client.h (right):

https://chromiumcodereview.appspot.com/496873006/diff/80001/extensions/browse...
extensions/browser/api/extensions_api_client.h:83: // return
WebViewGuestDelegate.
typo: returned

https://chromiumcodereview.appspot.com/496873006/diff/80001/extensions/browse...
extensions/browser/api/extensions_api_client.h:84: virtual WebViewGuestDelegate*
CreateWebViewGuestDelegate(
Functions should be const, like CreateAppViewGuestDelegate above.

https://chromiumcodereview.appspot.com/496873006/diff/80001/extensions/browse...
File extensions/browser/guest_view/DEPS (right):

https://chromiumcodereview.appspot.com/496873006/diff/80001/extensions/browse...
extensions/browser/guest_view/DEPS:2: # TODO(hanxi): Remove these.
It would help to have a bug number to track this.

[Fady Samuel, 2014-08-23 11:00:02]

Still lgtm with one nit.

https://codereview.chromium.org/496873006/diff/80001/extensions/browser/guest...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/496873006/diff/80001/extensions/browser/guest...
extensions/browser/guest_view/web_view/web_view_guest.cc:315: if
(web_view_guest_delegate_) {
Remove block since it's a single line statement.

[Xi Han, 2014-08-25 13:21:09]

Dominic: Please review changes in:
-extensions/browser/guest_view/DEPS

Thanks!

[Xi Han, 2014-08-25 13:29:26]

jamesr@chromium.org: Please review changes in:
extensions/browser/guest_view/DEPS

Thanks!

[Fady Samuel, 2014-08-25 14:22:52]

https://codereview.chromium.org/496873006/diff/100001/extensions/browser/gues...
File extensions/browser/guest_view/DEPS (right):

https://codereview.chromium.org/496873006/diff/100001/extensions/browser/gues...
extensions/browser/guest_view/DEPS:4:
"+third_party/WebKit/public/web/WebFindOptions.h",
Xi, is it possible for us to find ways to avoid these dependencies all together?

[Xi Han, 2014-08-25 17:20:20]

https://codereview.chromium.org/496873006/diff/100001/extensions/browser/gues...
File extensions/browser/guest_view/DEPS (right):

https://codereview.chromium.org/496873006/diff/100001/extensions/browser/gues...
extensions/browser/guest_view/DEPS:4:
"+third_party/WebKit/public/web/WebFindOptions.h",
On 2014/08/25 14:22:52, Fady Samuel wrote:
> Xi, is it possible for us to find ways to avoid these dependencies all
together?

The WebViewFindHelper still needs to include WebFindOptions.h;
WebViewGuest.cc needs to include web_view_internal_api.h.

Currently we still needs them.

[jamesr, 2014-08-25 17:22:08]

https://codereview.chromium.org/496873006/diff/120001/extensions/browser/gues...
File extensions/browser/guest_view/DEPS (right):

https://codereview.chromium.org/496873006/diff/120001/extensions/browser/gues...
extensions/browser/guest_view/DEPS:4:
"+third_party/WebKit/public/web/WebFindOptions.h",
this line lgtm. The +chrome/ looks bad, though

[Xi Han, 2014-08-25 17:26:15]

https://codereview.chromium.org/496873006/diff/120001/extensions/browser/gues...
File extensions/browser/guest_view/DEPS (right):

https://codereview.chromium.org/496873006/diff/120001/extensions/browser/gues...
extensions/browser/guest_view/DEPS:4:
"+third_party/WebKit/public/web/WebFindOptions.h",
On 2014/08/25 17:22:08, jamesr wrote:
> this line lgtm. The +chrome/ looks bad, though

This is just a temporary step. The include of web_view_internal_api.h will be
moved to extensions in a following CL.

[Xi Han, 2014-08-25 17:31:07]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-08-25 17:31:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hanxi@chromium.org/496873006/120001

[commit-bot: I haz the power, 2014-08-25 18:29:53]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-08-25 19:13:53]

Committed patchset #6 (120001) as 1218a23163baab9f53d70e9a63030db13ef04d04

[jamesr, 2014-08-25 22:48:55]

https://codereview.chromium.org/496873006/diff/120001/extensions/extensions.gyp
File extensions/extensions.gyp (right):

https://codereview.chromium.org/496873006/diff/120001/extensions/extensions.g...
extensions/extensions.gyp:483: 'browser/guest_view/web_view/web_view_guest.cc',
this file #includes headers from chrome/browser/.  those headers in turn
#include files from chrome/common/ that depend on generated headers.  this
target needs to declare its dependency on chrome/browser/ and chrome/browser
needs to export_dependent_settings from common in order to forward the hard
dependency on the target that generates chrome/common/extensions/api/types.h

it doesn't really seem right for code in src/extensions/ and src/chrome/ to both
be #including each other, though.  one or the other is wrong...

[James Cook, 2014-08-25 23:26:35]

On 2014/08/25 22:48:55, jamesr wrote:
>
https://codereview.chromium.org/496873006/diff/120001/extensions/extensions.gyp
> File extensions/extensions.gyp (right):
> 
>
https://codereview.chromium.org/496873006/diff/120001/extensions/extensions.g...
> extensions/extensions.gyp:483:
'browser/guest_view/web_view/web_view_guest.cc',
> this file #includes headers from chrome/browser/.  those headers in turn
> #include files from chrome/common/ that depend on generated headers.  this
> target needs to declare its dependency on chrome/browser/ and chrome/browser
> needs to export_dependent_settings from common in order to forward the hard
> dependency on the target that generates chrome/common/extensions/api/types.h
> 
> it doesn't really seem right for code in src/extensions/ and src/chrome/ to
both
> be #including each other, though.  one or the other is wrong...

Code in src/extensions cannot use code in src/chrome. The dependency chain is
chrome -> extensions -> content.

We can't compile app_shell now, probably due to this change.  Lemme see who is
online -- we might need to revert.

[James Cook, 2014-08-25 23:31:11]

A revert of this CL (patchset #6) has been created in
https://codereview.chromium.org/505893002/ by jamescook@chromium.org.

The reason for reverting is: This breaks the app_shell build for the reasons
jamesr describes (src/extensions now depends on generated headers from
src/chrome, but there cannot be a dependency from extensions -> chrome).

Hypothetical repro on Linux:
* rm -rf out/* && gclient runhooks && ninja -C out/Debug -j 1000 app_shell

chrome/browser/extensions/api/capture_web_contents_function.h:9:10: fatal error:
'chrome/common/extensions/api/types.h' file not found
.

[brettw, 2014-08-26 00:44:20]

brettw@chromium.org changed reviewers:
+ brettw@chromium.org

[brettw, 2014-08-26 00:44:20]

Also, when you re-land, you will need to update the GN build for the changes
you're making in extensions.gyp

[Xi Han, 2014-08-26 15:49:20]

Patchset #7 (id:140001) has been deleted

[Xi Han, 2014-08-26 15:52:09]

I move all of the dependency of find function in WebViewGuest to
ChromeWebViewGuestDelegate. The DEPS file is remvoed as well. Please take
another look. Thanks!

[Fady Samuel, 2014-08-26 16:46:39]

Still lgtm

[Xi Han, 2014-08-26 17:07:14]

The CQ bit was checked by hanxi@chromium.org

[commit-bot: I haz the power, 2014-08-26 17:08:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hanxi@chromium.org/496873006/160001

[commit-bot: I haz the power, 2014-08-26 18:12:52]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[commit-bot: I haz the power, 2014-08-26 18:40:00]

Committed patchset #7 (160001) as a7bbaa6984eb4b9e0189fe65a07bf1c144a6ba1b

[brettw, 2014-09-02 17:20:24]

I asked you to update the GN build when you re-landed and you did not. Please be
more careful in the future.

[Fady Samuel, 2014-09-02 17:22:09]

On 2014/09/02 17:20:24, brettw wrote:
> I asked you to update the GN build when you re-landed and you did not. Please
be
> more careful in the future.

For future reference, could you please share with us how to test the GN build?

[chromium-reviews, 2014-09-02 17:34:58]

All the dependencies were removed in my last update, so DEPS file was
deleted and there was no need to update the GN build. I am not familiar
with GN file as well, don't know how to update and test them:(


On Tue, Sep 2, 2014 at 1:22 PM, <fsamuel@chromium.org> wrote:

> On 2014/09/02 17:20:24, brettw wrote:
>
>> I asked you to update the GN build when you re-landed and you did not.
>> Please
>>
> be
>
>> more careful in the future.
>>
>
> For future reference, could you please share with us how to test the GN
> build?
>
> https://codereview.chromium.org/496873006/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tfarina, 2014-09-02 17:37:03]

On Tue, Sep 2, 2014 at 2:22 PM, <fsamuel@chromium.org> wrote:

> On 2014/09/02 17:20:24, brettw wrote:
>
>> I asked you to update the GN build when you re-landed and you did not.
>> Please
>>
> be
>
>> more careful in the future.
>>
>
> For future reference, could you please share with us how to test the GN
> build?
>
> It should be simple as:

$ gn gen out_gn
$ ninja -C out_gn target_name

In doubt, we have extensible documentation written for it in our wiki -
http://code.google.com/p/chromium/wiki/gn

-- 
Thiago Farina

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-09-10 02:36:34]

Patchset 7 (id:??) landed as
https://crrev.com/63bf863b12e8c2be593da2318af8ab7c7fbcc8a4
Cr-Commit-Position: refs/heads/master@{#291732}

[commit-bot: I haz the power, 2014-09-10 02:44:18]

Patchset 7 (id:??) landed as
https://crrev.com/0d0a1e97194c6e9ea93c0808b0d1e5ac567ba6b0
Cr-Commit-Position: refs/heads/master@{#291947}