Closed Bug 1236991 Opened 4 years ago Closed 4 years ago

Update or remove tests that use fillInPageTooltip

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
e10s + ---
firefox48 --- fixed

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).
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
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: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
See Also: → 1251809
(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)
(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)
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: --- → ?
You should assume that the popup.xml version is correct.
(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. :-)
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)
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?
(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.
> 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.
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)
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+
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)
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/
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/
Attachment #8740018 - Attachment is obsolete: true
Attachment #8740019 - Attachment is obsolete: true
Attachment #8740020 - Attachment is obsolete: true
Attachment #8740021 - Attachment is obsolete: true
Attachment #8740022 - Attachment is obsolete: true
(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 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)
(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 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 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+
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?
(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... :-\
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)
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/
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/
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/
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/
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/
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/
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/
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/
Attachment #8740647 - Flags: review?(enndeakin) → review+
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 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+
Attachment #8740649 - Flags: review?(enndeakin) → review+
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
Depends on: 1265177
You need to log in before you can comment on or make changes to this bug.