Skip to content

Commit

Permalink
Revert "Make Starter a WebContentUserData"
Browse files Browse the repository at this point in the history
This reverts commit 3915e20.

Reason for revert: this is causing crashes

Original change's description:
> Make Starter a WebContentUserData
>
> Before this change, the starter was owned by the Android platform
> delegate. In order to support other platforms, the starter is
> now a WebContentUserData and will be created on tab creation.
> This CL adds the creation for android only, a follow-up will
> add the same for other platforms.
>
> Bug: b:201964911
>
>
> Change-Id: I7fea6d3d47ea09502530f25daca891e0c09ab86c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3528555
> Reviewed-by: Clemens Arbesser <arbesser@google.com>
> Commit-Queue: Luca Hunkeler <hluca@google.com>
> Cr-Commit-Position: refs/heads/main@{#987368}

Bug: b:201964911
Change-Id: I17c8f30e422665c9950a1609039de827e0fa875c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3562918
Reviewed-by: Clemens Arbesser <arbesser@google.com>
Commit-Queue: Luca Hunkeler <hluca@google.com>
Cr-Commit-Position: refs/heads/main@{#987405}
  • Loading branch information
Luca Hunkeler authored and Chromium LUCI CQ committed Mar 31, 2022
1 parent 28f4438 commit 672f021
Show file tree
Hide file tree
Showing 8 changed files with 34 additions and 73 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,6 @@ static jlong JNI_Starter_FromWebContents(
std::move(dependencies));
auto* tab_helper_android =
StarterDelegateAndroid::FromWebContents(web_contents);
Starter::CreateForWebContents(
web_contents, tab_helper_android, ukm::UkmRecorder::Get(),
RuntimeManagerImpl::GetForWebContents(web_contents)->GetWeakPtr(),
base::DefaultTickClock::GetInstance());
return reinterpret_cast<intptr_t>(tab_helper_android);
}

Expand All @@ -71,19 +67,17 @@ void StarterDelegateAndroid::Attach(JNIEnv* env,
Detach(env, jcaller);
java_object_ = base::android::ScopedJavaGlobalRef<jobject>(jcaller);

starter_ = Starter::FromWebContents(&GetWebContents());
DCHECK(starter_);
starter_->Init();
starter_ = std::make_unique<Starter>(
&GetWebContents(), this, ukm::UkmRecorder::Get(),
RuntimeManagerImpl::GetForWebContents(&GetWebContents())->GetWeakPtr(),
base::DefaultTickClock::GetInstance());
}

void StarterDelegateAndroid::Detach(JNIEnv* env,
const JavaParamRef<jobject>& jcaller) {
java_object_ = nullptr;
java_dependencies_ = nullptr;
if (starter_) {
starter_->Init();
}
starter_ = nullptr;
starter_.reset();
}

std::unique_ptr<TriggerScriptCoordinator::UiDelegate>
Expand Down Expand Up @@ -341,10 +335,6 @@ StarterDelegateAndroid::CreateFieldTrialUtil() {
return dependencies_->CreateFieldTrialUtil();
}

bool StarterDelegateAndroid::IsAttached() {
return !!java_object_;
}

WEB_CONTENTS_USER_DATA_KEY_IMPL(StarterDelegateAndroid);

} // namespace autofill_assistant
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ class StarterDelegateAndroid
bool GetIsCustomTab() const override;
bool GetIsTabCreatedByGSA() const override;
std::unique_ptr<AssistantFieldTrialUtil> CreateFieldTrialUtil() override;
bool IsAttached() override;

// Called by Java to start an autofill-assistant flow for an incoming intent.
void Start(
Expand Down Expand Up @@ -125,7 +124,7 @@ class StarterDelegateAndroid
void CreateJavaDependenciesIfNecessary();

WEB_CONTENTS_USER_DATA_KEY_DECL();
raw_ptr<Starter> starter_;
std::unique_ptr<Starter> starter_;
// Contains AssistantStaticDependencies which do not change.
const std::unique_ptr<const Dependencies> dependencies_;
// Can change based on activity attachment.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,8 +119,4 @@ FakeStarterPlatformDelegate::CreateFieldTrialUtil() {
return std::make_unique<MockAssistantFieldTrialUtil>();
}

bool FakeStarterPlatformDelegate::IsAttached() {
return is_attached_;
}

} // namespace autofill_assistant
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ class FakeStarterPlatformDelegate : public StarterPlatformDelegate {
bool GetIsCustomTab() const override;
bool GetIsTabCreatedByGSA() const override;
std::unique_ptr<AssistantFieldTrialUtil> CreateFieldTrialUtil() override;
bool IsAttached() override;

// Intentionally public to give tests direct access.
std::unique_ptr<TriggerScriptCoordinator::UiDelegate>
Expand All @@ -75,7 +74,6 @@ class FakeStarterPlatformDelegate : public StarterPlatformDelegate {
bool is_custom_tab_ = true;
bool is_tab_created_by_gsa_ = true;
std::unique_ptr<AssistantFieldTrialUtil> field_trial_util_;
bool is_attached_ = true;

base::OnceCallback<void(
GURL url,
Expand Down
21 changes: 6 additions & 15 deletions components/autofill_assistant/browser/starter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,6 @@ Starter::Starter(content::WebContents* web_contents,
base::WeakPtr<RuntimeManager> runtime_manager,
const base::TickClock* tick_clock)
: content::WebContentsObserver(web_contents),
content::WebContentsUserData<Starter>(*web_contents),
current_ukm_source_id_(
ukm::GetSourceIdForWebContentsDocument(web_contents)),
cached_failed_trigger_script_fetches_(
Expand All @@ -192,7 +191,9 @@ Starter::Starter(content::WebContents* web_contents,
ukm_recorder_(ukm_recorder),
runtime_manager_(runtime_manager),
starter_heuristic_(GetOrCreateStarterHeuristic()),
tick_clock_(tick_clock) {}
tick_clock_(tick_clock) {
CheckSettings();
}

Starter::~Starter() = default;

Expand Down Expand Up @@ -360,18 +361,13 @@ void Starter::RegisterSyntheticFieldTrials(
}

void Starter::OnTabInteractabilityChanged(bool is_interactable) {
Init();
CheckSettings();
if (trigger_script_coordinator_) {
trigger_script_coordinator_->OnTabInteractabilityChanged(is_interactable);
}
}

void Starter::Init() {
if (!platform_delegate_->IsAttached()) {
CancelPendingStartup(Metrics::TriggerScriptFinishedState::CANCELED);
return;
}

void Starter::CheckSettings() {
bool prev_is_custom_tab = is_custom_tab_;
is_custom_tab_ = platform_delegate_->GetIsCustomTab();
bool switched_from_cct_to_tab = prev_is_custom_tab && !is_custom_tab_;
Expand Down Expand Up @@ -441,7 +437,7 @@ void Starter::RecordDependenciesInvalidated() const {

void Starter::OnDependenciesInvalidated() {
RecordDependenciesInvalidated();
Init();
CheckSettings();
}

void Starter::Start(std::unique_ptr<TriggerContext> trigger_context) {
Expand All @@ -453,9 +449,6 @@ void Starter::Start(std::unique_ptr<TriggerContext> trigger_context) {

CancelPendingStartup(Metrics::TriggerScriptFinishedState::CANCELED);
pending_trigger_context_ = std::move(trigger_context);
if (!platform_delegate_->IsAttached()) {
OnStartDone(/* start_regular_script = */ false);
}

if (base::CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
switches::kAutofillAssistantForceOnboarding) == "true") {
Expand Down Expand Up @@ -816,6 +809,4 @@ void Starter::DeleteTriggerScriptCoordinator() {
trigger_script_coordinator_.reset();
}

WEB_CONTENTS_USER_DATA_KEY_IMPL(Starter);

} // namespace autofill_assistant
9 changes: 2 additions & 7 deletions components/autofill_assistant/browser/starter.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,16 +23,14 @@
#include "components/autofill_assistant/browser/startup_util.h"
#include "components/autofill_assistant/browser/trigger_scripts/trigger_script_coordinator.h"
#include "content/public/browser/web_contents_observer.h"
#include "content/public/browser/web_contents_user_data.h"
#include "services/metrics/public/cpp/ukm_recorder.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

namespace autofill_assistant {

// Starts autofill-assistant flows. Uses a platform delegate to show UI and
// access platform-dependent features.
class Starter : public content::WebContentsObserver,
public content::WebContentsUserData<Starter> {
class Starter : public content::WebContentsObserver {
public:
explicit Starter(content::WebContents* web_contents,
StarterPlatformDelegate* platform_delegate,
Expand Down Expand Up @@ -66,15 +64,14 @@ class Starter : public content::WebContentsObserver,

// Re-check settings. This may cancel ongoing startup requests if the required
// settings are no longer enabled.
void Init();
void CheckSettings();

// Records the invalidation of platform-specific depencendies. For example:
// When the activity is changed on Android.
void OnDependenciesInvalidated();

private:
friend class StarterTest;
friend class content::WebContentsUserData<Starter>;

// Starts a flow for |url| if possible. Will fail (do nothing) if the feature
// is disabled or if there is already a pending startup.
Expand Down Expand Up @@ -148,8 +145,6 @@ class Starter : public content::WebContentsObserver,
void RegisterSyntheticFieldTrials(
const TriggerContext& trigger_context) const;

WEB_CONTENTS_USER_DATA_KEY_DECL();

// The UKM source id to use for UKM metrics.
ukm::SourceId current_ukm_source_id_ = ukm::kInvalidSourceId;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,6 @@ class StarterPlatformDelegate {
virtual bool GetIsTabCreatedByGSA() const = 0;
// Creates the util for registering synthetic field trials.
virtual std::unique_ptr<AssistantFieldTrialUtil> CreateFieldTrialUtil() = 0;
// Whether the starter platform delegate is currently attached.
// The starter platform delegate should only be interacted with while attached
// as it might not be able to perform its functions while detached.
virtual bool IsAttached() = 0;
};

} // namespace autofill_assistant
Expand Down

0 comments on commit 672f021

Please sign in to comment.