Last Comment Bug 1300784 - Combine e10s and non-e10s <select> dropdown mechanisms
: Combine e10s and non-e10s <select> dropdown mechanisms
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: 50 Branch
: Unspecified Unspecified
P1 normal (vote)
: mozilla53
Assigned To: Michael
:
: Jet Villegas (:jet)
Mentors: Jared Wein [:jaws] (please needinfo? me)
Mike Conley (:mconley)
: 1296270 1309937 1313132 1313418 (view as bug list)
Depends on: 1315132 1313195 1314798 1314802 1316722 1321376
Blocks: e10s-select 1296264 1300483 1318595 1329991 1331725 1091592 1296270 1316404 1316405 1316406
  Show dependency treegraph
 
Reported: 2016-09-06 07:21 PDT by Mike Conley (:mconley)
Modified: 2017-02-06 09:48 PST (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
Bug 1300784 - Update single-process firefox to implement new e10s select-dropdown (58 bytes, text/x-review-board-request)
2016-10-01 13:32 PDT, Michael
no flags Details | Review
Bug 1300784 - Update single-process firefox to implement new e10s select-dropdown (58 bytes, text/x-review-board-request)
2016-10-31 21:03 PDT, Michael
no flags Details | Review
Bug 1300784 - Update single-process firefox to implement new e10s select-dropdown (58 bytes, text/x-review-board-request)
2016-10-31 21:26 PDT, Michael
mconley: review-
Details | Review
Bug 1300784 - Update single-process firefox to implement new e10s select-dropdown (58 bytes, text/x-review-board-request)
2016-11-02 13:55 PDT, Michael
no flags Details | Review
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off. (58 bytes, text/x-review-board-request)
2016-11-02 14:33 PDT, Michael
mconley: review-
Details | Review
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off. (59 bytes, text/x-review-board-request)
2017-01-06 13:16 PST, Mike Conley (:mconley)
mconley: review+
Details | Review
Bug 1300784 - Follow-up: update browser_selectpopup.js to handle floating point values for bounding client rects. (59 bytes, text/x-review-board-request)
2017-01-06 13:16 PST, Mike Conley (:mconley)
jaws: review+
Details | Review
Bug 1300784 - Follow-up: Disable browser_selectpopup.js for non-e10s Linux debug builds for failing intermittently. (59 bytes, text/x-review-board-request)
2017-01-10 08:02 PST, Mike Conley (:mconley)
mconley: review+
Details | Review

Description User image Mike Conley (:mconley) 2016-09-06 07:21:03 PDT
We currently use two mechanisms for <select> dropdowns in Firefox. We have the legacy mechanism, and the newer mechanism which supports multi-process mode.

We should use the multi-process mechanism, even in single-process mode, so that we only have one codepath to worry about.
Comment 1 User image Mike Conley (:mconley) 2016-09-06 07:21:18 PDT
jaws and I are mentoring some students from MSU that will work on this.
Comment 2 User image Mats Palmgren (:mats) 2016-09-06 14:53:07 PDT
I agree that having one code path for this would be good.
But the current designs both have issues...  They both require a widget
for example.  Historically, they have also been fragile and prone to
security issues, such as overlapping the URL bar / other tabs.

I think a much simpler and robust solution would be to render the dropdown
menu as an ordinary abs.pos. frame within the page, using a 'z-index' that
is guaranteed to be on top of everything else in the page (we can reserve
a range of high 'z-index' values for this purpose).  This means the dropdown
can't overlap anything outside the viewport *by design*.  It also won't
require a view/widget.
Comment 3 User image Michael 2016-10-01 13:32:15 PDT Comment hidden (mozreview-request)
Comment 4 User image Michael 2016-10-14 08:35:36 PDT Comment hidden (mozreview-request)
Comment 5 User image Jim Mathies [:jimm] 2016-10-14 08:55:06 PDT
Comment on attachment 8796877 [details]
Bug 1300784 - Update single-process firefox to implement new e10s select-dropdown

https://reviewboard.mozilla.org/r/82588/#review84574

::: layout/forms/nsListControlFrame.cpp:144
(Diff revision 2)
>    mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mouseup"),
>                                        mEventListener, false);
>    mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mousemove"),
>                                        mEventListener, false);
>  
> -  if (XRE_IsContentProcess() &&
> +  if (Preferences::GetBool("browser.tabs.remote.desktopbehavior", false)) {

Note, this 'desktopbehavior' pref was added to differentiate between desktop and b2g select behaviors. We can remove this now that b2g is dead and just assume desktopbehavior.
Comment 6 User image Mike Conley (:mconley) 2016-10-14 10:27:23 PDT
Note that miguel and Fred are both working on this bug, but assigning to miguel since his machine seems to be doing the review requests.
Comment 7 User image Mike Conley (:mconley) 2016-10-14 10:27:50 PDT
Hey miguel / Fred,

Which test was the one that was failing? I want to point Enn at it so we can figure out what's going wrong.
Comment 8 User image Jared Wein [:jaws] (please needinfo? me) 2016-10-14 12:45:25 PDT
*** Bug 1309937 has been marked as a duplicate of this bug. ***
Comment 9 User image Michael 2016-10-14 12:52:06 PDT Comment hidden (mozreview-request)
Comment 10 User image Mike Conley (:mconley) 2016-10-14 13:34:09 PDT Comment hidden (obsolete)
Comment 11 User image Michael 2016-10-14 15:05:11 PDT
test_mousemove_correcttarget() from browser/base/content/test/general/browser_selectpopup.js fails in non e10s with this addition, I can reproduce this manually.
Comment 12 User image Mike Conley (:mconley) 2016-10-14 15:06:43 PDT
Hey Neil,

I think you wrote (and I reviewed) the test_mousemove_correctarget test a few months back. Any idea why it'd fail when using the ContentSelectDropdown in the single-process case?
Comment 13 User image Neil Deakin 2016-10-17 13:14:55 PDT
The test is checking that the mousecapture that gets applied when a mouse event is redispatched to the content process is cleared when the select popup is opened.

While the specific mouse capture being tested doesn't apply to non-e10s, the code in the test suggests that it should still work in non-e10s. I suspect it might be some code which prevents mousedown/mousemove/mouseup events from dispatching across different frames. I can't recall where this happens; probably in EventStateManager or PresShell.
Comment 14 User image Mike Conley (:mconley) 2016-10-17 15:08:15 PDT
(In reply to Neil Deakin from comment #13)
> The test is checking that the mousecapture that gets applied when a mouse
> event is redispatched to the content process is cleared when the select
> popup is opened.
> 
> While the specific mouse capture being tested doesn't apply to non-e10s, the
> code in the test suggests that it should still work in non-e10s. I suspect
> it might be some code which prevents mousedown/mousemove/mouseup events from
> dispatching across different frames. I can't recall where this happens;
> probably in EventStateManager or PresShell.

Thanks Enn. felipe, do you happen to know the area of ESM that Enn is referring to that prevents mouse events from crossing frames?
Comment 15 User image :Felipe Gomes (needinfo me!) 2016-10-17 18:42:34 PDT
Hmm no, not off the top of my head
Comment 16 User image Michael 2016-10-18 06:52:37 PDT Comment hidden (mozreview-request)
Comment 17 User image Mike Conley (:mconley) 2016-10-19 15:25:49 PDT
Hey Fred, miguel - is Enn's change from bug 1159301 in nsMenuPopupFrame.cpp running in the non-e10s case after the select dropdown opens?
Comment 18 User image Mike Conley (:mconley) 2016-10-28 10:16:38 PDT
*** Bug 1313132 has been marked as a duplicate of this bug. ***
Comment 19 User image Neil Deakin 2016-10-31 06:47:39 PDT
*** Bug 1313418 has been marked as a duplicate of this bug. ***
Comment 20 User image Michael 2016-10-31 21:03:49 PDT Comment hidden (mozreview-request)
Comment 21 User image Michael 2016-10-31 21:10:44 PDT
Comment on attachment 8806214 [details]
Bug 1300784 - Update single-process firefox to implement new e10s select-dropdown

https://reviewboard.mozilla.org/r/89698/#review89128

::: layout/forms/nsListControlFrame.cpp:144
(Diff revision 1)
>    mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mouseup"),
>                                        mEventListener, false);
>    mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mousemove"),
>                                        mEventListener, false);
>  
> -  if (Preferences::GetBool("browser.tabs.remote.desktopbehavior", false)) {
> +  if (XRE_IsContentProcess() &&

I believe mconley helped us to decide to keep XRE_IsContentProcess() in here, though that might have just been around 1788
Comment 22 User image Michael 2016-10-31 21:26:46 PDT Comment hidden (mozreview-request)
Comment 23 User image Mike Conley (:mconley) 2016-11-01 07:24:04 PDT
Comment on attachment 8806218 [details]
Bug 1300784 - Update single-process firefox to implement new e10s select-dropdown

https://reviewboard.mozilla.org/r/89700/#review89276

Hey Fred\_, miguel,

This looks pretty good, but I think there are a few things missing:

1. I browser_selectpopup.js needs to be enabled to work in non-e10s mode. That means removing [this rule](http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/browser/base/content/test/general/browser.ini#410) and then adding this before any other `add_task` in browser_selectpopup.js:

```JavaScript
add_task(function* setup() {
  yield SpecialPowers.pushPrefEnv({
    "set": [
      ["dom.select_popup_in_parent.enabled", true],
    ]
  });
});

```

This will automatically clear the pref once the test file exits.

2. See my notes below about adding a new static method for checking to see if we should be firing events or not.

Thanks for your work! Glad to see this taking shape.

::: layout/forms/nsListControlFrame.cpp:144
(Diff revision 1)
> -  if (XRE_IsContentProcess() &&
> +  if (Preferences::GetBool("browser.tabs.remote.desktopbehavior", false)) {
> -      Preferences::GetBool("browser.tabs.remote.desktopbehavior", false)) {

Hm. I don't think I understand why we're only checking `browser.tabs.remote.desktopbehavior` here. We probably should fire this if we're supposed to be firing events as opposed to using the old-school widget dropdown.

Perhaps part of the problem is that FireShowDropDownEvent actually has two purposes: determine if we should fire events, fire events if necessary, and return a `bool` saying whether or not events were necessary.

Maybe it'd be more useful to split that up, and expose a new static method that determines whether or not we should be firing events or using the old-school widget dropdown.

And then have all callers of `FireShowDropDownEvent` call that static method. If it return strue, have them then call `FireShowDropDownEvent`. Otherwise, do what they were doing when `FireShowDropDownEvent` returned `false`.

That way, here, you can just check your new static method instead of us duplicating all of the checking logic all over the place.

::: layout/forms/nsListControlFrame.cpp:1782
(Diff revision 1)
>    if (XRE_IsContentProcess() &&
> -      Preferences::GetBool("browser.tabs.remote.desktopbehavior", false)) {
> +      Preferences::GetBool("browser.tabs.remote.desktopbehavior", false) ||
> +      Preferences::GetBool("dom.select_popup_in_parent.enabled"))

I think with [C++ Operator Precedence](http://en.cppreference.com/w/cpp/language/operator_precedence), this is correct, but making the readers care about knowing operator precedence is asking for trouble.

I think it'd be clearer if it was:

```cpp
if ((XRE_IsContentProcess() &&
     Preferences::GetBool("browser.tabs.remote.desktopbehaviour", false)) ||
    Preferences::GetBool("dom.select_popup_in_parent.enabled")) {
```

And I guess that would go into the static function I suggested above.

::: modules/libpref/init/all.js:22
(Diff revision 1)
>   *  - Dashes are delimiters; use underscores instead.
>   *  - The first character after a period must be alphabetic.
>   *  - Computed values (e.g. 50 * 1024) don't work.
>   */
>  
> +pref("dom.select_popup_in_parent.enabled", false);

There's a DOM section that starts around line 120 - let's put this at the end of that section.

::: toolkit/content/browser-content.js:18
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "SelectContentHelper",
> +                                  "resource://gre/modules/SelectContentHelper.jsm");

Let's keep the formatting that's been established here - so let's un-indent line 19 so that it's only 2-spaced.

::: toolkit/content/browser-content.js:1727
(Diff revision 1)
> +addEventListener("mozshowdropdown", event => {
> +  if (!event.isTrusted)
> +    return;
> +
> +  if (!SelectContentHelper.open) {
> +    new SelectContentHelper(event.target, this);
> +  }
> +});

What about mozhidedropdown? And mozshowdropdown-sourcetouch? Those were in the select-child.js that you removed.

::: toolkit/content/widgets/browser.xml:1022
(Diff revision 1)
>                this.audioPlaybackStarted();
>                break;
>              case "AudioPlayback:Stop":
>                this.audioPlaybackStopped();
>                break;
> +              

Trailing whitespace.

::: toolkit/content/widgets/remote-browser.xml:463
(Diff revision 1)
>              case "Forms:ShowDropDown": {
>                if (!this._selectParentHelper) {
>                  this._selectParentHelper =
>                    Cu.import("resource://gre/modules/SelectParentHelper.jsm", {}).SelectParentHelper;
>                }
>  
>                let menulist = document.getElementById(this.getAttribute("selectmenulist"));
>                menulist.menupopup.style.direction = data.direction;
>                this._selectParentHelper.populate(menulist, data.options, data.selectedIndex, this._fullZoom);
>                this._selectParentHelper.open(this, menulist, data.rect, data.isOpenedViaTouch);
>                break;
>              }

Need to remove this too, no?
Comment 24 User image Jared Wein [:jaws] (please needinfo? me) 2016-11-02 11:15:42 PDT
Comment on attachment 8806218 [details]
Bug 1300784 - Update single-process firefox to implement new e10s select-dropdown

https://reviewboard.mozilla.org/r/89700/#review89780

Mike's review here is sufficient.
Comment 25 User image Michael 2016-11-02 13:50:05 PDT
Comment on attachment 8806218 [details]
Bug 1300784 - Update single-process firefox to implement new e10s select-dropdown

https://reviewboard.mozilla.org/r/89700/#review89832

::: layout/forms/nsListControlFrame.cpp:144
(Diff revision 1)
> -  if (XRE_IsContentProcess() &&
> +  if (Preferences::GetBool("browser.tabs.remote.desktopbehavior", false)) {
> -      Preferences::GetBool("browser.tabs.remote.desktopbehavior", false)) {

I like this idea a lot. I'm confused by you saying:

>  have all callers of `FireShowDropDownEvent` call that static method.

That sounds to me like this is a caller of `FireShowDropDownEvent`... or are you talking about methods like `DropDownToggleKey` that make checks against `FireShowDropDownEvent`: 

```
if (!FireShowDropDownEvent(mContent, true, false)) {

```
As so?
Comment 26 User image Michael 2016-11-02 13:55:29 PDT Comment hidden (mozreview-request)
Comment 27 User image Michael 2016-11-02 14:33:04 PDT Comment hidden (mozreview-request)
Comment 28 User image Mike Conley (:mconley) 2016-11-02 15:19:33 PDT
(In reply to Michael from comment #25)
> Comment on attachment 8806218 [details]
> Bug 1300784 - Update single-process firefox to implement new e10s
> select-dropdown
> 
> https://reviewboard.mozilla.org/r/89700/#review89832
> 
> ::: layout/forms/nsListControlFrame.cpp:144
> (Diff revision 1)
> > -  if (XRE_IsContentProcess() &&
> > +  if (Preferences::GetBool("browser.tabs.remote.desktopbehavior", false)) {
> > -      Preferences::GetBool("browser.tabs.remote.desktopbehavior", false)) {
> 
> I like this idea a lot. I'm confused by you saying:
> 
> >  have all callers of `FireShowDropDownEvent` call that static method.
> 
> That sounds to me like this is a caller of `FireShowDropDownEvent`... or are
> you talking about methods like `DropDownToggleKey` that make checks against
> `FireShowDropDownEvent`: 
> 
> ```
> if (!FireShowDropDownEvent(mContent, true, false)) {
> 
> ```
> As so?

Sorry, I should have been more clear. As discussed in IRC, we want to have this method called anytime we need to decide whether or not to fire an event (like moz[show|hide]dropdown), or work with the built-in old-school widget. See DestroyFrom, for example.
Comment 29 User image Mike Conley (:mconley) 2016-11-04 12:42:16 PDT
Comment on attachment 8806910 [details]
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off.

https://reviewboard.mozilla.org/r/90176/#review90478

A number of my previous review comments still stand and haven't been addressed, so r-'ing.
Comment 30 User image Michael 2016-11-09 09:26:00 PST Comment hidden (mozreview-request)
Comment 31 User image Michael 2016-11-09 09:27:47 PST
Comment on attachment 8806910 [details]
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off.

https://reviewboard.mozilla.org/r/90176/#review91620

::: layout/forms/nsListControlFrame.cpp:144
(Diff revision 2)
>    mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mouseup"),
>                                        mEventListener, false);
>    mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mousemove"),
>                                        mEventListener, false);
>  
> -  if (XRE_IsContentProcess() &&
> +  if (FireDropDownEvent()) {

this seemed to be an appropriate, but maybe confusing name. @mconley,what do you think?
Comment 32 User image Jared Wein [:jaws] (please needinfo? me) 2016-11-09 14:44:38 PST
Comment on attachment 8806910 [details]
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off.

https://reviewboard.mozilla.org/r/90176/#review91770

::: layout/forms/nsListControlFrame.cpp:144
(Diff revision 2)
>    mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mouseup"),
>                                        mEventListener, false);
>    mContent->RemoveSystemEventListener(NS_LITERAL_STRING("mousemove"),
>                                        mEventListener, false);
>  
> -  if (XRE_IsContentProcess() &&
> +  if (FireDropDownEvent()) {

Because this function doesn't actually "fire" the dropdown event, it should be renamed to ShouldFireDropDownEvent()
Comment 33 User image Mike Conley (:mconley) 2016-11-10 08:17:52 PST
Comment on attachment 8806910 [details]
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off.

https://reviewboard.mozilla.org/r/90176/#review91770

> Because this function doesn't actually "fire" the dropdown event, it should be renamed to ShouldFireDropDownEvent()

I concur.
Comment 34 User image Mike Conley (:mconley) 2016-11-10 08:22:25 PST
Comment on attachment 8806910 [details]
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off.

https://reviewboard.mozilla.org/r/90176/#review91990

This is really close! Just one issue, beyond the naming of the static function and some indentation. Otherwise, this looks solid to me. Great work you two!

::: layout/forms/nsListControlFrame.cpp:1780
(Diff revision 2)
> +  return (XRE_IsContentProcess() &&
> +      Preferences::GetBool("browser.tabs.remote.desktopbehavior", false)) ||
> +    Preferences::GetBool("dom.select_popup_in_parent.enabled")

Indentation here is a bit funky. We generally try to indent so that it's clear when something is inside a parenthesis. Example:

```C++
return (XRE_IsContentProcess() &&
        Preferences::GetBool("browser.tabs.remote.desktopbehavior", false)) ||
       Preferences::GetBool("dom.select_popup_in_parent.enabled", false);
```

Please don't forget to pass `false` as the second argument to `GetBool` for the `dom.select_popup_in_parent.enabled` pref, as this argument is supposed to be the "fallback" value in the event that the pref does not exist.
Comment 35 User image Michael 2016-11-13 07:55:41 PST Comment hidden (mozreview-request)
Comment 36 User image Michael 2016-11-13 08:15:45 PST Comment hidden (mozreview-request)
Comment 37 User image Michael 2016-11-13 08:20:09 PST Comment hidden (mozreview-request)
Comment 38 User image Michael 2016-11-13 08:42:42 PST Comment hidden (mozreview-request)
Comment 39 User image Michael 2016-11-13 10:46:55 PST Comment hidden (mozreview-request)
Comment 40 User image Mike Conley (:mconley) 2016-11-13 13:17:38 PST
Comment on attachment 8806910 [details]
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off.

https://reviewboard.mozilla.org/r/90176/#review92588

This looks good to me! Thanks!
Comment 41 User image Mike Conley (:mconley) 2016-11-13 13:19:12 PST
Comment on attachment 8806910 [details]
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off.

https://reviewboard.mozilla.org/r/90176/#review92590

::: layout/forms/nsListControlFrame.cpp
(Diff revision 7)
> -static bool ShouldFireDropDownEvent() {
> -  return (XRE_IsContentProcess() &&
> -          Preferences::GetBool("browser.tabs.remote.desktopbehavior", false)) ||
> -         Preferences::GetBool("dom.select_popup_in_parent.enabled", false);
> -}

Er... actually, I'm wondering if you've made sure that this patch is based off of central. ShouldFireDropDownEvent is something you added, but in this latest patch, it appears that you've moved it.

Are you sure you don't need to fold some commits together?
Comment 42 User image Michael 2016-11-16 09:04:59 PST
Comment on attachment 8806910 [details]
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off.

https://reviewboard.mozilla.org/r/90176/#review93522

::: layout/forms/nsListControlFrame.cpp:121
(Diff revision 7)
>  nsListControlFrame::~nsListControlFrame()
>  {
>    mComboboxFrame = nullptr;
>  }
>  
> +static bool ShouldFireDropDownEvent() {

@mconley, I moved it up here because I started having compile issues, compiler wanted this method defined before it was used in line 150
Comment 43 User image Michael 2016-11-16 09:13:11 PST Comment hidden (mozreview-request)
Comment 44 User image Mike Conley (:mconley) 2016-11-16 13:53:47 PST
Comment on attachment 8806910 [details]
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off.

https://reviewboard.mozilla.org/r/90176/#review93628

I don't think I communicated the problem clearly.

ShouldFireDropDownEvent was something you added as part of this bug.

This patch moves ShouldFireDropDownEvent from one place to another. Applying this patch to central doesn't make sense because ShouldFireDropDownEvent does not exist in the tree. So this patch must be based on something that's not in the list of commits to review.

What we should end up with is a single commit that adds ShouldFireDropDownEvent.

Does that make sense?
Comment 45 User image Michael 2016-11-16 14:57:04 PST Comment hidden (mozreview-request)
Comment 46 User image Michael 2016-11-17 07:08:10 PST Comment hidden (mozreview-request)
Comment 47 User image Mike Conley (:mconley) 2016-11-17 07:14:31 PST
Comment on attachment 8806910 [details]
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off.

https://reviewboard.mozilla.org/r/90176/#review93830

::: toolkit/modules/SelectParentHelper.jsm:11
(Diff revision 10)
> +const {utils: Cu} = Components;
> +const { AppConstants } = Cu.import("resource://gre/modules/AppConstants.jsm");

This doesn't appear to be used. Is this new?
Comment 48 User image Michael 2016-11-21 13:23:03 PST Comment hidden (mozreview-request)
Comment 49 User image Mike Conley (:mconley) 2016-11-21 13:26:40 PST
Comment on attachment 8806910 [details]
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off.

https://reviewboard.mozilla.org/r/90176/#review94702

Looking at the commit log, this looks like it's based on a good commit! Good stuff.

Two things:

1) Please get rid of the unnecessary import of Services.jsm in SelectParentHelper.jsm that was added
2) Please correct the commit message so that it doesn't look like:

```
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off. r?mconley

MozReview-Commit-ID: CkEOBXBfYhj
***
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off. r?mconley

MozReview-Commit-ID: JJTlFgLMM3i
```

and instead looks like:

```
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off. r?mconley

MozReview-Commit-ID: CkEOBXBfYhj
```

Thanks!

::: toolkit/modules/SelectParentHelper.jsm:11
(Diff revision 11)
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +

This is unused.
Comment 50 User image Michael 2016-11-21 16:11:01 PST Comment hidden (mozreview-request)
Comment 51 User image Mike Conley (:mconley) 2016-11-22 09:51:11 PST
Comment on attachment 8806910 [details]
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off.

https://reviewboard.mozilla.org/r/90176/#review94962

Huh - and now this patching is perma-oranging when I push to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d11394a4a493&selectedJob=31668476

Here is a test failure in the log viewer: https://treeherder.mozilla.org/logviewer.html#?job_id=31663456&repo=try

Any idea what's going on?
Comment 52 User image Virtual_ManPL [:Virtual] - (ni? me) 2016-11-24 03:16:24 PST
*** Bug 1313418 has been marked as a duplicate of this bug. ***
Comment 53 User image Mike Conley (:mconley) 2016-11-30 11:53:29 PST
Comment on attachment 8806910 [details]
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off.

https://reviewboard.mozilla.org/r/90176/#review94962

I've figured it out - see bug 1321376.
Comment 56 User image Mike Conley (:mconley) 2017-01-06 13:16:43 PST Comment hidden (mozreview-request)
Comment 57 User image Mike Conley (:mconley) 2017-01-06 13:16:43 PST Comment hidden (mozreview-request)
Comment 58 User image Mike Conley (:mconley) 2017-01-06 13:16:56 PST
Comment on attachment 8824544 [details]
Bug 1300784 - Combine non-e10s and e10s select dropdown implementations, preffed off.

https://reviewboard.mozilla.org/r/102998/#review103554
Comment 59 User image Jared Wein [:jaws] (please needinfo? me) 2017-01-06 14:22:31 PST
Comment on attachment 8824545 [details]
Bug 1300784 - Follow-up: update browser_selectpopup.js to handle floating point values for bounding client rects.

https://reviewboard.mozilla.org/r/103000/#review103572

::: browser/base/content/test/general/browser_selectpopup.js:454
(Diff revision 1)
> +      is(Math.round(selectPopup.childNodes[60].getBoundingClientRect().bottom),
> +         Math.round(selectPopup.getBoundingClientRect().bottom - bpBottom),
>           "Popup scroll at correct position " + bpBottom);

We have the isfuzzy() function which will test that two values are within some epsilon of each other. We should use that here.
Comment 60 User image Mike Conley (:mconley) 2017-01-06 14:41:39 PST
Comment on attachment 8824545 [details]
Bug 1300784 - Follow-up: update browser_selectpopup.js to handle floating point values for bounding client rects.

https://reviewboard.mozilla.org/r/103000/#review103572

> We have the isfuzzy() function which will test that two values are within some epsilon of each other. We should use that here.

Whoa, TIL! Thanks!
Comment 61 User image Mike Conley (:mconley) 2017-01-06 14:45:23 PST Comment hidden (mozreview-request)
Comment 62 User image Mike Conley (:mconley) 2017-01-06 14:46:11 PST
ni'ing myself to land this if we finally get a green try push. Fingers crossed!
Comment 63 User image Mike Conley (:mconley) 2017-01-10 07:18:08 PST
Looks like it's mostly green, but that the browser_selectpopup.js test, when enabled on non-e10s, fails intermittently.

I'm going to just land this thing with the test disabled for non-e10s, and then file a follow-up to get it enabled once we solve the intermittent.
Comment 64 User image Mike Conley (:mconley) 2017-01-10 08:02:54 PST Comment hidden (mozreview-request)
Comment 65 User image Mike Conley (:mconley) 2017-01-10 08:02:54 PST Comment hidden (mozreview-request)
Comment 66 User image Mike Conley (:mconley) 2017-01-10 08:02:54 PST Comment hidden (mozreview-request)
Comment 67 User image Mike Conley (:mconley) 2017-01-10 08:03:22 PST
Comment on attachment 8825420 [details]
Bug 1300784 - Follow-up: Disable browser_selectpopup.js for non-e10s Linux debug builds for failing intermittently.

https://reviewboard.mozilla.org/r/103586/#review104202
Comment 68 User image Pulsebot 2017-01-10 08:04:45 PST
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84e26201a8ee
Combine non-e10s and e10s select dropdown implementations, preffed off. r=mconley
https://hg.mozilla.org/integration/autoland/rev/d75cca55d824
Follow-up: update browser_selectpopup.js to handle floating point values for bounding client rects. r=jaws
https://hg.mozilla.org/integration/autoland/rev/7aca41117662
Follow-up: Disable browser_selectpopup.js for non-e10s Linux debug builds for failing intermittently. r=mconley
Comment 69 User image Pulsebot 2017-01-10 10:23:40 PST
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/c85f27d15031
Follow-up: Disable browser_selectpopup.js on Linux for frequently failing. r=fix-these-oranges
Comment 71 User image Mike Conley (:mconley) 2017-01-19 09:51:59 PST
*** Bug 1296270 has been marked as a duplicate of this bug. ***
Comment 72 User image Sylvestre Ledru [:sylvestre] 2017-01-25 05:19:10 PST
I guess we want this to ride the trains.
Comment 73 User image Justin [:JW_SoftvisionQA] 2017-02-06 09:15:47 PST
Setting the pref dom.forms.selectSearch to true and turning off e10s does not work. Should I reopen this issue or would this call for a new bug that is blocking this one?
Comment 74 User image Jared Wein [:jaws] (please needinfo? me) 2017-02-06 09:21:12 PST
(In reply to Justin [:JW_SoftvisionQA] from comment #73)
> Setting the pref dom.forms.selectSearch to true and turning off e10s does
> not work. Should I reopen this issue or would this call for a new bug that
> is blocking this one?

Please file a new bug and mark it as blocking the search bug (bug 1309935).
Comment 75 User image Mike Conley (:mconley) 2017-02-06 09:48:03 PST
(In reply to Justin [:JW_SoftvisionQA] from comment #73)
> Setting the pref dom.forms.selectSearch to true and turning off e10s does
> not work. Should I reopen this issue or would this call for a new bug that
> is blocking this one?

This is a consequence of us not having fixed bug 1331725, which turns on the menulist-powered <select> dropdown (with the search input) for non-e10s.

Note You need to log in before you can comment on or make changes to this bug.