Closed Bug 1142687 Opened 5 years ago Closed 5 years ago

Show context information for current page in the rooms view

Categories

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

defect
Points:
5

Tracking

(firefox39 fixed)

RESOLVED FIXED
mozilla39
Iteration:
39.2 - 23 Mar
Tracking Status
firefox39 --- fixed

People

(Reporter: jaws, Assigned: jaws)

References

Details

(Whiteboard: [context])

Attachments

(1 file, 6 obsolete files)

This will be hidden behind the loop.contextInConversations.enabled flag which is set to false by default until bug 1142671 is backed out.
Flags: qe-verify+
Flags: firefox-backlog+
Blocks: 1142525
No longer blocks: 1130079
Attached patch WIP patch (obsolete) — Splinter Review
Attachment #8578324 - Flags: feedback?(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.
Attachment #8578324 - Flags: feedback?(standard8) → feedback+
Rank: 5
Priority: -- → P1
Whiteboard: [context]
Attached patch Patch (obsolete) — Splinter Review
I had to relax the CSP policy to allow the images to load from external sites.
Attachment #8578324 - Attachment is obsolete: true
Attachment #8579037 - Flags: review?(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.
(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.
Attached patch Patch v1.1 (obsolete) — Splinter Review
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.
Attachment #8579037 - Attachment is obsolete: true
Attachment #8579037 - Flags: review?(standard8)
Attachment #8579723 - Flags: review?(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.
Attachment #8579723 - Flags: review?(standard8)
Attached patch Patch v1.2 (obsolete) — Splinter Review
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.
Attachment #8579723 - Attachment is obsolete: true
Attachment #8580296 - Flags: review?(standard8)
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.
Attachment #8580636 - Flags: feedback?(jaws)
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.
Attachment #8580296 - Flags: review?(standard8) → review+
Comment on attachment 8580636 [details] [diff] [review]
Possible ui-showcase extension for visibility trigger

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

Mmmmm.... monkey-patching :)
Attachment #8580636 - Flags: feedback?(jaws) → review+
Hi Jaws - any chance you can bump this into the work you're doing to land this early this week? :)
Flags: needinfo?(jaws)
Attached patch Patch v1.3 (obsolete) — Splinter Review
With unit tests for the context information and folded your patch with the ui-showcase fix.
Attachment #8580296 - Attachment is obsolete: true
Attachment #8580636 - Attachment is obsolete: true
Flags: needinfo?(jaws)
Attachment #8582023 - Flags: review?(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.
Attachment #8582023 - Flags: review?(standard8) → review+
We'll need to use the PageMetadata in a content script to fix the e10s failures.
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.
Attachment #8582608 - Flags: review?(standard8) → review+
https://hg.mozilla.org/mozilla-central/rev/d1f32269c3b4
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Depends on: 1169607
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.
Flags: needinfo?(jaws)
This bug does not need to be verified by itself. It will be verified indirectly when the full context-in-conversations feature is tested.
Flags: qe-verify-
Flags: qe-verify+
Flags: needinfo?(jaws)
You need to log in before you can comment on or make changes to this bug.