Rename AXObject to AXObjectImpl in modules/ and web/

Rename AXObject to AXObjectImpl in modules/ and web/

Rename AXObject to AXObjectImpl in modules/ and web/, and introduce an
empty abstract parent class in core/. This is pre-work to remove usages
of AXObjectImpl from web/, which will use AXObject instead.

This patch does not rename the usages in core/ since these should use
the new abstract object.

BUG=715382
Review-Url: https://codereview.chromium.org/2858493002
Cr-Original-Commit-Position: refs/heads/master@{#471663}
Committed: https://chromium.googlesource.com/chromium/src/+/234813ae8d52f622a1664e11365642751e8f26ba
Review-Url: https://codereview.chromium.org/2858493002
Cr-Commit-Position: refs/heads/master@{#472370}
Committed: https://chromium.googlesource.com/chromium/src/+/0cd996b5bea03c783b7d10062a669da39e2191da

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

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

[sashab, 2017-05-02 05:10:28]

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

[commit-bot: I haz the power, 2017-05-02 05:10:49]

Dry run: CQ is trying da patch.

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

[slangley, 2017-05-02 05:14:00]

On 2017/05/02 at 05:10:11, sashab wrote:
> 

I doubt this is what you want.

You **want** clients to refer to the least derived type, i.e. AXObject, and not
the concrete implementation of this type (i.e. AXObjectImpl).

[sashab, 2017-05-02 05:15:20]

Description was changed from

==========
Rename AXObject to AXObjectImpl

Rename AXObject to AXObjectImpl. This is pre-work to introduce a new
abstract class in core/ called AXObject, which AXObjectImpl will inherit
from.

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

to

==========
Rename AXObject to AXObjectImpl

Rename AXObject to AXObjectImpl. This is pre-work to introduce a new
abstract class in core/ called AXObject, which AXObjectImpl will inherit
from.

Some (or most) of these callsites will refer to the new AXObject abstract
class after it's created.

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

[sashab, 2017-05-02 05:16:04]

Yup, was going to do that in follow-up patches. So in this patch, its as bad as
before, but in subsequent ones it will get better as callers refer to AXObject
instead of AXObjectImpl.

Updated CL desription to clarify.

[slangley, 2017-05-02 05:24:33]

On 2017/05/02 at 05:16:04, sashab wrote:
> Yup, was going to do that in follow-up patches. So in this patch, its as bad
as before, but in subsequent ones it will get better as callers refer to
AXObject instead of AXObjectImpl.
> 
> Updated CL desription to clarify.

I'm not sure this CL helps you that much. 

Wouldn't it be easier to:

git mv modules/accessibility/AXObject.h core/.../AXObject.h

then change modules/accessibility/AXObject.h to be

#include "core/.../AXObject.h"

then in subsequent smaller CL's update the include paths to
"core/.../AXObject.h" - probably after you've implemented AXObjectImpl.

Also, this CL has changes to comments that I doubt you would want regardless :)

[sashab, 2017-05-02 05:57:38]

Huh, I guess I didn't think of that approach.

So... This would leave the AXObject.cpp file in modules/, correct? Seems kinda
gross to have the header file in one place and the cpp in the other.

So it would start as:

core/dom/AXObject.h
class AXObject { Foo(), Bar() }

modules/accessibility/AXObject.h
#include "core/dom/AXObject.h"

modules/accessibility/AXObject.cpp
#include "modules/accessibility/AXObject.h"
AXObject::Foo() { ... } Bar() { ... }

Then when I add AXObjectImpl:

core/dom/AXObject.h
class AXObject { virtual Foo() = 0, Bar() }

modules/accessibility/AXObject.h
#include "core/dom/AXObject.h"

modules/accessibility/AXObject.cpp
#include "modules/accessibility/AXObject.h"
AXObject::Bar() { ... }

modules/accessibility/AXObjectImpl.h
#include "core/dom/AXObject.h"
class AXObjectImpl { Foo() }

modules/accessibility/AXObjectImpl.cpp
#include "core/dom/AXObject.h"
AXObjectImpl::Foo() { ... }

This seems just as gross as the other way :(

I suppose its temporary, and beats trying to land a giant rename patch... But at
least the way I've done it we get the massive rename out of the way first, then
subsequent patches move less code.

[slangley, 2017-05-02 06:03:09]

I guess I'd assert that you don't want any of those renames, you just want
to update the location of the header file.



On Tue, May 2, 2017 at 3:57 PM, <sashab@chromium.org> wrote:

> Huh, I guess I didn't think of that approach.
>
> So... This would leave the AXObject.cpp file in modules/, correct? Seems
> kinda
> gross to have the header file in one place and the cpp in the other.
>
> So it would start as:
>
> core/dom/AXObject.h
> class AXObject { Foo(), Bar() }
>
> modules/accessibility/AXObject.h
> #include "core/dom/AXObject.h"
>
> modules/accessibility/AXObject.cpp
> #include "modules/accessibility/AXObject.h"
> AXObject::Foo() { ... } Bar() { ... }
>
> Then when I add AXObjectImpl:
>
> core/dom/AXObject.h
> class AXObject { virtual Foo() = 0, Bar() }
>
> modules/accessibility/AXObject.h
> #include "core/dom/AXObject.h"
>
> modules/accessibility/AXObject.cpp
> #include "modules/accessibility/AXObject.h"
> AXObject::Bar() { ... }
>
> modules/accessibility/AXObjectImpl.h
> #include "core/dom/AXObject.h"
> class AXObjectImpl { Foo() }
>
> modules/accessibility/AXObjectImpl.cpp
> #include "core/dom/AXObject.h"
> AXObjectImpl::Foo() { ... }
>
> This seems just as gross as the other way :(
>
> I suppose its temporary, and beats trying to land a giant rename patch...
> But at
> least the way I've done it we get the massive rename out of the way first,
> then
> subsequent patches move less code.
>
> https://codereview.chromium.org/2858493002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[slangley, 2017-05-02 06:10:30]

I guess I'd assert that you don't want any of those renames, you just want
to update the location of the header file.



On Tue, May 2, 2017 at 3:57 PM, <sashab@chromium.org> wrote:

> Huh, I guess I didn't think of that approach.
>
> So... This would leave the AXObject.cpp file in modules/, correct? Seems
> kinda
> gross to have the header file in one place and the cpp in the other.
>
> So it would start as:
>
> core/dom/AXObject.h
> class AXObject { Foo(), Bar() }
>
> modules/accessibility/AXObject.h
> #include "core/dom/AXObject.h"
>
> modules/accessibility/AXObject.cpp
> #include "modules/accessibility/AXObject.h"
> AXObject::Foo() { ... } Bar() { ... }
>
> Then when I add AXObjectImpl:
>
> core/dom/AXObject.h
> class AXObject { virtual Foo() = 0, Bar() }
>
> modules/accessibility/AXObject.h
> #include "core/dom/AXObject.h"
>
> modules/accessibility/AXObject.cpp
> #include "modules/accessibility/AXObject.h"
> AXObject::Bar() { ... }
>
> modules/accessibility/AXObjectImpl.h
> #include "core/dom/AXObject.h"
> class AXObjectImpl { Foo() }
>
> modules/accessibility/AXObjectImpl.cpp
> #include "core/dom/AXObject.h"
> AXObjectImpl::Foo() { ... }
>
> This seems just as gross as the other way :(
>
> I suppose its temporary, and beats trying to land a giant rename patch...
> But at
> least the way I've done it we get the massive rename out of the way first,
> then
> subsequent patches move less code.
>
> https://codereview.chromium.org/2858493002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2017-05-02 06:36:23]

I agree that this is a bit awkward for an intermediate step.

I think the goal is that the code in core/ still refers to AXObject,
but the code in modules/ refers to AXObjectImpl.

Note that none of the code in core/ actually uses an AXObject,
so you should be able to just define the abstract interface in
this change pretty easily:

Soure/core/dom/AXObject.h:

class AXObject {
};

Source/modules/accessibility/AXObjectImpl.h:

class AXObjectImpl : public AXObject {
   ...
};

That'd be a better first step. Then in the next step you could
start pulling some of the interfaces from AXObjectImpl.h to
AXObject.h so that Source/web only depends on the latter.

[slangley, 2017-05-02 07:03:01]

I think the goal is that the code in core/ still refers to AXObject,
> but the code in modules/ refers to AXObjectImpl.
>

Hi Dominic,


I've seen this pattern often in the Blink code base but I don't understand
why we would want to take a dependency on the implementation in preference
to the definition, when it's possible. Is there something I'm missing?

Thanks.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-05-02 07:03:14]

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

[commit-bot: I haz the power, 2017-05-02 07:03:15]

Dry run: This issue passed the CQ dry run.

[slangley, 2017-05-02 07:08:56]

I think the goal is that the code in core/ still refers to AXObject,
> but the code in modules/ refers to AXObjectImpl.
>

Hi Dominic,


I've seen this pattern often in the Blink code base but I don't understand
why we would want to take a dependency on the implementation in preference
to the definition, when it's possible. Is there something I'm missing?

Thanks.

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[sashab, 2017-05-02 23:34:54]

Thanks for your feedback Dom; I added your suggestions, see the new empty
AXObject.h class.

To echo stuarts question - why would we allow code in modules/ to depend on
AXObjectImpl? Can't all classes just use AXObject? Would certainly make things
easier if we wanted to move the code in modules/ again later :)

[sashab, 2017-05-02 23:34:55]

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

[commit-bot: I haz the power, 2017-05-02 23:35:24]

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-03 00:47:52]

Thanks for your input stu & dom; after some more investigation it looks like
AXObject has no dependencies in modules/ except for AXObjectCacheImpl, which I
could refactor to be a dependency on core/dom/AXObjectCache. Then we can move
both AXObject.h and AXObject.cpp to core/dom.

Dmazzoni wdyt?

[commit-bot: I haz the power, 2017-05-03 01:33:35]

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

[commit-bot: I haz the power, 2017-05-03 01:33:37]

Dry run: This issue passed the CQ dry run.

[sashab, 2017-05-03 03:56:57]

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

[commit-bot: I haz the power, 2017-05-03 03:57:14]

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-03 04:22:41]

