Make optional the checking for an element being enabled before clicking it.

Some controls do not have a disabled property. Skip checking for disabled before clicking them.

BUG=415683
Committed: https://crrev.com/725e8d6eb1e5be6ad2130d815ebc172d4dee078e
Cr-Commit-Position: refs/heads/master@{#295833}

[anandc, 2014-09-18 19:10:20]

anandc@chromium.org changed reviewers:
+ jamiewalch@chromium.org

[anandc, 2014-09-18 19:10:20]

Jamie, PTAL. Thanks.

Tested on a local Linux desktop that, with these changes, the ConnectToLocalHost
browser-test passes.

[anandc, 2014-09-18 19:11:45]

Patchset #1 (id:1) has been deleted

[Jamie, 2014-09-18 23:24:42]

I think you can simplify this. Since the only element that's causing a problem
is the tool-bar stub, you can just skip clicking on it altogether. Clicking the
stub just adjusts the CSS "top" property so shouldn't affect whether or not the
button responds to click().

If you do need to disable the check for elements that don't have a "disabled"
property, it would be better to check for the existence of that property, rather
than having the caller decide whether or not to check. If a test needs to click
a button that's disabled, it suggests that whatever functionality is handled by
that button would be better exposed directly via a JS API.

[anandc, 2014-09-19 19:18:45]

On 2014/09/18 23:24:42, Jamie wrote:
> I think you can simplify this. Since the only element that's causing a problem
> is the tool-bar stub, you can just skip clicking on it altogether. Clicking
the
> stub just adjusts the CSS "top" property so shouldn't affect whether or not
the
> button responds to click().
> 
> If you do need to disable the check for elements that don't have a "disabled"
> property, it would be better to check for the existence of that property,
rather
> than having the caller decide whether or not to check. If a test needs to
click
> a button that's disabled, it suggests that whatever functionality is handled
by
> that button would be better exposed directly via a JS API.

Thanks Jamie.
Went with the recommendation in your 2nd paragraph: check for existence of the
'disabled' property.
PTAL.

[anandc, 2014-09-19 19:20:42]

Patchset #2 (id:40001) has been deleted

[Jamie, 2014-09-19 20:29:45]

LGTM with nits.

https://codereview.chromium.org/582833003/diff/60001/chrome/test/remoting/rem...
File chrome/test/remoting/remote_desktop_browsertest.cc (right):

https://codereview.chromium.org/582833003/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_desktop_browsertest.cc:353: // Assuming the
toolbar-stub is always enabled, click on it.
This comment is no longer needed.

https://codereview.chromium.org/582833003/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_desktop_browsertest.cc:711: std::string
check_for_disabled =
Maybe call this has_disabled_attribute?

https://codereview.chromium.org/582833003/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_desktop_browsertest.cc:712:
"document.getElementById(\"" + name + "\").hasAttribute('disabled')";
Nit: Use single quotes for JS code (it also means you don't need to escape them
here).

[anandc, 2014-09-19 22:34:26]

Addressed last round of comments. Thanks.

https://codereview.chromium.org/582833003/diff/60001/chrome/test/remoting/rem...
File chrome/test/remoting/remote_desktop_browsertest.cc (right):

https://codereview.chromium.org/582833003/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_desktop_browsertest.cc:353: // Assuming the
toolbar-stub is always enabled, click on it.
On 2014/09/19 20:29:45, Jamie wrote:
> This comment is no longer needed.

Done.

https://codereview.chromium.org/582833003/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_desktop_browsertest.cc:711: std::string
check_for_disabled =
On 2014/09/19 20:29:45, Jamie wrote:
> Maybe call this has_disabled_attribute?

Ah, but that takes the line beyond 80 chars. :-)
Done.

https://codereview.chromium.org/582833003/diff/60001/chrome/test/remoting/rem...
chrome/test/remoting/remote_desktop_browsertest.cc:712:
"document.getElementById(\"" + name + "\").hasAttribute('disabled')";
On 2014/09/19 20:29:45, Jamie wrote:
> Nit: Use single quotes for JS code (it also means you don't need to escape
them
> here).

Done.

[anandc, 2014-09-19 22:34:52]

The CQ bit was checked by anandc@chromium.org

[commit-bot: I haz the power, 2014-09-19 22:36:23]

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

[Jamie, 2014-09-19 22:37:24]

lgtm

https://codereview.chromium.org/582833003/diff/80001/chrome/test/remoting/rem...
File chrome/test/remoting/remote_desktop_browsertest.cc (right):

https://codereview.chromium.org/582833003/diff/80001/chrome/test/remoting/rem...
chrome/test/remoting/remote_desktop_browsertest.cc:714: has_disabled_attribute))
{
Nit: Indent the second and subsequent parameters to align with the first.

[anandc, 2014-09-19 22:40:36]

The CQ bit was unchecked by anandc@chromium.org

[anandc, 2014-09-19 22:45:05]

Thanks. Unchecked the commit box, and updated the alignment.

Going to re-check commit next.

https://codereview.chromium.org/582833003/diff/80001/chrome/test/remoting/rem...
File chrome/test/remoting/remote_desktop_browsertest.cc (right):

https://codereview.chromium.org/582833003/diff/80001/chrome/test/remoting/rem...
chrome/test/remoting/remote_desktop_browsertest.cc:714: has_disabled_attribute))
{
On 2014/09/19 22:37:23, Jamie wrote:
> Nit: Indent the second and subsequent parameters to align with the first.

Done.

[anandc, 2014-09-19 22:45:15]

The CQ bit was checked by anandc@chromium.org

[commit-bot: I haz the power, 2014-09-19 22:47:38]

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

[commit-bot: I haz the power, 2014-09-19 22:47:45]

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

[commit-bot: I haz the power, 2014-09-20 00:37:59]

Committed patchset #4 (id:100001) as a1852574107588e7accb49777df4164b36519cfb

[commit-bot: I haz the power, 2014-09-20 00:38:28]

Patchset 4 (id:??) landed as
https://crrev.com/725e8d6eb1e5be6ad2130d815ebc172d4dee078e
Cr-Commit-Position: refs/heads/master@{#295833}