[Activity log] Rework extraction of URLs from argument lists

[Activity log] Rework extraction of URLs from argument lists

Redo the code for handling the tabs API and tab ID translation to make
it more generic.  Make it table driven: there is now a table of API
calls that can contain URL arguments, so it is easy to add additional
API calls that we want to have URLs extracted from.  Add a couple of
others I've come across in testing.

BUG=253368, 275701
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=222155

[mvrable, 2013-08-28 20:37:53]

No rush on reviewing this, but when you have a chance I think this is ready to
be looked at.

--Michael Vrable

[mvrable, 2013-08-28 21:37:27]

On 2013/08/28 20:37:53, mvrable wrote:
> No rush on reviewing this, but when you have a chance I think this is ready to
> be looked at.
> 
> --Michael Vrable

On second thought...

I was looking through APIs to figure out what to add in to the table as part of
a follow-up CL, and I realized that often we'll need to extract URLs out of
dictionary objects (at least, this is the case for the windows and bookmarks
calls).  I'm going to add some code in this CL to support that use case, but
that means there will be more changes coming.

--Michael Vrable

[mvrable, 2013-09-05 21:45:25]

kalman, would you be able to review a patch for me to some of the extension
activity logging code?  I'm trying to get some code reviewed by out-of-team
members.  If you need more background for reviewing I'm happy to answer
questions.

This particular patch cleans up the code responsible for processing the argument
list for API calls made by extensions (and also other monitored activity, such
as DOM manipulations performed by extensions) to extract URLs from the
arguments.  Any extracted URL is stored into a separate database column when the
extension activity is logged, so we can more easily query over URLs that are
referred to in API calls.

The old code had some hard-coded support for the chrome.tabs API.  This new code
is table-driven so we can support additional API calls much more easily.

I've added a couple of additional API calls to the table, but will do a more
systematic review of API calls and fill in the table further in a future CL.

--Michael Vrable

[not at google - send to devlin, 2013-09-06 17:36:14]

Small comments + a bigger comment, of which I'm not quite sure what to make of.

https://codereview.chromium.org/23545012/diff/25001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/23545012/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:69: const char*
arg_url_lookup;
this is kind of roundabout and looking at the code where it's implemented,
rather magical. I think an enum would be clearer, like

struct ApiInfo {
  enum Transformation {
    NO_TRANSFORMATION,
    LOOKUP_TAB_ID,
    EXTRACT_URL
  };

  ...
};

another option, if you envisage this to be too restrictive in the future, would
be to implement this as a callback.

https://codereview.chromium.org/23545012/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:98:
{Action::ACTION_API_CALL, "windows.create", 0, "url"},
Generally speaking - and this would be a much larger change - I feel like these
should be the property of APIs not the ActivityLog. That is, have a way of
marking in the API schemas what properties should be logged and parse that to
use here. That would be much more sustainable in the future.

https://codereview.chromium.org/23545012/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:121: if
(i->second->action_type != action_type)
Can this ever be false?

I think the comment on api_database_ is trying to address that question but it's
not clear why the comment is there nor why this isn't a CHECK.

https://codereview.chromium.org/23545012/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:205: base::ListValue*
tab_list;
= NULL

