Add a public content/ interface for intents. Use it.

Add a public content/ interface for intents. Use it.

R=jam@chromium.org
BUG=104980
TEST=None.


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

[jam, 2011-11-23 21:00:00]

looks great, some nits:

http://codereview.chromium.org/8666013/diff/1/content/browser/intents/intents...
File content/browser/intents/intents_host_impl.h (right):

http://codereview.chromium.org/8666013/diff/1/content/browser/intents/intents...
content/browser/intents/intents_host_impl.h:14: class TabContents;
nit: not needed since this is a TabContentsObserver

http://codereview.chromium.org/8666013/diff/1/content/browser/intents/intents...
content/browser/intents/intents_host_impl.h:36: TabContents* source_tab_;
nit: not needed, TabContentsObserver has a tab_contents() getter

http://codereview.chromium.org/8666013/diff/1/content/public/browser/intents_...
File content/public/browser/intents_host.h (right):

http://codereview.chromium.org/8666013/diff/1/content/public/browser/intents_...
content/public/browser/intents_host.h:29: virtual const
webkit_glue::WebIntentData& intent() = 0;
nit: chrome style is to use CamelCaps for virtual functions, so this should be
GetIntent()

[Greg Billock, 2011-11-24 00:42:12]

http://codereview.chromium.org/8666013/diff/1/content/browser/intents/intents...
File content/browser/intents/intents_host_impl.h (right):

http://codereview.chromium.org/8666013/diff/1/content/browser/intents/intents...
content/browser/intents/intents_host_impl.h:14: class TabContents;
On 2011/11/23 21:00:00, John Abd-El-Malek wrote:
> nit: not needed since this is a TabContentsObserver

Done.

http://codereview.chromium.org/8666013/diff/1/content/browser/intents/intents...
content/browser/intents/intents_host_impl.h:36: TabContents* source_tab_;
On 2011/11/23 21:00:00, John Abd-El-Malek wrote:
> nit: not needed, TabContentsObserver has a tab_contents() getter

Yes. I'm using value=null to also flag a "should I talk to this any more"
semantics, too. Do you think I should just use a boolean for that, instead? I
had it that way, but ended up changing (back, kinda, since the previous code
used it) to this.

http://codereview.chromium.org/8666013/diff/1/content/public/browser/intents_...
File content/public/browser/intents_host.h (right):

http://codereview.chromium.org/8666013/diff/1/content/public/browser/intents_...
content/public/browser/intents_host.h:29: virtual const
webkit_glue::WebIntentData& intent() = 0;
On 2011/11/23 21:00:00, John Abd-El-Malek wrote:
> nit: chrome style is to use CamelCaps for virtual functions, so this should be
> GetIntent()

Done.

[jam, 2011-11-24 21:27:49]

lgtm with removal of source_tab_

http://codereview.chromium.org/8666013/diff/1/content/browser/intents/intents...
File content/browser/intents/intents_host_impl.h (right):

http://codereview.chromium.org/8666013/diff/1/content/browser/intents/intents...
content/browser/intents/intents_host_impl.h:36: TabContents* source_tab_;
On 2011/11/24 00:42:13, Greg Billock wrote:
> On 2011/11/23 21:00:00, John Abd-El-Malek wrote:
> > nit: not needed, TabContentsObserver has a tab_contents() getter
> 
> Yes. I'm using value=null to also flag a "should I talk to this any more"
> semantics, too. Do you think I should just use a boolean for that, instead? I
> had it that way, but ended up changing (back, kinda, since the previous code
> used it) to this.

tab_contents() will return NULL after TabContentsDestroyed is called. check out
TabContentsObserver::TabContentsDestroyed()

[binji, 2011-11-28 22:50:14]

http://codereview.chromium.org/8666013/diff/6001/content/browser/intents/inte...
File content/browser/intents/intents_host_impl.cc (right):

http://codereview.chromium.org/8666013/diff/6001/content/browser/intents/inte...
content/browser/intents/intents_host_impl.cc:54:
intent_injector_->SourceTabContentsDestroyed(tab);
You might want to mention here that the intent_injector is destroyed by this
function.

http://codereview.chromium.org/8666013/diff/6001/content/public/browser/inten...
File content/public/browser/intents_host.h (right):

