Improve <adview> implementation and add tests.

Improve <adview> implementation and add tests.

The first CL for bug 180618 contained initial support for the <adview> tag. This is a follow up CL to

* fix issues related to various combinations of flags, permissions and property/attributes behavior,
* add corresponding tests,
* and improve code readability of "ad_view.js"

BUG=226125
TBR=asargent@chromium.org (for trivial changes in chrome/browser/component_updater/test/component_updater_service_unittest.cc)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196036

[rpaquay, 2013-03-26 22:15:56]

Add more tests for <adview>, and refactor adview shim.

Reviewers, please review the following files:

agl@
  content/test/net/url_request_prepackaged_interceptor.*

asargent@
  chrome/browser/component_updater/test/component_updater_service_unittest.cc
  chrome/browser/extensions/*
  chrome/renderer/resources/extensions/ad_view.js
  chrome/test/data/extensions/platform_apps/ad_view/*

fsamuel@
  chrome/renderer/resources/extensions/ad_view.js

[agl, 2013-03-26 22:27:16]

lgtm

https://codereview.chromium.org/12967016/diff/7001/content/test/net/url_reque...
File content/test/net/url_request_prepackaged_interceptor.cc (right):

https://codereview.chromium.org/12967016/diff/7001/content/test/net/url_reque...
content/test/net/url_request_prepackaged_interceptor.cc:118: std::string
scheme_;
I think these can be const.

https://codereview.chromium.org/12967016/diff/7001/content/test/net/url_reque...
File content/test/net/url_request_prepackaged_interceptor.h (right):

https://codereview.chromium.org/12967016/diff/7001/content/test/net/url_reque...
content/test/net/url_request_prepackaged_interceptor.h:25:
URLRequestPrepackagedInterceptor(const std::string& scheme,
This constructor needs a comment.

https://codereview.chromium.org/12967016/diff/7001/content/test/net/url_reque...
content/test/net/url_request_prepackaged_interceptor.h:44: std::string scheme_;
can be const I think.

https://codereview.chromium.org/12967016/diff/7001/content/test/net/url_reque...
content/test/net/url_request_prepackaged_interceptor.h:54: // Intercepts HTTP
requests and gives pre-defined responses to specified URLs.
Is this actually useful? It seems that just using
URLRequestPrepackagedInterceptor directly would be clearer, although I don't
object if you judge that it is.

[asargent_no_longer_on_chrome, 2013-03-27 00:14:00]

+benwells

Ben - do you mind reviewing the 

chrome/{browser,renderer/resources,test/data/}/extensions

parts here?

[benwells, 2013-03-27 21:43:05]

Yay for tests!

Still looking, but here is early feedback.

https://codereview.chromium.org/12967016/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/ad_view_browsertest.cc (right):

https://codereview.chromium.org/12967016/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/ad_view_browsertest.cc:46: // (image315.png). The test
will be enabled once the binary files are committed.
Is this comment out of date now?

https://codereview.chromium.org/12967016/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/ad_view_browsertest.cc:90: // The point of this test
is to ensure an invalid (i.e. not whitelisted)
These comments are helpful. Could you add them to the other tests as well?

https://codereview.chromium.org/12967016/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/ad_view_browsertest.cc:122:
content::URLRequestPrepackagedInterceptor interceptor(scheme, hostname);
Why does this test use an interceptor and the others a test server?

[rpaquay, 2013-03-27 22:57:25]

Addressed first wave of code review feedback.

https://codereview.chromium.org/12967016/diff/7001/chrome/browser/extensions/...
File chrome/browser/extensions/ad_view_browsertest.cc (right):

https://codereview.chromium.org/12967016/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/ad_view_browsertest.cc:46: // (image315.png). The test
will be enabled once the binary files are committed.
On 2013/03/27 21:43:05, benwells wrote:
> Is this comment out of date now?
Yes, I removed it. Note that I actually expect this CL to fail on the trybots
because it seems binary files (image315.png in this case) are not processed
correctly during initial commit. So, I will probably have to re-add the
"DISABLED_" prefix and the comment above, and the submit another CL to enable it
back...

https://codereview.chromium.org/12967016/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/ad_view_browsertest.cc:90: // The point of this test
is to ensure an invalid (i.e. not whitelisted)
On 2013/03/27 21:43:05, benwells wrote:
> These comments are helpful. Could you add them to the other tests as well?

Done.

https://codereview.chromium.org/12967016/diff/7001/chrome/browser/extensions/...
chrome/browser/extensions/ad_view_browsertest.cc:122:
content::URLRequestPrepackagedInterceptor interceptor(scheme, hostname);
On 2013/03/27 21:43:05, benwells wrote:
> Why does this test use an interceptor and the others a test server?
Because I thought it would be good to have a test verifying the behavior
end-2-end when using an ad-network from the whitelist (i.e. *not* using the
special test flag). Since the URL specified in the whitelist is arbitrary, I
needed to use the interceptor mechanism as opposed to the local http server.

https://codereview.chromium.org/12967016/diff/7001/content/test/net/url_reque...
File content/test/net/url_request_prepackaged_interceptor.cc (right):

https://codereview.chromium.org/12967016/diff/7001/content/test/net/url_reque...
content/test/net/url_request_prepackaged_interceptor.cc:118: std::string
scheme_;
On 2013/03/26 22:27:16, agl wrote:
> I think these can be const.

Done.

https://codereview.chromium.org/12967016/diff/7001/content/test/net/url_reque...
File content/test/net/url_request_prepackaged_interceptor.h (right):

https://codereview.chromium.org/12967016/diff/7001/content/test/net/url_reque...
content/test/net/url_request_prepackaged_interceptor.h:25:
URLRequestPrepackagedInterceptor(const std::string& scheme,
On 2013/03/26 22:27:16, agl wrote:
> This constructor needs a comment.

Done.

https://codereview.chromium.org/12967016/diff/7001/content/test/net/url_reque...
content/test/net/url_request_prepackaged_interceptor.h:44: std::string scheme_;
On 2013/03/26 22:27:16, agl wrote:
> can be const I think.

Done.

https://codereview.chromium.org/12967016/diff/7001/content/test/net/url_reque...
content/test/net/url_request_prepackaged_interceptor.h:54: // Intercepts HTTP
requests and gives pre-defined responses to specified URLs.
On 2013/03/26 22:27:16, agl wrote:
> Is this actually useful? It seems that just using
> URLRequestPrepackagedInterceptor directly would be clearer, although I don't
> object if you judge that it is.
I don't have a strong opinion on this. The common case is "http://localhost", so
I thought it would be useful to leave a shortcut for that case.

[benwells, 2013-03-28 00:16:48]

https://chromiumcodereview.appspot.com/12967016/diff/7001/chrome/browser/exte...
File chrome/browser/extensions/ad_view_browsertest.cc (right):

https://chromiumcodereview.appspot.com/12967016/diff/7001/chrome/browser/exte...
chrome/browser/extensions/ad_view_browsertest.cc:122:
content::URLRequestPrepackagedInterceptor interceptor(scheme, hostname);
On 2013/03/27 22:57:25, rpaquay wrote:
> On 2013/03/27 21:43:05, benwells wrote:
> > Why does this test use an interceptor and the others a test server?
> Because I thought it would be good to have a test verifying the behavior
> end-2-end when using an ad-network from the whitelist (i.e. *not* using the
> special test flag). Since the URL specified in the whitelist is arbitrary, I
> needed to use the interceptor mechanism as opposed to the local http server.

That makes sense. I'm wondering if using the interceptor is preferable for all
tests, would it be less flaky do you think?

https://chromiumcodereview.appspot.com/12967016/diff/19001/chrome/browser/ext...
File chrome/browser/extensions/ad_view_browsertest.cc (right):

https://chromiumcodereview.appspot.com/12967016/diff/19001/chrome/browser/ext...
chrome/browser/extensions/ad_view_browsertest.cc:27:
LoadAndLaunchPlatformApp("ad_view/loadcommit_event");
Any reason not to use RunPaltformAppTest? You won't need to have custom messages
and listeners then.

[rpaquay, 2013-03-28 16:20:46]

On 2013/03/28 00:16:48, benwells wrote:
>
https://chromiumcodereview.appspot.com/12967016/diff/7001/chrome/browser/exte...
> File chrome/browser/extensions/ad_view_browsertest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/12967016/diff/7001/chrome/browser/exte...
> chrome/browser/extensions/ad_view_browsertest.cc:122:
> content::URLRequestPrepackagedInterceptor interceptor(scheme, hostname);
> On 2013/03/27 22:57:25, rpaquay wrote:
> > On 2013/03/27 21:43:05, benwells wrote:
> > > Why does this test use an interceptor and the others a test server?
> > Because I thought it would be good to have a test verifying the behavior
> > end-2-end when using an ad-network from the whitelist (i.e. *not* using the
> > special test flag). Since the URL specified in the whitelist is arbitrary, I
> > needed to use the interceptor mechanism as opposed to the local http server.
> 
> That makes sense. I'm wondering if using the interceptor is preferable for all
> tests, would it be less flaky do you think?

I didn't notice flakyness with the local test server (on my local desktop
anyways). Is it known to be flaky? Also, the adview test flag is useful more 
than just for running automated tests, it is also useful when developing a new 
ad-network sdk so you don't have to update the whitelist and rebuild chrome 
(e.g. think admob team). So testing that configuration makes sense too.
Also, using interceptors require duplicating URLs in the test, which could
mean more maintenance. Other than that, I don't have a strong opinion on this.


> 
>
https://chromiumcodereview.appspot.com/12967016/diff/19001/chrome/browser/ext...
> File chrome/browser/extensions/ad_view_browsertest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/12967016/diff/19001/chrome/browser/ext...
> chrome/browser/extensions/ad_view_browsertest.cc:27:
> LoadAndLaunchPlatformApp("ad_view/loadcommit_event");
> Any reason not to use RunPaltformAppTest? You won't need to have custom
messages
> and listeners then.

No reason other I was not aware of "RunPlatformAppTest". I'll take a look.

[benwells, 2013-03-28 16:41:54]

On 2013/03/28 16:20:46, rpaquay wrote:
> On 2013/03/28 00:16:48, benwells wrote:
> >
>
https://chromiumcodereview.appspot.com/12967016/diff/7001/chrome/browser/exte...
> > File chrome/browser/extensions/ad_view_browsertest.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/12967016/diff/7001/chrome/browser/exte...
> > chrome/browser/extensions/ad_view_browsertest.cc:122:
> > content::URLRequestPrepackagedInterceptor interceptor(scheme, hostname);
> > On 2013/03/27 22:57:25, rpaquay wrote:
> > > On 2013/03/27 21:43:05, benwells wrote:
> > > > Why does this test use an interceptor and the others a test server?
> > > Because I thought it would be good to have a test verifying the behavior
> > > end-2-end when using an ad-network from the whitelist (i.e. *not* using
the
> > > special test flag). Since the URL specified in the whitelist is arbitrary,
I
> > > needed to use the interceptor mechanism as opposed to the local http
server.
> > 
> > That makes sense. I'm wondering if using the interceptor is preferable for
all
> > tests, would it be less flaky do you think?
> 
> I didn't notice flakyness with the local test server (on my local desktop
> anyways). Is it known to be flaky? Also, the adview test flag is useful more 
> than just for running automated tests, it is also useful when developing a new

> ad-network sdk so you don't have to update the whitelist and rebuild chrome 
> (e.g. think admob team). So testing that configuration makes sense too.
> Also, using interceptors require duplicating URLs in the test, which could
> mean more maintenance. Other than that, I don't have a strong opinion on this.

Yes, having an integration test with the server is good, and testing all the
supported configurations is good. I only have vague memories of test server
flakiness which may or may not be true, the comment was more in the spirit of
'less moving parts in tests is better', both for predictability and speed.

But I don't feel strongly either.

> 
> 
> > 
> >
>
https://chromiumcodereview.appspot.com/12967016/diff/19001/chrome/browser/ext...
> > File chrome/browser/extensions/ad_view_browsertest.cc (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/12967016/diff/19001/chrome/browser/ext...
> > chrome/browser/extensions/ad_view_browsertest.cc:27:
> > LoadAndLaunchPlatformApp("ad_view/loadcommit_event");
> > Any reason not to use RunPaltformAppTest? You won't need to have custom
> messages
> > and listeners then.
> 
> No reason other I was not aware of "RunPlatformAppTest". I'll take a look.

[rpaquay, 2013-04-01 21:00:33]

Addressing benwells@'s code review feedback.

[benwells, 2013-04-02 00:49:44]

Sorry this is dragging on so long. Unless there is no reason not too I think you
should pull the refactor out into a separate CL.

https://codereview.chromium.org/12967016/diff/27001/chrome/browser/extensions...
File chrome/browser/extensions/ad_view_browsertest.cc (right):

https://codereview.chromium.org/12967016/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/ad_view_browsertest.cc:21:
IN_PROC_BROWSER_TEST_F(AdViewTest, LoadCommitEventIsCalled) {
Where did all the comments go?

https://codereview.chromium.org/12967016/diff/27001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/ad_view.js (right):

https://codereview.chromium.org/12967016/diff/27001/chrome/renderer/resources...
chrome/renderer/resources/extensions/ad_view.js:13: 
Nit: remove extra blank line

https://codereview.chromium.org/12967016/diff/27001/chrome/renderer/resources...
chrome/renderer/resources/extensions/ad_view.js:19: 
Nit: remove extra blank line

https://codereview.chromium.org/12967016/diff/27001/chrome/renderer/resources...
chrome/renderer/resources/extensions/ad_view.js:22: * "adViewCustom" module was
injected. This is so that the
Is there some precedent for this style of multi line comment? For consistency
with the rest of chrome I think you should just use the normal pattern for
comments: that is, just "//" on each line.

https://codereview.chromium.org/12967016/diff/27001/chrome/renderer/resources...
chrome/renderer/resources/extensions/ad_view.js:29: 
Nit: remove extra blank line

https://codereview.chromium.org/12967016/diff/27001/chrome/renderer/resources...
chrome/renderer/resources/extensions/ad_view.js:47: name: 'ad-network',
Is there any reason to lump the refactor and the tests into the same CL? They
feel orthogonal. This refactor seems non-trivial.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
Nit: No (c) in new copyright notices.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:15:
chrome.test.assertEq('test', adview['ad-network']);
why do you check this twice? Or even at all, this branch only gets invoked if
this condition is true, no?

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:17:
chrome.test.assertEq(guestURL, adview.src);
Again, why test this twice in different ways?

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:20:
setTimeout(function() {
Is there a different way to catch this without using setTimeout? It is immediate
(i.e. 0 ms) so it's not that bad.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:30:
// Second callback: assert and finish test
What triggers the second callback?

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:38:
console.log("Test finished correctly.");
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]

Addressing benwells@'s code review feedback.

I would not too fond of moving the "refactor" into a separate CL because, as
mentioned in the comments, the refactor was initiated as the result of fixing
issues exposed by these additional tests. To me at least, it feels the tests and
refactor are pretty closely related. I can certainly make it into another CL if
you feel strongly about it though.

https://codereview.chromium.org/12967016/diff/27001/chrome/browser/extensions...
File chrome/browser/extensions/ad_view_browsertest.cc (right):

https://codereview.chromium.org/12967016/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/ad_view_browsertest.cc:21:
IN_PROC_BROWSER_TEST_F(AdViewTest, LoadCommitEventIsCalled) {
On 2013/04/02 00:49:45, benwells wrote:
> Where did all the comments go?
I moved them in their respective "chrometest.js" file, because the remaining
test code in this file is now pretty mechanical and uninteresting.

https://codereview.chromium.org/12967016/diff/27001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/ad_view.js (right):

https://codereview.chromium.org/12967016/diff/27001/chrome/renderer/resources...
chrome/renderer/resources/extensions/ad_view.js:13: 
On 2013/04/02 00:49:45, benwells wrote:
> Nit: remove extra blank line

Done.

https://codereview.chromium.org/12967016/diff/27001/chrome/renderer/resources...
chrome/renderer/resources/extensions/ad_view.js:19: 
On 2013/04/02 00:49:45, benwells wrote:
> Nit: remove extra blank line

Done.

https://codereview.chromium.org/12967016/diff/27001/chrome/renderer/resources...
chrome/renderer/resources/extensions/ad_view.js:22: * "adViewCustom" module was
injected. This is so that the
On 2013/04/02 00:49:45, benwells wrote:
> Is there some precedent for this style of multi line comment? For consistency
> with the rest of chrome I think you should just use the normal pattern for
> comments: that is, just "//" on each line.
There are a few files in this same folder using multi line comments
(web_view.js, last_error.js, platform_app.js). That (and also reading the
Javascript Style Guide) is the reason I made the switch to multi line comments.

https://codereview.chromium.org/12967016/diff/27001/chrome/renderer/resources...
chrome/renderer/resources/extensions/ad_view.js:29: 
On 2013/04/02 00:49:45, benwells wrote:
> Nit: remove extra blank line

Done.

https://codereview.chromium.org/12967016/diff/27001/chrome/renderer/resources...
chrome/renderer/resources/extensions/ad_view.js:47: name: 'ad-network',
On 2013/04/02 00:49:45, benwells wrote:
> Is there any reason to lump the refactor and the tests into the same CL? They
> feel orthogonal. This refactor seems non-trivial.
Most of the refactor was initiated as a result of adding new tests. The code was
becoming too complex to handle all the corner cases, so a refactor was needed to
manage complexity.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2013/04/02 00:49:45, benwells wrote:
> Nit: No (c) in new copyright notices.

Done.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:15:
chrome.test.assertEq('test', adview['ad-network']);
On 2013/04/02 00:49:45, benwells wrote:
> why do you check this twice? Or even at all, this branch only gets invoked if
> this condition is true, no?
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.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:17:
chrome.test.assertEq(guestURL, adview.src);
On 2013/04/02 00:49:45, benwells wrote:
> Again, why test this twice in different ways?
See comment above: property and attributes are different code paths in
"ad_view.js".

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:20:
setTimeout(function() {
On 2013/04/02 00:49:45, benwells wrote:
> Is there a different way to catch this without using setTimeout? It is
immediate
> (i.e. 0 ms) so it's not that bad.

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

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:30:
// Second callback: assert and finish test
On 2013/04/02 00:49:45, benwells wrote:
> What triggers the second callback?

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

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:38:
console.log("Test finished correctly.");
On 2013/04/02 00:49:45, benwells wrote:
> Running tests through the chrome.test system makes things easier but tests can
> 'successfully' complete too early. Did you observe this console.log message?

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

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

Refactor implies there is no change in behavior. If you're fixing things, this
isn't a refactor.

Either way the description feels misleading. If the tests are to check behavior
after a refactor please highlight on that. E.g. description should be like
"Refactor foo and add tests".

If there are fixes please make it clear with a description like "Fix bugs in foo
and add tests". If there are fixes there should also be a bug to link to.

I have looked over all the test code now and just have to look at the bindings
change.

https://codereview.chromium.org/12967016/diff/27001/chrome/browser/extensions...
File chrome/browser/extensions/ad_view_browsertest.cc (right):

https://codereview.chromium.org/12967016/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/ad_view_browsertest.cc:21:
IN_PROC_BROWSER_TEST_F(AdViewTest, LoadCommitEventIsCalled) {
On 2013/04/02 16:44:47, rpaquay wrote:
> On 2013/04/02 00:49:45, benwells wrote:
> > Where did all the comments go?
> I moved them in their respective "chrometest.js" file, because the remaining
> test code in this file is now pretty mechanical and uninteresting.

This is the first point of investigation for anyone looking into test failures.
Comments here are very useful if it is not obvious what a test is testing.

I think you should bring the comments back.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:30:
// Second callback: assert and finish test
On 2013/04/02 16:44:47, rpaquay wrote:
> On 2013/04/02 00:49:45, benwells wrote:
> > What triggers the second callback?
> 
> The line above:
>   adview['ad-network'] = 'test2';
> When the property value changes, there is another event raised.

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

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/flag_required/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/flag_required/chrometest.js:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
Nit: no (c)

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/flag_required/chrometest.js:31:
chrome.test.assertEq(true, adview.hasAttribute('ad-network'));
Why is this expected to be true?

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/flag_required/chrometest.js:37:
}
Shouldn't there be a line like adview.setAttribute somewhere? I thought this
test was to do the same thing as other tests, but check nothing happens as the
kEnableAdview flag is not present.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/invalid_ad_network/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/invalid_ad_network/chrometest.js:54:
setTimeout(function() {
I think you are avoiding chrome.runTests so can ensure test 1 happens before
document load and the others after.

Instead of putting in this timeout, you should call runTest3 when test 2
finishes. That would be clearer and more robust.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/loadabort_event/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/loadabort_event/chrometest.js:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
Nit: no (c). Please make sure they are all gone in the new files.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/onloadcommit_ack/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/onloadcommit_ack/chrometest.js:23:
window.addEventListener("message", function(event) {
Move adding the event listener to before setting the attribute. It is clearer
then that the set attribute triggers the flow of calls and callbacks.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/src_not_exposed/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/src_not_exposed/chrometest.js:5:
function runTests() {
Please add an initial comment here like in all the other test js files.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/src_not_exposed/manifest.json
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/src_not_exposed/manifest.json:12:
"adview", "webview"
why is webview permission needed?

[benwells, 2013-04-03 02:49:47]

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:20:
setTimeout(function() {
On 2013/04/02 16:44:47, rpaquay wrote:
> On 2013/04/02 00:49:45, benwells wrote:
> > Is there a different way to catch this without using setTimeout? It is
> immediate
> > (i.e. 0 ms) so it's not that bad.
> 
> 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

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]

Addressing benwells@'s code review feedback.

I created a bug and update the description for this CL. 

Ben, there are a few comments you made I was not sure about (see below). Let me
know what you think.

Thanks,
Renaud

https://codereview.chromium.org/12967016/diff/27001/chrome/browser/extensions...
File chrome/browser/extensions/ad_view_browsertest.cc (right):

https://codereview.chromium.org/12967016/diff/27001/chrome/browser/extensions...
chrome/browser/extensions/ad_view_browsertest.cc:21:
IN_PROC_BROWSER_TEST_F(AdViewTest, LoadCommitEventIsCalled) {
On 2013/04/03 02:40:03, benwells wrote:
> On 2013/04/02 16:44:47, rpaquay wrote:
> > On 2013/04/02 00:49:45, benwells wrote:
> > > Where did all the comments go?
> > I moved them in their respective "chrometest.js" file, because the remaining
> > test code in this file is now pretty mechanical and uninteresting.
> 
> This is the first point of investigation for anyone looking into test
failures.
> Comments here are very useful if it is not obvious what a test is testing.
> 
> I think you should bring the comments back.

Done.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:20:
setTimeout(function() {
On 2013/04/03 02:49:48, benwells wrote:
> On 2013/04/02 16:44:47, rpaquay wrote:
> > On 2013/04/02 00:49:45, benwells wrote:
> > > Is there a different way to catch this without using setTimeout? It is
> > immediate
> > > (i.e. 0 ms) so it's not that bad.
> > 
> > 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
> 
> 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?

I am not sure I understand what you mean.


> Is this usage of setTimeout normal in webview tests?

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.

> 
> 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.

Done.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:30:
// Second callback: assert and finish test
On 2013/04/03 02:40:03, benwells wrote:
> On 2013/04/02 16:44:47, rpaquay wrote:
> > On 2013/04/02 00:49:45, benwells wrote:
> > > What triggers the second callback?
> > 
> > The line above:
> >   adview['ad-network'] = 'test2';
> > When the property value changes, there is another event raised.
> 
> Nit: please add a comment explaining. People will need to debug test failures
> who aren't very familiar with how this API works.

Done.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/flag_required/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/flag_required/chrometest.js:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2013/04/03 02:40:03, benwells wrote:
> Nit: no (c)

Done.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/flag_required/chrometest.js:31:
chrome.test.assertEq(true, adview.hasAttribute('ad-network'));
On 2013/04/03 02:40:03, benwells wrote:
> Why is this expected to be true?
Because there is a value for the "ad-network" attribute in the index.html page.
Basically, this test makes sure that <adview> behaves like an "unknown" element
when the adview flag is not set. This also means the "setTimeout" is not
necessary in this case. I have updated the test to (hopefully) makes things a
bit clearer.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/flag_required/chrometest.js:37:
}
On 2013/04/03 02:40:03, benwells wrote:
> Shouldn't there be a line like adview.setAttribute somewhere? I thought this
> test was to do the same thing as other tests, but check nothing happens as the
> kEnableAdview flag is not present.
See comment above.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/invalid_ad_network/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/invalid_ad_network/chrometest.js:54:
setTimeout(function() {
On 2013/04/03 02:40:03, benwells wrote:
> I think you are avoiding chrome.runTests so can ensure test 1 happens before
> document load and the others after.
> 
> Instead of putting in this timeout, you should call runTest3 when test 2
> finishes. That would be clearer and more robust.

In this case, I was using "setTimeout" in "runTest3" for a different purpose: I
wanted to run code *after* the "DOMContentLoaded" event if fired and processed
by all listeners -- the reason being that ad_view.js contains code watching for
DOM change events which depends on when "DOMContentLoaded" is fired. Is there a
better way to achieve what I am trying to test?

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/loadabort_event/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/loadabort_event/chrometest.js:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2013/04/03 02:40:03, benwells wrote:
> Nit: no (c). Please make sure they are all gone in the new files.

Done.

As an aside, in general, how does one keep up to date with code style changes
like these? I made a similar mistake 2 weeks ago in another CL, and it was not
caught during code review.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/onloadcommit_ack/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/onloadcommit_ack/chrometest.js:23:
window.addEventListener("message", function(event) {
On 2013/04/03 02:40:03, benwells wrote:
> Move adding the event listener to before setting the attribute. It is clearer
> then that the set attribute triggers the flow of calls and callbacks.

Done.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/src_not_exposed/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/src_not_exposed/chrometest.js:5:
function runTests() {
On 2013/04/03 02:40:03, benwells wrote:
> Please add an initial comment here like in all the other test js files.

I am slightly confused about where to put comments :). I thought you asked me to
move the comments back to the ".cc" file. Did you mean to duplicate each comment
in both the ".cc" file and in the ".js" files?

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/src_not_exposed/manifest.json
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/src_not_exposed/manifest.json:12:
"adview", "webview"
On 2013/04/03 02:40:03, benwells wrote:
> why is webview permission needed?
This is a mistake, this should not have been there. Apologies.

[benwells, 2013-04-04 04:33:51]

I looked over ad_view.js and it seems sane to me, but most of it is not familiar
to me: I'll defer to fsamuel on that.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:20:
setTimeout(function() {
On 2013/04/03 18:30:05, rpaquay wrote:
> On 2013/04/03 02:49:48, benwells wrote:
> > On 2013/04/02 16:44:47, rpaquay wrote:
> > > On 2013/04/02 00:49:45, benwells wrote:
> > > > Is there a different way to catch this without using setTimeout? It is
> > > immediate
> > > > (i.e. 0 ms) so it's not that bad.
> > > 
> > > 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
> > 
> > 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?
> 
> I am not sure I understand what you mean.

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);

> 
> 
> > Is this usage of setTimeout normal in webview tests?
> 
> 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.
> 
> > 
> > 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.
> 
> Done.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/flag_required/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/flag_required/chrometest.js:31:
chrome.test.assertEq(true, adview.hasAttribute('ad-network'));
On 2013/04/03 18:30:05, rpaquay wrote:
> On 2013/04/03 02:40:03, benwells wrote:
> > Why is this expected to be true?
> Because there is a value for the "ad-network" attribute in the index.html
page.
> Basically, this test makes sure that <adview> behaves like an "unknown"
element
> when the adview flag is not set. This also means the "setTimeout" is not
> necessary in this case. I have updated the test to (hopefully) makes things a
> bit clearer.

Nit: please add a comment to explain why the ad-network attribute is expected
but not the others.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/flag_required/chrometest.js:37:
}
On 2013/04/03 18:30:05, rpaquay wrote:
> On 2013/04/03 02:40:03, benwells wrote:
> > Shouldn't there be a line like adview.setAttribute somewhere? I thought this
> > test was to do the same thing as other tests, but check nothing happens as
the
> > kEnableAdview flag is not present.
> See comment above.

OK. There is now a line like adview.setAttribute, so I guess I'm happy.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/invalid_ad_network/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/invalid_ad_network/chrometest.js:54:
setTimeout(function() {
On 2013/04/03 18:30:05, rpaquay wrote:
> On 2013/04/03 02:40:03, benwells wrote:
> > I think you are avoiding chrome.runTests so can ensure test 1 happens before
> > document load and the others after.
> > 
> > Instead of putting in this timeout, you should call runTest3 when test 2
> > finishes. That would be clearer and more robust.
> 
> In this case, I was using "setTimeout" in "runTest3" for a different purpose:
I
> wanted to run code *after* the "DOMContentLoaded" event if fired and processed
> by all listeners -- the reason being that ad_view.js contains code watching
for
> DOM change events which depends on when "DOMContentLoaded" is fired. Is there
a
> better way to achieve what I am trying to test?

OK, I see. Thanks for adding the comment, they really help with tests like this
where people unfamiliar with this area maybe end up working out why the test is
failing.

This setup feels slightly fragile though. It is currently the case that test 2
and test 3 will be running simultaneously. I don't think it can happen now, but
with some slight changes (e.g. one level of callbacks added) test 2 might finish
after test 3 has finished. This could mean a failure in test 2 would not get
caught.

It would be better to start test 3 once test 2 has completed. That would
guarantee the tests run sequentially, and also would ensure that test 3 happens
after the loaded event has been processed (but you should keep an updated
comment explaining so if test 2 is ever removed a timeout is added back).

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/loadabort_event/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/loadabort_event/chrometest.js:1:
// Copyright (c) 2013 The Chromium Authors. All rights reserved.
On 2013/04/03 18:30:05, rpaquay wrote:
> On 2013/04/03 02:40:03, benwells wrote:
> > Nit: no (c). Please make sure they are all gone in the new files.
> 
> Done.
> 
> As an aside, in general, how does one keep up to date with code style changes
> like these? I made a similar mistake 2 weeks ago in another CL, and it was not
> caught during code review.

Hmmm, you can read chromium-dev I guess. I found out about this one the same way
you did, on a code review of my own. This one is not checked much, for some
reason I tend to notice it.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/src_not_exposed/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/src_not_exposed/chrometest.js:5:
function runTests() {
On 2013/04/03 18:30:05, rpaquay wrote:
> On 2013/04/03 02:40:03, benwells wrote:
> > Please add an initial comment here like in all the other test js files.
> 
> I am slightly confused about where to put comments :). I thought you asked me
to
> move the comments back to the ".cc" file. Did you mean to duplicate each
comment
> in both the ".cc" file and in the ".js" files?

Yes sorry that was what I meant. They are really useful in both places.

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

Addressing benwells@'s code latest review feedback.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:20:
setTimeout(function() {
On 2013/04/04 04:33:52, benwells wrote:
> On 2013/04/03 18:30:05, rpaquay wrote:
> > On 2013/04/03 02:49:48, benwells wrote:
> > > On 2013/04/02 16:44:47, rpaquay wrote:
> > > > On 2013/04/02 00:49:45, benwells wrote:
> > > > > Is there a different way to catch this without using setTimeout? It is
> > > > immediate
> > > > > (i.e. 0 ms) so it's not that bad.
> > > > 
> > > > 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
> > > 
> > > 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?
> > 
> > I am not sure I understand what you mean.
> 
> 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);
> 
> > 
> > 
> > > Is this usage of setTimeout normal in webview tests?
> > 
> > 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.
> > 
> > > 
> > > 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.
> > 
> > Done.

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.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/flag_required/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/flag_required/chrometest.js:31:
chrome.test.assertEq(true, adview.hasAttribute('ad-network'));
On 2013/04/04 04:33:52, benwells wrote:
> On 2013/04/03 18:30:05, rpaquay wrote:
> > On 2013/04/03 02:40:03, benwells wrote:
> > > Why is this expected to be true?
> > Because there is a value for the "ad-network" attribute in the index.html
> page.
> > Basically, this test makes sure that <adview> behaves like an "unknown"
> element
> > when the adview flag is not set. This also means the "setTimeout" is not
> > necessary in this case. I have updated the test to (hopefully) makes things
a
> > bit clearer.
> 
> Nit: please add a comment to explain why the ad-network attribute is expected
> but not the others.

Done.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/invalid_ad_network/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/invalid_ad_network/chrometest.js:54:
setTimeout(function() {
On 2013/04/04 04:33:52, benwells wrote:
> On 2013/04/03 18:30:05, rpaquay wrote:
> > On 2013/04/03 02:40:03, benwells wrote:
> > > I think you are avoiding chrome.runTests so can ensure test 1 happens
before
> > > document load and the others after.
> > > 
> > > Instead of putting in this timeout, you should call runTest3 when test 2
> > > finishes. That would be clearer and more robust.
> > 
> > In this case, I was using "setTimeout" in "runTest3" for a different
purpose:
> I
> > wanted to run code *after* the "DOMContentLoaded" event if fired and
processed
> > by all listeners -- the reason being that ad_view.js contains code watching
> for
> > DOM change events which depends on when "DOMContentLoaded" is fired. Is
there
> a
> > better way to achieve what I am trying to test?
> 
> OK, I see. Thanks for adding the comment, they really help with tests like
this
> where people unfamiliar with this area maybe end up working out why the test
is
> failing.
> 
> This setup feels slightly fragile though. It is currently the case that test 2
> and test 3 will be running simultaneously. I don't think it can happen now,
but
> with some slight changes (e.g. one level of callbacks added) test 2 might
finish
> after test 3 has finished. This could mean a failure in test 2 would not get
> caught.
> 
> It would be better to start test 3 once test 2 has completed. That would
> guarantee the tests run sequentially, and also would ensure that test 3
happens
> after the loaded event has been processed (but you should keep an updated
> comment explaining so if test 2 is ever removed a timeout is added back).

I see what you mean. I made a change that should enforces test3 runs only after
test2 succeeds.

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
File
chrome/test/data/extensions/platform_apps/ad_view/src_not_exposed/chrometest.js
(right):

https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
chrome/test/data/extensions/platform_apps/ad_view/src_not_exposed/chrometest.js:5:
function runTests() {
On 2013/04/04 04:33:52, benwells wrote:
> On 2013/04/03 18:30:05, rpaquay wrote:
> > On 2013/04/03 02:40:03, benwells wrote:
> > > Please add an initial comment here like in all the other test js files.
> > 
> > I am slightly confused about where to put comments :). I thought you asked
me
> to
> > move the comments back to the ".cc" file. Did you mean to duplicate each
> comment
> > in both the ".cc" file and in the ".js" files?
> 
> Yes sorry that was what I meant. They are really useful in both places.

Done.

[benwells, 2013-04-07 23:47:04]

Awesome, lgtm.

Will leave the ad_view.js change to Fady.

On 2013/04/05 16:12:08, rpaquay wrote:
> Addressing benwells@'s code latest review feedback.
> 
>
https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
> File
>
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js
> (right):
> 
>
https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
>
chrome/test/data/extensions/platform_apps/ad_view/change_ad_network/chrometest.js:20:
> setTimeout(function() {
> On 2013/04/04 04:33:52, benwells wrote:
> > On 2013/04/03 18:30:05, rpaquay wrote:
> > > On 2013/04/03 02:49:48, benwells wrote:
> > > > On 2013/04/02 16:44:47, rpaquay wrote:
> > > > > On 2013/04/02 00:49:45, benwells wrote:
> > > > > > Is there a different way to catch this without using setTimeout? It
is
> > > > > immediate
> > > > > > (i.e. 0 ms) so it's not that bad.
> > > > > 
> > > > > 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
> > > > 
> > > > 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?
> > > 
> > > I am not sure I understand what you mean.
> > 
> > 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);
> > 
> > > 
> > > 
> > > > Is this usage of setTimeout normal in webview tests?
> > > 
> > > 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.
> > > 
> > > > 
> > > > 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.
> > > 
> > > Done.
> 
> 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.

True, but your version seems to have different semantics. I am not expert enough
in DOM / js to know if this is typical, but e.g. using setAttribute to set the
title on an element reflects immediately in element.title.

I don't know if this would ever be a problem outside of testing. If an API user
would ever need to do something after setting these attributes, and would have
to wait for it to flow through, you should consider adding a extra functions to
the API which have callbacks.

> 
>
https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
> File
> chrome/test/data/extensions/platform_apps/ad_view/flag_required/chrometest.js
> (right):
> 
>
https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
>
chrome/test/data/extensions/platform_apps/ad_view/flag_required/chrometest.js:31:
> chrome.test.assertEq(true, adview.hasAttribute('ad-network'));
> On 2013/04/04 04:33:52, benwells wrote:
> > On 2013/04/03 18:30:05, rpaquay wrote:
> > > On 2013/04/03 02:40:03, benwells wrote:
> > > > Why is this expected to be true?
> > > Because there is a value for the "ad-network" attribute in the index.html
> > page.
> > > Basically, this test makes sure that <adview> behaves like an "unknown"
> > element
> > > when the adview flag is not set. This also means the "setTimeout" is not
> > > necessary in this case. I have updated the test to (hopefully) makes
things
> a
> > > bit clearer.
> > 
> > Nit: please add a comment to explain why the ad-network attribute is
expected
> > but not the others.
> 
> Done.
> 
>
https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
> File
>
chrome/test/data/extensions/platform_apps/ad_view/invalid_ad_network/chrometest.js
> (right):
> 
>
https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
>
chrome/test/data/extensions/platform_apps/ad_view/invalid_ad_network/chrometest.js:54:
> setTimeout(function() {
> On 2013/04/04 04:33:52, benwells wrote:
> > On 2013/04/03 18:30:05, rpaquay wrote:
> > > On 2013/04/03 02:40:03, benwells wrote:
> > > > I think you are avoiding chrome.runTests so can ensure test 1 happens
> before
> > > > document load and the others after.
> > > > 
> > > > Instead of putting in this timeout, you should call runTest3 when test 2
> > > > finishes. That would be clearer and more robust.
> > > 
> > > In this case, I was using "setTimeout" in "runTest3" for a different
> purpose:
> > I
> > > wanted to run code *after* the "DOMContentLoaded" event if fired and
> processed
> > > by all listeners -- the reason being that ad_view.js contains code
watching
> > for
> > > DOM change events which depends on when "DOMContentLoaded" is fired. Is
> there
> > a
> > > better way to achieve what I am trying to test?
> > 
> > OK, I see. Thanks for adding the comment, they really help with tests like
> this
> > where people unfamiliar with this area maybe end up working out why the test
> is
> > failing.
> > 
> > This setup feels slightly fragile though. It is currently the case that test
2
> > and test 3 will be running simultaneously. I don't think it can happen now,
> but
> > with some slight changes (e.g. one level of callbacks added) test 2 might
> finish
> > after test 3 has finished. This could mean a failure in test 2 would not get
> > caught.
> > 
> > It would be better to start test 3 once test 2 has completed. That would
> > guarantee the tests run sequentially, and also would ensure that test 3
> happens
> > after the loaded event has been processed (but you should keep an updated
> > comment explaining so if test 2 is ever removed a timeout is added back).
> 
> I see what you mean. I made a change that should enforces test3 runs only
after
> test2 succeeds.
> 
>
https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
> File
>
chrome/test/data/extensions/platform_apps/ad_view/src_not_exposed/chrometest.js
> (right):
> 
>
https://codereview.chromium.org/12967016/diff/27001/chrome/test/data/extensio...
>
chrome/test/data/extensions/platform_apps/ad_view/src_not_exposed/chrometest.js:5:
> function runTests() {
> On 2013/04/04 04:33:52, benwells wrote:
> > On 2013/04/03 18:30:05, rpaquay wrote:
> > > On 2013/04/03 02:40:03, benwells wrote:
> > > > Please add an initial comment here like in all the other test js files.
> > > 
> > > I am slightly confused about where to put comments :). I thought you asked
> me
> > to
> > > move the comments back to the ".cc" file. Did you mean to duplicate each
> > comment
> > > in both the ".cc" file and in the ".js" files?
> > 
> > Yes sorry that was what I meant. They are really useful in both places.
> 
> Done.

[Fady Samuel, 2013-04-09 21:15:38]

The code cleanup in ad_view.js is good. LGTM with one nit.

https://codereview.chromium.org/12967016/diff/63014/chrome/renderer/resources...
File chrome/renderer/resources/extensions/ad_view.js (right):

https://codereview.chromium.org/12967016/diff/63014/chrome/renderer/resources...
chrome/renderer/resources/extensions/ad_view.js:405:
//this.browserPluginNode_.setAttribute('src', newValue);
Remove comment? Also, why are you switching from attribute to property?

[rpaquay, 2013-04-22 17:19:18]

On 2013/04/09 21:15:38, Fady Samuel wrote:
> The code cleanup in ad_view.js is good. LGTM with one nit.
> 
>
https://codereview.chromium.org/12967016/diff/63014/chrome/renderer/resources...
> File chrome/renderer/resources/extensions/ad_view.js (right):
> 
>
https://codereview.chromium.org/12967016/diff/63014/chrome/renderer/resources...
> chrome/renderer/resources/extensions/ad_view.js:405:
> //this.browserPluginNode_.setAttribute('src', newValue);
> Remove comment? Also, why are you switching from attribute to property?

Added comments explaining reason of usage of property instead attribute.

[rpaquay, 2013-04-22 17:20:08]

Rebasing.

https://codereview.chromium.org/12967016/diff/63014/chrome/renderer/resources...
File chrome/renderer/resources/extensions/ad_view.js (right):

https://codereview.chromium.org/12967016/diff/63014/chrome/renderer/resources...
chrome/renderer/resources/extensions/ad_view.js:405:
//this.browserPluginNode_.setAttribute('src', newValue);
On 2013/04/09 21:15:38, Fady Samuel wrote:
> Remove comment? Also, why are you switching from attribute to property?

Added comment explaining reason.

[commit-bot: I haz the power, 2013-04-22 17:22:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/12967016/105001

[commit-bot: I haz the power, 2013-04-22 17:22:40]

Presubmit check for 12967016-105001 failed and returned exit status 1.

INFO:root:Found 55 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/component_updater/test/component_updater_service_unittest.cc

Presubmit checks took 5.4s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[commit-bot: I haz the power, 2013-04-22 17:35:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/12967016/105001

[commit-bot: I haz the power, 2013-04-22 17:41:47]

Presubmit check for 12967016-105001 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 55 file(s).

[commit-bot: I haz the power, 2013-04-22 18:11:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/12967016/105001

[commit-bot: I haz the power, 2013-04-22 18:39:07]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-04-22 19:42:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/12967016/134001

[commit-bot: I haz the power, 2013-04-22 21:12:53]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-04-23 22:22:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rpaquay@chromium.org/12967016/147001

[commit-bot: I haz the power, 2013-04-24 05:10:10]

Change committed as 196036