Update ActivityLog detection to use previous values, hrefs.

Update ActivityLog detection to use previous values, hrefs.

Now that the updates to the blink side have been pushed through, we can use
ActivityLog with previous values for setters and for hrefs of anchor tags.
Add the logic to do so and update tests.

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

[Devlin, 2014-05-08 18:24:02]

Adrienne, mind taking a look?  (Tryjobs aren't having any of it, but it compiles
and passes locally, so should be fairly stable).

[felt, 2014-05-08 18:43:55]

nice CL, just a few nits.
lgtm upon fixing comments.

please ping if any big changes arise while getting the trybots to run green

https://codereview.chromium.org/270623005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/ad_injection_browsertest.cc (right):

https://codereview.chromium.org/270623005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/activity_log/ad_injection_browsertest.cc:123: if
(enabled_) {
nit:
if (!enabled_) return;

https://codereview.chromium.org/270623005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/activity_log/ad_injection_browsertest.cc:188: const
std::string& last_test() const { return last_test_; }
where is this called? I can only find direct accesses to last_test_. did you
meant to replace them?

[Devlin, 2014-05-08 23:13:33]

Also added a LIKELY_NEW_AD, very similar to the other LIKELY.  I figure the more
heuristic data we can get, the better we can compare and see where we stand.

https://codereview.chromium.org/270623005/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/activity_log/ad_injection_browsertest.cc (right):

https://codereview.chromium.org/270623005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/activity_log/ad_injection_browsertest.cc:123: if
(enabled_) {
On 2014/05/08 18:43:55, felt wrote:
> nit:
> if (!enabled_) return;

Done.

https://codereview.chromium.org/270623005/diff/40001/chrome/browser/extension...
chrome/browser/extensions/activity_log/ad_injection_browsertest.cc:188: const
std::string& last_test() const { return last_test_; }
On 2014/05/08 18:43:55, felt wrote:
> where is this called? I can only find direct accesses to last_test_. did you
> meant to replace them?

Whoops!  Originally I used it in the test body, but changed to not need it.
Removed.

[felt, 2014-05-09 01:03:48]

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

https://codereview.chromium.org/270623005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/activity_log/activity_actions.cc:102: // acceptable
uses for this, like "Show my mail".
Hmm, this does seem very noisy (especially for links). Are you sure about this
one? It seems to make more sense down in CheckSrcModification().

[Devlin, 2014-05-09 15:48:17]

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

https://codereview.chromium.org/270623005/diff/60001/chrome/browser/extension...
chrome/browser/extensions/activity_log/activity_actions.cc:102: // acceptable
uses for this, like "Show my mail".
On 2014/05/09 01:03:54, felt wrote:
> Hmm, this does seem very noisy (especially for links). Are you sure about this
> one? It seems to make more sense down in CheckSrcModification().

TL;DR: Yes, it could be noisy, but just putting it in CheckSrcModification()
won't help.  It will only affect this metric, so we can ignore it if it's
ridiculously noisy, but if there are ways to improve, I'd be happy to hear them.
:)

It could be potentially noisy, yeah, but it will be equally noisy here and in
CheckSrcModification(), because, chances are, if the extension injects an
iframe/anchor/embed, they set its src/href at some point, too.  My hope is that:
- There are actually relatively few extensions which actively inject any kind of
link or iframe into the DOM.  It seems like most valid cases would actually have
the link/iframe be static HTML (like maybe seen on a browser action popup or an
options page), rather than dynamically generated.
- The noisiness is fully contained to this metric.  If it's drastically higher
than we'd ever expect, then we can know that it's too simple a heuristic, and
re-evaluate.

In an ideal world, we'd actually have much more finely-grained metrics, with
individual counts for non-local iframes/embeds/hrefs (and in the future, images,
etc), but this gets unwieldy fast, when we consider that we track injections,
modifications, potential removals, etc.  We might be able to sit down and figure
out a better metrics system for these (maybe using histogram enum instead of
just a count), but that's a discussion for a later branch point. ;)

[Devlin, 2014-05-09 16:00:17]

Mark, mind taking a look at histograms.xml?

[felt, 2014-05-09 16:08:49]

On 2014/05/09 15:48:17, D Cronin wrote:
>
https://codereview.chromium.org/270623005/diff/60001/chrome/browser/extension...
> File chrome/browser/extensions/activity_log/activity_actions.cc (right):
> 
>
https://codereview.chromium.org/270623005/diff/60001/chrome/browser/extension...
> chrome/browser/extensions/activity_log/activity_actions.cc:102: // acceptable
> uses for this, like "Show my mail".
> On 2014/05/09 01:03:54, felt wrote:
> > Hmm, this does seem very noisy (especially for links). Are you sure about
this
> > one? It seems to make more sense down in CheckSrcModification().
> 
> TL;DR: Yes, it could be noisy, but just putting it in CheckSrcModification()
> won't help.  It will only affect this metric, so we can ignore it if it's
> ridiculously noisy, but if there are ways to improve, I'd be happy to hear
them.
> :)
> 
> It could be potentially noisy, yeah, but it will be equally noisy here and in
> CheckSrcModification(), because, chances are, if the extension injects an
> iframe/anchor/embed, they set its src/href at some point, too.  My hope is
that:
> - There are actually relatively few extensions which actively inject any kind
of
> link or iframe into the DOM.  It seems like most valid cases would actually
have
> the link/iframe be static HTML (like maybe seen on a browser action popup or
an
> options page), rather than dynamically generated.
> - The noisiness is fully contained to this metric.  If it's drastically higher
> than we'd ever expect, then we can know that it's too simple a heuristic, and
> re-evaluate.
> 
> In an ideal world, we'd actually have much more finely-grained metrics, with
> individual counts for non-local iframes/embeds/hrefs (and in the future,
images,
> etc), but this gets unwieldy fast, when we consider that we track injections,
> modifications, potential removals, etc.  We might be able to sit down and
figure
> out a better metrics system for these (maybe using histogram enum instead of
> just a count), but that's a discussion for a later branch point. ;)

ok, lgtm again.

[Mark P, 2014-05-13 21:14:38]

lgtm

lgtm

[Devlin, 2014-05-14 22:12:26]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-05-14 22:14:39]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/270623005/...

[commit-bot: I haz the power, 2014-05-15 01:33:04]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-05-15 01:35:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-15 01:35:33]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[Devlin, 2014-05-15 15:25:38]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-05-15 15:26:58]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/270623005/...

[commit-bot: I haz the power, 2014-05-15 15:33:32]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[commit-bot: I haz the power, 2014-05-15 15:37:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-15 15:37:55]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)

[Devlin, 2014-05-15 16:07:09]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2014-05-15 16:09:04]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/270623005/...

[commit-bot: I haz the power, 2014-05-15 18:41:46]

Change committed as 270752