Description was changed from

==========
Rename AXObject to AXObjectImpl

Rename AXObject to AXObjectImpl. This is pre-work to introduce a new
abstract class in core/ called AXObject, which AXObjectImpl will inherit
from.

Some (or most) of these callsites will refer to the new AXObject abstract
class after it's created.

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

to

==========
Move AXObject.h to core/

Move AXObject.h to core/, and update includes to use it. This is
pre-work to introduce a new class in modules/ called AXObjectImpl, which
will implement all the methods from AXObject.

This means that (temporarily) modules/accessibility/AXObject.cpp
implements core/dom/AXObject.h. This will be fixed in follow-up patches
with the introduction of AXObjectImpl.

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

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

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

[commit-bot: I haz the power, 2017-05-03 05:26:54]

Dry run: 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...)

[dmazzoni, 2017-05-03 06:31:25]

lgtm

[sashab, 2017-05-05 01:49:01]

Description was changed from

==========
Move AXObject.h to core/

Move AXObject.h to core/, and update includes to use it. This is
pre-work to introduce a new class in modules/ called AXObjectImpl, which
will implement all the methods from AXObject.

This means that (temporarily) modules/accessibility/AXObject.cpp
implements core/dom/AXObject.h. This will be fixed in follow-up patches
with the introduction of AXObjectImpl.

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