https://codereview.chromium.org/23545012/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:217: for (int i =
tab_list->GetSize(); i >= 0; i--) {
--i not i--.

and shouldn't this be tab_list->GetSize() - 1 ?

https://codereview.chromium.org/23545012/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:231: DictionaryValue*
dict;
= NULL

[mvrable, 2013-09-06 21:27:42]

Thanks for the feedback!  I've uploaded a new version with fixes for all of the
small issues I think you mentioned.

--Michael Vrable

https://codereview.chromium.org/23545012/diff/25001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/23545012/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:69: const char*
arg_url_lookup;
On 2013/09/06 17:36:15, kalman wrote:
> this is kind of roundabout and looking at the code where it's implemented,
> rather magical. I think an enum would be clearer, like
> 
> struct ApiInfo {
>   enum Transformation {
>     NO_TRANSFORMATION,
>     LOOKUP_TAB_ID,
>     EXTRACT_URL
>   };
> 
>   ...
> };
> 
> another option, if you envisage this to be too restrictive in the future,
would
> be to implement this as a callback.

That is a little more verbose but I don't mind changing this to an enum. 
Updated code is uploaded--how does this look?

https://codereview.chromium.org/23545012/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:98:
{Action::ACTION_API_CALL, "windows.create", 0, "url"},
On 2013/09/06 17:36:15, kalman wrote:
> Generally speaking - and this would be a much larger change - I feel like
these
> should be the property of APIs not the ActivityLog. That is, have a way of
> marking in the API schemas what properties should be logged and parse that to
> use here. That would be much more sustainable in the future.

Agreed, it would be much more sustainable if we had this information the
extension API metadata or somewhere like that, though that would take more work
to implement.  We'd also need to figure out how to support calls in the DOM in
that case.

Are you okay with this version so that we have something which works for us for
now, and then looking into something more generic later?

https://codereview.chromium.org/23545012/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:121: if
(i->second->action_type != action_type)
On 2013/09/06 17:36:15, kalman wrote:
> Can this ever be false?
> 
> I think the comment on api_database_ is trying to address that question but
it's
> not clear why the comment is there nor why this isn't a CHECK.

Yes, this could potentially be false since we could receive input (an Action
object) that matched the api_name but not action_type.  There's nothing (that I
know) that prevents an api_name from being used under multiple types.

As structured, we can only support adding behavior to extract a URL from a
single instance of each api_name (to simplify the lookup), but that's fine since
we currently don't need any better and it could always be changed later if
needed.

https://codereview.chromium.org/23545012/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:205: base::ListValue*
tab_list;
On 2013/09/06 17:36:15, kalman wrote:
> = NULL

Done.

https://codereview.chromium.org/23545012/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:217: for (int i =
tab_list->GetSize(); i >= 0; i--) {
On 2013/09/06 17:36:15, kalman wrote:
> --i not i--.

Done.

> and shouldn't this be tab_list->GetSize() - 1 ?

Yes, good catch.  (It worked before because presumably GetInteger had a bounds
check and returned false, but the code was incorrect.)

https://codereview.chromium.org/23545012/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:231: DictionaryValue*
dict;
On 2013/09/06 17:36:15, kalman wrote:
> = NULL

Done.

[not at google - send to devlin, 2013-09-06 22:05:05]

lgtm

https://codereview.chromium.org/23545012/diff/25001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/23545012/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:98:
{Action::ACTION_API_CALL, "windows.create", 0, "url"},
On 2013/09/06 21:27:42, mvrable wrote:
> On 2013/09/06 17:36:15, kalman wrote:
> > Generally speaking - and this would be a much larger change - I feel like
> these
> > should be the property of APIs not the ActivityLog. That is, have a way of
> > marking in the API schemas what properties should be logged and parse that
to
> > use here. That would be much more sustainable in the future.
> 
> Agreed, it would be much more sustainable if we had this information the
> extension API metadata or somewhere like that, though that would take more
work
> to implement.  We'd also need to figure out how to support calls in the DOM in
> that case.
> 
> Are you okay with this version so that we have something which works for us
for
> now, and then looking into something more generic later?

Totally fine to have this for now. It would be nice to work towards a
declarative system, though. Is there a bug for it? It may be good to reference
it here (or file a new one).

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

https://codereview.chromium.org/23545012/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:194: if (!action->args()
|| url_index < 0 ||
it doesn't seem that url_index can ever be < 0.

https://codereview.chromium.org/23545012/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:206: if
(api_info->arg_url_transform == NONE) {
use switch

[mvrable, 2013-09-06 22:32:49]

Updated.

I created a feature request for a more declarative system, in Issue 286492.

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

https://codereview.chromium.org/23545012/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:194: if (!action->args()
|| url_index < 0 ||
On 2013/09/06 22:05:05, kalman wrote:
> it doesn't seem that url_index can ever be < 0.

Not currently.  I was leaving that as an option (documented in struct ApiInfo)
for the case that we were storing information about an API in the list but
didn't need to extract a URL from the arguments.

If you'd prefer I drop this (since it isn't needed currently) I can do that.

https://codereview.chromium.org/23545012/diff/40001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:206: if
(api_info->arg_url_transform == NONE) {
On 2013/09/06 22:05:05, kalman wrote:
> use switch

Done.

[commit-bot: I haz the power, 2013-09-09 18:17:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvrable@chromium.org/23545012/53001

[commit-bot: I haz the power, 2013-09-10 00:40:25]

Change committed as 222155