Show ChromeVox caption panel when spoken feedback is enabled.

Show ChromeVox caption panel when spoken feedback is enabled.

This adds a widget to the top of the screen that shows the ChromeVox
caption panel whenever spoken feedback is enabled. It automatically
waits for the web content to load before showing it to avoid flicker.

Depends on: https://codereview.chromium.org/1277183003/

BUG=420795
Committed: https://crrev.com/94a4f8863e29f28060662ccbee973fe640d08626
Cr-Commit-Position: refs/heads/master@{#359499}

[dmazzoni, 2015-08-07 21:02:19]

dmazzoni@chromium.org changed reviewers:
+ mukai@chromium.org, sadrul@chromium.org

[dmazzoni, 2015-08-07 21:02:19]

Jun and/or Sadrul, I'd like to add a small bit of UI to the top of the screen
when ChromeVox is running. The display is working but I'm having trouble with
two things:

1. On the login screen, the view displays but doesn't seem to get any events.
When logged in, it gets events just fine. Any ideas?

2. How could I steal these pixels away from the available desktop area and move
any overlapping windows out from under it?

Happy to chat if this seems like the wrong direction.

[Jun Mukai, 2015-08-07 21:28:30]

On 2015/08/07 21:02:19, dmazzoni wrote:
> Jun and/or Sadrul, I'd like to add a small bit of UI to the top of the screen
> when ChromeVox is running. The display is working but I'm having trouble with
> two things:
> 
> 1. On the login screen, the view displays but doesn't seem to get any events.
> When logged in, it gets events just fine. Any ideas?
> 
> 2. How could I steal these pixels away from the available desktop area and
move
> any overlapping windows out from under it?
> 
> Happy to chat if this seems like the wrong direction.

For 1, I think login/lock screen has special handling of window layering and
event blocking to ensure any events can arrive to normal windows.  You may want
to specify the parent window from
https://code.google.com/p/chromium/codesearch#chromium/src/ash/shell_window_i...

For 2, I'm not sure but panel window implementation might be related. Search
code under ash/wm, but tdanderson@ knows much more of its details.
# but probably panel window may not work on login screen as well, for the same
reason. In that case you'll have to make changes on ash to do that.

[dmazzoni, 2015-08-07 21:30:14]

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

[dmazzoni, 2015-08-07 21:30:14]

+tdanderson

[dmazzoni, 2015-08-07 21:53:41]

Notes from in-person discussion with Jun Mukai: I'm going to try integrating
this with ash/shelf so that the Shelf knows about this window and
ShelfLayoutManager::UpdateBoundsAndOpacity could take it into account when
calling SetDisplayWorkAreaInsets, but the code for the contents of the window
would still live in chrome/browser/chromeos/accessibility.

[tdanderson, 2015-08-10 18:05:35]

On 2015/08/07 21:53:41, dmazzoni wrote:
> Notes from in-person discussion with Jun Mukai: I'm going to try integrating
> this with ash/shelf so that the Shelf knows about this window and
> ShelfLayoutManager::UpdateBoundsAndOpacity could take it into account when
> calling SetDisplayWorkAreaInsets, but the code for the contents of the window
> would still live in chrome/browser/chromeos/accessibility.

Sorry, but I don't actually have context/knowledge of the
concern you raise in #2, so I won't be of much help here.
sadrul@ should be able to advise better.

[sadrul, 2015-08-10 18:13:50]

On 2015/08/10 18:05:35, tdanderson wrote:
> On 2015/08/07 21:53:41, dmazzoni wrote:
> > Notes from in-person discussion with Jun Mukai: I'm going to try integrating
> > this with ash/shelf so that the Shelf knows about this window and
> > ShelfLayoutManager::UpdateBoundsAndOpacity could take it into account when
> > calling SetDisplayWorkAreaInsets, but the code for the contents of the
window
> > would still live in chrome/browser/chromeos/accessibility.
> 
> Sorry, but I don't actually have context/knowledge of the
> concern you raise in #2, so I won't be of much help here.
> sadrul@ should be able to advise better.

I don't really have a lot of insight here. Making sure the work-area is adjusted
correctly sounds like the right thing to do if we want this accessibility-window
to never overlap the normal windows. But whether that needs to happen in the
shelf code is less obvious to me.

I assume the reason to want to avoid overlaps between the accessibility window
and a 'normal' window (i.e. browser/app window) is so the contents of the window
is not occluded by the accessibility window. With that in mind, it might be
worth considering an approach similar to the find-bar or the status-bar bubbles,
where the bubbles re-position themselves when the content underneath would be
occluded by it.

[dmazzoni, 2015-11-04 23:55:58]

dmazzoni@chromium.org changed reviewers:
- tdanderson@chromium.org

[dmazzoni, 2015-11-04 23:59:31]

PTAL.

I finally got back to this change and basically got it working. I think I came
up with a clean way to make sure this panel window is removed from the available
work area, what do you think?

[Jun Mukai, 2015-11-05 17:38:01]

mukai@chromium.org changed reviewers:
+ oshima@chromium.org

[Jun Mukai, 2015-11-05 17:38:02]

+oshima

https://codereview.chromium.org/1274563004/diff/40001/ash/shell.h
File ash/shell.h (left):

https://codereview.chromium.org/1274563004/diff/40001/ash/shell.h#oldcode278
ash/shell.h:278: // Remove this.
I think removing this TODO wouldn't be a good idea, because your new method does
mostly the same thing as the old one. If it's unreliable as sky mentions, it
would be probably better to talk with him what's the problem and what it should
be (or just keep this TODO for the time being).

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/chromevox_panel.cc (right):

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:22: class
ChromeVoxPanelWebView : public views::WebView {
As I wrote below, OnDeviceScaleFactorChanged() method wouldn't be necessary, and
if you remove that, you could remove this class.

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:35: bool fullscreen_;
not used?

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:75: params.parent =
root_window_;
Please do not create a window directly under the root window, instead choose a
container which it belongs to.
The containers are listed at ash/shell_window_ids, and you can get it through
Shell::GetContainer().

I think kShellWindowId_SettingBubbleContainer would be the one.

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:78:
UpdateWidgetBounds();
IIRC, the initial bounds (set in params) and setting bounds after init is
slightly different. It's better to set the initial bounds.

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:85:
widget_->SetContentsView(web_view);
It's better to implement GetContentsView() rather than setting here.

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:103: return
root_window_;
You don't have to remember the root window.
It's just GetWidget()->GetNativeWindow()->GetRootWindow();

You can still keep this method, but I'd recommend to remove this.

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:111:
UpdateWidgetBounds();
I think you don't have to handle the device scale factor changes, because right
now DSF change causes OnDisplayMetricsChanged() (cc:oshima to confirm).

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:138:
ash::Shell::GetInstance()->UpdateDisplayWorkAreaInsets(root_window_);
This may not work quite well. At this point, the widget instance (and
ChromeVoxPanel) isn't deleted yet, therefore UpdateWidgetBounds() can be called
incorrectly.
Not always AccessibilityManager closes the panel, sometimes it can be closed by
someone else (for example, at shutdown).

I think the following code path would be better:
- implement views::WidgetObserver in AccessibilityManager side, and observe
OnWidgetDestroying()
- clear |vox_panel_| and invoke UpdateDisplayWorkAreaInsets() on
OnWidgetDestroying()

Or
- create a callback on AccessibilityManager(), say, OnVoxPanelClosing()
- override WindowClosing() method here, invoke that method

[dmazzoni, 2015-11-05 20:12:58]

https://codereview.chromium.org/1274563004/diff/40001/ash/shell.h
File ash/shell.h (left):

https://codereview.chromium.org/1274563004/diff/40001/ash/shell.h#oldcode278
ash/shell.h:278: // Remove this.
On 2015/11/05 17:38:01, Jun Mukai wrote:
> I think removing this TODO wouldn't be a good idea, because your new method
does
> mostly the same thing as the old one. If it's unreliable as sky mentions, it
> would be probably better to talk with him what's the problem and what it
should
> be (or just keep this TODO for the time being).

I asked him and he didn't remember why it was there anymore. It's from March
2012.

I suspect that the issues he was facing then have been fixed since then. The
work area bounds seem to be reliable now. I don't think the TODO is helpful if
it doesn't point to a specific bug and nobody remembers exactly how it's
unreliable.

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/chromevox_panel.cc (right):

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:22: class
ChromeVoxPanelWebView : public views::WebView {
On 2015/11/05 17:38:02, Jun Mukai wrote:
> As I wrote below, OnDeviceScaleFactorChanged() method wouldn't be necessary,
and
> if you remove that, you could remove this class.

Done.

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:35: bool fullscreen_;
On 2015/11/05 17:38:02, Jun Mukai wrote:
> not used?

Done.

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:75: params.parent =
root_window_;
On 2015/11/05 17:38:01, Jun Mukai wrote:
> Please do not create a window directly under the root window, instead choose a
> container which it belongs to.
> The containers are listed at ash/shell_window_ids, and you can get it through
> Shell::GetContainer().
> 
> I think kShellWindowId_SettingBubbleContainer would be the one.

Done.

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:78:
UpdateWidgetBounds();
On 2015/11/05 17:38:01, Jun Mukai wrote:
> IIRC, the initial bounds (set in params) and setting bounds after init is
> slightly different. It's better to set the initial bounds.

Done.

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:85:
widget_->SetContentsView(web_view);
On 2015/11/05 17:38:01, Jun Mukai wrote:
> It's better to implement GetContentsView() rather than setting here.

Done.

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:103: return
root_window_;
On 2015/11/05 17:38:01, Jun Mukai wrote:
> You don't have to remember the root window.
> It's just GetWidget()->GetNativeWindow()->GetRootWindow();
> 
> You can still keep this method, but I'd recommend to remove this.

Done.

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:111:
UpdateWidgetBounds();
On 2015/11/05 17:38:01, Jun Mukai wrote:
> I think you don't have to handle the device scale factor changes, because
right
> now DSF change causes OnDisplayMetricsChanged() (cc:oshima to confirm).

Done.

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:138:
ash::Shell::GetInstance()->UpdateDisplayWorkAreaInsets(root_window_);
On 2015/11/05 17:38:02, Jun Mukai wrote:
> This may not work quite well. At this point, the widget instance (and
> ChromeVoxPanel) isn't deleted yet, therefore UpdateWidgetBounds() can be
called
> incorrectly.
> Not always AccessibilityManager closes the panel, sometimes it can be closed
by
> someone else (for example, at shutdown).
> 
> I think the following code path would be better:
> - implement views::WidgetObserver in AccessibilityManager side, and observe
> OnWidgetDestroying()
> - clear |vox_panel_| and invoke UpdateDisplayWorkAreaInsets() on
> OnWidgetDestroying()
> 
> Or
> - create a callback on AccessibilityManager(), say, OnVoxPanelClosing()
> - override WindowClosing() method here, invoke that method

Thanks.

I went with the first approach, observing OnWidgetDestroying.

[oshima, 2015-11-05 23:58:51]

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/accessibility_manager.h (right):

https://codereview.chromium.org/1274563004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/accessibility_manager.h:301:
ChromeVoxPanel* chromevox_panel_;
document ownership

https://codereview.chromium.org/1274563004/diff/80001/ash/accessibility_deleg...
File ash/accessibility_delegate.h (right):

https://codereview.chromium.org/1274563004/diff/80001/ash/accessibility_deleg...
ash/accessibility_delegate.h:11: #include "ui/gfx/geometry/insets.h"
you can use forward decl for this too.

https://codereview.chromium.org/1274563004/diff/80001/ash/accessibility_deleg...
ash/accessibility_delegate.h:99: virtual gfx::Insets
GetWorkAreaInsets(aura::Window* root_window) const = 0;
ComputeWorkAreaInsets (or ComputeIdealWorkareaInsets) to distinguish this from a
method to obtain work area insets.

https://codereview.chromium.org/1274563004/diff/80001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/1274563004/diff/80001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.h:420: gfx::Insets work_area_insets_;
Insets is managed in gfx::Display, and you can obtain it through gfx::Screen.
Please use it instead.

https://codereview.chromium.org/1274563004/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/chromevox_panel.cc (right):

https://codereview.chromium.org/1274563004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:34: 
// content::WebContentsObserver

https://codereview.chromium.org/1274563004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:58:
ChromeVoxPanel::ChromeVoxPanel(Profile* profile) : profile_(profile) {
can this be just a BrowserContext?

https://codereview.chromium.org/1274563004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:72: LOG(ERROR) << "ROOT
initial: " << root_window;
remove this.

https://codereview.chromium.org/1274563004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:144:
GetRootWindow()->bounds().height());
bounds(GetRootWindow()->bounds().size());

[dmazzoni, 2015-11-06 17:28:15]

https://codereview.chromium.org/1274563004/diff/80001/ash/accessibility_deleg...
File ash/accessibility_delegate.h (right):

https://codereview.chromium.org/1274563004/diff/80001/ash/accessibility_deleg...
ash/accessibility_delegate.h:11: #include "ui/gfx/geometry/insets.h"
On 2015/11/05 23:58:50, oshima wrote:
> you can use forward decl for this too.

Done.

https://codereview.chromium.org/1274563004/diff/80001/ash/accessibility_deleg...
ash/accessibility_delegate.h:99: virtual gfx::Insets
GetWorkAreaInsets(aura::Window* root_window) const = 0;
On 2015/11/05 23:58:50, oshima wrote:
> ComputeWorkAreaInsets (or ComputeIdealWorkareaInsets) to distinguish this from
a
> method to obtain work area insets.

Done.

https://codereview.chromium.org/1274563004/diff/80001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/1274563004/diff/80001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.h:420: gfx::Insets work_area_insets_;
On 2015/11/05 23:58:50, oshima wrote:
> Insets is managed in gfx::Display, and you can obtain it through gfx::Screen.
> Please use it instead.

Currently ShelfLayoutManager updates the insets by calling
Shell::SetDisplayWorkAreaInsets, which updates the insets in the gfx::Display.
That works fine when the Shelf is the only place that wants to contribute to the
insets, but it doesn't work when we possibly want to get insets from different
places.

I refactored the code so that the shelf keeps track of its desired insets, and
the accessibility delegate can be queried for its desired insets, and when
anything changes, the Shell computes the new merged insets and updates the
display.

Querying the insets from gfx::Display here would be backwards.

https://codereview.chromium.org/1274563004/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/accessibility/chromevox_panel.cc (right):

https://codereview.chromium.org/1274563004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:34: 
On 2015/11/05 23:58:51, oshima wrote:
> // content::WebContentsObserver

Done.

https://codereview.chromium.org/1274563004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:58:
ChromeVoxPanel::ChromeVoxPanel(Profile* profile) : profile_(profile) {
On 2015/11/05 23:58:50, oshima wrote:
> can this be just a BrowserContext?

Done.

https://codereview.chromium.org/1274563004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:72: LOG(ERROR) << "ROOT
initial: " << root_window;
On 2015/11/05 23:58:51, oshima wrote:
> remove this.

Done.

https://codereview.chromium.org/1274563004/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:144:
GetRootWindow()->bounds().height());
On 2015/11/05 23:58:50, oshima wrote:
> bounds(GetRootWindow()->bounds().size());

Done.

[dmazzoni, 2015-11-06 17:46:38]

Note: I added SetDisplayWorkAreaInsetsForTesting to support existing tests. It
seems to make sense since the focus of the tests that call this involve
asserting what happens when there's a limited work area, not what happens to
cause the work area to change.

[Jun Mukai, 2015-11-06 18:21:04]

https://codereview.chromium.org/1274563004/diff/120001/ash/accessibility_dele...
File ash/accessibility_delegate.h (right):

https://codereview.chromium.org/1274563004/diff/120001/ash/accessibility_dele...
ash/accessibility_delegate.h:103: gfx::Insets* out_insets) const = 0;
Why this comes to an output parameter rather than the return value?
If this is the result of forward-declaration, I think it's better to revive the
old #include "insets.h" and move to the previous design.

https://codereview.chromium.org/1274563004/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_manager.h (right):

https://codereview.chromium.org/1274563004/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.h:308:
ChromeVoxPanelWidgetObserver* chromevox_panel_widget_observer_;
Consider using scoped_ptr. Also you don't have to reveal the class name.
scoped_ptr<views::WidgetObserver> would be fine.

https://codereview.chromium.org/1274563004/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/chromevox_panel.cc (right):

https://codereview.chromium.org/1274563004/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:132: return web_view_;
You don't have to remember |web_view_| as an instance variable. You can simply
create it here, and return it.

[oshima, 2015-11-06 18:21:16]

https://codereview.chromium.org/1274563004/diff/80001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/1274563004/diff/80001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.h:420: gfx::Insets work_area_insets_;
On 2015/11/06 17:28:14, dmazzoni wrote:
> On 2015/11/05 23:58:50, oshima wrote:
> > Insets is managed in gfx::Display, and you can obtain it through
gfx::Screen.
> > Please use it instead.
> 
> Currently ShelfLayoutManager updates the insets by calling
> Shell::SetDisplayWorkAreaInsets, which updates the insets in the gfx::Display.
> That works fine when the Shelf is the only place that wants to contribute to
the
> insets, but it doesn't work when we possibly want to get insets from different
> places.
> 
> I refactored the code so that the shelf keeps track of its desired insets, and
> the accessibility delegate can be queried for its desired insets, and when
> anything changes, the Shell computes the new merged insets and updates the
> display.
> 
> Querying the insets from gfx::Display here would be backwards.

Is there a reason to store this? Looks like you can just union insets when you
set the insets?

https://codereview.chromium.org/1274563004/diff/120001/ash/accessibility_dele...
File ash/accessibility_delegate.h (right):

https://codereview.chromium.org/1274563004/diff/120001/ash/accessibility_dele...
ash/accessibility_delegate.h:103: gfx::Insets* out_insets) const = 0;
just return gfx::Insets as gfx::Insets is small.

[dmazzoni, 2015-11-09 23:02:06]

https://codereview.chromium.org/1274563004/diff/80001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/1274563004/diff/80001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.h:420: gfx::Insets work_area_insets_;
On 2015/11/06 18:21:15, oshima wrote:
> On 2015/11/06 17:28:14, dmazzoni wrote:
> > On 2015/11/05 23:58:50, oshima wrote:
> > > Insets is managed in gfx::Display, and you can obtain it through
> gfx::Screen.
> > > Please use it instead.
> > 
> > Currently ShelfLayoutManager updates the insets by calling
> > Shell::SetDisplayWorkAreaInsets, which updates the insets in the
gfx::Display.
> > That works fine when the Shelf is the only place that wants to contribute to
> the
> > insets, but it doesn't work when we possibly want to get insets from
different
> > places.
> > 
> > I refactored the code so that the shelf keeps track of its desired insets,
and
> > the accessibility delegate can be queried for its desired insets, and when
> > anything changes, the Shell computes the new merged insets and updates the
> > display.
> > 
> > Querying the insets from gfx::Display here would be backwards.
> 
> Is there a reason to store this? Looks like you can just union insets when you
> set the insets?

What if the ChromeVox Panel closes and wants to update the work area? We can't
un-union the insets from the Display to figure out what the shelf insets are.

If we didn't want to keep track of the insets due to the shelf here, I think
what we'd have to do is:

1) Make ShelfLayoutManager responsible for unioning the insets due to the shelf
with the insets due to accessibility - that's fine with me if you prefer, but it
didn't seem as clean to me - shouldn't ShelfLayoutManager be just about the
shelf?

