Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10381143:
Add filtered event support to event.js. (Closed)
|
Created:
8 years, 7 months ago by koz (OOO until 15th September) Modified:
7 years, 9 months ago CC:
chromium-reviews, Aaron Boodman, mihaip-chromium-reviews_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org, not at google - send to devlin, benwells Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd filtered event support to event.js.
BUG=121479
Patch Set 1 #
Total comments: 10
Patch Set 2 : respond to comments #Patch Set 3 : respond to comments #
Total comments: 10
Patch Set 4 : respond to comments #
Messages
Total messages: 11 (0 generated)
https://chromiumcodereview.appspot.com/10381143/diff/1/chrome/renderer/extens... File chrome/renderer/extensions/event_unittest.cc (right): https://chromiumcodereview.appspot.com/10381143/diff/1/chrome/renderer/extens... chrome/renderer/extensions/event_unittest.cc:157: " 'hostSuffix': 'google.com'," this filter should be url: {etc} https://chromiumcodereview.appspot.com/10381143/diff/1/chrome/renderer/resour... File chrome/renderer/resources/extensions/event.js (right): https://chromiumcodereview.appspot.com/10381143/diff/1/chrome/renderer/resour... chrome/renderer/resources/extensions/event.js:9: var DetachFilteredEvent = eventBindingsNatives.DetachFilteredEvent; I don't see these in this CL. https://chromiumcodereview.appspot.com/10381143/diff/1/chrome/renderer/resour... chrome/renderer/resources/extensions/event.js:59: // Only attach / detach on the first / last listener removed. Is this proper formatting? I thought indent should still be +2 from the Unfiltered* line. https://chromiumcodereview.appspot.com/10381143/diff/1/chrome/renderer/resour... chrome/renderer/resources/extensions/event.js:147: "supportsFilters": false, nit: quotes are unnecessary https://chromiumcodereview.appspot.com/10381143/diff/1/chrome/renderer/resour... chrome/renderer/resources/extensions/event.js:310: this.removeListener(listener); This will result in detach being called with manual=true. Why not just call detach directly?
https://chromiumcodereview.appspot.com/10381143/diff/1/chrome/renderer/extens... File chrome/renderer/extensions/event_unittest.cc (right): https://chromiumcodereview.appspot.com/10381143/diff/1/chrome/renderer/extens... chrome/renderer/extensions/event_unittest.cc:157: " 'hostSuffix': 'google.com'," On 2012/05/15 19:28:27, Matt Perry wrote: > this filter should be url: {etc} Fixed here and elsewhere where the 'url' field was a {} not a [{}]. (Test added, too.) https://chromiumcodereview.appspot.com/10381143/diff/1/chrome/renderer/resour... File chrome/renderer/resources/extensions/event.js (right): https://chromiumcodereview.appspot.com/10381143/diff/1/chrome/renderer/resour... chrome/renderer/resources/extensions/event.js:9: var DetachFilteredEvent = eventBindingsNatives.DetachFilteredEvent; On 2012/05/15 19:28:27, Matt Perry wrote: > I don't see these in this CL. These are only exercised by the unit test where we provide an implementation (via OverrideNativeHandler()). I'm working on a separate patch where I implement the real version of these (which will be covered by a browser test). https://chromiumcodereview.appspot.com/10381143/diff/1/chrome/renderer/resour... chrome/renderer/resources/extensions/event.js:59: // Only attach / detach on the first / last listener removed. On 2012/05/15 19:28:27, Matt Perry wrote: > Is this proper formatting? I thought indent should still be +2 from the > Unfiltered* line. Done. https://chromiumcodereview.appspot.com/10381143/diff/1/chrome/renderer/resour... chrome/renderer/resources/extensions/event.js:147: "supportsFilters": false, On 2012/05/15 19:28:27, Matt Perry wrote: > nit: quotes are unnecessary Done. https://chromiumcodereview.appspot.com/10381143/diff/1/chrome/renderer/resour... chrome/renderer/resources/extensions/event.js:310: this.removeListener(listener); On 2012/05/15 19:28:27, Matt Perry wrote: > This will result in detach being called with manual=true. Why not just call > detach directly? Indeed that is what happens and this is dead code. Removed.
https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/ext... File chrome/renderer/extensions/event_unittest.cc (right): https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/ext... chrome/renderer/extensions/event_unittest.cc:157: " url: [{hostSuffix: 'google.com'}]," url should not be an array, it should be a single object. filter itself can be an array. i.e. filter = [ { url: {hostSuffix: 'google.com'} }, { url: {hostPrefix: 'mail.yahoo'} } ]; https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/res... File chrome/renderer/resources/extensions/event.js (right): https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/res... chrome/renderer/resources/extensions/event.js:54: this.eventName_ = event.eventName_; why bother caching eventName_? can't you access it through event_? https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/res... chrome/renderer/resources/extensions/event.js:307: chrome.Event.prototype.detach = function() { did you mean to make this a "public" method? when would an extension want to call event.detach()?
https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/ext... File chrome/renderer/extensions/event_unittest.cc (right): https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/ext... chrome/renderer/extensions/event_unittest.cc:157: " url: [{hostSuffix: 'google.com'}]," On 2012/05/16 20:45:00, Matt Perry wrote: > url should not be an array, it should be a single object. filter itself can be > an array. i.e. filter = [ > { url: {hostSuffix: 'google.com'} }, > { url: {hostPrefix: 'mail.yahoo'} } > ]; Done. https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/res... File chrome/renderer/resources/extensions/event.js (right): https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/res... chrome/renderer/resources/extensions/event.js:54: this.eventName_ = event.eventName_; On 2012/05/16 20:45:00, Matt Perry wrote: > why bother caching eventName_? can't you access it through event_? Done. https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/res... chrome/renderer/resources/extensions/event.js:307: chrome.Event.prototype.detach = function() { On 2012/05/16 20:45:00, Matt Perry wrote: > did you mean to make this a "public" method? when would an extension want to > call event.detach()? Done.
I don't see the new snapshot.
Oops, should be up now.
https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/ext... File chrome/renderer/extensions/event_unittest.cc (right): https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/ext... chrome/renderer/extensions/event_unittest.cc:157: " url: [{hostSuffix: 'google.com'}]," On 2012/05/18 04:50:07, koz wrote: > On 2012/05/16 20:45:00, Matt Perry wrote: > > url should not be an array, it should be a single object. filter itself can be > > an array. i.e. filter = [ > > { url: {hostSuffix: 'google.com'} }, > > { url: {hostPrefix: 'mail.yahoo'} } > > ]; > > Done. Thinking about this a bit I wonder if this is really what we want? Having filter be a list of dictionaries of single values means that we have to handle the general case every time, rather than the efficient case, which is the common case. By that I mean isn't it more common to want to specify the set of valid URLs and the set of valid tabIds for an event, rather than a set of individual filters? addListener(cb, [ {url: {a}, tabId: 1}, {url: {b}, tabId: 2}]); This seems like a rare scenario - we are interested in this URL for this tab, and this URL for that tab. To express what seems to me to be a much more common scenario, that the user is interested in these URLs for this tab, you have to replicate tabId many times: addListener(cb, [ {url: {a}, tabId: 1}, {url: {b}, tabId: 1}, {url: {c}, tabId: 1}, /* etc */]); By having the url field be an array I think it would match user intent more closely, and be simpler to implement (as we don't need to remember which URLs correspond to which other sets of constraints). The rare case above is still expressable through multiple calls to addListener(). WDYT?
+battre - We're adding request filters to certain events (specifically to use for webNavigation events). We're discussing the exact format. I think it should be similar to the declarative webrequest API to avoid adding another different style of filtering. Do you have an opinion on this? https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/ext... File chrome/renderer/extensions/event_unittest.cc (right): https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/ext... chrome/renderer/extensions/event_unittest.cc:157: " url: [{hostSuffix: 'google.com'}]," On 2012/05/21 01:27:50, koz wrote: > On 2012/05/18 04:50:07, koz wrote: > > On 2012/05/16 20:45:00, Matt Perry wrote: > > > url should not be an array, it should be a single object. filter itself can > be > > > an array. i.e. filter = [ > > > { url: {hostSuffix: 'google.com'} }, > > > { url: {hostPrefix: 'mail.yahoo'} } > > > ]; > > > > Done. > > Thinking about this a bit I wonder if this is really what we want? Having filter > be a list of dictionaries of single values means that we have to handle the > general case every time, rather than the efficient case, which is the common > case. > > By that I mean isn't it more common to want to specify the set of valid URLs and > the set of valid tabIds for an event, rather than a set of individual filters? > > addListener(cb, [ > {url: {a}, tabId: 1}, > {url: {b}, tabId: 2}]); > > This seems like a rare scenario - we are interested in this URL for this tab, > and this URL for that tab. To express what seems to me to be a much more common > scenario, that the user is interested in these URLs for this tab, you have to > replicate tabId many times: > > addListener(cb, [ > {url: {a}, tabId: 1}, > {url: {b}, tabId: 1}, > {url: {c}, tabId: 1}, > /* etc */]); > > By having the url field be an array I think it would match user intent more > closely, and be simpler to implement (as we don't need to remember which URLs > correspond to which other sets of constraints). The rare case above is still > expressable through multiple calls to addListener(). > > WDYT? That's a good point. I was just trying to make it mirror the declarative webrequest API as much as possible: http://code.google.com/chrome/extensions/trunk/declarativeWebRequest.html You're right, it's probably rare to want to filter on more than one tabId. But there are other filter criteria, like resource type (image, script, main frame, etc). Also, with your proposed way of organizing filters, there's no way to express something like "(url matches a AND resourceType matches x) OR (url matches b AND resourceType matches y)". Dominic, what do you think?
https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/ext... File chrome/renderer/extensions/event_unittest.cc (right): https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/ext... chrome/renderer/extensions/event_unittest.cc:157: " url: [{hostSuffix: 'google.com'}]," On 2012/05/22 19:20:52, Matt Perry wrote: > On 2012/05/21 01:27:50, koz wrote: > > On 2012/05/18 04:50:07, koz wrote: > > > On 2012/05/16 20:45:00, Matt Perry wrote: > > > > url should not be an array, it should be a single object. filter itself > can > > be > > > > an array. i.e. filter = [ > > > > { url: {hostSuffix: 'google.com'} }, > > > > { url: {hostPrefix: 'mail.yahoo'} } > > > > ]; > > > > > > Done. > > > > Thinking about this a bit I wonder if this is really what we want? Having > filter > > be a list of dictionaries of single values means that we have to handle the > > general case every time, rather than the efficient case, which is the common > > case. > > > > By that I mean isn't it more common to want to specify the set of valid URLs > and > > the set of valid tabIds for an event, rather than a set of individual filters? > > > > addListener(cb, [ > > {url: {a}, tabId: 1}, > > {url: {b}, tabId: 2}]); > > > > This seems like a rare scenario - we are interested in this URL for this tab, > > and this URL for that tab. To express what seems to me to be a much more > common > > scenario, that the user is interested in these URLs for this tab, you have to > > replicate tabId many times: > > > > addListener(cb, [ > > {url: {a}, tabId: 1}, > > {url: {b}, tabId: 1}, > > {url: {c}, tabId: 1}, > > /* etc */]); > > > > By having the url field be an array I think it would match user intent more > > closely, and be simpler to implement (as we don't need to remember which URLs > > correspond to which other sets of constraints). The rare case above is still > > expressable through multiple calls to addListener(). > > > > WDYT? > > That's a good point. I was just trying to make it mirror the declarative > webrequest API as much as possible: > http://code.google.com/chrome/extensions/trunk/declarativeWebRequest.html > > You're right, it's probably rare to want to filter on more than one tabId. But > there are other filter criteria, like resource type (image, script, main frame, > etc). > > Also, with your proposed way of organizing filters, there's no way to express > something like "(url matches a AND resourceType matches x) OR (url matches b AND > resourceType matches y)". > > Dominic, what do you think? What do you think of providing 'url' and 'urls'. addListener(cb, [ {url: {a}, tabId: 1}, {url: {b}, tabId: 1}, {url: {c}, tabId: 1}, /* etc */]); and addListener(cb, [ {urls: [{a}, {b}, {c}], tabId: 1}, ]); would be equivalent. Extend the declarative webRequest API accordingly. However, we cannot do this to an infinite degree. E.g. addListener(cb, [ {urls: [{a, schemes: ['http']}, {b, schemes: ['http']}, {c, schemes: ['http']}], tabId: 1}, ]); could not be simplified any further.
https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/ext... File chrome/renderer/extensions/event_unittest.cc (right): https://chromiumcodereview.appspot.com/10381143/diff/6001/chrome/renderer/ext... chrome/renderer/extensions/event_unittest.cc:157: " url: [{hostSuffix: 'google.com'}]," On 2012/05/22 20:19:45, battre wrote: > On 2012/05/22 19:20:52, Matt Perry wrote: > > On 2012/05/21 01:27:50, koz wrote: > > > On 2012/05/18 04:50:07, koz wrote: > > > > On 2012/05/16 20:45:00, Matt Perry wrote: > > > > > url should not be an array, it should be a single object. filter itself > > can > > > be > > > > > an array. i.e. filter = [ > > > > > { url: {hostSuffix: 'google.com'} }, > > > > > { url: {hostPrefix: 'mail.yahoo'} } > > > > > ]; > > > > > > > > Done. > > > > > > Thinking about this a bit I wonder if this is really what we want? Having > > filter > > > be a list of dictionaries of single values means that we have to handle the > > > general case every time, rather than the efficient case, which is the common > > > case. > > > > > > By that I mean isn't it more common to want to specify the set of valid URLs > > and > > > the set of valid tabIds for an event, rather than a set of individual > filters? > > > > > > addListener(cb, [ > > > {url: {a}, tabId: 1}, > > > {url: {b}, tabId: 2}]); > > > > > > This seems like a rare scenario - we are interested in this URL for this > tab, > > > and this URL for that tab. To express what seems to me to be a much more > > common > > > scenario, that the user is interested in these URLs for this tab, you have > to > > > replicate tabId many times: > > > > > > addListener(cb, [ > > > {url: {a}, tabId: 1}, > > > {url: {b}, tabId: 1}, > > > {url: {c}, tabId: 1}, > > > /* etc */]); > > > > > > By having the url field be an array I think it would match user intent more > > > closely, and be simpler to implement (as we don't need to remember which > URLs > > > correspond to which other sets of constraints). The rare case above is still > > > expressable through multiple calls to addListener(). > > > > > > WDYT? > > > > That's a good point. I was just trying to make it mirror the declarative > > webrequest API as much as possible: > > http://code.google.com/chrome/extensions/trunk/declarativeWebRequest.html > > > > You're right, it's probably rare to want to filter on more than one tabId. But > > there are other filter criteria, like resource type (image, script, main > frame, > > etc). > > > > Also, with your proposed way of organizing filters, there's no way to express > > something like "(url matches a AND resourceType matches x) OR (url matches b > AND > > resourceType matches y)". > > > > Dominic, what do you think? > > What do you think of providing 'url' and 'urls'. > > addListener(cb, [ > {url: {a}, tabId: 1}, > {url: {b}, tabId: 1}, > {url: {c}, tabId: 1}, > /* etc */]); > > and > > addListener(cb, [ > {urls: [{a}, {b}, {c}], tabId: 1}, > ]); > > would be equivalent. Extend the declarative webRequest API accordingly. > > However, we cannot do this to an infinite degree. > > E.g. > addListener(cb, [ > {urls: [{a, schemes: ['http']}, {b, schemes: ['http']}, {c, schemes: > ['http']}], tabId: 1}, > ]); > could not be simplified any further. That sounds like a good compromise to me. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
