Replacing WebUIBindings use of CPPBoundClass with v8::Extension.

Replacing WebUIBindings use of CPPBoundClass with v8::Extension.

Currently WebUIBindings replace the entire chrome.* js  namespace which will be fixed by this CL.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=169747

[abarth-chromium, 2012-08-23 20:13:24]

https://chromiumcodereview.appspot.com/10873038/diff/1/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10873038/diff/1/content/renderer/rende...
content/renderer/render_view_impl.cc:4650: if (enabled_bindings_ &
content::BINDINGS_POLICY_WEB_UI) {
What happened to the check for chrome::kChromeUIScheme and
chrome::kChromeUIScheme ?  Is that done elsewhere?

Also, previously this was done on a per-frame basis.  This appears to be a
per-view basis.  Is that a problem?

https://chromiumcodereview.appspot.com/10873038/diff/1/content/renderer/web_u...
File content/renderer/web_ui_extension.cc (right):

https://chromiumcodereview.appspot.com/10873038/diff/1/content/renderer/web_u...
content/renderer/web_ui_extension.cc:27: }
We must already have this code elsewhere.  Can we share it?

https://chromiumcodereview.appspot.com/10873038/diff/1/content/renderer/web_u...
content/renderer/web_ui_extension.cc:54: }
ditto

https://chromiumcodereview.appspot.com/10873038/diff/1/content/renderer/web_u...
content/renderer/web_ui_extension.cc:111:
frame_url.SchemeIs(chrome::kDataScheme));
I see, the check went here.  That's not as good because it means the function
will be visible in JS---it just won't do anything.  Maybe that's ok though.  I'm
not sure which renderviews have BINDINGS_POLICY_WEB_UI

[Shishir, 2012-11-06 20:08:38]

Sorry this became de-prioritized for a bit.

Adding:
piman for content/renderer changes.
estade for webui changes.

PTAL.

http://codereview.chromium.org/10873038/diff/1/content/renderer/render_view_i...
File content/renderer/render_view_impl.cc (right):

http://codereview.chromium.org/10873038/diff/1/content/renderer/render_view_i...
content/renderer/render_view_impl.cc:4650: if (enabled_bindings_ &
content::BINDINGS_POLICY_WEB_UI) {
On 2012/08/23 20:13:24, abarth wrote:
> What happened to the check for chrome::kChromeUIScheme and
> chrome::kChromeUIScheme ?  Is that done elsewhere?

Checks are now in the extension during the chrome.send call.

> 
> Also, previously this was done on a per-frame basis.  This appears to be a
> per-view basis.  Is that a problem?

Fixed this. Now the data is available in all the frames.

http://codereview.chromium.org/10873038/diff/1/content/renderer/web_ui_extens...
File content/renderer/web_ui_extension.cc (right):

http://codereview.chromium.org/10873038/diff/1/content/renderer/web_ui_extens...
content/renderer/web_ui_extension.cc:27: }
On 2012/08/23 20:13:24, abarth wrote:
> We must already have this code elsewhere.  Can we share it?

I cant find a common libaray that has these. I had created one in
content/common, but jam felt it was unnecessary to add this.

http://codereview.chromium.org/10873038/diff/1/content/renderer/web_ui_extens...
content/renderer/web_ui_extension.cc:54: }
On 2012/08/23 20:13:24, abarth wrote:
> ditto

Found a converter that does this and replaced this code.

http://codereview.chromium.org/10873038/diff/1/content/renderer/web_ui_extens...
content/renderer/web_ui_extension.cc:111:
frame_url.SchemeIs(chrome::kDataScheme));
On 2012/08/23 20:13:24, abarth wrote:
> I see, the check went here.  That's not as good because it means the function
> will be visible in JS---it just won't do anything.  Maybe that's ok though. 
I'm
> not sure which renderviews have BINDINGS_POLICY_WEB_UI

The webuis all have this along with one exception I found - the ballonhost
stuff. looking at the definition of BINDINGS_POLICY_WEB_UI in
content/public/common/bindings_policy.h, it says 'HTML-based UI bindings that
allows the JS content to send JSON-encoded data back to the browser process.' So
I am not sure why the other schemeis checks are required. 

