Introduce AXObjectCacheBase in core/ and remove WebNode dependency

Introduce AXObjectCacheBase in core/ and remove WebNode dependency

Introduce AXObjectCacheBase in core/ and add a pure virtual Get()
method, which removes the WebNode dependency on AXObjectCacheImpl since
it can use AXObjectCacheBase instead.

In follow-up patches, more methods will be added to AXObjectCacheBase
to remove the dependency of web/ files on modules/accessibility/.
Finally, AXObjectCacheBase will be refactored into AXObjectCache and
removed.

BUG=715382
Review-Url: https://codereview.chromium.org/2880993002
Cr-Commit-Position: refs/heads/master@{#473509}
Committed: https://chromium.googlesource.com/chromium/src/+/c2bc18b08c8f89e49dccb2c9708c64ec318aadf7

[sashab, 2017-05-15 05:18:27]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[sashab, 2017-05-15 05:18:31]

sashab@chromium.org changed reviewers:
+ slangley@chromium.org

[sashab, 2017-05-15 05:18:41]

Description was changed from

==========
Introduce AXObjectCacheBase in core/ and remove WebNode dependency

Introduce AXObjectCacheBase in core/ and add a pure virtual Get()
method, which removes the WebNode dependency on AXObjectCacheImpl since
it can use AXObjectCacheBase instead.

In follow-up patches, more methods will be added to AXObjectCacheBase
to remove the dependency of web/ files on modules/accessibility/.
Finally, AXObjectCacheBase will be 'merged' with AXObjectCache and
removed.

BUG=715382
==========

to

==========
Introduce AXObjectCacheBase in core/ and remove WebNode dependency

Introduce AXObjectCacheBase in core/ and add a pure virtual Get()
method, which removes the WebNode dependency on AXObjectCacheImpl since
it can use AXObjectCacheBase instead.

In follow-up patches, more methods will be added to AXObjectCacheBase
to remove the dependency of web/ files on modules/accessibility/.
Finally, AXObjectCacheBase will be refactored into AXObjectCache and
removed.

BUG=715382
==========

[commit-bot: I haz the power, 2017-05-15 05:18:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sashab, 2017-05-15 05:19:17]

sashab@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[commit-bot: I haz the power, 2017-05-15 05:22:17]

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

[commit-bot: I haz the power, 2017-05-15 05:22:18]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[slangley, 2017-05-15 05:27:20]

https://codereview.chromium.org/2880993002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/AXObjectCacheBase.h (right):

https://codereview.chromium.org/2880993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/AXObjectCacheBase.h:27: ~AXObjectCacheBase()
{}
Probably should be virtual right?

https://codereview.chromium.org/2880993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/AXObjectCacheBase.h:30: AXObjectCacheBase() :
AXObjectCache(){};
Do we need this? I think it should automatically invoke the base class ctor when
there are no args?

https://codereview.chromium.org/2880993002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.h
(right):

https://codereview.chromium.org/2880993002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.h:54:
~AXObjectCacheImpl();
doesn't this need an override?

[sashab, 2017-05-15 05:50:22]

https://codereview.chromium.org/2880993002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/AXObjectCacheBase.h (right):

https://codereview.chromium.org/2880993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/AXObjectCacheBase.h:27: ~AXObjectCacheBase()
{}
On 2017/05/15 at 05:27:19, slangley wrote:
> Probably should be virtual right?

Done. Also moved it above the other method.

https://codereview.chromium.org/2880993002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/AXObjectCacheBase.h:30: AXObjectCacheBase() :
AXObjectCache(){};
On 2017/05/15 at 05:27:19, slangley wrote:
> Do we need this? I think it should automatically invoke the base class ctor
when there are no args?

I get this error when I remove it:

../../third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:94:20:
error: constructor for 'blink::AXObjectCacheImpl' must explicitly initialize the
base class 'blink::AXObjectCacheBase' which does not have a default constructor
AXObjectCacheImpl::AXObjectCacheImpl(Document& document)
                   ^
../../third_party/WebKit/Source/core/dom/AXObjectCacheBase.h:22:19: note:
'blink::AXObjectCacheBase' declared here
class CORE_EXPORT AXObjectCacheBase : public AXObjectCache {
                  ^
1 error generated.

I added it back, but removed the explicit call to the parent class.

[sashab, 2017-05-15 05:50:33]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 05:50:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 05:53:53]

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

[commit-bot: I haz the power, 2017-05-15 05:53:53]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[slangley, 2017-05-15 06:16:45]

lgtm

https://codereview.chromium.org/2880993002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.h
(right):

https://codereview.chromium.org/2880993002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.h:54:
~AXObjectCacheImpl();
override

[sashab, 2017-05-15 06:40:23]

Done; thanks.

dmazzoni@ ptal :)

[dmazzoni, 2017-05-15 20:45:10]

lgtm

https://codereview.chromium.org/2880993002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/AXObjectCacheBase.h (right):

https://codereview.chromium.org/2880993002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/AXObjectCacheBase.h:28: virtual AXObjectImpl*
Get(Node*) = 0;
Just checking, but this will eventually only return an AXObject*, right?

Because web/ shouldn't reference AXObjectImpl...

[haraken1, 2017-05-15 20:52:04]

Maybe I'm missing something but why can't we simply start moving methods from
AXObjectCacheImpl to AXObjectCache?

[dmazzoni, 2017-05-15 21:17:57]

On 2017/05/15 20:52:04, haraken1 wrote:
> Maybe I'm missing something but why can't we simply start moving methods from
> AXObjectCacheImpl to AXObjectCache?

Is AXObjectCacheImpl::Get() the only thing we need to expose? If so, just
put it on AXObjectCache and put the TODO there. I don't have a problem with
exposing something there temporarily. The main thing I didn't like (from
an earlier proposal) was to expose *all* of AXObjectCacheImpl or AXObjectImpl
in the public interface, that seemed overkill.

We didn't discuss AXObjectCache as much. If it's just that one Get() function
then this extra step may not be needed.

[dmazzoni, 2017-05-15 21:17:59]

On 2017/05/15 20:52:04, haraken1 wrote:
> Maybe I'm missing something but why can't we simply start moving methods from
> AXObjectCacheImpl to AXObjectCache?

Is AXObjectCacheImpl::Get() the only thing we need to expose? If so, just
put it on AXObjectCache and put the TODO there. I don't have a problem with
exposing something there temporarily. The main thing I didn't like (from
an earlier proposal) was to expose *all* of AXObjectCacheImpl or AXObjectImpl
in the public interface, that seemed overkill.

We didn't discuss AXObjectCache as much. If it's just that one Get() function
then this extra step may not be needed.

[haraken, 2017-05-15 21:21:24]

On 2017/05/15 21:17:59, dmazzoni wrote:
> On 2017/05/15 20:52:04, haraken1 wrote:
> > Maybe I'm missing something but why can't we simply start moving methods
from
> > AXObjectCacheImpl to AXObjectCache?
> 
> Is AXObjectCacheImpl::Get() the only thing we need to expose? If so, just
> put it on AXObjectCache and put the TODO there. I don't have a problem with
> exposing something there temporarily. The main thing I didn't like (from
> an earlier proposal) was to expose *all* of AXObjectCacheImpl or AXObjectImpl
> in the public interface, that seemed overkill.

Ah, I now understand the point :)

I think sashab is planning to expose more methods than Get(). This CL LGTM.

> 
> We didn't discuss AXObjectCache as much. If it's just that one Get() function
> then this extra step may not be needed.

[sashab, 2017-05-15 23:37:07]

On 2017/05/15 at 21:21:24, haraken wrote:
> On 2017/05/15 21:17:59, dmazzoni wrote:
> > On 2017/05/15 20:52:04, haraken1 wrote:
> > > Maybe I'm missing something but why can't we simply start moving methods
from
> > > AXObjectCacheImpl to AXObjectCache?
> > 
> > Is AXObjectCacheImpl::Get() the only thing we need to expose? If so, just
> > put it on AXObjectCache and put the TODO there. I don't have a problem with
> > exposing something there temporarily. The main thing I didn't like (from
> > an earlier proposal) was to expose *all* of AXObjectCacheImpl or
AXObjectImpl
> > in the public interface, that seemed overkill.
> 
> Ah, I now understand the point :)
> 
> I think sashab is planning to expose more methods than Get(). This CL LGTM.

That's right. That's another reason for introducing AXObjectCacheBase - it helps
me break up the work so I can see the methods I want to add to AXObjectCache,
then decide whether to add them later. Depending on how many there are I might
be able to remove AXObjectCacheBase myself at the end of this :)

[sashab, 2017-05-15 23:37:10]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-05-15 23:37:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
slangley@chromium.org
Link to the patchset: https://codereview.chromium.org/2880993002/#ps40001
(title: "More review feedback")

[commit-bot: I haz the power, 2017-05-15 23:38:08]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sashab, 2017-05-16 00:10:01]

sashab@chromium.org changed reviewers:
+ haraken@chromium.org

[sashab, 2017-05-16 00:10:02]

https://codereview.chromium.org/2880993002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/AXObjectCacheBase.h (right):

