Remove per-tab preference machinery.

Remove per-tab preference machinery.

It's no longer needed, according to mnaganov@, and it wasn't interacting well
with extension-controlled preferences. This is a partial revert of the
following:

http://codereview.chromium.org/8879016/
http://codereview.chromium.org/8747015/
http://codereview.chromium.org/8716004/
http://codereview.chromium.org/8568019/

The "global" pref names are are kept as is. I plan to rename them back
to the original names after this patch.

Also, per-tab preferences introduced in the following are reverted:
https://chromiumcodereview.appspot.com/9264071/
https://chromiumcodereview.appspot.com/9255018/



BUG=105537
TEST=manually and unit_tests --gtest_filter=Pref* and
browser_tests --gtest_filter=PrefsTabHelperBrowserTest* still pass


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

[falken, 2012-03-23 10:41:11]

https://chromiumcodereview.appspot.com/9838050/diff/1/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (left):

https://chromiumcodereview.appspot.com/9838050/diff/1/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:575: UpdateWebPreferences();
It's OK to remove this, right? It was introduced here
http://codereview.chromium.org/8747015/

[mnaganov (inactive), 2012-03-23 10:47:03]

LGTM

Thanks for dealing with this!

https://chromiumcodereview.appspot.com/9838050/diff/1/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (left):

https://chromiumcodereview.appspot.com/9838050/diff/1/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:575: UpdateWebPreferences();
On 2012/03/23 10:41:12, falken wrote:
> It's OK to remove this, right? It was introduced here
> http://codereview.chromium.org/8747015/

I think so. This was needed because we had overrides for Chrome's WebPreferences
defaults, and it was crucial to apply them at the moment when RenderView has
been created.

[Bernhard Bauer, 2012-03-23 10:48:14]

https://chromiumcodereview.appspot.com/9838050/diff/1/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (left):

https://chromiumcodereview.appspot.com/9838050/diff/1/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:620: WebPreferences prefs =
rvhd->GetWebkitPrefs();
Where do we fill in the WebPreferences struct now?

https://chromiumcodereview.appspot.com/9838050/diff/1/webkit/glue/webpreferen...
File webkit/glue/webpreferences.h (left):

https://chromiumcodereview.appspot.com/9838050/diff/1/webkit/glue/webpreferen...
webkit/glue/webpreferences.h:52: bool images_enabled;
I think we still need this, no? Remember that we're only removing per-tab
preferences from Chrome, but Webkit still has the flag.

[mnaganov (inactive), 2012-03-23 10:58:23]

https://chromiumcodereview.appspot.com/9838050/diff/1/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (left):

https://chromiumcodereview.appspot.com/9838050/diff/1/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:620: WebPreferences prefs =
rvhd->GetWebkitPrefs();
On 2012/03/23 10:48:14, Bernhard Bauer wrote:
> Where do we fill in the WebPreferences struct now?

RenderViewHostDelegate does this. Here we were only overriding per-tab
preferences.

https://chromiumcodereview.appspot.com/9838050/diff/1/webkit/glue/webpreferen...
File webkit/glue/webpreferences.h (left):

https://chromiumcodereview.appspot.com/9838050/diff/1/webkit/glue/webpreferen...
webkit/glue/webpreferences.h:52: bool images_enabled;
On 2012/03/23 10:48:14, Bernhard Bauer wrote:
> I think we still need this, no? Remember that we're only removing per-tab
> preferences from Chrome, but Webkit still has the flag.

The flag has a default value, set in glue. There were no need to alter it before
I came in.

[falken, 2012-03-23 11:01:41]

https://chromiumcodereview.appspot.com/9838050/diff/1/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (left):

https://chromiumcodereview.appspot.com/9838050/diff/1/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:620: WebPreferences prefs =
rvhd->GetWebkitPrefs();
On 2012/03/23 10:58:23, Mikhail Naganov (Chromium) wrote:
> On 2012/03/23 10:48:14, Bernhard Bauer wrote:
> > Where do we fill in the WebPreferences struct now?
> 
> RenderViewHostDelegate does this. Here we were only overriding per-tab
> preferences.

