1603370 - Make RDM support target switching

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, enhancement, P1)

Description

[Alexandre Poirot [:ochameau]]

RDM would require some dedicated work, different from all panels listed in bug 1592575.
Mostly because it isn't hosted into a Toolbox.

The issue here is that when you navigate between two URLs which are loaded in distinct process, the TargetFront will change in favor of a new one.
As of today, it only happens when you navigate from special pages loaded in the parent process to websites (ex: about:robots to mozilla.org).
But if you enable Fission, it will happen if you switch between wikipedia.org and mozilla.org.

We should review all the call involving a target front and ensure that it supports this:
https://searchfox.org/mozilla-central/search?q=target&case=false&regexp=false&path=responsive

I think that the only code to tweak would be this one:
https://searchfox.org/mozilla-central/rev/62a130ba0ac80f75175e4b65536290b52391f116/devtools/client/responsive/ui.js#392-393
It memoizes the emulationFront and this will break if a target is being switched.
We could:

• use TargetList in order to watch for the new top level targets, see this code as example:
  https://searchfox.org/mozilla-central/rev/62a130ba0ac80f75175e4b65536290b52391f116/devtools/shared/resources/tests/browser_target_list_frames.js#112-144
• stop memoizing the emulationFront, or memoize it differently in order to check if we should spawn a new one if the target changed.

Comment 1

[Chris Peterson [:cpeterson]]

Tracking dt-fission-m2 bugs for Fission Nightly (M6)

Comment 2

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1603370: Define currentTarget getter. r?jdescottes!,ochameau!

Comment 3

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1603370: Reflect target switching. r?mtigley!,ochameau!

Depends on D58902

Comment 4

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1603370: Add a test for target switching on RDM. r?mtigley!,ochameau!

Depends on D58903

Comment 5

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1603370: Addressing backward-compatibility. r?mtigley

Depends on D58904

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b567b33ed95a
Define currentTarget getter. r=jdescottes,ochameau
https://hg.mozilla.org/integration/autoland/rev/1d819921c8a3
Reflect target switching. r=ochameau,mtigley
https://hg.mozilla.org/integration/autoland/rev/0124af9f8ded
Add a test for target switching on RDM. r=ochameau,mtigley

Comment 7

[Andreea Pavel [:apavel]]

Backed out for permafailing bug 1600635

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&revision=0124af9f8ded058e087be56f73c7bbd6174d0651&selectedJob=285174476

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=285174476&repo=autoland&lineNumber=52258

Backout: https://hg.mozilla.org/integration/autoland/rev/4b6a77b24e823bbf1225b551fac8a11d5e45dc1b

Comment 8

[Daisuke Akatsuka (:daisuke)]

Thanks Andreea!

Hi Micah!
I investigated this issue, it seems that the mix test of new UI and the old one caused the assertion.
And also I found browser_typeahead_find.js test which was failed also is passed if we remove the below line.
https://searchfox.org/mozilla-central/source/devtools/client/responsive/ui.js#274
We actually remove the class in destroy() (and I confirmed the class has been removing after closing tab.), but it seems that the style of the element will not be reverted perfectly? And this might be the cause.
https://searchfox.org/mozilla-central/source/devtools/client/responsive/ui.js#365
Do you have any ideas?

Or, I talked with Julian a bit ago, can we separate the browser.ini into two? One is for new UI, the other is not.
This also intended to avoid conflicting new UI and old. What do you think?

Comment 9

[Micah [:mtigley] (she/her)]

(In reply to Daisuke Akatsuka (:daisuke) from comment #8)

We actually remove the class in destroy() (and I confirmed the class has been removing after closing tab.), but it seems that the style of the element will not be reverted perfectly? And this might be the cause.
https://searchfox.org/mozilla-central/source/devtools/client/responsive/ui.js#365
Do you have any ideas?

Thank you for looking into this Daisuke! In the test we added, we set the pref for devtools.responsive.browserUI.enabled and devtools.target-switching.enabled to true. So for devtools.responsive.browserUI.enabled = true this means the code will run the lines you mentioned above. But at the end of test, I don't think the test will clear the prefs? I initially thought this might have been the case but after some investigating I still get the same failure even if I set those prefs back to false.

Or, I talked with Julian a bit ago, can we separate the browser.ini into two? One is for new UI, the other is not.
This also intended to avoid conflicting new UI and old. What do you think?

We recently extended addRDMTask to flip prefs between the old and new UIs. The idea was to ensure that existing code for the old RDM would still be functioning correctly while also running the same tests for the new RDM. But as you mentioned, there seems to still be a problem where the styling is not reverted completely, even though this function is setting the prefs as expected before this particular test runs.

If it's alright, I'd like to continue investigating why this particular test is failing before trying to split the browser.ini into two. Brad, did you have any thoughts on this?

Comment 10

[Brad Werth [:bradwerth]]

I'm sorry, I don't have any additional insight into this. I don't know what it would mean to split browser.ini. If that solves the problem, it sounds reasonable.

Since the browser_typeahead_find.js test fails anyway, we can consider landing this patch by also disabling the test as long as the behavior the test is supposed to exercise works interactively with this patch applied. In other words, does typeahead find work in RDM with this patch applied? If so, just disable the test entirely, because we don't have confidence in it.

Comment 11

[Daisuke Akatsuka (:daisuke)]

Thank you for your comments, Micah, Brad!

(In reply to Brad Werth [:bradwerth] from comment #10)

I'm sorry, I don't have any additional insight into this. I don't know what it would mean to split browser.ini. If that solves the problem, it sounds reasonable.

If this failure may be occurred by mixing both tests with/without fission, we thought separating the test is one of the ideas. So, I intended that we define something like fission-browser.ini to test only fission related tests.

(In reply to Micah Tigley [:mtigley] from comment #9)

If it's alright, I'd like to continue investigating why this particular test is failing before trying to split the browser.ini into two.

Yeah, if it's possible, I'd like you to investigate this one :)
Can you have time to take a look, Micah?

Comment 12

[Micah [:mtigley] (she/her)]

(In reply to Daisuke Akatsuka (:daisuke) from comment #11)

Yeah, if it's possible, I'd like you to investigate this one :)
Can you have time to take a look, Micah?

For sure! I will follow-up with you on this.

Comment 13

[Micah [:mtigley] (she/her)]

(In reply to Daisuke Akatsuka (:daisuke) from comment #11)

If this failure may be occurred by mixing both tests with/without fission, we thought separating the test is one of the ideas. So, I intended that we define something like fission-browser.ini to test only fission related tests.

I think this sounds fine. I'm still unsure of what's causing the test failure for browser_typeahead_find.js, but I don't think we should keep this from blocking the patch from moving forward any longer than necessary. Daisuke, would you be able to update the revision that splits the fission-only tests as you proposed?

Comment 14

[Daisuke Akatsuka (:daisuke)]

Thanks, Micah!

I investigated a bit more, I thought this is related to fission but it seems that this was not. (I'm sorry!)
browser_typeahead_find.js test will be failed when doing the test after doing the following test.

add_task(async function() {
  await pushPref("devtools.responsive.browserUI.enabled", true);
  const tab = await addTab("http://example.com/");
  await openRDM(tab);
  await wait(3000);
  await closeRDM(tab);
  await removeTab(tab);
});

So I changed the order so as to test browser_typeahead_find.js after browser_viewport_resolution_restore.js, then tried it, because it seemed that browser_viewport_resolution_restore.js run on new browserUI as well.
However, there were no failures, I didn't understand why the test was passed.
I investigated this. The reason is that the test on new UI never run even if we pass true for includeBrowserEmbeddedUI in addRDMTaskWithPreAndPost.
https://searchfox.org/mozilla-central/source/devtools/client/responsive/test/browser/browser_viewport_resolution_restore.js#62
We can confirm that the value of devtools.responsive.browserUI.enabled is always false at the below point.
https://searchfox.org/mozilla-central/source/devtools/client/responsive/test/browser/head.js#165-167
Because the async test will be done after setting the original value here as we didn't wait for the async test.
https://searchfox.org/mozilla-central/source/devtools/client/responsive/test/browser/head.js#209-212

So, now, we should do followings:

1. Find the reason why the browser_typeahead_find.js is failed after testing the new UI and fix that.
2. Fix the addRDMTaskWithPreAndPost behavior to run on the new UI.

So, for this bug, can we file a follow-up bug that fixes the failure and skips this test?
And about addRDMTaskWithPreAndPost, let's file a bug if you also could confirm.
What do you think, Micah?

Comment 15

[Micah [:mtigley] (she/her)]

(In reply to Daisuke Akatsuka (:daisuke) from comment #14)

So, for this bug, can we file a follow-up bug that fixes the failure and skips this test?
And about addRDMTaskWithPreAndPost, let's file a bug if you also could confirm.
What do you think, Micah?

Thanks Daisuke for doing more investigation into this. I agree that we can skip the test for now and fix the issue in a follow-up bug. I'll create it.

Comment 16

[Brad Werth [:bradwerth]]

Daisuke, thank you for working through all of this. It looks like I built the addRDMTask function incorrectly and I need to make it properly await the added tests before changing prefs and triggering another test. I've opened Bug 1610588 to address this.

Comment 17

[Daisuke Akatsuka (:daisuke)]

(In reply to Micah Tigley [:mtigley] from comment #15)

I agree that we can skip the test for now and fix the issue in a follow-up bug. I'll create it.

Thanks! I will make the test skip. And I filed bug 1610717.

(In reply to Brad Werth [:bradwerth] from comment #16)

Daisuke, thank you for working through all of this. It looks like I built the addRDMTask function incorrectly and I need to make it properly await the added tests before changing prefs and triggering another test. I've opened Bug 1610588 to address this.

Thank you for filing!

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ac263c823285
Define currentTarget getter. r=jdescottes,ochameau
https://hg.mozilla.org/integration/autoland/rev/7e29e22ea816
Reflect target switching. r=ochameau,mtigley
https://hg.mozilla.org/integration/autoland/rev/f2c8e5f773ae
Add a test for target switching on RDM. r=ochameau,mtigley

Comment 19

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/ac263c823285
https://hg.mozilla.org/mozilla-central/rev/7e29e22ea816
https://hg.mozilla.org/mozilla-central/rev/f2c8e5f773ae