Add per-tab "JavaScript enabled" preference.

Add per-tab "JavaScript enabled" preference.

BUG=105537
TEST=TabContentsWrapperTest.PerTabJavaScriptEnabled


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

[Avi (use Gerrit), 2011-11-28 13:35:14]

On 2011/11/28 12:17:42, Mikhail Naganov (Chromium) wrote:

Serious changes in functionality like this _require_ BUG numbers. Please file a
bug to track this, and change the CL description.

[mnaganov (inactive), 2011-11-28 13:44:46]

On 2011/11/28 13:35:14, Avi wrote:
> On 2011/11/28 12:17:42, Mikhail Naganov (Chromium) wrote:
> 
> Serious changes in functionality like this _require_ BUG numbers. Please file
a
> bug to track this, and change the CL description.

Done.

[Mattias Nissler (ping if slow), 2011-11-28 15:04:16]

Adding Avi, since I'm not so familiar with TabContents. The pref-related stuff
LGTM with nits, but please wait for Avi's opinion.

http://codereview.chromium.org/8716004/diff/1/chrome/browser/prefs/overlay_us...
File chrome/browser/prefs/overlay_user_pref_store_unittest.cc (left):

http://codereview.chromium.org/8716004/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store_unittest.cc:20: // There is no
real-life sense in mapping these two properties, it for test
s/it/it's/

http://codereview.chromium.org/8716004/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/8716004/diff/1/chrome/common/pref_names.cc#new...
chrome/common/pref_names.cc:191: "webkit.webprefs.global.javascript_enabled";
Is the strategy to interleave the new type here? Wouldn't it be clearer to
create another block below with a comment that explains what this is about?

[mnaganov (inactive), 2011-11-28 15:20:05]

Thanks!

http://codereview.chromium.org/8716004/diff/1/chrome/browser/prefs/overlay_us...
File chrome/browser/prefs/overlay_user_pref_store_unittest.cc (left):

http://codereview.chromium.org/8716004/diff/1/chrome/browser/prefs/overlay_us...
chrome/browser/prefs/overlay_user_pref_store_unittest.cc:20: // There is no
real-life sense in mapping these two properties, it for test
On 2011/11/28 15:04:16, Mattias Nissler wrote:
> s/it/it's/

Thanks, but this text is being removed :)

http://codereview.chromium.org/8716004/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

http://codereview.chromium.org/8716004/diff/1/chrome/common/pref_names.cc#new...
chrome/common/pref_names.cc:191: "webkit.webprefs.global.javascript_enabled";
On 2011/11/28 15:04:16, Mattias Nissler wrote:
> Is the strategy to interleave the new type here? Wouldn't it be clearer to
> create another block below with a comment that explains what this is about?

I think we need to group per-tab preferences. So I've moved
kWebKitJavascriptEnabled and added a group comment.

[Avi (use Gerrit), 2011-11-28 15:43:52]

Before, all you were doing was creating a PrefService 1:1 with tabs, so what you
were doing was OK. The problem here is that now you need more machinery, to
register prefs, handle them, etc.

Please do not add machinery to TCW. I'm trying to rip machinery out. TCW is
supposed to just hold items that need 1:1 correspondences with TCs. That's it.

What you want to do is create a PerTabPrefsTabHelper. Some class to *contain*
the PrefService. It can be an observer to observe what's needed; it can be a
TabContentsObserver if required.

Follow the lead of any of the existing TabHelpers, or ask if you have any
questions.

[mnaganov (inactive), 2011-11-29 13:18:48]

On 2011/11/28 15:43:52, Avi wrote:
> Before, all you were doing was creating a PrefService 1:1 with tabs, so what
you
> were doing was OK. The problem here is that now you need more machinery, to
> register prefs, handle them, etc.
> 
> Please do not add machinery to TCW. I'm trying to rip machinery out. TCW is
> supposed to just hold items that need 1:1 correspondences with TCs. That's it.
> 
> What you want to do is create a PerTabPrefsTabHelper. Some class to *contain*
> the PrefService. It can be an observer to observe what's needed; it can be a
> TabContentsObserver if required.
> 
> Follow the lead of any of the existing TabHelpers, or ask if you have any
> questions.

Avi, I've addressed your comment. Please take another look.

[Avi (use Gerrit), 2011-11-29 15:13:52]

Excellent! I have style comments, but the TabContents related code LGTM.

http://codereview.chromium.org/8716004/diff/8001/chrome/browser/ui/tab_conten...
File chrome/browser/ui/tab_contents/per_tab_prefs_tab_helper.cc (right):

http://codereview.chromium.org/8716004/diff/8001/chrome/browser/ui/tab_conten...
chrome/browser/ui/tab_contents/per_tab_prefs_tab_helper.cc:9: #include
"chrome/browser/prefs/pref_service.h"
alphabetize

http://codereview.chromium.org/8716004/diff/8001/chrome/browser/ui/tab_conten...
chrome/browser/ui/tab_contents/per_tab_prefs_tab_helper.cc:39: {
opening braces always share the previous line

http://codereview.chromium.org/8716004/diff/8001/chrome/browser/ui/tab_conten...
chrome/browser/ui/tab_contents/per_tab_prefs_tab_helper.cc:45: {
opening braces always share the previous line

[mnaganov (inactive), 2011-11-29 16:57:59]

Thanks!

I'm fixing Mac/Win compilation error (need to forward declare "struct
WebPreferences", not class), and will submit.

http://codereview.chromium.org/8716004/diff/8001/chrome/browser/ui/tab_conten...
File chrome/browser/ui/tab_contents/per_tab_prefs_tab_helper.cc (right):

http://codereview.chromium.org/8716004/diff/8001/chrome/browser/ui/tab_conten...
chrome/browser/ui/tab_contents/per_tab_prefs_tab_helper.cc:9: #include
"chrome/browser/prefs/pref_service.h"
On 2011/11/29 15:13:53, Avi wrote:
> alphabetize

Done.

http://codereview.chromium.org/8716004/diff/8001/chrome/browser/ui/tab_conten...
chrome/browser/ui/tab_contents/per_tab_prefs_tab_helper.cc:39: {
On 2011/11/29 15:13:53, Avi wrote:
> opening braces always share the previous line

Done.

http://codereview.chromium.org/8716004/diff/8001/chrome/browser/ui/tab_conten...
chrome/browser/ui/tab_contents/per_tab_prefs_tab_helper.cc:45: {
On 2011/11/29 15:13:53, Avi wrote:
> opening braces always share the previous line

Done.

[commit-bot: I haz the power, 2011-11-29 18:16:43]

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

[commit-bot: I haz the power, 2011-11-29 19:56:49]

Change committed as 112001