As Jeff requested in bug 980006, comment 50.
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+
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
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dfdd71d65e14 - there are some test failures, but they are (seemingly) unrelated intermittents: https://bugzilla.mozilla.org/show_bug.cgi?id=1137757 https://bugzilla.mozilla.org/show_bug.cgi?id=1157523 https://bugzilla.mozilla.org/show_bug.cgi?id=1162067 What do you think?
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.
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]
You need to log in before you can comment on or make changes to this bug.