Commit 25d32f13 authored by Aran Gilman's avatar Aran Gilman Committed by Commit Bot

Remove ReaderModeIconView from observer list where needed.

This should either fix the attached bug or at least help with debugging
by DCHECKing when the icon is destroyed without being removed from all
observer lists.

Bug: 1017341
Change-Id: I8a7457f7f82f3dc4936fb60112742af2a05a7d70
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2006468Reviewed-by: default avatarWei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: default avatarElly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Aran Gilman <gilmanmh@google.com>
Cr-Commit-Position: refs/heads/master@{#734057}
parent e902df34
...@@ -23,6 +23,13 @@ ReaderModeIconView::ReaderModeIconView( ...@@ -23,6 +23,13 @@ ReaderModeIconView::ReaderModeIconView(
icon_label_bubble_delegate, icon_label_bubble_delegate,
page_action_icon_delegate) {} page_action_icon_delegate) {}
ReaderModeIconView::~ReaderModeIconView() {
content::WebContents* contents = web_contents();
if (contents)
dom_distiller::RemoveObserver(contents, this);
DCHECK(!DistillabilityObserver::IsInObserverList());
}
void ReaderModeIconView::DidFinishNavigation( void ReaderModeIconView::DidFinishNavigation(
content::NavigationHandle* navigation_handle) { content::NavigationHandle* navigation_handle) {
if (GetVisible()) if (GetVisible())
...@@ -36,20 +43,29 @@ void ReaderModeIconView::UpdateImpl() { ...@@ -36,20 +43,29 @@ void ReaderModeIconView::UpdateImpl() {
return; return;
} }
// Notify the icon when navigation to and from a distilled page occurs so that
// it can hide the inkdrop.
Observe(contents);
if (IsDistilledPage(contents->GetLastCommittedURL())) { if (IsDistilledPage(contents->GetLastCommittedURL())) {
SetVisible(true); SetVisible(true);
SetActive(true); SetActive(true);
} else { } else {
dom_distiller::AddObserver(contents, this); // If the currently active web contents has changed since last time, stop
// observing the old web contents and start observing the new one.
// (WebContentsObserver::web_contents() is not updated until the call to
// Observe() below, so it should still contain the old contents.)
content::WebContents* old_contents = web_contents();
if (old_contents != contents) {
if (old_contents)
dom_distiller::RemoveObserver(old_contents, this);
dom_distiller::AddObserver(contents, this);
}
base::Optional<dom_distiller::DistillabilityResult> distillability = base::Optional<dom_distiller::DistillabilityResult> distillability =
dom_distiller::GetLatestResult(contents); dom_distiller::GetLatestResult(contents);
SetVisible(distillability && distillability.value().is_distillable); SetVisible(distillability && distillability.value().is_distillable);
SetActive(false); SetActive(false);
} }
// Notify the icon when navigation to and from a distilled page occurs so that
// it can hide the inkdrop.
Observe(contents);
} }
const gfx::VectorIcon& ReaderModeIconView::GetVectorIcon() const { const gfx::VectorIcon& ReaderModeIconView::GetVectorIcon() const {
......
...@@ -28,7 +28,7 @@ class ReaderModeIconView : public PageActionIconView, ...@@ -28,7 +28,7 @@ class ReaderModeIconView : public PageActionIconView,
ReaderModeIconView(CommandUpdater* command_updater, ReaderModeIconView(CommandUpdater* command_updater,
IconLabelBubbleView::Delegate* icon_label_bubble_delegate, IconLabelBubbleView::Delegate* icon_label_bubble_delegate,
PageActionIconView::Delegate* page_action_icon_delegate); PageActionIconView::Delegate* page_action_icon_delegate);
~ReaderModeIconView() override = default; ~ReaderModeIconView() override;
protected: protected:
// Detect when navigation to the distilled page completes. This is required to // Detect when navigation to the distilled page completes. This is required to
......
...@@ -38,6 +38,7 @@ std::ostream& operator<<(std::ostream& os, const DistillabilityResult& result); ...@@ -38,6 +38,7 @@ std::ostream& operator<<(std::ostream& os, const DistillabilityResult& result);
class DistillabilityObserver : public base::CheckedObserver { class DistillabilityObserver : public base::CheckedObserver {
public: public:
virtual void OnResult(const DistillabilityResult& result) = 0; virtual void OnResult(const DistillabilityResult& result) = 0;
~DistillabilityObserver() override = default;
}; };
// Add/remove objects to the list of observers to notify when the distillability // Add/remove objects to the list of observers to notify when the distillability
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment