On Tue, Dec 5, 2017 at 5:59 AM, Alan Third <alan@idiocy.org> wrote:

>
> In configure.ac you’re doing
>
>     elif test "$window_system" = "nextstep"; then
>
> I think instead of that you should be doing
>
>     elif test "${NS_IMPL_COCOA}" = "yes"; then
>
> as nextstep covers GNUstep, but it doesn’t have a webkit
> implementation (yet).
>

> There might be other places where NS_IMPL_COCOA is more appropriate
> than HAVE_NS, but it builds fine against GNUstep with xwidgets off, so
> it’s not too important.
>

Agree. I fixed to use "${NS_IMPL_COCOA}" in configura.ac
and #ifdef NS_IMPL_COCOA in implementation source files.
I am testing the fixes. It looks good to me also.


> I notice you’re adding .DS_Store to .gitignore, which is a good idea,
> but I don’t think it should be done in this patch. It seems a little
> off‐topic.


Okay, I will remove it from .gitignore.

I’m also a little unsure about this
>
>     <!-- FIXME: Uncomment for xwidget webkit to browse remote url,
>          but this set no restriction at all.  Consult apple's documentation
>          for detail information about `NSApplicationDefinedMask'. -->
>     <key>NSAppTransportSecurity</key>
>     <dict>
>       <key>NSAllowsArbitraryLoads</key>
>       <true/>
>     </dict>
>

> The comment says ‘uncomment’, but it *is* uncommented. Am I
> misunderstanding?
>

You are right, the comment is outdated and I will fix it.


> (Also I think there should be two spaces at the end of comments.)
>

I was curious about the two spaces ending comments.  It is clear now thanks
to you.
I will fix this also.


>
> I think we’ll want to add NSTRACE lines in each function, but perhaps
> we should create a new category for XWidgets.


I put off this item at this time, there will be chance to do this later.

I will soon post a patch accompanying above changes.

Thanks for your valuable comments.