Thanks for the patch Stig. I have only one significant comment. What are the consequences of passing NULL for the JsContext to OpenDialog?
Other than that, I can make the changes and submit once you're happy, so there's no need for you to send a new patch. I'll follow up with my updated patch. ======================================================================== http://mondrian.corp.google.com/file/9447321///depot/googleclient/gears/opensource/gears/ui/opera/html_dialog_op.cc?a=1 File //depot/googleclient/gears/opensource/gears/ui/opera/html_dialog_op.cc (snapshot 1) ------------------------------------ Line 32: #include "gears/base/opera/browsing_context_op.h" Includes should be in alphabetical order. ------------------------------------ Line 36: const char16 *arguments_string) { Arguments on separate lines if they don't all fit on a single line. ------------------------------------ Line 37: bool ret_val = false; This is not used. ------------------------------------ Line 44: = static_cast<OPBrowsingContext*>(browsing_context_); Break after operator= and indent 4 spaces ------------------------------------ Line 45: opera_api->OpenDialog(context ? context->GetJsContext() : NULL, What are the consequences of passing NULL for the JsContext to OpenDialog? If the JsContext isn't required, why do we pass it through to this method? ------------------------------------ Line 56: void *closure) { Indent to open brace if possible. ------------------------------------ Line 63: // TODO: Get the correct locale On WinCE, use GetCurrentSystemLocale() - see html_dialog_ie.cc ======================================================================== -- To respond, reply to this email or visit http://mondrian.corp.google.com/9447321
