[webkit-reviews] review canceled: [Bug 209678] API::PageConfiguration may have conflicting preference values between WebPreferences and WebPreferencesStore::ValueMap instance variables : [Attachment 395063] Patch v7

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has canceled David Kilzer
(:ddkilzer) <ddkilzer at webkit.org>'s request for review:
Bug 209678: API::PageConfiguration may have conflicting preference values
between WebPreferences and WebPreferencesStore::ValueMap instance variables
https://bugs.webkit.org/show_bug.cgi?id=209678

Attachment 395063: Patch v7

https://bugs.webkit.org/attachment.cgi?id=395063&action=review




--- Comment #28 from David Kilzer (:ddkilzer) <ddkilzer at webkit.org> ---
Comment on attachment 395063
  --> https://bugs.webkit.org/attachment.cgi?id=395063
Patch v7

View in context: https://bugs.webkit.org/attachment.cgi?id=395063&action=review

>> Source/WebKit/ChangeLog:10
>> +	    API::PageConfiguration::m_preferenceValues and
> 
> I'm wondering why we had this in the first place.

Well, because Chris Dumez thought in
<https://bugs.webkit.org/attachment.cgi?id=394576&action=review> that this
looked suspicious in WebPageProxy.cpp:

   
preferencesStore().getBoolValueForKey(WebPreferencesKey::attachmentElementEnabl
edKey())

And that this looked more correct:

    m_preferences->attachmentElementEnabled()

So in order to make the latter work in all cases, I eliminated
API::PageConfiguration::m_preferenceValues and
WebPageProxy::m_configurationPreferenceValues and forced all values to be
stored on WebPreferences::store() instead, which is where they end up anyway
the first time WebPageProxy::preferencesStore() is called.  (As I mention in
reply to Bren't comment below, WebPageProxy::preferencesStore() just looks like
it's implementing lazy initialization in way that requires
WebProxyPage::preferencesStore() to be called at least once before direct
methods on m_preferences work as expected.)

So I could have fixed this by somehow calling WebPageProxy::preferencesStore()
somewhere in WKWebView's initialization code, but eliminating the lazy
initialization seemed like a better solution.  I didn't see what advantage the
lazy initialization had, especially when WebPageProxy::preferencesStore()
recopied every value in m_configurationPreferenceValues each time it was
called.

And as I mention a couple lines below, the only thing you have to do is to make
sure that API::PageConfiguration::m_preferences is set to a valid
WebPreferences object before setting values.  (Maybe there was a reason that
API::PageConfiguration also needed to lazily set its m_preferences instance
variable, but I'm not sure what that was.  In all but one case (in -[WKView
initWithFrame:contextRef:pageGroupRef:relatedToPage:]),
API::PageConfiguration::m_preferences already existed by the time it was
needed.

>> Source/WebKit/UIProcess/API/mac/WKView.mm:1198
>> +   
configuration->preferences()->setSystemLayoutDirection(static_cast<uint32_t>(to
UserInterfaceLayoutDirection(self.userInterfaceLayoutDirection)));
> 
> static_cast<TextDirection>

Wil fix.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:-2775
>> -	return store;
> 
> Didn't this approach allow a given WebPageProxy to have settings that
differed from the global preferences?
> 
> Is this needed to support some form of per-page preference setting?

No, I don't think it is.

This method simply copies all the values from m_configurationPreferenceValues
back into the m_preferences->store() (and does it _every_ time you call the
method; -[WKWebView _setupPageConfiguration:] stores nearly 50 values in it!),
so after the first time it's called, every page would have the what was in
m_preferences->store() with values in m_configurationPreferenceValues
overriding them.

At best it was a way to override default preference settings with what was
stored in m_configurationPreferenceValues, but using lazy initialization code
with an implicit assumption that clients would _always_ call this:

   
preferencesStore().getBoolValueForKey(WebPreferencesKey::attachmentElementEnabl
edKey())

Instead of this:

    m_preferences->attachmentElementEnabled()

In practice, WebPageProxy::preferencesStore() would just have to be called once
(to copy the values from m_configurationPreferenceValues into
m_preferences->store()), then calling direct methods like
m_preferences->attachmentElementEnabled() would just work after that.  (This is
undesirable because calling direct methods on m_preferences should always work
the first time.)

>> Tools/TestWebKitAPI/Tests/WebKit/mac/GetBackingScaleFactor.mm:71
>> +	WKRetainPtr<WKPageGroupRef> pageGroup =
adoptWK(WKPageGroupCreateWithIdentifier(Util::toWK("GetBackingScaleFactorPageGr
oup").get()));
> 
> auto

Wil fix this one and the line below it.