Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9104018:
Modify schema to include defaulting information. (Closed)
|
Created:
8 years, 10 months ago by Greg Billock Modified:
8 years, 10 months ago CC:
chromium-reviews, dhollowa+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionModify schema to include defaulting information.
R=jhawkins@chromium.org,binji@chromium.org,groby@chromium.org
BUG=110636
TEST=None yet
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=121330
Patch Set 1 #
Total comments: 1
Patch Set 2 : Move to separate table. #Patch Set 3 : Add accessors, tests. #
Total comments: 26
Patch Set 4 : Strip all matching logic (going in registry) #Patch Set 5 : Fix forward decl. #
Total comments: 11
Patch Set 6 : Add remove method, test. Fix lint, comments. #
Total comments: 1
Patch Set 7 : Fix url prefix to pattern. Add GetAll method. #Patch Set 8 : Take out extension_url #Patch Set 9 : Split off .cc file for win/mac #Patch Set 10 : Add explicit destructor for mac. #Patch Set 11 : Add missing destructor decl. #
Messages
Total messages: 20 (0 generated)
https://chromiumcodereview.appspot.com/9104018/diff/1/chrome/browser/webdata/... File chrome/browser/webdata/web_intents_table.cc (right): https://chromiumcodereview.appspot.com/9104018/diff/1/chrome/browser/webdata/... chrome/browser/webdata/web_intents_table.cc:60: "default_action VARCHAR," These don't belong in the web_intents table. Defaults are per-filter, so we need new table web_intents_defaults that is keyed off action, type.
Here's a proposal for how to store defaulting information. Alternatives I considered: 1. Storing defaults in a side table. 2. Storing a lot simpler schema, since it might change I chose this way to minimize future schema changes, which are a pain to support, and to use the main table so as to minimize lookups. I think this information will be very dense wrt the main table, so there's probably no reason to separate it. The idea here is that each service can have a defaulting scope across action, type, and url of invoking page. If the default is set explicitly by the user, we write down that date. If we suppress the default for some reason, or for some period of time, we write that down (i.e. a new app is installed, or the current app seems to stop working). My intention is to build the defaulting rules in at this level, so callers will have an API to limit results to the default if one exists (perhaps adding machinery to existing methods, perhaps renaming, perhaps replacing, not sure yet).
On 2012/01/30 20:53:42, Greg Billock wrote: > Here's a proposal for how to store defaulting information. Alternatives I > considered: > > 1. Storing defaults in a side table. > 2. Storing a lot simpler schema, since it might change > > I chose this way to minimize future schema changes, which are a pain to support, > and to use the main table so as to minimize lookups. I think this information > will be very dense wrt the main table, so there's probably no reason to separate > it. > Anecdotally, updating the schema is not hard; there's already update code and tests in place. This should not be a deciding factor in the design of the schema. > The idea here is that each service can have a defaulting scope across action, > type, and url of invoking page. If the default is set explicitly by the user, we > write down that date. If we suppress the default for some reason, or for some > period of time, we write that down (i.e. a new app is installed, or the current > app seems to stop working). > > My intention is to build the defaulting rules in at this level, so callers will > have an API to limit results to the default if one exists (perhaps adding > machinery to existing methods, perhaps renaming, perhaps replacing, not sure > yet). We're overloading too much functionality into one API. Default registration is not 1:1 with all registrations; it's 1:1 with [action,type] values, and should be a foreign key into the web_intents table.
I think I disagree... We want defaults on a per type level, which is not possible if it's in the main table. (E.g. imagine an image/* handler, but you only want it to handle image/png, for whatever reason) I'd rather see us use a side table - there's a 1:m relation between defaults and registered intents. (Not to mention that extension-side intents are not mentioned in that table, but we certainly should store defaults if they point to extensions. Side bar: Is that still WDS?) > > My intention is to build the defaulting rules in at this level, so callers > will > have an API to limit results to the default if one exists (perhaps adding > machinery to existing methods, perhaps renaming, perhaps replacing, not > sure > yet). > Why not just handle that in the WebIntentsRegistry? That's where we've put "policy" code so far.
The schema now registers a row for each action/type, right? Or are action/type multi-valued? My sense was to store defaulting information alongside that registration. On Mon, Jan 30, 2012 at 1:18 PM, Rachel Blum <groby@chromium.org> wrote: > I think I disagree... We want defaults on a per type level, which is not > possible if it's in the main table. (E.g. imagine an image/* handler, but > you only want it to handle image/png, for whatever reason) I'm not sure I follow. The idea here is to allow a specification that for any action on an image/png, you can use this service by default? I'd envisioned the user capabilities being like scopes: users can choose default services per action, then by type, then by url. > I'd rather see us use a side table - there's a 1:m relation between defaults > and registered intents. (Not to mention that extension-side intents are not > mentioned in that table, but we certainly should store defaults if they > point to extensions. Side bar: Is that still WDS?) I was thinking each service/action/type specifier got its own row. Not correct? > >> >> >> My intention is to build the defaulting rules in at this level, so callers >> will >> have an API to limit results to the default if one exists (perhaps adding >> machinery to existing methods, perhaps renaming, perhaps replacing, not >> sure >> yet). > > > Why not just handle that in the WebIntentsRegistry? That's where we've put > "policy" code so far. Yeah. The logic is substantially simpler at this level. Basically you can augment our existing table scan to discard non-defaults or more-generic defaults as you find more specific entries. We can implement this same thing in WebIntentsRegistry, though, it probably makes little difference. Putting it down a bit allows a faint hope of putting it in SQL at some point, but this query would be pretty rough compared to our usual fare. But all this may be moot. We want to allow defaults for extensions, and those aren't even in the table, right? In that case, a side table is pretty much the way to go.
> The schema now registers a row for each action/type, right? Yes for explicit types. But keep in mind MIME types themselves allow wildcars, i.e. image/* > Or are > action/type multi-valued? My sense was to store defaulting information > alongside that registration. > Breaks down w/ wildcards > > I'm not sure I follow. The idea here is to allow a specification that > for any action on an image/png, you can use this service by default? > No, the idea is to be able to say "I want to share all PNGs on Flickr, but all BMPs on imgur", for example. > I'd envisioned the user capabilities being like scopes: users can > choose default services per action, then by type, then by url. > Per URL is not supported by your schema change, either - it allows us to specify exactly _one_ default per service end point. > > I was thinking each service/action/type specifier got its own row. Not > correct? > Not entirely. Wildcards come into play for type. Services can live in extensions, which are not listed in that table.
On Mon, Jan 30, 2012 at 2:15 PM, Rachel Blum <groby@chromium.org> wrote: > >> >> The schema now registers a row for each action/type, right? > > > Yes for explicit types. But keep in mind MIME types themselves allow > wildcars, i.e. image/* > >> >> Or are >> action/type multi-valued? My sense was to store defaulting information >> alongside that registration. > > > Breaks down w/ wildcards >> >> >> I'm not sure I follow. The idea here is to allow a specification that >> for any action on an image/png, you can use this service by default? > > > No, the idea is to be able to say "I want to share all PNGs on Flickr, but > all BMPs on imgur", for example. > >> >> I'd envisioned the user capabilities being like scopes: users can >> choose default services per action, then by type, then by url. > > > Per URL is not supported by your schema change, either - it allows us to > specify exactly _one_ default per service end point. >> >> >> I was thinking each service/action/type specifier got its own row. Not >> correct? > > > Not entirely. Wildcards come into play for type. Services can live in > extensions, which are not listed in that table. This seems like the biggest non-starter. I was imagining just storing multiple rows for the more complex cases like you mention above. I think that'd work for wildcards, but it might break existing assumptions; certainly its a pain I hadn't considered. Creating entries for extensions seems like a pretty big nonstarter.
OK, this change is ready for a look. Added accessors and tests. The theory of operation here is that the DB class stores default settings, and then retrieves them as desired. Right now it returns a superset of possible defaults; it is up to the caller to fine-tune the results and pick the right default. There's some room for improvement, but for now I kept the filtering simpler. On 2012/01/30 22:31:34, Greg Billock wrote: > On Mon, Jan 30, 2012 at 2:15 PM, Rachel Blum <mailto:groby@chromium.org> wrote: > > > >> > >> The schema now registers a row for each action/type, right? > > > > > > Yes for explicit types. But keep in mind MIME types themselves allow > > wildcars, i.e. image/* > > > >> > >> Or are > >> action/type multi-valued? My sense was to store defaulting information > >> alongside that registration. > > > > > > Breaks down w/ wildcards > >> > >> > >> I'm not sure I follow. The idea here is to allow a specification that > >> for any action on an image/png, you can use this service by default? > > > > > > No, the idea is to be able to say "I want to share all PNGs on Flickr, but > > all BMPs on imgur", for example. > > > >> > >> I'd envisioned the user capabilities being like scopes: users can > >> choose default services per action, then by type, then by url. > > > > > > Per URL is not supported by your schema change, either - it allows us to > > specify exactly _one_ default per service end point. > >> > >> > >> I was thinking each service/action/type specifier got its own row. Not > >> correct? > > > > > > Not entirely. Wildcards come into play for type. Services can live in > > extensions, which are not listed in that table. > > This seems like the biggest non-starter. I was imagining just storing > multiple rows for the more complex cases like you mention above. I > think that'd work for wildcards, but it might break existing > assumptions; certainly its a pain I hadn't considered. > > Creating entries for extensions seems like a pretty big nonstarter.
http://codereview.chromium.org/9104018/diff/9001/chrome/browser/intents/defau... File chrome/browser/intents/default_intent_service.h (right): http://codereview.chromium.org/9104018/diff/9001/chrome/browser/intents/defau... chrome/browser/intents/default_intent_service.h:8: #include <string> nit: Blank line after this. http://codereview.chromium.org/9104018/diff/9001/chrome/browser/intents/defau... chrome/browser/intents/default_intent_service.h:11: // Hold data related to a default settings for web intents service nit: s/Hold/Holds/ http://codereview.chromium.org/9104018/diff/9001/chrome/browser/intents/defau... chrome/browser/intents/default_intent_service.h:13: struct DefaultIntentService { DefaultWebIntentService http://codereview.chromium.org/9104018/diff/9001/chrome/browser/intents/defau... chrome/browser/intents/default_intent_service.h:14: string16 action; Document the member variables. http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.cc (right): http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.cc:5: #include <string> nit: <string> include goes after the web_intents_table.h include. http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.cc:169: bool TypeMatches(const string16 type_pattern, Document the method. http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.h (right): http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:43: // user_date Epoch time when the user made this default. What is |user_date| used for? http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:45: // service_url The URL of a service in the web_intents table. Why do we need two columns for URL?
http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.cc (right): http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.cc:177: // TODO(gbillock): put in MIME parameter matching here. We currently handle MIME type matching in WebIntentsRegistry::OnWebDataServiceRequestDone - I'd suggest the registry should also handle the filtering of defaults. http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.cc:179: return net::MatchesMimeType(pattern, mime); Does not work with wild cards on one hand side of the match. (I believe left hand side) MimeTypesAreEqual() in chrome/browser/intents/web_intents_registry.cc does that - maybe we should hoist it into net/base/mime_util.cc ? http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.cc:182: bool UrlMatches(const std::string& url_prefix, GURLs use string16 (since URLs can be unicode) http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.h (right): http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:42: // url_prefix URL prefix for which the default is invoked. I'm not sure what the url_prefix stands for? The page we invoked the default from? If so, do we want to actually allow that for sub-urls, or do we limit this to domains?
http://codereview.chromium.org/9104018/diff/9001/chrome/browser/intents/defau... File chrome/browser/intents/default_intent_service.h (right): http://codereview.chromium.org/9104018/diff/9001/chrome/browser/intents/defau... chrome/browser/intents/default_intent_service.h:8: #include <string> On 2012/02/05 05:21:51, James Hawkins wrote: > nit: Blank line after this. Done. http://codereview.chromium.org/9104018/diff/9001/chrome/browser/intents/defau... chrome/browser/intents/default_intent_service.h:11: // Hold data related to a default settings for web intents service On 2012/02/05 05:21:51, James Hawkins wrote: > nit: s/Hold/Holds/ Done. http://codereview.chromium.org/9104018/diff/9001/chrome/browser/intents/defau... chrome/browser/intents/default_intent_service.h:13: struct DefaultIntentService { On 2012/02/05 05:21:51, James Hawkins wrote: > DefaultWebIntentService Done. http://codereview.chromium.org/9104018/diff/9001/chrome/browser/intents/defau... chrome/browser/intents/default_intent_service.h:14: string16 action; On 2012/02/05 05:21:51, James Hawkins wrote: > Document the member variables. Added to class comment. http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.cc (right): http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.cc:5: #include <string> On 2012/02/05 05:21:51, James Hawkins wrote: > nit: <string> include goes after the web_intents_table.h include. Done. http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.cc:169: bool TypeMatches(const string16 type_pattern, On 2012/02/05 05:21:51, James Hawkins wrote: > Document the method. Done. http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.cc:177: // TODO(gbillock): put in MIME parameter matching here. On 2012/02/06 18:34:53, groby wrote: > We currently handle MIME type matching in > WebIntentsRegistry::OnWebDataServiceRequestDone - I'd suggest the registry > should also handle the filtering of defaults. That sounds good, especially if we already have stuff there to handle MIME type matching. Hoisting... http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.cc:179: return net::MatchesMimeType(pattern, mime); On 2012/02/06 18:34:53, groby wrote: > Does not work with wild cards on one hand side of the match. (I believe left > hand side) > > > MimeTypesAreEqual() in chrome/browser/intents/web_intents_registry.cc does that > - maybe we should hoist it into net/base/mime_util.cc ? Yeah, this assumes that the MIME type in the query (that is, the intent to be dispatched), will not be wildcarded (which I think is an appropriate restriction.) http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.cc:182: bool UrlMatches(const std::string& url_prefix, On 2012/02/06 18:34:53, groby wrote: > GURLs use string16 (since URLs can be unicode) GURL::spec() is UTF8, as are all DB entries where we store a url that I see. http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.h (right): http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:42: // url_prefix URL prefix for which the default is invoked. On 2012/02/06 18:34:53, groby wrote: > I'm not sure what the url_prefix stands for? The page we invoked the default > from? If so, do we want to actually allow that for sub-urls, or do we limit this > to domains? I'm not totally sure at this point. I want to leave it fairly flexible. I think this will do both. http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:43: // user_date Epoch time when the user made this default. On 2012/02/05 05:21:51, James Hawkins wrote: > What is |user_date| used for? Basically just to say that this is a default the user purposefully did. I think storing the date instead of a bool will be valuable for deciding on suppression logic. http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.h:45: // service_url The URL of a service in the web_intents table. On 2012/02/05 05:21:51, James Hawkins wrote: > Why do we need two columns for URL? I want to store extension defaults separately from web content defaults, so I think having two explicit fields is easier than storing the url and an extension/service flag.
http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.cc (right): http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.cc:182: bool UrlMatches(const std::string& url_prefix, Yes, but StartsWithASCII doesn't compare UTF8 :) On 2012/02/07 00:45:06, Greg Billock wrote: > On 2012/02/06 18:34:53, groby wrote: > > GURLs use string16 (since URLs can be unicode) > > GURL::spec() is UTF8, as are all DB entries where we store a url that I see.
Although I suppose it's moot since all this is gone now. http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... File chrome/browser/webdata/web_intents_table.cc (right): http://codereview.chromium.org/9104018/diff/9001/chrome/browser/webdata/web_i... chrome/browser/webdata/web_intents_table.cc:182: bool UrlMatches(const std::string& url_prefix, :-) True enough. For hostnames, it ought to work fine, but if we're talking about deeper segmentation, we need something better. On 2012/02/08 00:48:45, groby wrote: > Yes, but StartsWithASCII doesn't compare UTF8 :) > > On 2012/02/07 00:45:06, Greg Billock wrote: > > On 2012/02/06 18:34:53, groby wrote: > > > GURLs use string16 (since URLs can be unicode) > > > > GURL::spec() is UTF8, as are all DB entries where we store a url that I see. >
lgtm mod nits http://codereview.chromium.org/9104018/diff/21001/chrome/browser/intents/defa... File chrome/browser/intents/default_web_intent_service.h (right): http://codereview.chromium.org/9104018/diff/21001/chrome/browser/intents/defa... chrome/browser/intents/default_web_intent_service.h:18: // of the service or extension page handling the intent. The user_date nit: |user_date|, |suppression| http://codereview.chromium.org/9104018/diff/21001/chrome/browser/intents/defa... chrome/browser/intents/default_web_intent_service.h:22: string16 action; nit: Please document members. http://codereview.chromium.org/9104018/diff/21001/chrome/browser/webdata/web_... File chrome/browser/webdata/web_intents_table.h (right): http://codereview.chromium.org/9104018/diff/21001/chrome/browser/webdata/web_... chrome/browser/webdata/web_intents_table.h:34: // Web Intents Services are uniquely identified by the <service_url,action,type> nit: Intent (no s) http://codereview.chromium.org/9104018/diff/21001/chrome/browser/webdata/web_... chrome/browser/webdata/web_intents_table.h:90: bool SetDefaultService(const DefaultWebIntentService& default_service); No way to remove default services? http://codereview.chromium.org/9104018/diff/21001/chrome/browser/webdata/web_... File chrome/browser/webdata/web_intents_table_unittest.cc (right): http://codereview.chromium.org/9104018/diff/21001/chrome/browser/webdata/web_... chrome/browser/webdata/web_intents_table_unittest.cc:190: default_service.action = ASCIIToUTF16("action"); nit: Might want to use the pre-defined constants at the top of the file instead.
http://codereview.chromium.org/9104018/diff/21001/chrome/browser/intents/defa... File chrome/browser/intents/default_web_intent_service.h (right): http://codereview.chromium.org/9104018/diff/21001/chrome/browser/intents/defa... chrome/browser/intents/default_web_intent_service.h:18: // of the service or extension page handling the intent. The user_date On 2012/02/08 23:18:38, groby wrote: > nit: |user_date|, |suppression| Done. http://codereview.chromium.org/9104018/diff/21001/chrome/browser/intents/defa... chrome/browser/intents/default_web_intent_service.h:22: string16 action; On 2012/02/08 23:18:38, groby wrote: > nit: Please document members. I put this in the struct comment. Given the multi-field key, I thought that was clearer. Is something missing there that ought to be explained here? http://codereview.chromium.org/9104018/diff/21001/chrome/browser/webdata/web_... File chrome/browser/webdata/web_intents_table.h (right): http://codereview.chromium.org/9104018/diff/21001/chrome/browser/webdata/web_... chrome/browser/webdata/web_intents_table.h:34: // Web Intents Services are uniquely identified by the <service_url,action,type> On 2012/02/08 23:18:38, groby wrote: > nit: Intent (no s) ok http://codereview.chromium.org/9104018/diff/21001/chrome/browser/webdata/web_... chrome/browser/webdata/web_intents_table.h:90: bool SetDefaultService(const DefaultWebIntentService& default_service); On 2012/02/08 23:18:38, groby wrote: > No way to remove default services? Actually just adding that. http://codereview.chromium.org/9104018/diff/21001/chrome/browser/webdata/web_... File chrome/browser/webdata/web_intents_table_unittest.cc (right): http://codereview.chromium.org/9104018/diff/21001/chrome/browser/webdata/web_... chrome/browser/webdata/web_intents_table_unittest.cc:190: default_service.action = ASCIIToUTF16("action"); On 2012/02/08 23:18:38, groby wrote: > nit: Might want to use the pre-defined constants at the top of the file instead. ah, nice. yes.
http://codereview.chromium.org/9104018/diff/21001/chrome/browser/intents/defa... File chrome/browser/intents/default_web_intent_service.h (right): http://codereview.chromium.org/9104018/diff/21001/chrome/browser/intents/defa... chrome/browser/intents/default_web_intent_service.h:22: string16 action; I'm not sure what exactly is stored e.g. in |user_date| and |suppression|. I _assume_ it's a time_t when that became active. (Why not time_t, btw?) And while I'm on types, url_prefix should probably be URLPattern, so we can just MatchURL(). I'm also not sure about the distinction between |service_url| and |extension_url|. On 2012/02/08 23:34:06, Greg Billock wrote: > On 2012/02/08 23:18:38, groby wrote: > > nit: Please document members. > > I put this in the struct comment. Given the multi-field key, I thought that was > clearer. Is something missing there that ought to be explained here? http://codereview.chromium.org/9104018/diff/26001/chrome/browser/webdata/web_... File chrome/browser/webdata/web_intents_table.h (right): http://codereview.chromium.org/9104018/diff/26001/chrome/browser/webdata/web_... chrome/browser/webdata/web_intents_table.h:94: bool RemoveDefaultService(const DefaultWebIntentService& default_service); Should we instead pass in |action|,|type|,|url_prefix|?
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/9104018/28003
Change committed as 121330 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
