Closed Bug 960737 Opened 6 years ago Closed 6 years ago

Remove inline script / style in browser/devtools/fontinspector/font-inspector.xhtml

Categories

(DevTools :: Inspector, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: bgrins, Assigned: k)

References

(Blocks 1 open bug)

Details

(Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js][qa-])

Attachments

(1 file, 1 obsolete file)

Let's move any inline JavaScript, styles, and event handlers out of this file: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/fontinspector/font-inspector.xhtml
Adding the listener to #showall in font-inspector.js?

Is it normal that showAll() fails in https://mxr.mozilla.org/mozilla-central/source/browser/devtools/fontinspector/font-inspector.js#199 sometimes because node.ownerDocument seems to be not set?
Flags: needinfo?(bgrinstead)
(In reply to k from comment #1)
> Adding the listener to #showall in font-inspector.js?

Yes, exactly.  this.chromeDoc.querySelector("#showall") should grab the element.  We should also remove the event listener on destroy.  And in init, we should do this.showAll = this.showAll.bind(this) to make sure the context will be correct in the event listener.

> Is it normal that showAll() fails in
> https://mxr.mozilla.org/mozilla-central/source/browser/devtools/
> fontinspector/font-inspector.js#199 sometimes because node.ownerDocument
> seems to be not set?

Not that I know of, but I doubt that changing how the event handler is bound would affect that.  Have you bumped into this problem?
Flags: needinfo?(bgrinstead)
Can you assign me into this bug?
(In reply to Malintha Fernando from comment #3)
> Can you assign me into this bug?

Malintha, thanks for checking.  Kay, were you already working on this?  If not, then we will assign Malintha to this bug.  Otherwise, I will create a new bug for another file that needs the same change and link to it.
Flags: needinfo?(k)
Yes, I was working on it, just found this strange behavior with the node.ownerDocument being not set, but I will check this tomorrow.
Flags: needinfo?(k)
Malintha, take a look at Bug 961531 for a similar one that you can tackle.  If you leave a comment over there I will assign it to you.
Assignee: nobody → k
I get this error when clicking on "See all fonts used in this page":

JavaScript error: chrome://browser/content/devtools/fontinspector/font-inspector.js, line 199: contentDocument is undefined

Tried on google.de and facebook.com
Status: NEW → ASSIGNED
Flags: needinfo?(bgrinstead)
(In reply to k from comment #7)
> I get this error when clicking on "See all fonts used in this page":
> 
> JavaScript error:
> chrome://browser/content/devtools/fontinspector/font-inspector.js, line 199:
> contentDocument is undefined
> 
> Tried on google.de and facebook.com

Yes, I don't think this is related to your changes (I'm seeing it with no patches applied).  I've filed Bug 962085 to fix this.

I'd say you can proceed on this bug without worrying about this error.
Flags: needinfo?(bgrinstead)
Attached patch bug-960737.patch (obsolete) — Splinter Review
Okay.
Attachment #8362993 - Flags: review?(bgrinstead)
Comment on attachment 8362993 [details] [diff] [review]
bug-960737.patch

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

::: browser/devtools/fontinspector/font-inspector.js
@@ +207,5 @@
>  
>  window.setPanel = function(panel) {
>    window.fontInspector = new FontInspector(panel, window);
> +
> +  let button = document.getElementById("showall");

Can you move this logic into FontInspector.init and remove the handler in FontInspector.destroy?  This way it will be more consistent with the way the some of the other sidebar tabs get opened and closed.

In order to get this to work, you could set a couple of extra properties on init:

this.showAll = this.showAll.bind(this);
this.showAllButton = this.chromeDoc.getElementById("showall");
Attachment #8362993 - Flags: review?(bgrinstead)
I always solved those |this| problems with closures. The solution with bind seems much better.

thanks for the tip :)
Attachment #8362993 - Attachment is obsolete: true
Attachment #8363614 - Flags: review?(bgrinstead)
Comment on attachment 8363614 [details] [diff] [review]
clearedHtmlFromCssAndJs.patch

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

Code changes look good to me, I've pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=1acbde985a24
Attachment #8363614 - Flags: review?(bgrinstead) → review+
(In reply to k from comment #11)
> Created attachment 8363614 [details] [diff] [review]
> clearedHtmlFromCssAndJs.patch
> 
> I always solved those |this| problems with closures. The solution with bind
> seems much better.
> 
> thanks for the tip :)

No problem, and thank you for taking on these bugs!
https://hg.mozilla.org/integration/fx-team/rev/081e3e5c6d0a
https://tbpl.mozilla.org/?tree=Fx-Team&rev=081e3e5c6d0a
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js] → [mentor=bgrins][lang=html][good first bug][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/081e3e5c6d0a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js][fixed-in-fx-team] → [mentor=bgrins][lang=html][good first bug][lang=js]
Target Milestone: --- → Firefox 29
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js] → [mentor=bgrins][lang=html][good first bug][lang=js][qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.