Closed Bug 1352501 Opened 7 years ago Closed 7 years ago

Remove Reader Mode feature promotion panel

Categories

(Toolkit :: Reader Mode, enhancement, P1)

Product:
Toolkit
Component:
Reader Mode
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla55
Iteration:
55.4 - May 1
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox55 --- verified

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-performance])

Attachments

(1 file)

Bug 1352501 - Remove Reader Mode promotion panel.
59 bytes, text/x-review-board-request
MattN
: review+
Details 
Ehsan is doing a live profiling demo here in Toronto. He's got a profile showing that the call to showReaderModeInfoPanel can cause us to skip a frame:

http://searchfox.org/mozilla-central/rev/72fe012899a1b27d34838ab463ad1ae5b116d76b/browser/modules/ReaderParent.jsm#150-166

Specifically, UITour is creating a menu panel, and is causing multiple style flushes while doing so.

This panel is designed to show up once when the user has reached a document that is parseable by Reader Mode. Then we don't show it again.

We've been shipping it since Firefox 38 (see bug 1134507).

Is it time to remove this thing?
Whiteboard: [qf] → [qf:p1]
ni? to canuckinstani for considering removing this?
Flags: needinfo?(jgriffiths)
(In reply to Mike Conley (:mconley) from comment #1)
> ni? to canuckinstani for considering removing this?

In the short term, I agree as long as usage of the feature is as low as I seem to remember it is. Mike - is there telemetry we can look at just to check?
Flags: needinfo?(jgriffiths) → needinfo?(mconley)
To be clear, I don't mean removing Reader Mode - I mean removing the panel that advertises it on first run for new profiles. I think we should definitely _keep_ the Reader Mode feature.

fwiw, I'm not sure if the READER_MODE_DOWNLOAD_RESULT probe could be used to measure the usage of the actual feature, but that's the only one I see with potential.
Flags: needinfo?(mconley) → needinfo?(jgriffiths)
(In reply to Mike Conley (:mconley) from comment #3)
> To be clear, I don't mean removing Reader Mode - I mean removing the panel
> that advertises it on first run for new profiles. I think we should
> definitely _keep_ the Reader Mode feature.

Yeah totally. We hope to have much better capabilities in the mid-term to market features to users in a contextual way.

> fwiw, I'm not sure if the READER_MODE_DOWNLOAD_RESULT probe could be used to
> measure the usage of the actual feature, but that's the only one I see with
> potential.

Okay - but I think it's safe to remove this feature promotion bit now and revisit feature promotion more holistically later.
Flags: needinfo?(jgriffiths)
Flags: needinfo?(mconley)
Blocks: photon-performance-triage
Flags: needinfo?(mconley)
Summary: Reader Mode info panel can cause us to skip a frame when appearing → Remove Reader Mode feature promotion panel
Whiteboard: [qf:p1] → [qf:p1][photon-performance]
Flags: qe-verify?
Priority: -- → P2
Assignee: nobody → mconley
Status: NEW → ASSIGNED
Iteration: --- → 55.4 - May 1
Flags: qe-verify? → qe-verify+
Priority: P2 → P1
QA Contact: adrian.florinescu
See Also: → 1352518
Comment on attachment 8859730 [details]
Bug 1352501 - Remove Reader Mode promotion panel.

https://reviewboard.mozilla.org/r/131726/#review134684

I reviewed everything but ReaderParent.jsm and browser_readerMode.js. Maybe Jared can review those?
Attachment #8859730 - Flags: review?(MattN+bmo) → review+
Attachment #8859730 - Flags: review?(jaws)
Comment on attachment 8859730 [details]
Bug 1352501 - Remove Reader Mode promotion panel.

https://reviewboard.mozilla.org/r/131726/#review134758

::: commit-message-05c21:4
(Diff revision 1)
> +Bug 1352501 - Don't show Reader Mode promotion panel on first visit to an article. r?MattN
> +
> +The UITour library can still show the panel in the event that we want to
> +promote the feature that way.

Unless there are specific plans for UITour to do this, I think we should completely remove the panel now.
(In reply to Dão Gottwald [::dao] from comment #7)
> Unless there are specific plans for UITour to do this, I think we should
> completely remove the panel now.

MattN suggested agibson as a good person to ask about active UITours. Hey agibson, do you know if any UITour's plan on promoting the Reader Mode feature? Because we're thinking of just tearing out that panel entirely.
Flags: needinfo?(agibson)
(In reply to Mike Conley (:mconley) from comment #8)
> (In reply to Dão Gottwald [::dao] from comment #7)
> > Unless there are specific plans for UITour to do this, I think we should
> > completely remove the panel now.
> 
> MattN suggested agibson as a good person to ask about active UITours. Hey
> agibson, do you know if any UITour's plan on promoting the Reader Mode
> feature? Because we're thinking of just tearing out that panel entirely.

Hi - so i think that panel was only ever triggered in-product, since the Reader Mode tour could initiate from any website and not just a white-listed mozilla web property. We've never promoted Reader Mode via www.mozilla.org, and I don't think we have any plans to do so in the immediate future.

I'm not 100% sure on about:home promoting this feature, so I've needsinfo'd Giorgos who can answer there.
Flags: needinfo?(agibson) → needinfo?(giorgos)
(In reply to Dão Gottwald [::dao] from comment #7)
> ::: commit-message-05c21:4
> (Diff revision 1)
> > +Bug 1352501 - Don't show Reader Mode promotion panel on first visit to an article. r?MattN
> > +
> > +The UITour library can still show the panel in the event that we want to
> > +promote the feature that way.
> 
> Unless there are specific plans for UITour to do this, I think we should
> completely remove the panel now.

It wasn't its own panel, it was using a generic UITour infoPanel so there is no panel to remove. All that the commit message is referring to is the one line entry[1] in the UITour targets that references the ID of the reader mode toggle button.

(In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #4)
> (In reply to Mike Conley (:mconley) from comment #3)
> > To be clear, I don't mean removing Reader Mode - I mean removing the panel
> > that advertises it on first run for new profiles. I think we should
> > definitely _keep_ the Reader Mode feature.
> 
> Yeah totally. We hope to have much better capabilities in the mid-term to
> market features to users in a contextual way.

The above is a plan that may involve UITour and it's literally one line so that's why we should leave it.

[1] https://dxr.mozilla.org/mozilla-central/rev/20dff607fb88ee69135a280bbb7f32df75a86237/browser/components/uitour/UITour.jsm#161
(In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are blocking you) from comment #10)
> (In reply to Dão Gottwald [::dao] from comment #7)
> > ::: commit-message-05c21:4
> > (Diff revision 1)
> > > +Bug 1352501 - Don't show Reader Mode promotion panel on first visit to an article. r?MattN
> > > +
> > > +The UITour library can still show the panel in the event that we want to
> > > +promote the feature that way.
> > 
> > Unless there are specific plans for UITour to do this, I think we should
> > completely remove the panel now.
> 
> It wasn't its own panel, it was using a generic UITour infoPanel so there is
> no panel to remove. All that the commit message is referring to is the one
> line entry[1] in the UITour targets that references the ID of the reader
> mode toggle button.

But there's Reader Mode specific content sitting somewhere and code dealing with the panel in ReaderParent.jsm, and this stuff implies a maintenance cost, right? Whether it uses its own or some generic panel seems like an unimportant implementation detail.

> (In reply to Jeff Griffiths (:canuckistani) (:⚡︎) from comment #4)
> > (In reply to Mike Conley (:mconley) from comment #3)
> > > To be clear, I don't mean removing Reader Mode - I mean removing the panel
> > > that advertises it on first run for new profiles. I think we should
> > > definitely _keep_ the Reader Mode feature.
> > 
> > Yeah totally. We hope to have much better capabilities in the mid-term to
> > market features to users in a contextual way.
> 
> The above is a plan that may involve UITour and it's literally one line so
> that's why we should leave it.

I don't think this qualifies as a plan.
(In reply to Dão Gottwald [::dao] from comment #11)
> (In reply to Matthew N. [:MattN] (behind on bugmail; PM if requests are
> blocking you) from comment #10)
> > (In reply to Dão Gottwald [::dao] from comment #7)
> > > ::: commit-message-05c21:4
> > > (Diff revision 1)
> > > > +Bug 1352501 - Don't show Reader Mode promotion panel on first visit to an article. r?MattN
> > > > +
> > > > +The UITour library can still show the panel in the event that we want to
> > > > +promote the feature that way.
> > > 
> > > Unless there are specific plans for UITour to do this, I think we should
> > > completely remove the panel now.
> > 
> > It wasn't its own panel, it was using a generic UITour infoPanel so there is
> > no panel to remove. All that the commit message is referring to is the one
> > line entry[1] in the UITour targets that references the ID of the reader
> > mode toggle button.
> 
> But there's Reader Mode specific content sitting somewhere and code dealing
> with the panel in ReaderParent.jsm, and this stuff implies a maintenance
> cost, right? Whether it uses its own or some generic panel seems like an
> unimportant implementation detail.

Oh, I guess you're talking about showReaderModeInfoPanel() which is different than what the commit message was talking about since UITour doesn't use showReaderModeInfoPanel. I don't have ownership over or strong opinions on removing showReaderModeInfoPanel from ReaderParent.jsm as that won't prevent UITours from showing their own panel since the strings would come from the hosted tour JS.
Flags: needinfo?(giorgos)
Comment on attachment 8859730 [details]
Bug 1352501 - Remove Reader Mode promotion panel.

https://reviewboard.mozilla.org/r/131726/#review134998

I think we should remove the UITour _readerModeInfoPanelOpen-related code. Since we don't have plans to promote it anytime soon then it's basically dead code that we're maintaining for no benefit.
Attachment #8859730 - Flags: review?(jaws) → review+
Comment on attachment 8859730 [details]
Bug 1352501 - Remove Reader Mode promotion panel.

This is different enough from the original that it probably requires re-review.
Attachment #8859730 - Flags: review+ → review?(MattN+bmo)
(In reply to Alex Gibson [:agibson] from comment #9)
> I'm not 100% sure on about:home promoting this feature, so I've needsinfo'd
> Giorgos who can answer there.

We are not promoting reader with snippets either. Thanks for the heads up Alex
Comment on attachment 8859730 [details]
Bug 1352501 - Remove Reader Mode promotion panel.

https://reviewboard.mozilla.org/r/131726/#review136538
Attachment #8859730 - Flags: review?(MattN+bmo) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3d616c30f104
Remove Reader Mode promotion panel. r=jaws,MattN
https://hg.mozilla.org/mozilla-central/rev/3d616c30f104
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Tested this on the latest Nightly 55.0a1 build (2017-05-16) across platforms: Windows 10 x64, Ubuntu 16.04 LTS x64 and Mac OS X 10.12.5.

The Reader Mode promotion panel is not displayed anymore when first running new profiles.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: qe-verify+
Performance Impact: --- → P1
Whiteboard: [qf:p1][photon-performance] → [photon-performance]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: