Create ChromeWebView Framework.

Create ChromeWebView Framework.

Converts //ios/web_view target to build ChromeWebView.framework, which
includes the necessary Chrome resources.

BUG=688247
Review-Url: https://codereview.chromium.org/2745653010
Cr-Commit-Position: refs/heads/master@{#456735}
Committed: https://chromium.googlesource.com/chromium/src/+/09165f0b1ceba1bb69afa0e786e323839acefe3b

[michaeldo, 2017-03-11 00:57:26]

michaeldo@chromium.org changed reviewers:
+ eugenebut@chromium.org, sdefresne@chromium.org

[Eugene But (OOO till 7-30), 2017-03-11 01:15:01]

I defer GN review to sdefresne@ :)

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/internal/cwv.mm
File ios/web_view/internal/cwv.mm (right):

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/internal/cwv.m...
ios/web_view/internal/cwv.mm:11: #import "base/mac/bundle_locations.h"
s/import/include

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/internal/cwv.m...
ios/web_view/internal/cwv.mm:45: base::mac::SetOverrideFrameworkBundle([NSBundle
bundleForClass:[self class]]);
We should get rid of CWV class. Is there a better place for this call?

[sdefresne, 2017-03-13 10:32:24]

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/BUILD.gn
File ios/web_view/BUILD.gn (right):

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/BUILD.gn#newco...
ios/web_view/BUILD.gn:67: configs -= [
"//build/config/gcc:symbol_visibility_hidden" ]
Why do you need to use configs to change the visibility? Looking at cwv.h, I see
that CWV interface is marked with the correct attribute to have public
visibility __attribute__((visibility("default"))), so it should not be
necessary. Does it work if you remove the
//build/config/gcc:symbol_visibility_default?

If it does not work, what is the error.

Note (no action required): since sources & public_headers only list .h files,
the configs is not necessary //build/config/compiler:enable_arc (since it is
only used when building source files, and there are no source file here).
Probably best to leave it here though so that if some source file is added in
the future it will be compiled with ARC.

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/BUILD.gn#newco...
ios/web_view/BUILD.gn:77: "//components/resources:components_resources",
Don't you also want to include //ios/web:resources too?

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/Info.plist
File ios/web_view/Info.plist (right):

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/Info.plist#new...
ios/web_view/Info.plist:14:
<string>${IOS_BUNDLE_ID_PREFIX}.test.${EXECUTABLE_NAME:rfc1034identifier}</string>
Remove ".test"

[michaeldo, 2017-03-13 17:27:01]

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/BUILD.gn
File ios/web_view/BUILD.gn (right):

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/BUILD.gn#newco...
ios/web_view/BUILD.gn:67: configs -= [
"//build/config/gcc:symbol_visibility_hidden" ]
On 2017/03/13 10:32:24, sdefresne wrote:
> Why do you need to use configs to change the visibility? Looking at cwv.h, I
see
> that CWV interface is marked with the correct attribute to have public
> visibility __attribute__((visibility("default"))), so it should not be
> necessary. Does it work if you remove the
> //build/config/gcc:symbol_visibility_default?
> 
> If it does not work, what is the error.
> 
> Note (no action required): since sources & public_headers only list .h files,
> the configs is not necessary //build/config/compiler:enable_arc (since it is
> only used when building source files, and there are no source file here).
> Probably best to leave it here though so that if some source file is added in
> the future it will be compiled with ARC.

It does work and I can remove this. I copied this logic from another framework
which is where this config option originated. Thanks for the info about ARC.

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/BUILD.gn#newco...
ios/web_view/BUILD.gn:77: "//components/resources:components_resources",
On 2017/03/13 10:32:23, sdefresne wrote:
> Don't you also want to include //ios/web:resources too?

yes, you are right, I added it. Thank you for likely saving lots of debugging
later!

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/Info.plist
File ios/web_view/Info.plist (right):

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/Info.plist#new...
ios/web_view/Info.plist:14:
<string>${IOS_BUNDLE_ID_PREFIX}.test.${EXECUTABLE_NAME:rfc1034identifier}</string>
On 2017/03/13 10:32:24, sdefresne wrote:
> Remove ".test"

Done.

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/internal/cwv.mm
File ios/web_view/internal/cwv.mm (right):

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/internal/cwv.m...
ios/web_view/internal/cwv.mm:11: #import "base/mac/bundle_locations.h"
On 2017/03/11 01:15:01, Eugene But wrote:
> s/import/include

Please confirm, bundle_locations imports Foundation for NSBundle object

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/internal/cwv.m...
ios/web_view/internal/cwv.mm:45: base::mac::SetOverrideFrameworkBundle([NSBundle
bundleForClass:[self class]]);
On 2017/03/11 01:15:01, Eugene But wrote:
> We should get rid of CWV class. Is there a better place for this call?

