Added a system caret used for accessibility on Windows.

Added a system caret used for accessibility on Windows.

Screen magnifiers need to know where the caret is located so that they can show a magnified caret allowing the user to more easily track it.
Existing screen magnifiers, such as the built-in Windows Magnifier, require a system caret to be present in order to track it. However, Chrome doesn't use the system caret.
This is an object that implements the same accessibility interface that would have been returned by the system if the system caret had been used.
R=dmazzoni@chromium.org, shuchen@chromium.org, ellyjones@chromium.org, ananta@chromium.org, sky@chromium.org, sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2852763002
Cr-Original-Original-Original-Commit-Position: refs/heads/master@{#483558}
Committed: https://chromium.googlesource.com/chromium/src/+/3fc0df1e01ab7e17318ce50724890f939bc00fa4
Review-Url: https://codereview.chromium.org/2852763002
Cr-Original-Original-Commit-Position: refs/heads/master@{#483825}
Committed: https://chromium.googlesource.com/chromium/src/+/99936ad737f80a465b479d6bc0abacd22767deaf
Review-Url: https://codereview.chromium.org/2852763002
Cr-Original-Commit-Position: refs/heads/master@{#484167}
Committed: https://chromium.googlesource.com/chromium/src/+/300ba535f4b2e5c8fa8953eacbace7f2e19c9082
Review-Url: https://codereview.chromium.org/2852763002
Cr-Commit-Position: refs/heads/master@{#484603}
Committed: https://chromium.googlesource.com/chromium/src/+/ce13d454d8dc34121c9f93b776d010a9180a8399

[nektarios, 2017-04-28 19:30:12]

Description was changed from

==========
Added a fake caret used for accessibility.

Implementation for Views.
R=dmazzoni@chromium.org
==========

to

==========
Added a fake caret used for accessibility.

Implementation for Views.
R=dmazzoni@chromium.org, shuchen@chromium.org, ellyjones@chromium.org,
sky@chromium.org, sadrul@chromium.org
==========

[nektarios, 2017-04-28 19:30:12]

nektar@chromium.org changed reviewers:
+ ellyjones@chromium.org, sadrul@chromium.org, shuchen@chromium.org,
sky@chromium.org

[nektarios, 2017-04-28 19:31:38]

@shuchen
Please review the small change I made to ui/base/ime/....
@sky and @sadrul
Please review changes to ui/views/....

[dmazzoni, 2017-04-28 21:28:07]

I'm confused about the relationship between this change and
https://codereview.chromium.org/2781613003/ because
this change seems to include a lot of the same code.

If this change is supposed to be based on that other one,
please use git branch --set-upstream-to=other_branch
and upload again to make the dependency explicit.

[chromium-reviews, 2017-05-01 15:52:44]

The only same code I can see is the AXFakeCaret class. I kept this in 
thinking that you might want to test it.
I switched to using upstream.

-- 
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-01 16:00:35]

Can you revert all of the changes to content/ from
this patch? This patch should really be just the
changes to add views support and nothing else

https://codereview.chromium.org/2852763002/diff/40001/chrome/common/extension...
File chrome/common/extensions/api/automation.idl (left):

https://codereview.chromium.org/2852763002/diff/40001/chrome/common/extension...
chrome/common/extensions/api/automation.idl:82: caret,
Is this accidental? I don't think you mean to delete this

https://codereview.chromium.org/2852763002/diff/40001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_android.cc (left):

https://codereview.chromium.org/2852763002/diff/40001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_android.cc:568: case
ui::AX_ROLE_CARET:
Same - revert this change

[dmazzoni, 2017-05-01 16:01:06]

Actually looks like the views code got deleted
somehow

[chromium-reviews, 2017-05-01 18:14:04]

I am really confused now.

It took a long time to revert all the content code and now it's back, 
plus the Views code is deleted?
I just did "git branch --set-upstream-to=previous_branch" and "git pull".
Sigh, I really don't want to deal with this again.

-- 
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-01 18:17:10]

If you don't want to deal with this, just land your first patch and then
fix this one up.

What I would do is switch to the first branch, then create a new branch
based on it, then patch in what you first uploaded, resolve the conflicts,
and then upload.

On Mon, May 1, 2017 at 11:14 AM 'Nektarios Paisios' via Chromium-reviews <
chromium-reviews@chromium.org> wrote:

> I am really confused now.
>
> It took a long time to revert all the content code and now it's back,
> plus the Views code is deleted?
> I just did "git branch --set-upstream-to=previous_branch" and "git pull".
> Sigh, I really don't want to deal with this again.
>
> --
> 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.
>
>

-- 
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.

[chromium-reviews, 2017-05-01 18:26:22]

On 5/1/2017 2:16 PM, Dominic Mazzoni wrote:
> If you don't want to deal with this, just land your first patch and 
> then fix this one up.
>
> What I would do is switch to the first branch, then create a new 
> branch based on it, then patch in what you first uploaded, resolve the 
> conflicts, and then upload.
>

Yes, this is what I am currently trying to do.
I am trying to find out how to download an older patch I uploaded.

-- 
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-01 18:51:30]

To find a git commit that you made locally but then accidentally lost due
to rebasing, you can use git reflog.

From the code review site there are "raw" links to each patch set. I think
you want this one:

https://codereview.chromium.org/download/issue2852763002_20001.diff


On Mon, May 1, 2017 at 11:26 AM 'Nektarios Paisios' via Chromium-reviews <
chromium-reviews@chromium.org> wrote:

> On 5/1/2017 2:16 PM, Dominic Mazzoni wrote:
> > If you don't want to deal with this, just land your first patch and
> > then fix this one up.
> >
> > What I would do is switch to the first branch, then create a new
> > branch based on it, then patch in what you first uploaded, resolve the
> > conflicts, and then upload.
> >
>
> Yes, this is what I am currently trying to do.
> I am trying to find out how to download an older patch I uploaded.
>
> --
> 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.
>
>

-- 
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.

[nektarios, 2017-05-01 18:52:58]

FYI, the diff on my local computer looked fine but the diff on the codereview
site is the one messed up.
It seems that "git branch --set-upstream-to=older_branch" messed up everything.
As soon as I removed the upstream, the diff on the codereview site looks fine.
Please have a look.

[dmazzoni, 2017-05-01 19:50:34]

Setting the upstream assumes the branches share the same ancestry. It
sounds like the branches had diverged and that's why it was looking that
way.


On Mon, May 1, 2017 at 11:53 AM <nektar@chromium.org> wrote:

> FYI, the diff on my local computer looked fine but the diff on the
> codereview
> site is the one messed up.
> It seems that "git branch --set-upstream-to=older_branch" messed up
> everything.
> As soon as I removed the upstream, the diff on the codereview site looks
> fine.
> Please have a look.
>
> https://codereview.chromium.org/2852763002/
>

-- 
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-01 19:50:38]

Looks ready to review now, thanks

[dmazzoni, 2017-05-01 19:50:41]

Looks good now


On Mon, May 1, 2017 at 12:50 PM Dominic Mazzoni <dmazzoni@chromium.org>
wrote:

> Setting the upstream assumes the branches share the same ancestry. It
> sounds like the branches had diverged and that's why it was looking that
> way.
>
>
> On Mon, May 1, 2017 at 11:53 AM <nektar@chromium.org> wrote:
>
>> FYI, the diff on my local computer looked fine but the diff on the
>> codereview
>> site is the one messed up.
>> It seems that "git branch --set-upstream-to=older_branch" messed up
>> everything.
>> As soon as I removed the upstream, the diff on the codereview site looks
>> fine.
>> Please have a look.
>>
>> https://codereview.chromium.org/2852763002/
>>
>

-- 
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-01 19:54:20]

https://codereview.chromium.org/2852763002/diff/80001/ui/views/win/hwnd_messa...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/2852763002/diff/80001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:973:
delegate_->RemoveInputMethodObserver(*this);
I don't think this is right. This will be called
frequently, every time focus moves away from a text
box. I think you shouldn't remove the observer here.

https://codereview.chromium.org/2852763002/diff/80001/ui/views/win/hwnd_messa...
ui/views/win/hwnd_message_handler.cc:1562: base::win::ScopedComPtr<IAccessible>
fake_caret_accessible =
I think you should only do this if the window has focus

[chromium-reviews, 2017-05-01 20:07:43]

ttps://codereview.chromium.org/2852763002/diff/80001/ui/views/win/hwnd_message_handler.cc#newcode1562

<https://codereview.chromium.org/2852763002/diff/80001/ui/views/win/hwnd_messa...>
> ui/views/win/hwnd_message_handler.cc:1562:
> base::win::ScopedComPtr<IAccessible> fake_caret_accessible =
> I think you should only do this if the window has focus

If the window doesn't have focus, isn't the caret still visible?
What if I have two Chrome windows side by side on the screen. Does the 
caret disappear from the one when I focus the other?

-- 
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-01 20:12:35]

Yes, the caret disappears from a window when that window loses focus. Just
like only one thing on the screen can have input focus, there can only be
one active caret.

On Mon, May 1, 2017 at 1:07 PM Nektarios Paisios <nektar@google.com> wrote:

>
>
ttps://codereview.chromium.org/2852763002/diff/80001/ui/views/win/hwnd_message_handler.cc#newcode1562
>
<https://codereview.chromium.org/2852763002/diff/80001/ui/views/win/hwnd_messa...>
>
> ui/views/win/hwnd_message_handler.cc:1562:
> base::win::ScopedComPtr<IAccessible> fake_caret_accessible =
> I think you should only do this if the window has focus
>
>
> If the window doesn't have focus, isn't the caret still visible?
> What if I have two Chrome windows side by side on the screen. Does the
> caret disappear from the one when I focus the other?
>
>

-- 
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.

[chromium-reviews, 2017-05-01 23:39:45]

https://codereview.chromium.org/2852763002/diff/80001/ui/views/win/hwnd_messa...
> File ui/views/win/hwnd_message_handler.cc (right):
>
>
https://codereview.chromium.org/2852763002/diff/80001/ui/views/win/hwnd_messa...
> ui/views/win/hwnd_message_handler.cc:973:
> delegate_->RemoveInputMethodObserver(*this);
> I don't think this is right. This will be called
> frequently, every time focus moves away from a text
> box. I think you shouldn't remove the observer here.
>
Removed.

>
https://codereview.chromium.org/2852763002/diff/80001/ui/views/win/hwnd_messa...
> ui/views/win/hwnd_message_handler.cc:1562:
> base::win::ScopedComPtr<IAccessible> fake_caret_accessible =
> I think you should only do this if the window has focus
Added call to IsActive().

-- 
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:55:23]

lgtm with just one last issue

https://codereview.chromium.org/2852763002/diff/100001/ui/views/win/hwnd_mess...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/2852763002/diff/100001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:1560: } else if (IsActive() &&
ax_fake_caret_ &&
IsActive won't work. Only top-level windows can be active, but the legacy hwnd
is a
child window. If there isn't already IsFocused(), you could just check if
GetFocus() == hwnd()

[chromium-reviews, 2017-05-02 20:42:34]

https://codereview.chromium.org/2852763002/diff/100001/ui/views/win/hwnd_mess...
> File ui/views/win/hwnd_message_handler.cc (right):
>
>
https://codereview.chromium.org/2852763002/diff/100001/ui/views/win/hwnd_mess...
> ui/views/win/hwnd_message_handler.cc:1560: } else if (IsActive() &&
> ax_fake_caret_ &&
> IsActive won't work. Only top-level windows can be active, but the
> legacy hwnd is a
> child window. If there isn't already IsFocused(), you could just check
> if GetFocus() == hwnd()

Done.

-- 
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.

[sky, 2017-05-03 03:34:50]

Please update the description to indicate what a 'fake caret' is. Also, please
add test coveage.

https://codereview.chromium.org/2852763002/diff/120001/ui/views/win/hwnd_mess...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/2852763002/diff/120001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:940: ax_fake_caret_ = nullptr;
Why do you need to explicitly destroy the caret? Please document that here.

https://codereview.chromium.org/2852763002/diff/120001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:955: DCHECK(ax_fake_caret_);
Move DCHECK closer to where it matters, line 961.

https://codereview.chromium.org/2852763002/diff/120001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:961:
ax_fake_caret_->MoveCaretTo(caret_bounds);
If these bounds are in screen coordinates how are they updated when the HWND
moves?

https://codereview.chromium.org/2852763002/diff/120001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:973: ax_fake_caret_ = nullptr;
Remove observer?

https://codereview.chromium.org/2852763002/diff/120001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:1565: fake_caret_accessible.Detach());
How is this associated with lifetime of ax_fake_caret?

https://codereview.chromium.org/2852763002/diff/120001/ui/views/win/hwnd_mess...
File ui/views/win/hwnd_message_handler_delegate.h (right):

https://codereview.chromium.org/2852763002/diff/120001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler_delegate.h:38: virtual void
AddInputMethodObserver(ui::InputMethodObserver& observer) = 0;
Generally observers are in terms of pointers, so please pass in a pointer here
(and below).

[dmazzoni, 2017-06-15 06:00:28]

Test code looks pretty good, just a couple of thoughts

https://codereview.chromium.org/2852763002/diff/160001/ui/accessibility/platf...
File ui/accessibility/platform/ax_fake_caret_win_interactive_uitest.cc (right):

https://codereview.chromium.org/2852763002/diff/160001/ui/accessibility/platf...
ui/accessibility/platform/ax_fake_caret_win_interactive_uitest.cc:35:
gl::GLSurfaceTestSupport::InitializeOneOff();
Why this line?

https://codereview.chromium.org/2852763002/diff/160001/ui/accessibility/platf...
ui/accessibility/platform/ax_fake_caret_win_interactive_uitest.cc:39:
ResourceBundle::InitSharedInstanceWithPakPath(ui_test_pak_path);
Are these lines needed? Why?

https://codereview.chromium.org/2852763002/diff/160001/ui/accessibility/platf...
ui/accessibility/platform/ax_fake_caret_win_interactive_uitest.cc:85:
caret_position = textfield_test_api.GetCursorViewOrigin();
It might be nice to call this caret_position2 and assert that it's not the same
as caret_position1, that way you can feel confident that the
MOVE_TO_END_OF_DOCUMENT
worked correctly. Otherwise in theory we could break the code that updates the
fake
caret and the test would still pass if the initial location was right

[nektarios, 2017-06-16 16:19:32]

Description was changed from

==========
Added a fake caret used for accessibility.

Implementation for Views.
R=dmazzoni@chromium.org, shuchen@chromium.org, ellyjones@chromium.org,
sky@chromium.org, sadrul@chromium.org
==========

to

==========
Added a fake caret used for accessibility.

Screen magnifiers need to know where the caret is located so that they can show
a magnified caret allowing the user to more easily track it.
Existing screen magnifiers, such as the built-in Windows Magnifier, require a
system caret to be present in order to track it. However, Chrome doesn't use the
system caret.
This is a fake caret that implements the same accessibility interface that would
have been returned by the system if the system caret had been used.
R=dmazzoni@chromium.org, shuchen@chromium.org, ellyjones@chromium.org,
sky@chromium.org, sadrul@chromium.org
==========

[nektarios, 2017-06-16 16:22:39]

On 2017/06/15 at 06:00:28, dmazzoni wrote:
> Test code looks pretty good, just a couple of thoughts
> 
>
https://codereview.chromium.org/2852763002/diff/160001/ui/accessibility/platf...
> File ui/accessibility/platform/ax_fake_caret_win_interactive_uitest.cc
(right):
> 
>
https://codereview.chromium.org/2852763002/diff/160001/ui/accessibility/platf...
> ui/accessibility/platform/ax_fake_caret_win_interactive_uitest.cc:35:
gl::GLSurfaceTestSupport::InitializeOneOff();
> Why this line?

It turns on software graphics rendering. Apparently, this creates the same test
environment on all bots, but I don't know all the details. Locally, tests pass
if I remove it, but I suspect it's required for the tests to pass on all bots.

> 
>
https://codereview.chromium.org/2852763002/diff/160001/ui/accessibility/platf...
> ui/accessibility/platform/ax_fake_caret_win_interactive_uitest.cc:39:
ResourceBundle::InitSharedInstanceWithPakPath(ui_test_pak_path);
> Are these lines needed? Why?

Yes, tests fail if these lines are not present. They allow tests to have access
to their data. Something to do with file bundles not being able to be found.

> 
>
https://codereview.chromium.org/2852763002/diff/160001/ui/accessibility/platf...
> ui/accessibility/platform/ax_fake_caret_win_interactive_uitest.cc:85:
caret_position = textfield_test_api.GetCursorViewOrigin();
> It might be nice to call this caret_position2 and assert that it's not the
same
> as caret_position1, that way you can feel confident that the
MOVE_TO_END_OF_DOCUMENT
> worked correctly. Otherwise in theory we could break the code that updates the
fake
> caret and the test would still pass if the initial location was right

Done.

[chromium-reviews, 2017-06-16 16:30:09]

Please update the description to indicate what a 'fake caret' is. Also, 
please
> add test coveage.
>
Done.

>
>
https://codereview.chromium.org/2852763002/diff/120001/ui/views/win/hwnd_mess...
> File ui/views/win/hwnd_message_handler.cc (right):
>
>
https://codereview.chromium.org/2852763002/diff/120001/ui/views/win/hwnd_mess...
> ui/views/win/hwnd_message_handler.cc:940: ax_fake_caret_ = nullptr;
> Why do you need to explicitly destroy the caret? Please document that
> here.
>
It's just for making sure that the same caret will not be used again. 
Unlikely but better to be safe.

>
https://codereview.chromium.org/2852763002/diff/120001/ui/views/win/hwnd_mess...
> ui/views/win/hwnd_message_handler.cc:955: DCHECK(ax_fake_caret_);
> Move DCHECK closer to where it matters, line 961.
>
Removed and dynamically created caret if not present.

>
https://codereview.chromium.org/2852763002/diff/120001/ui/views/win/hwnd_mess...
> ui/views/win/hwnd_message_handler.cc:961:
> ax_fake_caret_->MoveCaretTo(caret_bounds);
> If these bounds are in screen coordinates how are they updated when the
> HWND moves?
>
Done. I destroy the caret when the hwnd moves. I don't know how to 
trigger a repositioning of the fake caret, but it will be repositioned 
next time the user clicks in an edit field or uses the cursor keys to move.

>
https://codereview.chromium.org/2852763002/diff/120001/ui/views/win/hwnd_mess...
> ui/views/win/hwnd_message_handler.cc:973: ax_fake_caret_ = nullptr;
> Remove observer?
>
Why, I thought that the input method is re-created when moving to 
another input control.

>
https://codereview.chromium.org/2852763002/diff/120001/ui/views/win/hwnd_mess...
> ui/views/win/hwnd_message_handler.cc:1565:
> fake_caret_accessible.Detach());
> How is this associated with lifetime of ax_fake_caret?
>
We are handing out a COM interface to AXFakeCaret. If the AXFakeCaret is 
destructed, any calls to methods on that interface will return an error 
code. See |AXPlatformNodeWin::accLocation|.

>
https://codereview.chromium.org/2852763002/diff/120001/ui/views/win/hwnd_mess...
> File ui/views/win/hwnd_message_handler_delegate.h (right):
>
>
https://codereview.chromium.org/2852763002/diff/120001/ui/views/win/hwnd_mess...
> ui/views/win/hwnd_message_handler_delegate.h:38: virtual void
> AddInputMethodObserver(ui::InputMethodObserver& observer) = 0;
> Generally observers are in terms of pointers, so please pass in a
> pointer here (and below).
>
Done.

-- 
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-06-16 16:30:42]

lgtm

I see now. The stuff in SetUp is normally done for all
browser tests or UI tests, but because you're inheriting
from WidgetTest you needed to copy it directly to your test.

What do you need from WidgetTest? I wonder if it'd work
to inherit from one of the Browser tests base classes
instead and copy some code from WidgetTest's SetUp
instead?

Anyway, no concerns there, it at least makes sense.

[chromium-reviews, 2017-06-16 16:31:16]

@sky
I apologize for the extremely long delay in updating this patch.
Please have another look.

-- 
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.

[Shu Chen, 2017-06-17 09:02:58]

lgtm for ui/base/ime/...

[sky, 2017-06-19 15:00:15]

Thanks for updating the description to indicate what a 'fake care' is. The name
FakeCaret does not convey at all what it is used for. Did you consider something
like SystemCaretBridge? That name readily conveys what it is used for.

https://codereview.chromium.org/2852763002/diff/200001/ui/accessibility/platf...
File ui/accessibility/platform/ax_fake_caret_win_interactive_uitest.cc (right):

https://codereview.chromium.org/2852763002/diff/200001/ui/accessibility/platf...
ui/accessibility/platform/ax_fake_caret_win_interactive_uitest.cc:32: class
AXFakeCaretWinTest : public views::test::WidgetTest {
How come you put this code here rather than in views? Isn't this testing views
code?

https://codereview.chromium.org/2852763002/diff/200001/ui/accessibility/platf...
ui/accessibility/platform/ax_fake_caret_win_interactive_uitest.cc:67:
views::Widget* widget_;
Initialize widget_ and textfield_ in the member initializer.

https://codereview.chromium.org/2852763002/diff/200001/ui/views/win/hwnd_mess...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/2852763002/diff/200001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:939:
delegate_->RemoveInputMethodObserver(this);
Why do you need to remove the observer here? More specifically, can it be
removed from the destructor?

https://codereview.chromium.org/2852763002/diff/200001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:940: DestroyFakeCaret();
Similarly why does the fake care need to be destroyed here, why not the
destructor?

https://codereview.chromium.org/2852763002/diff/200001/ui/views/win/hwnd_mess...
File ui/views/win/hwnd_message_handler.h (right):

https://codereview.chromium.org/2852763002/diff/200001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.h:236: void OnFocus() override {}
Don't inline virtual functions like this in the header (see style guide).

https://codereview.chromium.org/2852763002/diff/200001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.h:582: // Destructs the fake caret used for
accessibility. This will result in any
Destructs -> Deletes

https://codereview.chromium.org/2852763002/diff/200001/ui/views/win/hwnd_mess...
File ui/views/win/hwnd_message_handler_delegate.h (right):

https://codereview.chromium.org/2852763002/diff/200001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler_delegate.h:38: virtual void
AddInputMethodObserver(ui::InputMethodObserver* observer) = 0;
How about exposing the InputMethod instead, e.g.
GetHWNDMessageDelegateInputMethod().

[chromium-reviews, 2017-06-19 16:04:55]

On 6/19/2017 11:00 AM, sky@chromium.org wrote:
> Thanks for updating the description to indicate what a 'fake care' is. 
> The name
> FakeCaret does not convey at all what it is used for. Did you consider 
> something
> like SystemCaretBridge? That name readily conveys what it is used for.
>
I apologize, English is not my first language and even though to me AX 
fake caret sounds reasonable, I am open to considering better names.
I chose AXFakeCaret because:
A) It's used for accessibility purposes.
B) It's not a real caret: it's not visible on screen and it doesn't 
behave like a system caret. You can't call SetCaret or something for 
example.
> C) It tracks Chrome's internal caret.

If I use SystemCaretBridge doesn't it imply that it would behave like 
the system caret?
This class only implements the accessibility APIs on Windows that are 
used by screen magnifiers to track the caret. It doesn't implement all 
APIs that the system caret might respond to.


>
https://codereview.chromium.org/2852763002/diff/200001/ui/accessibility/platf...
> File ui/accessibility/platform/ax_fake_caret_win_interactive_uitest.cc
> (right):
>
>
https://codereview.chromium.org/2852763002/diff/200001/ui/accessibility/platf...
> ui/accessibility/platform/ax_fake_caret_win_interactive_uitest.cc:32:
> class AXFakeCaretWinTest : public views::test::WidgetTest {
> How come you put this code here rather than in views? Isn't this testing
> views code?
>
It's testing if AXFakeCaret works in Views. I prefer to have all 
accessibility related tests in the same place. If I am looking for the 
tests that test fake caret functionality, would I think to go looking 
under views/win?
This is not a unit test that strictly goes with the class it tests but 
an interactive UI test.

>
https://codereview.chromium.org/2852763002/diff/200001/ui/accessibility/platf...
> ui/accessibility/platform/ax_fake_caret_win_interactive_uitest.cc:67:
> views::Widget* widget_;
> Initialize widget_ and textfield_ in the member initializer.
>
Isn't it safer to do it after I called
views::test::WidgetTest::SetUp();
Even if SetUP in the base class doesn't do anything interesting for now, 
somebody might update it in the future.
My concern is that I am calling |CreateNativeDesktopWidget| to create 
the widget. If I call that before SetUp methods from all the base 
classes run, isn't it risky?

>
https://codereview.chromium.org/2852763002/diff/200001/ui/views/win/hwnd_mess...
> File ui/views/win/hwnd_message_handler.cc (right):
>
>
https://codereview.chromium.org/2852763002/diff/200001/ui/views/win/hwnd_mess...
> ui/views/win/hwnd_message_handler.cc:939:
> delegate_->RemoveInputMethodObserver(this);
> Why do you need to remove the observer here? More specifically, can it
> be removed from the destructor?
>
Done.

>
https://codereview.chromium.org/2852763002/diff/200001/ui/views/win/hwnd_mess...
> ui/views/win/hwnd_message_handler.cc:940: DestroyFakeCaret();
> Similarly why does the fake care need to be destroyed here, why not the
> destructor?
>
Done.

>
https://codereview.chromium.org/2852763002/diff/200001/ui/views/win/hwnd_mess...
> File ui/views/win/hwnd_message_handler.h (right):
>
>
https://codereview.chromium.org/2852763002/diff/200001/ui/views/win/hwnd_mess...
> ui/views/win/hwnd_message_handler.h:236: void OnFocus() override {}
> Don't inline virtual functions like this in the header (see style
> guide).
>
Done.

>
https://codereview.chromium.org/2852763002/diff/200001/ui/views/win/hwnd_mess...
> ui/views/win/hwnd_message_handler.h:582: // Destructs the fake caret
> used for accessibility. This will result in any
> Destructs -> Deletes
>
Done.

>
https://codereview.chromium.org/2852763002/diff/200001/ui/views/win/hwnd_mess...
> File ui/views/win/hwnd_message_handler_delegate.h (right):
>
>
https://codereview.chromium.org/2852763002/diff/200001/ui/views/win/hwnd_mess...
> ui/views/win/hwnd_message_handler_delegate.h:38: virtual void
> AddInputMethodObserver(ui::InputMethodObserver* observer) = 0;
> How about exposing the InputMethod instead, e.g.
> GetHWNDMessageDelegateInputMethod().

Done.

-- 
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-06-19 19:37:24]

On 2017/06/19 16:04:55, chromium-reviews wrote:
> On 6/19/2017 11:00 AM, mailto:sky@chromium.org wrote:
> > Thanks for updating the description to indicate what a 'fake care' is. 
> > The name
> > FakeCaret does not convey at all what it is used for. Did you consider 
> > something
> > like SystemCaretBridge? That name readily conveys what it is used for.
> >
> I apologize, English is not my first language and even though to me AX 
> fake caret sounds reasonable, I am open to considering better names.

Let's call it AXSystemCaretWin.

Throughout the Chrome code, the AX prefix already implies that an object
is an implementation of accessibility interfaces for some other object,
for example AXRenderFrame for RenderFrame, or AXLayoutObject for LayoutObject.
In this case, it's the accessible interface for the system caret on Windows.

I can see Scott's objection in that it's not really fake - the caret
is real, it's just custom-drawn, so we need a custom accessible interface
for it rather than getting the default one from Windows for free.
But that's not really different than any other accessible objects
we create.

[chromium-reviews, 2017-06-19 19:45:45]

Sure. I can use AXSystemCaretWin.
I'll wait to see if Scott agrees with the name too, before making the 
change.

-- 
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.

[sky, 2017-06-19 21:10:08]

That names SGTM.



On Mon, Jun 19, 2017 at 12:45 PM, Nektarios Paisios <nektar@google.com>
wrote:

> Sure. I can use AXSystemCaretWin.
> I'll wait to see if Scott agrees with the name too, before making the
> change.
>
>

-- 
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.

[nektarios, 2017-06-20 15:00:25]

Description was changed from

==========
Added a fake caret used for accessibility.

Screen magnifiers need to know where the caret is located so that they can show
a magnified caret allowing the user to more easily track it.
Existing screen magnifiers, such as the built-in Windows Magnifier, require a
system caret to be present in order to track it. However, Chrome doesn't use the
system caret.
This is a fake caret that implements the same accessibility interface that would
have been returned by the system if the system caret had been used.
R=dmazzoni@chromium.org, shuchen@chromium.org, ellyjones@chromium.org,
sky@chromium.org, sadrul@chromium.org
==========

to

==========
Added a system caret used for accessibility on Windows.

Screen magnifiers need to know where the caret is located so that they can show
a magnified caret allowing the user to more easily track it.
Existing screen magnifiers, such as the built-in Windows Magnifier, require a
system caret to be present in order to track it. However, Chrome doesn't use the
system caret.
This is an object that implements the same accessibility interface that would
have been returned by the system if the system caret had been used.
R=dmazzoni@chromium.org, shuchen@chromium.org, ellyjones@chromium.org,
sky@chromium.org, sadrul@chromium.org
==========

[nektarios, 2017-06-20 15:01:12]

I used AXSystemCaretWin all throughout and modified the description to include
the new name.

[nektarios, 2017-06-20 15:02:31]

Description was changed from

==========
Added a system caret used for accessibility on Windows.

Screen magnifiers need to know where the caret is located so that they can show
a magnified caret allowing the user to more easily track it.
Existing screen magnifiers, such as the built-in Windows Magnifier, require a
system caret to be present in order to track it. However, Chrome doesn't use the
system caret.
This is an object that implements the same accessibility interface that would
have been returned by the system if the system caret had been used.
R=dmazzoni@chromium.org, shuchen@chromium.org, ellyjones@chromium.org,
sky@chromium.org, sadrul@chromium.org
==========

to

==========
Added a system caret used for accessibility on Windows.

Screen magnifiers need to know where the caret is located so that they can show
a magnified caret allowing the user to more easily track it.
Existing screen magnifiers, such as the built-in Windows Magnifier, require a
system caret to be present in order to track it. However, Chrome doesn't use the
system caret.
This is an object that implements the same accessibility interface that would
have been returned by the system if the system caret had been used.
R=dmazzoni@chromium.org, shuchen@chromium.org, ellyjones@chromium.org,
ananta@chromium.org, sky@chromium.org, sadrul@chromium.org
==========

[nektarios, 2017-06-20 15:02:31]

nektar@chromium.org changed reviewers:
+ ananta@chromium.org

[nektarios, 2017-06-20 15:03:51]

@ananta
Please approve a small change to legacy_render_widget_host_win, renaming
AXFakeCaretWin to AXSystemCaretWin.

[sky, 2017-06-20 18:28:40]

Thanks for the improved names. Only one question.

https://codereview.chromium.org/2852763002/diff/300001/ui/accessibility/platf...
File ui/accessibility/platform/ax_system_caret_win.h (right):

https://codereview.chromium.org/2852763002/diff/300001/ui/accessibility/platf...
ui/accessibility/platform/ax_system_caret_win.h:29:
AXSystemCaretWin(gfx::AcceleratedWidget event_target);
explicit

https://codereview.chromium.org/2852763002/diff/300001/ui/views/win/hwnd_mess...
File ui/views/win/hwnd_message_handler.cc (right):

https://codereview.chromium.org/2852763002/diff/300001/ui/views/win/hwnd_mess...
ui/views/win/hwnd_message_handler.cc:363: DestroyAXSystemCaret();
Is this needed? ax_system_caret_ is a scoped pointer, so why do you need the
explicit destroy?

[chromium-reviews, 2017-06-20 23:15:21]

https://codereview.chromium.org/2852763002/diff/300001/ui/accessibility/platf...
> File ui/accessibility/platform/ax_system_caret_win.h (right):
>
>
https://codereview.chromium.org/2852763002/diff/300001/ui/accessibility/platf...
> ui/accessibility/platform/ax_system_caret_win.h:29:
> AXSystemCaretWin(gfx::AcceleratedWidget event_target);
> explicit
>
Done.

>
https://codereview.chromium.org/2852763002/diff/300001/ui/views/win/hwnd_mess...
> File ui/views/win/hwnd_message_handler.cc (right):
>
>
https://codereview.chromium.org/2852763002/diff/300001/ui/views/win/hwnd_mess...
> ui/views/win/hwnd_message_handler.cc:363: DestroyAXSystemCaret();
> Is this needed? ax_system_caret_ is a scoped pointer, so why do you need
> the explicit destroy?

Sorry removed. While designing this, on destruction I used to set the 
caret to location (0, 0) so that if clients are still holding onto it 
they would get an invalid location, but now they would get a COM error 
code which is cleaner.

-- 
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.

[nektarios, 2017-06-20 23:16:39]

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

[commit-bot: I haz the power, 2017-06-20 23:17:00]

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-06-20 23:21:01]

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

[commit-bot: I haz the power, 2017-06-20 23:21:02]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[sky, 2017-06-21 00:42:12]

LGTM

[ananta, 2017-06-26 17:52:06]

lgtm

[nektarios, 2017-06-27 01:12:42]

@dominic and @sky
I am sorry but the tests I wrote started failing. It seems that sometimes a test
would crash because GL is being initialized multiple times and they are hitting
a DCHECK.
I had to add a test suite that will turn on GL only at the start of all tests.
Could you review files
ui/accessibility/platform/ax_system_caret_win_interactive_ui_tests.cc and
ui/accessibility/build.gn?

[nektarios, 2017-06-27 01:13:00]

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

[commit-bot: I haz the power, 2017-06-27 01:13:21]

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-06-27 01:17:05]

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

[commit-bot: I haz the power, 2017-06-27 01:17:06]

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...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[nektarios, 2017-06-27 01:33:10]

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

[commit-bot: I haz the power, 2017-06-27 01:33:33]

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-06-27 01:37:02]

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

[commit-bot: I haz the power, 2017-06-27 01:37:03]

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

[nektarios, 2017-06-27 01:53:50]

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

[commit-bot: I haz the power, 2017-06-27 01:54:05]

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-06-27 02:12:05]

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

[commit-bot: I haz the power, 2017-06-27 02:12:07]

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

[nektarios, 2017-06-27 02:54:58]

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

[commit-bot: I haz the power, 2017-06-27 02:55:13]

Dry run: CQ is trying da patch.

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

[sky, 2017-06-27 03:35:13]

On 2017/06/27 01:12:42, nektarios wrote:
> @dominic and @sky
> I am sorry but the tests I wrote started failing. It seems that sometimes a
test
> would crash because GL is being initialized multiple times and they are
hitting
> a DCHECK.
> I had to add a test suite that will turn on GL only at the start of all tests.
> Could you review files
> ui/accessibility/platform/ax_system_caret_win_interactive_ui_tests.cc and
> ui/accessibility/build.gn?

Do the accessibility unit tests run serially on bots that are logged on? I
suspect not and that you should be putting your test with all the other
interactive ui tests. That is, not in the accessibility unit tests. Dominic
should know for sure though.

[commit-bot: I haz the power, 2017-06-27 04:05:01]

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

[commit-bot: I haz the power, 2017-06-27 04:05:03]

Dry run: This issue passed the CQ dry run.

[chromium-reviews, 2017-06-27 15:14:59]

Do the accessibility unit tests run serially on bots that are logged on? I
> suspect not and that you should be putting your test with all the other
> interactive ui tests. That is, not in the accessibility unit tests. 
> Dominic
> should know for sure though.

That's what I did in ui/accessibility/build.gn:
I added a new test target called accessibility_interactive_ui_tests and 
I added the supporting components ui/gl/test_support and 
base/tes/test_support, etc.
I removed the use of base/test/run_all_unit_tests.cc and associate 
component.
I don't know if there is anything else I would need to do, but looking 
through ui/views/build.gn for its interactive_ui_tests targets, I don't 
see anything that I might have missed.
However, this is my first time doing this.

-- 
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-06-27 15:27:25]

A new test target seems overkill. The interactive_ui_tests are different in that
the bots run them
differently, without parallelism.

My suggestion would be to just move the test to chrome/browser/accessibility and
then
add it to the existing interactive_ui_tests build rule in chrome/test/BUILD.gn

[dmazzoni, 2017-06-27 15:28:42]

https://codereview.chromium.org/2852763002/diff/400001/ui/accessibility/BUILD.gn
File ui/accessibility/BUILD.gn (right):

https://codereview.chromium.org/2852763002/diff/400001/ui/accessibility/BUILD...
ui/accessibility/BUILD.gn:160: test("accessibility_interactive_ui_tests") {
This would require extra work because it won't even be run on the
bots unless we update all of their configs.

Much better to just use an existing test suite.

[chromium-reviews, 2017-06-27 16:58:30]

I added the tests to the build rule under 
chrome/test/build.gn:interactive_ui_tests and moved the test file to 
ui/views/accessibility/ as it would be easier to find under views than 
chrome/browser/accessibility.
There are other views tests that have rules under chrome/test/build.gn.
However, the test now complains that GL is not initialized.
It's surprising because other Views interactive uitests should need GL 
too and they seem to run without complaining.

-- 
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.

[nektarios, 2017-06-27 16:58:38]

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

[commit-bot: I haz the power, 2017-06-27 16:58:54]

Dry run: CQ is trying da patch.

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

[dmazzoni, 2017-06-27 17:06:55]

Can you pinpoint where GL is needed, like a stack tracek when the GL is not
initialized error happens?

On Tue, Jun 27, 2017 at 9:58 AM 'Nektarios Paisios' via Chromium-reviews <
chromium-reviews@chromium.org> wrote:

> I added the tests to the build rule under
> chrome/test/build.gn:interactive_ui_tests and moved the test file to
> ui/views/accessibility/ as it would be easier to find under views than
> chrome/browser/accessibility.
> There are other views tests that have rules under chrome/test/build.gn.
> However, the test now complains that GL is not initialized.
> It's surprising because other Views interactive uitests should need GL
> too and they seem to run without complaining.
>
> --
> 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.
>
>

-- 
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.

[chromium-reviews, 2017-06-27 17:24:07]

On 6/27/2017 1:06 PM, Dominic Mazzoni wrote:
> Can you pinpoint where GL is needed, like a stack tracek when the GL 
> is not initialized error happens?
>

It's on my local machine a DCHECK in gl_test_support.cc. However, let's 
wait for the bots. They might run fine after all.

-- 
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-06-27 19:20:01]

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

[commit-bot: I haz the power, 2017-06-27 19:20:03]

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

[sky, 2017-06-27 21:29:55]

It's not just a new test target, it's a new test target would need to run
serially. I agree with Dominic here; add the test to chrome
interactive_ui_tests. This isn't ideal from a layering perspective, but
it's the pattern we've taken with similar tests.

  -Scott

On Tue, Jun 27, 2017 at 8:27 AM, <dmazzoni@chromium.org> wrote:

> A new test target seems overkill. The interactive_ui_tests are different
> in that
> the bots run them
> differently, without parallelism.
>
> My suggestion would be to just move the test to
> chrome/browser/accessibility and
> then
> add it to the existing interactive_ui_tests build rule in
> chrome/test/BUILD.gn
>
>
> https://codereview.chromium.org/2852763002/
>

-- 
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.

[nektarios, 2017-06-27 22:12:31]

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

[commit-bot: I haz the power, 2017-06-27 22:12:56]

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-06-27 23:22:20]

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

[commit-bot: I haz the power, 2017-06-27 23:22:22]

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/...)

[nektarios, 2017-06-28 00:20:28]

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

[commit-bot: I haz the power, 2017-06-28 00:21:15]

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-06-28 01:16:48]

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

[commit-bot: I haz the power, 2017-06-28 01:16:50]

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

[nektarios, 2017-06-28 01:32:58]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2017-06-28 01:32:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, shuchen@chromium.org, sky@chromium.org,
ananta@chromium.org
Link to the patchset: https://codereview.chromium.org/2852763002/#ps460001
(title: "Added a system caret for accessibility on Windows.")

[commit-bot: I haz the power, 2017-06-28 01:33: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-06-28 02:29:20]

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

[commit-bot: I haz the power, 2017-06-28 02:29:22]

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/...)

[nektarios, 2017-06-28 03:13:41]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2017-06-28 03:13:42]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
shuchen@chromium.org, ananta@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2852763002/#ps480001
(title: "Added a system caret for accessibility on Windows.")

[commit-bot: I haz the power, 2017-06-28 03:14:00]

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-06-28 03:38:42]

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

[commit-bot: I haz the power, 2017-06-28 03:38:44]

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_...)

[nektarios, 2017-06-28 15:13:53]

The CQ bit was checked by nektar@chromium.org

[commit-bot: I haz the power, 2017-06-28 15:14: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-06-28 15:59:12]

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

[commit-bot: I haz the power, 2017-06-28 15:59:14]

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_...)

[nektarios, 2017-06-28 19:50:09]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2017-06-28 19:50:09]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
shuchen@chromium.org, ananta@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2852763002/#ps500001
(title: "Added a system caret for accessibility on Windows.")

[commit-bot: I haz the power, 2017-06-28 19:50:42]

CQ is trying da patch.

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

[nektarios, 2017-06-28 19:58:00]

The CQ bit was unchecked by nektar@chromium.org

[nektarios, 2017-06-28 19:59:49]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2017-06-28 19:59:49]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
shuchen@chromium.org, ananta@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2852763002/#ps520001
(title: "Added a system caret for accessibility on Windows.")

[commit-bot: I haz the power, 2017-06-28 20:00: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-06-28 21:30:28]

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

[commit-bot: I haz the power, 2017-06-28 21:30:30]

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_...)

[nektarios, 2017-06-28 22:04:44]

The CQ bit was checked by nektar@chromium.org

[commit-bot: I haz the power, 2017-06-28 22:05:27]

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-06-29 01:06:21]

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

[commit-bot: I haz the power, 2017-06-29 01:06:23]

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_...)

[nektarios, 2017-06-29 18:21:26]

The CQ bit was checked by nektar@chromium.org

[commit-bot: I haz the power, 2017-06-29 18:21:59]

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-06-29 21:09:26]

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

[commit-bot: I haz the power, 2017-06-29 21:09:28]

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_...)

[nektarios, 2017-06-29 22:59:27]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2017-06-29 22:59:27]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
shuchen@chromium.org, ananta@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2852763002/#ps540001
(title: "Added a system caret for accessibility on Windows.")

[commit-bot: I haz the power, 2017-06-29 22:59:43]

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-06-30 00:17:23]

CQ is committing da patch.

Bot data: {"patchset_id": 540001, "attempt_start_ts": 1498777167486290,
"parent_rev": "91ecb9e41fcb5ae18e059bdaff4469244bbebb12", "commit_rev":
"3fc0df1e01ab7e17318ce50724890f939bc00fa4"}

[commit-bot: I haz the power, 2017-06-30 00:17:42]

Description was changed from

==========
Added a system caret used for accessibility on Windows.

Screen magnifiers need to know where the caret is located so that they can show
a magnified caret allowing the user to more easily track it.
Existing screen magnifiers, such as the built-in Windows Magnifier, require a
system caret to be present in order to track it. However, Chrome doesn't use the
system caret.
This is an object that implements the same accessibility interface that would
have been returned by the system if the system caret had been used.
R=dmazzoni@chromium.org, shuchen@chromium.org, ellyjones@chromium.org,
ananta@chromium.org, sky@chromium.org, sadrul@chromium.org
==========

to

==========
Added a system caret used for accessibility on Windows.

Screen magnifiers need to know where the caret is located so that they can show
a magnified caret allowing the user to more easily track it.
Existing screen magnifiers, such as the built-in Windows Magnifier, require a
system caret to be present in order to track it. However, Chrome doesn't use the
system caret.
This is an object that implements the same accessibility interface that would
have been returned by the system if the system caret had been used.
R=dmazzoni@chromium.org, shuchen@chromium.org, ellyjones@chromium.org,
ananta@chromium.org, sky@chromium.org, sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2852763002
Cr-Commit-Position: refs/heads/master@{#483558}
Committed:
https://chromium.googlesource.com/chromium/src/+/3fc0df1e01ab7e17318ce5072489...
==========

[commit-bot: I haz the power, 2017-06-30 00:17:45]

Committed patchset #28 (id:540001) as
https://chromium.googlesource.com/chromium/src/+/3fc0df1e01ab7e17318ce5072489...

[Timothy Loh, 2017-06-30 03:08:24]

A revert of this CL (patchset #28 id:540001) has been created in
https://codereview.chromium.org/2962273002/ by timloh@chromium.org.

The reason for reverting is: The added test
AXSystemCaretWinTest.TestMovingWindow is failing on the bots, e.g.
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2....

[nektarios, 2017-06-30 16:38:17]

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

[commit-bot: I haz the power, 2017-06-30 16:38:36]

Dry run: CQ is trying da patch.

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

[nektarios, 2017-06-30 16:44:23]

Reland:
Interactive test that moved the window using mouse and then tested that
AXSystemCaretWin got invalidated was intermiddently failing on bots.
I don't know how to reliably simulate moving a window using the mouse.
Internally, moving the window happens by sending a WM_MOVE message via
SendMessage system API call.
This API is blocking, so there is no runloop to wait on.
Perhaps base::GetCurrentLoop().WaitUntilIdle() or something similar would have
done the trict but to be on the safe side I decided to simply resize the window
using
Widget::SetBounds(...)
I know that this doesn't test all the code added to HWNDMessageHandler.cc but it
might suffice?

[dmazzoni, 2017-06-30 16:56:46]

Can you clarify what code in HWNDMessageHandler was covered by the test before
that's not covered now?

I don't see where HWNDMessageHandler cares whether the window was moved by
the mouse or for some other reason...

[commit-bot: I haz the power, 2017-06-30 17:52:46]

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

[commit-bot: I haz the power, 2017-06-30 17:52:48]

Dry run: This issue passed the CQ dry run.

[chromium-reviews, 2017-06-30 19:31:17]

On 6/30/2017 12:56 PM, dmazzoni@chromium.org wrote:
> Can you clarify what code in HWNDMessageHandler was covered by the 
> test before
> that's not covered now?

Look at HWNDMessageHandler.h
     CR_MSG_WM_SYSCOMMAND(OnSysCommand)
I added code to destroy the caret in HWNDMessageHandler ::OnSysCommand. 
The OnSysCommand function is only called when the appropriate message is 
sent by the operating system. I think in the following cases:
The user pressed Maximize, minimize or activates a menu item.
Chrome detected that the user is dragging the window via the mouse or 
touch and has sent the (WM_MOVE | WM_DRAG_MOVE) message to the window.

I guess I could try sending the maximize message.
Okay, test updated.

-- 
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.

[nektarios, 2017-06-30 19:34:40]

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

[commit-bot: I haz the power, 2017-06-30 19:35:15]

Dry run: CQ is trying da patch.

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

[dmazzoni, 2017-06-30 19:57:29]

I guess I'm wondering if that's really the right place to be watching for
those changes anyway. What happens if a user presses Chrome's fullscreen
shortcut, for example, which resizes / moves the window - does that trigger
a windows event? Or what if JavaScript is used to open a pop-up window and
then it gets moved? All of those things are possible.

I'd say that in terms of priorities, those are less important because for
sure the caret location will still get updated as soon as anything else
changes. But probably the proper place to add these hooks is somewhere else
- maybe any time a widget moves, or any time an aura window moves, for
example. Also, I wonder if maybe the right solution is to notify IME that
the text caret moved and have the IME change trigger the fake caret change.

For now: land the change, with a robust test, maybe with a TODO to check
for window movement somewhere other than WM_SYSCOMMAND.


On Fri, Jun 30, 2017 at 12:31 PM Nektarios Paisios <nektar@google.com>
wrote:

> On 6/30/2017 12:56 PM, dmazzoni@chromium.org wrote:
>
> Can you clarify what code in HWNDMessageHandler was covered by the test
> before
> that's not covered now?
>
>
> Look at HWNDMessageHandler.h
>     CR_MSG_WM_SYSCOMMAND(OnSysCommand)
> I added code to destroy the caret in HWNDMessageHandler ::OnSysCommand.
> The OnSysCommand function is only called when the appropriate message is
> sent by the operating system. I think in the following cases:
> The user pressed Maximize, minimize or activates a menu item.
> Chrome detected that the user is dragging the window via the mouse or
> touch and has sent the (WM_MOVE | WM_DRAG_MOVE) message to the window.
>
> I guess I could try sending the maximize message.
> Okay, test updated.
>
>

-- 
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.

[chromium-reviews, 2017-06-30 20:05:03]

I'd say that in terms of priorities, those are less important because 
for sure the caret location will still get updated as soon as anything 
else changes. But probably the proper place to add these hooks is 
somewhere else - maybe any time a widget moves, or any time an aura 
window moves, for example. Also, I wonder if maybe the right solution is 
to notify IME that the text caret moved and have the IME change trigger 
the fake caret change.

That was my original idea, to change InputMethod to fire the event that 
the caret moved any time the window moved, but I don't know the IME code 
very well.

> For now: land the change, with a robust test, maybe with a TODO to 
> check for window movement somewhere other than WM_SYSCOMMAND.

I'll file a bug instead. We already got some reports that caret tracking 
has some issues in WebView, so I'll need to work on this more.
I'll need to find someone who can explain the Aura, Views and widget 
code to me.


-- 
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-06-30 21:01:18]

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

[commit-bot: I haz the power, 2017-06-30 21:01:20]

Dry run: This issue passed the CQ dry run.

[nektarios, 2017-06-30 21:01:40]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2017-06-30 21:01:41]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
shuchen@chromium.org, ananta@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2852763002/#ps580001
(title: "Added a test for maximizing the window.")

[commit-bot: I haz the power, 2017-06-30 21:01:58]

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-06-30 21:22:28]

CQ is committing da patch.

Bot data: {"patchset_id": 580001, "attempt_start_ts": 1498856500609490,
"parent_rev": "f3c32d8c2da3969d676bc6255224d779bda01ed2", "commit_rev":
"d724ab99b09476d1683885026050b1923ceab9a6"}

[commit-bot: I haz the power, 2017-06-30 21:22:49]

CQ is committing da patch.

Bot data: {"patchset_id": 580001, "attempt_start_ts": 1498856500609490,
"parent_rev": "c5aeb440d84ce37b5c8a1d8f5bc185479b325665", "commit_rev":
"99936ad737f80a465b479d6bc0abacd22767deaf"}

[commit-bot: I haz the power, 2017-06-30 21:23:07]

Description was changed from

==========
Added a system caret used for accessibility on Windows.

Screen magnifiers need to know where the caret is located so that they can show
a magnified caret allowing the user to more easily track it.
Existing screen magnifiers, such as the built-in Windows Magnifier, require a
system caret to be present in order to track it. However, Chrome doesn't use the
system caret.
This is an object that implements the same accessibility interface that would
have been returned by the system if the system caret had been used.
R=dmazzoni@chromium.org, shuchen@chromium.org, ellyjones@chromium.org,
ananta@chromium.org, sky@chromium.org, sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2852763002
Cr-Commit-Position: refs/heads/master@{#483558}
Committed:
https://chromium.googlesource.com/chromium/src/+/3fc0df1e01ab7e17318ce5072489...
==========

to

==========
Added a system caret used for accessibility on Windows.

Screen magnifiers need to know where the caret is located so that they can show
a magnified caret allowing the user to more easily track it.
Existing screen magnifiers, such as the built-in Windows Magnifier, require a
system caret to be present in order to track it. However, Chrome doesn't use the
system caret.
This is an object that implements the same accessibility interface that would
have been returned by the system if the system caret had been used.
R=dmazzoni@chromium.org, shuchen@chromium.org, ellyjones@chromium.org,
ananta@chromium.org, sky@chromium.org, sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2852763002
Cr-Original-Commit-Position: refs/heads/master@{#483558}
Committed:
https://chromium.googlesource.com/chromium/src/+/3fc0df1e01ab7e17318ce5072489...
Review-Url: https://codereview.chromium.org/2852763002
Cr-Commit-Position: refs/heads/master@{#483825}
Committed:
https://chromium.googlesource.com/chromium/src/+/99936ad737f80a465b479d6bc0ab...
==========

[commit-bot: I haz the power, 2017-06-30 21:23:10]

Committed patchset #30 (id:580001) as
https://chromium.googlesource.com/chromium/src/+/99936ad737f80a465b479d6bc0ab...

[foolip, 2017-07-03 08:33:15]

A revert of this CL (patchset #30 id:580001) has been created in
https://codereview.chromium.org/2965933002/ by foolip@chromium.org.

The reason for reverting is: AXSystemCaretWinTest.TestMovingWindow is flaky.

BUG=738617.

[nektarios, 2017-07-04 22:42:53]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2017-07-04 22:42:53]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
shuchen@chromium.org, ananta@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2852763002/#ps600001
(title: "Disabled flaky test.")

[commit-bot: I haz the power, 2017-07-04 22:43:08]

CQ is trying da patch.

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

[nektarios, 2017-07-04 22:46:14]

I am disabling the test and re-landing. The functionality implemented by this
patch is somewhat of a high priority as it is requested by one of our partners
and I would rather debug the failing test case in an isolated patch.

[nektarios, 2017-07-04 22:46:19]

I am disabling the test and re-landing. The functionality implemented by this
patch is somewhat of a high priority as it is requested by one of our partners
and I would rather debug the failing test case in an isolated patch.

[commit-bot: I haz the power, 2017-07-04 23:57:43]

CQ is committing da patch.

Bot data: {"patchset_id": 600001, "attempt_start_ts": 1499208173290510,
"parent_rev": "7fbc0e1b8bd5cbba085337a41b1efdb8734fab8a", "commit_rev":
"300ba535f4b2e5c8fa8953eacbace7f2e19c9082"}

[commit-bot: I haz the power, 2017-07-04 23:58:02]

Description was changed from

==========
Added a system caret used for accessibility on Windows.

Screen magnifiers need to know where the caret is located so that they can show
a magnified caret allowing the user to more easily track it.
Existing screen magnifiers, such as the built-in Windows Magnifier, require a
system caret to be present in order to track it. However, Chrome doesn't use the
system caret.
This is an object that implements the same accessibility interface that would
have been returned by the system if the system caret had been used.
R=dmazzoni@chromium.org, shuchen@chromium.org, ellyjones@chromium.org,
ananta@chromium.org, sky@chromium.org, sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2852763002
Cr-Original-Commit-Position: refs/heads/master@{#483558}
Committed:
https://chromium.googlesource.com/chromium/src/+/3fc0df1e01ab7e17318ce5072489...
Review-Url: https://codereview.chromium.org/2852763002
Cr-Commit-Position: refs/heads/master@{#483825}
Committed:
https://chromium.googlesource.com/chromium/src/+/99936ad737f80a465b479d6bc0ab...
==========

to

==========
Added a system caret used for accessibility on Windows.

Screen magnifiers need to know where the caret is located so that they can show
a magnified caret allowing the user to more easily track it.
Existing screen magnifiers, such as the built-in Windows Magnifier, require a
system caret to be present in order to track it. However, Chrome doesn't use the
system caret.
This is an object that implements the same accessibility interface that would
have been returned by the system if the system caret had been used.
R=dmazzoni@chromium.org, shuchen@chromium.org, ellyjones@chromium.org,
ananta@chromium.org, sky@chromium.org, sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2852763002
Cr-Original-Original-Commit-Position: refs/heads/master@{#483558}
Committed:
https://chromium.googlesource.com/chromium/src/+/3fc0df1e01ab7e17318ce5072489...
Review-Url: https://codereview.chromium.org/2852763002
Cr-Original-Commit-Position: refs/heads/master@{#483825}
Committed:
https://chromium.googlesource.com/chromium/src/+/99936ad737f80a465b479d6bc0ab...
Review-Url: https://codereview.chromium.org/2852763002
Cr-Commit-Position: refs/heads/master@{#484167}
Committed:
https://chromium.googlesource.com/chromium/src/+/300ba535f4b2e5c8fa8953eacbac...
==========

[commit-bot: I haz the power, 2017-07-04 23:58:05]

Committed patchset #31 (id:600001) as
https://chromium.googlesource.com/chromium/src/+/300ba535f4b2e5c8fa8953eacbac...

[Elly Fong-Jones, 2017-07-06 12:39:49]

A revert of this CL (patchset #31 id:600001) has been created in
https://codereview.chromium.org/2967373003/ by ellyjones@chromium.org.

The reason for reverting is: The following tests are flaky:

AXSystemCaretWinTest.TestOnInputTypeChangeInTextField
AXSystemCaretWinTest.TestOnCaretBoundsChangeInTextField

see bugs:

https://crbug.com/739682
https://crbug.com/739683.

[nektarios, 2017-07-06 14:19:23]

The CQ bit was checked by nektar@chromium.org

[nektarios, 2017-07-06 14:19:24]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org,
shuchen@chromium.org, ananta@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/2852763002/#ps620001
(title: "Disabled all tests")

[commit-bot: I haz the power, 2017-07-06 14:19: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-07-06 15:29:19]

CQ is committing da patch.

Bot data: {"patchset_id": 620001, "attempt_start_ts": 1499350763669030,
"parent_rev": "ca13ce668a72df97d03286c7e7d9e8a4e7c79ec9", "commit_rev":
"ce13d454d8dc34121c9f93b776d010a9180a8399"}

[commit-bot: I haz the power, 2017-07-06 15:29:34]

Description was changed from

==========
Added a system caret used for accessibility on Windows.

Screen magnifiers need to know where the caret is located so that they can show
a magnified caret allowing the user to more easily track it.
Existing screen magnifiers, such as the built-in Windows Magnifier, require a
system caret to be present in order to track it. However, Chrome doesn't use the
system caret.
This is an object that implements the same accessibility interface that would
have been returned by the system if the system caret had been used.
R=dmazzoni@chromium.org, shuchen@chromium.org, ellyjones@chromium.org,
ananta@chromium.org, sky@chromium.org, sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2852763002
Cr-Original-Original-Commit-Position: refs/heads/master@{#483558}
Committed:
https://chromium.googlesource.com/chromium/src/+/3fc0df1e01ab7e17318ce5072489...
Review-Url: https://codereview.chromium.org/2852763002
Cr-Original-Commit-Position: refs/heads/master@{#483825}
Committed:
https://chromium.googlesource.com/chromium/src/+/99936ad737f80a465b479d6bc0ab...
Review-Url: https://codereview.chromium.org/2852763002
Cr-Commit-Position: refs/heads/master@{#484167}
Committed:
https://chromium.googlesource.com/chromium/src/+/300ba535f4b2e5c8fa8953eacbac...
==========

to

==========
Added a system caret used for accessibility on Windows.

Screen magnifiers need to know where the caret is located so that they can show
a magnified caret allowing the user to more easily track it.
Existing screen magnifiers, such as the built-in Windows Magnifier, require a
system caret to be present in order to track it. However, Chrome doesn't use the
system caret.
This is an object that implements the same accessibility interface that would
have been returned by the system if the system caret had been used.
R=dmazzoni@chromium.org, shuchen@chromium.org, ellyjones@chromium.org,
ananta@chromium.org, sky@chromium.org, sadrul@chromium.org

Review-Url: https://codereview.chromium.org/2852763002
Cr-Original-Original-Original-Commit-Position: refs/heads/master@{#483558}
Committed:
https://chromium.googlesource.com/chromium/src/+/3fc0df1e01ab7e17318ce5072489...
Review-Url: https://codereview.chromium.org/2852763002
Cr-Original-Original-Commit-Position: refs/heads/master@{#483825}
Committed:
https://chromium.googlesource.com/chromium/src/+/99936ad737f80a465b479d6bc0ab...
Review-Url: https://codereview.chromium.org/2852763002
Cr-Original-Commit-Position: refs/heads/master@{#484167}
Committed:
https://chromium.googlesource.com/chromium/src/+/300ba535f4b2e5c8fa8953eacbac...
Review-Url: https://codereview.chromium.org/2852763002
Cr-Commit-Position: refs/heads/master@{#484603}
Committed:
https://chromium.googlesource.com/chromium/src/+/ce13d454d8dc34121c9f93b776d0...
==========

[commit-bot: I haz the power, 2017-07-06 15:29:36]

Committed patchset #32 (id:620001) as
https://chromium.googlesource.com/chromium/src/+/ce13d454d8dc34121c9f93b776d0...

[sky, 2017-07-11 19:19:18]

On 2017/06/30 16:44:23, nektarios wrote:
> Reland:
> Interactive test that moved the window using mouse and then tested that
> AXSystemCaretWin got invalidated was intermiddently failing on bots.
> I don't know how to reliably simulate moving a window using the mouse.
> Internally, moving the window happens by sending a WM_MOVE message via
> SendMessage system API call.
> This API is blocking, so there is no runloop to wait on.
> Perhaps base::GetCurrentLoop().WaitUntilIdle() or something similar would have
> done the trict but to be on the safe side I decided to simply resize the
window
> using
> Widget::SetBounds(...)
> I know that this doesn't test all the code added to HWNDMessageHandler.cc but
it
> might suffice?

See
https://chromium.googlesource.com/chromium/src/+/master/chrome/test/base/inte...
for functions that can be used to move the move the mouse and wait for it to
take.