The MDN tooltip should be controlled by a pref

RESOLVED FIXED in Firefox 40

Status

DevTools
Inspector
RESOLVED FIXED
3 years ago
a month ago

People

(Reporter: wbamberg, Assigned: wbamberg)

Tracking

unspecified
Firefox 40

Firefox Tracking Flags

(firefox40 fixed)

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
As Jeff requested in bug 980006, comment 50.
(Assignee)

Updated

3 years ago
Whiteboard: [devedition-40][difficulty=easy]
(Assignee)

Updated

3 years ago
Assignee: nobody → wbamberg
(Assignee)

Comment 1

3 years ago
Created attachment 8599571 [details] [diff] [review]
mdn-tooltip-preffable.patch

I wasn't sure what the desired approach is here, so I basically copied what we do with "devtools.inspector.showUserAgentStyles".
Attachment #8599571 - Flags: review?(pbrosset)
Comment on attachment 8599571 [details] [diff] [review]
mdn-tooltip-preffable.patch

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

Very nice. I don't see any problem with this.

Just one comment, but that won't stop landing this: The pref observer mechanism in rule-view that dynamically updates the behavior when the pref changes is nice to have at this stage, and I would also accept a patch that didn't have that (the main use case for the pref isn't really for users to turn on/off, but rather for us to disable the tooltip should we find any problem with it).

Anyway, good job. Thanks.

::: browser/devtools/styleinspector/test/browser_ruleview_context-menu-show-mdn-docs-03.js
@@ +27,5 @@
> + * 10. set the popupNode for the context menu
> + *   -> check that the MDN context menu item is not hidden
> + * 11. ensure "devtools.inspector.mdnDocsTooltip.enabled" is reset
> + * to its original value
> + */

No specific comment about this new test, it's really nice.
The only thing is I think this big comment about the steps of the test isn't useful. We can directly read the code, since it's as clear as the comment. If you think it's not, then you can potentially add some more comments within the code. But having both this long comment and the code below means we essentially have twice the same logic explained that have to be maintained whenever the test will change in the future.
Attachment #8599571 - Flags: review?(pbrosset) → review+
(Assignee)

Comment 3

3 years ago
Created attachment 8602493 [details] [diff] [review]
0001-Bug-1159109-The-MDN-tooltip-should-be-controlled-by-.patch

Thanks pbrosset! I've removed the comment from the test file - I agree, it wasn't useful.
Attachment #8599571 - Attachment is obsolete: true
(Assignee)

Comment 5

3 years ago
I've retriggered the three failed tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfdd71d65e14

2 of the failures passed this time, but the Linux debug timeout is still there (bug 1137757). This seems like a really active intermittent, and it seems to be infrastructure-related, not related to this patch, so I'm asking for checkin anyway.
Flags: needinfo?(pbrosset)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/14b2376c96fa
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]

Updated

a month ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.