[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
http://gwt-code-reviews.appspot.com/1084801/diff/57001/58022 File plugins/npapi/VisualStudio/npapi-plugin.sln (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58022#newcode3 plugins/npapi/VisualStudio/npapi-plugin.sln:3: # Visual Studio 2008 On 2010/11/18 22:02:02, jat wrote: Are we making a conscious decision to drop support for VS2005? Yes. http://gwt-code-reviews.appspot.com/1084801/diff/57001/58023 File plugins/npapi/VisualStudio/npapi-plugin.vcproj (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58023#newcode74 plugins/npapi/VisualStudio/npapi-plugin.vcproj:74: DataExecutionPrevention=0 On 2010/11/18 22:02:02, jat wrote: Why not DEP if we are switching to VS2008? mainly because there are known issues of DEP with ATL and we did not test that extensively. http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
On 2010/11/15 19:53:51, conroy wrote: lgtm http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
LGTM with nits. http://gwt-code-reviews.appspot.com/1084801/diff/57001/58001 File plugins/npapi/DevModeOptions/.classpath (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58001#newcode3 plugins/npapi/DevModeOptions/.classpath:3: classpathentry kind=src path=src/ Shouldn't this use a linked resource relate to GWT_ROOT, and the eclipse files be under $GWT_ROOT/eclipse? http://gwt-code-reviews.appspot.com/1084801/diff/57001/58012 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/LocalStorage.java (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58012#newcode56 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/LocalStorage.java:56: return this.setItem(key, JSON.stringify(dataObject)); Why return for a void method? http://gwt-code-reviews.appspot.com/1084801/diff/57001/58022 File plugins/npapi/VisualStudio/npapi-plugin.sln (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58022#newcode3 plugins/npapi/VisualStudio/npapi-plugin.sln:3: # Visual Studio 2008 Are we making a conscious decision to drop support for VS2005? http://gwt-code-reviews.appspot.com/1084801/diff/57001/58023 File plugins/npapi/VisualStudio/npapi-plugin.vcproj (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58023#newcode74 plugins/npapi/VisualStudio/npapi-plugin.vcproj:74: DataExecutionPrevention=0 Why not DEP if we are switching to VS2008? http://gwt-code-reviews.appspot.com/1084801/diff/57001/58024 File plugins/npapi/main.cpp (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58024#newcode26 plugins/npapi/main.cpp:26: DisableThreadLibraryCalls(hModule); Spacing. http://gwt-code-reviews.appspot.com/1084801/diff/57001/58024#newcode242 plugins/npapi/main.cpp:242: return 0 ; IIUC, this means we silently ignore any events rather than report them as unhandled -- is that right? http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
fabio, can you respond to the windows questions? http://gwt-code-reviews.appspot.com/1084801/diff/57001/58001 File plugins/npapi/DevModeOptions/.classpath (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58001#newcode3 plugins/npapi/DevModeOptions/.classpath:3: classpathentry kind=src path=src/ On 2010/11/18 22:02:02, jat wrote: Shouldn't this use a linked resource relate to GWT_ROOT, and the eclipse files be under $GWT_ROOT/eclipse? yeah, i'll fix this. http://gwt-code-reviews.appspot.com/1084801/diff/57001/58012 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/LocalStorage.java (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58012#newcode56 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/LocalStorage.java:56: return this.setItem(key, JSON.stringify(dataObject)); On 2010/11/18 22:02:02, jat wrote: Why return for a void method? because I blindly copied from speedtracer which also does thisfixed locally. http://gwt-code-reviews.appspot.com/1084801/diff/57001/58024 File plugins/npapi/main.cpp (right): http://gwt-code-reviews.appspot.com/1084801/diff/57001/58024#newcode26 plugins/npapi/main.cpp:26: DisableThreadLibraryCalls(hModule); On 2010/11/18 22:02:02, jat wrote: Spacing. fixed locally. http://gwt-code-reviews.appspot.com/1084801/diff/57001/58024#newcode242 plugins/npapi/main.cpp:242: return 0 ; On 2010/11/18 22:02:02, jat wrote: IIUC, this means we silently ignore any events rather than report them as unhandled -- is that right? Chrome calls NPP_HandleEvent in its paint loop on OSX as a workaround for the fact that OSX doesn't support child windows. This is theoretically our opportunity to draw on screen for OSX, but we're not doing that. According to the API, if we aren't handling the event we should return 0. http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
http://gwt-code-reviews.appspot.com/1084801/diff/28001/29020 File plugins/npapi/VisualStudio/npapi-plugin.vcproj (left): http://gwt-code-reviews.appspot.com/1084801/diff/28001/29020#oldcode1 plugins/npapi/VisualStudio/npapi-plugin.vcproj:1: ?xml version=1.0 encoding=UTF-8? not sure how you are building this project. I could not find any targets in build.xml or any other reference to devenv.exe you may also want to double check that the bin is release flavor as the default is debug when you invoke vs build. http://gwt-code-reviews.appspot.com/1084801/diff/28001/29020#oldcode4 plugins/npapi/VisualStudio/npapi-plugin.vcproj:4: Version=8.00 not sure what the intent was, but if you put back allowdialog.h you will need to add allowdialog.cpp in the project as well. http://gwt-code-reviews.appspot.com/1084801/diff/28001/29020#oldcode7 plugins/npapi/VisualStudio/npapi-plugin.vcproj:7: RootNamespace=npapi-plugin Also, maybe it will be in the next step, but It seems that the out bin is not checked out for replacement. I am guessing it will done a later step. http://gwt-code-reviews.appspot.com/1084801/diff/28001/29020#oldcode9 plugins/npapi/VisualStudio/npapi-plugin.vcproj:9: I made most of these fixes on my box and it is building fine. if you will I can roll them in your CL, bar knowing how you want to invoke the vs build. http://gwt-code-reviews.appspot.com/1084801/diff/28001/29028 File plugins/platform/Win/AllowDialog.h (left): http://gwt-code-reviews.appspot.com/1084801/diff/28001/29028#oldcode1 plugins/platform/Win/AllowDialog.h:1: #ifndef _H_AllowDialog looks like allowdialog.h is still referenced in main.cpp. http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
http://gwt-code-reviews.appspot.com/1084801/diff/28001/29020 File plugins/npapi/VisualStudio/npapi-plugin.vcproj (left): http://gwt-code-reviews.appspot.com/1084801/diff/28001/29020#oldcode1 plugins/npapi/VisualStudio/npapi-plugin.vcproj:1: ?xml version=1.0 encoding=UTF-8? On 2010/11/12 20:50:38, fabiomfv wrote: not sure how you are building this project. I could not find any targets in build.xml or any other reference to devenv.exe you may also want to double check that the bin is release flavor as the default is debug when you invoke vs build. huh? the build.xml is brand new and is just for building the DevModeOptions GWT module. http://gwt-code-reviews.appspot.com/1084801/diff/28001/29020#oldcode4 plugins/npapi/VisualStudio/npapi-plugin.vcproj:4: Version=8.00 On 2010/11/12 20:50:38, fabiomfv wrote: not sure what the intent was, but if you put back allowdialog.h you will need to add allowdialog.cpp in the project as well. the intent is to nuke it so we have a common, platform independent UI. http://gwt-code-reviews.appspot.com/1084801/diff/28001/29020#oldcode9 plugins/npapi/VisualStudio/npapi-plugin.vcproj:9: On 2010/11/12 20:50:38, fabiomfv wrote: I made most of these fixes on my box and it is building fine. if you will I can roll them in your CL, bar knowing how you want to invoke the vs build. my only changes here are just to nuke the allowdialog/preferences stuff. hopefully these changes didn't break the build. are you saying the build was already borked before this CL? i think the mechanism for building before was just invoking the build in VS, no? in either case, send me a patch and I'll update accordingly. http://gwt-code-reviews.appspot.com/1084801/diff/28001/29028 File plugins/platform/Win/AllowDialog.h (left): http://gwt-code-reviews.appspot.com/1084801/diff/28001/29028#oldcode1 plugins/platform/Win/AllowDialog.h:1: #ifndef _H_AllowDialog On 2010/11/12 20:50:38, fabiomfv wrote: looks like allowdialog.h is still referenced in main.cpp. fixed. http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
http://gwt-code-reviews.appspot.com/1084801/diff/28001/29020 File plugins/npapi/VisualStudio/npapi-plugin.vcproj (left): http://gwt-code-reviews.appspot.com/1084801/diff/28001/29020#oldcode1 plugins/npapi/VisualStudio/npapi-plugin.vcproj:1: ?xml version=1.0 encoding=UTF-8? On 2010/11/12 21:53:42, conroy wrote: On 2010/11/12 20:50:38, fabiomfv wrote: not sure how you are building this project. I could not find any targets in build.xml or any other reference to devenv.exe you may also want to double check that the bin is release flavor as the default is debug when you invoke vs build. huh? the build.xml is brand new and is just for building the DevModeOptions GWT module. I meant to ask how we build the plugin for windows. what invokes the project (msbuild, devenv, ant) to build. I just wanted to make sure we can build this solution successfully with these changes. http://gwt-code-reviews.appspot.com/1084801/diff/28001/29020#oldcode4 plugins/npapi/VisualStudio/npapi-plugin.vcproj:4: Version=8.00 On 2010/11/12 21:53:42, conroy wrote: On 2010/11/12 20:50:38, fabiomfv wrote: not sure what the intent was, but if you put back allowdialog.h you will need to add allowdialog.cpp in the project as well. the intent is to nuke it so we have a common, platform independent UI. my comment is that the vcproj as-is is broken and will not build. http://gwt-code-reviews.appspot.com/1084801/diff/28001/29020#oldcode9 plugins/npapi/VisualStudio/npapi-plugin.vcproj:9: On 2010/11/12 21:53:42, conroy wrote: On 2010/11/12 20:50:38, fabiomfv wrote: I made most of these fixes on my box and it is building fine. if you will I can roll them in your CL, bar knowing how you want to invoke the vs build. my only changes here are just to nuke the allowdialog/preferences stuff. hopefully these changes didn't break the build. are you saying the build was already borked before this CL? i think the mechanism for building before was just invoking the build in VS, no? in either case, send me a patch and I'll update accordingly. I just wanted to brought up that allowdialog.h was deleted in the CL and main.cpp still references it int the windows build. this will break the build for windows. http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
patch updated with some mac build fixups and the public key is now embedded in the manifest so that packed/unpacked use the same well known URI for the background page. http://gwt-code-reviews.appspot.com/1084801/diff/28001/29022 File plugins/npapi/manifest-template.json (right): http://gwt-code-reviews.appspot.com/1084801/diff/28001/29022#newcode13 plugins/npapi/manifest-template.json:13: key: MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQDi6RrEy9YllRLM8bGBcIEk5ECAG2z+8ngTz7wwzRAQJpOzDp1Alq8fQFjH0+dzxok4RFLrWKHjxGqvXzWyWyTEo2nY3ScHLN/RoANMs8pl9X6TygRyO+3naqZOtLCrYHfV49JKXnYoFVbY5eBVYxHYY3BHAOKJj9onyAM4UPmMzQIDAQAB, note, this is the public key. since the extension URI is derived from the signature, embedding the public key here lets any user develop and load the unpacked version without having the actual key. we use this same trick in speedtracer. http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006#newcode5 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml:5: inherits name='com.google.gwt.dom.DOM' / User should automatically include DOM Core. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006#newcode12 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml:12: inherits name='com.google.gwt.user.theme.standard.Standard'/ Are you using the Standard styles? If not, you can remove this. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006#newcode13 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml:13: !-- inherits name='com.google.gwt.user.theme.chrome.Chrome'/ -- Should be able to remove these comments. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7007 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/DevModeOptions.java (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7007#newcode36 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/DevModeOptions.java:36: StyleInjector.inject(bundle.css().getText(), true); Minor thing, but if you inject this in onModuleLoad the compiler will not have to defensively call the static initializer on method invocations. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013#newcode1 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css:1: @def TEXTWIDTH 30em; Throw a copyright up here ^ http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013#newcode3 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css:3: font-weight: bold; I think your eclipse setup is putting either tabs or too many spaces for the css indentation. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013#newcode17 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css:17: color: IndianRed; IndianRed! OMG, you're so unix! http://gwt-code-reviews.appspot.com/1084801/diff/6001/7015 File plugins/npapi/DevModeOptions/war/WEB-INF/web.xml (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7015#newcode7 plugins/npapi/DevModeOptions/war/WEB-INF/web.xml:7: !-- TODO: Add servlet tags for each servlet here. -- Use officially sanctioned TODO style: TODO(conroy): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7016 File plugins/npapi/Makefile (left): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7016#oldcode127 plugins/npapi/Makefile:127: ($(foreach src,$(SRCS),$(DEPEND)) true) Makefile Wow, I don't even know what this does. Next time I have Makefile questions, I'm coming to you. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018 File plugins/npapi/ScriptableInstance.cpp (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018#newcode270 plugins/npapi/ScriptableInstance.cpp:270: NPN_GetProperty(getNPP(), window, locationID, locationVariant.addressForReturn()); You should use NPN_GetValue with NPNVWindowNPObject to get the window object and avoid using NPN_GetProperty. Technically, the window property is const, but there shouldn't be any reason to rely on the property when we can get the object we need directly from the context. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018#newcode338 plugins/npapi/ScriptableInstance.cpp:338: strncpy(out, retStr.c_str(), retStr.size()); c_str is null terminated, so you should be able to safely copy size() + 1. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7024 File plugins/npapi/prebuilt/gwt-dev-plugin/options.html (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7024#newcode1 plugins/npapi/prebuilt/gwt-dev-plugin/options.html:1: !doctype html Do you even use this page? http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
Oh, do you have a screenshot of the UI elements? On 2010/11/08 19:27:30, knorton wrote: http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006#newcode5 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml:5: inherits name='com.google.gwt.dom.DOM' / User should automatically include DOM Core. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006#newcode12 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml:12: inherits name='com.google.gwt.user.theme.standard.Standard'/ Are you using the Standard styles? If not, you can remove this. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006#newcode13 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml:13: !-- inherits name='com.google.gwt.user.theme.chrome.Chrome'/ -- Should be able to remove these comments. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7007 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/DevModeOptions.java (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7007#newcode36 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/DevModeOptions.java:36: StyleInjector.inject(bundle.css().getText(), true); Minor thing, but if you inject this in onModuleLoad the compiler will not have to defensively call the static initializer on method invocations. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013#newcode1 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css:1: @def TEXTWIDTH 30em; Throw a copyright up here ^ http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013#newcode3 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css:3: font-weight: bold; I think your eclipse setup is putting either tabs or too many spaces for the css indentation. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013#newcode17 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css:17: color: IndianRed; IndianRed! OMG, you're so unix! http://gwt-code-reviews.appspot.com/1084801/diff/6001/7015 File plugins/npapi/DevModeOptions/war/WEB-INF/web.xml (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7015#newcode7 plugins/npapi/DevModeOptions/war/WEB-INF/web.xml:7: !-- TODO: Add servlet tags for each servlet here. -- Use officially sanctioned TODO style: TODO(conroy): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7016 File plugins/npapi/Makefile (left): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7016#oldcode127 plugins/npapi/Makefile:127: ($(foreach src,$(SRCS),$(DEPEND)) true) Makefile Wow, I don't even know what this does. Next time I have Makefile questions, I'm coming to you. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018 File plugins/npapi/ScriptableInstance.cpp (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018#newcode270 plugins/npapi/ScriptableInstance.cpp:270: NPN_GetProperty(getNPP(), window, locationID, locationVariant.addressForReturn()); You should use NPN_GetValue with NPNVWindowNPObject to get the window object and avoid using NPN_GetProperty. Technically, the window property is const, but there shouldn't be any reason to rely on the property when we can get the object we need directly from the context. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018#newcode338 plugins/npapi/ScriptableInstance.cpp:338: strncpy(out, retStr.c_str(), retStr.size()); c_str is null terminated, so you should be able to safely copy size() + 1. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7024 File plugins/npapi/prebuilt/gwt-dev-plugin/options.html (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7024#newcode1 plugins/npapi/prebuilt/gwt-dev-plugin/options.html:1: !doctype html Do you even use this page? http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018
File plugins/npapi/ScriptableInstance.cpp (right):
http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018#newcode337
plugins/npapi/ScriptableInstance.cpp:337: char* out =
(char*)NPN_MemAlloc(retStr.size() + 1);
Instead use:
NPVariantProxy(*this, *result) = retStr;
or at least:
STDSTRING_TO_NPVARIANT(retStr, result);
NPVariantProxy was written to make sure that refcounting/and memory
ownership was done properly. In this case, since it is only ever a
string it wouldn't be too bad to just use the macro directly.
I believe what you are doing below would actually fail if there are
non-ASCII characters in the string.
http://gwt-code-reviews.appspot.com/1084801/diff/6001/7022
File plugins/npapi/prebuilt/gwt-dev-plugin/background.html (right):
http://gwt-code-reviews.appspot.com/1084801/diff/6001/7022#newcode34
plugins/npapi/prebuilt/gwt-dev-plugin/background.html:34: function
devModeTabListener(tabId, changeInfo, tab) {
Have you verified that the tab ID is constant across refreshes or
selecting a bookmark in a given tab?
http://gwt-code-reviews.appspot.com/1084801/diff/6001/7029
File plugins/platform/Win/Preferences.cpp (left):
http://gwt-code-reviews.appspot.com/1084801/diff/6001/7029#oldcode34
plugins/platform/Win/Preferences.cpp:34: static char* getAccessList(HKEY
keyHandle) {
Do we care about migrating existing user's preferences?
Ie, should we keep a variant of this code for a release, and when it is
first run on Windows it loads all the preferences from the registry into
the Chrome local storage?
http://gwt-code-reviews.appspot.com/1084801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
patch updated per knorton's comments. will address jat's comments in the next round. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006#newcode5 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml:5: inherits name='com.google.gwt.dom.DOM' / On 2010/11/08 19:27:30, knorton wrote: User should automatically include DOM Core. fixed. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006#newcode12 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml:12: inherits name='com.google.gwt.user.theme.standard.Standard'/ On 2010/11/08 19:27:30, knorton wrote: Are you using the Standard styles? If not, you can remove this. fixed. (added font-size: small to the mainPanel's css) http://gwt-code-reviews.appspot.com/1084801/diff/6001/7006#newcode13 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/DevModeOptions.gwt.xml:13: !-- inherits name='com.google.gwt.user.theme.chrome.Chrome'/ -- On 2010/11/08 19:27:30, knorton wrote: Should be able to remove these comments. done. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7007 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/DevModeOptions.java (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7007#newcode36 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/DevModeOptions.java:36: StyleInjector.inject(bundle.css().getText(), true); On 2010/11/08 19:27:30, knorton wrote: Minor thing, but if you inject this in onModuleLoad the compiler will not have to defensively call the static initializer on method invocations. done. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013 File plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013#newcode1 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css:1: @def TEXTWIDTH 30em; On 2010/11/08 19:27:30, knorton wrote: Throw a copyright up here ^ I need it on pretty much all the new files actually. Good catch. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7013#newcode3 plugins/npapi/DevModeOptions/src/com/google/gwt/devmodeoptions/client/resources/DevModeOptions.css:3: font-weight: bold; On 2010/11/08 19:27:30, knorton wrote: I think your eclipse setup is putting either tabs or too many spaces for the css indentation. fixed. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7015 File plugins/npapi/DevModeOptions/war/WEB-INF/web.xml (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7015#newcode7 plugins/npapi/DevModeOptions/war/WEB-INF/web.xml:7: !-- TODO: Add servlet tags for each servlet here. -- On 2010/11/08 19:27:30, knorton wrote: Use officially sanctioned TODO style: TODO(conroy): WebAppCreator made these TODO's for me :P. I'll just remove them since the only reason the web.xml is there is for debugging this thing. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7016 File plugins/npapi/Makefile (left): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7016#oldcode127 plugins/npapi/Makefile:127: ($(foreach src,$(SRCS),$(DEPEND)) true) Makefile On 2010/11/08 19:27:30, knorton wrote: Wow, I don't even know what this does. Next time I have Makefile questions, I'm coming to you. Please don't! I'm just adding to this file, this isn't new! IMHO Makefiles shouldn't be written by humans. CMake ftw. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018 File plugins/npapi/ScriptableInstance.cpp (right): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018#newcode270 plugins/npapi/ScriptableInstance.cpp:270: NPN_GetProperty(getNPP(), window, locationID, locationVariant.addressForReturn()); On 2010/11/08 19:27:30, knorton wrote: You should use NPN_GetValue with NPNVWindowNPObject to get the window object and avoid using NPN_GetProperty. Technically, the window property is const, but there shouldn't be any reason to rely on the property when we can get the object we need directly from the context. that comment is wrong, but i think the code is right. i'm getting the location property from the window object which is set in the constructor via NPN_GetValue(npp, NPNVWindowNPObject, window) like you suggest. i'll update the commnent to refer to window.location http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018#newcode338 plugins/npapi/ScriptableInstance.cpp:338: strncpy(out, retStr.c_str(), retStr.size()); On 2010/11/08 19:27:30, knorton wrote: c_str is null terminated, so you should be able to safely copy size() + 1. you sir, are correct. http://gwt-code-reviews.appspot.com/1084801/diff/6001/7024 File plugins/npapi/prebuilt/gwt-dev-plugin/options.html (right):
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
scriptableinstance updated per jat's comments. responses below.
http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018
File plugins/npapi/ScriptableInstance.cpp (right):
http://gwt-code-reviews.appspot.com/1084801/diff/6001/7018#newcode337
plugins/npapi/ScriptableInstance.cpp:337: char* out =
(char*)NPN_MemAlloc(retStr.size() + 1);
On 2010/11/08 20:00:00, jat wrote:
Instead use:
NPVariantProxy(*this, *result) = retStr;
or at least:
STDSTRING_TO_NPVARIANT(retStr, result);
NPVariantProxy was written to make sure that refcounting/and memory
ownership
was done properly. In this case, since it is only ever a string it
wouldn't be
too bad to just use the macro directly.
I believe what you are doing below would actually fail if there are
non-ASCII
characters in the string.
done.
http://gwt-code-reviews.appspot.com/1084801/diff/6001/7022
File plugins/npapi/prebuilt/gwt-dev-plugin/background.html (right):
http://gwt-code-reviews.appspot.com/1084801/diff/6001/7022#newcode34
plugins/npapi/prebuilt/gwt-dev-plugin/background.html:34: function
devModeTabListener(tabId, changeInfo, tab) {
On 2010/11/08 20:00:00, jat wrote:
Have you verified that the tab ID is constant across refreshes or
selecting a
bookmark in a given tab?
In my testing, it is constant for a given tab. Though, it doesn't really
matter: the listener checks whenever a tab's URL is updated--regardless
of the tabId.
Note that this is purely informational: no permissions changes result
from this pageAction.
http://gwt-code-reviews.appspot.com/1084801/diff/6001/7029
File plugins/platform/Win/Preferences.cpp (left):
http://gwt-code-reviews.appspot.com/1084801/diff/6001/7029#oldcode34
plugins/platform/Win/Preferences.cpp:34: static char* getAccessList(HKEY
keyHandle) {
On 2010/11/08 20:00:00, jat wrote:
Do we care about migrating existing user's preferences?
Ie, should we keep a variant of this code for a release, and when it
is first
run on Windows it loads all the preferences from the registry into the
Chrome
local storage?
I would vote no. I'd rather just move wholesale to the unified
localstorage model. It's easy enough for the user to add them back in.
Another conversation we may want to have is whether or not the rules
will ever support actual patterns. As it is right now, it's an exact
string match which makes the whole notion of exclusion a bit silly.
http://gwt-code-reviews.appspot.com/1084801/show
--
http://groups.google.com/group/Google-Web-Toolkit-Contributors
[gwt-contrib] Re: Add a permissions model to the Chrome NPAPI plugin. (issue1084801)
http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
