Hector Zhao [:hectorz] Assignee
	Description • 6 years ago

As mentioned in bug 1383669, most browsers in China allows users to close a tab by dblclicking it, and we at Beijing office currently ships a Mozilla signed extension to implement this.

Since dblclick on unselected tab would select that tab before it gets closed, I'll limit the implementation to selected tab only. Stats we've collected [1] shows this covers ~80% of the dblclicks. Also, this limitation would prevent a malfunctioning mouse from accidentally close another tab when a user wants to select it, which is a common complaint we get.

I'm sending my initial patch for early feedback. Is there a better way to detect a dblclick on selected tab? Also should I still add logic to xbl bindings or not?

I'll update the patch to include tests and expose this to WebExtension later.

[1]: https://github.com/MozillaOnline/tabtweak/commit/cf363014

	Hector Zhao [:hectorz] Assignee
	Comment 5 • 6 years ago

(In reply to Hector Zhao [:hectorz] from comment #0)
> 
> I'm sending my initial patch for early feedback. Is there a better way to
> detect a dblclick on selected tab? Also should I still add logic to xbl
> bindings or not?
> 
> I'll update the patch to include tests and expose this to WebExtension later.

Patches updated to include these two parts.

I thought this might be affected by bug 1392352 (looks like it isn't), and waited until it landed before I picked this back up.

I also learned about UIEvent.detail of click/dblclick/mousedown/mouseup when trying to figure out the tests, and updated the part 1 to use it instead of counting by myself.

	Shane Caraveo (:mixedpuppy)
	Comment 6 • 6 years ago

Comment on attachment 8956371 [details]
Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension.

I dont see any reason to expose this in webextensions.  

The pref can be switched on by default for the china distribution.  The option should also be exposed in about:preferences.

	Dão Gottwald [:dao]
	Comment 7 • 6 years ago
mozreview-review

Comment on attachment 8947708 [details]
Bug 1435142 - Part 1: pref to enable close selected tab by dblclicking it.

https://reviewboard.mozilla.org/r/217422/#review231290

::: browser/base/content/tabbrowser.xml:982
(Diff revision 2)
>          } else if (tab.closing) {
>            this.tabbrowser._endRemoveTab(tab);
>          }
>        ]]></handler>
>  
> +      <handler event="mousedown" button="0" phase="capturing"><![CDATA[

Why are you doing this in the tabbrowser-tabs binding rather than in tabbrowser-tab?

::: browser/base/content/tabbrowser.xml:984
(Diff revision 2)
>          }
>        ]]></handler>
>  
> +      <handler event="mousedown" button="0" phase="capturing"><![CDATA[
> +        if (!this._closeTabByDblclick ||
> +            event.button != 0 ||

You already put button="0" on the handler element.

::: browser/base/content/tabbrowser.xml:987
(Diff revision 2)
> +      <handler event="mousedown" button="0" phase="capturing"><![CDATA[
> +        if (!this._closeTabByDblclick ||
> +            event.button != 0 ||
> +            event.detail != 1 ||
> +            event.target.localName != "tab") {
> +          return;

Invert the conditions and move  this._firstMouseDownOnSelectedTab = ... into this block?

::: browser/base/content/tabbrowser.xml:995
(Diff revision 2)
> +        this._firstMouseDownOnSelectedTab = event.target.selected;
> +      ]]></handler>
> +
>        <handler event="dblclick"><![CDATA[
> +        if (this._closeTabByDblclick &&
> +            this._firstMouseDownOnSelectedTab &&

Why are you checking whether the tab is selected in the mousedown handler only and not here? The tab could be a different one here. That's actually not that unrealistic.

::: browser/base/content/tabbrowser.xml:1001
(Diff revision 2)
> +            event.button == 0 &&
> +            event.target.localName == "tab") {
> +          let tab = event.target;
> +          if (!tab._overPlayingIcon) {
> +            this.tabbrowser.removeTab(tab, {animate: true,
> +                byMouse: event.mozInputSource == MouseEvent.MOZ_SOURCE_MOUSE});

nit: format like this:

this.tabbrowser.removeTab(tab, {
  animate: true,
  byMouse: event.mozInputSource == MouseEvent.MOZ_SOURCE_MOUSE,
});

	Shane Caraveo (:mixedpuppy)
	Comment 8 • 6 years ago

mconca for approval of new api.

	Mike Conca [:mconca]
	Comment 9 • 6 years ago

Thanks :mixedpuppy for giving me a heads up. The request to expose this browser preference via a WebExtensions API came without much notice. That said, it was a common feature of several add-ons in the past including TabMixPlus, one of the most popular tab managment add-ons.  In addition, the UX supported by this change is common in China, making it likely that this feature will continue to be supported in the future.

I recommended this API be approved.

	Hector Zhao [:hectorz] Assignee
	Comment 16 • 6 years ago
mozreview-review-reply

Comment on attachment 8947708 [details]
Bug 1435142 - Part 1: pref to enable close selected tab by dblclicking it.

https://reviewboard.mozilla.org/r/217422/#review231290

> Why are you doing this in the tabbrowser-tabs binding rather than in tabbrowser-tab?

Moved into tabbrowser-tab. I think it is because this patch was created based on our extension, where adding an event listener to tabContainer is easier than individual tabs.

> You already put button="0" on the handler element.

Keep it since there's no `button="0"` on tabbrowser-tab's mousedown handler.

> Invert the conditions and move  this._firstMouseDownOnSelectedTab = ... into this block?

Done.

> Why are you checking whether the tab is selected in the mousedown handler only and not here? The tab could be a different one here. That's actually not that unrealistic.

When I first tried gathering some stats about dblclicking, I checked `tab.selected` in the dblclick handler, but they're all true. I guess the `mousedown,mouseup,click,mousedown,mouseup,click,dblclick` sequence means the tab where `dblclick` happens must have been selected by the preceding `mousedown`?

> nit: format like this:
> 
> this.tabbrowser.removeTab(tab, {
>   animate: true,
>   byMouse: event.mozInputSource == MouseEvent.MOZ_SOURCE_MOUSE,
> });

Done.

	Hector Zhao [:hectorz] Assignee
	Comment 17 • 6 years ago

(In reply to Mike Conca [:mconca] (Denver, CO, USA UTC-6) from comment #9)
> Thanks :mixedpuppy for giving me a heads up. The request to expose this
> browser preference via a WebExtensions API came without much notice. That
> said, it was a common feature of several add-ons in the past including
> TabMixPlus, one of the most popular tab managment add-ons.  In addition, the
> UX supported by this change is common in China, making it likely that this
> feature will continue to be supported in the future.
> 
> I recommended this API be approved.

Thanks, with this exposed, we'll finally be able to move our tabtweak@mozillaonline.com to a proper WebExtension.

Sorry I didn't raise this earlier.

(In reply to Shane Caraveo (:mixedpuppy) from comment #6)
> 
> I dont see any reason to expose this in webextensions.  
> 
> The pref can be switched on by default for the china distribution.

Thanks for flagging :mconca about this. An extension API is more flexable to use than the distribution files.

> The option should also be exposed in about:preferences.

I think none of the tab behavior prefs behind `openBookmarksInNewTabs`, `openSearchResultsInNewTabs` and the incoming `newTabPosition` are exposed in about:preferences?

	Dão Gottwald [:dao]
	Comment 18 • 6 years ago
mozreview-review

Comment on attachment 8947708 [details]
Bug 1435142 - Part 1: pref to enable close selected tab by dblclicking it.

https://reviewboard.mozilla.org/r/217422/#review231598

::: browser/base/content/tabbrowser.xml:950
(Diff revision 4)
> +              animate: true,
> +              byMouse: event.mozInputSource == MouseEvent.MOZ_SOURCE_MOUSE,
> +            });
> +          }
> +          return;
> +        }

Move this to the tabbrowser-tab binding as well?

::: browser/base/content/tabbrowser.xml:1910
(Diff revision 4)
>        <![CDATA[
> +        let tabContainer = this.parentNode;
> +        if (tabContainer._closeTabByDblclick &&
> +            event.button == 0 &&
> +            event.detail == 1 &&
> +            event.target.localName == "tab") {

I don't think event.target.localName == "tab" can ever be false here...

	Shane Caraveo (:mixedpuppy)
	Comment 19 • 6 years ago
mozreview-review

Comment on attachment 8956371 [details]
Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension.

https://reviewboard.mozilla.org/r/225232/#review231648

This looks fine given my comment below.  I'm waiting to r+ for a bit of conversation on whether this is the right approach or something more generic like a click listener wouldn't be better.

::: toolkit/components/extensions/schemas/browser_settings.json:120
(Diff revision 3)
> +      "closeTabByDblclick": {
> +        "$ref": "types.Setting",
> +        "description": "This boolean setting controls whether the selected tab can be closed with a dblclick."
> +      },

Please use complete words for anything public facing.

"closeTabsByDoubleClick"

"can be closed with a double click"

	Hector Zhao [:hectorz] Assignee
	Comment 23 • 6 years ago
mozreview-review-reply

Comment on attachment 8947708 [details]
Bug 1435142 - Part 1: pref to enable close selected tab by dblclicking it.

https://reviewboard.mozilla.org/r/217422/#review231598

> Move this to the tabbrowser-tab binding as well?

Moved. I also moved `tabbrowser-tabs._firstMouseDownOnSelectedTab` to `tabbrowser-tab._selectedOnFirstMouseDown`. I think it's better not to read the pref for every tab so I kept `tabbrowser-tabs._closeTabByDblclick` where it is.

> I don't think event.target.localName == "tab" can ever be false here...

Removed.

	Hector Zhao [:hectorz] Assignee
	Comment 24 • 6 years ago
mozreview-review-reply

Comment on attachment 8956371 [details]
Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension.

https://reviewboard.mozilla.org/r/225232/#review231648

The idea of a more generic tab event listener was proposed and denied in bug 1246706.

> Please use complete words for anything public facing.
> 
> "closeTabsByDoubleClick"
> 
> "can be closed with a double click"

Done.

	Shane Caraveo (:mixedpuppy)
	Comment 25 • 6 years ago
mozreview-review

Comment on attachment 8956371 [details]
Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension.

https://reviewboard.mozilla.org/r/225232/#review232370

::: toolkit/components/extensions/ext-browserSettings.js:234
(Diff revision 4)
> +        closeTabsByDoubleClick: getSettingsAPI(
> +          extension, "closeTabsByDoubleClick",
> +          () => {
> +            return Services.prefs.getBoolPref("browser.tabs.closeTabByDblclick");
> +          }),

I think you probably should address android here.  Have you run a try that includes android?

	Hector Zhao [:hectorz] Assignee
	Comment 26 • 6 years ago
mozreview-review-reply

Comment on attachment 8956371 [details]
Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension.

https://reviewboard.mozilla.org/r/225232/#review232370

> I think you probably should address android here.  Have you run a try that includes android?

My patch doesn't touch any Android code, I believe this pref won't work there. I haven't pushed this to try, but will do that later today.

Are you suggesting adding some documentation about this fact? A default value here to prevent `getBoolPref` throwing in Android? Or making some changes to hide this API from Android?

	Hector Zhao [:hectorz] Assignee
	Comment 27 • 6 years ago

(In reply to Hector Zhao [:hectorz] from comment #26)
> 
> > I think you probably should address android here.  Have you run a try that includes android?
> 
> My patch doesn't touch any Android code, I believe this pref won't work
> there. I haven't pushed this to try, but will do that later today.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7969e71766756bfec289cb749db02a63a2b1ee72

	Shane Caraveo (:mixedpuppy)
	Comment 28 • 6 years ago
mozreview-review-reply

Comment on attachment 8956371 [details]
Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension.

https://reviewboard.mozilla.org/r/225232/#review232370

> My patch doesn't touch any Android code, I believe this pref won't work there. I haven't pushed this to try, but will do that later today.
> 
> Are you suggesting adding some documentation about this fact? A default value here to prevent `getBoolPref` throwing in Android? Or making some changes to hide this API from Android?

This file runs on android.  If you look through it, you'll see code to handle things that dont work on android.  Since the pref is not defaulted on android, the getter will throw.  e.g. see proxyConfig

	Hector Zhao [:hectorz] Assignee
	Comment 32 • 6 years ago
mozreview-review-reply

Comment on attachment 8956371 [details]
Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension.

https://reviewboard.mozilla.org/r/225232/#review232370

> This file runs on android.  If you look through it, you'll see code to handle things that dont work on android.  Since the pref is not defaulted on android, the getter will throw.  e.g. see proxyConfig

Updated it to a no-op on Android, like proxyConfig.

	Hector Zhao [:hectorz] Assignee
	Comment 34 • 6 years ago
mozreview-review-reply

Comment on attachment 8956371 [details]
Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension.

https://reviewboard.mozilla.org/r/225232/#review232370

> Updated it to a no-op on Android, like proxyConfig.

Sorry, not exactly a no-op, and the tests are skipped on Android, like proxyConfig.

	Shane Caraveo (:mixedpuppy)
	Comment 35 • 6 years ago
mozreview-review

Comment on attachment 8956371 [details]
Bug 1435142 - Part 3: expose closeTabByDblclick to WebExtension.

https://reviewboard.mozilla.org/r/225232/#review233510

r+ with assertion test

::: toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js:168
(Diff revisions 4 - 6)
>      await testSetting(
>        "contextMenuShowEvent", "mousedown",
>        {"ui.context_menus.after_mouseup": false});
>    }
>  
> +  if (AppConstants.platform !== "android") {

This is just skipping any test on android now.  I'm guessing you should be able to add to test_bad_value, something like

await browser.test.assertRejects(
      browser.browserSettings.closeTabsByDoubleClick.set({value: true}),
       /closeTabsByDoubleClick is not supported on android/,
        "closeTabsByDoubleClick.set rejects on Android.");

	Hector Zhao [:hectorz] Assignee
	Comment 37 • 6 years ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #35)
> 
> This is just skipping any test on android now.  I'm guessing you should be
> able to add to test_bad_value, something like

Since the original check in `test_bad_value` applies to both Android and other platforms, a separate function named `test_bad_value_android` was added for this. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3a49bc18cb64734496d298718d01c339a9d36fe3

	Dão Gottwald [:dao]
	Comment 38 • 6 years ago
mozreview-review

Comment on attachment 8947708 [details]
Bug 1435142 - Part 1: pref to enable close selected tab by dblclicking it.

https://reviewboard.mozilla.org/r/217422/#review234176

::: browser/base/content/tabbrowser.xml:2012
(Diff revision 6)
> +        let tabContainer = this.parentNode;
> +        if (tabContainer._closeTabByDblclick &&
> +            this._selectedOnFirstMouseDown &&
> +            this.selected &&
> +            !this._overPlayingIcon) {
> +          tabContainer.tabbrowser.removeTab(this, {

The tabbrowser property has been removed, please use gBrowser instead

	Dão Gottwald [:dao]
	Comment 39 • 6 years ago
mozreview-review

Comment on attachment 8956370 [details]
Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick.

https://reviewboard.mozilla.org/r/225230/#review234182

::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:19
(Diff revision 5)
> +  let tab = gBrowser.selectedTab;
> +
> +  let promise = BrowserTestUtils.waitForEvent(tab, "dblclick");
> +  triggerDblclickOn(tab);
> +  await promise;
> +  Assert.notStrictEqual(tab.closing, true,

ok(!tab.closing, ...);

::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:20
(Diff revision 5)
> +
> +  let promise = BrowserTestUtils.waitForEvent(tab, "dblclick");
> +  triggerDblclickOn(tab);
> +  await promise;
> +  Assert.notStrictEqual(tab.closing, true,
> +    `Double click the selected tab won't close it`);

nit: use double quotes

::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:31
(Diff revision 5)
> +  ]});
> +
> +  let promise = BrowserTestUtils.waitForNewTab(gBrowser, "about:mozilla");
> +  BrowserTestUtils.addTab(gBrowser, "about:mozilla");
> +  let tab = await promise;
> +  Assert.notEqual(tab, gBrowser.selectedTab,

isnot(...);

::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:37
(Diff revision 5)
> +    `The new tab is in the background`);
> +
> +  promise = BrowserTestUtils.waitForEvent(tab, "dblclick");
> +  triggerDblclickOn(tab);
> +  await promise;
> +  Assert.equal(tab, gBrowser.selectedTab,

is(...);

::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:43
(Diff revision 5)
> +    `Double click a background tab will select it`);
> +
> +  promise = BrowserTestUtils.waitForEvent(tab, "dblclick");
> +  triggerDblclickOn(tab);
> +  await promise;
> +  Assert.strictEqual(tab.closing, true,

ok(tab.closing, ...);

	Dão Gottwald [:dao]
	Comment 40 • 6 years ago
mozreview-review

Comment on attachment 8956370 [details]
Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick.

https://reviewboard.mozilla.org/r/225230/#review234188

::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:47
(Diff revision 5)
> +  await promise;
> +  Assert.strictEqual(tab.closing, true,
> +    `Double click the selected tab will close it`);
> +
> +  await BrowserTestUtils.tabRemoved(tab);
> +  await SpecialPowers.popPrefEnv();

This will happen automatically after the test: https://searchfox.org/mozilla-central/rev/6e96a3f1e44e286ddae5fdafab737709741d237a/testing/mochitest/browser-test.js#740

	Hector Zhao [:hectorz] Assignee
	Comment 44 • 6 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=92899f59174aecd3a0b2e7e735d073c8f43667b7

Failures of M-e10s(cl) are results of using `./mach try fuzzy` with paths, I've filed bug 1446884 for that.

	Hector Zhao [:hectorz] Assignee
	Comment 45 • 6 years ago

I just saw bug 1442465, not sure if that would be on autoland when my patches land.

Not sure what's the right thing to do here, I'm attaching a fix for the conflict as tradtional patch here and flagging :dao for review.

	Hector Zhao [:hectorz] Assignee
	Comment 46 • 6 years ago

Comment on attachment 8960100 [details] [diff] [review]
conflict-with-bug-1442465.patch

Review of attachment 8960100 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Hector Zhao [:hectorz] from comment #45)
> 
> I just saw bug 1442465, not sure if that would be on autoland when my
> patches land.
> 
> Not sure what's the right thing to do here, I'm attaching a fix for the
> conflict as tradtional patch here and flagging :dao for review.

Okay, bug 1442465 was now in autoland, I'll just merge this into part 2.

	Hector Zhao [:hectorz] Assignee
	Comment 50 • 6 years ago

Please see comment 44 for the try push. The patches were rebased onto current m-c to resolve the conflict with bug 1442465.

	Pulsebot
	Comment 51 • 6 years ago

Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/069febd18b9e
Part 1: pref to enable close selected tab by dblclicking it. r=dao
https://hg.mozilla.org/integration/autoland/rev/e9ddfb08a712
Part 2: add a browser mochitest for closeTabByDblclick. r=dao
https://hg.mozilla.org/integration/autoland/rev/bcde232543f8
Part 3: expose closeTabByDblclick to WebExtension. r=mixedpuppy

	Dorel Luca [:dluca]
	Comment 52 • 6 years ago

Backed out 3 changesets (bug 1435142) for Mochitest failure on browser/base/content/test/tabs/browser_close_tab_by_dblclick.js

Log of the fail:
https://treeherder.mozilla.org/logviewer.html#?job_id=168949991&repo=autoland&lineNumber=2676

Backout:
https://hg.mozilla.org/integration/autoland/rev/ea5c6ffc930ecf532368ac533f002367c1905dea

Push that got backed out:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=bcde232543f89e90211e4eb7d760c2fe53503030

	Hector Zhao [:hectorz] Assignee
	Comment 53 • 6 years ago

Oops, sorry, just realized I "can finish without waiting for the closing tabs" with changes in https://hg.mozilla.org/mozilla-central/diff/aa1497ed75e5/testing/mochitest/browser-test.js. Will push the updated commits soon.

	Hector Zhao [:hectorz] Assignee
	Comment 57 • 6 years ago

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d83b5fa7d3d96486f491e27778c6354be0f0bea

Failures of test-android-4.3-arm7-api-16/opt-test-verify look like bug 1442503.

	Pulsebot
	Comment 58 • 6 years ago

Pushed by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/79b72c904600
Part 1: pref to enable close selected tab by dblclicking it. r=dao
https://hg.mozilla.org/integration/autoland/rev/ff4b0af1e773
Part 2: add a browser mochitest for closeTabByDblclick. r=dao
https://hg.mozilla.org/integration/autoland/rev/9193e64cfe29
Part 3: expose closeTabByDblclick to WebExtension. r=mixedpuppy

	Noemi Erli[:noemi_erli]
	Comment 59 • 6 years ago

Backed out 3 changesets (bug 1435142) for failing in browser/base/content/test/tabs/browser_close_tab_by_dblclick.js
Push with the failures: 
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9193e64cfe29d8df8e30d957b4f58785389d9ed5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=success&filter-resultStatus=running

Failure log example: https://treeherder.mozilla.org/logviewer.html#?job_id=169173974&repo=autoland&lineNumber=2121

Backout: https://hg.mozilla.org/integration/autoland/rev/c7d3eda7dedcfd0d3ec6840d55d403eabec89633

	Shane Caraveo (:mixedpuppy)
	Comment 60 • 6 years ago
mozreview-review

Comment on attachment 8956370 [details]
Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick.

https://reviewboard.mozilla.org/r/225230/#review235110

::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:40
(Diff revisions 7 - 8)
>    is(tab, gBrowser.selectedTab, "Double click a background tab will select it");
>  
>    promise = BrowserTestUtils.waitForEvent(tab, "dblclick");
>    triggerDblclickOn(tab);
>    await promise;
>    ok(tab.closing, "Double click the selected tab will close it");

Please wait for the tab to be closed.  

await BrowserTestUtils.tabRemoved(tab);

	Shane Caraveo (:mixedpuppy)
	Comment 61 • 6 years ago
mozreview-review

Comment on attachment 8956370 [details]
Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick.

https://reviewboard.mozilla.org/r/225230/#review235114

::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:8
(Diff revision 8)
> +function triggerDblclickOn(target) {
> +  EventUtils.synthesizeMouseAtCenter(target, { clickCount: 1 });
> +  EventUtils.synthesizeMouseAtCenter(target, { clickCount: 2 });
> +}

Change to:

let promise = BrowserTestUtils.waitForEvent(tab, "dblclick");
EventUtils.synthesizeMouseAtCenter(target, { clickCount: 1 });
EventUtils.synthesizeMouseAtCenter(target, { clickCount: 2 });
return promise;

	Hector Zhao [:hectorz] Assignee
	Comment 62 • 6 years ago
mozreview-review-reply

Comment on attachment 8956370 [details]
Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick.

https://reviewboard.mozilla.org/r/225230/#review235110

> Please wait for the tab to be closed.  
> 
> await BrowserTestUtils.tabRemoved(tab);

Thanks for helping me with diagnosing this. But tabRemoved was removed in bug 1442465, and its replacement waitForSessionStoreUpdate caused failures in yesterday’s first attempt.

	Hector Zhao [:hectorz] Assignee
	Comment 66 • 6 years ago
mozreview-review-reply

Comment on attachment 8956370 [details]
Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick.

https://reviewboard.mozilla.org/r/225230/#review235110

> Thanks for helping me with diagnosing this. But tabRemoved was removed in bug 1442465, and its replacement waitForSessionStoreUpdate caused failures in yesterday’s first attempt.

I removed the wait of `tabRemoved` in my second attempt, because with the changes in bug 1442465, mochitest stops complaining about "Found an unexpected tab at the end of test run" as long as `tab.closing` is true.

Failures in the second attempt are time outs after "The new tab is in the background". My current guess is the animation for adding a new tab is interfering with subsequent dblclick events and I've managed to eliminate failures of `browser_close_tab_by_dblclick.js` by waiting until `tab._fullyOpen` is set (try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ad202981a61beaae6f68d8d581e27cf624bc419).

However there're multiple M-e10s(bc3) failures on Windows (tracked in bug 1397098), I don't see how my patches might make it happen more frequently, but I'm not so sure, especially after two backouts.

	Shane Caraveo (:mixedpuppy)
	Comment 67 • 6 years ago
mozreview-review

Comment on attachment 8956370 [details]
Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick.

https://reviewboard.mozilla.org/r/225230/#review235474

::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:26
(Diff revision 9)
> +  let tab = BrowserTestUtils.addTab(gBrowser, "about:mozilla");
> +  await BrowserTestUtils.waitForCondition(() => tab._fullyOpen);

waitforcondition should not be necessary, and this is rather hacky.

You could try:

let tab = await BrowserTestUtils.waitForNewTab(gBrowser, "about:mozilla", true);

or maybe:

let tab = BrowserTestUtils.addTab(gBrowser, "about:mozilla", { skipAnimation: true });
await BrowserTestUtils.browserLoaded(tab.linkedBrowser);

	Shane Caraveo (:mixedpuppy)
	Comment 68 • 6 years ago

You shouldn't worry about the BC failures from Bug 1397098.

	Hector Zhao [:hectorz] Assignee
	Comment 72 • 6 years ago
mozreview-review-reply

Comment on attachment 8956370 [details]
Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick.

https://reviewboard.mozilla.org/r/225230/#review235474

> waitforcondition should not be necessary, and this is rather hacky.
> 
> You could try:
> 
> let tab = await BrowserTestUtils.waitForNewTab(gBrowser, "about:mozilla", true);
> 
> or maybe:
> 
> let tab = BrowserTestUtils.addTab(gBrowser, "about:mozilla", { skipAnimation: true });
> await BrowserTestUtils.browserLoaded(tab.linkedBrowser);

I assume you said waitForCondition is "hacky" because it's polling. I don't want to use `await BrowserTestUtils.browserLoaded(tab.linkedBrowser)` as that feels like "should wait for some other thing, and waiting for ... was longer enough for the other thing" as discouraged in bug 1442465, and "transionend" turns out to be a more accurate signal of the end of said "animation" I'm concerned.

	Hector Zhao [:hectorz] Assignee
	Comment 73 • 6 years ago

Try run with patches from earlier today: https://treeherder.mozilla.org/#/jobs?repo=try&revision=433210ed64d404041b10abd92a4119dddfa60d80&group_state=expanded

It's part of bc7 on Win 7, and part of bc2 on Win 10 x64.

	Shane Caraveo (:mixedpuppy)
	Comment 74 • 6 years ago

(In reply to Hector Zhao [:hectorz] from comment #72)
> Comment on attachment 8956370 [details]
> Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick.
> 
> https://reviewboard.mozilla.org/r/225230/#review235474
> 
> > waitforcondition should not be necessary, and this is rather hacky.
> > 
> > You could try:
> > 
> > let tab = await BrowserTestUtils.waitForNewTab(gBrowser, "about:mozilla", true);
> > 
> > or maybe:
> > 
> > let tab = BrowserTestUtils.addTab(gBrowser, "about:mozilla", { skipAnimation: true });
> > await BrowserTestUtils.browserLoaded(tab.linkedBrowser);
> 
> I assume you said waitForCondition is "hacky" because it's polling. I don't
> want to use `await BrowserTestUtils.browserLoaded(tab.linkedBrowser)` as
> that feels like "should wait for some other thing, and waiting for ... was
> longer enough for the other thing" as discouraged in bug 1442465, and
> "transionend" turns out to be a more accurate signal of the end of said
> "animation" I'm concerned.

It was hacky because the polling and because you're digging into the internals which is not necessary because the right functionality is provided in BTU.  BrowserTestUtils.browserLoaded handles this just fine, waitForNewTab is better as it does that internally.  These are used in tons of tests that need to wait for the tab to be accessible.

	Dão Gottwald [:dao]
	Comment 75 • 6 years ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #74)
> > I assume you said waitForCondition is "hacky" because it's polling. I don't
> > want to use `await BrowserTestUtils.browserLoaded(tab.linkedBrowser)` as
> > that feels like "should wait for some other thing, and waiting for ... was
> > longer enough for the other thing" as discouraged in bug 1442465, and
> > "transionend" turns out to be a more accurate signal of the end of said
> > "animation" I'm concerned.
> 
> It was hacky because the polling and because you're digging into the
> internals which is not necessary because the right functionality is provided
> in BTU.  BrowserTestUtils.browserLoaded handles this just fine,
> waitForNewTab is better as it does that internally.  These are used in tons
> of tests that need to wait for the tab to be accessible.

This test shouldn't care about the content being loaded. From what I can tell it should open a tab without the animation and then go on without waiting for anything.

	Hector Zhao [:hectorz] Assignee
	Comment 76 • 6 years ago

(In reply to Dão Gottwald [::dao] from comment #75)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #74)
> > 
> > It was hacky because the polling and because you're digging into the
> > internals which is not necessary ...

I see.

> 
> This test shouldn't care about the content being loaded. From what I can
> tell it should open a tab without the animation and then go on without
> waiting for anything.

Thanks for the advice.

	Shane Caraveo (:mixedpuppy)
	Comment 77 • 6 years ago
mozreview-review

Comment on attachment 8956370 [details]
Bug 1435142 - Part 2: add a browser mochitest for closeTabByDblclick.

https://reviewboard.mozilla.org/r/225230/#review235804

::: browser/base/content/test/tabs/browser_close_tab_by_dblclick.js:26
(Diff revision 10)
> +  let tab = BrowserTestUtils.addTab(gBrowser, "about:mozilla");
> +  await BrowserTestUtils.waitForEvent(tab, "transitionend", false, event => {
> +    return event.propertyName == "max-width";
> +  });

Given comments from dao and myself, you should just add the skipAnimation flag and remove any wait after addTab.

	Hector Zhao [:hectorz] Assignee
	Comment 81 • 6 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=939f89fdbe2f1cacca2557b03c359e2525918a0e&group_state=expanded

Failure of TV on Android is bug 1442503;
Failures of M-e10s(bc1) on Win 7 and M-e10s(bc7) on Win 10 x64 are bug 1397098.

	Hector Zhao [:hectorz] Assignee
	Comment 82 • 6 years ago

(In reply to Hector Zhao [:hectorz] from comment #81)
> Try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=939f89fdbe2f1cacca2557b03c359e2525918a0e&group_state=e
> xpanded

Retrigger of build-signing-win32-pgo/opt doesn't seem to work properly, so I did another try push for that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e14d13f842ba4594443b40157ec5a8fe63e728dd&group_state=expanded

	Pulsebot
	Comment 83 • 6 years ago

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1505855fc858
Part 1: pref to enable close selected tab by dblclicking it. r=dao
https://hg.mozilla.org/integration/autoland/rev/cd802f66b7aa
Part 2: add a browser mochitest for closeTabByDblclick. r=dao,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/7334da12de35
Part 3: expose closeTabByDblclick to WebExtension. r=mixedpuppy

	Narcis Beleuzu [:NarcisB]
	Comment 84 • 6 years ago

Backed out for browser-chrome failures on browser_preloadedBrowser_zoom.js

Backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8b7876354e566bfba91874af329bd167b5e78067
Push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7334da12de35917bb8d66f31677d1f8595f9ce63
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=169933219&repo=autoland&lineNumber=2685

	Shane Caraveo (:mixedpuppy)
	Comment 85 • 6 years ago

browser_preloadedBrowser_zoom.js was disabled in bug 1397098 due to failures in it.  I don't think the patches here are a cause of that problem.

	Hector Zhao [:hectorz] Assignee
	Comment 86 • 6 years ago

(In reply to Narcis Beleuzu [:NarcisB] from comment #84)
> Backed out for browser-chrome failures on browser_preloadedBrowser_zoom.js

Thanks to :arai's tireless work over the weekend, bug in browser_preloadedBrowser_zoom.js was identified and fixed.

:NarcisB, Can you please clarify when a patch would be backed out with failures in a known intermittent test? Thanks.

	Pulsebot
	Comment 87 • 6 years ago

Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f6615d8ddb5d
Part 1: pref to enable close selected tab by dblclicking it. r=dao
https://hg.mozilla.org/integration/autoland/rev/46bbc788a3d4
Part 2: add a browser mochitest for closeTabByDblclick. r=dao,mixedpuppy
https://hg.mozilla.org/integration/autoland/rev/a27611cba7d7
Part 3: expose closeTabByDblclick to WebExtension. r=mixedpuppy

	Narcis Beleuzu [:NarcisB]
	Comment 88 • 6 years ago

(In reply to Hector Zhao [:hectorz] from comment #86)
> (In reply to Narcis Beleuzu [:NarcisB] from comment #84)
> > Backed out for browser-chrome failures on browser_preloadedBrowser_zoom.js
> 
> Thanks to :arai's tireless work over the weekend, bug in
> browser_preloadedBrowser_zoom.js was identified and fixed.
> 
> :NarcisB, Can you please clarify when a patch would be backed out with
> failures in a known intermittent test? Thanks.

I did that because I noticed that the test started to almost-perma fail starting with that push.
The fail was on "browser/base/content/test/tabs/" and on cd802f66b7aa there were changes made there.
Also, seems that after that backout, the test didn`t fail anymore.
https://treeherder.mozilla.org/#/jobs?repo=autoland&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=runnable&filter-resultStatus=success&filter-searchStr=dbfc7212166852e040b8acb6c02ef28fcef45e44&fromchange=cbcc7b902237d350cc78ca9b3cd2d04abbf2e7d5&tochange=e4e466004d34b06fec1e4abddb0f537f9e74493c

	Hector Zhao [:hectorz] Assignee
	Comment 89 • 6 years ago

(In reply to Narcis Beleuzu [:NarcisB] from comment #88)
> 
> I did that because I noticed that the test started to almost-perma fail
> starting with that push.
> The fail was on "browser/base/content/test/tabs/" and on cd802f66b7aa there
> were changes made there.
> Also, seems that after that backout, the test didn`t fail anymore.

Even with fix in in bug 1397098, I couldn't see how my patch will make it more likely to fail. But I can understand the rationale behind the backout now, thanks for the explanation.

	Natalia Csoregi [:nataliaCs]
	Comment 90 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/f6615d8ddb5d
https://hg.mozilla.org/mozilla-central/rev/46bbc788a3d4
https://hg.mozilla.org/mozilla-central/rev/a27611cba7d7

	Hector Zhao [:hectorz] Assignee
	Comment 92 • 6 years ago

(In reply to OrangeFactor Robot from comment #91)
> 
> For more details, see:
> https://treeherder.mozilla.org/intermittent-failures.html#/
> bugdetails?bug=1435142&startday=2018-03-20&endday=2018-03-26&tree=trunk

Looks like this was meant to be bug 1432430

	Yanfang Liu
	Comment 93 • 6 years ago

Verified with Nightly 61.0a1 build 20180327220126 on Windows 10 64 bit.

	Revertron
	Comment 95 • 6 years ago

It would be very helpful to hide close button if we enable closeTabByDblclic.
Or introduce a different preference for that at least.

	Irene Smith
	Comment 96 • 5 years ago

Added page for the property, and created a pull request for BCD

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browserSettings/browserSettings.closeTabsByDoubleClick

https://github.com/mdn/browser-compat-data/pull/3237

	Hector Zhao [:hectorz] Assignee
	Comment 97 • 5 years ago

Hi Irene, thanks for documenting this!

(In reply to Irene Smith from comment #96)
> Added page for the property, and created a pull request for BCD
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/
> browserSettings/browserSettings.closeTabsByDoubleClick

You might want to make it more clear this option only applies to the current selected tab.

> 
> https://github.com/mdn/browser-compat-data/pull/3237

This doens't really work on Fx for Android.