to

==========
Rename AXObject to AXObjectImpl

Rename AXObject to AXObjectImpl. This is pre-work to introduce a new
abstract class in core/ called AXObject, which AXObjectImpl will inherit
from.

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

[sashab, 2017-05-05 01:58:31]

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

[commit-bot: I haz the power, 2017-05-05 01:59:12]

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-05 02:14:16]

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

[commit-bot: I haz the power, 2017-05-05 02:14:49]

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-05 02:17:14]

sashab@chromium.org changed reviewers:
+ aboxhall@chromium.org, mkwst@chromium.org

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

dmazzoni - ptal again at changes, i changed a lot more than source files this
time, lmkwyt
slangley - rs pls
aboxhall - fyi :)
mkwst - please rs everything outside of modules/accessibility

[commit-bot: I haz the power, 2017-05-05 03:58:54]

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

[commit-bot: I haz the power, 2017-05-05 03:58:55]

Dry run: This issue passed the CQ dry run.

[slangley, 2017-05-05 04:11:42]

lgtm

[aboxhall, 2017-05-05 04:58:53]

On 2017/05/05 04:11:42, slangley wrote:
> lgtm

How about we roll in the addition of the *empty* interface into this patch? Then
the core/ changes can just point to that empty interface and we can avoid a
rename and rename back in core/. 

web/ will still need to point to the -Impl for this patch, since that will
require figuring out the public interface, which is a much larger and
non-mechanical task.

[dmazzoni, 2017-05-05 05:18:46]

lgtm except for some comment/docs/test changes that I
think should be left alone

https://codereview.chromium.org/2858493002/diff/80001/docs/accessibility/over...
File docs/accessibility/overview.md (right):

https://codereview.chromium.org/2858493002/diff/80001/docs/accessibility/over...
docs/accessibility/overview.md:414: page it is rendering. WebAXObject is the
public API wrapper around [AXObjectImpl],
Please revert these changes

https://codereview.chromium.org/2858493002/diff/80001/docs/accessibility/over...
docs/accessibility/overview.md:496: [AXObjectImpl]:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/access...
This one is fine

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/accessibility/corresponding-control-deleted-crash.html
(right):

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/corresponding-control-deleted-crash.html:13:
description("Make sure that a debug assert is not triggered when a call to
LayoutBlockFlow::deleteLineBoxTree calls
AccessibilityRenderObject::accessibilityIsIgnored which may require the
AXObjectImpl for a node that is being deleted.");
Please revert any changes to LayoutTests.

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4261: // Create the AXObjectImpl
cache in a focus change because Chromium relies
A couple instances of "AXObject cache" being changed to "AXObjectImpl cache" in
comments, but that just makes it more confusing, it's really supposed to be
referring to the AXObjectCache class. Either leave it alone or explicitly change
it
to AXObjectCache.

[aboxhall, 2017-05-05 05:22:17]

haha I had all the same comments as Dominic in drafts

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/accessibility/corresponding-control-deleted-crash.html
(right):

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/corresponding-control-deleted-crash.html:13:
description("Make sure that a debug assert is not triggered when a call to
LayoutBlockFlow::deleteLineBoxTree calls
AccessibilityRenderObject::accessibilityIsIgnored which may require the
AXObjectImpl for a node that is being deleted.");
I'd revert these comments (i.e. all the LayoutTests) - this will eventually be
an AXObject and we'll probably forget to change these back.

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/accessibility/not-ignore-landmark-roles.html
(right):

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/not-ignore-landmark-roles.html:46:
var AXObjectImpl = containerNode.childAtIndex(i);
Similarly, no need to be specific re -Impl here.

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4261: // Create the AXObjectImpl
cache in a focus change because Chromium relies
Bad replace :) Maybe update to AXObjectCache instead of AXObject cache?

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:429: // Override scrollbar
notifications to update the AXObjectImpl cache.
Bad replace too

[aboxhall, 2017-05-05 05:22:19]

haha I had all the same comments as Dominic in drafts

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/accessibility/corresponding-control-deleted-crash.html
(right):

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/corresponding-control-deleted-crash.html:13:
description("Make sure that a debug assert is not triggered when a call to
LayoutBlockFlow::deleteLineBoxTree calls
AccessibilityRenderObject::accessibilityIsIgnored which may require the
AXObjectImpl for a node that is being deleted.");
I'd revert these comments (i.e. all the LayoutTests) - this will eventually be
an AXObject and we'll probably forget to change these back.

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/accessibility/not-ignore-landmark-roles.html
(right):

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/accessibility/not-ignore-landmark-roles.html:46:
var AXObjectImpl = containerNode.childAtIndex(i);
Similarly, no need to be specific re -Impl here.

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Document.cpp:4261: // Create the AXObjectImpl
cache in a focus change because Chromium relies
Bad replace :) Maybe update to AXObjectCache instead of AXObject cache?

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/FrameView.h (right):

https://codereview.chromium.org/2858493002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/FrameView.h:429: // Override scrollbar
notifications to update the AXObjectImpl cache.
Bad replace too

[Mike West, 2017-05-05 09:34:53]

LGTM (but I only skimmed it, as the change seemed mechanical enough).

[haraken, 2017-05-05 13:33:47]

LGTM

[sashab, 2017-05-08 23:54:24]

Description was changed from

==========
Rename AXObject to AXObjectImpl

Rename AXObject to AXObjectImpl. This is pre-work to introduce a new
abstract class in core/ called AXObject, which AXObjectImpl will inherit
from.

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

to

==========
Rename AXObject to AXObjectImpl in modules/ and web/

Rename AXObject to AXObjectImpl in modules/ and web/, and introduce an
empty abstract parent class in core/. This is pre-work to remove usages
of AXObjectImpl from web/, which will use AXObject instead.

This patch does not rename the usages in core/ since these should use
the new abstract object.

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

[sashab, 2017-05-08 23:54:24]

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

[sashab, 2017-05-08 23:54:30]

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

[sashab, 2017-05-08 23:58:34]

aboxhall, ptal. If you lgtm this I will land it.

I think I've covered all the review feedback. This is no longer a *completely*
mechanical change, although it's mostly mechanical. Here's the changes I made to
create this patch:

1. No changes to Source/core (including comments - none of these make sense to
change)
2. No changes to documentation/layout tests (except for specific one in file
listing in overview.md)
3. Rename all usages of "AXObject" to "AXObjectImpl" in modules/accessibility
4. Rename all usages of "AXObject" to "AXObjectImpl" in Source/web
5. Introduced empty class "AXObject" in Source/core/dom
6. Introduced toAXObjectImpl() (temporarily) and called it in classes in web/
that implement interfaces in core/ that take an AXObject parameter and need to
downcast
  - Only exception are some places in modules/accessibility that now need to
downcast as well; will look at fixing these later

I also updated the CL description.

https://codereview.chromium.org/2858493002/diff/100001/docs/accessibility/ove...
File docs/accessibility/overview.md (right):

https://codereview.chromium.org/2858493002/diff/100001/docs/accessibility/ove...
docs/accessibility/overview.md:496: [AXObjectImpl]:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/access...
Only non-code change

https://codereview.chromium.org/2858493002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/AXObject.h (right):

https://codereview.chromium.org/2858493002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/AXObject.h:14: class CORE_EXPORT AXObject {};
Introduced virtual parent here

https://codereview.chromium.org/2858493002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/2858493002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:2499:
ToAXObjectImpl(toHTMLInputElement(GetNode())->PopupRootAXObject()))
Although this code is in modules/, it's using a public interface from core/
which returns an AXObject. So the downcast is needed here.

I can fix these in follow-up patches by adding more methods to AXObject, or we
can just leave this here.

https://codereview.chromium.org/2858493002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObjectImpl.h (right):

https://codereview.chromium.org/2858493002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObjectImpl.h:1138: //
TODO(sashab): Remove this once callers in web/ can use AXObject directly.
aboxhall - wdyt of this comment?

[commit-bot: I haz the power, 2017-05-09 00:07:12]

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-09 01:21:40]

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

[commit-bot: I haz the power, 2017-05-09 01:21:41]

Dry run: 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...)
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[aboxhall, 2017-05-09 02:01:02]

lgtm

https://codereview.chromium.org/2858493002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/2858493002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:2499:
ToAXObjectImpl(toHTMLInputElement(GetNode())->PopupRootAXObject()))
On 2017/05/08 23:58:34, sashab wrote:
> Although this code is in modules/, it's using a public interface from core/
> which returns an AXObject. So the downcast is needed here.
> 
> I can fix these in follow-up patches by adding more methods to AXObject, or we
> can just leave this here.

This seems ok to me. Dominic, what do you think?

https://codereview.chromium.org/2858493002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObjectImpl.h (right):

https://codereview.chromium.org/2858493002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObjectImpl.h:1138: //
TODO(sashab): Remove this once callers in web/ can use AXObject directly.
On 2017/05/08 23:58:34, sashab wrote:
> aboxhall - wdyt of this comment?

We may not want or need to remove the ability to cast, per the other example you
called out. Once web/ no longer depends on modules/ it won't be able to call
this cast anyway. So I don't think the comment is necessary.

Also, I think we can use the DEFINE_TYPE_CASTS macro here, like in
AXObjectCache:

DEFINE_TYPE_CASTS(AXObjectImpl, AXObject, cache, true, true);

[dmazzoni, 2017-05-09 05:07:07]

lgtm

https://codereview.chromium.org/2858493002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/2858493002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:2499:
ToAXObjectImpl(toHTMLInputElement(GetNode())->PopupRootAXObject()))
On 2017/05/09 02:01:02, aboxhall wrote:
> On 2017/05/08 23:58:34, sashab wrote:
> > Although this code is in modules/, it's using a public interface from core/
> > which returns an AXObject. So the downcast is needed here.
> > 
> > I can fix these in follow-up patches by adding more methods to AXObject, or
we
> > can just leave this here.
> 
> This seems ok to me. Dominic, what do you think?

Yes, this seems fine. This is a reasonable use of a downcast.

I'm wondering if there might be a cleaner solution that doesn't
even involve core/ returning an AXObject at all. The only place
where it's being used now is to return the root of a popup,
and it ought to work to just return the Document instead and
we could use AXObjectCacheImpl to get an AXObject from
there.

But anyway, for now this is great.

[sashab, 2017-05-10 04:24:43]

Thanks everyone! Yay, we reached a much a better solution after a thorough
review process :)

https://codereview.chromium.org/2858493002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp (right):

https://codereview.chromium.org/2858493002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXLayoutObject.cpp:2499:
ToAXObjectImpl(toHTMLInputElement(GetNode())->PopupRootAXObject()))
On 2017/05/09 at 05:07:07, dmazzoni wrote:
> On 2017/05/09 02:01:02, aboxhall wrote:
> > On 2017/05/08 23:58:34, sashab wrote:
> > > Although this code is in modules/, it's using a public interface from
core/
> > > which returns an AXObject. So the downcast is needed here.
> > > 
> > > I can fix these in follow-up patches by adding more methods to AXObject,
or we
> > > can just leave this here.
> > 
> > This seems ok to me. Dominic, what do you think?
> 
> Yes, this seems fine. This is a reasonable use of a downcast.
> 
> I'm wondering if there might be a cleaner solution that doesn't
> even involve core/ returning an AXObject at all. The only place
> where it's being used now is to return the root of a popup,
> and it ought to work to just return the Document instead and
> we could use AXObjectCacheImpl to get an AXObject from
> there.
> 
> But anyway, for now this is great.

Sgtm :)

https://codereview.chromium.org/2858493002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/accessibility/AXObjectImpl.h (right):

