Mark down in web intents picker controller when it is part of a TabContents that is hosting a web i…

Mark down in web intents picker controller when it is part of a TabContents that is hosting a web intents service.


R=groby@chromium.org
BUG=None
TEST=WebIntentsPickerControllerBrowserTest.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149039

[groby-ooo-7-16, 2012-07-26 17:44:08]

http://codereview.chromium.org/10805094/diff/5001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/10805094/diff/5001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:156: private:
Why not make the controller a WebContentsObserver?

http://codereview.chromium.org/10805094/diff/5001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:731:
window_disposition_source_ = NULL;
I wonder if me were served equally well if WebContents could be WeakPtr's...

(I don't think they can be right now)

http://codereview.chromium.org/10805094/diff/5001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

http://codereview.chromium.org/10805094/diff/5001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:116: void
SourceDispatcherReplied(webkit_glue::WebIntentReplyType reply_type);
It seems odd that the controller needs to know about the state of the dispatcher
- as if we've got some dependency wires crossed.

http://codereview.chromium.org/10805094/diff/5001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:210: // with which this
controller is associated is hosting a web intents window
To clarify - did you mean "hosting a web intent service with disposition
'window'" ? (That's how I read it, but as is this might refer to a service in
Chrome instead)

http://codereview.chromium.org/10805094/diff/5001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc
(right):

http://codereview.chromium.org/10805094/diff/5001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:360:
EXPECT_FALSE(NULL == GetWindowDispositionTarget(
EXPECT_TRUE(GetWindowDisposition.... ?

[Greg Billock, 2012-07-26 18:03:30]

http://codereview.chromium.org/10805094/diff/5001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/10805094/diff/5001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:156: private:
On 2012/07/26 17:44:08, groby wrote:
> Why not make the controller a WebContentsObserver?

I thought about that option, but I think it's confusing -- it would be very
inviting to assume from that that it is observing the WebContents of the
TabContents to which it is attached as a member.

http://codereview.chromium.org/10805094/diff/5001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:731:
window_disposition_source_ = NULL;
I had the same thought this morning, but there's no WeakPtr there or in
TabContents.

On 2012/07/26 17:44:08, groby wrote:
> I wonder if me were served equally well if WebContents could be WeakPtr's...
> 
> (I don't think they can be right now)

http://codereview.chromium.org/10805094/diff/5001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.h (right):

http://codereview.chromium.org/10805094/diff/5001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:116: void
SourceDispatcherReplied(webkit_glue::WebIntentReplyType reply_type);
On 2012/07/26 17:44:08, groby wrote:
> It seems odd that the controller needs to know about the state of the
dispatcher
> - as if we've got some dependency wires crossed. 

The main reason for this is that the intent may have been caused by something
not anchored to any WebContents, and so we need to get the dispatcher and
there's no other way to get it than to have a pointer.

http://codereview.chromium.org/10805094/diff/5001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:210: // with which this
controller is associated is hosting a web intents window
Yes, that's the right reading. Any better wording?

On 2012/07/26 17:44:08, groby wrote:
> To clarify - did you mean "hosting a web intent service with disposition
> 'window'" ? (That's how I read it, but as is this might refer to a service in
> Chrome instead)

http://codereview.chromium.org/10805094/diff/5001/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc
(right):

http://codereview.chromium.org/10805094/diff/5001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:360:
EXPECT_FALSE(NULL == GetWindowDispositionTarget(
On 2012/07/26 17:44:08, groby wrote:
> EXPECT_TRUE(GetWindowDisposition.... ?

Done.

[groby-ooo-7-16, 2012-07-30 20:10:05]

lgtm with nit

http://codereview.chromium.org/10805094/diff/6002/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/10805094/diff/6002/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:740: return
window_disposition_source_ != NULL ||
nit: without != NULL

http://codereview.chromium.org/10805094/diff/6002/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc
(right):

http://codereview.chromium.org/10805094/diff/6002/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:360:
EXPECT_TRUE(NULL != GetWindowDispositionTarget(
nit: Without the NULL!=?

[Greg Billock, 2012-07-30 20:23:57]

http://codereview.chromium.org/10805094/diff/6002/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller.cc (right):

http://codereview.chromium.org/10805094/diff/6002/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.cc:740: return
window_disposition_source_ != NULL ||
On 2012/07/30 20:10:05, groby wrote:
> nit: without != NULL

Done.

http://codereview.chromium.org/10805094/diff/6002/chrome/browser/ui/intents/w...
File chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc
(right):

http://codereview.chromium.org/10805094/diff/6002/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc:360:
EXPECT_TRUE(NULL != GetWindowDispositionTarget(
On 2012/07/30 20:10:05, groby wrote:
> nit: Without the NULL!=?

Done.

[commit-bot: I haz the power, 2012-07-30 20:24:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gbillock@chromium.org/10805094/12001

[commit-bot: I haz the power, 2012-07-30 21:55:41]

Change committed as 149039