Fix for broken CertificateViewerUITest.testDetails.

Fix for broken CertificateViewerUITest.testDetails.

BUG=127732
TEST=browser_tests --gtest_filter="CertificateViewerUITest*.*"


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137984

[Kevin Greer, 2012-05-17 02:15:51]

There were two issues:

1. The delay I introduced to postpone the population of the second tab was
causing the certHierarchy to not yet be populated when the test started.  I'm
not sure what's the best technique for solving this, but what I did to fix this
is to have the certificate_viewer.js check if it is being run as a test, and
don't delay in that case.

2. When the test sets the selectedItem to be the first childNode from the
certHierarchy, it wasn't causing an event to be fired from the Tree control
because that value was already selected, and the tree control only fires an
event when the value changes.  The lack of event caused the
cert_viewer.getCertificateFields() method to not get called.  I fixed this by
first setting the selectedItem to null.

[flackr, 2012-05-17 02:28:16]

https://chromiumcodereview.appspot.com/10382145/diff/13001/chrome/browser/res...
File chrome/browser/resources/certificate_viewer.js (right):

https://chromiumcodereview.appspot.com/10382145/diff/13001/chrome/browser/res...
chrome/browser/resources/certificate_viewer.js:34: // a race-condition and makes
the test flakey.
It seems to me like the ideal way to fix this would be to make the test
explicitly switch to the details tab, and to make switching to the details tab
force the tab to be initialized if done before the timeout fires cancelling the
timeout. This way we wouldn't have special testing behaviour and in the event
that either the user manages to switch to the second tab before the timeout
fires or we have some code path that loads the second tab on startup everything
works.

[Kevin Greer, 2012-05-17 11:50:58]

Please review.  I created a new function called initializeSecondTab() which can
be called from the unit-test to ensure that the second tab has been initialized.
 This is a 'oneShot' function, meaning that if you call it more than once, it
will still only run once.

[flackr, 2012-05-17 15:46:55]

http://codereview.chromium.org/10382145/diff/15001/chrome/browser/resources/c...
File chrome/browser/resources/certificate_viewer.js (right):

http://codereview.chromium.org/10382145/diff/15001/chrome/browser/resources/c...
chrome/browser/resources/certificate_viewer.js:23:
setTimeout(initializeSecondTab, 200);
Can we also hook this up to the tab change and use a tab change to trigger the
method from the test?

http://codereview.chromium.org/10382145/diff/15001/chrome/browser/resources/c...
chrome/browser/resources/certificate_viewer.js:55: var initializeSecondTab =
oneShot(function() {
nit: Call this initializeDetailsTab.

http://codereview.chromium.org/10382145/diff/15001/chrome/test/data/webui/cer...
File chrome/test/data/webui/certificate_viewer_dialog_test.js (right):

http://codereview.chromium.org/10382145/diff/15001/chrome/test/data/webui/cer...
chrome/test/data/webui/certificate_viewer_dialog_test.js:84:
cert_viewer.initializeSecondTab();
I don't think this will work since initializeSecondTab isn't exported. Ideally
though as I mentioned in my other comment we would trigger this by selecting the
details tab.

[Kevin Greer, 2012-05-17 15:56:12]

Changed name and exported method.  Added comment.

http://codereview.chromium.org/10382145/diff/15001/chrome/browser/resources/c...
File chrome/browser/resources/certificate_viewer.js (right):

http://codereview.chromium.org/10382145/diff/15001/chrome/browser/resources/c...
chrome/browser/resources/certificate_viewer.js:23:
setTimeout(initializeSecondTab, 200);
Are you sure?  That seems a little hackish and adds an extra layer of
indirection to both the code and the test, for no benefit (IMO).
On 2012/05/17 15:46:56, flackr wrote:
> Can we also hook this up to the tab change and use a tab change to trigger the
> method from the test?

http://codereview.chromium.org/10382145/diff/15001/chrome/browser/resources/c...
chrome/browser/resources/certificate_viewer.js:55: var initializeSecondTab =
oneShot(function() {
Agreed.  That's a better name.  Changed.
On 2012/05/17 15:46:56, flackr wrote:
> nit: Call this initializeDetailsTab.

[flackr, 2012-05-17 16:56:58]

http://codereview.chromium.org/10382145/diff/15001/chrome/browser/resources/c...
File chrome/browser/resources/certificate_viewer.js (right):

http://codereview.chromium.org/10382145/diff/15001/chrome/browser/resources/c...
chrome/browser/resources/certificate_viewer.js:23:
setTimeout(initializeSecondTab, 200);
By triggering it on selecting the details tab the test will simulate a user
really quickly clicking on details rather than calling a special exported
function to initialize the tab. In the regular user case this will just be a
noop event handler since it is a one shot function: i.e.

$('tabbox').addEventListener('selectedChange', function f(e) {
  $('tabbox').removeEventListener('selectedChange', f);
  initializeSecondTab();
}

On 2012/05/17 15:56:12, Kevin Greer wrote:
> Are you sure?  That seems a little hackish and adds an extra layer of
> indirection to both the code and the test, for no benefit (IMO).
> On 2012/05/17 15:46:56, flackr wrote:
> > Can we also hook this up to the tab change and use a tab change to trigger
the
> > method from the test?
>

[Kevin Greer, 2012-05-17 18:32:13]

Please review.

[flackr, 2012-05-17 18:43:18]

LGTM

[mazda, 2012-05-18 07:32:29]

lgtm

[commit-bot: I haz the power, 2012-05-18 11:29:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kgr@chromium.org/10382145/14004

[commit-bot: I haz the power, 2012-05-18 11:30:03]

Presubmit check for 10382145-14004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome/browser/resources

Presubmit checks took 1.4s to calculate.

[Kevin Greer, 2012-05-18 13:04:24]

Evan, please review (I need an owner).  Thanks

[Evan Stade, 2012-05-18 20:45:51]

lgtm

[commit-bot: I haz the power, 2012-05-18 21:06:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kgr@chromium.org/10382145/14004

[commit-bot: I haz the power, 2012-05-19 00:08:32]

Change committed as 137984