https://codereview.chromium.org/2858493002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/accessibility/AXObjectImpl.h:1138: //
TODO(sashab): Remove this once callers in web/ can use AXObject directly.
On 2017/05/09 at 02:01:02, aboxhall wrote:
> On 2017/05/08 23:58:34, sashab wrote:
> > aboxhall - wdyt of this comment?
> 
> We may not want or need to remove the ability to cast, per the other example
you called out. Once web/ no longer depends on modules/ it won't be able to call
this cast anyway. So I don't think the comment is necessary.
> 
> Also, I think we can use the DEFINE_TYPE_CASTS macro here, like in
AXObjectCache:
> 
> DEFINE_TYPE_CASTS(AXObjectImpl, AXObject, cache, true, true);

Thanks; removed the comment and changed to use the macro :)

[sashab, 2017-05-10 04:24:47]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-05-10 04:24:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
slangley@chromium.org, haraken@chromium.org, mkwst@chromium.org,
dmazzoni@chromium.org, aboxhall@chromium.org
Link to the patchset: https://codereview.chromium.org/2858493002/#ps120001
(title: "Review feedback")

[commit-bot: I haz the power, 2017-05-10 04:25:35]

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-10 07:57:27]

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

[commit-bot: I haz the power, 2017-05-10 07:57:29]

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-11 01:28:05]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2017-05-11 01:29:27]

CQ is trying da patch.

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

[sashab, 2017-05-11 01:58:47]

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

[commit-bot: I haz the power, 2017-05-11 02:00:16]

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-11 02:48:41]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-05-11 02:48:42]

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

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

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-11 05:55:04]

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

[commit-bot: I haz the power, 2017-05-11 05:55:06]

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-12 00:05:22]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2017-05-12 00:06:24]

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-12 01:17:59]

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

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

Failed to apply patch for
third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.h:
While running git apply --index -3 -p1;
  error: patch failed:
third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.h:30
  Falling back to three-way merge...
  Applied patch to
'third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.h' with
conflicts.
  U third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.h

Patch:       third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.h
Index: third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.h
diff --git a/third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.h
b/third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.h
index
bc2cb0069da2759c7b469a4d675240e25fe52d39..d27c24fde26e879342013c94d54925a879cd64c5
100644
--- a/third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.h
+++ b/third_party/WebKit/Source/modules/accessibility/AXInlineTextBox.h
@@ -30,13 +30,13 @@
 #define AXInlineTextBox_h
 
 #include "core/layout/line/AbstractInlineTextBox.h"
-#include "modules/accessibility/AXObject.h"
+#include "modules/accessibility/AXObjectImpl.h"
 
 namespace blink {
 
 class AXObjectCacheImpl;
 
-class AXInlineTextBox final : public AXObject {
+class AXInlineTextBox final : public AXObjectImpl {
   WTF_MAKE_NONCOPYABLE(AXInlineTextBox);
 
  private:
@@ -51,16 +51,16 @@ class AXInlineTextBox final : public AXObject {
 
   AccessibilityRole RoleValue() const override { return kInlineTextBoxRole; }
   String GetName(AXNameFrom&,
-                 AXObject::AXObjectVector* name_objects) const override;
+                 AXObjectImpl::AXObjectVector* name_objects) const override;
   void TextCharacterOffsets(Vector<int>&) const override;
   void GetWordBoundaries(Vector<AXRange>&) const override;
-  void GetRelativeBounds(AXObject** out_container,
+  void GetRelativeBounds(AXObjectImpl** out_container,
                          FloatRect& out_bounds_in_container,
                          SkMatrix44& out_container_transform) const override;
-  AXObject* ComputeParent() const override;
+  AXObjectImpl* ComputeParent() const override;
   AccessibilityTextDirection GetTextDirection() const override;
-  AXObject* NextOnLine() const override;
-  AXObject* PreviousOnLine() const override;
+  AXObjectImpl* NextOnLine() const override;
+  AXObjectImpl* PreviousOnLine() const override;
 
  private:
   RefPtr<AbstractInlineTextBox> inline_text_box_;

[sashab, 2017-05-15 02:24:49]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-05-15 02:24:50]

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

[commit-bot: I haz the power, 2017-05-15 02:25:07]

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 04:17:09]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1494815089920500,
"parent_rev": "009667de27f51fbf1877ceea08c037e35f3a5bdf", "commit_rev":
"234813ae8d52f622a1664e11365642751e8f26ba"}

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

Description was changed from

==========
Rename AXObject to AXObjectImpl in modules/ and web/

Rename AXObject to AXObjectImpl in modules/ and web/, and introduce an
empty abstract parent class in core/. This is pre-work to remove usages
of AXObjectImpl from web/, which will use AXObject instead.

This patch does not rename the usages in core/ since these should use
the new abstract object.

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

to

==========
Rename AXObject to AXObjectImpl in modules/ and web/

Rename AXObject to AXObjectImpl in modules/ and web/, and introduce an
empty abstract parent class in core/. This is pre-work to remove usages
of AXObjectImpl from web/, which will use AXObject instead.

This patch does not rename the usages in core/ since these should use
the new abstract object.

BUG=715382

Review-Url: https://codereview.chromium.org/2858493002
Cr-Commit-Position: refs/heads/master@{#471663}
Committed:
https://chromium.googlesource.com/chromium/src/+/234813ae8d52f622a1664e113656...
==========

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

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

[sashab, 2017-05-17 04:17:24]

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:18:26]

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-17 04:18:38]

The CQ bit was checked by sashab@chromium.org

[sashab, 2017-05-17 04:18:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, slangley@chromium.org, dmazzoni@chromium.org,
mkwst@chromium.org, aboxhall@chromium.org
Link to the patchset: https://codereview.chromium.org/2858493002/#ps180001
(title: "Fixed rebase")

[commit-bot: I haz the power, 2017-05-17 04:19:26]

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:16:41]

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

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

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-17 06:36:10]

The CQ bit was checked by sashab@chromium.org

[commit-bot: I haz the power, 2017-05-17 06:36:39]

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 07:31:50]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495002970386140,
"parent_rev": "2ff93adf7ed746c0e945c250e52b8d30adfbf719", "commit_rev":
"0cd996b5bea03c783b7d10062a669da39e2191da"}

[commit-bot: I haz the power, 2017-05-17 07:33:47]

Description was changed from

==========
Rename AXObject to AXObjectImpl in modules/ and web/

Rename AXObject to AXObjectImpl in modules/ and web/, and introduce an
empty abstract parent class in core/. This is pre-work to remove usages
of AXObjectImpl from web/, which will use AXObject instead.

This patch does not rename the usages in core/ since these should use
the new abstract object.

BUG=715382

Review-Url: https://codereview.chromium.org/2858493002
Cr-Commit-Position: refs/heads/master@{#471663}
Committed:
https://chromium.googlesource.com/chromium/src/+/234813ae8d52f622a1664e113656...
==========

to

==========
Rename AXObject to AXObjectImpl in modules/ and web/

Rename AXObject to AXObjectImpl in modules/ and web/, and introduce an
empty abstract parent class in core/. This is pre-work to remove usages
of AXObjectImpl from web/, which will use AXObject instead.

This patch does not rename the usages in core/ since these should use
the new abstract object.

BUG=715382

Review-Url: https://codereview.chromium.org/2858493002
Cr-Original-Commit-Position: refs/heads/master@{#471663}
Committed:
https://chromium.googlesource.com/chromium/src/+/234813ae8d52f622a1664e113656...
Review-Url: https://codereview.chromium.org/2858493002
Cr-Commit-Position: refs/heads/master@{#472370}
Committed:
https://chromium.googlesource.com/chromium/src/+/0cd996b5bea03c783b7d10062a66...
==========

[commit-bot: I haz the power, 2017-05-17 07:33:49]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/0cd996b5bea03c783b7d10062a66...