Closed
Bug 960737
Opened 11 years ago
Closed 11 years ago
Remove inline script / style in browser/devtools/fontinspector/font-inspector.xhtml
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
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)
3.71 KB,
patch
|
bgrins
:
review+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 2•11 years ago
|
||
(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)
Comment 3•11 years ago
|
||
Can you assign me into this bug?
Reporter | ||
Comment 4•11 years ago
|
||
(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)
Reporter | ||
Comment 6•11 years ago
|
||
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)
Reporter | ||
Comment 8•11 years ago
|
||
(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)
Reporter | ||
Comment 10•11 years ago
|
||
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)
Assignee | ||
Comment 11•11 years ago
|
||
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)
Reporter | ||
Comment 12•11 years ago
|
||
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+
Reporter | ||
Comment 13•11 years ago
|
||
(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!
Reporter | ||
Comment 14•11 years ago
|
||
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]
Comment 15•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 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
Updated•11 years ago
|
Whiteboard: [mentor=bgrins][lang=html][good first bug][lang=js] → [mentor=bgrins][lang=html][good first bug][lang=js][qa-]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•