Create CoreTabHelper, move remaining core TCW functionality into it.

Create a bunch of TabHelpers and TabObservers, move all TCW functionality into them.

BUG=105872
TEST=no change

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

[Avi (use Gerrit), 2011-12-07 23:01:15]

Trybot runs for this patch:

http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/6672
http://build.chromium.org/p/tryserver.chromium/builders/linux_view/builds/1067
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromeos/builds...
http://build.chromium.org/p/tryserver.chromium/builders/win/builds/7124
http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/6781

[Jói, 2011-12-08 11:23:37]

http://codereview.chromium.org/8865004/diff/1/chrome/browser/ui/tab_contents/...
File chrome/browser/ui/tab_contents/core_tab_helper.h (right):

http://codereview.chromium.org/8865004/diff/1/chrome/browser/ui/tab_contents/...
chrome/browser/ui/tab_contents/core_tab_helper.h:19: class CoreTabHelper :
public TabContentsObserver,
Both the name of this class (...Helper) and its comment bug me a bit; the
responsibility of the class is quite unclear, and the current set of
functionality is quite a mixed bag.  Since we're refactoring existing code I
guess it can't be perfect, but is there any way to better express the nature of
this class?  Should it perhaps be more than one class, each with a clearly
defined responsibility?

http://codereview.chromium.org/8865004/diff/1/chrome/browser/ui/tab_contents/...
chrome/browser/ui/tab_contents/core_tab_helper.h:26: void
set_delegate(CoreTabHelperDelegate* d) { delegate_ = d; }
A comment that caller must ensure lifetime of delegate exceeds lifetime of
CoreTabHelper would be nice.

[Avi (use Gerrit), 2011-12-08 15:27:38]

http://codereview.chromium.org/8865004/diff/1/chrome/browser/ui/tab_contents/...
File chrome/browser/ui/tab_contents/core_tab_helper.h (right):

http://codereview.chromium.org/8865004/diff/1/chrome/browser/ui/tab_contents/...
chrome/browser/ui/tab_contents/core_tab_helper.h:19: class CoreTabHelper :
public TabContentsObserver,
I know.

Right now, TCW has on it random bits of functionality that haven't fit anywhere
else, one-off functionality that made it a dumping ground. I can pull things
like error page url, snapshots, etc off into their own classes, but that would
mean a class just to listen to one message. (Although that's certainly cleaner
and would definitely be making a point.)

I think "core" things like titles and status text belong together on a 'core'
TH.

BTW, anything that hangs off the TCW and provides per-tab service is called a
TabHelper. We ran out of terminology a long time ago.

http://codereview.chromium.org/8865004/diff/1/chrome/browser/ui/tab_contents/...
chrome/browser/ui/tab_contents/core_tab_helper.h:26: void
set_delegate(CoreTabHelperDelegate* d) { delegate_ = d; }
On 2011/12/08 11:23:37, Jói wrote:
> A comment that caller must ensure lifetime of delegate exceeds lifetime of
> CoreTabHelper would be nice.

Is that comment found elsewhere with delegates? (And it's actually not true in
this case...)

[Jói, 2011-12-08 15:31:51]

LGTM

http://codereview.chromium.org/8865004/diff/1/chrome/browser/ui/tab_contents/...
File chrome/browser/ui/tab_contents/core_tab_helper.h (right):

http://codereview.chromium.org/8865004/diff/1/chrome/browser/ui/tab_contents/...
chrome/browser/ui/tab_contents/core_tab_helper.h:19: class CoreTabHelper :
public TabContentsObserver,
On 2011/12/08 15:27:38, Avi wrote:
> I know.
> 
> Right now, TCW has on it random bits of functionality that haven't fit
anywhere
> else, one-off functionality that made it a dumping ground. I can pull things
> like error page url, snapshots, etc off into their own classes, but that would
> mean a class just to listen to one message. (Although that's certainly cleaner
> and would definitely be making a point.)
> 
> I think "core" things like titles and status text belong together on a 'core'
> TH.
> 
> BTW, anything that hangs off the TCW and provides per-tab service is called a
> TabHelper. We ran out of terminology a long time ago.

I'm fine with doing it this way for now, and maybe splitting into separate
classes down the line as clarity emerges.  How about that?

http://codereview.chromium.org/8865004/diff/1/chrome/browser/ui/tab_contents/...
chrome/browser/ui/tab_contents/core_tab_helper.h:26: void
set_delegate(CoreTabHelperDelegate* d) { delegate_ = d; }
On 2011/12/08 15:27:38, Avi wrote:
> On 2011/12/08 11:23:37, Jói wrote:
> > A comment that caller must ensure lifetime of delegate exceeds lifetime of
> > CoreTabHelper would be nice.
> 
> Is that comment found elsewhere with delegates? (And it's actually not true in
> this case...)

I've seen (and myself added) similar comments about lifetime responsibilities. 
The fact that I misunderstood the lifetime contract indicates there is probably
a need for a comment :)  I guess a more correct description is that whoever
calls set_delegate must ensure the lifetime of the delegate until the
CoreTabHelper is deleted, or set_delegate is set with a different value.

[Avi (use Gerrit), 2011-12-08 15:58:12]

Proposed:

CoreTabHelper: GetDefaultTitle and GetStatusText
SnapshotTabHelper: CaptureSnapshot and the snapshot message handling
FullscreenTabHelper: ExitFullscreenMode (where is EnterFullscreenMode?)
PDFTabObserver: Handle PDF messages
AlternateErrorPageTabObserver: Handle all that alternate error page crap

Yes? I'm going to do that.

[Jói, 2011-12-08 16:00:55]

Avi, that sounds fantastic.

Cheers,
Jói


On Thu, Dec 8, 2011 at 3:58 PM,  <avi@chromium.org> wrote:
> Proposed:
>
> CoreTabHelper: GetDefaultTitle and GetStatusText
> SnapshotTabHelper: CaptureSnapshot and the snapshot message handling
> FullscreenTabHelper: ExitFullscreenMode (where is EnterFullscreenMode?)
> PDFTabObserver: Handle PDF messages
> AlternateErrorPageTabObserver: Handle all that alternate error page crap
>
> Yes? I'm going to do that.
>
> http://codereview.chromium.org/8865004/

[Avi (use Gerrit), 2011-12-08 22:16:48]

Done; ptal.

(Can't stick around to see the results of the trybots but they seem to be going
OK...)

[Jói, 2011-12-09 12:36:47]

LGTM

Very cool!

http://codereview.chromium.org/8865004/diff/6001/chrome/browser/ui/alternate_...
File chrome/browser/ui/alternate_error_tab_observer.h (right):

http://codereview.chromium.org/8865004/diff/6001/chrome/browser/ui/alternate_...
chrome/browser/ui/alternate_error_tab_observer.h:15: // Per-tab class to handle
functionality that is core to the operation of tabs.
I think this is copy-pasted from the original change, probably needs to be
updated?

[Avi (use Gerrit), 2011-12-09 16:12:11]

http://codereview.chromium.org/8865004/diff/6001/chrome/browser/ui/alternate_...
File chrome/browser/ui/alternate_error_tab_observer.h (right):

http://codereview.chromium.org/8865004/diff/6001/chrome/browser/ui/alternate_...
chrome/browser/ui/alternate_error_tab_observer.h:15: // Per-tab class to handle
functionality that is core to the operation of tabs.
On 2011/12/09 12:36:47, Jói wrote:
> I think this is copy-pasted from the original change, probably needs to be
> updated?

Indeed.