Bug 67584 – [Chromium] Add WebSandboxSupport and WebThemeEngine for Android.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Hao Zheng]

This will make src/PlatformSupport.cpp and src/WebFrameImpl.cpp compile on Chromium port for Android.

[Hao Zheng]

Created attachment 106303 [details]
Proposed patch.

[Adam Barth]

Comment on attachment 106303 [details]
Proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=106303&action=review

> Source/WebKit/chromium/public/android/WebThemeEngine.h:36
> +#include "../WebCanvas.h"
> +#include "../WebColor.h"
> +#include "../WebSize.h"

I think these includes shouldn't have the "../" part, but I'm not 100% sure.

> Source/WebKit/chromium/src/PlatformSupport.cpp:467
> +    // TODO(zhenghao): use simple logic for now.

TODO(zhenghao) => FIXME

> Source/WebKit/chromium/src/PlatformSupport.cpp:468
> +    return WebString("Arial");

This seems really lame.  Should we file a bug and include a link here?

[Hao Zheng]

Created attachment 106309 [details]
Proposed patch 2.

[Hao Zheng]

Created attachment 106310 [details]
Proposed patch 3.

I tried to remove '../' from #include path, but it caused compile errors on chromium/linux:
In file included from out/Release/obj/gen/webkit/third_party/WebKit/Source/WebKit/chromium/public/linux/WebThemeEngine.h:2,
                 from Source/WebKit/chromium/webkit/glue/webthemeengine_impl_linux.h:8,
                 from Source/WebKit/chromium/webkit/glue/webkitclient_impl.h:16,
                 from Source/WebKit/chromium/webkit/glue/webkitclient_impl.cc:5:
So for future implementation of webthemeengine_impl_android, we need keep '../'.

[Steve Block]

Comment on attachment 106310 [details]
Proposed patch 3.

View in context: https://bugs.webkit.org/attachment.cgi?id=106310&action=review

> Source/WebKit/chromium/WebKit.gyp:92
> +                'public/android/WebSandboxSupport.h',

This new file doesn't seem to be used by any of the other code you're adding here, so is it best to add it in a separate change with the code that uses it? That way the addition will have more context and you can perhaps add some of the methods too, rather than just an empty class.

[Hao Zheng]

"WebSandboxSupport.h" is used by PlatformSupport.cpp. I need it to compile successfully.

(In reply to comment #5)
> (From update of attachment 106310 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106310&action=review
> 
> > Source/WebKit/chromium/WebKit.gyp:92
> > +                'public/android/WebSandboxSupport.h',
> 
> This new file doesn't seem to be used by any of the other code you're adding here, so is it best to add it in a separate change with the code that uses it? That way the addition will have more context and you can perhaps add some of the methods too, rather than just an empty class.

[Steve Block]

Comment on attachment 106310 [details]
Proposed patch 3.

View in context: https://bugs.webkit.org/attachment.cgi?id=106310&action=review

>>> Source/WebKit/chromium/WebKit.gyp:92

>> 
>> This new file doesn't seem to be used by any of the other code you're adding here, so is it best to add it in a separate change with the code that uses it? That way the addition will have more context and you can perhaps add some of the methods too, rather than just an empty class.
> 
> 

But WebSandboxSupport isn't used in PlatformSupport.cpp on Android, right? So I still think it's best to hold off on WebSandboxSupport until there's more context. Adding an empty class is a little odd.

[Adam Barth]

> I tried to remove '../' from #include path, but it caused compile errors on chromium/linux:

Ok.  I'm probably wrong about that then.

[Adam Barth]

When you're ready, please add [e-mail] to the CC list.  Because this patch touches code in Source/WebKit/chromium/public, he'll need to do the final review.

[Steve Block]

Comment on attachment 106310 [details]
Proposed patch 3.

View in context: https://bugs.webkit.org/attachment.cgi?id=106310&action=review

> Source/WebKit/chromium/public/android/WebSandboxSupport.h:38
> +class WebSandboxSupport {

As discussed, I'm very hesitant about adding an empty class. You should either provide an explanation of why it's empty, a bug for when it will be filled out, or ideally avoid the need for it altogether.

[Hao Zheng]

Created attachment 106385 [details]
Proposed patch 4.

WebKit::WebSandboxSupport is needed for many files to compile in chromium ('grep WebSandboxSupport' in WebKit&chromium dir), so use an empty class for now. Add a FIXME there.

[Steve Block]

Comment on attachment 106385 [details]
Proposed patch 4.

View in context: https://bugs.webkit.org/attachment.cgi?id=106385&action=review

LGTM

> Source/WebKit/chromium/public/android/WebSandboxSupport.h:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

s/2010/2011

> Source/WebKit/chromium/public/android/WebSandboxSupport.h:36
> +// FIXME: Empty class for now, as we need it to compile.

Please link to a bug so this ugliness doesn't get forgotten about.

> Source/WebKit/chromium/public/android/WebSandboxSupport.h:37
> +// Put methods here that are required due to sandbox restrictions.

I don't think this comment is needed

> Source/WebKit/chromium/public/android/WebThemeEngine.h:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

s/2010/2011

[Hao Zheng]

(In reply to comment #12)
> (From update of attachment 106385 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106385&action=review
> 
> LGTM

Thanks!

> 
> > Source/WebKit/chromium/public/android/WebSandboxSupport.h:2
> > + * Copyright (C) 2010 Google Inc. All rights reserved.
> 
> s/2010/2011

Ok.

> 
> > Source/WebKit/chromium/public/android/WebSandboxSupport.h:36
> > +// FIXME: Empty class for now, as we need it to compile.
> 
> Please link to a bug so this ugliness doesn't get forgotten about.
> 

Actually, I don't think it's ugly at all. All we need is a null class to conform to the APIs. And we don't have any methods in our own code (not yet upstreamed code) also, until now at least. It's same as extracting an interface, and providing some substantial implementation on some ports, and providing a null implementation on other ports. If we don't expect to add any methods there, shall I file a bug?

> > Source/WebKit/chromium/public/android/WebSandboxSupport.h:37
> > +// Put methods here that are required due to sandbox restrictions.
> 
> I don't think this comment is needed
> 
> > Source/WebKit/chromium/public/android/WebThemeEngine.h:2
> > + * Copyright (C) 2010 Google Inc. All rights reserved.
> 
> s/2010/2011

Ok.

[Peter Beverloo]

(In reply to comment #13)
> > 
> > > Source/WebKit/chromium/public/android/WebSandboxSupport.h:36
> > > +// FIXME: Empty class for now, as we need it to compile.
> > 
> > Please link to a bug so this ugliness doesn't get forgotten about.
> > 
> 
> Actually, I don't think it's ugly at all. All we need is a null class to conform to the APIs. And we don't have any methods in our own code (not yet upstreamed code) also, until now at least. It's same as extracting an interface, and providing some substantial implementation on some ports, and providing a null implementation on other ports. If we don't expect to add any methods there, shall I file a bug?

I think the ideal solution would be to leave out the WebSandboxSupport.h file for Android altogether as we don't need it (see the comment at WebKitPlatformSupport::sandboxSupport() [1]).
This will indeed cause errors at Chromium's side[2], for which we either have to define WebKit::WebSandboxSupport as an empty class there, or change RendererWebKitPlatformSupportImpl::SandboxSupport to not inherit from it. Either way, RendererWebKitPlatformSupportImpl::sandboxSupport should probably return NULL.

fishd, do you have a view on this? The comment states that sandboxSupport() can return NULL in case the platform doesn't need it, which is the case for Android, but current code requires the class WebKit::WebSandboxSupport to be defined.

[1] http://codesearch.google.com/#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/public/WebKitPlatformSupport.h&l=85
[2] See renderer_webkitplatformsupport_impl.cc and ppapi_webkitplatformsupport_impl.cc

[Steve Block]

> This will indeed cause errors at Chromium's side[2], for which we either have to define WebKit::WebSandboxSupport as an empty class there, or change RendererWebKitPlatformSupportImpl::SandboxSupport to not inherit from it. Either way, RendererWebKitPlatformSupportImpl::sandboxSupport should probably return NULL.
Agreed

[Hao Zheng]

(In reply to comment #14)
> (In reply to comment #13)
> > > 
> > > > Source/WebKit/chromium/public/android/WebSandboxSupport.h:36
> > > > +// FIXME: Empty class for now, as we need it to compile.
> > > 
> > > Please link to a bug so this ugliness doesn't get forgotten about.
> > > 
> > 
> > Actually, I don't think it's ugly at all. All we need is a null class to conform to the APIs. And we don't have any methods in our own code (not yet upstreamed code) also, until now at least. It's same as extracting an interface, and providing some substantial implementation on some ports, and providing a null implementation on other ports. If we don't expect to add any methods there, shall I file a bug?
> 
> I think the ideal solution would be to leave out the WebSandboxSupport.h file for Android altogether as we don't need it (see the comment at WebKitPlatformSupport::sandboxSupport() [1]).

Even we return NULL at WebKitPlatformSupport::sandboxSupport() and other similar points, we still need an empty class in WebSandboxSupport.h, because the return type is WebSandboxSupport. If we don't provide an empty class, there will be link error. It's an API, so I think we cannot say we don't need it.

> This will indeed cause errors at Chromium's side[2], for which we either have to define WebKit::WebSandboxSupport as an empty class there, or change RendererWebKitPlatformSupportImpl::SandboxSupport to not inherit from it. Either way, RendererWebKitPlatformSupportImpl::sandboxSupport should probably return NULL.
> 
> fishd, do you have a view on this? The comment states that sandboxSupport() can return NULL in case the platform doesn't need it, which is the case for Android, but current code requires the class WebKit::WebSandboxSupport to be defined.
> 
> [1] http://codesearch.google.com/#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/public/WebKitPlatformSupport.h&l=85
> [2] See renderer_webkitplatformsupport_impl.cc and ppapi_webkitplatformsupport_impl.cc

[Darin Fisher (:fishd, Google)]

Comment on attachment 106385 [details]
Proposed patch 4.

View in context: https://bugs.webkit.org/attachment.cgi?id=106385&action=review

> Source/WebKit/chromium/public/android/WebSandboxSupport.h:38
> +class WebSandboxSupport {

I'm confused about why you need this.  Ordinarily, this is only required if someone actually tries to use the pointer returned via WebKitPlatformSupport::sandboxSupport().  Otherwise the "class WebSandboxSupport;" forward decl should be sufficient.

[Hao Zheng]

(In reply to comment #17)
> (From update of attachment 106385 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106385&action=review
> 
> > Source/WebKit/chromium/public/android/WebSandboxSupport.h:38
> > +class WebSandboxSupport {
> 
> I'm confused about why you need this.  Ordinarily, this is only required if someone actually tries to use the pointer returned via WebKitPlatformSupport::sandboxSupport().  Otherwise the "class WebSandboxSupport;" forward decl should be sufficient.

Hi Darin,
Code won't compile if we don't supply WebSandboxSupport.h.

third_party/WebKit/Source/WebKit/chromium/src/PlatformSupport.cpp:55:31: error: WebSandboxSupport.h: No such file or directory
make: *** [out/Release/obj.target/webkit/third_party/WebKit/Source/WebKit/chromium/src/PlatformBridge.o] Error 1

Shall I use some OS guard to exclude it on Android?

[Adam Barth]

> Shall I use some OS guard to exclude it on Android?

Does that file exist in any configuration?  Perhaps we should just remove that include line.

[Hao Zheng]

(In reply to comment #19)
> > Shall I use some OS guard to exclude it on Android?
> 
> Does that file exist in any configuration?  Perhaps we should just remove that include line.

The file is provided by each platform:
./third_party/WebKit/Source/WebKit/chromium/public/linux/WebSandboxSupport.h
./third_party/WebKit/Source/WebKit/chromium/public/mac/WebSandboxSupport.h
./third_party/WebKit/Source/WebKit/chromium/public/win/WebSandboxSupport.h

We cannot remove that line, or other platforms won't compile.

And I tried more. Even for Android, while PlatformSupport can compile if I removed the line, other compile errors occur if we don't provide android/WebSandboxSupport.h:

content/ppapi_plugin/ppapi_webkitclient_impl.cc:29: error: invalid use of incomplete type 'struct WebKit::WebSandboxSupport'
./third_party/WebKit/Source/WebKit/chromium/public/WebKitClient.h:68: error: forward declaration of 'struct WebKit::WebSandboxSupport'
content/ppapi_plugin/ppapi_webkitclient_impl.cc: In member function 'virtual WebKit::WebSandboxSupport* PpapiWebKitClientImpl::sandboxSupport()':
content/ppapi_plugin/ppapi_webkitclient_impl.cc:132: error: cannot convert 'PpapiWebKitClientImpl::SandboxSupport*' to 'WebKit::WebSandboxSupport*' in return
make: *** [out/Release/obj.target/content_ppapi_plugin/content/ppapi_plugin/ppapi_webkitclient_impl.o] Error 1

./content/renderer/renderer_webkitclient_impl.cc:118 has the same problem.

[Darin Fisher (:fishd, Google)]

OK, thanks for the explanation.  I agree that adding an empty WebSandboxSupport is the right answer.  I was remembering a time when we only had that defined for Windows, and I thought that was still the case, but given that every other platform has it, Android should too.  That way we can avoid #ifdefs!

[Hao Zheng]

Created attachment 106835 [details]
Proposed patch 5.

I'd glad to clarify it.

Fixed s/2010/2011/. Could someone do a final review?

[Steve Block]

Comment on attachment 106835 [details]
Proposed patch 5.

View in context: https://bugs.webkit.org/attachment.cgi?id=106835&action=review

> Source/WebKit/chromium/public/android/WebSandboxSupport.h:36
> +// Empty class for now, as we need it to compile.

I still think 'for now' is confusing unless you have plans to add methods.

[Hao Zheng]

Created attachment 106864 [details]
Proposed patch 6.

[Steve Block]

Comment on attachment 106864 [details]
Proposed patch 6.

r=me

[WebKit Review Bot]

Comment on attachment 106864 [details]
Proposed patch 6.

Clearing flags on attachment: 106864

Committed r94859: <http://trac.webkit.org/changeset/94859>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.