Closed Bug 1239562 Opened 4 years ago Closed 4 years ago

Intermittent browser_responsiveui.js | Floating scrollbars shouldn't change the width - Got 1280, expected 320

Categories

(DevTools :: Responsive Design Mode, defect)

defect
Not set

Tracking

(firefox46 fixed, firefox47 fixed)

RESOLVED FIXED
Firefox 47
Tracking Status
firefox46 --- fixed
firefox47 --- fixed

People

(Reporter: philor, Assigned: jryans)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Assignee: nobody → jryans
Status: NEW → ASSIGNED
:ntim, planning to adapt your patch with async RDM startup from bug 828008 to resolve this one, just so you are aware.
:ochameau, this ended up a lot larger than I had hoped... :/ There were just many places where the tests were quite racy.
Comment on attachment 8713840 [details]
MozReview Request: Bug 1239562 - Use explicit events to fix test races in responsive design. r=ochameau

https://reviewboard.mozilla.org/r/32837/#review29789

Looks good overall, I just hope we can simplify the notifyResize thing.

::: devtools/client/responsivedesign/responsivedesign-child.js:28
(Diff revision 1)
> +  addMessageListener("ResponsiveMode:NotifyOnResize", notifyOnResize);

Per my previous comment, we could just get rid of this message (NotifyOnResize). It looks weird to have something else than just Start/Stop here. Even Stop could be done in startResponsiveMode.

::: devtools/client/responsivedesign/responsivedesign-child.js:62
(Diff revision 1)
> -  }, false);
> +    }, false);

To be perfect, we could removeEventListener this in stopResponsiveMode...
but it doesn't seem to pull much dependencies in its scope. So it doesn't looks very important at the first look.

::: devtools/client/responsivedesign/responsivedesign.jsm:207
(Diff revision 1)
> +    }

Is it the key part of this patch?
(with the related child.js change)

So, is it important to register the "resize" event listener in the child, before calling ResponsiveMode:Start/startResponsiveMode?
If yes, we should tweak the comment about that.

Otherwise, if not, which seems likely? What about passing another argument to ResponsideMode:Start?
Like requiresFloatingScrollbars. We could go for `testing` or `notifyOnResize`. I don't think we need another custom message just for this.

If that helps, you can delay resolving Start:Done after DOMWindowCreated and the resize registering.

::: devtools/client/responsivedesign/test/browser_responsive_cmd.js:27
(Diff revision 1)
> +          return helpers.setInput(options, "resize toggle");

I don't fully understand helpers API, but can't we resolve setup once `done` is resolve?
I imagine not, there is some action being done in between setup and post?

This `done` global seems weak. But I don't see how to do otherwise.
Attachment #8713840 - Flags: review?(poirot.alex) → review+
https://reviewboard.mozilla.org/r/32837/#review29789

> Per my previous comment, we could just get rid of this message (NotifyOnResize). It looks weird to have something else than just Start/Stop here. Even Stop could be done in startResponsiveMode.

Yep, the separate message is gone now.

> To be perfect, we could removeEventListener this in stopResponsiveMode...
> but it doesn't seem to pull much dependencies in its scope. So it doesn't looks very important at the first look.

At first I was worried there could be some resize event after the "Stop" message that would be missed... but now the close() method yields on the resize message from the last size change before sending "Stop" so it should be fine.

I am now removing the listener.

> Is it the key part of this patch?
> (with the related child.js change)
> 
> So, is it important to register the "resize" event listener in the child, before calling ResponsiveMode:Start/startResponsiveMode?
> If yes, we should tweak the comment about that.
> 
> Otherwise, if not, which seems likely? What about passing another argument to ResponsideMode:Start?
> Like requiresFloatingScrollbars. We could go for `testing` or `notifyOnResize`. I don't think we need another custom message just for this.
> 
> If that helps, you can delay resolving Start:Done after DOMWindowCreated and the resize registering.

I think the "key" part is that we are using yield to block the UI instance from setting the size until after the resize listener is bound.

Since we currently send the "Start" message right after, it seems fine to combine notifyOnResize into the start message.

It seems to work well locally, hopefully try will agree.

> I don't fully understand helpers API, but can't we resolve setup once `done` is resolve?
> I imagine not, there is some action being done in between setup and post?
> 
> This `done` global seems weak. But I don't see how to do otherwise.

I don't know the helpers API very well either...  Yes, there is work done after `setup` and before `post`: it runs the command.  So, I need to wait for the event before that happens and check for it after that.

Since the current helpers only give `setup` and `post` to work with, I think the `done` global is still needed.  But yeah, I agree it's not the best.
https://hg.mozilla.org/mozilla-central/rev/b884d7fa86f5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
This affects Aurora as well. Can we please request approval for uplift? Thanks!
Flags: needinfo?(jryans)
Comment on attachment 8713840 [details]
MozReview Request: Bug 1239562 - Use explicit events to fix test races in responsive design. r=ochameau

Approval Request Comment
[Feature/regressing bug #]: No specific regression, race conditions in tests added over time
[User impact if declined]: No user impact, many pieces of responsive design tests to remove race conditions
[Describe test coverage new/current, TreeHerder]: On m-c, tests greatly improved (so far, at least!)
[Risks and why]: Low, while some product code changes are included, they are only meant to improve test behavior.
[String/UUID change made/needed]: None
Flags: needinfo?(jryans)
Attachment #8713840 - Flags: approval-mozilla-aurora?
Comment on attachment 8713840 [details]
MozReview Request: Bug 1239562 - Use explicit events to fix test races in responsive design. r=ochameau

Eliminates some race conditions in responsive design tests.
Please uplift to aurora.
Attachment #8713840 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
has problems during uplift:

warning: conflicts while merging devtools/client/responsivedesign/responsivedesign-child.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
Flags: needinfo?(jryans)
Depends on: 1249967
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.