Cache the current WebPreferences on RenderViewHostImpl.

Cache the current WebPreferences on RenderViewHostImpl.

This makes lookups of a WebPreferences field fast.
In order do this, add code to listen to all preferences updates in order to recompute the cache.

BUG=390799
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=284592

[jam, 2014-07-15 00:29:59]

(I saw you replied on the cr-dev thread and also here; I'll reply here since it
seems more targeted.)

I think this adds unnecessary complexity in terms of the new concept of
overrider in the public api, and the web preferences manager inside content. if
the code is switched to have chrome push new changes instead of content polling,
then there wouldn't be any new concepts added.

I wonder what's taking .7ms to generate this data, is it one expensive call
somewhere or a lot of string manipulation spread out? If it's one thing that's
slow, perhaps we can optimize that until this code is refactored in simpler way?

[chrishtr, 2014-07-15 04:48:44]

On 2014/07/15 00:29:59, jam wrote:
> (I saw you replied on the cr-dev thread and also here; I'll reply here since
it
> seems more targeted.)

Sure, no problem.

> 
> I think this adds unnecessary complexity in terms of the new concept of
> overrider in the public api, and the web preferences manager inside content.
if
> the code is switched to have chrome push new changes instead of content
polling,
> then there wouldn't be any new concepts added.
 
Well there would be the concept of pushing the new change, which is not present
right now, so a collection of API methods for that. And code to wrap things that
update the prefs in order to
distribute state to the central prefs registry.

The overrider concept already exists in a different form. The only new concept
is looking up a specific preference (web security enabled in this example). I
added an interface in order to have
a way to group more methods than just the current OverrideWebkitPrefs method. If
that's too heavyweight I could undo that and just add a method next to
OverrideWebkitPrefs in the same
interface it's already in, but the design expects there to be more of these over
time, enough to justify the new interface. If it waasn't for the overrider, this
CL would have been a lot easier..

PrefService also looks like a generic key/value store interface, with some
notifier hooks for specific paths. So some code would need to wrap all calls
into it in order to keep local state? Or hook into the PrefNotifier concept...
And whatever one-off code overrides prefs right now for Chrome or WebView would
have to be rewritten to do so at renderer startup time, as opposed to on-demand.

IMO the WebPreferencesManager interface does seem to be reasonably clean as
well. It seems good to factor code that registers flags and sets defaults into a
class, given that these flags are global flags, not per-renderer. That it's
duplicated for each renderer process is an implementation detail.

I concluded based on all of this, plus the comments below, that it's not worth
it to rewrite a whole bunch of code and introduce potential state management
bugs just to get a little cleaner API.

> 
> I wonder what's taking .7ms to generate this data, is it one expensive call
> somewhere or a lot of string manipulation spread out? If it's one thing that's
> slow, perhaps we can optimize that until this code is refactored in simpler
way?

It was about half in the overrider, half in the non-overrider according to my
Android traces. I didn't dig into which specific piece, but since almost all
prefs are some function call(s) plus looking up some data plus maybe a
string/number conversion or a lookup of the current process ID, I figured it was
a little of each.

In the overrider, there is the call to GetPrefs() in the PrefsService, which is
presumably expensive. It looks like these preferences can potentially be on disk
or elsewhere expensive to look up from.
Also, font data was so expensive it was commented out for Android:
http://crbug.com/308033. This CL doesn't fix that directly though.

So in summary, my guess is this CL would save somewhere between a quarter and a
half of the total time to compute all prefs, most likely closer to the former.
And for looking up wether web security is enabled, it would be a ton faster. I
would expect to write follow-on CLs that would put more use cases behind
targeted interfaces, so ideally the only ones left that would be a heavyweight
computation of all prefs would be when reading all prefs in order to send them
across to a new Blink thread, since that struct is the way to communicate these
prefs with Blink.

The motivating example of this CL would be basically fixed by just adding the
web security enabled-specific code and skipping the rest, but I felt it was
worth dong some more work towards better factored code.

[jam, 2014-07-15 19:17:48]

A few comments:
-looking closer, it appears that Chrome already has a way to notify content
whenever the prefs change (both WebPreferences and RendererPreferences). see
chrome/browser/ui/prefs/prefs_tab_helper.cc. so i wonder if we can make content
stop polling for this information now. perhaps that necessitates adding more
items to kPrefsToObserve?
-PrefService is a chrome concept and we have avoided content knowing about it by
design
-the fact that we still have WebPreferences and RendererPreferences is
unnecessary now. That was needed when we had multiple embedders of blink's
chromium api (i.e. the old test_shell). Now that only content embeds it,
WebPreferences and RendererPreferences should be merged. That's beyond the scope
of this work, just pointing it out in case you or anyone is motivated to do
this.

