Allow AccessibilityHostMsg_Notifications messages to be sent from the renderer while it is swapped …

Allow AccessibilityHostMsg_Notifications messages to be sent from the renderer while it is swapped out, to prevent getting stuck

BUG=243847
R=creis@chromium.org

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

[Charlie Reis, 2013-06-13 05:42:17]

Why is this message being sent when swapped out?  I think it might be better to
avoid sending it, rather than risking confusion by having it arrive in a
RenderViewHost that isn't visible.

[dmazzoni, 2013-06-13 05:57:22]

OK, it sounds like renderer_accessibility_complete.cc will have to check
is_swapped_out, and if so, it will have to keep the notifications in the queue
and not try to send the IPC at all.

[dmazzoni, 2013-06-14 20:24:07]

Take a look - does this seem reasonable?

If we're swapped out, this will prevent us from sending
queued notifications. They'll stay queued.

When swapping in, the navigation to the new page ought
to generate some new accessibility events, which will
trigger SendPendingAccessibilityNotifications again.

[Charlie Reis, 2013-06-14 20:53:04]

Yes, this should work.  Is it possible to test this using something similar to
RenderViewImplTest.SendSwapOutACK?

[dmazzoni, 2013-06-19 09:54:47]

On 2013/06/14 20:53:04, creis wrote:
> Yes, this should work.  Is it possible to test this using something similar to
> RenderViewImplTest.SendSwapOutACK?

Good idea, done.

[Charlie Reis, 2013-06-19 15:09:47]

Great, thanks for the test!  LGTM with nits.

https://codereview.chromium.org/16903002/diff/10001/content/renderer/accessib...
File content/renderer/accessibility/renderer_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/16903002/diff/10001/content/renderer/accessib...
content/renderer/accessibility/renderer_accessibility_browsertest.cc:329: //
before sending it. It shouldn't sent the notification while
nit: s/sent/send/

https://codereview.chromium.org/16903002/diff/10001/content/renderer/accessib...
content/renderer/accessibility/renderer_accessibility_browsertest.cc:339:
EXPECT_EQ(NULL, sink_->GetUniqueMessageMatching(
nit: EXPECT_FALSE rather than EXPECT_EQ(NULL
(Consistent with other tests.)

https://codereview.chromium.org/16903002/diff/10001/content/renderer/accessib...
content/renderer/accessibility/renderer_accessibility_browsertest.cc:357:
EXPECT_TRUE(NULL != sink_->GetUniqueMessageMatching(
nit: Don't need "NULL !="

https://codereview.chromium.org/16903002/diff/10001/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/16903002/diff/10001/content/renderer/render_v...
content/renderer/render_view_impl.h:882:
FRIEND_TEST_ALL_PREFIXES(RendererAccessibilityTest,
nit: This should be alphabetized above the RenderViewImplTest ones.

[dmazzoni, 2013-06-19 15:34:21]

https://codereview.chromium.org/16903002/diff/10001/content/renderer/accessib...
File content/renderer/accessibility/renderer_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/16903002/diff/10001/content/renderer/accessib...
content/renderer/accessibility/renderer_accessibility_browsertest.cc:329: //
before sending it. It shouldn't sent the notification while
On 2013/06/19 15:09:47, creis wrote:
> nit: s/sent/send/

Done.

https://codereview.chromium.org/16903002/diff/10001/content/renderer/accessib...
content/renderer/accessibility/renderer_accessibility_browsertest.cc:339:
EXPECT_EQ(NULL, sink_->GetUniqueMessageMatching(
On 2013/06/19 15:09:47, creis wrote:
> nit: EXPECT_FALSE rather than EXPECT_EQ(NULL
> (Consistent with other tests.)

Done.

https://codereview.chromium.org/16903002/diff/10001/content/renderer/accessib...
content/renderer/accessibility/renderer_accessibility_browsertest.cc:357:
EXPECT_TRUE(NULL != sink_->GetUniqueMessageMatching(
On 2013/06/19 15:09:47, creis wrote:
> nit: Don't need "NULL !="

Done.

https://codereview.chromium.org/16903002/diff/10001/content/renderer/render_v...
File content/renderer/render_view_impl.h (right):

https://codereview.chromium.org/16903002/diff/10001/content/renderer/render_v...
content/renderer/render_view_impl.h:882:
FRIEND_TEST_ALL_PREFIXES(RendererAccessibilityTest,
On 2013/06/19 15:09:47, creis wrote:
> nit: This should be alphabetized above the RenderViewImplTest ones.

Done.

[commit-bot: I haz the power, 2013-06-19 15:34:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aboxhall@chromium.org/16903002/20001

[commit-bot: I haz the power, 2013-06-19 15:41:45]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2013-06-20 23:35:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aboxhall@chromium.org/16903002/20001

[dmazzoni, 2013-06-21 06:52:25]

Committed patchset #4 manually as r207738 (presubmit successful).