1158880 - Separate out the Pocket UI to an imported XHTML file and scoped stylesheets

Status: RESOLVED INVALID

(Firefox :: Pocket, defect)

Description

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

We want to move the Pocket UI out to a #include'd XHTML file, use scoped stylesheets for it, and also move as much logic as possible out of CustomizableWidgets.jsm and in to Pocket.jsm.

Comment 1

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

Attachment: Patch

Comment 2

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

Attachment: Patch

Comment 3

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

Attachment: Patch

Comment 4

[Florian Quèze [:florian]]

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

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

::: browser/components/customizableui/CustomizableWidgets.jsm
@@ +1087,5 @@
>        tooltiptext: PocketBundle.GetStringFromName("pocket-button.tooltiptext"),
> +      onCreated: Pocket.onWidgetNodeCreated.bind(this),
> +      onViewShowing: Pocket.onPanelViewShowing.bind(this),
> +      onViewHiding: Pocket.onPanelViewHiding.bind(this),
> +      handleEvent: Pocket.handleEvent.bind(this),

onPanelViewShowing, onPanelViewHiding and handleEvent don't seem to use the 'this' value, so I think we should drop the .bind calls.

@@ +1092,2 @@
>  
>        USER_REMOVED_PREF: "browser.pocket.removedByUser",

Could we just move this to a const at the top of Pocket.jsm?

::: browser/components/pocket/Pocket.jsm
@@ +158,5 @@
> +
> +    let addTagsField = doc.getElementById("pocket-page-tags-field");
> +    let addTagsButton = doc.getElementById("pocket-page-tags-add");
> +    addTagsField.addEventListener("input", this);
> +    addTagsButton.addEventListener("command", this);

Could these 2 addEventListener calls just set the Pocket object as the listener? Then we can remove the handleEvent: Pocket.handleEvent.bind(this) indirection this patch has in CustomizableWidgets.jsm.

::: browser/components/pocket/jar.mn
@@ +6,5 @@
>    content/browser/pocket/img/signup_logo.png     (img/signup_logo.png)
>    content/browser/pocket/img/[e-mail]  (img/[e-mail])
>    content/browser/pocket/img/signup_or.png       (img/signup_or.png)
>    content/browser/pocket/img/[e-mail]    (img/[e-mail])
> +  content/browser/pocket/panel.css               (panel.css)

Is this intentionally in content, rather than in browser/themes/shared?

Comment 5

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)

> ::: browser/components/pocket/Pocket.jsm
> @@ +158,5 @@
> > +
> > +    let addTagsField = doc.getElementById("pocket-page-tags-field");
> > +    let addTagsButton = doc.getElementById("pocket-page-tags-add");
> > +    addTagsField.addEventListener("input", this);
> > +    addTagsButton.addEventListener("command", this);
> 
> Could these 2 addEventListener calls just set the Pocket object as the
> listener? Then we can remove the handleEvent: Pocket.handleEvent.bind(this)
> indirection this patch has in CustomizableWidgets.jsm.

With the patch applied I get this error in the terminal whenever I type a character in the tag input field:
JavaScript error: , line 0: TypeError: Property 'handleEvent' is not callable.

Comment 6

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

Attachment: Patch v2

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8598888 [details] [diff] [review]
Patch v2

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

Looks good from code inspection, I haven't verified that the issue in comment 5 is fixed.

Comment 8

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

(In reply to Florian Quèze [:florian] [:flo] from comment #7)
> Comment on attachment 8598888 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8598888 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good from code inspection, I haven't verified that the issue in
> comment 5 is fixed.

I verified it was fixed before uploading the patch :)

Comment 9

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

This bug is closed in favor of bug 1159744.