Closed Bug 1240913 Opened 8 years ago Closed 8 years ago

Preserve page state when toggling RDM

Categories

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

Product:
DevTools
Component:
Responsive Design Mode
Type:
defect

Tracking

(firefox49 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 49
Iteration:
49.2 - May 23
Tracking Flags:
Tracking Status
firefox49 --- verified

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [multiviewport] [mvp-rdm])

Attachments

(6 files, 4 obsolete files)

MozReview Request: Bug 1240913 - Catch errors on RDM open / close. r=ochameau
58 bytes, text/x-review-board-request
ochameau
: review+
Details
MozReview Request: Bug 1240913 - Add @allowfullscreen to appease swapFrameLoaders. r=ochameau
58 bytes, text/x-review-board-request
ochameau
: review+
Details
MozReview Request: Bug 1240913 - Swap page state between tabs and RDM viewports. r=ochameau
58 bytes, text/x-review-board-request
ochameau
: review+
Details
MozReview Request: Bug 1240913 - Destroy RDM early on quit. r=ochameau
58 bytes, text/x-review-board-request
ochameau
: review+
Details
Video of toggling on Mac
710.47 KB, image/gif
Details
additional issues.ogv
9.28 MB, video/ogg
Details 
The basic shell in bug 1239437 does not preserve page state when toggling RDM on and off, where page state means:

* Current scroll position
* Current state in a single page app (any state lost by reloading from the current URL)

This makes RDM much less convenient to use, especially with complex single page apps.
Priority: -- → P3
Priority: P3 → P2
Whiteboard: [multiviewport] → [multiviewport] [mvp-rdm]
This is the bug where I imagine we'd make use of swapFrameLoaders (bug 1242644).
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Iteration: --- → 48.3 - Apr 25
Priority: P2 → P1
QA Contact: mihai.boldan
Iteration: 48.3 - Apr 25 → 49.1 - May 9
We don't really need fullscreen access here, but we do need the same access to
browser features as regular browser tabs.  The `swapFrameLoaders` platform API
we use compares such features before allowing the swap to proceed.

Review commit: https://reviewboard.mozilla.org/r/51217/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51217/
This change brings the following improvements to RDM:

* Page state is preserved when toggling in and out of RDM
* Session history is no longer manipulated, so the tool UI won't end up in the
  tab's back-forward page list.

Known issues to be fixed later:

* The browser UI is not hooked up to the viewport browser
* Restarting the browser with the tool open shows a confused, empty RDM

Review commit: https://reviewboard.mozilla.org/r/51219/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51219/
Comment on attachment 8749880 [details]
MozReview Request: Bug 1240913 - Catch errors on RDM open / close. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51215/diff/1-2/
Comment on attachment 8749881 [details]
MozReview Request: Bug 1240913 - Add @allowfullscreen to appease swapFrameLoaders. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51217/diff/1-2/
Comment on attachment 8749882 [details]
MozReview Request: Bug 1240913 - Swap page state between tabs and RDM viewports. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51219/diff/1-2/
Iteration: 49.1 - May 9 → 49.2 - May 23
Comment on attachment 8749880 [details]
MozReview Request: Bug 1240913 - Catch errors on RDM open / close. r=ochameau

https://reviewboard.mozilla.org/r/51215/#review48391
Attachment #8749880 - Flags: review?(poirot.alex) → review+
Comment on attachment 8749881 [details]
MozReview Request: Bug 1240913 - Add @allowfullscreen to appease swapFrameLoaders. r=ochameau

https://reviewboard.mozilla.org/r/51217/#review48393

I'll need some more time to process the last patch!!
Attachment #8749881 - Flags: review?(poirot.alex) → review+
Attachment #8749882 - Flags: review?(poirot.alex)
Comment on attachment 8749882 [details]
MozReview Request: Bug 1240913 - Swap page state between tabs and RDM viewports. r=ochameau

https://reviewboard.mozilla.org/r/51219/#review48385

Here is a first round.
I need some time to digest swapToInnerBrowser.

::: devtools/client/responsive.html/browser/swap.js:60
(Diff revision 2)
> +
> +      // 3. Create the initial viewport inside the tool UI.
> +      // The calling application will use container page loaded into the tab to
> +      // do whatever it needs to create the inner browser.
> +      yield tabLoaded(containerTab);
> +      innerBrowser = yield getInnerBrowser(containerBrowser);

May be assert here that innerBrowser is mozbrowser+remote?

::: devtools/client/responsive.html/components/browser.js:50
(Diff revision 2)
> +    if (!this.props.swapAfterMount) {
> +      yield this.startFrameScript();
> +    }
> +
> +    // manager.js waits for this signal before allowing browser tests to start
> +    this.props.onBrowserMounted();

