Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10204010:
Handling default service in the web intents picker controller. (Closed)
|
Created:
8 years, 8 months ago by Greg Billock Modified:
8 years, 7 months ago Reviewers:
groby-ooo-7-16 CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionHandling default service in the web intents picker controller.
BUG=115214
TEST=None
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=135253
Patch Set 1 #
Total comments: 28
Patch Set 2 : Delegate to model more. #Patch Set 3 : Rebasing #Patch Set 4 : Rebase to head #
Total comments: 14
Patch Set 5 : Switch to GURL #Patch Set 6 : Merge to head #
Messages
Total messages: 12 (0 generated)
http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... File chrome/browser/ui/intents/web_intent_picker_controller.cc (right): http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:210: pending_async_count_ += 2; Why exactly do we keep a pending count? (Nobody implements OnPendingAsyncCompleted except for a stub, so we can probably kill this machinery) http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:211: pending_registry_calls_count_ += 1; Why add 1 here? We already do that when we call the registry? (Also, why at all? We will always call the Callback as soon as we complete, and the count will always be 1) http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:255: // TODO(gbillock): use browser_? I believe there has been a CL (or there is one in flight) that does do that already - the switch to WebContents? http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:311: // TODO(gbillock): use browser_? See above http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:380: // TODO(gbillock): use browser_? See above http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:422: RegistryCallsCompleted(); Why do we flag registry calls completed here, not when we get the default service? http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:436: CreatePicker(); This is more of a TBD, but I really think we need to (at some point) separate the idea of the picker and the inline disposition contents into two separate objects. http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... File chrome/browser/ui/intents/web_intent_picker_controller.h (right): http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.h:115: void RegistryCallsCompleted(); That's a rather vague description :) http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.h:161: void CreatePicker(); "Helper to create picker dialog UI" is probably enough of a description. http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.h:194: std::string default_service_url_; Doesn't that belong on the model? http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.h:206: Browser* browser_; That's available from the TabContentsWrapper, IIRC. http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.h:209: string16 action_; Do we have to store that here? Can't we query the picker? http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... File chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc (right): http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:102: void reset() { Why separate this out? Nobody calls reset? http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:481: controller_->SetIntentsDispatcher(&dispatcher); Why do we set up a dispatcher when we install a target service?
http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... File chrome/browser/ui/intents/web_intent_picker_controller.cc (right): http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:210: pending_async_count_ += 2; On 2012/04/24 21:36:51, groby wrote: > Why exactly do we keep a pending count? (Nobody implements > OnPendingAsyncCompleted except for a stub, so we can probably kill this > machinery) I thought it was used to guide the operation of the picker dialog. If not, yes, we can scrap it. Let's do that separately though. http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:211: pending_registry_calls_count_ += 1; There's now two registry callbacks we need to wait for. The right solution is a barrier closure, but we don't have one. :-/ On 2012/04/24 21:36:51, groby wrote: > Why add 1 here? We already do that when we call the registry? (Also, why at all? > We will always call the Callback as soon as we complete, and the count will > always be 1) http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:255: // TODO(gbillock): use browser_? got rid of this. I'll add a todo to scrap the ShowDialog arg, as we don't need it any more. On 2012/04/24 21:36:51, groby wrote: > I believe there has been a CL (or there is one in flight) that does do that > already - the switch to WebContents? http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:311: // TODO(gbillock): use browser_? On 2012/04/24 21:36:51, groby wrote: > See above Done. http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:380: // TODO(gbillock): use browser_? On 2012/04/24 21:36:51, groby wrote: > See above Done. http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:422: RegistryCallsCompleted(); Bad name. This is the method we want to happen when both are complete. On 2012/04/24 21:36:51, groby wrote: > Why do we flag registry calls completed here, not when we get the default > service? http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.cc:436: CreatePicker(); On 2012/04/24 21:36:51, groby wrote: > This is more of a TBD, but I really think we need to (at some point) separate > the idea of the picker and the inline disposition contents into two separate > objects. Makes sense. http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... File chrome/browser/ui/intents/web_intent_picker_controller.h (right): http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.h:115: void RegistryCallsCompleted(); improved. On 2012/04/24 21:36:51, groby wrote: > That's a rather vague description :) http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.h:161: void CreatePicker(); On 2012/04/24 21:36:51, groby wrote: > "Helper to create picker dialog UI" is probably enough of a description. Done. http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.h:194: std::string default_service_url_; On 2012/04/24 21:36:51, groby wrote: > Doesn't that belong on the model? Yeah, you're right. It has "action" too. Rejiggering... http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.h:206: Browser* browser_; On 2012/04/24 21:36:51, groby wrote: > That's available from the TabContentsWrapper, IIRC. Yes, we're currently using static methods to look it up. Should we do that, or just write it down from ShowDialog? (Or get rid of the arg to ShowDialog if we don't need it...) http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller.h:209: string16 action_; On 2012/04/24 21:36:51, groby wrote: > Do we have to store that here? Can't we query the picker? This is stored from ShowDialog, where now we don't show the picker yet in that method. http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... File chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc (right): http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:102: void reset() { On 2012/04/24 21:36:51, groby wrote: > Why separate this out? Nobody calls reset? I was using this but found a better way and forgot to put it back. http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:481: controller_->SetIntentsDispatcher(&dispatcher); On 2012/04/24 21:36:51, groby wrote: > Why do we set up a dispatcher when we install a target service? Needed for smooth operation of the controller. I looked into going deeper and installing it directly, but this just seemed a lot easier.
On 2012/04/25 16:04:32, Greg Billock wrote: > http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... > File chrome/browser/ui/intents/web_intent_picker_controller.cc (right): > > http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... > chrome/browser/ui/intents/web_intent_picker_controller.cc:210: > pending_async_count_ += 2; > On 2012/04/24 21:36:51, groby wrote: > > Why exactly do we keep a pending count? (Nobody implements > > OnPendingAsyncCompleted except for a stub, so we can probably kill this > > machinery) > > I thought it was used to guide the operation of the picker dialog. If not, yes, > we can scrap it. Let's do that separately though. > > http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... > chrome/browser/ui/intents/web_intent_picker_controller.cc:211: > pending_registry_calls_count_ += 1; > There's now two registry callbacks we need to wait for. > The right solution is a barrier closure, but we don't have one. :-/ > > On 2012/04/24 21:36:51, groby wrote: > > Why add 1 here? We already do that when we call the registry? (Also, why at > all? > > We will always call the Callback as soon as we complete, and the count will > > always be 1) > > http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... > chrome/browser/ui/intents/web_intent_picker_controller.cc:255: // > TODO(gbillock): use browser_? > got rid of this. I'll add a todo to scrap the ShowDialog arg, as we don't need > it any more. > > On 2012/04/24 21:36:51, groby wrote: > > I believe there has been a CL (or there is one in flight) that does do that > > already - the switch to WebContents? > > http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... > chrome/browser/ui/intents/web_intent_picker_controller.cc:311: // > TODO(gbillock): use browser_? > On 2012/04/24 21:36:51, groby wrote: > > See above > > Done. > > http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... > chrome/browser/ui/intents/web_intent_picker_controller.cc:380: // > TODO(gbillock): use browser_? > On 2012/04/24 21:36:51, groby wrote: > > See above > > Done. > > http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... > chrome/browser/ui/intents/web_intent_picker_controller.cc:422: > RegistryCallsCompleted(); > Bad name. This is the method we want to happen when both are complete. > > On 2012/04/24 21:36:51, groby wrote: > > Why do we flag registry calls completed here, not when we get the default > > service? > > http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... > chrome/browser/ui/intents/web_intent_picker_controller.cc:436: CreatePicker(); > On 2012/04/24 21:36:51, groby wrote: > > This is more of a TBD, but I really think we need to (at some point) separate > > the idea of the picker and the inline disposition contents into two separate > > objects. > > Makes sense. > > http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... > File chrome/browser/ui/intents/web_intent_picker_controller.h (right): > > http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... > chrome/browser/ui/intents/web_intent_picker_controller.h:115: void > RegistryCallsCompleted(); > improved. > > On 2012/04/24 21:36:51, groby wrote: > > That's a rather vague description :) > > http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... > chrome/browser/ui/intents/web_intent_picker_controller.h:161: void > CreatePicker(); > On 2012/04/24 21:36:51, groby wrote: > > "Helper to create picker dialog UI" is probably enough of a description. > > Done. > > http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... > chrome/browser/ui/intents/web_intent_picker_controller.h:194: std::string > default_service_url_; > On 2012/04/24 21:36:51, groby wrote: > > Doesn't that belong on the model? > > Yeah, you're right. It has "action" too. Rejiggering... > > http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... > chrome/browser/ui/intents/web_intent_picker_controller.h:206: Browser* browser_; > On 2012/04/24 21:36:51, groby wrote: > > That's available from the TabContentsWrapper, IIRC. > > Yes, we're currently using static methods to look it up. Should we do that, or > just write it down from ShowDialog? (Or get rid of the arg to ShowDialog if we > don't need it...) > > http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... > chrome/browser/ui/intents/web_intent_picker_controller.h:209: string16 action_; > On 2012/04/24 21:36:51, groby wrote: > > Do we have to store that here? Can't we query the picker? > > This is stored from ShowDialog, where now we don't show the picker yet in that > method. > > http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... > File chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc > (right): > > http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... > chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:102: void > reset() { > On 2012/04/24 21:36:51, groby wrote: > > Why separate this out? Nobody calls reset? > > I was using this but found a better way and forgot to put it back. > > http://codereview.chromium.org/10204010/diff/1/chrome/browser/ui/intents/web_... > chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:481: > controller_->SetIntentsDispatcher(&dispatcher); > On 2012/04/24 21:36:51, groby wrote: > > Why do we set up a dispatcher when we install a target service? > > Needed for smooth operation of the controller. I looked into going deeper and > installing it directly, but this just seemed a lot easier. OK, this is merged with the expclit intents change now. Ready for another look.
https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... File chrome/browser/ui/intents/web_intent_picker_controller.cc (right): https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... chrome/browser/ui/intents/web_intent_picker_controller.cc:479: const WebIntentPickerModel::InstalledService* default_service = I'm wondering if we should just keep a InstalledService* on the model and set that in OnWebIntentDefaultsAvailable - this simplifies logic here to if(!default_svc || default_svc == INLINE) { CreatePicker(); SetActionString(); // might need that for inline disposition, too } if(default_svc) OnServiceChosen() - or at least similar. https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... chrome/browser/ui/intents/web_intent_picker_controller.cc:498: // the make-default checkbox to "true" upon display. Probably belongs on the model https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... File chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc (left): https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:16: #include "chrome/browser/intents/web_intents_registry_factory.h" Nice catch :) https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... File chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc (right): https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:120: LOG(INFO) << "OnPendingAsyncCompleted()"; Do we really need this? As a LOG, no less? At least DLOG or DVLOG, maybe? https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:585: intent.type = ASCIIToUTF16("b"); I'm confused - why would that find any matches? Shouldn't this be kType1? https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:595: EXPECT_EQ(0, picker_.num_icons_changed_); How is this significant to defaults? Do they never pull icons? https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... File chrome/browser/ui/intents/web_intent_picker_model.h (right): https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... chrome/browser/ui/intents/web_intent_picker_model.h:87: default_service_url_ = default_url; See previous comment on keeping an InstalledService* instead. https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... chrome/browser/ui/intents/web_intent_picker_model.h:175: std::string default_service_url_; If it's an URL, shouldn't it be a GURL, not std::string?
https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... File chrome/browser/ui/intents/web_intent_picker_controller.cc (right): https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... chrome/browser/ui/intents/web_intent_picker_controller.cc:479: const WebIntentPickerModel::InstalledService* default_service = That makes sense. Let's refactor that separately? On 2012/05/01 15:28:33, groby wrote: > I'm wondering if we should just keep a InstalledService* on the model and set > that in OnWebIntentDefaultsAvailable - this simplifies logic here to > > if(!default_svc || default_svc == INLINE) { > CreatePicker(); > SetActionString(); // might need that for inline disposition, too > } > > if(default_svc) > OnServiceChosen() > > - or at least similar. https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... chrome/browser/ui/intents/web_intent_picker_controller.cc:498: // the make-default checkbox to "true" upon display. On 2012/05/01 15:28:33, groby wrote: > Probably belongs on the model Done. https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... File chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc (right): https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:120: LOG(INFO) << "OnPendingAsyncCompleted()"; removed On 2012/05/01 15:28:33, groby wrote: > Do we really need this? As a LOG, no less? At least DLOG or DVLOG, maybe? https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:585: intent.type = ASCIIToUTF16("b"); On 2012/05/01 15:28:33, groby wrote: > I'm confused - why would that find any matches? Shouldn't this be kType1? Yeah, this is confused. These action/types aren't actually used (it's the ShowDialog args), but that's confusing and will undoubtedly be fixed. https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:595: EXPECT_EQ(0, picker_.num_icons_changed_); On 2012/05/01 15:28:33, groby wrote: > How is this significant to defaults? Do they never pull icons? Basically the picker isn't activated, so they don't. It isn't really essential, though. I can remove it -- the big thing is there's no OnServiceChosen call here. https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... File chrome/browser/ui/intents/web_intent_picker_model.h (right): https://chromiumcodereview.appspot.com/10204010/diff/9007/chrome/browser/ui/i... chrome/browser/ui/intents/web_intent_picker_model.h:175: std::string default_service_url_; On 2012/05/01 15:28:33, groby wrote: > If it's an URL, shouldn't it be a GURL, not std::string? Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/10204010/16007
Can't apply patch for file chrome/browser/ui/intents/web_intent_picker_controller.cc. While running patch -p1 --forward --force; patching file chrome/browser/ui/intents/web_intent_picker_controller.cc Hunk #2 FAILED at 163. Hunk #3 succeeded at 218 (offset -1 lines). Hunk #4 succeeded at 427 (offset -1 lines). Hunk #5 succeeded at 456 (offset -1 lines). 1 out of 5 hunks FAILED -- saving rejects to file chrome/browser/ui/intents/web_intent_picker_controller.cc.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/10204010/20007
Try job failure for 10204010-20007 on win_rel for step "update". http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu... Step "update" is always a major failure. Look at the try server FAQ for more details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/10204010/20007
Change committed as 135253 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
