Bug 67584

Created attachment 106303 [details] Proposed patch.

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?

Created attachment 106309 [details] Proposed patch 2.

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 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.

"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 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.

> I tried to remove '../' from #include path, but it caused compile errors on chromium/linux: Ok. I'm probably wrong about that then.

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 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.

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 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

(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.

(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

> 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

(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 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.

(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?

> 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.

(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.

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!

Created attachment 106835 [details] Proposed patch 5. I'd glad to clarify it. Fixed s/2010/2011/. Could someone do a final review?

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.

Created attachment 106864 [details] Proposed patch 6.

Comment on attachment 106864 [details] Proposed patch 6. r=me

Comment on attachment 106864 [details] Proposed patch 6. Clearing flags on attachment: 106864 Committed r94859: <http://trac.webkit.org/changeset/94859>

All reviewed patches have been landed. Closing bug.