I am also not sure if we can expose the chrome.send eventhough it will not do
anything. Will let anyone else chime in.

[piman, 2012-11-07 00:02:41]

LGTM for renderer, modulo abarth's or some security person to look at it to
verify the origin checks.

[Evan Stade, 2012-11-07 00:33:54]

http://codereview.chromium.org/10873038/diff/5001/content/renderer/web_ui_ext...
File content/renderer/web_ui_extension_data.h (right):

http://codereview.chromium.org/10873038/diff/5001/content/renderer/web_ui_ext...
content/renderer/web_ui_extension_data.h:35: std::map<string16, string16>
variable_map_;
why is this UTF16 instead of UTF8? seems v8 can accept either one, and it's
easier to leave this as utf8 since that's the format OnSetWebUIProperty expects.

[Shishir, 2012-11-07 01:53:29]

PTAL.

http://codereview.chromium.org/10873038/diff/5001/content/renderer/web_ui_ext...
File content/renderer/web_ui_extension_data.h (right):

http://codereview.chromium.org/10873038/diff/5001/content/renderer/web_ui_ext...
content/renderer/web_ui_extension_data.h:35: std::map<string16, string16>
variable_map_;
On 2012/11/07 00:33:54, Evan Stade wrote:
> why is this UTF16 instead of UTF8? seems v8 can accept either one, and it's
> easier to leave this as utf8 since that's the format OnSetWebUIProperty
expects.

Changed to utf8. The V8 string objects seem string16 based so I initially wrote
this to be consistent with v8. But it makes sense to have this compatible with
SetWebUI calls.

[Evan Stade, 2012-11-07 23:09:39]

http://codereview.chromium.org/10873038/diff/5003/content/renderer/web_ui_ext...
File content/renderer/web_ui_extension.cc (right):

http://codereview.chromium.org/10873038/diff/5003/content/renderer/web_ui_ext...
content/renderer/web_ui_extension.cc:42: static const char* const
kWebUIExtensionJS =
document

http://codereview.chromium.org/10873038/diff/5003/content/renderer/web_ui_ext...
content/renderer/web_ui_extension.cc:54: 
there's an extra newline here

http://codereview.chromium.org/10873038/diff/5003/content/renderer/web_ui_ext...
content/renderer/web_ui_extension.cc:74: : v8::Extension(kWebUIExtensionName,
kWebUIExtensionJS) { }
nit: {}

http://codereview.chromium.org/10873038/diff/5003/content/renderer/web_ui_ext...
content/renderer/web_ui_extension.cc:80: if
(name->Equals(v8::String::New("Send"))) {
nit: don't think these conditions need curlies... Chrome's not super consistent
on this but I think it tends towards leaving them out. Up to you.

http://codereview.chromium.org/10873038/diff/5003/content/renderer/web_ui_ext...
content/renderer/web_ui_extension.cc:142:
CHECK(content->IsType(Value::TYPE_LIST));
imo, use GetAsListValue, make content a scoped_ptr<ListValue>, and avoid the
cast below.

http://codereview.chromium.org/10873038/diff/5003/content/renderer/web_ui_ext...
content/renderer/web_ui_extension.cc:167:
WebUIExtensionData::Get(render_view)->GetValue(key)));
can't you use v8::String::New on utf8?

[Shishir, 2012-11-08 00:10:38]

PTAL.

http://codereview.chromium.org/10873038/diff/5003/content/renderer/web_ui_ext...
File content/renderer/web_ui_extension.cc (right):

http://codereview.chromium.org/10873038/diff/5003/content/renderer/web_ui_ext...
content/renderer/web_ui_extension.cc:42: static const char* const
kWebUIExtensionJS =
On 2012/11/07 23:09:39, Evan Stade wrote:
> document

Done.

http://codereview.chromium.org/10873038/diff/5003/content/renderer/web_ui_ext...
content/renderer/web_ui_extension.cc:54: 
On 2012/11/07 23:09:39, Evan Stade wrote:
> there's an extra newline here

Done.

