Convert DomAutomationController from CppBoundClass to gin::Wrappable

Convert DomAutomationController from CppBoundClass to gin::Wrappable

BUG=297480
R=jam@chromium.org

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

[jochen (gone - plz use gerrit), 2014-01-09 12:53:36]

plz review

[jam, 2014-01-09 18:14:18]

On 2014/01/09 12:53:36, jochen wrote:
> plz review

I trust that the code is right, but since I'm not familiar with gin, the best
reviewer is one who's familiar with it (even if they're not an owner in content,
since you are)

[jochen (gone - plz use gerrit), 2014-01-09 18:18:13]

Aaron, can you have a look plz

[Aaron Boodman, 2014-01-09 20:11:06]

https://codereview.chromium.org/131573003/diff/50001/content/renderer/dom_aut...
File content/renderer/dom_automation_controller.cc (right):

https://codereview.chromium.org/131573003/diff/50001/content/renderer/dom_aut...
content/renderer/dom_automation_controller.cc:68: if
(args->PeekNext()->IsString()) {
Weird API is weird. Why just these three JSON types? Why is there the ability to
send a serialized JSON string in the first place? Oh well.

https://codereview.chromium.org/131573003/diff/50001/content/renderer/dom_aut...
content/renderer/dom_automation_controller.cc:69: std::string str;
You could shorten this a bit by using V8ValueConverter:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...

Check the input is one of the three allowed types, then call the converter. If
people want to allow more JSON types in the future, it will work easily.

https://codereview.chromium.org/131573003/diff/50001/content/renderer/dom_aut...
File content/renderer/dom_automation_controller.h (right):

https://codereview.chromium.org/131573003/diff/50001/content/renderer/dom_aut...
content/renderer/dom_automation_controller.h:89: explicit
DomAutomationController(RenderViewImpl* render_view);
In general, the convention is that constructors are always private and instances
are created via factories that return gin::Handle<WrappableThing>.

https://codereview.chromium.org/131573003/diff/50001/content/renderer/dom_aut...
content/renderer/dom_automation_controller.h:97: bool Send(gin::Arguments*
args);
Did you try const gin::Arguments& ? I thought that worked too, and would be more
stylistically correct.

https://codereview.chromium.org/131573003/diff/50001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/131573003/diff/50001/content/renderer/render_...
content/renderer/render_view_impl.h:1500: // DOM automation bindings are
enabled.
How does RV lifetime relate to frame lifetime, and more specifically this
pointer? Can the JS context shut down making this pointer garbage while RV is
still outstanding?

Either way some comments would be good.

[Aaron Boodman, 2014-01-09 20:19:20]

https://codereview.chromium.org/131573003/diff/50001/content/renderer/render_...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/131573003/diff/50001/content/renderer/render_...
content/renderer/render_view_impl.cc:3665: void
RenderViewImpl::didClearWindowObject(WebFrame* frame) {
Does this get called for every frame in the view, or only the top one? I think
it gets called for every frame.

If so, this code is going to lead to some weird behavior in cases where there
are multiple frames because the one instance of DomAutomationController will be
shared, but will be destroyed when the first frame unloads.

It seems like each frame should have its own instance.

https://codereview.chromium.org/131573003/diff/50001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/131573003/diff/50001/content/renderer/render_...
content/renderer/render_view_impl.h:1500: // DOM automation bindings are
enabled.
On 2014/01/09 20:11:06, Aaron Boodman wrote:
> How does RV lifetime relate to frame lifetime, and more specifically this
> pointer? Can the JS context shut down making this pointer garbage while RV is
> still outstanding?
> 
> Either way some comments would be good.

On second thought, instead of storing this scary looking pointer, what about
stashing DomAutomatonController in JS-land instead, via
frame->context()->global()->SetHiddenValue()?

On the other hand, it looks like this pointer is never used except as a flag as
to whether the controller has been installed already. In that case, maybe we
shouldn't store it at all, but rather actually look in the JS context and see if
it's there. Or store a boolean did_install_ instead?

[jochen (gone - plz use gerrit), 2014-01-09 20:30:37]

https://codereview.chromium.org/131573003/diff/50001/content/renderer/dom_aut...
File content/renderer/dom_automation_controller.cc (right):

https://codereview.chromium.org/131573003/diff/50001/content/renderer/dom_aut...
content/renderer/dom_automation_controller.cc:69: std::string str;
On 2014/01/09 20:11:06, Aaron Boodman wrote:
> You could shorten this a bit by using V8ValueConverter:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...
> 
> Check the input is one of the three allowed types, then call the converter. If
> people want to allow more JSON types in the future, it will work easily.

The problem is that some tests rely on passing a double to send() but an int32
being transferred...

https://codereview.chromium.org/131573003/diff/50001/content/renderer/dom_aut...
File content/renderer/dom_automation_controller.h (right):

https://codereview.chromium.org/131573003/diff/50001/content/renderer/dom_aut...
content/renderer/dom_automation_controller.h:97: bool Send(gin::Arguments*
args);
On 2014/01/09 20:11:06, Aaron Boodman wrote:
> Did you try const gin::Arguments& ? I thought that worked too, and would be
more
> stylistically correct.

GetNext() is non-const :-/

https://codereview.chromium.org/131573003/diff/50001/content/renderer/render_...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/131573003/diff/50001/content/renderer/render_...
content/renderer/render_view_impl.h:1500: // DOM automation bindings are
enabled.
On 2014/01/09 20:19:20, Aaron Boodman wrote:
> On 2014/01/09 20:11:06, Aaron Boodman wrote:
> > How does RV lifetime relate to frame lifetime, and more specifically this
> > pointer? Can the JS context shut down making this pointer garbage while RV
is
> > still outstanding?
> > 
> > Either way some comments would be good.
> 
> On second thought, instead of storing this scary looking pointer, what about
> stashing DomAutomatonController in JS-land instead, via
> frame->context()->global()->SetHiddenValue()?
> 
> On the other hand, it looks like this pointer is never used except as a flag
as
> to whether the controller has been installed already. In that case, maybe we
> shouldn't store it at all, but rather actually look in the JS context and see
if
> it's there. Or store a boolean did_install_ instead?

I think I'll just install one controller per frame and don't keep a reference to
it at all

[Aaron Boodman, 2014-01-09 20:41:25]

https://codereview.chromium.org/131573003/diff/50001/content/renderer/dom_aut...
File content/renderer/dom_automation_controller.cc (right):

https://codereview.chromium.org/131573003/diff/50001/content/renderer/dom_aut...
content/renderer/dom_automation_controller.cc:69: std::string str;
On 2014/01/09 20:30:38, jochen wrote:
> On 2014/01/09 20:11:06, Aaron Boodman wrote:
> > You could shorten this a bit by using V8ValueConverter:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/ren...
> > 
> > Check the input is one of the three allowed types, then call the converter.
If
> > people want to allow more JSON types in the future, it will work easily.
> 
> The problem is that some tests rely on passing a double to send() but an int32
> being transferred...

Do you mean they pass a non-integral number, or that they pass a Number. If the
latter, V8ValueConverter will send an int32 for you:

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/v...

https://codereview.chromium.org/131573003/diff/50001/content/renderer/dom_aut...
File content/renderer/dom_automation_controller.h (right):

https://codereview.chromium.org/131573003/diff/50001/content/renderer/dom_aut...
content/renderer/dom_automation_controller.h:97: bool Send(gin::Arguments*
args);
On 2014/01/09 20:30:38, jochen wrote:
> On 2014/01/09 20:11:06, Aaron Boodman wrote:
> > Did you try const gin::Arguments& ? I thought that worked too, and would be
> more
> > stylistically correct.
> 
> GetNext() is non-const :-/

Ugh. I have hit that problem several times. The fact that Arguments behaves as
an iterator always bothered me. Perhaps I will separate out the iterator aspect,
or just get rid of it.

[jochen (gone - plz use gerrit), 2014-01-10 10:03:56]

ptal

[Aaron Boodman, 2014-01-11 05:41:18]

Very nice. LGTM.

https://codereview.chromium.org/131573003/diff/220001/content/renderer/dom_au...
File content/renderer/dom_automation_controller.cc (right):

https://codereview.chromium.org/131573003/diff/220001/content/renderer/dom_au...
content/renderer/dom_automation_controller.cc:71: V8ValueConverterImpl conv;
cool

[commit-bot: I haz the power, 2014-01-11 06:21:27]

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

[commit-bot: I haz the power, 2014-01-11 06:59:36]

Retried try job too often on mac_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-11 11:50:50]

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

[commit-bot: I haz the power, 2014-01-11 23:30:52]

Change committed as 244394

[abarth-chromium, 2014-01-12 01:51:55]

https://codereview.chromium.org/131573003/diff/460001/content/renderer/dom_au...
File content/renderer/dom_automation_controller.h (right):

https://codereview.chromium.org/131573003/diff/460001/content/renderer/dom_au...
content/renderer/dom_automation_controller.h:112: blink::WebFrame* frame_;
What keeps this pointer from becoming stale?

[jochen (gone - plz use gerrit), 2014-01-12 17:52:16]

A revert of this CL has been created in
https://codereview.chromium.org/135983002/ by jochen@chromium.org.

The reason for reverting is: requires one more iteration.

[Aaron Boodman, 2014-01-15 19:17:53]

https://codereview.chromium.org/131573003/diff/50001/content/renderer/dom_aut...
File content/renderer/dom_automation_controller.h (right):

https://codereview.chromium.org/131573003/diff/50001/content/renderer/dom_aut...
content/renderer/dom_automation_controller.h:97: bool Send(gin::Arguments*
args);
On 2014/01/09 20:41:25, Aaron Boodman wrote:
> On 2014/01/09 20:30:38, jochen wrote:
> > On 2014/01/09 20:11:06, Aaron Boodman wrote:
> > > Did you try const gin::Arguments& ? I thought that worked too, and would
be
> > more
> > > stylistically correct.
> > 
> > GetNext() is non-const :-/
> 
> Ugh. I have hit that problem several times. The fact that Arguments behaves as
> an iterator always bothered me. Perhaps I will separate out the iterator
aspect,
> or just get rid of it.

Following up on this: I looked into factoring out iteration of arguments so that
this could be const, but I realized there is another thing preventing constness:
SetReturnValue().

With that, it looks like passing gin::Arguments by pointer is the right thing.