Export all CWV* symbols from the ios/web_view dynamic library.

Export all CWV* symbols from the ios/web_view dynamic library.

Only symbols explicitly marked __attribute__((visibility("default"))) are
exported from a dynamic library. So all public classes in ios/web_view
must be marked as __attribute__((visibility("default"))). Otherwise the
users of the dynamic library cannot access these classes.

Here I followed other code using __attribute__((visibility("default")))
in Chromium like this one:
https://cs.chromium.org/chromium/src/dbus/dbus_export.h
to define a macro CWV_EXPORT instead of directly using __attribute__.

This makes it possible to export symbols only when *building* the dynamic
library (by checking CWV_IMPLEMENTATION), not when *using* the dynamic
library.

BUG=704043
Review-Url: https://codereview.chromium.org/2770633002
Cr-Commit-Position: refs/heads/master@{#460017}
Committed: https://chromium.googlesource.com/chromium/src/+/a548ad0c0108bc06d5e622acfc89777bcb00ee23

[Hiroshi Ichikawa, 2017-03-22 09:31:07]

Description was changed from

==========
Export all CWV* symbols from the ios/web_view dynamic library.

BUG=
==========

to

==========
Export all CWV* symbols from the ios/web_view dynamic library.

BUG=704043
==========

[Hiroshi Ichikawa, 2017-03-22 09:44:23]

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

[Hiroshi Ichikawa, 2017-03-22 09:44:24]

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h
File ios/web_view/public/cwv.h (right):

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h#n...
ios/web_view/public/cwv.h:11: #include "cwv_export.h"
Can you advise me if this relative include is appropriate here? I know that
absolute includes are usually used in Chromium, but absolute includes won't work
in client code of the dynamic framework.

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h#n...
ios/web_view/public/cwv.h:17: CWV_EXPORT @interface CWV : NSObject
I found that this token (or even __attribute__((visibility("default"))))
confuses git cl format and let it format the line in a weird way. Do you know of
any workaround?

[Eugene But (OOO till 7-30), 2017-03-22 15:18:55]

What does it do in practice? Would it allow to strip all unused symbols from the
dynamic framework. Could you please specify in CL description.

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h
File ios/web_view/public/cwv.h (right):

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h#n...
ios/web_view/public/cwv.h:11: #include "cwv_export.h"
On 2017/03/22 09:44:24, Hiroshi Ichikawa wrote:
> Can you advise me if this relative include is appropriate here? I know that
> absolute includes are usually used in Chromium, but absolute includes won't
work
> in client code of the dynamic framework.
This relative include may not work for clients who are going to include this
framework header. For all public headers we should probably use framework style
includes with angle braces.

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h#n...
ios/web_view/public/cwv.h:17: CWV_EXPORT @interface CWV : NSObject
On 2017/03/22 09:44:24, Hiroshi Ichikawa wrote:
> I found that this token (or even __attribute__((visibility("default"))))
> confuses git cl format and let it format the line in a weird way. Do you know
of
> any workaround?
I don't know about workarounds, but you can file a bug agains clang-format.

[michaeldo, 2017-03-22 23:16:22]

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

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/BUILD.gn#newco...
ios/web_view/BUILD.gn:28: "public/cwv_export.h",
Is this file missing, I don't see it within this CL and it isn't already in the
code base.

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

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/internal/BUILD...
ios/web_view/internal/BUILD.gn:40: defines = [ "CWV_IMPLEMENTATION" ]
Can you explain what this is for, probably should add explanation to the CL
description to for later reference in case someone has the same question.

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h
File ios/web_view/public/cwv.h (right):

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h#n...
ios/web_view/public/cwv.h:11: #include "cwv_export.h"
On 2017/03/22 15:18:54, Eugene But wrote:
> On 2017/03/22 09:44:24, Hiroshi Ichikawa wrote:
> > Can you advise me if this relative include is appropriate here? I know that
> > absolute includes are usually used in Chromium, but absolute includes won't
> work
> > in client code of the dynamic framework.
> This relative include may not work for clients who are going to include this
> framework header. For all public headers we should probably use framework
style
> includes with angle braces.

I don't think we should have this here as-is. Maybe it's possible to use a
define of some sort within ios/web_view/BUILD.gn in the
ios_framework_bundle("web_view") section and if it is building the framework,
include it like #include <ChromeWebView/cwv_export.h> otherwise, include it with
absolute path. I'm not positive this will work, but may be worth trying.

[Hiroshi Ichikawa, 2017-03-23 03:16:38]

Description was changed from

==========
Export all CWV* symbols from the ios/web_view dynamic library.

BUG=704043
==========

to

==========
Export all CWV* symbols from the ios/web_view dynamic library.

Only symbols explicitly marked __attribute__((visibility("default"))) are
exported from a dynamic library. So all public classes in ios/web_view must be
marked as __attribute__((visibility("default"))). Otherwise the users of the
dynamic library cannot access these classes.

Here I define and use a macro CWV_EXPORT, which is expanded to
__attribute__((visibility("default"))) only when *building* the dynamic library
(by checking CWV_IMPLEMENTATION), not when *using* the dynamic library.

By using this macro, it can export symbols from the dynamic library, while
avoiding unnecessary exporting the symbols from binaries which use the library.

BUG=704043
==========

[Hiroshi Ichikawa, 2017-03-23 03:17:39]

Description was changed from

==========
Export all CWV* symbols from the ios/web_view dynamic library.

Only symbols explicitly marked __attribute__((visibility("default"))) are
exported from a dynamic library. So all public classes in ios/web_view must be
marked as __attribute__((visibility("default"))). Otherwise the users of the
dynamic library cannot access these classes.

Here I define and use a macro CWV_EXPORT, which is expanded to
__attribute__((visibility("default"))) only when *building* the dynamic library
(by checking CWV_IMPLEMENTATION), not when *using* the dynamic library.

By using this macro, it can export symbols from the dynamic library, while
avoiding unnecessary exporting the symbols from binaries which use the library.

BUG=704043
==========

to

==========
Export all CWV* symbols from the ios/web_view dynamic library.

Only symbols explicitly marked __attribute__((visibility("default"))) are
exported from a dynamic library. So all public classes in ios/web_view
must be marked as __attribute__((visibility("default"))). Otherwise the
users of the dynamic library cannot access these classes.

Here I define and use a macro CWV_EXPORT, which is expanded to
__attribute__((visibility("default"))) only when *building* the dynamic
library (by checking CWV_IMPLEMENTATION), not when *using* the dynamic
library.

By using this macro, it can export symbols from the dynamic library, while
avoiding unnecessary exporting the symbols from binaries which use the
library.

BUG=704043
==========

[Hiroshi Ichikawa, 2017-03-23 04:22:39]

Description was changed from

==========
Export all CWV* symbols from the ios/web_view dynamic library.

Only symbols explicitly marked __attribute__((visibility("default"))) are
exported from a dynamic library. So all public classes in ios/web_view
must be marked as __attribute__((visibility("default"))). Otherwise the
users of the dynamic library cannot access these classes.

Here I define and use a macro CWV_EXPORT, which is expanded to
__attribute__((visibility("default"))) only when *building* the dynamic
library (by checking CWV_IMPLEMENTATION), not when *using* the dynamic
library.

By using this macro, it can export symbols from the dynamic library, while
avoiding unnecessary exporting the symbols from binaries which use the
library.

BUG=704043
==========

to

==========
Export all CWV* symbols from the ios/web_view dynamic library.

Only symbols explicitly marked __attribute__((visibility("default"))) are
exported from a dynamic library. So all public classes in ios/web_view
must be marked as __attribute__((visibility("default"))). Otherwise the
users of the dynamic library cannot access these classes.

Here I followed other code using __attribute__((visibility("default"))) in
Chromium like this one:
https://cs.chromium.org/chromium/src/dbus/dbus_export.h
to define a macro CWV_EXPORT instead of directly using __attribute__.

This makes it possible to export symbols only when *building* the dynamic
library (by checking CWV_IMPLEMENTATION), not when *using* the dynamic
library.

BUG=704043
==========

[Hiroshi Ichikawa, 2017-03-23 04:27:35]

Description was changed from

==========
Export all CWV* symbols from the ios/web_view dynamic library.

Only symbols explicitly marked __attribute__((visibility("default"))) are
exported from a dynamic library. So all public classes in ios/web_view
must be marked as __attribute__((visibility("default"))). Otherwise the
users of the dynamic library cannot access these classes.

Here I followed other code using __attribute__((visibility("default"))) in
Chromium like this one:
https://cs.chromium.org/chromium/src/dbus/dbus_export.h
to define a macro CWV_EXPORT instead of directly using __attribute__.

This makes it possible to export symbols only when *building* the dynamic
library (by checking CWV_IMPLEMENTATION), not when *using* the dynamic
library.

BUG=704043
==========

to

==========
Export all CWV* symbols from the ios/web_view dynamic library.

Only symbols explicitly marked __attribute__((visibility("default"))) are
exported from a dynamic library. So all public classes in ios/web_view
must be marked as __attribute__((visibility("default"))). Otherwise the
users of the dynamic library cannot access these classes.

Here I followed other code using __attribute__((visibility("default")))
in Chromium like this one:
https://cs.chromium.org/chromium/src/dbus/dbus_export.h
to define a macro CWV_EXPORT instead of directly using __attribute__.

This makes it possible to export symbols only when *building* the dynamic
library (by checking CWV_IMPLEMENTATION), not when *using* the dynamic
library.

BUG=704043
==========

[michaeldo, 2017-03-23 04:46:11]

Thanks for updating the description, I just looked into the WebKit headers for
reference which does in fact do something similar with the #define for the
visibility attribute.

[Hiroshi Ichikawa, 2017-03-23 05:03:03]

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

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/BUILD.gn#newco...
ios/web_view/BUILD.gn:28: "public/cwv_export.h",
On 2017/03/22 23:16:22, michaeldo wrote:
> Is this file missing, I don't see it within this CL and it isn't already in
the
> code base.

Oops sorry. Added.

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

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/internal/BUILD...
ios/web_view/internal/BUILD.gn:40: defines = [ "CWV_IMPLEMENTATION" ]
On 2017/03/22 23:16:22, michaeldo wrote:
> Can you explain what this is for, probably should add explanation to the CL
> description to for later reference in case someone has the same question.

Added a description to the CL. Also added a short description here as a comment.

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h
File ios/web_view/public/cwv.h (right):

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h#n...
ios/web_view/public/cwv.h:11: #include "cwv_export.h"
On 2017/03/22 23:16:22, michaeldo wrote:
> On 2017/03/22 15:18:54, Eugene But wrote:
> > On 2017/03/22 09:44:24, Hiroshi Ichikawa wrote:
> > > Can you advise me if this relative include is appropriate here? I know
that
> > > absolute includes are usually used in Chromium, but absolute includes
won't
> > work
> > > in client code of the dynamic framework.
> 
> I don't think we should have this here as-is. Maybe it's possible to use a
> define of some sort within ios/web_view/BUILD.gn in the
> ios_framework_bundle("web_view") section and if it is building the framework,
> include it like #include <ChromeWebView/cwv_export.h> otherwise, include it
with
> absolute path. I'm not positive this will work, but may be worth trying.

Thanks, I tried it and it looks working. It looks a bit silly that we need to
write 5 lines of code everywhere to include cwv_export.h which essentially is
just 5 lines, though. 

Alternative idea is to directly embed the contents of cwv_export.h to all files.
But I slightly prefer the current structure because we probably don't want to
copy the comments in cwv_export.h to all files as well.

Another issue I found is that source code inside //ios/web_view/shell only
accepts absolute path include, not in the form of #include <ChromeWebView/...h>.
Is it as expected, or should it accept #include <ChromeWebView/...h>? I defined
CWV_IMPLEMENTATION in //ios/web_view/shell/BUILD.gn as a workaround for now.

> > This relative include may not work for clients who are going to include this
> > framework header. For all public headers we should probably use framework
> style
> > includes with angle braces.

I thought that #include "..." usually searches for the file in the same
directory too. But isn't it always the case?

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h#n...
ios/web_view/public/cwv.h:17: CWV_EXPORT @interface CWV : NSObject
On 2017/03/22 15:18:54, Eugene But wrote:
> On 2017/03/22 09:44:24, Hiroshi Ichikawa wrote:
> > I found that this token (or even __attribute__((visibility("default"))))
> > confuses git cl format and let it format the line in a weird way. Do you
know
> of
> > any workaround?
> I don't know about workarounds, but you can file a bug agains clang-format.

Do you know where to file a bug? Do we use the one provided by Clang?:
https://clang.llvm.org/docs/ClangFormat.html

It's not critical because I can fix it manually after running clang-format,
though. It looks also tricky to format it correctly, which requires to know that
CWV_EXPORT is a macro which works as an annotation for @interface.

[michaeldo, 2017-03-23 15:12:42]

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

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/internal/BUILD...
ios/web_view/internal/BUILD.gn:40: defines = [ "CWV_IMPLEMENTATION" ]
On 2017/03/23 05:03:02, Hiroshi Ichikawa wrote:
> On 2017/03/22 23:16:22, michaeldo wrote:
> > Can you explain what this is for, probably should add explanation to the CL
> > description to for later reference in case someone has the same question.
> 
> Added a description to the CL. Also added a short description here as a
comment.

Understand now, thank you!

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h
File ios/web_view/public/cwv.h (right):

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h#n...
ios/web_view/public/cwv.h:11: #include "cwv_export.h"
On 2017/03/23 05:03:02, Hiroshi Ichikawa wrote:
> On 2017/03/22 23:16:22, michaeldo wrote:
> > On 2017/03/22 15:18:54, Eugene But wrote:
> > > On 2017/03/22 09:44:24, Hiroshi Ichikawa wrote:
> > > > Can you advise me if this relative include is appropriate here? I know
> that
> > > > absolute includes are usually used in Chromium, but absolute includes
> won't
> > > work
> > > > in client code of the dynamic framework.
> > 
> > I don't think we should have this here as-is. Maybe it's possible to use a
> > define of some sort within ios/web_view/BUILD.gn in the
> > ios_framework_bundle("web_view") section and if it is building the
framework,
> > include it like #include <ChromeWebView/cwv_export.h> otherwise, include it
> with
> > absolute path. I'm not positive this will work, but may be worth trying.
> 
> Thanks, I tried it and it looks working. It looks a bit silly that we need to
> write 5 lines of code everywhere to include cwv_export.h which essentially is
> just 5 lines, though. 
> 
> Alternative idea is to directly embed the contents of cwv_export.h to all
files.
> But I slightly prefer the current structure because we probably don't want to
> copy the comments in cwv_export.h to all files as well.
> 
> Another issue I found is that source code inside //ios/web_view/shell only
> accepts absolute path include, not in the form of #include
<ChromeWebView/...h>.
> Is it as expected, or should it accept #include <ChromeWebView/...h>? I
defined
> CWV_IMPLEMENTATION in //ios/web_view/shell/BUILD.gn as a workaround for now.
> 
> > > This relative include may not work for clients who are going to include
this
> > > framework header. For all public headers we should probably use framework
> > style
> > > includes with angle braces.
> 
> I thought that #include "..." usually searches for the file in the same
> directory too. But isn't it always the case?

I'm not sure if it will always search current directory, I suppose there are
both style reasons and functional reasons for using full path includes. I think
this solution is OK, but I'd like to hear what Eugene thinks as well.

I agree we shouldn't duplicate the same code in all the headers. I wonder if it
is reasonable to only use the framework style include as we currently only build
the framework and it's likely that no other code in Chromium code base should
depend on pieces of ios/web_view without depending on the build framework
directly. This would let us simplify this code and prevent unexpected
dependancies on ios/web_view.

I will look into why shell doesn't allow those includes. Maybe it's dependency
on this lib is not connected correctly.

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h#n...
ios/web_view/public/cwv.h:17: CWV_EXPORT @interface CWV : NSObject
On 2017/03/23 05:03:02, Hiroshi Ichikawa wrote:
> On 2017/03/22 15:18:54, Eugene But wrote:
> > On 2017/03/22 09:44:24, Hiroshi Ichikawa wrote:
> > > I found that this token (or even __attribute__((visibility("default"))))
> > > confuses git cl format and let it format the line in a weird way. Do you
> know
> > of
> > > any workaround?
> > I don't know about workarounds, but you can file a bug agains clang-format.
> 
> Do you know where to file a bug? Do we use the one provided by Clang?:
> https://clang.llvm.org/docs/ClangFormat.html
> 
> It's not critical because I can fix it manually after running clang-format,
> though. It looks also tricky to format it correctly, which requires to know
that
> CWV_EXPORT is a macro which works as an annotation for @interface.

Just a heads up that fixing it manually isn't ideal because if someone else
touches the file, they will likely not manually fix a strange format (and this
is ok and expected actually).

How is it formatting CWV_EXPORT?

https://codereview.chromium.org/2770633002/diff/20001/ios/web_view/public/cwv...
File ios/web_view/public/cwv_export.h (right):

https://codereview.chromium.org/2770633002/diff/20001/ios/web_view/public/cwv...
ios/web_view/public/cwv_export.h:14: // Here I define a macro CWV_EXPORT instead
of directly using __attribute__.
Remove "Here I". I can't find the reference, but we don't use "we" in comments,
and I don't believe we should have I in them either since it is arbitrary.

[Hiroshi Ichikawa, 2017-03-24 04:57:42]

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h
File ios/web_view/public/cwv.h (right):

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h#n...
ios/web_view/public/cwv.h:11: #include "cwv_export.h"
On 2017/03/23 15:12:41, michaeldo wrote:
> On 2017/03/23 05:03:02, Hiroshi Ichikawa wrote:
> > On 2017/03/22 23:16:22, michaeldo wrote:
> > > On 2017/03/22 15:18:54, Eugene But wrote:
> > > > On 2017/03/22 09:44:24, Hiroshi Ichikawa wrote:
> > > > > Can you advise me if this relative include is appropriate here? I know
> > that
> > > > > absolute includes are usually used in Chromium, but absolute includes
> > won't
> > > > work
> > > > > in client code of the dynamic framework.
> > > 
> > > I don't think we should have this here as-is. Maybe it's possible to use a
> > > define of some sort within ios/web_view/BUILD.gn in the
> > > ios_framework_bundle("web_view") section and if it is building the
> framework,
> > > include it like #include <ChromeWebView/cwv_export.h> otherwise, include
it
> > with
> > > absolute path. I'm not positive this will work, but may be worth trying.
> > 
> > Thanks, I tried it and it looks working. It looks a bit silly that we need
to
> > write 5 lines of code everywhere to include cwv_export.h which essentially
is
> > just 5 lines, though. 
> > 
> > Alternative idea is to directly embed the contents of cwv_export.h to all
> files.
> > But I slightly prefer the current structure because we probably don't want
to
> > copy the comments in cwv_export.h to all files as well.
> > 
> > Another issue I found is that source code inside //ios/web_view/shell only
> > accepts absolute path include, not in the form of #include
> <ChromeWebView/...h>.
> > Is it as expected, or should it accept #include <ChromeWebView/...h>? I
> defined
> > CWV_IMPLEMENTATION in //ios/web_view/shell/BUILD.gn as a workaround for now.
> > 
> > > > This relative include may not work for clients who are going to include
> this
> > > > framework header. For all public headers we should probably use
framework
> > > style
> > > > includes with angle braces.
> > 
> > I thought that #include "..." usually searches for the file in the same
> > directory too. But isn't it always the case?
> 
> I'm not sure if it will always search current directory, I suppose there are
> both style reasons and functional reasons for using full path includes. I
think
> this solution is OK, but I'd like to hear what Eugene thinks as well.
> 
> I agree we shouldn't duplicate the same code in all the headers. I wonder if
it
> is reasonable to only use the framework style include as we currently only
build
> the framework and it's likely that no other code in Chromium code base should
> depend on pieces of ios/web_view without depending on the build framework
> directly. This would let us simplify this code and prevent unexpected
> dependancies on ios/web_view.
> 
> I will look into why shell doesn't allow those includes. Maybe it's dependency
> on this lib is not connected correctly.

Do you mean that
#include <ChromeWebView/cwv_export.h>
should work while building the framework?

Currently it doesn't work, causing this error:

In file included from ../../ios/web_view/internal/cwv.mm:5:
../../ios/web_view/public/cwv.h:14:10: fatal error: 'ChromeWebView/cwv_export.h'
file not found

https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h#n...
ios/web_view/public/cwv.h:17: CWV_EXPORT @interface CWV : NSObject
On 2017/03/23 15:12:41, michaeldo wrote:
> On 2017/03/23 05:03:02, Hiroshi Ichikawa wrote:
> > On 2017/03/22 15:18:54, Eugene But wrote:
> > > On 2017/03/22 09:44:24, Hiroshi Ichikawa wrote:
> > > > I found that this token (or even __attribute__((visibility("default"))))
> > > > confuses git cl format and let it format the line in a weird way. Do you
> > know
> > > of
> > > > any workaround?
> > > I don't know about workarounds, but you can file a bug agains
clang-format.
> > 
> > Do you know where to file a bug? Do we use the one provided by Clang?:
> > https://clang.llvm.org/docs/ClangFormat.html
> > 
> > It's not critical because I can fix it manually after running clang-format,
> > though. It looks also tricky to format it correctly, which requires to know
> that
> > CWV_EXPORT is a macro which works as an annotation for @interface.
> 
> Just a heads up that fixing it manually isn't ideal because if someone else
> touches the file, they will likely not manually fix a strange format (and this
> is ok and expected actually).
> 
> How is it formatting CWV_EXPORT?

I agree that fixing manually every time is annoying.

I found that format won't be broken if I break the line after CWV_EXPORT, so
updated in that way. How does it look? I'm not sure if it's preferred to break
the line after CWV_EXPORT anyway (I believe it's not covered by the style
guide).

This is the current format by "git cl format" without breaking the line after
CWV_EXPORT:


// Main interface for the CWV library.
CWV_EXPORT @interface CWV
    : NSObject

      // Initializes the CWV library. This function should be called from
      // |application:didFinishLaunchingWithOptions:|.
      +
      (void)configureWithDelegate : (id<CWVDelegate>)delegate;

// Shuts down the CWV library.  This function should be called from
// |applicationwillTerminate:|.
+ (void)shutDown;

// Creates and returns a web view.
+ (CWVWebView*)webViewWithFrame:(CGRect)frame;

@end

[Hiroshi Ichikawa, 2017-03-24 06:34:20]

https://codereview.chromium.org/2770633002/diff/20001/ios/web_view/public/cwv...
File ios/web_view/public/cwv_export.h (right):

https://codereview.chromium.org/2770633002/diff/20001/ios/web_view/public/cwv...
ios/web_view/public/cwv_export.h:14: // Here I define a macro CWV_EXPORT instead
of directly using __attribute__.
On 2017/03/23 15:12:42, michaeldo wrote:
> Remove "Here I". I can't find the reference, but we don't use "we" in
comments,
> and I don't believe we should have I in them either since it is arbitrary.

Done.

[michaeldo, 2017-03-24 15:42:52]

On 2017/03/24 04:57:42, Hiroshi Ichikawa wrote:
> https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h
> File ios/web_view/public/cwv.h (right):
> 
>
https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h#n...
> ios/web_view/public/cwv.h:11: #include "cwv_export.h"
> On 2017/03/23 15:12:41, michaeldo wrote:
> > On 2017/03/23 05:03:02, Hiroshi Ichikawa wrote:
> > > On 2017/03/22 23:16:22, michaeldo wrote:
> > > > On 2017/03/22 15:18:54, Eugene But wrote:
> > > > > On 2017/03/22 09:44:24, Hiroshi Ichikawa wrote:
> > > > > > Can you advise me if this relative include is appropriate here? I
know
> > > that
> > > > > > absolute includes are usually used in Chromium, but absolute
includes
> > > won't
> > > > > work
> > > > > > in client code of the dynamic framework.
> > > > 
> > > > I don't think we should have this here as-is. Maybe it's possible to use
a
> > > > define of some sort within ios/web_view/BUILD.gn in the
> > > > ios_framework_bundle("web_view") section and if it is building the
> > framework,
> > > > include it like #include <ChromeWebView/cwv_export.h> otherwise, include
> it
> > > with
> > > > absolute path. I'm not positive this will work, but may be worth trying.
> > > 
> > > Thanks, I tried it and it looks working. It looks a bit silly that we need
> to
> > > write 5 lines of code everywhere to include cwv_export.h which essentially
> is
> > > just 5 lines, though. 
> > > 
> > > Alternative idea is to directly embed the contents of cwv_export.h to all
> > files.
> > > But I slightly prefer the current structure because we probably don't want
> to
> > > copy the comments in cwv_export.h to all files as well.
> > > 
> > > Another issue I found is that source code inside //ios/web_view/shell only
> > > accepts absolute path include, not in the form of #include
> > <ChromeWebView/...h>.
> > > Is it as expected, or should it accept #include <ChromeWebView/...h>? I
> > defined
> > > CWV_IMPLEMENTATION in //ios/web_view/shell/BUILD.gn as a workaround for
now.
> > > 
> > > > > This relative include may not work for clients who are going to
include
> > this
> > > > > framework header. For all public headers we should probably use
> framework
> > > > style
> > > > > includes with angle braces.
> > > 
> > > I thought that #include "..." usually searches for the file in the same
> > > directory too. But isn't it always the case?
> > 
> > I'm not sure if it will always search current directory, I suppose there are
> > both style reasons and functional reasons for using full path includes. I
> think
> > this solution is OK, but I'd like to hear what Eugene thinks as well.
> > 
> > I agree we shouldn't duplicate the same code in all the headers. I wonder if
> it
> > is reasonable to only use the framework style include as we currently only
> build
> > the framework and it's likely that no other code in Chromium code base
should
> > depend on pieces of ios/web_view without depending on the build framework
> > directly. This would let us simplify this code and prevent unexpected
> > dependancies on ios/web_view.
> > 
> > I will look into why shell doesn't allow those includes. Maybe it's
dependency
> > on this lib is not connected correctly.
> 
> Do you mean that
> #include <ChromeWebView/cwv_export.h>
> should work while building the framework?
> 
> Currently it doesn't work, causing this error:
> 
> In file included from ../../ios/web_view/internal/cwv.mm:5:
> ../../ios/web_view/public/cwv.h:14:10: fatal error:
'ChromeWebView/cwv_export.h'
> file not found


Yes, I believe it should work, but for some reason it doesn't currently. I sent
out an email about this and am working to try to fix the BUILD files so that
these style headers can be used in ios/web_view/shell.


> 
>
https://codereview.chromium.org/2770633002/diff/1/ios/web_view/public/cwv.h#n...
> ios/web_view/public/cwv.h:17: CWV_EXPORT @interface CWV : NSObject
> On 2017/03/23 15:12:41, michaeldo wrote:
> > On 2017/03/23 05:03:02, Hiroshi Ichikawa wrote:
> > > On 2017/03/22 15:18:54, Eugene But wrote:
> > > > On 2017/03/22 09:44:24, Hiroshi Ichikawa wrote:
> > > > > I found that this token (or even
__attribute__((visibility("default"))))
> > > > > confuses git cl format and let it format the line in a weird way. Do
you
> > > know
> > > > of
> > > > > any workaround?
> > > > I don't know about workarounds, but you can file a bug agains
> clang-format.
> > > 
> > > Do you know where to file a bug? Do we use the one provided by Clang?:
> > > https://clang.llvm.org/docs/ClangFormat.html
> > > 
> > > It's not critical because I can fix it manually after running
clang-format,
> > > though. It looks also tricky to format it correctly, which requires to
know
> > that
> > > CWV_EXPORT is a macro which works as an annotation for @interface.
> > 
> > Just a heads up that fixing it manually isn't ideal because if someone else
> > touches the file, they will likely not manually fix a strange format (and
this
> > is ok and expected actually).
> > 
> > How is it formatting CWV_EXPORT?
> 
> I agree that fixing manually every time is annoying.
> 
> I found that format won't be broken if I break the line after CWV_EXPORT, so
> updated in that way. How does it look? I'm not sure if it's preferred to break
> the line after CWV_EXPORT anyway (I believe it's not covered by the style
> guide).
> 
> This is the current format by "git cl format" without breaking the line after
> CWV_EXPORT:
> 
> 
> // Main interface for the CWV library.
> CWV_EXPORT @interface CWV
>     : NSObject
> 
>       // Initializes the CWV library. This function should be called from
>       // |application:didFinishLaunchingWithOptions:|.
>       +
>       (void)configureWithDelegate : (id<CWVDelegate>)delegate;
> 
> // Shuts down the CWV library.  This function should be called from
> // |applicationwillTerminate:|.
> + (void)shutDown;
> 
> // Creates and returns a web view.
> + (CWVWebView*)webViewWithFrame:(CGRect)frame;
> 
> @end


I much prefer the break after CWV_EXPORT as you have now over the git cl format
version. Inside WebKit headers, their macro is on it's own line although it
looks a little different because they also have the availability macro there
like "API_AVAILABLE(...)" but I think it is OK on it's own line.

[michaeldo, 2017-03-24 15:49:25]

lgtm

https://codereview.chromium.org/2770633002/diff/60001/ios/web_view/shell/BUIL...
File ios/web_view/shell/BUILD.gn (right):

https://codereview.chromium.org/2770633002/diff/60001/ios/web_view/shell/BUIL...
ios/web_view/shell/BUILD.gn:33: # A workaround for an issue that we need to use
the form:
Please add crbug.com/704946 reference here.

[Eugene But (OOO till 7-30), 2017-03-24 16:39:20]

https://codereview.chromium.org/2770633002/diff/60001/ios/web_view/internal/B...
File ios/web_view/internal/BUILD.gn (right):

https://codereview.chromium.org/2770633002/diff/60001/ios/web_view/internal/B...
ios/web_view/internal/BUILD.gn:42: defines = [ "CWV_IMPLEMENTATION" ]
Is it possible to avoid duplicating CWV_IMPLEMENTATION?

https://codereview.chromium.org/2770633002/diff/60001/ios/web_view/public/cri...
File ios/web_view/public/criwv_translate_manager.h (right):

https://codereview.chromium.org/2770633002/diff/60001/ios/web_view/public/cri...
ios/web_view/public/criwv_translate_manager.h:10: #if
defined(CWV_IMPLEMENTATION)
This is quite ugly for a public framework header. WKWebView.h header does not
have these ifdefs, so can we do the same thing? ChromeWebView only supports
dynamic linking and if we ever need to support static linking, then we can add
ifdefs back. WDYT?

[Hiroshi Ichikawa, 2017-03-27 02:09:25]

https://codereview.chromium.org/2770633002/diff/60001/ios/web_view/internal/B...
File ios/web_view/internal/BUILD.gn (right):

https://codereview.chromium.org/2770633002/diff/60001/ios/web_view/internal/B...
ios/web_view/internal/BUILD.gn:42: defines = [ "CWV_IMPLEMENTATION" ]
On 2017/03/24 16:39:20, Eugene But wrote:
> Is it possible to avoid duplicating CWV_IMPLEMENTATION? 

What do you mean by duplicating? Dup between which and which?

https://codereview.chromium.org/2770633002/diff/60001/ios/web_view/public/cri...
File ios/web_view/public/criwv_translate_manager.h (right):

https://codereview.chromium.org/2770633002/diff/60001/ios/web_view/public/cri...
ios/web_view/public/criwv_translate_manager.h:10: #if
defined(CWV_IMPLEMENTATION)
On 2017/03/24 16:39:20, Eugene But wrote:
> This is quite ugly for a public framework header. WKWebView.h header does not
> have these ifdefs, so can we do the same thing? ChromeWebView only supports
> dynamic linking and if we ever need to support static linking, then we can add
> ifdefs back. WDYT?

This is currently necessary, even just to support dynamic framework. Pasting my
comment to another comment for reference:

> Do you mean that
> #include <ChromeWebView/cwv_export.h>
> should work while building the framework?
> 
> Currently it doesn't work, causing this error:
> 
> In file included from ../../ios/web_view/internal/cwv.mm:5:
> ../../ios/web_view/public/cwv.h:14:10: fatal error:
'ChromeWebView/cwv_export.h'
> file not found

We are trying to find a solution, but can I leave it as is and submit this until
we find a solution? Added a TODO comment.

https://codereview.chromium.org/2770633002/diff/60001/ios/web_view/shell/BUIL...
File ios/web_view/shell/BUILD.gn (right):

https://codereview.chromium.org/2770633002/diff/60001/ios/web_view/shell/BUIL...
ios/web_view/shell/BUILD.gn:33: # A workaround for an issue that we need to use
the form:
On 2017/03/24 15:49:25, michaeldo wrote:
> Please add crbug.com/704946 reference here.

Done.

[Eugene But (OOO till 7-30), 2017-03-27 16:24:29]

lgtm for a (hopefully) temporary solution. Sorry for holding this

[Hiroshi Ichikawa, 2017-03-28 00:24:21]

The CQ bit was checked by ichikawa@chromium.org

[Hiroshi Ichikawa, 2017-03-28 00:24:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeldo@chromium.org
Link to the patchset: https://codereview.chromium.org/2770633002/#ps100001
(title: "Add TODO comment.")

[commit-bot: I haz the power, 2017-03-28 00:24:55]

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-28 00:28:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 00:28:16]

Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Hiroshi Ichikawa, 2017-03-28 03:29:15]

The CQ bit was checked by ichikawa@chromium.org

[Hiroshi Ichikawa, 2017-03-28 03:29:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeldo@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2770633002/#ps120001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-03-28 03:29:38]

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-28 03:37:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 03:37:05]

Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[Hiroshi Ichikawa, 2017-03-28 04:35:02]

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

https://codereview.chromium.org/2770633002/diff/140001/ios/web_view/BUILD.gn#...
ios/web_view/BUILD.gn:22: "public/cwv_export.h",
I needed to add this here too, otherwise it causes a weird error below. Maybe we
should add all headers to "sources" as well?

$ gn gen //out/Debug-iphonesimulator --check
ERROR at //ios/web_view/public/cwv.h:14:11: Include not allowed.
#include "ios/web_view/public/cwv_export.h"
          ^-------------------------------
It is not in any dependency of
  //ios/web_view:web_view_compile_headers_map
The include file is in the target(s):
  //ios/web_view:web_view_copy_public_headers
  //ios/web_view/public:public
at least one of which should somehow be reachable.

[Hiroshi Ichikawa, 2017-03-28 04:35:17]

The CQ bit was checked by ichikawa@chromium.org

[Hiroshi Ichikawa, 2017-03-28 04:35:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeldo@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2770633002/#ps140001
(title: "Add cwv_export.h to "sources" attribute of //ios/web_view:web_view.")

[commit-bot: I haz the power, 2017-03-28 04:35:38]

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-28 05:34:49]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490675717493410,
"parent_rev": "6badfa25e06f54413e408cd6d14427a7f44356a1", "commit_rev":
"a548ad0c0108bc06d5e622acfc89777bcb00ee23"}

[commit-bot: I haz the power, 2017-03-28 05:36:44]

Description was changed from

==========
Export all CWV* symbols from the ios/web_view dynamic library.

Only symbols explicitly marked __attribute__((visibility("default"))) are
exported from a dynamic library. So all public classes in ios/web_view
must be marked as __attribute__((visibility("default"))). Otherwise the
users of the dynamic library cannot access these classes.

Here I followed other code using __attribute__((visibility("default")))
in Chromium like this one:
https://cs.chromium.org/chromium/src/dbus/dbus_export.h
to define a macro CWV_EXPORT instead of directly using __attribute__.

This makes it possible to export symbols only when *building* the dynamic
library (by checking CWV_IMPLEMENTATION), not when *using* the dynamic
library.

BUG=704043
==========

to

==========
Export all CWV* symbols from the ios/web_view dynamic library.

Only symbols explicitly marked __attribute__((visibility("default"))) are
exported from a dynamic library. So all public classes in ios/web_view
must be marked as __attribute__((visibility("default"))). Otherwise the
users of the dynamic library cannot access these classes.

Here I followed other code using __attribute__((visibility("default")))
in Chromium like this one:
https://cs.chromium.org/chromium/src/dbus/dbus_export.h
to define a macro CWV_EXPORT instead of directly using __attribute__.