I agree this class will go away soon, but I think it is best here for now. I
believe it will need to move to a dispatch once inside CWVWebViewConfiguration
or CWVWebView (and called from the other entry points) but there will be less
entry points to worry about once the configuration and DataStore objects are
merged so I will move this when CWV is removed if that sounds OK.

[Eugene But (OOO till 7-30), 2017-03-13 20:37:12]

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/internal/cwv.mm
File ios/web_view/internal/cwv.mm (right):

https://codereview.chromium.org/2745653010/diff/1/ios/web_view/internal/cwv.m...
ios/web_view/internal/cwv.mm:45: base::mac::SetOverrideFrameworkBundle([NSBundle
bundleForClass:[self class]]);
On 2017/03/13 17:27:01, michaeldo wrote:
> On 2017/03/11 01:15:01, Eugene But wrote:
> > We should get rid of CWV class. Is there a better place for this call?
> 
> I agree this class will go away soon, but I think it is best here for now. I
> believe it will need to move to a dispatch once inside CWVWebViewConfiguration
> or CWVWebView (and called from the other entry points) but there will be less
> entry points to worry about once the configuration and DataStore objects are
> merged so I will move this when CWV is removed if that sounds OK.
Sure.

[Eugene But (OOO till 7-30), 2017-03-13 21:11:49]

lgtm as long as Sylvain is happy.

[sdefresne, 2017-03-14 09:16:34]

lgtm with a question/suggestion

https://codereview.chromium.org/2745653010/diff/20001/ios/web_view/BUILD.gn
File ios/web_view/BUILD.gn (right):

https://codereview.chromium.org/2745653010/diff/20001/ios/web_view/BUILD.gn#n...
ios/web_view/BUILD.gn:11: "//ios/web_view/internal",
nit: you don't need absolute path here, you can just use relative paths (this
makes moving code around easier), so this can be

config("config") {
  include_dirs = [
    "internal",
    "public",
  ]
}

https://codereview.chromium.org/2745653010/diff/20001/ios/web_view/internal/c...
File ios/web_view/internal/cwv.mm (right):

https://codereview.chromium.org/2745653010/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/cwv.mm:45: base::mac::SetOverrideFrameworkBundle([NSBundle
bundleForClass:[self class]]);
Why not putting this in WebViewWebMainParts::PreMainMessageLoopRun, just before
loading the web_view_resources.pak?

[michaeldo, 2017-03-14 15:34:23]

https://codereview.chromium.org/2745653010/diff/20001/ios/web_view/BUILD.gn
File ios/web_view/BUILD.gn (right):

https://codereview.chromium.org/2745653010/diff/20001/ios/web_view/BUILD.gn#n...
ios/web_view/BUILD.gn:11: "//ios/web_view/internal",
On 2017/03/14 09:16:34, sdefresne wrote:
> nit: you don't need absolute path here, you can just use relative paths (this
> makes moving code around easier), so this can be
> 
> config("config") {
>   include_dirs = [
>     "internal",
>     "public",
>   ]
> }

Thanks for the tip. Done.

https://codereview.chromium.org/2745653010/diff/20001/ios/web_view/internal/c...
File ios/web_view/internal/cwv.mm (right):

https://codereview.chromium.org/2745653010/diff/20001/ios/web_view/internal/c...
ios/web_view/internal/cwv.mm:45: base::mac::SetOverrideFrameworkBundle([NSBundle
bundleForClass:[self class]]);
On 2017/03/14 09:16:34, sdefresne wrote:
> Why not putting this in WebViewWebMainParts::PreMainMessageLoopRun, just
before
> loading the web_view_resources.pak?

Excellent idea, I didn't know where the best place for this was, but it makes a
lot of sense there, so I've moved it.

[michaeldo, 2017-03-14 15:35:50]

The CQ bit was checked by michaeldo@chromium.org

[michaeldo, 2017-03-14 15:35:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, sdefresne@chromium.org
Link to the patchset: https://codereview.chromium.org/2745653010/#ps40001
(title: "Respond to comments.")

[commit-bot: I haz the power, 2017-03-14 15:36:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 16:54:36]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489505750195360,
"parent_rev": "52e9989e699a33119481a562117408bff3eef9b4", "commit_rev":
"09165f0b1ceba1bb69afa0e786e323839acefe3b"}

[commit-bot: I haz the power, 2017-03-14 16:55:15]

Description was changed from

==========
Create ChromeWebView Framework.

Converts //ios/web_view target to build ChromeWebView.framework, which
includes the necessary Chrome resources.

BUG=688247
==========

to

==========
Create ChromeWebView Framework.

Converts //ios/web_view target to build ChromeWebView.framework, which
includes the necessary Chrome resources.

BUG=688247

Review-Url: https://codereview.chromium.org/2745653010
Cr-Commit-Position: refs/heads/master@{#456735}
Committed:
https://chromium.googlesource.com/chromium/src/+/09165f0b1ceba1bb69afa0e786e3...
==========

[commit-bot: I haz the power, 2017-03-14 16:55:16]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/09165f0b1ceba1bb69afa0e786e3...