Move blocked content from TabContents to TabContentsWrapper.

Move blocked content from TabContents to TabContentsWrapper.

BUG=71097
TEST=hammer on popup blocking; should be no visible change

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

[Avi (use Gerrit), 2011-04-14 21:23:25]

jochen: this is your area, review away
jam: fyi; anything jochen doesn't cover

As bad as this looks, this is basically creating browser/ui/content_settings/,
moving some content settings ui code into there, ripping out the content
settings code from TabContents and making a ContentSettingsTabHelper. The rest
is fallout. There shouldn't be anything tricky, but as always, feel free to give
me a holler.

[Avi (use Gerrit), 2011-04-14 21:41:57]

BTW, what wasn't moved was TabSpecificContentSettings. Alas, it's called from
the renderer code, so it'll stick there... for now.

[jochen (gone - plz use gerrit), 2011-04-14 23:11:04]

http://codereview.chromium.org/6854035/diff/1/chrome/browser/ui/content_setti...
File chrome/browser/ui/content_settings/content_settings_tab_helper.h (right):

http://codereview.chromium.org/6854035/diff/1/chrome/browser/ui/content_setti...
chrome/browser/ui/content_settings/content_settings_tab_helper.h:19: // Per-tab
class to manage various allowed and disallowed content.
so, there seems to be an misunderstanding.

there are 'content settings', where content is e.g. images

and there is 'blocked contents', which is a blocked TabContent, i.e. a blocked
popup

this helper seems to be about the latter, so maybe a name like
BlockedContentTabHelper or similar would be better?

http://codereview.chromium.org/6854035/diff/1/chrome/browser/ui/content_setti...
File chrome/browser/ui/content_settings/content_settings_tab_helper_delegate.h
(right):

http://codereview.chromium.org/6854035/diff/1/chrome/browser/ui/content_setti...
chrome/browser/ui/content_settings/content_settings_tab_helper_delegate.h:15: //
ContentSettingsTabHelper and to provide necessary functionality.
this delegate seems to be unrelated to the popup blocker helper you've
implemented

[Avi (use Gerrit), 2011-04-15 00:20:12]

http://codereview.chromium.org/6854035/diff/1/chrome/browser/ui/content_setti...
File chrome/browser/ui/content_settings/content_settings_tab_helper.h (right):

http://codereview.chromium.org/6854035/diff/1/chrome/browser/ui/content_setti...
chrome/browser/ui/content_settings/content_settings_tab_helper.h:19: // Per-tab
class to manage various allowed and disallowed content.
Gah.

The reason I did this in the first place was to fix the content settings
bubbles, which apparently also handle blocked contents. Whee! Lemme do some
renaming.

http://codereview.chromium.org/6854035/diff/1/chrome/browser/ui/content_setti...
File chrome/browser/ui/content_settings/content_settings_tab_helper_delegate.h
(right):

http://codereview.chromium.org/6854035/diff/1/chrome/browser/ui/content_setti...
chrome/browser/ui/content_settings/content_settings_tab_helper_delegate.h:15: //
ContentSettingsTabHelper and to provide necessary functionality.
On 2011/04/14 23:11:04, jochen wrote:
> this delegate seems to be unrelated to the popup blocker helper you've
> implemented

The GetConstrainingContents does belong as a delegate of ... um ...
BlockedContentsTabHelper, or whatever that gets renamed to. I'll put the content
settings functions back. For now.

[Avi (use Gerrit), 2011-04-15 16:00:00]

OK, new snapshot up, should be a lot more to your liking. Please take a look.

[Avi (use Gerrit), 2011-04-18 13:25:32]

jochen--

ping; please review

[Avi (use Gerrit), 2011-04-18 17:11:48]

John, until Jochen can take a look, can you review? I'm out for tomorrow/Wed,
but this has been sitting too long and I'd like to land it soon.

[Avi (use Gerrit), 2011-04-21 13:50:32]

Sky, can you look at this? I'd like to land this as the ground keeps shifting
under my feet.

[Avi (use Gerrit), 2011-04-21 13:57:38]

Gah, sky's out 'til Monday. Elliot?

[Avi (use Gerrit), 2011-04-21 15:21:02]

Adding Steven.

Steven, you committed your popup fix which I had to incorporate into this CL.
Please review as well.

I renamed AddOrBlockNewContents back to AddNewContents because content/ doesn't
know about blocking. When you renamed it, the sole remaining caller of
AddNewContents was in AddPopup, which now moved to BlockedContentTabHelper, so I
altered it so that it calls AddNewContents with user_gesture == true to force
the same code path.

Bug 79777 verified as still fixed.

[stevenjb, 2011-04-21 19:02:59]

This will have some conflicts with http://codereview.chromium.org/6881073/, but
I am happy to hold off on that and do the merge since that is a smaller CL.

Apologies for all the nits about tab_contents -> tab_contents_wrapper, but the
tab/browser code is pretty daunting, so I think that anything that helps with
readability, even just a little, is worth the effort. This cleanup will
certainly help a lot!

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:2012:
TabContentsWrapper* tab_contents =
nit: 'tab_contents_wrapper'

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:3581:
TabContentsWrapper* tab_contents =
nit: 'tab_contents_wrapper'

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:3791:
BlockedContentTabHelper* blocked_content =
nit: blocked_content_helper? Very close to 'blocked_contents'

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:3814:
TabContentsWrapper* tab_contents = GetTabContentsWrapperFromDict(
nit: 'tab_contents_wrapper'

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:3826:
BlockedContentTabHelper* blocked_content =
nit: blocked_content_helper?

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
File chrome/browser/ui/blocked_content/blocked_content_container.cc (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
chrome/browser/ui/blocked_content/blocked_content_container.cc:40: void
BlockedContentContainer::AddTabContents(TabContentsWrapper* tab_contents,
nit: tab_contents_wrapper, or maybe just wrapper? To avoid confusing
tab_contents->tab_contents() below?

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
chrome/browser/ui/blocked_content/blocked_content_container.cc:63:
TabContentsWrapper* tab_contents) {
nit: wrapper?

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
chrome/browser/ui/blocked_content/blocked_content_container.cc:135:
TabContentsWrapper* tab_contents = i->tab_contents;
nit: tab_contents_wrapper

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
File chrome/browser/ui/blocked_content/blocked_content_container.h (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
chrome/browser/ui/blocked_content/blocked_content_container.h:48:
std::vector<TabContentsWrapper*>* blocked_contents) const;
nit: Does it make sense to typedef std::vector<TabContentsWrapper*> given its
use here and in iterators elsewhere?

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
File chrome/browser/ui/blocked_content/blocked_content_tab_helper.cc (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
chrome/browser/ui/blocked_content/blocked_content_tab_helper.cc:95: //
BlockedContentContainer::AddTabContents()?
As long as this is getting re-factored, it might be a good time to eliminate the
user_gesture from BlockedContentContainer::AddTabContents() since we currently
only ever pass in 'true'.

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
File chrome/browser/ui/blocked_content/blocked_content_tab_helper.h (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
chrome/browser/ui/blocked_content/blocked_content_tab_helper.h:56:
std::vector<TabContentsWrapper*>* blocked_contents) const;
This is confusing - it's the same function name as in
BlockedContentTabHelperDelegate, and it just calls
blocked_contents_->GetBlockedContents(). I think the code would be easier to
follow if we just had a public blocked_contents() accessor and eliminated the
above two functions.

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
chrome/browser/ui/blocked_content/blocked_content_tab_helper.h:77:
BlockedContentTabHelperDelegate* delegate_;
This confused me for a while since delegate_ is unused by this class, and
blocked_contents_ is also a BlockedContentTabHelperDelegate. It looks like this
typically points to a Browser which is also a BlockedContentTabHelperDelegate,
so that other classes can access it? I don't know if calling this owner_ would
be any clearer, but maybe add something like (e.g. a Browser) after "owner" in
the comment?

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:2926: AddPopup(new_wrapper, initial_pos,
user_gesture);
Use gesture will always be false here, do we need that parameter in
BlockedContentTabHelper?

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/cocoa/con...
File
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_unittest.mm
(right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/cocoa/con...
chrome/browser/ui/cocoa/content_settings/content_setting_bubble_cocoa_unittest.mm:22:
DummyContentSettingBubbleModel(TabContentsWrapper* tab_contents,
nit: tab_contents_wrapper

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/cocoa/loc...
File chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/cocoa/loc...
chrome/browser/ui/cocoa/location_bar/content_setting_decoration.mm:252:
TabContentsWrapper* tabContents =
nit: tab_contents_wrapper

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.cc (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:30:
ContentSettingTitleAndLinkModel(TabContentsWrapper* tab_contents,
nit: tab_contents_wrapper

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:135:
ContentSettingTitleLinkAndCustomModel(TabContentsWrapper* tab_contents,
nit: tab_contents_wrapper

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:169:
ContentSettingSingleRadioGroup(TabContentsWrapper* tab_contents,
nit: tab_contents_wrapper

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:318:
ContentSettingCookiesBubbleModel(TabContentsWrapper* tab_contents,
nit: tab_contents_wrapper

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:349:
ContentSettingPluginBubbleModel(TabContentsWrapper* tab_contents,
nit: tab_contents_wrapper

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:373:
ContentSettingPopupBubbleModel(TabContentsWrapper* tab_contents,
nit: tab_contents_wrapper

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:413:
ContentSettingDomainListBubbleModel(TabContentsWrapper* tab_contents,
nit: tab_contents_wrapper

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:482:
TabContentsWrapper* tab_contents,
nit: tab_contents_wrapper

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.cc:506:
TabContentsWrapper* tab_contents,
nit: tab_contents_wrapper

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/content_s...
File chrome/browser/ui/content_settings/content_setting_bubble_model.h (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:95:
TabContentsWrapper* tab_contents() const { return tab_contents_; }
nit: tab_contents_wrapper()

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/content_s...
chrome/browser/ui/content_settings/content_setting_bubble_model.h:120:
TabContentsWrapper* tab_contents_;
nit: tab_contents_wrapper_

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/gtk/locat...
File chrome/browser/ui/gtk/location_bar_view_gtk.cc (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/gtk/locat...
chrome/browser/ui/gtk/location_bar_view_gtk.cc:1362: TabContentsWrapper*
tab_contents = parent_->GetTabContentsWrapper();
nit: tab_contents_wrapper

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/views/loc...
File chrome/browser/ui/views/location_bar/content_setting_image_view.cc (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/views/loc...
chrome/browser/ui/views/location_bar/content_setting_image_view.cc:131:
TabContentsWrapper* tab_contents = parent_->GetTabContentsWrapper();
nit: tab_contents_wrapper

http://codereview.chromium.org/6854035/diff/14002/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/6854035/diff/14002/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:353: bool user_gesture);
I'm fine with renaming this back to AddNewContents. One reason I renamed it was
to help with confusion between TabContents::AddNewContents and
TabContentsDelegate::AddNewContents(). I don't know how much of a pita
eliminating this fucntion would be, now that it just calls
delegate()->AddNewContents(), but at least it's now just a wrapper.

[Avi (use Gerrit), 2011-04-21 20:06:02]

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:2012:
TabContentsWrapper* tab_contents =
On 2011/04/21 19:03:00, Steven Bennetts wrote:
> nit: 'tab_contents_wrapper'

In most cases I let that stay, as TabContentsWrapper will eventually be renamed
TabContents.

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
File chrome/browser/ui/blocked_content/blocked_content_container.h (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
chrome/browser/ui/blocked_content/blocked_content_container.h:48:
std::vector<TabContentsWrapper*>* blocked_contents) const;
Dunno. Tried that, but with the scoped name
(BlockedContentTabHelper::ContentsVector) It's just as long.

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
File chrome/browser/ui/blocked_content/blocked_content_tab_helper.cc (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
chrome/browser/ui/blocked_content/blocked_content_tab_helper.cc:95: //
BlockedContentContainer::AddTabContents()?
On 2011/04/21 19:03:00, Steven Bennetts wrote:
> As long as this is getting re-factored, it might be a good time to eliminate
the
> user_gesture from BlockedContentContainer::AddTabContents() since we currently
> only ever pass in 'true'.

Done.

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
File chrome/browser/ui/blocked_content/blocked_content_tab_helper.h (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
chrome/browser/ui/blocked_content/blocked_content_tab_helper.h:56:
std::vector<TabContentsWrapper*>* blocked_contents) const;
On 2011/04/21 19:03:00, Steven Bennetts wrote:
> I think the code would be easier to
> follow if we just had a public blocked_contents() accessor and eliminated the
> above two functions.

Nononono. It used to be that way. The problem was that then the blocked contents
container needed to have knowledge about the upper levels, and call back out to
its owner on some conditions, etc. It was a nightmare, complicated code
severely, and caused at least one security bug that I could find.

BlockedContentTabHelper completely wraps BlockedContentContainer. No one except
for the BCTH should be accessing the BCC.

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
chrome/browser/ui/blocked_content/blocked_content_tab_helper.h:77:
BlockedContentTabHelperDelegate* delegate_;
Sure. In general, the Browser is always the delegate for the TabContents, the
TCW, and all the TabHelpers.

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/browser.c...
chrome/browser/ui/browser.cc:2926: AddPopup(new_wrapper, initial_pos,
user_gesture);
On 2011/04/21 19:03:00, Steven Bennetts wrote:
> Use gesture will always be false here, do we need that parameter in
> BlockedContentTabHelper?

Done.

http://codereview.chromium.org/6854035/diff/14002/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/6854035/diff/14002/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:353: bool user_gesture);
On 2011/04/21 19:03:00, Steven Bennetts wrote:
>  I don't know how much of a pita
> eliminating this fucntion would be

My change scares me enough already :)

[stevenjb, 2011-04-21 21:06:44]

LGTM

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/automation/t...
File chrome/browser/automation/testing_automation_provider.cc (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/automation/t...
chrome/browser/automation/testing_automation_provider.cc:2012:
TabContentsWrapper* tab_contents =
On 2011/04/21 20:06:02, Avi wrote:
> On 2011/04/21 19:03:00, Steven Bennetts wrote:
> > nit: 'tab_contents_wrapper'
> 
> In most cases I let that stay, as TabContentsWrapper will eventually be
renamed
> TabContents.
OK, fair enough, that will be better.

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
File chrome/browser/ui/blocked_content/blocked_content_container.h (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
chrome/browser/ui/blocked_content/blocked_content_container.h:48:
std::vector<TabContentsWrapper*>* blocked_contents) const;
On 2011/04/21 20:06:02, Avi wrote:
> Dunno. Tried that, but with the scoped name
> (BlockedContentTabHelper::ContentsVector) It's just as long.
It's not so much a length issue, just easier on the eyes to see
TabContentsWrapperList::iterator, etc. Just a nit though.

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
File chrome/browser/ui/blocked_content/blocked_content_tab_helper.h (right):

http://codereview.chromium.org/6854035/diff/14002/chrome/browser/ui/blocked_c...
chrome/browser/ui/blocked_content/blocked_content_tab_helper.h:56:
std::vector<TabContentsWrapper*>* blocked_contents) const;
On 2011/04/21 20:06:02, Avi wrote:
> On 2011/04/21 19:03:00, Steven Bennetts wrote:
> > I think the code would be easier to
> > follow if we just had a public blocked_contents() accessor and eliminated
the
> > above two functions.
> 
> Nononono. It used to be that way. The problem was that then the blocked
contents
> container needed to have knowledge about the upper levels, and call back out
to
> its owner on some conditions, etc. It was a nightmare, complicated code
> severely, and caused at least one security bug that I could find.
> 
> BlockedContentTabHelper completely wraps BlockedContentContainer. No one
except
> for the BCTH should be accessing the BCC.

Yeah, this makes more sense after doing the entire review than it did when I
wrote the comment. Maybe change the function comment here to something like
"Wrapper for blocked_contents_->GetBlockedContents()" to make it clear that the
implementation is in BlockedContentContainer?

http://codereview.chromium.org/6854035/diff/14002/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.h (right):

http://codereview.chromium.org/6854035/diff/14002/content/browser/tab_content...
content/browser/tab_contents/tab_contents.h:353: bool user_gesture);
On 2011/04/21 20:06:02, Avi wrote:
> On 2011/04/21 19:03:00, Steven Bennetts wrote:
> >  I don't know how much of a pita
> > eliminating this fucntion would be
> 
> My change scares me enough already :)
Understood :)

[Avi (use Gerrit), 2011-04-21 21:13:43]

On 2011/04/21 21:06:44, Steven Bennetts wrote:
> Yeah, this makes more sense after doing the entire review than it did when I
> wrote the comment. Maybe change the function comment here to something like
> "Wrapper for blocked_contents_->GetBlockedContents()" to make it clear that
the
> implementation is in BlockedContentContainer?

Sure.

[Avi (use Gerrit), 2011-04-21 21:20:50]

I just realized an issue.

If we drop the user_gesture flag from the blockedcontentcontainer, I thing we
break all contents blocked.

I think that's instant. It sets that and drops that flag. When it drops that
flag, we have to reevaluate the popups. We can't properly reevaluate that flag
if we haven't been settings it.

Damn, gotta put that back.

[stevenjb, 2011-04-21 22:38:07]

One small comment that you can ignore if you like. LGTM.

http://codereview.chromium.org/6854035/diff/19001/chrome/browser/ui/blocked_c...
File chrome/browser/ui/blocked_content/blocked_content_tab_helper.cc (right):

http://codereview.chromium.org/6854035/diff/19001/chrome/browser/ui/blocked_c...
chrome/browser/ui/blocked_content/blocked_content_tab_helper.cc:73: bool
user_gesture) {
This doesn't actually need user_gesture since it doesn't use it.

[Avi (use Gerrit), 2011-04-22 03:22:28]

http://codereview.chromium.org/6854035/diff/19001/chrome/browser/ui/blocked_c...
File chrome/browser/ui/blocked_content/blocked_content_tab_helper.cc (right):

http://codereview.chromium.org/6854035/diff/19001/chrome/browser/ui/blocked_c...
chrome/browser/ui/blocked_content/blocked_content_tab_helper.cc:73: bool
user_gesture) {
On 2011/04/21 22:38:07, Steven Bennetts wrote:
> This doesn't actually need user_gesture since it doesn't use it.

True. Next CL; I just want to commit this :)