Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10827056:
Use IntentsQuery as the WDS consumer. By unencumbering WebIntentsRegistry of this responsibility we… (Closed)
|
Created:
8 years, 5 months ago by Steve McKay Modified:
8 years, 4 months ago CC:
chromium-reviews, gbillock+watch_chromium.org, smckay+watch_chromium.org, groby-ooo-7-16 Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionUse IntentsQuery as the WDS consumer. By unencumbering WebIntentsRegistry of this responsibility we'll be able to more easily accommodate new request types.
BUG=137907
TEST=WebIntentsRegistryTest.*
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148714
Patch Set 1 #
Total comments: 15
Patch Set 2 : Respond to review comments. #
Total comments: 1
Patch Set 3 : Rename ClaimQuery to ReleaseQuery. #
Total comments: 2
Patch Set 4 : Respond to review comments. #
Messages
Total messages: 11 (0 generated)
This change is a precursor to adding support for exposing all registered defaults as is needed to show them in the settings ui.
Just to clarify, this is an intermediary CL (as smaller CLs were requested by the team). The next step will be to decompose IntentsQuery into separate sub classes each of which handle the correct query type. This will eliminate the road block of dispatching results handling based on WDS result type.
You might consider templatizing the IntentsQuery instead of manually defining a bunch of derived classes. http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... File chrome/browser/intents/web_intents_registry.cc (right): http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... chrome/browser/intents/web_intents_registry.cc:156: WebIntentsRegistry* registry_; Please document variable and ownership. (I.e. I assume this is weak?) http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... chrome/browser/intents/web_intents_registry.cc:202: if (result->GetType() == WEB_INTENTS_RESULT) { Why is this not a callback? This way we can save ourselves a lot of type-based dispatching? (And the back-link to the registry, too) http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... chrome/browser/intents/web_intents_registry.cc:474: const Extension* WebIntentsRegistry::ExtensionForURL(const std::string& url) { Technically, functions should be in order of declaration in the header. If you move them, you might as well put them in the right order (or update the header) http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... File chrome/browser/intents/web_intents_registry.h (right): http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... chrome/browser/intents/web_intents_registry.h:140: QueryVector pending_queries_; Why no map?
http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... File chrome/browser/intents/web_intents_registry.cc (right): http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... chrome/browser/intents/web_intents_registry.cc:202: if (result->GetType() == WEB_INTENTS_RESULT) { On 2012/07/27 00:11:26, groby wrote: > Why is this not a callback? This way we can save ourselves a lot of type-based > dispatching? (And the back-link to the registry, too) We discussed doing that. With that design, we'd need a separate trampoline class anyway, so this is a simpler intermediate position. Ultimately, the WDS methods should take callbacks, making all this query object crap vanish. http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... chrome/browser/intents/web_intents_registry.cc:234: WebDataService::Handle h, Can get rid of handle param http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... chrome/browser/intents/web_intents_registry.cc:271: WebDataService::Handle h, We don't need the handle anymore, right? http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... chrome/browser/intents/web_intents_registry.cc:474: const Extension* WebIntentsRegistry::ExtensionForURL(const std::string& url) { Yeah, update the header. On 2012/07/27 00:11:26, groby wrote: > Technically, functions should be in order of declaration in the header. If you > move them, you might as well put them in the right order (or update the header) http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... File chrome/browser/intents/web_intents_registry.h (right): http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... chrome/browser/intents/web_intents_registry.h:137: bool ClaimQuery(IntentsQuery* query); How about UntrackQuery?
PTAL. http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... File chrome/browser/intents/web_intents_registry.cc (right): http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... chrome/browser/intents/web_intents_registry.cc:156: WebIntentsRegistry* registry_; On 2012/07/27 00:11:26, groby wrote: > Please document variable and ownership. (I.e. I assume this is weak?) Done. http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... chrome/browser/intents/web_intents_registry.cc:202: if (result->GetType() == WEB_INTENTS_RESULT) { On 2012/07/27 00:11:26, groby wrote: > Why is this not a callback? This way we can save ourselves a lot of type-based > dispatching? (And the back-link to the registry, too) IntentsQuery will be decomposed into sub-classes in a future CL. Since the team asked me to keep my CLs small, this seemed like a good first step in the change. http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... chrome/browser/intents/web_intents_registry.cc:234: WebDataService::Handle h, On 2012/07/27 00:41:29, Greg Billock wrote: > Can get rid of handle param Done. http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... chrome/browser/intents/web_intents_registry.cc:271: WebDataService::Handle h, On 2012/07/27 00:41:29, Greg Billock wrote: > We don't need the handle anymore, right? Done. http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... chrome/browser/intents/web_intents_registry.cc:474: const Extension* WebIntentsRegistry::ExtensionForURL(const std::string& url) { On 2012/07/27 00:41:29, Greg Billock wrote: > Yeah, update the header. > > On 2012/07/27 00:11:26, groby wrote: > > Technically, functions should be in order of declaration in the header. If you > > move them, you might as well put them in the right order (or update the > header) > Done. http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... File chrome/browser/intents/web_intents_registry.h (right): http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... chrome/browser/intents/web_intents_registry.h:140: QueryVector pending_queries_; On 2012/07/27 00:11:26, groby wrote: > Why no map? The map only existed to associate wds handles with intent query objects. This was needed to look up the query when processing a wds query response. Since the IntentsQuery is now the consumer of the wds response we no longer need to do this. The remaining purpose of the map was as an enumeration of active queries needed for cleanup on object delete. This only requires some type of a simple collection like a vector, so it makes sense to reduce the type to a vector.
On 2012/07/27 00:54:46, Steve McKay wrote: > PTAL. > > http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... > File chrome/browser/intents/web_intents_registry.cc (right): > > http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... > chrome/browser/intents/web_intents_registry.cc:156: WebIntentsRegistry* > registry_; > On 2012/07/27 00:11:26, groby wrote: > > Please document variable and ownership. (I.e. I assume this is weak?) > > Done. > > http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... > chrome/browser/intents/web_intents_registry.cc:202: if (result->GetType() == > WEB_INTENTS_RESULT) { > On 2012/07/27 00:11:26, groby wrote: > > Why is this not a callback? This way we can save ourselves a lot of type-based > > dispatching? (And the back-link to the registry, too) > > IntentsQuery will be decomposed into sub-classes in a future CL. Since the team > asked me to keep my CLs small, this seemed like a good first step in the change. > > http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... > chrome/browser/intents/web_intents_registry.cc:234: WebDataService::Handle h, > On 2012/07/27 00:41:29, Greg Billock wrote: > > Can get rid of handle param > > Done. > > http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... > chrome/browser/intents/web_intents_registry.cc:271: WebDataService::Handle h, > On 2012/07/27 00:41:29, Greg Billock wrote: > > We don't need the handle anymore, right? > > Done. > > http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... > chrome/browser/intents/web_intents_registry.cc:474: const Extension* > WebIntentsRegistry::ExtensionForURL(const std::string& url) { > On 2012/07/27 00:41:29, Greg Billock wrote: > > Yeah, update the header. > > > > On 2012/07/27 00:11:26, groby wrote: > > > Technically, functions should be in order of declaration in the header. If > you > > > move them, you might as well put them in the right order (or update the > > header) > > > > Done. > > http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... > File chrome/browser/intents/web_intents_registry.h (right): > > http://codereview.chromium.org/10827056/diff/1/chrome/browser/intents/web_int... > chrome/browser/intents/web_intents_registry.h:140: QueryVector pending_queries_; > On 2012/07/27 00:11:26, groby wrote: > > Why no map? > > The map only existed to associate wds handles with intent query objects. This > was needed to look up the query when processing a wds query response. Since the > IntentsQuery is now the consumer of the wds response we no longer need to do > this. The remaining purpose of the map was as an enumeration of active queries > needed for cleanup on object delete. This only requires some type of a simple > collection like a vector, so it makes sense to reduce the type to a vector. lgtm
I know... more nits. http://codereview.chromium.org/10827056/diff/8002/chrome/browser/intents/web_... File chrome/browser/intents/web_intents_registry.cc (right): http://codereview.chromium.org/10827056/diff/8002/chrome/browser/intents/web_... chrome/browser/intents/web_intents_registry.cc:494: QueryVector::iterator it = std::find( standard C++ idiom for this: std::erase(std::remove(pending_queries.begin(), pending_queries.end(), query), pending_queries_.end() ) Since you want to ensure we only claim existing queries, you might want DCHECK(std::count(pending_queries_.begin(), pending_queries_.end(), query); And if that's too much STL tomfoolery, at least move the DCHECK into Claim - it's part of the contract. http://codereview.chromium.org/10827056/diff/13003/chrome/browser/intents/web... File chrome/browser/intents/web_intents_registry.cc (right): http://codereview.chromium.org/10827056/diff/13003/chrome/browser/intents/web... chrome/browser/intents/web_intents_registry.cc:493: DCHECK(query); Also, no need to DCHECK. This works fine with NULL queries.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/smckay@chromium.org/10827056/13003
Dones. Submitting. http://codereview.chromium.org/10827056/diff/13003/chrome/browser/intents/web... File chrome/browser/intents/web_intents_registry.cc (right): http://codereview.chromium.org/10827056/diff/13003/chrome/browser/intents/web... chrome/browser/intents/web_intents_registry.cc:493: DCHECK(query); On 2012/07/27 01:17:04, groby wrote: > Also, no need to DCHECK. This works fine with NULL queries. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/smckay@chromium.org/10827056/9004
Change committed as 148714 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
