Closed
Bug 1236991
Opened 8 years ago
Closed 8 years ago
Update or remove tests that use fillInPageTooltip
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 48
People
(Reporter: mconley, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
Attachments
(5 files, 5 obsolete files)
58 bytes,
text/x-review-board-request
|
mconley
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
enndeakin
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
enndeakin
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
enndeakin
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
enndeakin
:
review+
|
Details |
browser_bug329212.js seems to be concerned with SVG element tooltips, and calls popup.xml#tooltip's "fillInPageTooltip" method to populate the tooltip label given the SVG node. With e10s enabled, this forces us to use CPOWs to send the node to fillInPageTooltip. This test breaks when we outlaw CPOWs (bug 1233497). I'm not sure this test adds much value. As far as I can tell, fillInPageTooltip isn't used by anything but tests anyways (except for browser.js's "FillInHTMLTooltip", which isn't ever called by anyone). I've disabled the test for e10s for now, but we might just want to get rid of it entirely (as well as fillInPageTooltip).
Reporter | ||
Updated•8 years ago
|
Blocks: e10s-tests
Reporter | ||
Comment 1•8 years ago
|
||
Actually, it seems there are a number of tests that use fillInPageTooltip. They are: browser_bug329212.js browser_bug331772_xul_tooltiptext_in_html.js browser_bug561623.js browser_bug581947.js browser_input_file_tooltips.js These tests will just break when CPOWs go away and should be re-written to use a different mechanism to exercise tooltips.
Summary: browser_bug329212.js exercises only non-remote browser tooltip → Update or remove tests that use fillInPageTooltip
Comment 2•8 years ago
|
||
While the global fillInPageTooltip function in browser.js is deprecated, the code for it was moved into popup.xml, and the functionality still needs to be tested. In e10s though, a different codepath is used that does more or less the same thing is used instead (in nsDocShellTreeOwner.cpp). These tests should be rewitten to open up real tooltips and then verify their contents which would handle both cases. Ideally, of course, both codepaths would also be merged but that is a separate issue.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Neil Deakin from comment #2) > While the global fillInPageTooltip function in browser.js is deprecated, the > code for it was moved into popup.xml, and the functionality still needs to > be tested. > > In e10s though, a different codepath is used that does more or less the same > thing is used instead (in nsDocShellTreeOwner.cpp). > > These tests should be rewitten to open up real tooltips and then verify > their contents which would handle both cases. > > Ideally, of course, both codepaths would also be merged but that is a > separate issue. The tests actually in several cases test that the tooltip is empty and not shown for particular elements. That's not really testable, because tooltips are not shown synchronously, and while you can wait for a popupshown event for the tooltip to be shown, you can't really determine when a tooltip would not be shown. Is there a better way to do this?
Flags: needinfo?(enndeakin)
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #3) > (In reply to Neil Deakin from comment #2) > > While the global fillInPageTooltip function in browser.js is deprecated, the > > code for it was moved into popup.xml, and the functionality still needs to > > be tested. > > > > In e10s though, a different codepath is used that does more or less the same > > thing is used instead (in nsDocShellTreeOwner.cpp). > > > > These tests should be rewitten to open up real tooltips and then verify > > their contents which would handle both cases. > > > > Ideally, of course, both codepaths would also be merged but that is a > > separate issue. > > The tests actually in several cases test that the tooltip is empty and not > shown for particular elements. That's not really testable, because tooltips > are not shown synchronously, and while you can wait for a popupshown event > for the tooltip to be shown, you can't really determine when a tooltip would > not be shown. > > Is there a better way to do this? Per discussion with Neil on IRC, it's unclear that there's a way to wait for a tooltip to not be shown, so instead I'll look at unifying the code here and then fixing the tests to test the same codepath in e10s and non-e10s.
Flags: needinfo?(enndeakin)
Assignee | ||
Comment 5•8 years ago
|
||
nsDocShellTreeOwner's code actually behaves subtly different from the popup.xml code (in particular, it doesn't currently deal with SVG <title> elements inside SVG <a> elements -- there might be other bugs I'll uncover when adapting these tests), so we should probably fix this sooner rather than later. Requesting tracking to keep this on the radar.
tracking-e10s:
--- → ?
Comment 6•8 years ago
|
||
You should assume that the popup.xml version is correct.
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Neil Deakin from comment #6) > You should assume that the popup.xml version is correct. Right, and so I did - hence requesting tracking-e10s, because that means a number of edgecases are due to regress with e10s unless we fix them. :-)
Assignee | ||
Comment 8•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45493/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45493/
Attachment #8740023 -
Flags: review?(mconley)
Assignee | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45495/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45495/
Assignee | ||
Comment 10•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45497/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45497/
Assignee | ||
Comment 11•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45499/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45499/
Assignee | ||
Comment 12•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45501/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45501/
Assignee | ||
Comment 13•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45503/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45503/
Assignee | ||
Comment 14•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45505/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45505/
Assignee | ||
Updated•8 years ago
|
Attachment #8740018 -
Flags: review?(enndeakin)
Attachment #8740019 -
Flags: review?(enndeakin)
Attachment #8740020 -
Flags: review?(enndeakin)
Attachment #8740021 -
Flags: review?(enndeakin)
Attachment #8740022 -
Flags: review?(enndeakin)
Attachment #8740024 -
Flags: review?(enndeakin)
Updated•8 years ago
|
Comment 15•8 years ago
|
||
I'd actually expected you to make a JS component out of the popup.xml code. It doesn't matter I guess, but was there a reason not to?
Assignee | ||
Comment 16•8 years ago
|
||
(In reply to Neil Deakin from comment #15) > I'd actually expected you to make a JS component out of the popup.xml code. > It doesn't matter I guess, but was there a reason not to? I did consider that option. The reason I didn't was the comment in popup.xml: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/popup.xml#533-535 Note that DefaultTooltipTextProvider::GetNodeText() from nsDocShellTreeOwner.cpp also performs the same function, but for embedded clients that don't use a XUL/JS layer. These two should be kept synchronized. I assumed that using a JS component when there wasn't a JS layer wasn't the right thing to do. It also just seemed kind of wrong to make embedding/ depend on the toolkit-provided JS component, and embedding itself seemed to have no existing JS components - I wouldn't know where/how to get them packaged from there.
Comment 17•8 years ago
|
||
> I assumed that using a JS component when there wasn't a JS layer wasn't the
> right thing to do.
I think that comment refers to a xul browser ui. There are many JS components that would be available in an embedded environment. Look for .manifest files along with their corresponding .js file.
Assignee | ||
Updated•8 years ago
|
Attachment #8740018 -
Flags: review?(enndeakin)
Attachment #8740019 -
Flags: review?(enndeakin)
Attachment #8740020 -
Flags: review?(enndeakin)
Attachment #8740021 -
Flags: review?(enndeakin)
Attachment #8740022 -
Flags: review?(enndeakin)
Reporter | ||
Comment 18•8 years ago
|
||
Comment on attachment 8740023 [details] MozReview Request: Bug 1236991 - part 3: allow use of todo() from ContentTask, r=mconley https://reviewboard.mozilla.org/r/45503/#review42405
Attachment #8740023 -
Flags: review?(mconley) → review+
Assignee | ||
Comment 19•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45847/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45847/
Attachment #8740023 -
Attachment description: MozReview Request: Bug 1236991 - part 6: allow use of todo() from ContentTask, r?mconley → MozReview Request: Bug 1236991 - part 3: allow use of todo() from ContentTask, r=mconley
Attachment #8740024 -
Attachment description: MozReview Request: Bug 1236991 - part 7: fix the tests to run in e10s mode, r?enndeakin → MozReview Request: Bug 1236991 - part 4: fix the tests to run in e10s mode, r?enndeakin
Attachment #8740647 -
Flags: review?(enndeakin)
Attachment #8740648 -
Flags: review?(enndeakin)
Attachment #8740649 -
Flags: review?(enndeakin)
Assignee | ||
Comment 20•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45849/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45849/
Assignee | ||
Comment 21•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45851/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45851/
Assignee | ||
Comment 22•8 years ago
|
||
Comment on attachment 8740023 [details] MozReview Request: Bug 1236991 - part 3: allow use of todo() from ContentTask, r=mconley Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45503/diff/1-2/
Assignee | ||
Comment 23•8 years ago
|
||
Comment on attachment 8740024 [details] MozReview Request: Bug 1236991 - part 4: fix the tests to run in e10s mode, r?enndeakin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45505/diff/1-2/
Assignee | ||
Updated•8 years ago
|
Attachment #8740018 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8740019 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8740020 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8740021 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8740022 -
Attachment is obsolete: true
Assignee | ||
Comment 24•8 years ago
|
||
(In reply to Neil Deakin from comment #17) > > I assumed that using a JS component when there wasn't a JS layer wasn't the > > right thing to do. > > I think that comment refers to a xul browser ui. There are many JS > components that would be available in an embedded environment. Look for > .manifest files along with their corresponding .js file. Done. I stuck it in toolkit/, but I guess we could put it somewhere else if you think that makes more sense. I use hg cp on popup.xml so it'd be easier to see what I changed, and it kind of worked... I had to change a number of the references to DOM globals (Node, SVGElement, etc.) to rely on the tipElement's defaultView, and I refactored the final lines to not set this.label / this.style.direction but to return the two (see also the patch labeled "part 1"), but it's otherwise unchanged.
Comment 25•8 years ago
|
||
Comment on attachment 8740649 [details] MozReview Request: Bug 1236991 - bonus: move all the tests into the component dir instead of cluttering up browser/.../tests/general, r?enndeakin https://reviewboard.mozilla.org/r/45851/#review42961 You want to remove title_test.svg from browser.ini as well.
Attachment #8740649 -
Flags: review?(enndeakin)
Assignee | ||
Comment 26•8 years ago
|
||
(In reply to Neil Deakin from comment #25) > Comment on attachment 8740649 [details] > MozReview Request: Bug 1236991 - bonus: move all the tests into the > component dir instead of cluttering up browser/.../tests/general, r?enndeakin > > https://reviewboard.mozilla.org/r/45851/#review42961 > > You want to remove title_test.svg from browser.ini as well. It is also used from https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/offlineQuotaNotification.cacheManifest which is in turn used in some offline quota tests. :-(
Comment 27•8 years ago
|
||
Comment on attachment 8740647 [details] MozReview Request: Bug 1236991 - part 1: allow forwarding label direction through nsITooltipTextProvider, r?enndeakin https://reviewboard.mozilla.org/r/45847/#review42963 The direction handling is actually bug 1169648 that was never finished. Perhaps coordinate there to make sure there isn't anything missing here.
Attachment #8740647 -
Flags: review?(enndeakin)
Comment 28•8 years ago
|
||
Comment on attachment 8740024 [details] MozReview Request: Bug 1236991 - part 4: fix the tests to run in e10s mode, r?enndeakin https://reviewboard.mozilla.org/r/45505/#review42965
Attachment #8740024 -
Flags: review?(enndeakin) → review+
Comment 29•8 years ago
|
||
The try run at https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f6e3151904f&selectedJob=19378728 has some failures. Is that reflective of the latest patches and should I wait to finishing reviewing here?
Assignee | ||
Comment 30•8 years ago
|
||
(In reply to Neil Deakin from comment #29) > The try run at > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=3f6e3151904f&selectedJob=19378728 has some failures. > Is that reflective of the latest patches and should I wait to finishing > reviewing here? That's surprising. I guess you can wait while I update for some of these review comments and try to work out why the test isn't working - I'm fairly sure I ran it locally and it worked, but clearly it's not working on infra... :-\
Assignee | ||
Comment 31•8 years ago
|
||
Comment on attachment 8740647 [details] MozReview Request: Bug 1236991 - part 1: allow forwarding label direction through nsITooltipTextProvider, r?enndeakin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45847/diff/1-2/
Attachment #8740647 -
Flags: review?(enndeakin)
Attachment #8740649 -
Flags: review?(enndeakin)
Assignee | ||
Comment 32•8 years ago
|
||
Comment on attachment 8740648 [details] MozReview Request: Bug 1236991 - part 2: implement a default tooltiptextprovider in toolkit, r?enndeakin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45849/diff/1-2/
Assignee | ||
Comment 33•8 years ago
|
||
Comment on attachment 8740023 [details] MozReview Request: Bug 1236991 - part 3: allow use of todo() from ContentTask, r=mconley Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45503/diff/2-3/
Assignee | ||
Comment 34•8 years ago
|
||
Comment on attachment 8740024 [details] MozReview Request: Bug 1236991 - part 4: fix the tests to run in e10s mode, r?enndeakin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45505/diff/2-3/
Assignee | ||
Comment 35•8 years ago
|
||
Comment on attachment 8740649 [details] MozReview Request: Bug 1236991 - bonus: move all the tests into the component dir instead of cluttering up browser/.../tests/general, r?enndeakin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45851/diff/1-2/
Assignee | ||
Comment 36•8 years ago
|
||
Comment on attachment 8740648 [details] MozReview Request: Bug 1236991 - part 2: implement a default tooltiptextprovider in toolkit, r?enndeakin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45849/diff/2-3/
Assignee | ||
Comment 37•8 years ago
|
||
Comment on attachment 8740023 [details] MozReview Request: Bug 1236991 - part 3: allow use of todo() from ContentTask, r=mconley Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45503/diff/3-4/
Assignee | ||
Comment 38•8 years ago
|
||
Comment on attachment 8740024 [details] MozReview Request: Bug 1236991 - part 4: fix the tests to run in e10s mode, r?enndeakin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45505/diff/3-4/
Assignee | ||
Comment 39•8 years ago
|
||
Comment on attachment 8740649 [details] MozReview Request: Bug 1236991 - bonus: move all the tests into the component dir instead of cluttering up browser/.../tests/general, r?enndeakin Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45851/diff/2-3/
Updated•8 years ago
|
Attachment #8740647 -
Flags: review?(enndeakin) → review+
Comment 40•8 years ago
|
||
Comment on attachment 8740647 [details] MozReview Request: Bug 1236991 - part 1: allow forwarding label direction through nsITooltipTextProvider, r?enndeakin https://reviewboard.mozilla.org/r/45847/#review43071
Comment 41•8 years ago
|
||
Comment on attachment 8740648 [details] MozReview Request: Bug 1236991 - part 2: implement a default tooltiptextprovider in toolkit, r?enndeakin https://reviewboard.mozilla.org/r/45849/#review43073
Attachment #8740648 -
Flags: review?(enndeakin) → review+
Updated•8 years ago
|
Attachment #8740649 -
Flags: review?(enndeakin) → review+
Comment 42•8 years ago
|
||
Comment on attachment 8740649 [details] MozReview Request: Bug 1236991 - bonus: move all the tests into the component dir instead of cluttering up browser/.../tests/general, r?enndeakin https://reviewboard.mozilla.org/r/45851/#review43075
Comment 43•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/86c2f4fb77a5 https://hg.mozilla.org/integration/mozilla-inbound/rev/d42f9b1e05a3 https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e38e7b5ae7 https://hg.mozilla.org/integration/mozilla-inbound/rev/a91d2ac5476f https://hg.mozilla.org/integration/mozilla-inbound/rev/1418ce4fcd0b
Comment 45•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/86c2f4fb77a5 https://hg.mozilla.org/mozilla-central/rev/d42f9b1e05a3 https://hg.mozilla.org/mozilla-central/rev/b6e38e7b5ae7 https://hg.mozilla.org/mozilla-central/rev/a91d2ac5476f https://hg.mozilla.org/mozilla-central/rev/1418ce4fcd0b https://hg.mozilla.org/mozilla-central/rev/9f90060e64a0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in
before you can comment on or make changes to this bug.
Description
•