http://codereview.chromium.org/10873038/diff/5003/content/renderer/web_ui_ext...
content/renderer/web_ui_extension.cc:74: : v8::Extension(kWebUIExtensionName,
kWebUIExtensionJS) { }
On 2012/11/07 23:09:39, Evan Stade wrote:
> nit: {}

Done.

http://codereview.chromium.org/10873038/diff/5003/content/renderer/web_ui_ext...
content/renderer/web_ui_extension.cc:80: if
(name->Equals(v8::String::New("Send"))) {
On 2012/11/07 23:09:39, Evan Stade wrote:
> nit: don't think these conditions need curlies... Chrome's not super
consistent
> on this but I think it tends towards leaving them out. Up to you.

Done.

http://codereview.chromium.org/10873038/diff/5003/content/renderer/web_ui_ext...
content/renderer/web_ui_extension.cc:142:
CHECK(content->IsType(Value::TYPE_LIST));
On 2012/11/07 23:09:39, Evan Stade wrote:
> imo, use GetAsListValue, make content a scoped_ptr<ListValue>, and avoid the
> cast below.

Done.

http://codereview.chromium.org/10873038/diff/5003/content/renderer/web_ui_ext...
content/renderer/web_ui_extension.cc:167:
WebUIExtensionData::Get(render_view)->GetValue(key)));
On 2012/11/07 23:09:39, Evan Stade wrote:
> can't you use v8::String::New on utf8?

Done.

[Evan Stade, 2012-11-08 01:37:59]

lgtm

http://codereview.chromium.org/10873038/diff/14022/content/renderer/web_ui_ex...
File content/renderer/web_ui_extension.cc (right):

http://codereview.chromium.org/10873038/diff/14022/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.cc:40: //      name as the first argument and
can have other optional arguments.
nit: there is only one other argument, which is an array of parameters. This is
different than having a variable-length parameter list (which is what this
documentation makes it sound like).

[abarth-chromium, 2012-11-09 18:19:43]

https://codereview.chromium.org/10873038/diff/14022/content/renderer/web_ui_e...
File content/renderer/web_ui_extension.cc (right):

https://codereview.chromium.org/10873038/diff/14022/content/renderer/web_ui_e...
content/renderer/web_ui_extension.cc:28: return
UTF16ToUTF8(string16(reinterpret_cast<const char16*>(*s), s.length()));
I don't think this is quite right.  V8 strings are UCS2, not UTF16.  We don't
have a helper function for this elsewhere?

https://codereview.chromium.org/10873038/diff/14022/content/renderer/web_ui_e...
content/renderer/web_ui_extension.cc:89: bool
WebUIExtensionWrapper::ShouldRespondToRequest(
It looks like the main difference is now we're exposing these APIs all the time,
but we're ignoring the calls if the WebFrame doesn't have the proper privilege.

It would be better to only expose the APIs to Frames that have the proper
privileges in the first place...  For example, we only expose extension APIs to
extensions, not to the entire web.

[Shishir, 2012-11-09 19:20:08]

PTAL.

http://codereview.chromium.org/10873038/diff/14022/content/renderer/web_ui_ex...
File content/renderer/web_ui_extension.cc (right):

http://codereview.chromium.org/10873038/diff/14022/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.cc:28: return
UTF16ToUTF8(string16(reinterpret_cast<const char16*>(*s), s.length()));
On 2012/11/09 18:19:43, abarth wrote:
> I don't think this is quite right.  V8 strings are UCS2, not UTF16.  We don't
> have a helper function for this elsewhere?

Done.

http://codereview.chromium.org/10873038/diff/14022/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.cc:40: //      name as the first argument and
can have other optional arguments.
On 2012/11/08 01:37:59, Evan Stade wrote:
> nit: there is only one other argument, which is an array of parameters. This
is
> different than having a variable-length parameter list (which is what this
> documentation makes it sound like).

Done.

http://codereview.chromium.org/10873038/diff/14022/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.cc:89: bool
WebUIExtensionWrapper::ShouldRespondToRequest(
On 2012/11/09 18:19:43, abarth wrote:
> It looks like the main difference is now we're exposing these APIs all the
time,
> but we're ignoring the calls if the WebFrame doesn't have the proper
privilege.
> 
> It would be better to only expose the APIs to Frames that have the proper
> privileges in the first place...  For example, we only expose extension APIs
to
> extensions, not to the entire web.

Done.

[Evan Stade, 2012-11-09 22:53:27]

http://codereview.chromium.org/10873038/diff/15023/content/renderer/web_ui_ex...
File content/renderer/web_ui_extension.cc (right):

http://codereview.chromium.org/10873038/diff/15023/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.cc:158: const std::string& value =
the const& doesn't do anything here because GetValue() doesn't return a
reference.

[Shishir, 2012-11-09 22:58:55]

PTAL.

http://codereview.chromium.org/10873038/diff/15023/content/renderer/web_ui_ex...
File content/renderer/web_ui_extension.cc (right):

http://codereview.chromium.org/10873038/diff/15023/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.cc:158: const std::string& value =
On 2012/11/09 22:53:27, Evan Stade wrote:
> the const& doesn't do anything here because GetValue() doesn't return a
> reference.

Done.

[abarth-chromium, 2012-11-09 23:07:44]

LGTM

https://codereview.chromium.org/10873038/diff/13010/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/10873038/diff/13010/content/renderer/render_v...
content/renderer/render_view_impl.cc:5004: void
RenderViewImpl::OnAllowBindings(int enabled_bindings_flags) {
I take it there isn't a way to remove bindings.

[Shishir, 2012-11-09 23:47:47]

https://codereview.chromium.org/10873038/diff/13010/content/renderer/render_v...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/10873038/diff/13010/content/renderer/render_v...
content/renderer/render_view_impl.cc:5004: void
RenderViewImpl::OnAllowBindings(int enabled_bindings_flags) {
On 2012/11/09 23:07:44, abarth wrote:
> I take it there isn't a way to remove bindings.

Not that I could find. I think the way to unregister an extension would be to
register a extension that removes the previous bindings. I think it might be
safer to add the bindings selectively than to remove it.

[commit-bot: I haz the power, 2012-11-12 18:51:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/10873038/13010

[commit-bot: I haz the power, 2012-11-12 18:51:59]

Presubmit check for 10873038-13010 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Messages **
If this change has an associated bug, add BUG=[bug number].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/sync
    chrome/common/extensions/docs

Presubmit checks took 2.7s to calculate.

[Shishir, 2012-11-12 18:58:56]

Adding 

atwilson for chrome/browser/sync/...
cduvall for chrome/common/extensions/docs/...

PTAL.

[sreeram, 2012-11-12 19:42:44]

http://codereview.chromium.org/10873038/diff/14022/content/renderer/web_ui_ex...
File content/renderer/web_ui_extension.cc (right):

http://codereview.chromium.org/10873038/diff/14022/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.cc:28: return
UTF16ToUTF8(string16(reinterpret_cast<const char16*>(*s), s.length()));
On 2012/11/09 19:20:08, Shishir wrote:
> On 2012/11/09 18:19:43, abarth wrote:
> > I don't think this is quite right.  V8 strings are UCS2, not UTF16.  We
don't
> > have a helper function for this elsewhere?
> 
> Done.

For the record, I think the earlier code was fine (though the new code which
uses v8::String::Utf8Value() is much cleaner).

JavaScript is UCS-2, sure. But the internal representation is still 16-bit code
points, which is the same as UTF-16, so the reinterpret_cast and subsequent
conversion will work fine (cf:
http://mathiasbynens.be/notes/javascript-encoding).

[cduvall, 2012-11-12 22:19:06]

c/c/e/docs/ lgtm

[Andrew T Wilson (Slow), 2012-11-13 08:35:49]

browser/sync lgtm

[commit-bot: I haz the power, 2012-11-13 18:31:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/10873038/13010

[commit-bot: I haz the power, 2012-11-13 18:39:52]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[sreeram, 2012-11-14 17:18:00]

LGTM with nits

third_party/trace-viewer/src/base.js contains references to chrome.toolkit.
Could you file a bug or upstream a change to
http://code.google.com/p/trace-viewer/?

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
File content/renderer/web_ui_extension.cc (right):

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.cc:72: else if
(name->Equals(v8::String::New("GetVariableValue")))
No need for "else". Just:
  if (...)
    return ...;
  if (...)
    return ...;
  return ...;

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.cc:146: v8::Handle<v8::Value>
WebUIExtensionWrapper::GetVariableValue(
Add "// static" on top.

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.cc:161: v8::Extension* WebUIExtension::Get() {
Add "// static" on top.

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
File content/renderer/web_ui_extension.h (right):

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
No "(c)" here and in other newly added files.

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.h:6: #define
CONTENT_RENDERER_EXTENSIONS_WEBUI_EXTENSION_H_
CONTENT_RENDERER_WEB_UI_EXTENSION_H_ (also on line 29 below).

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.h:23: 
No blank line here.

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.h:24:
DISALLOW_COPY_AND_ASSIGN(WebUIExtension);
Use DISALLOW_IMPLICIT_CONSTRUCTORS instead.

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
File content/renderer/web_ui_extension_data.cc (right):

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension_data.cc:24: return "";
return std::string();

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
File content/renderer/web_ui_extension_data.h (right):

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension_data.h:6: #define
CONTENT_RENDERER_EXTENSIONS_WEBUI_EXTENSION_DATA_H_
CONTENT_RENDERER_WEB_UI_EXTENSION_DATA_H_ (also on line 41)

[Shishir, 2012-11-14 19:32:34]

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
File content/renderer/web_ui_extension.cc (right):

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.cc:72: else if
(name->Equals(v8::String::New("GetVariableValue")))
On 2012/11/14 17:18:01, sreeram wrote:
> No need for "else". Just:
>   if (...)
>     return ...;
>   if (...)
>     return ...;
>   return ...;

Done.

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.cc:146: v8::Handle<v8::Value>
WebUIExtensionWrapper::GetVariableValue(
On 2012/11/14 17:18:01, sreeram wrote:
> Add "// static" on top.

Done.

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.cc:161: v8::Extension* WebUIExtension::Get() {
On 2012/11/14 17:18:01, sreeram wrote:
> Add "// static" on top.

Done.

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
File content/renderer/web_ui_extension.h (right):

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/11/14 17:18:01, sreeram wrote:
> No "(c)" here and in other newly added files.

Done.

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.h:6: #define
CONTENT_RENDERER_EXTENSIONS_WEBUI_EXTENSION_H_
On 2012/11/14 17:18:01, sreeram wrote:
> CONTENT_RENDERER_WEB_UI_EXTENSION_H_ (also on line 29 below).

Done.

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.h:23: 
On 2012/11/14 17:18:01, sreeram wrote:
> No blank line here.

Done.

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension.h:24:
DISALLOW_COPY_AND_ASSIGN(WebUIExtension);
On 2012/11/14 17:18:01, sreeram wrote:
> Use DISALLOW_IMPLICIT_CONSTRUCTORS instead.

Done.

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
File content/renderer/web_ui_extension_data.cc (right):

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension_data.cc:24: return "";
On 2012/11/14 17:18:01, sreeram wrote:
> return std::string();

Done.

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
File content/renderer/web_ui_extension_data.h (right):

http://codereview.chromium.org/10873038/diff/13010/content/renderer/web_ui_ex...
content/renderer/web_ui_extension_data.h:6: #define
CONTENT_RENDERER_EXTENSIONS_WEBUI_EXTENSION_DATA_H_
On 2012/11/14 17:18:01, sreeram wrote:
> CONTENT_RENDERER_WEB_UI_EXTENSION_DATA_H_ (also on line 41)

Done.

[commit-bot: I haz the power, 2012-11-14 19:37:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/10873038/19010

[commit-bot: I haz the power, 2012-11-14 20:52:21]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-11-15 19:19:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/10873038/18025

[commit-bot: I haz the power, 2012-11-15 21:48:23]

Retried try job too often for step(s) browser_tests

[sreeram, 2012-11-26 10:13:02]

On 2012/11/15 21:48:23, I haz the power (commit-bot) wrote:
> Retried try job too often for step(s) browser_tests

The SelectFileDialogExtensionBrowserTest tests are failing because of the
following error:
[12762:12762:1126/014151:921914704173:INFO:select_file_dialog_extension_browsertest.cc(146)]
Waiting for JavaScript ready message.
[12762:12762:1126/014152:921915600736:INFO:CONSOLE(1015)] "Uncaught TypeError:
Object #<Object> has no method 'getVariableValue'", source:
chrome-extension://hhaomjibdihmijegdhdafkllkbggdgoj/js/main_scripts.js (1015)

The "hhao..." extension is a whitelisted extension (which is allowed to access
chrome.send() and such APIs). The main_scripts.js file
(chrome/browser/resources/file_manager/js/main_scripts.js) just includes all the
webui JS.

So I guess you need to figure out why RenderViewImpl::didClearWindowObject()
allowed access in the previous code, but the new code's combination of
RenderViewImpl::OnAllowBindings() and
WebUIExtensionWrapper::ShouldRespondToRequest() deny access.

[Shishir, 2012-11-26 21:25:01]

On 2012/11/26 10:13:02, sreeram wrote:
> On 2012/11/15 21:48:23, I haz the power (commit-bot) wrote:
> > Retried try job too often for step(s) browser_tests
> 
> The SelectFileDialogExtensionBrowserTest tests are failing because of the
> following error:
>
[12762:12762:1126/014151:921914704173:INFO:select_file_dialog_extension_browsertest.cc(146)]
> Waiting for JavaScript ready message.
> [12762:12762:1126/014152:921915600736:INFO:CONSOLE(1015)] "Uncaught TypeError:
> Object #<Object> has no method 'getVariableValue'", source:
> chrome-extension://hhaomjibdihmijegdhdafkllkbggdgoj/js/main_scripts.js (1015)
> 
> The "hhao..." extension is a whitelisted extension (which is allowed to access
> chrome.send() and such APIs). The main_scripts.js file
> (chrome/browser/resources/file_manager/js/main_scripts.js) just includes all
the
> webui JS.
> 
> So I guess you need to figure out why RenderViewImpl::didClearWindowObject()
> allowed access in the previous code, but the new code's combination of
> RenderViewImpl::OnAllowBindings() and
> WebUIExtensionWrapper::ShouldRespondToRequest() deny access.

It was a JS issue:

/GTK/.test(chrome.toolkit) will return false when chrome.toolkit is not defined,
but

/GTK/.test(chrome.getVariableValue('toolkit')); will throw an exception when
chrome.getVariableValue is not defined.

Fixed. PTAL.

[sreeram, 2012-11-26 21:34:47]

On 2012/11/26 21:25:01, Shishir wrote:
> It was a JS issue:
> 
> /GTK/.test(chrome.toolkit) will return false when chrome.toolkit is not
defined,
> but
> 
> /GTK/.test(chrome.getVariableValue('toolkit')); will throw an exception when
> chrome.getVariableValue is not defined.
> 
> Fixed. PTAL.

Nice catch. Could you try using typeof foo == 'function' instead, instead of "!=
'undefined'"?

[Shishir, 2012-11-26 21:40:50]

On 2012/11/26 21:34:47, sreeram wrote:
> On 2012/11/26 21:25:01, Shishir wrote:
> > It was a JS issue:
> > 
> > /GTK/.test(chrome.toolkit) will return false when chrome.toolkit is not
> defined,
> > but
> > 
> > /GTK/.test(chrome.getVariableValue('toolkit')); will throw an exception when
> > chrome.getVariableValue is not defined.
> > 
> > Fixed. PTAL.
> 
> Nice catch. Could you try using typeof foo == 'function' instead, instead of
"!=
> 'undefined'"?

Done. PTAL.

[sreeram, 2012-11-26 21:46:08]

lgtm

[commit-bot: I haz the power, 2012-11-26 22:54:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/10873038/29005

[commit-bot: I haz the power, 2012-11-27 21:00:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shishir@chromium.org/10873038/29005

[commit-bot: I haz the power, 2012-11-27 21:03:23]

Change committed as 169747