2) When the ChromeVoxPanel closes, the ShelfLayoutManager would have to
recompute its insets by calling CalculateTargetBounds and
UpdateBoundsAndOpacity. It may be fine but glancing through the code I'm a bit
worried about calling that when it happens to be in the middle of a shelf
animation, it looks like it would reset the animation unless I refactored it a
lot.

Maybe there's an easier way I'm not seeing, but if so please explain how a
sequence like this would work:

1. Initially, ChromeVoxPanel is hidden and shelf is hidden
2. Show ChromeVoxPanel
3. Show shelf
4. Hide ChromeVoxPanel - what happens now to recompute the insets?

https://codereview.chromium.org/1274563004/diff/120001/ash/accessibility_dele...
File ash/accessibility_delegate.h (right):

https://codereview.chromium.org/1274563004/diff/120001/ash/accessibility_dele...
ash/accessibility_delegate.h:103: gfx::Insets* out_insets) const = 0;
On 2015/11/06 18:21:15, oshima wrote:
> just return gfx::Insets as gfx::Insets is small.

Done.

https://codereview.chromium.org/1274563004/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_manager.h (right):

https://codereview.chromium.org/1274563004/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.h:308:
ChromeVoxPanelWidgetObserver* chromevox_panel_widget_observer_;
On 2015/11/06 18:21:04, Jun Mukai wrote:
> Consider using scoped_ptr. Also you don't have to reveal the class name.
> scoped_ptr<views::WidgetObserver> would be fine.

Switched to using scoped_ptr, but I can't use scoped_ptr<views::WidgetObserver>
because its destructor is protected.

https://codereview.chromium.org/1274563004/diff/120001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/chromevox_panel.cc (right):

https://codereview.chromium.org/1274563004/diff/120001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:132: return web_view_;
On 2015/11/06 18:21:04, Jun Mukai wrote:
> You don't have to remember |web_view_| as an instance variable. You can simply
> create it here, and return it.

Well, I need the BrowserContext to initialize the web view, so I don't think it
makes any more sense to keep the BrowserContext as an instance variable instead.

I made the instance variable a plan views::View instead. Will that work?

(Also, I know that currently Widget only queries
WidgetDelegate::GetContextView() once on initialization, but this isn't
documented anywhere and it's not at all obvious that it isn't allowed to call it
twice. I glanced through other WidgetDelegates and I couldn't find another
example that creates the view each time it's called.)

[oshima, 2015-11-10 01:47:31]

https://codereview.chromium.org/1274563004/diff/80001/ash/shelf/shelf_layout_...
File ash/shelf/shelf_layout_manager.h (right):

https://codereview.chromium.org/1274563004/diff/80001/ash/shelf/shelf_layout_...
ash/shelf/shelf_layout_manager.h:420: gfx::Insets work_area_insets_;
On 2015/11/09 23:02:06, dmazzoni wrote:
> On 2015/11/06 18:21:15, oshima wrote:
> > On 2015/11/06 17:28:14, dmazzoni wrote:
> > > On 2015/11/05 23:58:50, oshima wrote:
> > > > Insets is managed in gfx::Display, and you can obtain it through
> > gfx::Screen.
> > > > Please use it instead.
> > > 
> > > Currently ShelfLayoutManager updates the insets by calling
> > > Shell::SetDisplayWorkAreaInsets, which updates the insets in the
> gfx::Display.
> > > That works fine when the Shelf is the only place that wants to contribute
to
> > the
> > > insets, but it doesn't work when we possibly want to get insets from
> different
> > > places.
> > > 
> > > I refactored the code so that the shelf keeps track of its desired insets,
> and
> > > the accessibility delegate can be queried for its desired insets, and when
> > > anything changes, the Shell computes the new merged insets and updates the
> > > display.
> > > 
> > > Querying the insets from gfx::Display here would be backwards.
> > 
> > Is there a reason to store this? Looks like you can just union insets when
you
> > set the insets?
> 
> What if the ChromeVox Panel closes and wants to update the work area? We can't
> un-union the insets from the Display to figure out what the shelf insets are.
> 
> If we didn't want to keep track of the insets due to the shelf here, I think
> what we'd have to do is:
> 
> 1) Make ShelfLayoutManager responsible for unioning the insets due to the
shelf
> with the insets due to accessibility - that's fine with me if you prefer, but
it
> didn't seem as clean to me - shouldn't ShelfLayoutManager be just about the
> shelf?
> 
> 2) When the ChromeVoxPanel closes, the ShelfLayoutManager would have to
> recompute its insets by calling CalculateTargetBounds and
> UpdateBoundsAndOpacity. It may be fine but glancing through the code I'm a bit
> worried about calling that when it happens to be in the middle of a shelf
> animation, it looks like it would reset the animation unless I refactored it a
> lot.

There is already another factor (dock window) that affect the size of the
workspace, and ShelfLayoutMmanager is responsible for computing insets now.
Ideally, we should refactor
and move the responsibility to a separate class, but if we dont, then I prefer
not to split the responsibility.

Re: animation, the same animation will be applied since the shelf's state didn't
change, and it will continue where it stopped because it's using
IMMEDIATELY_ANIMATE_TO_NEW_TARGET.

> 
> Maybe there's an easier way I'm not seeing, but if so please explain how a
> sequence like this would work:
> 
> 1. Initially, ChromeVoxPanel is hidden and shelf is hidden
> 2. Show ChromeVoxPanel
> 3. Show shelf
> 4. Hide ChromeVoxPanel - what happens now to recompute the insets?

You can call call ShelfLayoutMananger::LayoutShelf(true /* animate */), in which
you can call AccessibilityDelegate. I'm actually fine if you want to store the
bounds for chromebox panel instead of adding delegate method, if you like.

[dmazzoni, 2015-11-10 18:45:38]

I think I understand what you had in mind now. Take a look, I took out the
delegate function and just put the inset logic inside ShelfLayoutManager.

[oshima, 2015-11-10 19:47:51]

thanks. lgtm if you addressed nits below.

https://codereview.chromium.org/1274563004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/1274563004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:284:
widget_->RemoveObserver(this);
nit: you can just use the widget passed instead?

https://codereview.chromium.org/1274563004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:444:
input_method::InputMethodManager::Get()->RemoveObserver(this);
DCHECK(!chromevox_panel_)?

https://codereview.chromium.org/1274563004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/chromevox_panel.cc (right):

https://codereview.chromium.org/1274563004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:59:
ChromeVoxPanel::ChromeVoxPanel(content::BrowserContext* browser_context) {
initialize fullscreen_. I'd initialize widget_/web_views_ here as well.

[dmazzoni, 2015-11-10 21:55:51]

https://codereview.chromium.org/1274563004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/accessibility_manager.cc (right):

https://codereview.chromium.org/1274563004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:284:
widget_->RemoveObserver(this);
On 2015/11/10 19:47:51, oshima wrote:
> nit: you can just use the widget passed instead?

Done.

https://codereview.chromium.org/1274563004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/accessibility_manager.cc:444:
input_method::InputMethodManager::Get()->RemoveObserver(this);
On 2015/11/10 19:47:51, oshima wrote:
> DCHECK(!chromevox_panel_)?

Done.

https://codereview.chromium.org/1274563004/diff/160001/chrome/browser/chromeo...
File chrome/browser/chromeos/accessibility/chromevox_panel.cc (right):

https://codereview.chromium.org/1274563004/diff/160001/chrome/browser/chromeo...
chrome/browser/chromeos/accessibility/chromevox_panel.cc:59:
ChromeVoxPanel::ChromeVoxPanel(content::BrowserContext* browser_context) {
On 2015/11/10 19:47:51, oshima wrote:
> initialize fullscreen_. I'd initialize widget_/web_views_ here as well.

Done.

[dmazzoni, 2015-11-13 04:58:14]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2015-11-13 04:58:14]

The patchset sent to the CQ was uploaded after l-g-t-m from oshima@chromium.org
Link to the patchset: https://codereview.chromium.org/1274563004/#ps300001
(title: "Don't let ChromeVox Classic run on panel")

[commit-bot: I haz the power, 2015-11-13 04:58:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1274563004/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1274563004/300001

[commit-bot: I haz the power, 2015-11-13 05:47:00]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-11-13 05:47:57]

Patchset 16 (id:??) landed as
https://crrev.com/94a4f8863e29f28060662ccbee973fe640d08626
Cr-Commit-Position: refs/heads/master@{#359499}