http://codereview.chromium.org/8666013/diff/6001/content/public/browser/inten...
content/public/browser/intents_host.h:32: virtual void
DispatchIntent(TabContents* tab_contents) = 0;
forward declare TabContents?

http://codereview.chromium.org/8666013/diff/6001/content/public/browser/inten...
content/public/browser/intents_host.h:37: const string16& data) = 0;
remove two spaces before const

[Greg Billock, 2011-11-29 00:19:55]

http://codereview.chromium.org/8666013/diff/1/content/browser/intents/intents...
File content/browser/intents/intents_host_impl.h (right):

http://codereview.chromium.org/8666013/diff/1/content/browser/intents/intents...
content/browser/intents/intents_host_impl.h:36: TabContents* source_tab_;
Good point. It's documented in the API and everything. Deleted.

On 2011/11/24 21:27:50, John Abd-El-Malek wrote:
> On 2011/11/24 00:42:13, Greg Billock wrote:
> > On 2011/11/23 21:00:00, John Abd-El-Malek wrote:
> > > nit: not needed, TabContentsObserver has a tab_contents() getter
> > 
> > Yes. I'm using value=null to also flag a "should I talk to this any more"
> > semantics, too. Do you think I should just use a boolean for that, instead?
I
> > had it that way, but ended up changing (back, kinda, since the previous code
> > used it) to this.
> 
> tab_contents() will return NULL after TabContentsDestroyed is called. check
out
> TabContentsObserver::TabContentsDestroyed()

http://codereview.chromium.org/8666013/diff/6001/content/browser/intents/inte...
File content/browser/intents/intents_host_impl.cc (right):

http://codereview.chromium.org/8666013/diff/6001/content/browser/intents/inte...
content/browser/intents/intents_host_impl.cc:54:
intent_injector_->SourceTabContentsDestroyed(tab);
Good catch. I'd forgotten to update the IntentInjector with this new ownership
semantics, which are a lot cleaner.

On 2011/11/28 22:50:14, binji wrote:
> You might want to mention here that the intent_injector is destroyed by this
> function.

http://codereview.chromium.org/8666013/diff/6001/content/public/browser/inten...
File content/public/browser/intents_host.h (right):

http://codereview.chromium.org/8666013/diff/6001/content/public/browser/inten...
content/public/browser/intents_host.h:32: virtual void
DispatchIntent(TabContents* tab_contents) = 0;
On 2011/11/28 22:50:14, binji wrote:
> forward declare TabContents?

Done.

http://codereview.chromium.org/8666013/diff/6001/content/public/browser/inten...
content/public/browser/intents_host.h:37: const string16& data) = 0;
On 2011/11/28 22:50:14, binji wrote:
> remove two spaces before const

Done.

[James Hawkins, 2011-11-29 02:57:54]

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

http://codereview.chromium.org/8666013/diff/18001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:46: // this picker was
created. The picker takes ownership of this object.
s/this object/intents_host/

Probably document it "must not be NULL".

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
File content/browser/intents/intents_host_impl.cc (right):

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
content/browser/intents/intents_host_impl.cc:27: DCHECK(intent_injector_ ==
NULL);
DCHECK(!intent_injector);

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
File content/browser/intents/intents_host_impl.h (right):

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
content/browser/intents/intents_host_impl.h:15: class IntentsHostImpl : public
content::IntentsHost,
Document the class.

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
content/browser/intents/intents_host_impl.h:18: IntentsHostImpl(TabContents*
source_tab,
Document the params.

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
content/browser/intents/intents_host_impl.h:23: // Implement IntentsHost
// IntentsHost implementation.

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
content/browser/intents/intents_host_impl.h:28: virtual void
RegisterReplyNotification(const base::Closure& closure) OVERRIDE;
#include "base/compiler_specific.h"

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
content/browser/intents/intents_host_impl.h:34: // The intent data.
Redundant comment; either spruce up or remove.

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
content/browser/intents/intents_host_impl.h:37: // The intent ID.
Same as above.

http://codereview.chromium.org/8666013/diff/18001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents_delegate.cc (right):

http://codereview.chromium.org/8666013/diff/18001/content/browser/tab_content...
content/browser/tab_contents/tab_contents_delegate.cc:318: delete intents_host;
This is a very strange method indeed.  Can you add documentation about the
implementation?

http://codereview.chromium.org/8666013/diff/18001/content/public/browser/inte...
File content/public/browser/intents_host.h (right):

http://codereview.chromium.org/8666013/diff/18001/content/public/browser/inte...
content/public/browser/intents_host.h:28: virtual ~IntentsHost() {}
Move to protected.

[Greg Billock, 2011-11-29 19:18:15]

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

http://codereview.chromium.org/8666013/diff/18001/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:46: // this picker was
created. The picker takes ownership of this object.
On 2011/11/29 02:57:55, James Hawkins wrote:
> s/this object/intents_host/
> 
> Probably document it "must not be NULL".

Done.

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
File content/browser/intents/intents_host_impl.cc (right):

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
content/browser/intents/intents_host_impl.cc:27: DCHECK(intent_injector_ ==
NULL);
On 2011/11/29 02:57:55, James Hawkins wrote:
> DCHECK(!intent_injector);

Done.

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
File content/browser/intents/intents_host_impl.h (right):

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
content/browser/intents/intents_host_impl.h:15: class IntentsHostImpl : public
content::IntentsHost,
On 2011/11/29 02:57:55, James Hawkins wrote:
> Document the class.

Done.

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
content/browser/intents/intents_host_impl.h:18: IntentsHostImpl(TabContents*
source_tab,
On 2011/11/29 02:57:55, James Hawkins wrote:
> Document the params.

Done.

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
content/browser/intents/intents_host_impl.h:23: // Implement IntentsHost
On 2011/11/29 02:57:55, James Hawkins wrote:
> // IntentsHost implementation.

Done.

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
content/browser/intents/intents_host_impl.h:28: virtual void
RegisterReplyNotification(const base::Closure& closure) OVERRIDE;
On 2011/11/29 02:57:55, James Hawkins wrote:
> #include "base/compiler_specific.h"

Done.

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
content/browser/intents/intents_host_impl.h:34: // The intent data.
On 2011/11/29 02:57:55, James Hawkins wrote:
> Redundant comment; either spruce up or remove.

Done.

http://codereview.chromium.org/8666013/diff/18001/content/browser/intents/int...
content/browser/intents/intents_host_impl.h:37: // The intent ID.
On 2011/11/29 02:57:55, James Hawkins wrote:
> Same as above.

Done.

http://codereview.chromium.org/8666013/diff/18001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents_delegate.cc (right):

http://codereview.chromium.org/8666013/diff/18001/content/browser/tab_content...
content/browser/tab_contents/tab_contents_delegate.cc:318: delete intents_host;
On 2011/11/29 02:57:55, James Hawkins wrote:
> This is a very strange method indeed.  Can you add documentation about the
> implementation?

Done.

http://codereview.chromium.org/8666013/diff/18001/content/public/browser/inte...
File content/public/browser/intents_host.h (right):

http://codereview.chromium.org/8666013/diff/18001/content/public/browser/inte...
content/public/browser/intents_host.h:28: virtual ~IntentsHost() {}
On 2011/11/29 02:57:55, James Hawkins wrote:
> Move to protected.

This object is created by the content API, but ownership is passed to client
code.

[James Hawkins, 2011-11-29 19:20:04]

LGTM with nit.

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

http://codereview.chromium.org/8666013/diff/22003/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:47: // Must not be
NULL.
To be clear:

"|intents_host| must not be NULL."

otherwise it's ambiguous.

[Greg Billock, 2011-11-29 23:41:40]

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

http://codereview.chromium.org/8666013/diff/22003/chrome/browser/ui/intents/w...
chrome/browser/ui/intents/web_intent_picker_controller.h:47: // Must not be
NULL.
On 2011/11/29 19:20:04, James Hawkins wrote:
> To be clear:
> 
> "|intents_host| must not be NULL."
> 
> otherwise it's ambiguous.

Done.

[groby-ooo-7-16, 2011-12-01 00:58:14]

lgtm

[commit-bot: I haz the power, 2011-12-01 20:34:32]

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

[commit-bot: I haz the power, 2011-12-01 21:54:55]

Change committed as 112545