Disable zoom-in and zoom-out buttons in toolbar menu for a crashed page.

Disable zoom-in and zoom-out buttons in toolbar menu for a crashed page.

BUG=602228
Committed: https://crrev.com/dabf5535ceee6f9b8460fb78489caaffbc764a59
Cr-Commit-Position: refs/heads/master@{#397031}

[Tom (Use chromium acct), 2016-04-12 19:08:28]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[Tom (Use chromium acct), 2016-04-12 19:08:40]

thomasanderson@google.com changed reviewers:
+ pkasting@chromium.org

[commit-bot: I haz the power, 2016-04-12 19:08:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1877373003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1877373003/20001

[commit-bot: I haz the power, 2016-04-12 19:17:13]

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

[commit-bot: I haz the power, 2016-04-12 19:17:14]

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

[Tom (Use chromium acct), 2016-04-12 20:15:12]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-12 20:15:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1877373003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1877373003/20001

[commit-bot: I haz the power, 2016-04-12 20:25:35]

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

[commit-bot: I haz the power, 2016-04-12 20:25:36]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-04-12 23:39:56]

https://codereview.chromium.org/1877373003/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/1877373003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands.cc:581: zoom_controller->GetZoomPercent() !=
contents->GetMaximumZoomPercent();
Nit: Wrong indenting (2 places)

https://codereview.chromium.org/1877373003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu.cc (right):

https://codereview.chromium.org/1877373003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu.cc:674: zoom >
selected_tab->GetMinimumZoomPercent());
It feels wrong that we duplicate this logic both here and in
browser_commands.cc.  Can we put an appropriate function/pair of functions
somewhere both places can call instead?

[Tom (Use chromium acct), 2016-04-14 19:18:43]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-14 19:19:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1877373003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1877373003/40001

[Tom (Use chromium acct), 2016-04-14 19:19:37]

https://codereview.chromium.org/1877373003/diff/20001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/1877373003/diff/20001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands.cc:581: zoom_controller->GetZoomPercent() !=
contents->GetMaximumZoomPercent();
On 2016/04/12 23:39:56, Peter Kasting wrote:
> Nit: Wrong indenting (2 places)

Done.

https://codereview.chromium.org/1877373003/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu.cc (right):

https://codereview.chromium.org/1877373003/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu.cc:674: zoom >
selected_tab->GetMinimumZoomPercent());
On 2016/04/12 23:39:56, Peter Kasting wrote:
> It feels wrong that we duplicate this logic both here and in
> browser_commands.cc.  Can we put an appropriate function/pair of functions
> somewhere both places can call instead?

Done.

https://codereview.chromium.org/1877373003/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands.h (right):

https://codereview.chromium.org/1877373003/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands.h:76: int 
GetZoomPercent(content::WebContents* contents);
Do these four functions that deal with WebContents belong in this file?

[commit-bot: I haz the power, 2016-04-14 19:59:49]

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

[commit-bot: I haz the power, 2016-04-14 19:59:50]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-04-15 00:40:36]

LGTM, but you'll probably end up refactoring again :P

https://codereview.chromium.org/1877373003/diff/40001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_commands.h (right):

https://codereview.chromium.org/1877373003/diff/40001/chrome/browser/ui/brows...
chrome/browser/ui/browser_commands.h:76: int 
GetZoomPercent(content::WebContents* contents);
On 2016/04/14 19:19:36, Tom Anderson wrote:
> Do these four functions that deal with WebContents belong in this file?

I don't think so, at least not for GetZoomPercent().  This is more for functions
that execute a command rather than oracles about whether a command can be
executed.

Since these all deal with Zoom, one possible place to put them is as static
methods on ZoomController.  Note that it already has something very close to
your GetZoomPercent() as GetZoomLevelForWebContents().  It also seems like a lot
of the callers of ZoomController::GetZoomPercent() are starting with a
WebContents* (as you are).  Maybe GetZoomPercent() should be replaced by a
GetZoomPercentForWebContents()?  Or maybe there should be a function to convert
zoom levels to zoom percents, so you can call GetZoomLevelForWebContents() and
then that?  Or (minor variation) maybe it should convert from a zoom factor
(instead of a zoom level) to a zoom percent, and GetZoomLevelForWebContents()
should be changed to return a zoom factor instead (since its lone caller wants
that)?

It's less clear to me where the other functions should live, since e.g. "can
zoom in" checking the current and maximum zoom levels as well as the crashed
state seems like maybe it shouldn't live in the ZoomController, but I'm not
really sure where it should live.

I would consult the components/ui/zoom OWNERS.

[Tom (Use chromium acct), 2016-04-15 03:26:37]

Patchset #4 (id:60001) has been deleted

[Tom (Use chromium acct), 2016-04-15 03:29:19]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-15 03:29:29]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1877373003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1877373003/80001

[commit-bot: I haz the power, 2016-04-15 04:47:07]

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

[commit-bot: I haz the power, 2016-04-15 04:47:08]

Dry run: This issue passed the CQ dry run.

[Tom (Use chromium acct), 2016-04-15 23:52:38]

The CQ bit was checked by thomasanderson@google.com

[Tom (Use chromium acct), 2016-04-15 23:52:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1877373003/#ps80001
(title: "Moved GetZoomPercent() into ZoomController")

[commit-bot: I haz the power, 2016-04-15 23:53:25]

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

[commit-bot: I haz the power, 2016-04-16 00:02:51]

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

[commit-bot: I haz the power, 2016-04-16 00:02:52]

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

[Tom (Use chromium acct), 2016-04-16 02:34:19]

thomasanderson@google.com changed reviewers:
+ dbeam@chromium.org, wjmaclean@chromium.org

[Tom (Use chromium acct), 2016-04-16 02:34:19]

Adding dbeam and wjmaclean for owner's approval

[wjmaclean, 2016-04-18 14:58:40]

Your issue description has nothing but a title, and it's far from obvious as to
what your CL is doing.

I'm going to *guess* that

1) the contents for the crashed-page interstitial doesn't have a ZoomCOntroller,
and

2) the contents for the crashed page only allows zoom of 100%

If (2) is true, I find that somewhat concerning from an accessibility point of
view ...

[Tom (Use chromium acct), 2016-04-18 16:49:20]

On 2016/04/18 14:58:40, wjmaclean wrote:
> Your issue description has nothing but a title, and it's far from obvious as
to
> what your CL is doing.
> 
> I'm going to *guess* that
> 
> 1) the contents for the crashed-page interstitial doesn't have a
ZoomCOntroller,
> and
> 
> 2) the contents for the crashed page only allows zoom of 100%
> 
> If (2) is true, I find that somewhat concerning from an accessibility point of
> view ...

Please take a look at BUG 602228.  You're correct, a crashed page only allows a
zoom of 100%.  This makes the zoom-in and zoom-out buttons in the menu bar
useless because it stays at 100% anyway.

I don't think the "Aw snap" page is implemented as an interstitial  because the
renderer process has crashed and cannot display any pages.  I think the code for
this custom view is in SadTabView.  I guess this means it can't really have a
ZoomController?

[wjmaclean, 2016-04-18 16:51:36]

On 2016/04/18 14:58:40, wjmaclean wrote:
> Your issue description has nothing but a title, and it's far from obvious as
to
> what your CL is doing.
> 
> I'm going to *guess* that
> 
> 1) the contents for the crashed-page interstitial doesn't have a
ZoomCOntroller,
> and
> 
> 2) the contents for the crashed page only allows zoom of 100%
> 
> If (2) is true, I find that somewhat concerning from an accessibility point of
> view ...

I tried out you patch, and when I follow the repro instructions in the
associated bug I don't see anyplace where either web_contents or
ZoomController::FromWebContents(web_contents) is null ... is there some other
case where this happens? It seems the only reason we need to add the new
function to ZoomController is if one of these conditions is true.

[Tom (Use chromium acct), 2016-04-18 16:57:03]

On 2016/04/18 16:51:36, wjmaclean wrote:
> On 2016/04/18 14:58:40, wjmaclean wrote:
> > Your issue description has nothing but a title, and it's far from obvious as
> to
> > what your CL is doing.
> > 
> > I'm going to *guess* that
> > 
> > 1) the contents for the crashed-page interstitial doesn't have a
> ZoomCOntroller,
> > and
> > 
> > 2) the contents for the crashed page only allows zoom of 100%
> > 
> > If (2) is true, I find that somewhat concerning from an accessibility point
of
> > view ...
> 
> I tried out you patch, and when I follow the repro instructions in the
> associated bug I don't see anyplace where either web_contents or
> ZoomController::FromWebContents(web_contents) is null ... is there some other
> case where this happens? It seems the only reason we need to add the new
> function to ZoomController is if one of these conditions is true.

Ok, I'll revert zoom_controller.* and just use
FromWebContents()->GetZoomPercent() instead

[Tom (Use chromium acct), 2016-04-18 17:18:34]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-18 17:19:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1877373003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1877373003/100001

[wjmaclean, 2016-04-18 17:30:36]

On 2016/04/18 17:19:04, commit-bot: I haz the power wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1877373003/100001
> View timeline at
>  https://chromium-cq-status.appspot.com/patch-timeline/1877373003/100001

My next question is: why does a crashed page only allow a zoom level of 100%

I think we need to loop in the accessibility people, as it seems wrong we cannot
zoom this page, and I would like their input before disabling the zoom buttons.

At the absolute minimum, the "Aw snap" page needs to respect the default zoom
level, and ideally it should be fully zoomable.

I'm concerned we're fixing the wrong bug here.

+lpalmaro@

[wjmaclean, 2016-04-18 17:31:11]

Description was changed from

==========
Disable zoom-in and zoom-out buttons in toolbar menu for a crashed page.

BUG=602228
==========

to

==========
Disable zoom-in and zoom-out buttons in toolbar menu for a crashed page.

BUG=602228
==========

[wjmaclean, 2016-04-18 17:31:11]

wjmaclean@chromium.org changed reviewers:
+ lpalmaro@google.com

[commit-bot: I haz the power, 2016-04-18 18:09:13]

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

[commit-bot: I haz the power, 2016-04-18 18:09:14]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-05-31 21:29:53]

so what's the deal here?  can we close this?

[Tom (Use chromium acct), 2016-05-31 21:37:17]

On 2016/05/31 21:29:53, Dan Beam wrote:
> so what's the deal here?  can we close this?

Well, I'm concerned about @wjmaclean's comment about accessibility.  However,
the change itself is ready to submit and has OWNERS approval.

[wjmaclean, 2016-05-31 23:25:27]

On 2016/05/31 21:37:17, Tom Anderson wrote:
> On 2016/05/31 21:29:53, Dan Beam wrote:
> > so what's the deal here?  can we close this?
> 
> Well, I'm concerned about @wjmaclean's comment about accessibility.  However,
> the change itself is ready to submit and has OWNERS approval.

It's probably OK to land this, but my accessibility comment stands: crashed
pages should render at (1) by default the default zoom level, and (2) allow also
a user-settable zoom level. There should be a bug filed for that, since the
correct fix for this is not to just disable the buttons, but to make the buttons
actually work :-)

[Tom (Use chromium acct), 2016-05-31 23:34:34]

The CQ bit was checked by thomasanderson@google.com

[Tom (Use chromium acct), 2016-05-31 23:34:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1877373003/#ps100001
(title: "Revered zoom_controller.*")

[commit-bot: I haz the power, 2016-05-31 23:35:21]

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

[commit-bot: I haz the power, 2016-05-31 23:39:26]

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

[commit-bot: I haz the power, 2016-05-31 23:39:28]

Try jobs failed on following builders:
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Tom (Use chromium acct), 2016-05-31 23:40:28]

On 2016/05/31 23:25:27, wjmaclean wrote:
> On 2016/05/31 21:37:17, Tom Anderson wrote:
> > On 2016/05/31 21:29:53, Dan Beam wrote:
> > > so what's the deal here?  can we close this?
> > 
> > Well, I'm concerned about @wjmaclean's comment about accessibility. 
However,
> > the change itself is ready to submit and has OWNERS approval.
> 
> It's probably OK to land this, but my accessibility comment stands: crashed
> pages should render at (1) by default the default zoom level, and (2) allow
also
> a user-settable zoom level. There should be a bug filed for that, since the
> correct fix for this is not to just disable the buttons, but to make the
buttons
> actually work :-)

Issue created here https://bugs.chromium.org/p/chromium/issues/detail?id=616299

[Tom (Use chromium acct), 2016-06-01 00:48:08]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-01 00:49:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1877373003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1877373003/120001

[commit-bot: I haz the power, 2016-06-01 02:36:55]

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

[commit-bot: I haz the power, 2016-06-01 02:36:56]

Dry run: This issue passed the CQ dry run.

[Tom (Use chromium acct), 2016-06-01 02:39:52]

The CQ bit was checked by thomasanderson@google.com

[Tom (Use chromium acct), 2016-06-01 02:39:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1877373003/#ps120001
(title: "Rebase")

[commit-bot: I haz the power, 2016-06-01 02:40:17]

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

[commit-bot: I haz the power, 2016-06-01 02:44:27]

Description was changed from

==========
Disable zoom-in and zoom-out buttons in toolbar menu for a crashed page.

BUG=602228
==========

to

==========
Disable zoom-in and zoom-out buttons in toolbar menu for a crashed page.

BUG=602228
==========

[commit-bot: I haz the power, 2016-06-01 02:44:29]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2016-06-01 02:46:34]

Description was changed from

==========
Disable zoom-in and zoom-out buttons in toolbar menu for a crashed page.

BUG=602228
==========

to

==========
Disable zoom-in and zoom-out buttons in toolbar menu for a crashed page.

BUG=602228

Committed: https://crrev.com/dabf5535ceee6f9b8460fb78489caaffbc764a59
Cr-Commit-Position: refs/heads/master@{#397031}
==========

[commit-bot: I haz the power, 2016-06-01 02:46:36]

Patchset 6 (id:??) landed as
https://crrev.com/dabf5535ceee6f9b8460fb78489caaffbc764a59
Cr-Commit-Position: refs/heads/master@{#397031}