Closed Bug 1158880 Opened 4 years ago Closed 4 years ago

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

Categories

(Firefox :: Pocket, defect)

defect
Not set
Points:
3

Tracking

()

RESOLVED INVALID
Tracking Status
firefox40 --- affected

People

(Reporter: jaws, Assigned: jaws)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch Patch (obsolete) — Splinter Review
Attachment #8598104 - Flags: review?(florian)
Iteration: --- → 40.2 - 27 Apr
Flags: qe-verify?
Flags: firefox-backlog+
Points: --- → 3
Flags: qe-verify? → qe-verify-
Attached patch Patch (obsolete) — Splinter Review
Attachment #8598104 - Attachment is obsolete: true
Attachment #8598104 - Flags: review?(florian)
Attachment #8598159 - Flags: review?(florian)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8598159 - Attachment is obsolete: true
Attachment #8598159 - Flags: review?(florian)
Attachment #8598172 - Flags: review?(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/signup_logo@2x.png  (img/signup_logo@2x.png)
>    content/browser/pocket/img/signup_or.png       (img/signup_or.png)
>    content/browser/pocket/img/signup_or@2x.png    (img/signup_or@2x.png)
> +  content/browser/pocket/panel.css               (panel.css)

Is this intentionally in content, rather than in browser/themes/shared?
Attachment #8598172 - Flags: review?(florian) → feedback+
(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.
Iteration: 40.2 - 27 Apr → 40.3 - 11 May
Attached patch Patch v2Splinter Review
Attachment #8598172 - Attachment is obsolete: true
Attachment #8598888 - Flags: review?(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.
Attachment #8598888 - Flags: review?(florian) → review+
(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 :)
This bug is closed in favor of bug 1159744.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → INVALID
Iteration: 40.3 - 11 May → ---
You need to log in before you can comment on or make changes to this bug.