https://codereview.chromium.org/2880993002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/AXObjectCacheBase.h:28: virtual AXObjectImpl*
Get(Node*) = 0;
On 2017/05/15 at 20:45:10, dmazzoni wrote:
> Just checking, but this will eventually only return an AXObject*, right?
> 
> Because web/ shouldn't reference AXObjectImpl...

Yes :) Was going to put a TODO but it will happen naturally as I remove
dependencies from web/.

[commit-bot: I haz the power, 2017-05-16 00:54:23]

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

[commit-bot: I haz the power, 2017-05-16 00:54:24]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sashab, 2017-05-16 03:44:36]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[sashab, 2017-05-16 03:44:39]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-05-16 03:44:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, slangley@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2880993002/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-05-16 03:45:01]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 04:32:45]

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

[commit-bot: I haz the power, 2017-05-16 04:32:46]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sashab, 2017-05-16 04:33:14]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2017-05-16 04:33:33]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 06:06:18]

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

[commit-bot: I haz the power, 2017-05-16 06:06:19]

Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[sashab, 2017-05-17 04:56:55]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 04:57:36]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 06:21:15]

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

[commit-bot: I haz the power, 2017-05-17 06:21:15]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[sashab, 2017-05-18 09:30:36]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[sashab, 2017-05-18 09:30:40]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-05-18 09:30:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, slangley@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2880993002/#ps100001
(title: "Rebase after fixing ax rename")

[commit-bot: I haz the power, 2017-05-18 09:31:02]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 10:31:21]

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

[commit-bot: I haz the power, 2017-05-18 10:31:22]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sashab, 2017-05-18 10:37:36]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2017-05-18 10:38:11]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 12:07:49]

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

[commit-bot: I haz the power, 2017-05-18 12:07:51]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[sashab, 2017-05-18 12:17:02]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2017-05-18 12:17:37]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 13:30:15]

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

[commit-bot: I haz the power, 2017-05-18 13:30:16]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[sashab, 2017-05-22 03:26:50]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[sashab, 2017-05-22 03:26:53]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-05-22 03:26:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, slangley@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2880993002/#ps120001
(title: "Added cpp file to fix linker error")

[commit-bot: I haz the power, 2017-05-22 03:27:01]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 03:30:51]

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

[commit-bot: I haz the power, 2017-05-22 03:30:52]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sashab, 2017-05-22 03:50:17]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[sashab, 2017-05-22 03:50:22]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-05-22 03:50:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, slangley@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2880993002/#ps140001
(title: "Uploaded missing file")

[commit-bot: I haz the power, 2017-05-22 03:50:30]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 04:00:52]

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

[commit-bot: I haz the power, 2017-05-22 04:00:53]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[sashab, 2017-05-22 04:06:21]

The CQ bit was checked by sashab@chromium.org to run a CQ dry run

[sashab, 2017-05-22 04:06:23]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-05-22 04:06:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, slangley@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2880993002/#ps160001
(title: "Small fix")

[commit-bot: I haz the power, 2017-05-22 04:06:31]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 05:51:53]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1495425983429800,
"parent_rev": "8e3f9e3a9823a1814f0f1f4ff3c4594abbf8a7d5", "commit_rev":
"c2bc18b08c8f89e49dccb2c9708c64ec318aadf7"}

[commit-bot: I haz the power, 2017-05-22 05:52:07]

Description was changed from

==========
Introduce AXObjectCacheBase in core/ and remove WebNode dependency

Introduce AXObjectCacheBase in core/ and add a pure virtual Get()
method, which removes the WebNode dependency on AXObjectCacheImpl since
it can use AXObjectCacheBase instead.

In follow-up patches, more methods will be added to AXObjectCacheBase
to remove the dependency of web/ files on modules/accessibility/.
Finally, AXObjectCacheBase will be refactored into AXObjectCache and
removed.

BUG=715382
==========

to

==========
Introduce AXObjectCacheBase in core/ and remove WebNode dependency

Introduce AXObjectCacheBase in core/ and add a pure virtual Get()
method, which removes the WebNode dependency on AXObjectCacheImpl since
it can use AXObjectCacheBase instead.

In follow-up patches, more methods will be added to AXObjectCacheBase
to remove the dependency of web/ files on modules/accessibility/.
Finally, AXObjectCacheBase will be refactored into AXObjectCache and
removed.

BUG=715382

Review-Url: https://codereview.chromium.org/2880993002
Cr-Commit-Position: refs/heads/master@{#473509}
Committed:
https://chromium.googlesource.com/chromium/src/+/c2bc18b08c8f89e49dccb2c9708c...
==========

[commit-bot: I haz the power, 2017-05-22 05:52:12]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/c2bc18b08c8f89e49dccb2c9708c...