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): http://gwt-code-reviews.appspot.com/1084801/diff/6001/7024#newcode1 plugins/npapi/prebuilt/gwt-dev-plugin/options.html:1: <!doctype html> On 2010/11/08 19:27:30, knorton wrote:
Do you even use this page?
haha nope! this was just a rough prototype. baleting... http://gwt-code-reviews.appspot.com/1084801/show -- http://groups.google.com/group/Google-Web-Toolkit-Contributors
