balta2ar / brotab

Control your browser's tabs from the command line
MIT License
406 stars 29 forks source link

implements query command which directly exposes chrome.tabs.query #10

Closed pkfm closed 4 years ago

pkfm commented 4 years ago

This exposes the chrome.tabs.query method through a new brotab command query. chrome.tabs.query({active: True}) was already exposed via the brotab command active, and chrome.tabs.query({}), via list. I was particularly interested in exposing the audible, title, and url parameters. The other parameters are not of immediate interest to me, but it seems better to implement a single function that handles them all rather than a seperate functions for each parameter. This way they can be combined in one call.

This also allows the list and active commands to be rewritten as basically aliases to query with specific parameters. In fact, query without any arguments produces the same output as list. Other more useful parameters can similarly be aliased.

The object argument to chrome.tabs.query accepts many optional boolean arguments. These have been implented for each argument name in the form [-+]<name>, where the + prefix denotes True and the - prefix denotes False. This makes for (slightly) more concise commands when compared to something like -foo True -bar False. Non-boolean parameters assume the traditional -<name> format, which, in relation to the +/- syntax, may be confusing to some. It also doubles the number of parameters for that subset of the command. Is this acceptable? The entire queryInfo object can be passed as a string literal through the info argument.

The windowId parameter may be completely useless. I have not checked if brotab performs any conversion or keeps any intermediate table.

The name query is consistent with the chrome.tabs api, but query is an argument of search and possibly other brotab commands. It is not really an issue for usage, but might be within source.

The whole query to the js portion is base 64 encoded via a brotab utility function because I had difficulty preventing mangling with quoting and escaping. This is probably good for long arrays of urls, but overkill for one or two booleans. A check for the presences of certain parameters can pivot appraches, but I don't know if thats necessary.

balta2ar commented 4 years ago

This pull request is awesome!

One thing that concerns me is that now since you allow arbitrary "info" objects in the command line arguments, and they are passed "as is" to a browser, it makes it so easy for the user to craft invalid queries, which will lead to errors, and onSuccess not being called. Which means meditator will not receive a reply.

I haven't been very careful with the error handling in this project so far, to be honest, but it may be the case when proper error reporting should be done right, at least for that single specific query.

Does it make sense? What do you think?

pkfm commented 4 years ago

Good catch. Seems I had only tested clean bools and strings lol. So I've added some checks in api.py to ensure the info argument is at least valid json and can eventually be coerced into a proper javascript object. And I added a failure handle for queryTabs in background.js, which posts an empty response and allows return when chrome.tabs.query fails. I also added some type coercion in queryTabs so that, among other things, anything dynamically constructing an info argument can sort of lazily quote all keys and values and things "just work". That part might belong in utility functions, but I'm not sure if there is any use for it elsewhere yet.