Skip to content

Commit

Permalink
Clean up InternalSettings and ensure all settings are saved/restored
Browse files Browse the repository at this point in the history
1. InternalSettingsGenerated now saves/restores all settings,
   including those don't have an automatically generated setter method
   in the class, to avoid error-prone custom save/restore in
   InternalSettings and ensure all changed settings, either changed
   by the generated setter methods or custom methods in
   InternalSettings, are saved and restored.
2. Also save/restore generic font family settings.
3. Because the page is never null (it's passed as a reference, and
   InternalSettings keep a reference to it in Member<Page>), the
   exception on null page never happens. The check and exception code
   are removed.
4. Unused and duplicate methods are removed.
5. Other simplifications and cleanups.

This addresses the recent regression about PerferCompositingToLCDText
caused by crrev.com/c/4337728. It may also fix some existing flaky
tests.

Bug: 1427271
Change-Id: Ifdc3265340a024bdfa896fd4f2d7229af9050254
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4368701
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Stefan Zager <szager@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1122468}
  • Loading branch information
wangxianzhu authored and Chromium LUCI CQ committed Mar 27, 2023
1 parent e241d4a commit 2e9c1d9
Show file tree
Hide file tree
Showing 7 changed files with 142 additions and 391 deletions.
Expand Up @@ -10,31 +10,38 @@

namespace blink {

InternalSettingsGenerated::InternalSettingsGenerated(Page* page)
: page_(page)
{% for setting in settings if setting.type|to_idl_type %}
, {{setting.name}}_(page->GetSettings().Get{{setting.name.to_upper_camel_case()}}())
{% endfor %}
{
InternalSettingsGenerated::InternalSettingsGenerated(Page& page)
: InternalSettingsPageSupplementBase(page) {
{% for setting in settings %}
backup_.Set{{setting.name.to_upper_camel_case()}}(
GetSettings().Get{{setting.name.to_upper_camel_case()}}());
{% endfor %}
}

InternalSettingsGenerated::~InternalSettingsGenerated() {}

void InternalSettingsGenerated::resetToConsistentState() {
{% for setting in settings if setting.type|to_idl_type %}
page_->GetSettings().Set{{setting.name.to_upper_camel_case()}}({{setting.name}}_);
{% endfor %}
void InternalSettingsGenerated::ResetToConsistentState() {
{% for setting in settings %}
GetSettings().Set{{setting.name.to_upper_camel_case()}}(
backup_.Get{{setting.name.to_upper_camel_case()}}());
{% endfor %}
}
{% for setting in settings if setting.type|to_idl_type %}

void InternalSettingsGenerated::set{{setting.name.to_upper_camel_case()}}({{setting.type|to_passing_type}} {{setting.name}}) {
page_->GetSettings().Set{{setting.name.to_upper_camel_case()}}({{setting.name}});
void InternalSettingsGenerated::set{{setting.name.to_upper_camel_case()}}(
{{setting.type|to_passing_type}} {{setting.name}}) {
GetSettings().Set{{setting.name.to_upper_camel_case()}}({{setting.name}});
}
{% endfor %}

void InternalSettingsGenerated::Trace(Visitor* visitor) const {
visitor->Trace(page_);
ScriptWrappable::Trace(visitor);
InternalSettingsPageSupplementBase::Trace(visitor);
}

Settings& InternalSettingsGenerated::GetSettings() {
CHECK(GetSupplementable());
return GetSupplementable()->GetSettings();
}

} // namespace blink
Expand Up @@ -7,6 +7,8 @@
#define {{header_guard}}

#include "base/memory/scoped_refptr.h"
#include "third_party/blink/renderer/core/frame/settings.h"
#include "third_party/blink/renderer/core/page/page.h"
#include "third_party/blink/renderer/platform/bindings/script_wrappable.h"
#include "third_party/blink/renderer/platform/heap/garbage_collected.h"
#include "third_party/blink/renderer/platform/heap/member.h"
Expand All @@ -15,27 +17,31 @@

namespace blink {

class Page;

class InternalSettingsGenerated : public ScriptWrappable {
class InternalSettingsGenerated : public ScriptWrappable,
public InternalSettingsPageSupplementBase {
DEFINE_WRAPPERTYPEINFO();

public:
explicit InternalSettingsGenerated(Page*);
explicit InternalSettingsGenerated(Page&);
virtual ~InternalSettingsGenerated();
void resetToConsistentState();

{% for setting in settings if setting.type|to_idl_type %}
void set{{setting.name.to_upper_camel_case()}}({{setting.type|to_passing_type}} {{setting.name}});
{% endfor %}

void Trace(Visitor*) const override;

private:
Member<Page> page_;
protected:
Settings& GetSettings();

{% for setting in settings if setting.type|to_idl_type %}
{{setting.type}} {{setting.name}}_;
{% endfor %}
// Resets all settings to be the initial state when this is constructed.
// This also includes settings without an idl type (thus without an
// automatically generated setter method in this class), in case the settings
// are changed by custom methods defined in InternalSettings.
void ResetToConsistentState();

private:
Settings backup_;
};

} // namespace blink
Expand Down
3 changes: 3 additions & 0 deletions third_party/blink/renderer/core/frame/settings.h
Expand Up @@ -56,6 +56,9 @@ class CORE_EXPORT Settings {

public:
Settings();

// Default copy and assignment are forbidden because SettingsDelegate only
// supports 1:1 relationship with Settings.
Settings(const Settings&) = delete;
Settings& operator=(const Settings&) = delete;

Expand Down

0 comments on commit 2e9c1d9

Please sign in to comment.