Enabling AdInjectionBrowserTest after Activity Log refactoring.

Enabling AdInjectionBrowserTest after Activity Log refactoring.
Cleaning code that became redundant.

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

[pmarch, 2014-07-15 11:45:02]

Adrienne, Devlin

Please review this CL. Note, that there is no logging for Embed HTML element.

Thanks.

[Devlin, 2014-07-15 17:21:52]

Do we plan to have support for embeds ever?

https://codereview.chromium.org/384983003/diff/1/chrome/browser/extensions/ac...
File chrome/browser/extensions/activity_log/activity_actions.cc (right):

https://codereview.chromium.org/384983003/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/activity_actions.cc:57: namespace
ad_injection_constants {
nit: I think we can just take out the namespace and use kBlinkSetAttributeEvent,
etc directly.

https://codereview.chromium.org/384983003/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/activity_actions.cc:447: bool
arg_url_could_be_ad = UrlCouldBeAd(url);
nit: this is no longer the |arg_url_|, so we should probably rename it to
"new_url_could_be_ad".

https://codereview.chromium.org/384983003/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/activity_log/ad_injection/content_script.js
(left):

https://codereview.chromium.org/384983003/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/activity_log/ad_injection/content_script.js:362:
functions.push(function RemoveAdBySettingSrc() {
These don't work anymore?

https://codereview.chromium.org/384983003/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/activity_log/ad_injection/content_script.js
(right):

https://codereview.chromium.org/384983003/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/activity_log/ad_injection/content_script.js:258:
frame.src = kAdNetwork;
Since we have a lower-level api now, can we check things like
frame.setAttribute('src', kAdNetwork) as well?

[pmarch, 2014-07-16 10:34:17]

https://codereview.chromium.org/384983003/diff/1/chrome/browser/extensions/ac...
File chrome/browser/extensions/activity_log/activity_actions.cc (right):

https://codereview.chromium.org/384983003/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/activity_actions.cc:57: namespace
ad_injection_constants {
On 2014/07/15 17:21:52, Devlin wrote:
> nit: I think we can just take out the namespace and use
kBlinkSetAttributeEvent,
> etc directly.

Done.

https://codereview.chromium.org/384983003/diff/1/chrome/browser/extensions/ac...
chrome/browser/extensions/activity_log/activity_actions.cc:447: bool
arg_url_could_be_ad = UrlCouldBeAd(url);
On 2014/07/15 17:21:52, Devlin wrote:
> nit: this is no longer the |arg_url_|, so we should probably rename it to
> "new_url_could_be_ad".

Done.

https://codereview.chromium.org/384983003/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/activity_log/ad_injection/content_script.js
(left):

https://codereview.chromium.org/384983003/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/activity_log/ad_injection/content_script.js:362:
functions.push(function RemoveAdBySettingSrc() {
On 2014/07/15 17:21:52, Devlin wrote:
> These don't work anymore?
Yes, there is no tracking for embed elements by Activity Logger.

https://codereview.chromium.org/384983003/diff/1/chrome/test/data/extensions/...
File chrome/test/data/extensions/activity_log/ad_injection/content_script.js
(right):

https://codereview.chromium.org/384983003/diff/1/chrome/test/data/extensions/...
chrome/test/data/extensions/activity_log/ad_injection/content_script.js:258:
frame.src = kAdNetwork;
On 2014/07/15 17:21:52, Devlin wrote:
> Since we have a lower-level api now, can we check things like
> frame.setAttribute('src', kAdNetwork) as well?

Done.

[pmarch, 2014-07-16 10:35:29]

On 2014/07/15 17:21:52, Devlin wrote:
> Do we plan to have support for embeds ever?

It is not in the plans, but if this needed to track flash ads, I can enable
logging for embed elements. Do you want me to do so?

>
https://codereview.chromium.org/384983003/diff/1/chrome/browser/extensions/ac...
> File chrome/browser/extensions/activity_log/activity_actions.cc (right):
> 
>
https://codereview.chromium.org/384983003/diff/1/chrome/browser/extensions/ac...
> chrome/browser/extensions/activity_log/activity_actions.cc:57: namespace
> ad_injection_constants {
> nit: I think we can just take out the namespace and use
kBlinkSetAttributeEvent,
> etc directly.
> 
>
https://codereview.chromium.org/384983003/diff/1/chrome/browser/extensions/ac...
> chrome/browser/extensions/activity_log/activity_actions.cc:447: bool
> arg_url_could_be_ad = UrlCouldBeAd(url);
> nit: this is no longer the |arg_url_|, so we should probably rename it to
> "new_url_could_be_ad".
> 
>
https://codereview.chromium.org/384983003/diff/1/chrome/test/data/extensions/...
> File chrome/test/data/extensions/activity_log/ad_injection/content_script.js
> (left):
> 
>
https://codereview.chromium.org/384983003/diff/1/chrome/test/data/extensions/...
> chrome/test/data/extensions/activity_log/ad_injection/content_script.js:362:
> functions.push(function RemoveAdBySettingSrc() {
> These don't work anymore?
> 
>
https://codereview.chromium.org/384983003/diff/1/chrome/test/data/extensions/...
> File chrome/test/data/extensions/activity_log/ad_injection/content_script.js
> (right):
> 
>
https://codereview.chromium.org/384983003/diff/1/chrome/test/data/extensions/...
> chrome/test/data/extensions/activity_log/ad_injection/content_script.js:258:
> frame.src = kAdNetwork;
> Since we have a lower-level api now, can we check things like
> frame.setAttribute('src', kAdNetwork) as well?

[Devlin, 2014-07-16 20:14:49]

https://codereview.chromium.org/384983003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/activity_actions.cc (right):

https://codereview.chromium.org/384983003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/activity_log/activity_actions.cc:57: const char
kBlinkSetAttributeEvent[] = "blinkSetAttribute";
These should go in the anonymous namespace above.

https://codereview.chromium.org/384983003/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/activity_log/ad_injection/content_script.js
(left):

https://codereview.chromium.org/384983003/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/activity_log/ad_injection/content_script.js:364:
return INJECTION_REMOVED_AD;
Even though these are using embeds now, they shouldn't need to be.  Can we
change them to use iframes and keep the tests?

[pmarch, 2014-07-16 22:08:36]

On 2014/07/16 10:35:29, pmarch wrote:
> On 2014/07/15 17:21:52, Devlin wrote:
> > Do we plan to have support for embeds ever?
> 
> It is not in the plans, but if this needed to track flash ads, I can enable
> logging for embed elements. Do you want me to do so?
> 
> >
>
https://codereview.chromium.org/384983003/diff/1/chrome/browser/extensions/ac...
> > File chrome/browser/extensions/activity_log/activity_actions.cc (right):
> > 
> >
>
https://codereview.chromium.org/384983003/diff/1/chrome/browser/extensions/ac...
> > chrome/browser/extensions/activity_log/activity_actions.cc:57: namespace
> > ad_injection_constants {
> > nit: I think we can just take out the namespace and use
> kBlinkSetAttributeEvent,
> > etc directly.
> > 
> >
>
https://codereview.chromium.org/384983003/diff/1/chrome/browser/extensions/ac...
> > chrome/browser/extensions/activity_log/activity_actions.cc:447: bool
> > arg_url_could_be_ad = UrlCouldBeAd(url);
> > nit: this is no longer the |arg_url_|, so we should probably rename it to
> > "new_url_could_be_ad".
> > 
> >
>
https://codereview.chromium.org/384983003/diff/1/chrome/test/data/extensions/...
> > File chrome/test/data/extensions/activity_log/ad_injection/content_script.js
> > (left):
> > 
> >
>
https://codereview.chromium.org/384983003/diff/1/chrome/test/data/extensions/...
> > chrome/test/data/extensions/activity_log/ad_injection/content_script.js:362:
> > functions.push(function RemoveAdBySettingSrc() {
> > These don't work anymore?
> > 
> >
>
https://codereview.chromium.org/384983003/diff/1/chrome/test/data/extensions/...
> > File chrome/test/data/extensions/activity_log/ad_injection/content_script.js
> > (right):
> > 
> >
>
https://codereview.chromium.org/384983003/diff/1/chrome/test/data/extensions/...
> > chrome/test/data/extensions/activity_log/ad_injection/content_script.js:258:
> > frame.src = kAdNetwork;
> > Since we have a lower-level api now, can we check things like
> > frame.setAttribute('src', kAdNetwork) as well?

Do you want me to enable tracing embeds in Activity Logger? Is it needed for ad
replacement tracking?

[Devlin, 2014-07-16 22:18:26]

On 2014/07/16 22:08:36, pmarch wrote:

> Do you want me to enable tracing embeds in Activity Logger? Is it needed for
ad
> replacement tracking?

Whoops, meant to answer that one.  No, we can leave embeds out for now.  Thanks,
though. :)

[pmarch, 2014-07-17 14:04:26]

https://codereview.chromium.org/384983003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/activity_actions.cc (right):

https://codereview.chromium.org/384983003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/activity_log/activity_actions.cc:57: const char
kBlinkSetAttributeEvent[] = "blinkSetAttribute";
On 2014/07/16 20:14:49, Devlin wrote:
> These should go in the anonymous namespace above.

Done.

Out of curiosity, why does it need to go to anonymous namespace?

https://codereview.chromium.org/384983003/diff/20001/chrome/test/data/extensi...
File chrome/test/data/extensions/activity_log/ad_injection/content_script.js
(left):

https://codereview.chromium.org/384983003/diff/20001/chrome/test/data/extensi...
chrome/test/data/extensions/activity_log/ad_injection/content_script.js:364:
return INJECTION_REMOVED_AD;
On 2014/07/16 20:14:49, Devlin wrote:
> Even though these are using embeds now, they shouldn't need to be.  Can we
> change them to use iframes and keep the tests?

Done.

[Devlin, 2014-07-17 15:31:52]

lgtm if the bots are happy.

Thanks for all your work on this; it's much cleaner now.  It's great to see CLs
that net -200 LOC. =)

https://codereview.chromium.org/384983003/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/activity_actions.cc (right):

https://codereview.chromium.org/384983003/diff/20001/chrome/browser/extension...
chrome/browser/extensions/activity_log/activity_actions.cc:57: const char
kBlinkSetAttributeEvent[] = "blinkSetAttribute";
On 2014/07/17 14:04:26, pmarch wrote:
> On 2014/07/16 20:14:49, Devlin wrote:
> > These should go in the anonymous namespace above.
> 
> Done.
> 
> Out of curiosity, why does it need to go to anonymous namespace?

If it's in the global namespace, you potentially have to worry about naming
conflicts, so it's good practice to put everything that's only used in one cc
file in an anonymous namespace.

https://codereview.chromium.org/384983003/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/activity_log/ad_injection/content_script.js
(right):

https://codereview.chromium.org/384983003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/activity_log/ad_injection/content_script.js:338: //
Remove an existing embed ad by setting it's src to a non-ad network.
nit: update comment.

[pmarch, 2014-07-17 16:35:46]

https://codereview.chromium.org/384983003/diff/40001/chrome/test/data/extensi...
File chrome/test/data/extensions/activity_log/ad_injection/content_script.js
(right):

https://codereview.chromium.org/384983003/diff/40001/chrome/test/data/extensi...
chrome/test/data/extensions/activity_log/ad_injection/content_script.js:338: //
Remove an existing embed ad by setting it's src to a non-ad network.
On 2014/07/17 15:31:52, Devlin wrote:
> nit: update comment.

Done.

[pmarch, 2014-07-18 08:51:07]

The CQ bit was checked by pmarch@chromium.org

[commit-bot: I haz the power, 2014-07-18 08:51:31]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pmarch@chromium.org/384983003/60001

[commit-bot: I haz the power, 2014-07-18 10:50:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-18 12:55:47]

Change committed as 284067