For reference, this just reverts to the code before the per-tab change:
http://codereview.chromium.org/8716004/diff/14001/chrome/browser/ui/tab_conte...

[Bernhard Bauer, 2012-03-23 11:34:33]

https://chromiumcodereview.appspot.com/9838050/diff/1/webkit/glue/webpreferen...
File webkit/glue/webpreferences.h (left):

https://chromiumcodereview.appspot.com/9838050/diff/1/webkit/glue/webpreferen...
webkit/glue/webpreferences.h:52: bool images_enabled;
On 2012/03/23 10:58:23, Mikhail Naganov (Chromium) wrote:
> On 2012/03/23 10:48:14, Bernhard Bauer wrote:
> > I think we still need this, no? Remember that we're only removing per-tab
> > preferences from Chrome, but Webkit still has the flag.
> 
> The flag has a default value, set in glue. There were no need to alter it
before
> I came in.

But couldn't it still be changed by a different content embedder (i.e. Android)?

[mnaganov (inactive), 2012-03-23 11:40:49]

On 2012/03/23 11:34:33, Bernhard Bauer wrote:
>
https://chromiumcodereview.appspot.com/9838050/diff/1/webkit/glue/webpreferen...
> File webkit/glue/webpreferences.h (left):
> 
>
https://chromiumcodereview.appspot.com/9838050/diff/1/webkit/glue/webpreferen...
> webkit/glue/webpreferences.h:52: bool images_enabled;
> On 2012/03/23 10:58:23, Mikhail Naganov (Chromium) wrote:
> > On 2012/03/23 10:48:14, Bernhard Bauer wrote:
> > > I think we still need this, no? Remember that we're only removing per-tab
> > > preferences from Chrome, but Webkit still has the flag.
> > 
> > The flag has a default value, set in glue. There were no need to alter it
> before
> > I came in.
> 
> But couldn't it still be changed by a different content embedder (i.e.
Android)?

OK. I agree that it might be useful. Matt, can you please leave 'images_enabled'
in view_messages.h and in webpreferences.{h|cc}?

[falken, 2012-03-23 11:56:20]

On 2012/03/23 11:40:49, Mikhail Naganov (Chromium) wrote:
> On 2012/03/23 11:34:33, Bernhard Bauer wrote:
> >
>
https://chromiumcodereview.appspot.com/9838050/diff/1/webkit/glue/webpreferen...
> > File webkit/glue/webpreferences.h (left):
> > 
> >
>
https://chromiumcodereview.appspot.com/9838050/diff/1/webkit/glue/webpreferen...
> > webkit/glue/webpreferences.h:52: bool images_enabled;
> > On 2012/03/23 10:58:23, Mikhail Naganov (Chromium) wrote:
> > > On 2012/03/23 10:48:14, Bernhard Bauer wrote:
> > > > I think we still need this, no? Remember that we're only removing
per-tab
> > > > preferences from Chrome, but Webkit still has the flag.
> > > 
> > > The flag has a default value, set in glue. There were no need to alter it
> > before
> > > I came in.
> > 
> > But couldn't it still be changed by a different content embedder (i.e.
> Android)?
> 
> OK. I agree that it might be useful. Matt, can you please leave
'images_enabled'
> in view_messages.h and in webpreferences.{h|cc}?

OK, done.

[Bernhard Bauer, 2012-03-23 12:02:07]

LGTM!

[falken, 2012-03-23 12:11:18]

Bernhard and Mikhail, thanks for the review!

Avi: chrome/browser/tab_contents OWNER
Peter: chrome/browser/ui OWNER
Tony: webkit OWNER

[tony, 2012-03-23 18:05:05]

Does this turn off the xss auditor?  Was it enabled before?

[falken, 2012-03-23 18:15:48]

On 2012/03/23 18:05:05, tony wrote:
> Does this turn off the xss auditor?  Was it enabled before?

It's reverting https://chromiumcodereview.appspot.com/9264071/