[chrishtr, 2014-07-15 20:45:14]

On 2014/07/15 19:17:48, jam wrote:
> A few comments:
> -looking closer, it appears that Chrome already has a way to notify content
> whenever the prefs change (both WebPreferences and RendererPreferences). see
> chrome/browser/ui/prefs/prefs_tab_helper.cc. so i wonder if we can make
content
> stop polling for this information now. perhaps that necessitates adding more
> items to kPrefsToObserve?

Right. I mentioned that above as an option for uses of the prefs that are
maintained by the PrefService.
But there are also commandline flags, which are not covered by this, some
one-off examples such as 
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

And this stuff, implemented by Java:
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/na...

I think we'd need to add a new interface for setting each and every one of the
web preferences, that updated the central location (but not sent to Blink unless
some code said explicitly to do so,
or else we'd have a footgun). And the one-off examples like
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...
would have to be changed to somehow detect if the stuff being computed there
might have changed, and run that code.

Also note that PrefsTabHelper::OnWebPrefChanged() works by at the moment only
being called for certain font family changes.
If we were to generalize for all preferences, we'd have to have a switch
statement that mapped from string back
to the field in WebPreferences to set (poor man's reflection).

> -PrefService is a chrome concept and we have avoided content knowing about it
by
> design
> -the fact that we still have WebPreferences and RendererPreferences is
> unnecessary now. That was needed when we had multiple embedders of blink's
> chromium api (i.e. the old test_shell). Now that only content embeds it,
> WebPreferences and RendererPreferences should be merged. That's beyond the
scope
> of this work, just pointing it out in case you or anyone is motivated to do
> this.

We might not want all of these to be available to Blink though? Hmm. Maybe that
doesn't matter much compared with simplifying the # of preference object types.

[jam, 2014-07-16 05:13:38]

On 2014/07/15 20:45:14, chrishtr wrote:
> On 2014/07/15 19:17:48, jam wrote:
> > A few comments:
> > -looking closer, it appears that Chrome already has a way to notify content
> > whenever the prefs change (both WebPreferences and RendererPreferences). see
> > chrome/browser/ui/prefs/prefs_tab_helper.cc. so i wonder if we can make
> content
> > stop polling for this information now. perhaps that necessitates adding more
> > items to kPrefsToObserve?
> 
> Right. I mentioned that above as an option for uses of the prefs that are
> maintained by the PrefService.
> But there are also commandline flags, which are not covered by this, some
> one-off examples such as 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

members of WebPreferences that are modified by command line flags never change,
so there's nothing to observe. To be very clear, I'm not saying that every
member will be updated through prefs system. I'm saying that watching for
changes to prefs is what triggers us to tell WebContents to update its prefs. At
that point, it goes and reads all the values.

> 
> And this stuff, implemented by Java:
>
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/na...

Similar to above, the android_webview code can tell WebContents whenever
WebSettings change. It looks like this is already being done, see
https://code.google.com/p/chromium/codesearch#chromium/src/android_webview/ja...
which calls mEventHandler.updateWebkitPreferencesLocked() for every WebSettings
method.

> 
> I think we'd need to add a new interface for setting each and every one of the
> web preferences, that updated the central location (but not sent to Blink
unless
> some code said explicitly to do so,
> or else we'd have a footgun).

I don't follow what you're talking about. Perhaps we should VC since that'll be
faster than email? (my latency is also worse than normal while I catch up after
vacation).

I'm saying we should use the existing PrefsTabHelper to push changes to chrome,
and take out the calls from content that ask chrome to update prefs.


> And the one-off examples like
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr...

(sorry that link is showing something that's probably different from what you
intended; cs looks like it got updated since you pasted that link)

> would have to be changed to somehow detect if the stuff being computed there
> might have changed, and run that code.
> 
> Also note that PrefsTabHelper::OnWebPrefChanged() works by at the moment only
> being called for certain font family changes.
> If we were to generalize for all preferences, we'd have to have a switch
> statement that mapped from string back
> to the field in WebPreferences to set (poor man's reflection).

It looks at more than fonts, also a hardcoded list of prefs. See kPrefsToObserve
which is used in:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...
> 
> > -PrefService is a chrome concept and we have avoided content knowing about
it
> by
> > design
> > -the fact that we still have WebPreferences and RendererPreferences is
> > unnecessary now. That was needed when we had multiple embedders of blink's
> > chromium api (i.e. the old test_shell). Now that only content embeds it,
> > WebPreferences and RendererPreferences should be merged. That's beyond the
> scope
> > of this work, just pointing it out in case you or anyone is motivated to do
> > this.
> 
> We might not want all of these to be available to Blink though? Hmm. Maybe
that
> doesn't matter much compared with simplifying the # of preference object
types.

I was only referring to the classes in content, I'm not suggesting we change
what's available to Blink. Right now we send two classes from content/browser to
content/renderer. Most, but not all, of WebPreferences goes to update Blink. I'm
saying we can merge them into one struct.

[chrishtr, 2014-07-18 23:10:11]

Rewrote the CL according to your suggestions. Turned out to
be a lot simpler than I thought, thanks for the design
persistence.

[jam, 2014-07-21 17:40:34]

lgtm

https://codereview.chromium.org/373323003/diff/290001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/373323003/diff/290001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:60:
prefs::kWebKitPasswordEchoEnabled,
this is the same as above. did you mean to add something else?

https://codereview.chromium.org/373323003/diff/290001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:79: prefs::kEnableHyperlinkAuditing,
nit: it would be helpful to sort this list

https://codereview.chromium.org/373323003/diff/290001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/373323003/diff/290001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1436: return
*web_preferences_;
nit: indentation

https://codereview.chromium.org/373323003/diff/290001/content/public/browser/...
File content/public/browser/render_view_host.h (right):

https://codereview.chromium.org/373323003/diff/290001/content/public/browser/...
content/public/browser/render_view_host.h:197: // Returns the current WebKit
preferences.
nit: now that the behavior of this method changed to return a cached version,
this should be called out in the comment

[jam, 2014-07-21 17:40:57]

also, please update the cl description

[chrishtr, 2014-07-21 18:16:07]

Also updated CL description.

https://codereview.chromium.org/373323003/diff/290001/chrome/browser/ui/prefs...
File chrome/browser/ui/prefs/prefs_tab_helper.cc (right):

https://codereview.chromium.org/373323003/diff/290001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:60:
prefs::kWebKitPasswordEchoEnabled,
On 2014/07/21 17:40:33, jam wrote:
> this is the same as above. did you mean to add something else?

No, it's a dup. Fixed.

https://codereview.chromium.org/373323003/diff/290001/chrome/browser/ui/prefs...
chrome/browser/ui/prefs/prefs_tab_helper.cc:79: prefs::kEnableHyperlinkAuditing,
On 2014/07/21 17:40:33, jam wrote:
> nit: it would be helpful to sort this list

Done.

https://codereview.chromium.org/373323003/diff/290001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/373323003/diff/290001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:1436: return
*web_preferences_;
On 2014/07/21 17:40:33, jam wrote:
> nit: indentation

Done.

https://codereview.chromium.org/373323003/diff/290001/content/public/browser/...
File content/public/browser/render_view_host.h (right):

https://codereview.chromium.org/373323003/diff/290001/content/public/browser/...
content/public/browser/render_view_host.h:197: // Returns the current WebKit
preferences.
On 2014/07/21 17:40:33, jam wrote:
> nit: now that the behavior of this method changed to return a cached version,
> this should be called out in the comment

Done.

[chrishtr, 2014-07-21 23:22:48]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-07-21 23:24:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishtr@chromium.org/373323003/370001

[commit-bot: I haz the power, 2014-07-22 00:41:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-22 00:44:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-22 00:44:14]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[chrishtr, 2014-07-22 01:54:24]

+michaelbai for approval of android_webview/native

[michaelbai, 2014-07-22 02:03:59]

lgtm for android_webview

[chrishtr, 2014-07-22 02:04:29]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2014-07-22 02:05:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/chrishtr@chromium.org/373323003/370001

[commit-bot: I haz the power, 2014-07-22 02:10:45]

Change committed as 284592

[Avi (use Gerrit), 2014-07-30 20:47:58]

On 2014/07/22 02:10:45, I haz the power (commit-bot) wrote:
> Change committed as 284592

Chrome now locks up at startup:
https://code.google.com/p/chromium/issues/detail?id=398751 and the backtrace
runs through new code introduced in this CL. Can you take a look?

[erikchen, 2014-07-31 02:18:58]

A revert of this CL has been created in
https://codereview.chromium.org/429413003/ by erikchen@chromium.org.

The reason for reverting is: Causes DOA on 38-dev.

https://code.google.com/p/chromium/issues/detail?id=398751.