Move WebAXObjectProxy and AccessibleController from CppBoundClass to gin::Wrappable

Move WebAXObjectProxy and AccessibleController from CppBoundClass to gin::Wrappable

BUG=297480, 331301
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253821

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

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

[kouhei (in TOK), 2014-02-21 01:27:25]

https://codereview.chromium.org/172263002/diff/50001/gin/unsafe_persistent.h
File gin/unsafe_persistent.h (right):

https://codereview.chromium.org/172263002/diff/50001/gin/unsafe_persistent.h#...
gin/unsafe_persistent.h:32: }
You also need to define operator= and destructor when you define your own copy
c-tor.
http://en.wikipedia.org/wiki/Rule_of_three_(C%2B%2B_programming)

[dmazzoni, 2014-02-21 05:57:29]

Do we have to move all of the test code from the Blink codebase to the Chromium
codebase?

That means that every change that adds a new enum or public interface will
require first adding the definition to Blink, waiting for the roll, then adding
test support to Chromium, then actually implementing it and adding the layout
test in Blink, then waiting for a second roll, then finally making use of the
new interface in Chromium.

That's going to slow down our velocity a lot. Is there any alternative?

[jochen (gone - plz use gerrit), 2014-02-21 06:03:58]

On 2014/02/21 05:57:29, dmazzoni wrote:
> Do we have to move all of the test code from the Blink codebase to the
Chromium
> codebase?
> 

The main reason for the move was that in blink, we don't have support for base
classes or javascript bindings outside of core. So the test runner had to
maintain local copies of basic stuff like a scoped ptr and complex stuff like
CppBoundClass. Moving it to content allows for fixing this.

> That means that every change that adds a new enum or public interface will
> require first adding the definition to Blink, waiting for the roll, then
adding
> test support to Chromium, then actually implementing it and adding the layout
> test in Blink, then waiting for a second roll, then finally making use of the
> new interface in Chromium.

There weren't many changes to test runner during the last year. All of them
required changes to content as well.

> 
> That's going to slow down our velocity a lot. Is there any alternative?

It's possible to move stuff to window.internal which is part of blink core.

[dmazzoni, 2014-02-21 06:14:39]

On 2014/02/21 05:57:29, dmazzoni wrote:
> Do we have to move all of the test code from the Blink codebase to the
Chromium
> codebase?
> 
> That means that every change that adds a new enum or public interface will
> require first adding the definition to Blink, waiting for the roll, then
adding
> test support to Chromium, then actually implementing it and adding the layout
> test in Blink, then waiting for a second roll, then finally making use of the
> new interface in Chromium.
> 
> That's going to slow down our velocity a lot. Is there any alternative?

Nevermind, I just discovered this happened a couple of months ago. (I've been on
leave, just returned.)
I was confused since this changelist seemed to add new implementations but
didn't seem to delete
old ones.

I just hope the repositories can merge before too long...

[hajimehoshi, 2014-02-21 11:07:23]

There are still 11 test fails out of 166 accessibility tests.

Nine failures is crashing at assertV8RecursionScope, which means that there are
some wrong way how to call a function.

accessibility/accessibility-controller-notificationlistener-leak-callback.html
accessibility/text-change-notification.html
accessibility/contenteditable-notifications.html	
accessibility/other-aria-attribute-change-sends-notification.html
accessibility/loading-iframe-sends-notification.html
accessibility/aria-checkbox-sends-notification.html
accessibility/multiselect-list-reports-active-option.html
accessibility/is-ignored-change-sends-notification.html
accessibility/notification-listeners.html

One is that the test timed out
(accessibility/deleting-iframe-destroys-axcache.html)

The other is that simply the actual result is different from the expected result
(accessibility/textarea-selected-text-range.html)

I think I can fix them soon, but today I have to leave now. PTAL

https://codereview.chromium.org/172263002/diff/50001/gin/unsafe_persistent.h
File gin/unsafe_persistent.h (right):

https://codereview.chromium.org/172263002/diff/50001/gin/unsafe_persistent.h#...
gin/unsafe_persistent.h:32: }
On 2014/02/21 01:27:26, kouhei wrote:
> You also need to define operator= and destructor when you define your own copy
> c-tor.
> http://en.wikipedia.org/wiki/Rule_of_three_%28C%252B%252B_programming)