This comment is not relevant anymore with the frame script story in parallel.
getInnerBrowser/swap is waiting for this signal.

Also, wouldn't it be easier to do:
message.post(window, "browser-mounted")
? instead of passing onBrowserMounted as props?

::: devtools/client/responsive.html/index.js:126
(Diff revision 2)
>    }
>  };
>  
>  /**
> - * Called by manager.js when tests want to use the viewport's message manager.
> - * It is packed into an object because this is the format most easily usable
> + * Called by manager.js when tests want to use the viewport's browser to access
> + * this content inside. It is packed into an object because this is the format

"to access this content inside"
 => to access its content ?

"It is packed into an object" is no longer relevant as we no longer refer to the message manager.
=> ...to access its content. We mock <xul:browser> to make it usable with ContentTask.spawn() which expect just a `messageManager` property.

::: devtools/client/responsive.html/manager.js:221
(Diff revision 2)
> -    let toolWindow = this.toolWindow = tabBrowser.contentWindow;
> -    toolWindow.addEventListener("message", this);
> -    yield waitForMessage(toolWindow, "init");
> -    toolWindow.addInitialViewport(contentURI);
> -    yield waitForMessage(toolWindow, "browser-mounted");
> +      tab: this.tab,
> +      containerURL: TOOL_URL,
> +      getInnerBrowser: Task.async(function* (containerBrowser) {
> +        let toolWindow = ui.toolWindow = containerBrowser.contentWindow;
> +        toolWindow.addEventListener("message", ui);
> +        yield message.wait(toolWindow, "init");

I don't get how it doesn't race. in swapToInnerBrowser(), you wait for tabLoaded() before calling getInnerBrowser.
"init" message is only sent on "load".
So this is very likely to race?

Oh, but the "init" message is sent after a few operations after load event, that may be it.

One way to address these kind of issues is to expose a promise from index.js before load and yield on it from here via toolWindow.
Or use a double message like start-frame-script...
https://reviewboard.mozilla.org/r/51219/#review48385

> May be assert here that innerBrowser is mozbrowser+remote?

Yes, we should at least assert that the remoteness matches what we expect.  This process _should_ work with a XUL browser too, since we're just adding some "decorations" to the mozbrowser to make it look more like XUL anyway.  Now I don't know why we'd _want_ to do that... but anyway, I don't think we need to assert mozbrowser.

> This comment is not relevant anymore with the frame script story in parallel.
> getInnerBrowser/swap is waiting for this signal.
> 
> Also, wouldn't it be easier to do:
> message.post(window, "browser-mounted")
> ? instead of passing onBrowserMounted as props?

That's true, I'll update the comment.

It would be _easier_ to call `message.post` directly, yes.  So far we have left all the `postMessage` calls at the top level and passed them as props in the same way that we do this for Redux state changes (calls to `dispatch`).  I am still unsure what's "best" here, and it's mostly a code style thing.  I may leave it to a follow up to figure out the best style for these `postMessage` things.

> I don't get how it doesn't race. in swapToInnerBrowser(), you wait for tabLoaded() before calling getInnerBrowser.
> "init" message is only sent on "load".
> So this is very likely to race?
> 
> Oh, but the "init" message is sent after a few operations after load event, that may be it.
> 
> One way to address these kind of issues is to expose a promise from index.js before load and yield on it from here via toolWindow.
> Or use a double message like start-frame-script...

What's the race you are thinking of here?  

We load the tool UI into a container page.  Then we need to get the browser inside.  As a first step, we need to be able to talk to the page from manager.js, so we wait for "init" so we know the page has basic code alive and running, which is sent after the Redux store is up in `index.js`.  We tell the page "please create an initial viewport".  It will create it, and once the browser frame has mounted, it sends the "browser-mounted" message and this code continues on to retrieve the browser frame.

If you think there is a race, what are the two things that are racing?
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #13)
> > I don't get how it doesn't race. in swapToInnerBrowser(), you wait for tabLoaded() before calling getInnerBrowser.
> > "init" message is only sent on "load".
> > So this is very likely to race?
> > 
> > Oh, but the "init" message is sent after a few operations after load event, that may be it.
> > 
> > One way to address these kind of issues is to expose a promise from index.js before load and yield on it from here via toolWindow.
> > Or use a double message like start-frame-script...
> 
> What's the race you are thinking of here?  
> 
> We load the tool UI into a container page.  Then we need to get the browser
> inside.  As a first step, we need to be able to talk to the page from
> manager.js, so we wait for "init" so we know the page has basic code alive
> and running, which is sent after the Redux store is up in `index.js`.  We
> tell the page "please create an initial viewport".  It will create it, and
> once the browser frame has mounted, it sends the "browser-mounted" message
> and this code continues on to retrieve the browser frame.
> 
> If you think there is a race, what are the two things that are racing?

Here is the race:
  manager.js's getInnerBrowser: yield message.wait(toolWindow, "init"); happens after
  index.js's bootstrap.init: window.postMessage({ type: "init" }, "*");

It is likely to happen as:
getInnerBrowser is only called after "load" event (due to the `yield tabLoaded(containerTab);`)
and window.postMessage is called after "load" event.
Whereas message.wait(... "init") should be listening before the postMessage may happen if we want to be 100% sure it won't race. Both listener and dispatcher are racing and order may be shuffled on the one winning the load event race.
https://reviewboard.mozilla.org/r/51219/#review48385

> What's the race you are thinking of here?  
> 
> We load the tool UI into a container page.  Then we need to get the browser inside.  As a first step, we need to be able to talk to the page from manager.js, so we wait for "init" so we know the page has basic code alive and running, which is sent after the Redux store is up in `index.js`.  We tell the page "please create an initial viewport".  It will create it, and once the browser frame has mounted, it sends the "browser-mounted" message and this code continues on to retrieve the browser frame.
> 
> If you think there is a race, what are the two things that are racing?

Thanks for noticing this!  I've changed to using init / init:done messages to avoid this issue.
Watch for `quit-application-granted` and destroy RDM when it happens.  This
resolves issues with session restore getting confused about the state of content
managed by RDM.

Review commit: https://reviewboard.mozilla.org/r/52055/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52055/
Attachment #8751505 - Flags: review?(poirot.alex)
Attachment #8749882 - Flags: review?(poirot.alex)
Comment on attachment 8749882 [details]
MozReview Request: Bug 1240913 - Swap page state between tabs and RDM viewports. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51219/diff/2-3/
Comment on attachment 8751505 [details]
MozReview Request: Bug 1240913 - Destroy RDM early on quit. r=ochameau

https://reviewboard.mozilla.org/r/52055/#review49143

Hum. Wouldn't it be better to watch to responsive.html/index.xhtml unload?
There is another STR which is broken and it seems to be related to the same issue:
 - open firefox
 - open google.com
 - toggle responsive mode
 - open google.com in the same tab so that it replaces the responsive mode with a regular google.com
 - toggle responsive mode again
Actual: aOtherBrowser.ownerDocument.defaultView is null exception
Expected: working responsive mode
Attachment #8751505 - Flags: review?(poirot.alex)
https://reviewboard.mozilla.org/r/52055/#review49143

I think we don't want to worry about what happens when using the location bar to navigate just yet.  In future work (bug 1240900) the location bar will be wired up to the inner browser of the viewport, so the steps you listed would not close RDM, but instead would navigate to that page using the inner viewport.

I did make a few attempts at your suggestion in any case.  When you try to navigate the tab to a regular page currently, we do get an "unload" inside RDM, but it's because the browser UI just flipped the remoteness of the tab to remote == true to load that page, so there's no way to unwind the swap anymore.

Since that shouldn't be any issue further along, I think we can ignore this case for now, but we should test it again after some more work is in place.

This did make me realize it would be good to try other events to watch for the shutdown / browser close case.  "beforeunload" on the browser window appears to come in even before the "quit-application-granted" notification, so I think I'll switch to that.
Comment on attachment 8749882 [details]
MozReview Request: Bug 1240913 - Swap page state between tabs and RDM viewports. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51219/diff/3-4/
Attachment #8751505 - Flags: review?(poirot.alex)
Comment on attachment 8751505 [details]
MozReview Request: Bug 1240913 - Destroy RDM early on quit. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52055/diff/1-2/
Comment on attachment 8751505 [details]
MozReview Request: Bug 1240913 - Destroy RDM early on quit. r=ochameau

https://reviewboard.mozilla.org/r/52055/#review49235

::: devtools/client/responsive.html/manager.js:268
(Diff revision 2)
> +    switch (event.type) {
> +      case "message":
> +        this.handleMessage(event);
> +        break;
> +      case "beforeunload":
> +        if (event.target.location != TOOL_URL) {

nit: Wouldn't it be ultimately better to check for event.target == this.toolWindow?
Or is this chasing an about:blank event?
Attachment #8751505 - Flags: review?(poirot.alex) → review+
https://reviewboard.mozilla.org/r/52055/#review49235

> nit: Wouldn't it be ultimately better to check for event.target == this.toolWindow?
> Or is this chasing an about:blank event?

Your guess is right, it's to avoid about:blank.  I'll add a comment.
Comment on attachment 8749882 [details]
MozReview Request: Bug 1240913 - Swap page state between tabs and RDM viewports. r=ochameau

https://reviewboard.mozilla.org/r/51219/#review49245

It looks good overall. 
We may just strengthen the init/destroy codepath a bit.
Tell me if you feel more confident about also doing that in followup.

If you keep the RDM toggle shortcut pressed, you will see "The inner browser's remoteness must match the original tab." exceptions
and then when you close the tab, another RDM tabs appears from nowhere.

Otherwise here is various cases that don't work within mozbrowser frames:
- data:text/html,<script>onclick=() => window.open("data:text/html,ok");</script>
  requires to listen for mozbrowseropenwindow on the iframe
- data:text/html,<script>onclick=function(){alert('ok')}</script>
  I think it relates to mozbrowsershowmodalprompt

Also I imagine you are not there yet with the toolbox, but when you toggle the responsive mode it closes the toolbox.

I see a weird exception when clicking on links within the RDM:
  content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts).

::: devtools/client/responsive.html/browser/swap.js:64
(Diff revision 4)
> +      yield tabLoaded(containerTab);
> +      innerBrowser = yield getInnerBrowser(containerBrowser);
> +      addXULBrowserDecorations(innerBrowser);
> +      if (innerBrowser.isRemoteBrowser != tab.linkedBrowser.isRemoteBrowser) {
> +        throw new Error("The inner browser's remoteness must match the " +
> +                        "original tab.");

Whenever I shakes RDM hard, I always end up with 
leftover of hidden tab.
What about using a try/catch or promise.catch and try to cleanup things if something went wrong?
Note sure it is that easy to cleanup things...

::: devtools/client/responsive.html/browser/swap.js:73
(Diff revision 4)
> +      //    the viewport in the tool UI, preserving all state via
> +      //    `gBrowser._swapBrowserDocShells`.
> +      gBrowser._swapBrowserDocShells(tab, innerBrowser);
> +
> +      // 5. Mark the viewport browser's docshell as active so the content is
> +      //    rendered.

The first docShellIsActive on the newly created hidden tab makes some sense.
This one, a bit less. innerBrowser is in the hidden tab but has been flagged as active, doesn't that translates to its child docshells?
I imagine the fact that the tab is still hidden takes the precedence over the fact that the parent is active?
I very short note about why this docshell isn't active may help.

::: devtools/client/responsive.html/browser/swap.js:78
(Diff revision 4)
> +      //    rendered.
> +      innerBrowser.frameLoader.tabParent.docShellIsActive = true;
> +
> +      // 6. Force the original browser tab to be non-remote since the tool UI
> +      //    must loaded in the parent process, and we're about to swap the tool
> +      //    UI into this tab.

must loaded -> must be loaded?

::: devtools/client/responsive.html/browser/swap.js:83
(Diff revision 4)
> +      //    UI into this tab.
> +      gBrowser.updateBrowserRemoteness(tab.linkedBrowser, false);
> +
> +      // 7. Swap the tool UI (with viewport showing the content) into the
> +      //    original browser tab and close the temporary tab used to load the
> +      //    tool via `swapBrowsersAndCloseOther`.

I'm not sure it is necessary to mention "via swapBrowsesAndCloseOther" in the comment?
Same comment applies to other comment in this file mentioning the function it calls right after.

::: devtools/client/responsive.html/test/browser/browser_page_state.js:116
(Diff revision 4)
> +    mm.addMessageListener("Test:History", function listener({data}) {
> +      mm.removeMessageListener("Test:History", listener);
> +      resolve(data);
> +    });
> +    mm.loadFrameScript(`data:,(${frameScript})();`, true);
> +  });

Couldn't you use ContentTask instead of manual frame script??

::: devtools/docs/responsive-design-mode.md:64
(Diff revision 4)
> +
> +* A: Restore the tab content without any RDM tool displayed **OR**
> +* B: Restore the RDM tool the tab content inside, just as before the restart
> +
> +Option A (no RDM after session restore) seems more in line with how the rest of
> +DevTools currently functions after restore.

You may rephrase this with the final behavior you went for.
Attachment #8749882 - Flags: review?(poirot.alex)
https://reviewboard.mozilla.org/r/51219/#review49245

Thanks for pointing out the init / destroy.  First, I noticed that we weren't stopping the responsive frame script in the child on close, so I've fixed that and added a test to verify it.  I've also tried to harden the init / destroy in general.  We now make sure only one destroy operation can be in progress, which seems to make it much better in my testing.  The "hold down the shortcut" case you mention now works as expected.

For the specific mozbrowser issues (window.open, alert, etc.) that's great to know about!  I think we can ignore these for now though and look into them later on as needed.  I have added them to my mental list of things we may need. :)

For the toolbox, yes it's a known issue that is closes, we have bug 1240912 for this.  It also currently targets the tool UI and not the page content, which is bug 1240907.

I wasn't able to reproduce the specific issue you saw when clicking links.  Do you have more details or steps?  It sounds like something we can handle in a follow up.

> Whenever I shakes RDM hard, I always end up with 
> leftover of hidden tab.
> What about using a try/catch or promise.catch and try to cleanup things if something went wrong?
> Note sure it is that easy to cleanup things...

I think this should be much better now with the improved init / destroy process, please try some more shaking!

> The first docShellIsActive on the newly created hidden tab makes some sense.
> This one, a bit less. innerBrowser is in the hidden tab but has been flagged as active, doesn't that translates to its child docshells?
> I imagine the fact that the tab is still hidden takes the precedence over the fact that the parent is active?
> I very short note about why this docshell isn't active may help.

Actually, it looks like this step is not needed anymore.  It used to be needed in a previous variation of these steps when I used some lower level methods to do the swapping.  I've removed this step.

> must loaded -> must be loaded?

Fixed.

> I'm not sure it is necessary to mention "via swapBrowsesAndCloseOther" in the comment?
> Same comment applies to other comment in this file mentioning the function it calls right after.

I agree it's a bit redundant in the code, but I'd like to keeps the comments identical to the steps in the doc file, and it seems useful to mention the names over there.

> Couldn't you use ContentTask instead of manual frame script??

Yes, not sure why I didn't before... Anyway, fixed now.

> You may rephrase this with the final behavior you went for.

Okay, I'll update it in a new version of the shutdown changes (which you already r+ed).
Comment on attachment 8749882 [details]
MozReview Request: Bug 1240913 - Swap page state between tabs and RDM viewports. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51219/diff/4-5/
Attachment #8749882 - Flags: review?(poirot.alex)
Comment on attachment 8751505 [details]
MozReview Request: Bug 1240913 - Destroy RDM early on quit. r=ochameau

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52055/diff/2-3/
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #28)
> https://reviewboard.mozilla.org/r/51219/#review49245
> 
> The "hold down the shortcut" case you mention now works as expected.

Confirmed.

> I wasn't able to reproduce the specific issue you saw when clicking links. 
> Do you have more details or steps?  It sounds like something we can handle
> in a follow up.

Nothing special, open mozilla.org, toggle responsive mode, click on the first link, get that in the stdout:
WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use isContentWindowPrivate instead (but only for frame scripts).
pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:25:14
ContentClick.contentAreaClick@resource:///modules/ContentClick.jsm:69:12
ContentClick.receiveMessage@resource:///modules/ContentClick.jsm:27:9
JavaScript error: resource:///modules/ContentClick.jsm, line 74: TypeError: window.whereToOpenLink is not a function
JavaScript error: resource:///modules/ReaderParent.jsm, line 82: TypeError: win.gBrowser is undefined

It may require to have browser.dom.window.dump.enabled to see them...
> > Whenever I shakes RDM hard, I always end up with 
> > leftover of hidden tab.
> > What about using a try/catch or promise.catch and try to cleanup things if something went wrong?
> > Note sure it is that easy to cleanup things...
> 
> I think this should be much better now with the improved init / destroy
> process, please try some more shaking!

It works much better regarding errors, but it seems to be blinking/slow.
Let say yout toggle RDM on https://www.mozilla.org/en-US/privacy/firefox/
You will see the mozilla logo blink black and also see the scrollbars switch from desktop style to mobile one and all this seems to be junky and slow. May be some polish on shuffling when we do display stuff. Use of visibility: hidden/display: none, ...


Now, when we close the browser I get these errors on stdout and browser doesn't close anymore:
JavaScript error: resource://gre/modules/ExtensionContent.jsm, line 914: TypeError: this.globals.get(...) is undefined
JavaScript error: resource://gre/modules/ExtensionContent.jsm, line 914: TypeError: this.globals.get(...) is undefined
WARNING: At least one completion condition is taking too long to complete. Conditions: [{"name":"SessionStore: flushing all windows","state":{"total":1,"current":0},"filename":"resource:///modules/sessionstore/SessionStore.jsm","lineNumber":1454,"stack":["resource:///modules/sessionstore/SessionStore.jsm:ssi_onQuitApplicationGranted:1454","resource:///modules/sessionstore/SessionStore.jsm:ssi_observe:643","chrome://global/content/globalOverlay.js:goQuitApplication:69","chrome://browser/content/browser.xul:oncommand:1"]}] Barrier: quit-application-granted


May be that's because of my rebase? I had to resolve conflicts in manager.js regarding touch simulation.
Attachment #8749882 - Attachment is obsolete: true
Attachment #8749882 - Flags: review?(poirot.alex)
We don't really need fullscreen access here, but we do need the same access to
browser features as regular browser tabs.  The `swapFrameLoaders` platform API
we use compares such features before allowing the swap to proceed.

Review commit: https://reviewboard.mozilla.org/r/53398/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53398/
This change brings the following improvements to RDM:

* Page state is preserved when toggling in and out of RDM
* Session history is no longer manipulated, so the tool UI won't end up in the
  tab's back-forward page list.

Known issues to be fixed later:

* The browser UI is not hooked up to the viewport browser
* Restarting the browser with the tool open shows a confused, empty RDM

Review commit: https://reviewboard.mozilla.org/r/53400/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53400/
Watch for the tab's `beforeunload` and destroy RDM when it happens.  This
resolves issues with session restore getting confused about the state of content
managed by RDM.

Review commit: https://reviewboard.mozilla.org/r/53402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53402/
(In reply to Alexandre Poirot [:ochameau] from comment #32)
> > I wasn't able to reproduce the specific issue you saw when clicking links. 
> > Do you have more details or steps?  It sounds like something we can handle
> > in a follow up.
> 
> Nothing special, open mozilla.org, toggle responsive mode, click on the
> first link, get that in the stdout:
> WARNING: content window passed to PrivateBrowsingUtils.isWindowPrivate. Use
> isContentWindowPrivate instead (but only for frame scripts).
> pbu_isWindowPrivate@resource://gre/modules/PrivateBrowsingUtils.jsm:25:14
> ContentClick.contentAreaClick@resource:///modules/ContentClick.jsm:69:12
> ContentClick.receiveMessage@resource:///modules/ContentClick.jsm:27:9
> JavaScript error: resource:///modules/ContentClick.jsm, line 74: TypeError:
> window.whereToOpenLink is not a function
> JavaScript error: resource:///modules/ReaderParent.jsm, line 82: TypeError:
> win.gBrowser is undefined
> 
> It may require to have browser.dom.window.dump.enabled to see them...

I do have dump enabled, but I am still not seeing the private browsing error.  Let's move it to a follow up and work it out there.

I filed bug 1273718 for this.
(In reply to Alexandre Poirot [:ochameau] from comment #33)
> > > Whenever I shakes RDM hard, I always end up with 
> > > leftover of hidden tab.
> > > What about using a try/catch or promise.catch and try to cleanup things if something went wrong?
> > > Note sure it is that easy to cleanup things...
> > 
> > I think this should be much better now with the improved init / destroy
> > process, please try some more shaking!
> 
> It works much better regarding errors, but it seems to be blinking/slow.
> Let say yout toggle RDM on https://www.mozilla.org/en-US/privacy/firefox/
> You will see the mozilla logo blink black and also see the scrollbars switch
> from desktop style to mobile one and all this seems to be junky and slow.
> May be some polish on shuffling when we do display stuff. Use of visibility:
> hidden/display: none, ...

I am not seeing this slowness or blinking.  Maybe it's a difference between Mac and Linux?  Please let me know more, hopefully just some visual tweaks like you say.

I attached a screen capture of what I am seeing.
(In reply to Alexandre Poirot [:ochameau] from comment #33)
> Now, when we close the browser I get these errors on stdout and browser
> doesn't close anymore:
> JavaScript error: resource://gre/modules/ExtensionContent.jsm, line 914:
> TypeError: this.globals.get(...) is undefined
> JavaScript error: resource://gre/modules/ExtensionContent.jsm, line 914:
> TypeError: this.globals.get(...) is undefined
> WARNING: At least one completion condition is taking too long to complete.
> Conditions: [{"name":"SessionStore: flushing all
> windows","state":{"total":1,"current":0},"filename":"resource:///modules/
> sessionstore/SessionStore.jsm","lineNumber":1454,"stack":["resource:///
> modules/sessionstore/SessionStore.jsm:ssi_onQuitApplicationGranted:1454",
> "resource:///modules/sessionstore/SessionStore.jsm:ssi_observe:643","chrome:/
> /global/content/globalOverlay.js:goQuitApplication:69","chrome://browser/
> content/browser.xul:oncommand:1"]}] Barrier: quit-application-granted
> 
> May be that's because of my rebase? I had to resolve conflicts in manager.js
> regarding touch simulation.

The shutdown blocker should be fixed, I think it was related to the rebase.  I had to make an additional change after the new touch simulation code.
I am aware that errors are currently logged by ReaderParent.jsm and ExtensionContent.jsm.  These are harmless errors that are related to content scripts still being loaded into the child frame.

I filed bug 1273721 for these.
Another shutdown crash STR:
- Open www.lemonde.fr (heavy website)
- toggle RDM
- open a new tab
- close the browser from the hamburger menu

Firefox won't close until a while and will end up crashing:
Crash Annotation GraphicsCriticalError: |[C0][GFX1-]: Receive IPC close with reason=AbnormalShutdown (t=75.8457) 
###!!! [Child][MessageChannel] Error: (msgtype=0x4600DE,name=PContent::Msg_GraphicsError) Channel closing: too late to send/recv, messages will be lost

[GFX1-]: Receive IPC close with reason=AbnormalShutdown

Note that the website is still loading while the browser is closing.
If you press Escape before toggle RDM to stop website load, it won't crash.

The good news is that yesterday issue is gone.
Comment on attachment 8753612 [details]
MozReview Request: Bug 1240913 - Catch errors on RDM open / close. r=ochameau

https://reviewboard.mozilla.org/r/53396/#review50270
Attachment #8753612 - Flags: review?(poirot.alex) → review+
Comment on attachment 8753613 [details]
MozReview Request: Bug 1240913 - Add @allowfullscreen to appease swapFrameLoaders. r=ochameau

https://reviewboard.mozilla.org/r/53398/#review50272
Attachment #8753613 - Flags: review?(poirot.alex) → review+
Comment on attachment 8753614 [details]
MozReview Request: Bug 1240913 - Swap page state between tabs and RDM viewports. r=ochameau

https://reviewboard.mozilla.org/r/53400/#review50274

Let this hacker boat ship. And keep an eye on the various followups!

::: devtools/client/responsive.html/browser/swap.js:12
(Diff revision 1)
> +const promise = require("promise");
> +const { Task } = require("resource://gre/modules/Task.jsm");
> +
> +/**
> + * Swap page content from an existing tab into a new browser within a container
> + * page.  Page state is preserved by using `swapFrameLoaders`, just like when

Duplicated spaces:
s/page.  Page state/page. Page state/

::: devtools/client/responsive.html/browser/swap.js:13
(Diff revision 1)
> +const { Task } = require("resource://gre/modules/Task.jsm");
> +
> +/**
> + * Swap page content from an existing tab into a new browser within a container
> + * page.  Page state is preserved by using `swapFrameLoaders`, just like when
> + * you move a tab to a new window.  This provides a seamless transition for the

Same here before "This provides"

::: devtools/client/responsive.html/browser/swap.js:29
(Diff revision 1)
> + *        A browser tab with content to be swapped.
> + * @param containerURL
> + *        URL to a page that holds an inner browser.
> + * @param getInnerBrowser
> + *        Function that returns a Promise to the inner browser within the
> + *        container page.  It is called with the outer browser that loaded the

Same before "It is called"

::: devtools/client/responsive.html/browser/swap.js:119
(Diff revision 1)
> +}
> +
> +/**
> + * Browser elements that are passed to `gBrowser._swapBrowserDocShells` are
> + * expected to have certain properties that currently exist only on
> + * <xul:browser> elements.  In particular, <iframe mozbrowser> elements don't

Same here and in next comments in this file

::: devtools/client/responsive.html/manager.js:63
(Diff revision 1)
>     *         Resolved to the ResponsiveUI instance for this tab when opening is
>     *         complete.
>     */
>    openIfNeeded: Task.async(function* (window, tab) {
> +    if (!tab.linkedBrowser.isRemoteBrowser) {
> +      return promise.reject(new Error("RDM only available for remote tabs."));

Followup: you may want a visible warning instead of just a message in the browser console.

::: devtools/client/responsive.html/manager.js:251
(Diff revision 1)
> +
> +    this.touchEventSimulator =
> +      new TouchEventSimulator(toolViewportContentBrowser);
> +
> +    // TODO: Session restore continues to store the tool UI as the page's URL.
> +    // Most likely related to browser UI's inability to show correct location.

Isn't this comment out of date?
Attachment #8753614 - Flags: review?(poirot.alex) → review+
Comment on attachment 8753615 [details]
MozReview Request: Bug 1240913 - Destroy RDM early on quit. r=ochameau

https://reviewboard.mozilla.org/r/53402/#review50284

::: devtools/client/responsive.html/manager.js:322
(Diff revision 1)
> +        // Ignore the event for locations like about:blank
> +        if (event.target.location != TOOL_URL) {
> +          return;
> +        }
> +        // We want to ensure we close RDM before browser window close when
> +        // quitting.  Session restore starts its final session data flush when

Duplicated spaces again. 3 times in this comment.
Attachment #8753615 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #27)
> Otherwise here is various cases that don't work within mozbrowser frames:
> - data:text/html,<script>onclick=() =>
> window.open("data:text/html,ok");</script>
>   requires to listen for mozbrowseropenwindow on the iframe
> - data:text/html,<script>onclick=function(){alert('ok')}</script>
>   I think it relates to mozbrowsershowmodalprompt

I filed bug 1273997 to track this.
(In reply to Alexandre Poirot [:ochameau] from comment #43)
> Another shutdown crash STR:
> - Open www.lemonde.fr (heavy website)
> - toggle RDM
> - open a new tab
> - close the browser from the hamburger menu
> 
> Firefox won't close until a while and will end up crashing:
> Crash Annotation GraphicsCriticalError: |[C0][GFX1-]: Receive IPC close with
> reason=AbnormalShutdown (t=75.8457) 
> ###!!! [Child][MessageChannel] Error:
> (msgtype=0x4600DE,name=PContent::Msg_GraphicsError) Channel closing: too
> late to send/recv, messages will be lost
> 
> [GFX1-]: Receive IPC close with reason=AbnormalShutdown
> 
> Note that the website is still loading while the browser is closing.
> If you press Escape before toggle RDM to stop website load, it won't crash.

I can't reproduce this on Mac, but let's track it as a follow up.  Filed bug 1274019.
https://reviewboard.mozilla.org/r/53400/#review50274

> Duplicated spaces:
> s/page.  Page state/page. Page state/

I don't think these double spaces are a problem.  It's a style I commonly use when writing text with monospaced fonts, so that would be most code comments and .md files.  As long as the style is used consistently within one file, I think it's fine as is.

You can see many such usages in /devtools today:

https://dxr.mozilla.org/mozilla-central/search?q=%22.++%22+path%3Adevtools&redirect=false&case=true

> Isn't this comment out of date?

Yes, the comment is removed in the next patch that actually fixes the shutdown issues.
https://reviewboard.mozilla.org/r/53402/#review50284

I am going to add a tiny test to this shutdown code to ensure it runs synchronously in the future.  It seems like this will be easy to miss without a test.
https://reviewboard.mozilla.org/r/53400/#review50274

> Followup: you may want a visible warning instead of just a message in the browser console.

Yes, good idea.  I filed bug 1274032 for this.
Attached video additional issues.ogv — 
I managed to test this issue on Firefox 49.0a1 (2016-05-26) and on Windows 10 x64, Ubuntu 14.04 x64 and Mac OS X 10.10.5.

I confirm that the page state is preserved when toggling RDM, but I noticed a few other issuse:
- If a web page is accessed (Eg. www.google.com) and RDM is enabled and disabled afterwards, the pointer from the text box is not displayed anymore. You can still add text to the text box, but the pointer is not displayed.
- On some websites that have dropdown buttons, when RDM is enabled, the dropdown buttons are not displayed (Eg. http://www.ebay.com/, http://www.bikeshop.ro/src.php?lng=ro&tip=am)
- When enabling RDM, the dropdown buttons are not working (Eg. https://bugzilla.mozilla.org/query.cgi)

I've attached a video with the issues described above.
Should I log new bugs for these issues?
Flags: needinfo?(jryans)
(In reply to Mihai Boldan, QA [:mboldan] from comment #55)
> Created attachment 8757272 [details]
> additional issues.ogv
> 
> I managed to test this issue on Firefox 49.0a1 (2016-05-26) and on Windows
> 10 x64, Ubuntu 14.04 x64 and Mac OS X 10.10.5.
> 
> I confirm that the page state is preserved when toggling RDM, but I noticed
> a few other issuse:
> - If a web page is accessed (Eg. www.google.com) and RDM is enabled and
> disabled afterwards, the pointer from the text box is not displayed anymore.
> You can still add text to the text box, but the pointer is not displayed.
> - On some websites that have dropdown buttons, when RDM is enabled, the
> dropdown buttons are not displayed (Eg. http://www.ebay.com/,
> http://www.bikeshop.ro/src.php?lng=ro&tip=am)
> - When enabling RDM, the dropdown buttons are not working (Eg.
> https://bugzilla.mozilla.org/query.cgi)
> 
> I've attached a video with the issues described above.
> Should I log new bugs for these issues?

Wow, thanks for finding these things!  Yes, please log bugs for these issues.  They all seem like independent issues to me, so I would say a separate bug for each.

Some of them seem OS specific, so please note that in bugs where it applies.  At least for #2 "dropdown buttons are not displayed", I do not see this issue on Mac.
Flags: needinfo?(jryans)
I logged Bug 1276607, Bug 1276629 and Bug 1276644 for the issues described in Comment 55.
Since the found issues were logged separately, I am marking this issue Verified Fixed.
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: