Improve <adview> implementation and add tests.

chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js

Index: chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js
diff --git a/chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js b/chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js
new file mode 100644
index 0000000000000000000000000000000000000000..4e324e68911b4a7580862d6579f4b3b53ed6fdec
--- /dev/null
+++ b/chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js
@@ -0,0 +1,62 @@
+// Copyright (c) 2013 The Chromium Authors. All rights reserved.

[benwells, 2013/04/02 00:49:45]

Nit: No (c) in new copyright notices.

[rpaquay, 2013/04/02 16:44:47]

Done.

+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+// This test checks that 1) it is possible change the value of the "ad-network"
+// attribute of an <adview> element and 2) changing the value will reset the
+// "src" attribute.
+
+function onLoadCommit(adview, guestURL) {
+  console.log("onLoadCommit(" + adview.getAttribute('ad-network') + ")");
+  // First callback: assert and run another callback
+  if (adview.getAttribute('ad-network') == 'test') {
+    // Check 'src' and 'ad-network' attributes/properties
+    chrome.test.assertEq('test', adview.getAttribute('ad-network'));
+    chrome.test.assertEq('test', adview['ad-network']);

[benwells, 2013/04/02 00:49:45]

why do you check this twice? Or even at all, this branch only gets invoked if
this condition is true, no?

[rpaquay, 2013/04/02 16:44:47]

adview is the shadow element implemented in "ad_view.js". There is (somewhat non
trivial) code in there to make sure the "src" and "ad-network" properties and
attributes are in sync, which is which both the property and the attribute value
are checked.

As for you second question, it is true that the assert on the "ad-network"
attribute is redundant with the "if" test above. The only reason it is there is
to have all assertEq in this file "paired" (i.e. check both attribute and
property value) for consistency.

+    chrome.test.assertEq(guestURL, adview.getAttribute('src'));
+    chrome.test.assertEq(guestURL, adview.src);

[benwells, 2013/04/02 00:49:45]

Again, why test this twice in different ways?

[rpaquay, 2013/04/02 16:44:47]

See comment above: property and attributes are different code paths in
"ad_view.js".

+
+    adview['ad-network'] = '';
+    setTimeout(function() {

[benwells, 2013/04/02 00:49:45]

Is there a different way to catch this without using setTimeout? It is immediate
(i.e. 0 ms) so it's not that bad.

[rpaquay, 2013/04/02 16:44:47]

Not that I know of. I put a comment in other test using this technique
explaining why it is necessary:

  // Note: The assertion has to happen in a callback function because of the
  //  way mutation observer events are processed. See
  //  http://lists.w3.org/Archives/Public/public-webapps/2011JulSep/1622.html

[benwells, 2013/04/03 02:49:48]

Oops forgot this one.

I think this should be covered by the API. How would users do something after a
change like this has taken effect?

Is this usage of setTimeout normal in webview tests?

Also, the comment isn't really useful as it is explaining your implementation.
Something like "using a callback as adview attribute changes are not reflected
immediately" would be more useful.

[rpaquay, 2013/04/03 18:30:05]

I am not sure I understand what you mean.
Good question. Fady would know better, but I just found at least one example in
src/chrome/test/data/extensions/platform_apps/web_view/shim/main.js:

      // Timeout is necessary to give the mutation observers a chance to fire.
      setTimeout(function() {
        chrome.test.assertEq(330, dynamicWebViewTag.offsetWidth);
        chrome.test.assertEq(230, dynamicWebViewTag.offsetHeight);

        chrome.test.succeed();
      }, 0);

As an aside, I agree with you setTimeout is far from ideal and I am all ears
open for a better solution.
Done.

[benwells, 2013/04/04 04:33:52]

I mean a user of this API might want to do the same thing as this test: set an
attribute, and once that has flowed through do something else. The only way to
do it now would be to use a setTimeout(..., 0), which is not obvious.

It would be better if it was clear from the API that a callback is needed, by
having a callback parameter to the call. E.g. foo.setAttribute(xxx, callback);

[rpaquay, 2013/04/05 16:12:08]

Hmm... setAttribute is a DOM pre-defined method. I am not sure re-defining is
possible, or if this would help with the confusing aspect of the behavior, as
setAttribute is so widely used as-is.

+      // Check 'src' and 'ad-network' attributes/properties
+      chrome.test.assertEq('', adview.getAttribute('ad-network'));
+      chrome.test.assertEq('', adview['ad-network']);
+      chrome.test.assertEq('', adview.getAttribute('src'));
+      chrome.test.assertEq('', adview.src);
+
+      adview['ad-network'] = 'test2';
+    }, 0);
+  }
+  // Second callback: assert and finish test

[benwells, 2013/04/02 00:49:45]

What triggers the second callback?

[rpaquay, 2013/04/02 16:44:47]

The line above:
  adview['ad-network'] = 'test2';
When the property value changes, there is another event raised.

[benwells, 2013/04/03 02:40:03]

Nit: please add a comment explaining. People will need to debug test failures
who aren't very familiar with how this API works.

[rpaquay, 2013/04/03 18:30:05]

Done.

+  else if (adview.getAttribute('ad-network') == 'test2') {
+    // Check 'src' and 'ad-network' attributes/properties
+    chrome.test.assertEq('test2', adview.getAttribute('ad-network'));
+    chrome.test.assertEq('test2', adview['ad-network']);
+    chrome.test.assertEq('', adview.getAttribute('src'));
+    chrome.test.assertEq('', adview['src']);
+
+    console.log("Test finished correctly.");

[benwells, 2013/04/02 00:49:45]

Running tests through the chrome.test system makes things easier but tests can
'successfully' complete too early. Did you observe this console.log message?

[rpaquay, 2013/04/02 16:44:47]

Yes. As far as I could tell, tests run until "chrome.test.succeed()" is called.

+    chrome.test.succeed();
+  }
+};
+
+function runTests(guestURL) {
+  chrome.test.runTests([
+    function test() {
+      var adview = document.getElementsByTagName("adview")[0];
+
+      adview.addEventListener("loadcommit", function(event) {
+        onLoadCommit(adview, guestURL);
+      });
+      adview.setAttribute("src", guestURL);
+    }
+  ]);
+}
+
+window.onload = function() {
+  chrome.test.getConfig(function(config) {
+    var guestURL = 'http://localhost:' + config.testServer.port +
+        '/files/extensions/platform_apps/ad_view/ad_network_site/testsdk.html';
+    runTests(guestURL);
+  });
+}