Apply per-tab preference after RenderViewImpl creation.

Apply per-tab preference after RenderViewImpl creation.

BUG=105537
TEST=none yet


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

[mnaganov (inactive), 2011-11-30 14:52:49]

Hi Avi and Mattias,

I've discovered a small issue with applying per-tab prefs. If at the moment when
we are overriding a preference,  a RenderViewImpl instance hasn't yet been
created, then ViewMsg_UpdateWebPreferences message will be ignored, and an
instance created afterwards will not get the override. So it looks like we need
to send overrides explicitly upon RVI creation.

I'm addressing this issue in this patch. Please tell me if the approach is OK,
then I'll write a test.

[Avi (use Gerrit), 2011-11-30 15:15:17]

:( lgtm

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

http://codereview.chromium.org/8747015/diff/1/chrome/browser/ui/tab_contents/...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:239: friend class
PerTabPrefsTabHelper;  // for UpdateWebPreferences
This doesn't make me happy, but I'm not sure I have a choice.

Right now the TCW handles preferences, and that should be its job. In an ideal
world there would be a pref tab helper that would handle all the preference
stuff, both what TCW does now and the per-tab stuff. But that's probably going
to have to wait until I can attack it.

[mnaganov (inactive), 2011-11-30 15:19:35]

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

http://codereview.chromium.org/8747015/diff/1/chrome/browser/ui/tab_contents/...
chrome/browser/ui/tab_contents/tab_contents_wrapper.h:239: friend class
PerTabPrefsTabHelper;  // for UpdateWebPreferences
On 2011/11/30 15:15:18, Avi wrote:
> This doesn't make me happy, but I'm not sure I have a choice.
> 
> Right now the TCW handles preferences, and that should be its job. In an ideal
> world there would be a pref tab helper that would handle all the preference
> stuff, both what TCW does now and the per-tab stuff. But that's probably going
> to have to wait until I can attack it.

An alternative arrangement is to put a call to UpdateWebPreferences into
TabContentsWrapper::RenderViewCreated. This will eliminate the need for
introducing friendship. To me, handling this in PerTabPrefsHelper is more
logical because these are overridden settings that need to be updated.

[Avi (use Gerrit), 2011-11-30 15:23:26]

I'm looking at a different level. TCW shouldn't be doing what it's doing. I need
to refactor it to fix it. For now, yeah.

[Avi (use Gerrit), 2011-11-30 19:07:50]

On 2011/11/30 15:23:26, Avi wrote:
> I'm looking at a different level. TCW shouldn't be doing what it's doing. I
need
> to refactor it to fix it. For now, yeah.

BTW, re crbug.com/105872, I'm going to rip all the prefs code from TCW and turn
your per-tab-prefs tab helper into a general prefs tab helper. Hope you don't
mind. Will do it once you land this.

[mnaganov (inactive), 2011-12-01 15:44:18]

On 2011/11/30 19:07:50, Avi wrote:
> On 2011/11/30 15:23:26, Avi wrote:
> > I'm looking at a different level. TCW shouldn't be doing what it's doing. I
> need
> > to refactor it to fix it. For now, yeah.
> 
> BTW, re crbug.com/105872, I'm going to rip all the prefs code from TCW and
turn
> your per-tab-prefs tab helper into a general prefs tab helper. Hope you don't
> mind. Will do it once you land this.

Sure, go ahead! I agree that TCW currently follows the "Blob" pattern and needs
to be fixed.

I've added a test, so now I'll pass the CL through trybots and submit.

[commit-bot: I haz the power, 2011-12-01 16:46:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mnaganov@chromium.org/8747015/5001

[commit-bot: I haz the power, 2011-12-01 17:43:16]

Change committed as 112479