This makes it possible to export symbols only when *building* the dynamic
library (by checking CWV_IMPLEMENTATION), not when *using* the dynamic
library.

BUG=704043

Review-Url: https://codereview.chromium.org/2770633002
Cr-Commit-Position: refs/heads/master@{#460017}
Committed:
https://chromium.googlesource.com/chromium/src/+/a548ad0c0108bc06d5e622acfc89...
==========

[commit-bot: I haz the power, 2017-03-28 05:36:48]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/a548ad0c0108bc06d5e622acfc89...

[sdefresne, 2017-03-28 10:29:26]

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

[sdefresne, 2017-03-28 10:29:27]

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

https://codereview.chromium.org/2770633002/diff/140001/ios/web_view/BUILD.gn#...
ios/web_view/BUILD.gn:22: "public/cwv_export.h",
On 2017/03/28 04:35:02, Hiroshi Ichikawa wrote:
> I needed to add this here too, otherwise it causes a weird error below. Maybe
we
> should add all headers to "sources" as well?
> 
> $ gn gen //out/Debug-iphonesimulator --check
> ERROR at //ios/web_view/public/cwv.h:14:11: Include not allowed.
> #include "ios/web_view/public/cwv_export.h"
>           ^-------------------------------
> It is not in any dependency of
>   //ios/web_view:web_view_compile_headers_map
> The include file is in the target(s):
>   //ios/web_view:web_view_copy_public_headers
>   //ios/web_view/public:public
> at least one of which should somehow be reachable.

public_headers are not automatically added to sources. public_headers just
inform the build systems that the headers should be copied into the generated
framework.

[michaeldo, 2017-03-28 17:34:19]

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

https://codereview.chromium.org/2770633002/diff/140001/ios/web_view/BUILD.gn#...
ios/web_view/BUILD.gn:22: "public/cwv_export.h",
On 2017/03/28 04:35:02, Hiroshi Ichikawa wrote:
> I needed to add this here too, otherwise it causes a weird error below. Maybe
we
> should add all headers to "sources" as well?
> 
> $ gn gen //out/Debug-iphonesimulator --check
> ERROR at //ios/web_view/public/cwv.h:14:11: Include not allowed.
> #include "ios/web_view/public/cwv_export.h"
>           ^-------------------------------
> It is not in any dependency of
>   //ios/web_view:web_view_compile_headers_map
> The include file is in the target(s):
>   //ios/web_view:web_view_copy_public_headers
>   //ios/web_view/public:public
> at least one of which should somehow be reachable.

I think it's fine to only have cwv_export.h in sources. none of the other
headers are referenced so they should be fine to leave only in the
public_headers section.