Bug 67584 - [Chromium] Add WebSandboxSupport and WebThemeEngine for Android.
Summary: [Chromium] Add WebSandboxSupport and WebThemeEngine for Android.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Android Android
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 66689
  Show dependency treegraph
 
Reported: 2011-09-04 21:43 PDT by Hao Zheng
Modified: 2011-09-09 09:57 PDT (History)
6 users (show)

See Also:


Attachments
Proposed patch. (11.63 KB, patch)
2011-09-04 21:48 PDT, Hao Zheng
no flags Details | Formatted Diff | Diff
Proposed patch 2. (18.77 KB, patch)
2011-09-04 23:29 PDT, Hao Zheng
no flags Details | Formatted Diff | Diff
Proposed patch 3. (11.71 KB, patch)
2011-09-04 23:49 PDT, Hao Zheng
steveblock: review-
Details | Formatted Diff | Diff
Proposed patch 4. (11.75 KB, patch)
2011-09-05 21:23 PDT, Hao Zheng
no flags Details | Formatted Diff | Diff
Proposed patch 5. (11.68 KB, patch)
2011-09-08 21:03 PDT, Hao Zheng
steveblock: review+
Details | Formatted Diff | Diff
Proposed patch 6. (11.67 KB, patch)
2011-09-09 06:24 PDT, Hao Zheng
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hao Zheng 2011-09-04 21:43:33 PDT
This will make src/PlatformSupport.cpp and src/WebFrameImpl.cpp compile on Chromium port for Android.
Comment 1 Hao Zheng 2011-09-04 21:48:36 PDT
Created attachment 106303 [details]
Proposed patch.
Comment 2 Adam Barth 2011-09-04 22:15:35 PDT
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?
Comment 3 Hao Zheng 2011-09-04 23:29:48 PDT
Created attachment 106309 [details]
Proposed patch 2.
Comment 4 Hao Zheng 2011-09-04 23:49:25 PDT
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 '../'.
Comment 5 Steve Block 2011-09-05 02:24:48 PDT
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.
Comment 6 Hao Zheng 2011-09-05 02:28:55 PDT
"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.
Comment 7 Steve Block 2011-09-05 02:34:17 PDT
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.
Comment 8 Adam Barth 2011-09-05 11:40:22 PDT
> I tried to remove '../' from #include path, but it caused compile errors on chromium/linux:

Ok.  I'm probably wrong about that then.
Comment 9 Adam Barth 2011-09-05 11:42:09 PDT
When you're ready, please add fishd@chromium.org to the CC list.  Because this patch touches code in Source/WebKit/chromium/public, he'll need to do the final review.
Comment 10 Steve Block 2011-09-05 13:20:56 PDT
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.
Comment 11 Hao Zheng 2011-09-05 21:23:25 PDT
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.
Comment 12 Steve Block 2011-09-06 02:40:20 PDT
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
Comment 13 Hao Zheng 2011-09-06 03:02:03 PDT
(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.
Comment 14 Peter Beverloo 2011-09-06 04:23:41 PDT
(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
Comment 15 Steve Block 2011-09-06 04:31:11 PDT
> 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
Comment 16 Hao Zheng 2011-09-06 08:34:47 PDT
(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
Comment 17 Darin Fisher (:fishd, Google) 2011-09-08 00:16:53 PDT
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.
Comment 18 Hao Zheng 2011-09-08 01:31:06 PDT
(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?
Comment 19 Adam Barth 2011-09-08 01:43:27 PDT
> 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.
Comment 20 Hao Zheng 2011-09-08 02:17:08 PDT
(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.
Comment 21 Darin Fisher (:fishd, Google) 2011-09-08 11:14:42 PDT
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!
Comment 22 Hao Zheng 2011-09-08 21:03:55 PDT
Created attachment 106835 [details]
Proposed patch 5.

I'd glad to clarify it.

Fixed s/2010/2011/. Could someone do a final review?
Comment 23 Steve Block 2011-09-09 01:59:40 PDT
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.
Comment 24 Hao Zheng 2011-09-09 06:24:25 PDT
Created attachment 106864 [details]
Proposed patch 6.
Comment 25 Steve Block 2011-09-09 07:04:04 PDT
Comment on attachment 106864 [details]
Proposed patch 6.

r=me
Comment 26 WebKit Review Bot 2011-09-09 09:57:17 PDT
Comment on attachment 106864 [details]
Proposed patch 6.

Clearing flags on attachment: 106864

Committed r94859: <http://trac.webkit.org/changeset/94859>
Comment 27 WebKit Review Bot 2011-09-09 09:57:23 PDT
All reviewed patches have been landed.  Closing bug.