Closed Bug 1429929 Opened 6 years ago Closed 6 years ago

Remove "tabbrowser-close-tab-button" binding

Categories

(Firefox :: Tabbed Browser, task, P3)

Product:
Firefox
Component:
Tabbed Browser
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: Kwan, Assigned: dao)

References

Details

(Whiteboard: [xbl-available])

Attachments

(1 file)

Bug 1429929 - Remove tabbrowser-close-tab-button binding.
59 bytes, text/x-review-board-request
Gijs
: review+
Details 
https://mozilla.logbot.info/developers/20171220#c14038395-c14038406
><Gijs> Kwan|away: maybe file a followup bug to just move the handler stuff into JS and/or tabbrowser as a whole?
><Gijs> pretty sure that basically the only "point" of the handlers right now is to catch stuff before the enclosing tab does
><Gijs> and we could just update the enclosing tab's handlers to check for the close button
><Gijs> and then remove the binding
><Gijs> wheee
Whiteboard: [xbl-available]
Priority: -- → P3
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
Comment on attachment 8942209 [details]
Bug 1429929 - Remove tabbrowser-close-tab-button binding.

https://reviewboard.mozilla.org/r/212474/#review218204

::: browser/base/content/tabbrowser.xml:8269
(Diff revision 1)
> +      <handler event="dblclick" button="0" phase="capturing">
> +        // for the one-close-button case

I'm assuming you've tested the edgecases here (ie I haven't tested this patch myself).

Was there a particular reason not to use a CDATA block here? I understand that it's perfectly valid as-is, but it'll break as soon as someone uses one of the XML chars, so I guess I normally would have added one. Up to you.
Attachment #8942209 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #2)
> I'm assuming you've tested the edgecases here (ie I haven't tested this
> patch myself).

Tested except for the tab opening behavior -- we don't exercise that in most situations nowadays.

> Was there a particular reason not to use a CDATA block here?

The original handler that I moved didn't have one...
(In reply to Dão Gottwald [::dao] from comment #3)
> (In reply to :Gijs from comment #2)
> > Was there a particular reason not to use a CDATA block here?
> 
> The original handler that I moved didn't have one...

Oh, d'oh. The diff just highlights the changes to the existing one for the previous method which is why I noticed. Carry on, though feel free to put a block in.
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8c2097e14828
Remove tabbrowser-close-tab-button binding. r=Gijs
Backed out for failing browser-chrome's browser_audioTabIcon.js and browser_usercontextid_tabdrop.js:

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8c2097e148288b26439cf0aa000b10a6fd82ca11&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception

Failure log browser_audioTabIcon.js: https://treeherder.mozilla.org/logviewer.html#?job_id=155963161&repo=autoland
[task 2018-01-12T17:08:52.248Z] 17:08:52     INFO - TEST-PASS | browser/base/content/test/general/browser_audioTabIcon.js | No muted attribute should be persisted - 
[task 2018-01-12T17:08:52.249Z] 17:08:52     INFO - TEST-PASS | browser/base/content/test/general/browser_audioTabIcon.js | No muteReason property should be persisted - 
[task 2018-01-12T17:08:52.250Z] 17:08:52     INFO - TEST-PASS | browser/base/content/test/general/browser_audioTabIcon.js | Clicking on mute should not change the currently selected tab - 
[task 2018-01-12T17:08:52.251Z] 17:08:52     INFO - Buffered messages logged at 17:07:21
[task 2018-01-12T17:08:52.252Z] 17:08:52     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 1
[task 2018-01-12T17:08:52.253Z] 17:08:52     INFO - Buffered messages finished
[task 2018-01-12T17:08:52.254Z] 17:08:52     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_audioTabIcon.js | Test timed out - 
[task 2018-01-12T17:08:52.255Z] 17:08:52     INFO - GECKO(1067) | MEMORY STAT | vsize 20974278MB | residentFast 1212MB
[task 2018-01-12T17:08:52.256Z] 17:08:52     INFO - TEST-OK | browser/base/content/test/general/browser_audioTabIcon.js | took 180047ms
[task 2018-01-12T17:08:52.257Z] 17:08:52     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-01-12T17:08:52.258Z] 17:08:52     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_audioTabIcon.js | Found a tab after previous test timed out: about:blank - 

Failure log browser_usercontextid_tabdrop.js: https://treeherder.mozilla.org/logviewer.html#?job_id=155963775&repo=autoland
[task 2018-01-12T17:04:21.070Z] 17:04:21     INFO - TEST-START | browser/components/contextualidentity/test/browser/browser_usercontextid_tabdrop.js
[task 2018-01-12T17:04:21.336Z] 17:04:21     INFO - TEST-INFO | started process screentopng
[task 2018-01-12T17:04:21.876Z] 17:04:21     INFO - TEST-INFO | screentopng: exit 0
[task 2018-01-12T17:04:21.876Z] 17:04:21     INFO - Buffered messages logged at 17:04:21
[task 2018-01-12T17:04:21.877Z] 17:04:21     INFO - Entering test bound 
[task 2018-01-12T17:04:21.878Z] 17:04:21     INFO - Buffered messages finished
[task 2018-01-12T17:04:21.879Z] 17:04:21     INFO - TEST-UNEXPECTED-FAIL | browser/components/contextualidentity/test/browser/browser_usercontextid_tabdrop.js | uncaught exception - TypeError: event.originalTarget.getAttribute is not a function at onxbldragstart@chrome://browser/content/tabbrowser.xml:8230:13
Flags: needinfo?(dao+bmo)
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/a3d15c0fe9a8
Backed out changeset 8c2097e14828 for failing browser-chrome's browser_audioTabIcon.js and browser_usercontextid_tabdrop.js
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11fd0d47649f
Remove tabbrowser-close-tab-button binding. r=Gijs
Flags: needinfo?(dao+bmo)
https://hg.mozilla.org/mozilla-central/rev/11fd0d47649f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: