1159109 - The MDN tooltip should be controlled by a pref

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Will Bamberg [:wbamberg]]

As Jeff requested in bug 980006, comment 50.

Comment 1

[Will Bamberg [:wbamberg]]

Attachment: 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".

Comment 2

[Patrick Brosset <:pbro>]

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.

Comment 3

[Will Bamberg [:wbamberg]]

Attachment: 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.

Comment 4

[Will Bamberg [:wbamberg]]

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?

Comment 5

[Will Bamberg [:wbamberg]]

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.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/14b2376c96fa

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/14b2376c96fa