Done. (Removed and used the default copy constructors)

[hajimehoshi, 2014-02-24 11:03:00]

The rest failures are two as long as https://codereview.chromium.org/177553002/
is applied.

* accessibility/loading-iframe-sends-notification.html	
* accessibility/notification-listeners.html

These tests cause inconsistency of the number of arguments. I'll fix them soon.

[jochen (gone - plz use gerrit), 2014-02-24 11:21:52]

overall looks great!

https://codereview.chromium.org/172263002/diff/380001/content/shell/renderer/...
File content/shell/renderer/test_runner/accessibility_controller.cc (right):

https://codereview.chromium.org/172263002/diff/380001/content/shell/renderer/...
content/shell/renderer/test_runner/accessibility_controller.cc:49: // static
nit. we only use this comment for methods.

https://codereview.chromium.org/172263002/diff/380001/content/shell/renderer/...
content/shell/renderer/test_runner/accessibility_controller.cc:51:
gin::kEmbedderNativeGin,
nit. no , at end and }; should be in this line, and 4 spaces indent

https://codereview.chromium.org/172263002/diff/380001/content/shell/renderer/...
content/shell/renderer/test_runner/accessibility_controller.cc:128: return
controller_ ? controller_->AccessibleElementById(id)
is this clang-formated?

https://codereview.chromium.org/172263002/diff/380001/gin/unsafe_persistent.h
File gin/unsafe_persistent.h (right):

https://codereview.chromium.org/172263002/diff/380001/gin/unsafe_persistent.h...
gin/unsafe_persistent.h:5: #ifndef GIN_UNSAFE_PERSISTENT_H_
instead of putting this into gin/, i'd put it into
content/shell/renderer/test_runner to discourage use outside of it.

[hajimehoshi, 2014-02-25 04:27:47]

Thank you!

https://codereview.chromium.org/172263002/diff/380001/content/shell/renderer/...
File content/shell/renderer/test_runner/accessibility_controller.cc (right):

https://codereview.chromium.org/172263002/diff/380001/content/shell/renderer/...
content/shell/renderer/test_runner/accessibility_controller.cc:49: // static
On 2014/02/24 11:21:52, jochen wrote:
> nit. we only use this comment for methods.

Done. (WebAXObjectProxy as well)

https://codereview.chromium.org/172263002/diff/380001/content/shell/renderer/...
content/shell/renderer/test_runner/accessibility_controller.cc:51:
gin::kEmbedderNativeGin,
On 2014/02/24 11:21:52, jochen wrote:
> nit. no , at end and }; should be in this line, and 4 spaces indent

Done. (WebAXObjectProxy as well)

https://codereview.chromium.org/172263002/diff/380001/content/shell/renderer/...
content/shell/renderer/test_runner/accessibility_controller.cc:128: return
controller_ ? controller_->AccessibleElementById(id)
On 2014/02/24 11:21:52, jochen wrote:
> is this clang-formated?

Done.

https://codereview.chromium.org/172263002/diff/380001/gin/unsafe_persistent.h
File gin/unsafe_persistent.h (right):

https://codereview.chromium.org/172263002/diff/380001/gin/unsafe_persistent.h...
gin/unsafe_persistent.h:5: #ifndef GIN_UNSAFE_PERSISTENT_H_
On 2014/02/24 11:21:52, jochen wrote:
> instead of putting this into gin/, i'd put it into
> content/shell/renderer/test_runner to discourage use outside of it.

Done. (I put this into the namespace 'content', but it might be better to put
UnsafePersistent into the other namespace, like WebTestRunner).

[hajimehoshi, 2014-02-25 04:44:59]

Now all accessibility tests passed!

The remaining task is to manipulate the recursive state of V8 on blink side
(https://codereview.chromium.org/177553002/).

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

lgtm

[hajimehoshi, 2014-02-27 03:35:06]

Oh yeah, callFunctionEvenIfScriptDisabled worked without a hack on blink! PTAL
just in case.

[kouhei (in TOK), 2014-02-27 05:50:53]

https://codereview.chromium.org/172263002/diff/520001/content/shell/renderer/...
File content/shell/renderer/test_runner/accessibility_controller.cc (right):

https://codereview.chromium.org/172263002/diff/520001/content/shell/renderer/...
content/shell/renderer/test_runner/accessibility_controller.cc:220: // FIXME:
Implement this
Would you implement this?

notification_callbacks_.reset() should be enough IMO.

https://codereview.chromium.org/172263002/diff/520001/content/shell/renderer/...
File content/shell/renderer/test_runner/unsafe_persistent.h (right):

https://codereview.chromium.org/172263002/diff/520001/content/shell/renderer/...
content/shell/renderer/test_runner/unsafe_persistent.h:34: T* operator ->()
const {
no space between operator and ->

[hajimehoshi, 2014-02-27 07:33:07]

Thank you!

https://codereview.chromium.org/172263002/diff/520001/content/shell/renderer/...
File content/shell/renderer/test_runner/accessibility_controller.cc (right):

https://codereview.chromium.org/172263002/diff/520001/content/shell/renderer/...
content/shell/renderer/test_runner/accessibility_controller.cc:220: // FIXME:
Implement this
On 2014/02/27 05:50:54, kouhei wrote:
> Would you implement this?
> 
> notification_callbacks_.reset() should be enough IMO.

We realized that we don't have to set multiple callbacks for one object, so I
changed it to have only one callback. The methods will changed from
{add/remove}NotificationCallback to {set/unset}NotificationCallback. Same as
WebAXObject.

https://codereview.chromium.org/172263002/diff/520001/content/shell/renderer/...
File content/shell/renderer/test_runner/unsafe_persistent.h (right):

https://codereview.chromium.org/172263002/diff/520001/content/shell/renderer/...
content/shell/renderer/test_runner/unsafe_persistent.h:34: T* operator ->()
const {
On 2014/02/27 05:50:54, kouhei wrote:
> no space between operator and ->

Done (Removed).

[kouhei (in TOK), 2014-02-27 07:42:44]

https://codereview.chromium.org/172263002/diff/540001/content/shell/renderer/...
File content/shell/renderer/test_runner/unsafe_persistent.h (right):

https://codereview.chromium.org/172263002/diff/540001/content/shell/renderer/...
content/shell/renderer/test_runner/unsafe_persistent.h:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
Remove this file.

[jochen (gone - plz use gerrit), 2014-02-27 07:46:22]

On 2014/02/27 07:42:44, kouhei wrote:
>
https://codereview.chromium.org/172263002/diff/540001/content/shell/renderer/...
> File content/shell/renderer/test_runner/unsafe_persistent.h (right):
> 
>
https://codereview.chromium.org/172263002/diff/540001/content/shell/renderer/...
> content/shell/renderer/test_runner/unsafe_persistent.h:1: // Copyright 2014
The
> Chromium Authors. All rights reserved.
> Remove this file.

why?

[kouhei (in TOK), 2014-02-27 07:50:24]

On 2014/02/27 07:46:22, jochen (OOO from March 1) wrote:
> On 2014/02/27 07:42:44, kouhei wrote:
> >
>
https://codereview.chromium.org/172263002/diff/540001/content/shell/renderer/...
> > File content/shell/renderer/test_runner/unsafe_persistent.h (right):
> > 
> >
>
https://codereview.chromium.org/172263002/diff/540001/content/shell/renderer/...
> > content/shell/renderer/test_runner/unsafe_persistent.h:1: // Copyright 2014
> The
> > Chromium Authors. All rights reserved.
> > Remove this file.
> 
> why?

I thought we should avoid UnsafePersistent as much as possible. We are
introducing UnsafePersistent only for containers, but speed isn't critical here
so we can use regular v8 dict/array here without introducing UnsafePersistent.

[jochen (gone - plz use gerrit), 2014-02-27 08:25:14]

On 2014/02/27 07:50:24, kouhei wrote:
> On 2014/02/27 07:46:22, jochen (OOO from March 1) wrote:
> > On 2014/02/27 07:42:44, kouhei wrote:
> > >
> >
>
https://codereview.chromium.org/172263002/diff/540001/content/shell/renderer/...
> > > File content/shell/renderer/test_runner/unsafe_persistent.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/172263002/diff/540001/content/shell/renderer/...
> > > content/shell/renderer/test_runner/unsafe_persistent.h:1: // Copyright
2014
> > The
> > > Chromium Authors. All rights reserved.
> > > Remove this file.
> > 
> > why?
> 
> I thought we should avoid UnsafePersistent as much as possible. We are
> introducing UnsafePersistent only for containers, but speed isn't critical
here
> so we can use regular v8 dict/array here without introducing UnsafePersistent.

I expect that we get the changes to be able to put Persistent into stl
containers some time next week. At that point, a vector will be more convenient,
so going with unsafe persistent for now is ok

[kouhei (in TOK), 2014-02-27 08:30:46]

On 2014/02/27 08:25:14, jochen (OOO from March 1) wrote:
> On 2014/02/27 07:50:24, kouhei wrote:
> > On 2014/02/27 07:46:22, jochen (OOO from March 1) wrote:
> > > On 2014/02/27 07:42:44, kouhei wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/172263002/diff/540001/content/shell/renderer/...
> > > > File content/shell/renderer/test_runner/unsafe_persistent.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/172263002/diff/540001/content/shell/renderer/...
> > > > content/shell/renderer/test_runner/unsafe_persistent.h:1: // Copyright
> 2014
> > > The
> > > > Chromium Authors. All rights reserved.
> > > > Remove this file.
> > > 
> > > why?
> > 
> > I thought we should avoid UnsafePersistent as much as possible. We are
> > introducing UnsafePersistent only for containers, but speed isn't critical
> here
> > so we can use regular v8 dict/array here without introducing
UnsafePersistent.
> 
> I expect that we get the changes to be able to put Persistent into stl
> containers some time next week. At that point, a vector will be more
convenient,
> so going with unsafe persistent for now is ok

That's a great news. lgtm then.

[hajimehoshi, 2014-02-27 08:31:38]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-02-27 08:31:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/172263002/540001

[hajimehoshi, 2014-02-27 09:19:58]

The CQ bit was unchecked by hajimehoshi@chromium.org

[hajimehoshi, 2014-02-27 09:30:24]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-02-27 09:30:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/172263002/560001

[commit-bot: I haz the power, 2014-02-27 14:54:18]

Change committed as 253821

[apavlov, 2014-02-27 17:12:53]

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

The reason for reverting is: Tentatively resulted in V8 crashes for
fast/dom/custom/document-register-reentrant-throwing-constructor.html,
fast/events/before-unload-return-bad-value.html,
fast/events/onerror-no-constructor.html.

[hajimehoshi, 2014-02-28 06:45:35]

On 2014/02/27 17:12:53, apavlov wrote:
> A revert of this CL has been created in
> https://codereview.chromium.org/180803004/ by mailto:apavlov@chromium.org.
> 
> The reason for reverting is: Tentatively resulted in V8 crashes for
> fast/dom/custom/document-register-reentrant-throwing-constructor.html,
> fast/events/before-unload-return-bad-value.html,
> fast/events/onerror-no-constructor.html.

I found that ObjectTemplate::NewInstance might return an empty object in some
extreme cases, e.g., Object.constructor is overwritten by user JavaScript. I
fixed it to return the empty handle early in such these cases in gin::Wrappable.
My fix might be ad hoc.

PTAL

[jochen (gone - plz use gerrit), 2014-02-28 06:48:41]

On 2014/02/28 06:45:35, hajimehoshi wrote:
> On 2014/02/27 17:12:53, apavlov wrote:
> > A revert of this CL has been created in
> > https://codereview.chromium.org/180803004/ by mailto:apavlov@chromium.org.
> > 
> > The reason for reverting is: Tentatively resulted in V8 crashes for
> > fast/dom/custom/document-register-reentrant-throwing-constructor.html,
> > fast/events/before-unload-return-bad-value.html,
> > fast/events/onerror-no-constructor.html.
> 
> I found that ObjectTemplate::NewInstance might return an empty object in some
> extreme cases, e.g., Object.constructor is overwritten by user JavaScript. I
> fixed it to return the empty handle early in such these cases in
gin::Wrappable.
> My fix might be ad hoc.
> 
> PTAL

Can you move the gin change into a sperate cl (and add a test)?

[hajimehoshi, 2014-03-03 04:34:53]

On 2014/02/28 06:48:41, jochen (OOO until March 10) wrote:
> On 2014/02/28 06:45:35, hajimehoshi wrote:
> > On 2014/02/27 17:12:53, apavlov wrote:
> > > A revert of this CL has been created in
> > > https://codereview.chromium.org/180803004/ by mailto:apavlov@chromium.org.
> > > 
> > > The reason for reverting is: Tentatively resulted in V8 crashes for
> > > fast/dom/custom/document-register-reentrant-throwing-constructor.html,
> > > fast/events/before-unload-return-bad-value.html,
> > > fast/events/onerror-no-constructor.html.
> > 
> > I found that ObjectTemplate::NewInstance might return an empty object in
some
> > extreme cases, e.g., Object.constructor is overwritten by user JavaScript. I
> > fixed it to return the empty handle early in such these cases in
> gin::Wrappable.
> > My fix might be ad hoc.
> > 
> > PTAL
> 
> Can you move the gin change into a sperate cl (and add a test)?

Done (https://codereview.chromium.org/184253004/).

kouhei-san or haraken-san, would you take a look? Apparently I need another
approval for another patch set.

[kouhei (in TOK), 2014-03-03 04:37:13]

still lgtm

[hajimehoshi, 2014-03-03 04:38:44]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-03-03 04:39:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/172263002/600001

[haraken, 2014-03-03 04:48:18]

LGTM

[commit-bot: I haz the power, 2014-03-03 07:50:24]

Change committed as 254432

[apavlov, 2014-03-04 08:18:14]

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

The reason for reverting is: fast/dom/Window/property-access-on-cached*.html
tests (4 in total) assert on debug bots..

[hajimehoshi, 2014-03-04 09:49:58]

Current failures I found on my local machine:

$ ./webkit/tools/layout_tests/run_webkit_tests.py --debug fast/dom/Window/*.html
...
[42/111]
fast/dom/Window/property-access-on-cached-properties-after-frame-navigated.html
failed unexpectedly (renderer crashed [pid=16364])
[43/111]
fast/dom/Window/property-access-on-cached-properties-after-frame-removed.html
failed unexpectedly (renderer crashed [pid=16409])
[44/111]
fast/dom/Window/property-access-on-cached-window-after-frame-navigated.html
failed unexpectedly (renderer crashed [pid=16454])
[45/111]
fast/dom/Window/property-access-on-cached-window-after-frame-removed.html failed
unexpectedly (renderer crashed [pid=16499])

[hajimehoshi, 2014-03-05 05:44:28]

On 2014/03/04 09:49:58, hajimehoshi wrote:
> Current failures I found on my local machine:
> 
> $ ./webkit/tools/layout_tests/run_webkit_tests.py --debug
fast/dom/Window/*.html
> ...
> [42/111]
>
fast/dom/Window/property-access-on-cached-properties-after-frame-navigated.html
> failed unexpectedly (renderer crashed [pid=16364])
> [43/111]
> fast/dom/Window/property-access-on-cached-properties-after-frame-removed.html
> failed unexpectedly (renderer crashed [pid=16409])
> [44/111]
> fast/dom/Window/property-access-on-cached-window-after-frame-navigated.html
> failed unexpectedly (renderer crashed [pid=16454])
> [45/111]
> fast/dom/Window/property-access-on-cached-window-after-frame-removed.html
failed
> unexpectedly (renderer crashed [pid=16499])

I hope they will be fixed by https://codereview.chromium.org/184233004/...

[hajimehoshi, 2014-03-06 04:13:11]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-03-06 04:14:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/172263002/620001

[commit-bot: I haz the power, 2014-03-06 04:26:04]

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

[commit-bot: I haz the power, 2014-03-06 04:26:05]

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

[hajimehoshi, 2014-03-06 04:36:11]

The CQ bit was checked by hajimehoshi@chromium.org

[commit-bot: I haz the power, 2014-03-06 04:37:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/172263002/620001

[commit-bot: I haz the power, 2014-03-06 06:18:57]

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

[commit-bot: I haz the power, 2014-03-06 06:18:58]

Try jobs failed on following builders: mac_chromium_rel

[hajimehoshi, 2014-03-06 06:26:43]

The CQ bit was checked by hajimehoshi@chromium.org

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

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hajimehoshi@chromium.org/172263002/620001

[commit-bot: I haz the power, 2014-03-06 14:20:26]

Change committed as 255342