Implement WebUI extension via gin instead of v8::Extension

Implement WebUI extension via gin instead of v8::Extension

BUG=334679
R=dcarney@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=251626

[jochen (gone - plz use gerrit), 2014-02-13 15:02:44]

+arv

[arv (Not doing code reviews), 2014-02-13 15:34:51]

https://codereview.chromium.org/163683004/diff/1/content/renderer/web_ui_exte...
File content/renderer/web_ui_extension.cc (right):

https://codereview.chromium.org/163683004/diff/1/content/renderer/web_ui_exte...
content/renderer/web_ui_extension.cc:77:
global->Set(gin::StringToSymbol(isolate, "chrome"), chrome);
OT, Can we rename StringToSymbol before gin gets used too much? It is confusing
with ES6 Symbols. Maybe StringToPropertyName?

https://codereview.chromium.org/163683004/diff/1/content/renderer/web_ui_exte...
content/renderer/web_ui_extension.cc:97: args->ThrowError();
Is there a ThrowNotEnoughArgumentsError? mkwst spent a lot of effort getting rid
of our generic error messages and we should make sure we continue down that
path.

https://codereview.chromium.org/163683004/diff/1/content/renderer/web_ui_exte...
content/renderer/web_ui_extension.cc:104: if (args->PeekNext().IsEmpty()) {
This is change in semantics. `chrome.send('abc')` and `chrome.send('abc',
undefined)` should be treated the same.

https://codereview.chromium.org/163683004/diff/1/content/renderer/web_ui_exte...
content/renderer/web_ui_extension.cc:131: std::string
WebUIExtension::GetVariableValue(const std::string& name) {
Does the gin bindings already handle toString?

chrome.getVariableValue({toString: function() {return 'abc';}})

[jochen (gone - plz use gerrit), 2014-02-13 15:44:23]

https://codereview.chromium.org/163683004/diff/1/content/renderer/web_ui_exte...
File content/renderer/web_ui_extension.cc (right):

https://codereview.chromium.org/163683004/diff/1/content/renderer/web_ui_exte...
content/renderer/web_ui_extension.cc:77:
global->Set(gin::StringToSymbol(isolate, "chrome"), chrome);
On 2014/02/13 15:34:51, arv wrote:
> OT, Can we rename StringToSymbol before gin gets used too much? It is
confusing
> with ES6 Symbols. Maybe StringToPropertyName?

depends a bit on where you're coming from. If you're hacking on bindings, symbol
is not confusing at all :)

https://codereview.chromium.org/163683004/diff/1/content/renderer/web_ui_exte...
content/renderer/web_ui_extension.cc:97: args->ThrowError();
On 2014/02/13 15:34:51, arv wrote:
> Is there a ThrowNotEnoughArgumentsError? mkwst spent a lot of effort getting
rid
> of our generic error messages and we should make sure we continue down that
> path.

gin::Arguments internally keeps track of what went wrong and will throw
something like 'error parsing argument 0'

chrome.send();
TypeError: Insufficient number of arguments.
chrome.send(1);
TypeError: Error processing argument 0.

https://codereview.chromium.org/163683004/diff/1/content/renderer/web_ui_exte...
content/renderer/web_ui_extension.cc:104: if (args->PeekNext().IsEmpty()) {
On 2014/02/13 15:34:51, arv wrote:
> This is change in semantics. `chrome.send('abc')` and `chrome.send('abc',
> undefined)` should be treated the same.

Done.

https://codereview.chromium.org/163683004/diff/1/content/renderer/web_ui_exte...
content/renderer/web_ui_extension.cc:131: std::string
WebUIExtension::GetVariableValue(const std::string& name) {
On 2014/02/13 15:34:51, arv wrote:
> Does the gin bindings already handle toString?
> 
> chrome.getVariableValue({toString: function() {return 'abc';}})

no (but the previous code doesn't handle that either)

[arv (Not doing code reviews), 2014-02-13 15:55:17]

LGTM

gin bindings look really great and makes the code much cleaner too.

https://codereview.chromium.org/163683004/diff/1/content/renderer/web_ui_exte...
File content/renderer/web_ui_extension.cc (right):

https://codereview.chromium.org/163683004/diff/1/content/renderer/web_ui_exte...
content/renderer/web_ui_extension.cc:97: args->ThrowError();
On 2014/02/13 15:44:24, jochen wrote:
> On 2014/02/13 15:34:51, arv wrote:
> > Is there a ThrowNotEnoughArgumentsError? mkwst spent a lot of effort getting
> rid
> > of our generic error messages and we should make sure we continue down that
> > path.
> 
> gin::Arguments internally keeps track of what went wrong and will throw
> something like 'error parsing argument 0'
> 
> chrome.send();
> TypeError: Insufficient number of arguments.
> chrome.send(1);
> TypeError: Error processing argument 0.

Nice.

https://codereview.chromium.org/163683004/diff/1/content/renderer/web_ui_exte...
content/renderer/web_ui_extension.cc:131: std::string
WebUIExtension::GetVariableValue(const std::string& name) {
On 2014/02/13 15:44:24, jochen wrote:
> On 2014/02/13 15:34:51, arv wrote:
> > Does the gin bindings already handle toString?
> > 
> > chrome.getVariableValue({toString: function() {return 'abc';}})
> 
> no (but the previous code doesn't handle that either)

You are right. I got distracted by the ToString call in the old code.

[jochen (gone - plz use gerrit), 2014-02-13 16:37:44]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2014-02-13 16:38:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/163683004/2

[commit-bot: I haz the power, 2014-02-13 16:38:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 16:38:16]

Failed to apply patch for content/renderer/render_view_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/render_view_impl.cc
  Hunk #1 succeeded at 3292 (offset -2 lines).
  Hunk #2 FAILED at 4336.
  1 out of 2 hunks FAILED -- saving rejects to file
content/renderer/render_view_impl.cc.rej

Patch:       content/renderer/render_view_impl.cc
Index: content/renderer/render_view_impl.cc
diff --git a/content/renderer/render_view_impl.cc
b/content/renderer/render_view_impl.cc
index
ef64a94a8d5cf2e4035cbdb22bda5ee8618cb9be..aa1b1f9ccf9e7616c226df4e1fa0fc64e146225b
100644
--- a/content/renderer/render_view_impl.cc
+++ b/content/renderer/render_view_impl.cc
@@ -3294,6 +3294,9 @@ void RenderViewImpl::didClearWindowObject(WebFrame* frame,
int world_id) {
   if (world_id)
     return;
 
+  if (enabled_bindings_& BINDINGS_POLICY_WEB_UI)
+    WebUIExtension::Install(frame);
+
   if (enabled_bindings_ & BINDINGS_POLICY_DOM_AUTOMATION)
     DomAutomationController::Install(this, frame);
 
@@ -4333,8 +4336,6 @@ void RenderViewImpl::OnAllowBindings(int
enabled_bindings_flags) {
       !(enabled_bindings_ & BINDINGS_POLICY_WEB_UI)) {
     // WebUI uses <dialog> which is not yet enabled by default in Chrome.
     WebRuntimeFeatures::enableDialogElement(true);
-
-    RenderThread::Get()->RegisterExtension(WebUIExtension::Get());
     new WebUIExtensionData(this);
   }

[jochen (gone - plz use gerrit), 2014-02-16 09:56:09]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2014-02-16 09:56:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/163683004/210001

[commit-bot: I haz the power, 2014-02-16 10:15:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-16 10:15:26]

Retried try job too often on mac_rel for step(s) app_list_unittests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chromedriver_unittests, components_unittests, content_browsertests,
content_unittests, crypto_unittests, google_apis_unittests, gpu_unittests,
ipc_tests, jingle_unittests, media_unittests, message_center_unittests,
nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests,
sql_unittests, sync_integration_tests, sync_unit_tests,
telemetry_perf_unittests, telemetry_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[jochen (gone - plz use gerrit), 2014-02-16 10:15:59]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2014-02-16 10:16:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/163683004/210001

[commit-bot: I haz the power, 2014-02-16 22:19:46]

Change committed as 251626