1142687 - Show context information for current page in the rooms view

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Jared Wein [:jaws] (please needinfo? me)]

This will be hidden behind the loop.contextInConversations.enabled flag which is set to false by default until bug 1142671 is backed out.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: WIP patch

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 8578324 [details] [diff] [review]
WIP patch

Review of attachment 8578324 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/content/js/panel.jsx
@@ +700,5 @@
> +      };
> +    },
> +
> +    onDocumentVisible: function() {
> +      let metadata = this.props.mozLoop.getSelectedTabMetadata();

If this ever changed to async, then I'd propose moving this data out to a store, as otherwise we'll get too much business logic here - but as it is currently, I think it is fine like this.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

I had to relax the CSP policy to allow the images to load from external sites.

Comment 4

[Mark Banner (:standard8)]

Comment on attachment 8579037 [details] [diff] [review]
Patch

Review of attachment 8579037 [details] [diff] [review]:
-----------------------------------------------------------------

I've not looked at the css yet, but here's some comments I've got so far.

::: browser/components/loop/MozLoopAPI.jsm
@@ +838,5 @@
>          return "https://www.gravatar.com/avatar/" + md5Email + ".jpg?default=blank&s=" + size;
>        }
>      },
>  
> +    getSelectedTabMetadata: {

Please add jsdoc for this.

::: browser/components/loop/content/js/panel.jsx
@@ +682,5 @@
> +
> +  /**
> +   * Context info that is offered to be part of a Room.
> +   */
> +  var ContextInfo = React.createClass({

Please update the ui-showcase to show this view.

@@ +700,5 @@
> +
> +    onDocumentVisible: function() {
> +      var metadata = this.props.mozLoop.getSelectedTabMetadata();
> +      var previewImage = metadata.previews.length ? metadata.previews[0] : "";
> +      var description = metadata.description || metadata.title;

I would suggest adding a small couple of unit tests here to validate that we're doing the right things with the meta data if fields aren't present etc.

@@ +723,5 @@
> +          <label className="context_enabled">
> +            <input type="checkbox"/>
> +            {mozL10n.get("context_offer_label")}
> +          </label>
> +          <img className="context_preview" src={this.state.previewImage}/>

I'm not seeing this image on the pages I'm looking at. Is there something I'm missing? How does this get generated?

@@ +724,5 @@
> +            <input type="checkbox"/>
> +            {mozL10n.get("context_offer_label")}
> +          </label>
> +          <img className="context_preview" src={this.state.previewImage}/>
> +          <span className="context_description">{this.state.description}</span>

It looks like we normally use dashes rather than underscores for our class names, so I think we should continue that here.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Mark Banner (:standard8) from comment #4)
> > +          <label className="context_enabled">
> > +            <input type="checkbox"/>
> > +            {mozL10n.get("context_offer_label")}
> > +          </label>
> > +          <img className="context_preview" src={this.state.previewImage}/>
> 
> I'm not seeing this image on the pages I'm looking at. Is there something
> I'm missing? How does this get generated?

The previewImage state gets pulled in through the PageMetadata.jsm module. You will need to rebuild browser/app to update the CSP policy to allow the image to be loaded since it is likely on a third-party server. I used an article from nytimes.com to test that the image loaded properly, bug Bugzilla also works.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.1

For the ui-showcase, how can I mock mozLoop so that I can fake the response of mozLoop.getLoopPref("contextInConverations.enabled")?

This way we can showcase the RoomsList with both the context-in-conversations enabled and disabled.

Comment 7

[Mark Banner (:standard8)]

Comment on attachment 8579723 [details] [diff] [review]
Patch v1.1

Review of attachment 8579723 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, I guess I hadn't compiled it fully last time.

For the ui-showcase, you should be able to modify browser/components/loop/ui/fake-mozLoop.js to return a value. I don't think we need to display the "false" case as we're going to be moving forward with this.

::: browser/components/loop/MozLoopAPI.jsm
@@ +842,5 @@
>      /**
> +     * Gets a metadata object that is related to the currently selected tab in
> +     * the most recent window.
> +     *
> +     * @return {object} An object containing information scraped from the page.

nit: missing '*/'

::: browser/components/loop/content/js/panel.jsx
@@ +593,5 @@
>    var RoomList = React.createClass({
>      mixins: [Backbone.Events, sharedMixins.WindowCloseMixin],
>  
>      propTypes: {
> +      mozLoop: React.PropTypes.object,

This should be an .isRequired

@@ +684,5 @@
> +   * Context info that is offered to be part of a Room.
> +   */
> +  var ContextInfo = React.createClass({
> +    propTypes: {
> +      mozLoop: React.PropTypes.object,

This should be an .isRequired

@@ +702,5 @@
> +      var metadata = this.props.mozLoop.getSelectedTabMetadata();
> +      var previewImage = metadata.previews.length ? metadata.previews[0] : "";
> +      var description = metadata.description || metadata.title;
> +      var url = metadata.url;
> +      this.setState({previewImage: previewImage,

I think if there's no url (e.g. an about: page) then we shouldn't display the checkbox and information.

Currently it seems like some part of PageMetadata.getData() fails for getting information from about: pages, so we might want to look into that error as well.

I don't mind if handling those pages nicely are moved out to a separate bug.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.2

I updated the ui-showcase now, but since the context information is only displayed after onDocumentVisible, you have to load the ui-showcase, change tabs, then come back to it to get the state filled in correctly from the fake mozLoop.

Comment 9

[Mark Banner (:standard8)]

Attachment: Possible ui-showcase extension for visibility trigger

I did a bit of hacking on the showcase and came up with this. Basically, it saves the visibility listeners, and then triggers them after everything has rendered.

I'm quite happy for you to include this in the main patch if you think its good.

Comment 10

[Mark Banner (:standard8)]

Comment on attachment 8580296 [details] [diff] [review]
Patch v1.2

Review of attachment 8580296 [details] [diff] [review]:
-----------------------------------------------------------------

r=Standard8 with the requirement that we add a unit test (either separate patch or update this patch) before this lands.

I think the test case wants to be for display of context information when supplied, and no display if there's a title/description but no url.

::: browser/components/loop/content/css/panel.css
@@ +230,5 @@
>    padding: .5rem 1rem;
> +  border-radius: 0 0 3px 3px;
> +}
> +
> +/* Remove when bug 1142671 is backed out. */

I think this comment is now obsolete - for when we hide it for pages without urls etc.

@@ +236,1 @@
>    border-radius: 3px;

I think this needs a bit of marge/padding above the button for the not(.context) case. Otherwise its right on the bottom of the room list at the moment.

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8580636 [details] [diff] [review]
Possible ui-showcase extension for visibility trigger

Review of attachment 8580636 [details] [diff] [review]:
-----------------------------------------------------------------

Mmmmm.... monkey-patching :)

Comment 12

[:shell escalante]

Hi Jaws - any chance you can bump this into the work you're doing to land this early this week? :)

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v1.3

With unit tests for the context information and folded your patch with the ui-showcase fix.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 8582023 [details] [diff] [review]
Patch v1.3

Review of attachment 8582023 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just one minor comment to address.

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +651,5 @@
>          React.createElement(loop.panel.RoomList, {
>            store: roomStore,
>            dispatcher: dispatcher,
> +          userDisplayName: fakeEmail,
> +          mozLoop: navigator.mozLoop

Please make these fakeMozLoop rather than navigator.mozLoop. We should be moving away from navigator.mozLoop generally.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/b192e6e16c1b

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-e10s failures.
https://hg.mozilla.org/integration/fx-team/rev/6e463e53546b

https://treeherder.mozilla.org/logviewer.html#?job_id=2386003&repo=fx-team

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

We'll need to use the PageMetadata in a content script to fix the e10s failures.

Comment 18

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch using messageManager

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #18)
> Created attachment 8582608 [details] [diff] [review]
> Patch using messageManager

https://bugzilla.mozilla.org/attachment.cgi?oldid=8582023&action=interdiff&newid=8582608&headers=1 shows the interdiff.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 8582608 [details] [diff] [review]
Patch using messageManager

Review of attachment 8582608 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, just a couple of minor nits to change.

::: browser/components/loop/MozLoopAPI.jsm
@@ +849,5 @@
>      /**
> +     * Gets a metadata object that is related to the currently selected tab in
> +     * the most recent window.
> +     *
> +     * @return {object} An object containing information scraped from the page.

The jsdoc needs updating here now for the callback.

::: browser/components/loop/ui/fake-mozLoop.js
@@ +128,5 @@
>      return "http://www.gravatar.com/avatar/" + (Math.ceil(Math.random() * 3) === 2 ?
>        "0a996f0fe2727ef1668bdb11897e4459" : "foo") + ".jpg?default=blank&s=40";
>    },
> +  getSelectedTabMetadata: function() {
> +    return {

This needs updating to fake the callback.

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

Re-landed, https://hg.mozilla.org/integration/fx-team/rev/d1f32269c3b4

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d1f32269c3b4

Comment 23

[Ada [:adalucinet]]

Jared, based on qe-verify + flag, is there any way to verify this fix on 39 branch? Please note Mark's comment via bug 1169607 comment 1.

Comment 24

[Jared Wein [:jaws] (please needinfo? me)]

This bug does not need to be verified by itself. It will be verified indirectly when the full context-in-conversations feature is tested.