Mikhail, can you confirm that the change to xss_auditor_enabled in
webpreferences.cc is OK?

[Peter Kasting, 2012-03-23 18:19:55]

LGTM w/question

http://codereview.chromium.org/9838050/diff/16/chrome/browser/ui/prefs/prefs_...
File chrome/browser/ui/prefs/prefs_tab_helper.h (left):

http://codereview.chromium.org/9838050/diff/16/chrome/browser/ui/prefs/prefs_...
chrome/browser/ui/prefs/prefs_tab_helper.h:21: class PrefsTabHelper : public
content::WebContentsObserver,
Can this parent class be removed now too?  Or do you still need to observe it?

[mnaganov (inactive), 2012-03-23 18:27:53]

In Chrome xss_auditor_enabled is initialized using a command-line flag:

http://code.google.com/searchframe#OAMlx_jo-ck/src/content/browser/tab_conten...

So it's actually doesn't matter what the initial value is. I'd prefer
to have it to be 'true', just in case. Tests that require it to be
false set it explicitly.

On Fri, Mar 23, 2012 at 18:15,  <falken@chromium.org> wrote:
> On 2012/03/23 18:05:05, tony wrote:
>>
>> Does this turn off the xss auditor?  Was it enabled before?
>
>
> It's reverting https://chromiumcodereview.appspot.com/9264071/
>
> Mikhail, can you confirm that the change to xss_auditor_enabled in
> webpreferences.cc is OK?
>
> http://codereview.chromium.org/9838050/

[falken, 2012-03-26 03:39:02]

I've changed the xss auditor back to enabled by default (so now there's no
change to webkit/).

Peter, Avi (OWNERS): please take another look.

http://codereview.chromium.org/9838050/diff/16/chrome/browser/ui/prefs/prefs_...
File chrome/browser/ui/prefs/prefs_tab_helper.h (left):

http://codereview.chromium.org/9838050/diff/16/chrome/browser/ui/prefs/prefs_...
chrome/browser/ui/prefs/prefs_tab_helper.h:21: class PrefsTabHelper : public
content::WebContentsObserver,
On 2012/03/23 18:19:55, Peter Kasting wrote:
> Can this parent class be removed now too?  Or do you still need to observe it?

It looks right to remove it. Done.

[Avi (use Gerrit), 2012-03-26 03:54:24]

lgtm

[falken, 2012-03-26 04:38:47]

On 2012/03/26 03:54:24, Avi wrote:
> lgtm

Thanks. I just noticed render_view_context_menu.cc should be checking
kWebKitGlobalJavascriptEnabled, not kWebkitJavaScriptEnabled. It's a trivial
change so shouldn't need another review. It will move back when I remove the
"Global" names in a subsequent patch.

I also looked for other bad accessing of the removed per-tab pref names and
didn't find any.

[falken, 2012-03-26 06:15:51]

On 2012/03/26 03:39:02, falken wrote:
> I've changed the xss auditor back to enabled by default (so now there's no
> change to webkit/).
> 
> Peter, Avi (OWNERS): please take another look.
> 
>
http://codereview.chromium.org/9838050/diff/16/chrome/browser/ui/prefs/prefs_...
> File chrome/browser/ui/prefs/prefs_tab_helper.h (left):
> 
>
http://codereview.chromium.org/9838050/diff/16/chrome/browser/ui/prefs/prefs_...
> chrome/browser/ui/prefs/prefs_tab_helper.h:21: class PrefsTabHelper : public
> content::WebContentsObserver,
> On 2012/03/23 18:19:55, Peter Kasting wrote:
> > Can this parent class be removed now too?  Or do you still need to observe
it?
> 
> It looks right to remove it. Done.

I'll go ahead and land this as the change to remove the WebContentsObserver
parent class was quite simple.

[commit-bot: I haz the power, 2012-03-26 06:16:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/falken@chromium.org/9838050/9016

[commit-bot: I haz the power, 2012-03-26 07:55:05]

Change committed as 128871