Closed Bug 1240900 Opened 9 years ago Closed 8 years ago

Connect primary browser UI to the viewport

Categories

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

Product:
DevTools
Component:
Responsive Design Mode
Type:
defect

Tracking

(firefox50 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 50
Iteration:
50.2 - Jul 4
Tracking Flags:
Tracking Status
firefox50 --- verified

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 2 open bugs)

Details

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

Attachments

(2 files, 2 obsolete files)

Bug 1240900 - Connect primary browser UI to the viewport.
(deleted), text/x-review-board-request
ochameau
: review+
Details
Bug 1240900 - Use outerWindowID directly from browser.
(deleted), text/x-review-board-request
ochameau
: review+
Details 
The basic shell in bug 1239437 shows the chrome:// URL of the new RDM tool in the location bar.

This not how our current RDM works (and also not how it works for other vendors).  We'll want to instead present the web content location in the location bar.

This should be simpler to achieve once we have switched to <iframe mozbrowser> in bug 1240896.
Extending this to include the page's title as well.
Summary: Present page's URL in the location bar → Present page's URL and title in the browser UI
Priority: -- → P3
Priority: P3 → P2
Whiteboard: [multiviewport] → [multiviewport] [mvp-rdm]
Assignee: nobody → jryans
status-firefox46: affected → ---
Status: NEW → ASSIGNED
Iteration: --- → 48.3 - Apr 18
Priority: P2 → P1
Iteration: 48.3 - Apr 25 → 49.1 - May 9
Iteration: 49.1 - May 9 → 49.2 - May 23
QA Contact: mihai.boldan
Iteration: 49.2 - May 23 → 49.3 - Jun 6
The scope is a bit larger than just the URL and title.  The primary browser navigational UI should behave as if it's connected to the page content in the viewport:

* Content page's URL is displayed in location bar
* Content page's title is displayed on the tab
* Back / forward navigates the viewport
* Entering a location navigates the viewport
* Page loading progress is displayed in the status bar as usual
Summary: Present page's URL and title in the browser UI → Connect primary browser UI to the viewport
The primary browser navigational UI should now behave as if it's connected to
the page content in the viewport, including things like:

* Content page's URL is displayed in location bar
* Content page's title is displayed on the tab
* Back / forward navigates the viewport
* Entering a location navigates the viewport
* Page loading progress is displayed in the status bar as usual

Review commit: https://reviewboard.mozilla.org/r/57134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57134/
Attachment #8759048 - Flags: review?(poirot.alex)
Attachment #8759049 - Flags: review?(poirot.alex)
Comment on attachment 8759048 [details]
MozReview Request: Bug 1240900 - Connect primary browser UI to the viewport. r=ochameau

https://reviewboard.mozilla.org/r/57134/#review53962

Haven't tested the patch yet. But here is a bunch of comments.

::: devtools/client/responsive.html/browser/tunnel.js:78
(Diff revision 1)
> +      // The `permanentKey` property on a <xul:browser> is used to index into
> +      // various maps held by the session store.  When you swap content around
> +      // with `_swapBrowserDocShells`, these keys are also swapped so they
> +      // follow the content.  This means the key that matches the content is on
> +      // the inner browser along with all the other state associated the content
> +      // when it used to live in a <xul:browser>.  Since we want the browser UI

This sentence is hard to process. Isn't there an issue around "associated the content".

May be "along with all the other state associated the content when it used to live in a <xul:browser>" is not that helpful?

::: devtools/client/responsive.html/browser/tunnel.js:87
(Diff revision 1)
> +
> +      // Replace the outer browser's native messageManager with a message
> +      // manager tunnel which we can use to route messages of interest to the
> +      // inner browser instead.  Note: The _actual_ messageManager accessible
> +      // from `browser.frameLoader.messageManager` is not overridable and is
> +      // left unchanged.  Only the XBL getter `browser.messageManager` (which is

It looks like noone is using frameLoader.messageManager from /browser/?
It may be relevant to state that in comment.

::: devtools/client/responsive.html/browser/tunnel.js:89
(Diff revision 1)
> +      // manager tunnel which we can use to route messages of interest to the
> +      // inner browser instead.  Note: The _actual_ messageManager accessible
> +      // from `browser.frameLoader.messageManager` is not overridable and is
> +      // left unchanged.  Only the XBL getter `browser.messageManager` (which is
> +      // used in most Firefox UI code) is overridden.
> +      // Override XBL getter from browser.xml, removed in stop().

(here and in other similar comments) I imagine it is obvious all overrides are going to be reverted in stop.

::: devtools/client/responsive.html/browser/tunnel.js:107
(Diff revision 1)
> +      // the tab's remoteness effectively is the remoteness of the inner
> +      // browser.
> +      // Override XBL getter from browser.xml, removed in stop().
> +      Object.defineProperty(outer, "isRemoteBrowser", {
> +        get() {
> +          return inner.isRemoteBrowser;

nit: we assume it is true in all this code. So we may just return true.

::: devtools/client/responsive.html/browser/tunnel.js:120
(Diff revision 1)
> +      // thing we need to route to the inner browser (the messages), instead of
> +      // having to tweak many different browser properties.  It is safe to alter
> +      // a XBL binding dynamically.  The content within is not reloaded.
> +      outer.setAttribute("style",
> +                         "-moz-binding: url(chrome://browser/content/" +
> +                         "tabbrowser.xml#tabbrowser-remote-browser);");

hum. xbl. 2016. Still not dead?!

::: devtools/client/responsive.html/browser/tunnel.js:124
(Diff revision 1)
> +                         "-moz-binding: url(chrome://browser/content/" +
> +                         "tabbrowser.xml#tabbrowser-remote-browser);");
> +
> +      // The constructor of the new XBL binding is run asynchronously, so give
> +      // it a turn of the event loop to complete.
> +      yield DevToolsUtils.waitForTick();

Are you sure one tick is enough? I have more complex stories in my memories about XBL.
Do you run a test experiencing this code on a very slow machines like windows (XP) debug or linux32 debug?

::: devtools/client/responsive.html/browser/tunnel.js:127
(Diff revision 1)
> +      // The constructor of the new XBL binding is run asynchronously, so give
> +      // it a turn of the event loop to complete.
> +      yield DevToolsUtils.waitForTick();
> +
> +      // Replace the `webNavigation` object with our own version which tries to
> +      // use mozbrowser APIs where possible.

It may help to mention that we override fields defined by the remote browser xbl here.

::: devtools/client/responsive.html/browser/tunnel.js:136
(Diff revision 1)
> +
> +      // Now that we've flipped to the remote browser XBL binding, add
> +      // `progressListener` onto the remote version of `webProgress`.
> +      let tab = gBrowser.getTabForBrowser(outer);
> +      let progressListener = gBrowser._tabListeners.get(tab);
> +      outer.webProgress.addProgressListener(progressListener);

Shouldn't we remove the already registered one?

::: devtools/client/responsive.html/browser/tunnel.js:147
(Diff revision 1)
> +        "canGoForward",
> +        "_currentURI",
> +      ];
> +      for (let property of remoteWebNavigationState) {
> +        webNavigation[property] = inner._remoteWebNavigationImpl[property];
> +      }

I don't get that. Why don't you do that from BrowserElementWebNavigation definition?

::: devtools/client/responsive.html/browser/tunnel.js:246
(Diff revision 1)
> +  // When we're in the process of swapping content around, we end up receiving a
> +  // SessionStore:update message which lists the special tool UI that is loaded
> +  // into the outer browser as part of the history.  We want SessionStore's view
> +  // of the history for our tab to only have the page content of the inner
> +  // browser, so we wait until the one errant message has gone by, and then we
> +  // copy the permanentKey after that.

Ouch.
Can't we do that closer to the call that is dging to dispatch the SessionStore:update message?

::: devtools/client/responsive.html/browser/tunnel.js:269
(Diff revision 1)
> +}
> +
> +/**
> + * This module allows specific messages of interest to be directed from the
> + * outer browser to the inner browser (and vice versa) in a targetted fashion
> + * without having to touch the original code paths that use them.

Why not forwarding everything to/from the inner?
Attachment #8759048 - Flags: review?(poirot.alex)
https://reviewboard.mozilla.org/r/57136/#review54022

::: devtools/shared/client/main.js
(Diff revision 1)
>          } else {
>            // Tabs in parent process
> -          let windowUtils = browser.contentWindow
> +          packet.outerWindowID = browser.outerWindowID;
> -            .QueryInterface(Ci.nsIInterfaceRequestor)
> -            .getInterface(Ci.nsIDOMWindowUtils);
> -          packet.outerWindowID = windowUtils.outerWindowID;

That won't work with mozbrowser iframe.
At least not without your hacks.
Could you at least fallback to use windowsUtils to fetch the outerWindowID if browser.outerWindowID isn't set?
https://reviewboard.mozilla.org/r/57136/#review54022

> That won't work with mozbrowser iframe.
> At least not without your hacks.
> Could you at least fallback to use windowsUtils to fetch the outerWindowID if browser.outerWindowID isn't set?

Is that a real use case that we support today?  This code starts by calling `tab.linkedBrowser` first.  Is there really a configuration that uses a mozbrowser frame as a tab's linkedBrowser?

I can change it as you asked, but it does not seem like a case that's actually used.
https://reviewboard.mozilla.org/r/57134/#review53962

> This sentence is hard to process. Isn't there an issue around "associated the content".
> 
> May be "along with all the other state associated the content when it used to live in a <xul:browser>" is not that helpful?

Yeah, there is a lot going on, so it was hard to know exactly what to say here.  I'll remove the part you mentioned.

> It looks like noone is using frameLoader.messageManager from /browser/?
> It may be relevant to state that in comment.

Right, I'll strengthen that part of the comment.

> (here and in other similar comments) I imagine it is obvious all overrides are going to be reverted in stop.

Okay, I'll remove them.

> nit: we assume it is true in all this code. So we may just return true.

Yep, seems fine to do that.

> hum. xbl. 2016. Still not dead?!

To be honest, I am not sure why there is both a remote and non-remote binding.  The remote binding should work everywhere (even for content where isRemoteBrowser == false), it just uses message manager a bit more.  Perhaps when e10s finally ships it will be consolidated so there is only one.

From some searching, it looks like `style.MozBinding` is a slightly cleaner way to set this, instead of messing with the attribute.

> Are you sure one tick is enough? I have more complex stories in my memories about XBL.
> Do you run a test experiencing this code on a very slow machines like windows (XP) debug or linux32 debug?

I am not sure, I was also worried about this...  There does not appear to be a nice way to get an event here.  I did find a test[1] that calls rAF twice (?!) and seems to think that is "good enough".

Another angle is that the constuctor will call `addMessageListener`, so I could watch for that from the tunnel to be more confident.

I am seeing some docshell leaks on Windows 7 debug, but the tests appear to run successfully...  I'll make sure to run those other slow platforms as well.

[1]: https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/tests/chrome/test_bug1050049.html#26

> It may help to mention that we override fields defined by the remote browser xbl here.

Yes, good point.  I also realized from looking at this that we don't need to backup the original values of these fields.  They are no longer used after we reset the binding in `stop`.

> Shouldn't we remove the already registered one?

No, there is not one registered at this point.  Flipping the binding above here creates a fresh webProgress without any listeners, so we need to reattach it here.  I'll add more to the comment about this.

> I don't get that. Why don't you do that from BrowserElementWebNavigation definition?

I did not do it originally because `BrowserElementWebNavigation` just implements its interface and does not really know about the tunneling going on.  I can move this to some kind of `import` method on it though, seems reasonable.

> Ouch.
> Can't we do that closer to the call that is dging to dispatch the SessionStore:update message?

I agree this part is pretty unfortunate...  Some of the complexity is due to the fact that SessionStore:update is sent after a 1 sec timeout[1] that starts counting down once the session history changes.  So, it would seem like we need to watch for the message since we don't reliably know when it will appear otherwise.

I'll keep thinking about it.

[1]: https://dxr.mozilla.org/mozilla-central/rev/4d63dde701b47b8661ab7990f197b6b60e543839/browser/components/sessionstore/content/content-sessionStore.js#776

> Why not forwarding everything to/from the inner?

It would be possible to send everythng from outer to inner, since they all call sendAsyncMessage.  However, I cannot listen for everything from inner to outer: I have to listen for each message of interest by name (there is no "listen to everything" API as far as I know).  Since I have to do that in one direction, I figured I would do it for both, so that I don't implement half of a communication pattern.

There may also be messages that I don't want to actually reach the inner browser, though I don't have a specific example of that yet.
https://reviewboard.mozilla.org/r/57134/#review53962

> I am not sure, I was also worried about this...  There does not appear to be a nice way to get an event here.  I did find a test[1] that calls rAF twice (?!) and seems to think that is "good enough".
> 
> Another angle is that the constuctor will call `addMessageListener`, so I could watch for that from the tunnel to be more confident.
> 
> I am seeing some docshell leaks on Windows 7 debug, but the tests appear to run successfully...  I'll make sure to run those other slow platforms as well.
> 
> [1]: https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/tests/chrome/test_bug1050049.html#26

I did another try run[1] with extra logging and made sure to include Windows XP and Linux 32 debug.  Ironically, I still do not know what will happen on those platforms...  For both of them, it appears we don't actually run mochitest devtools e10s suites.  We _do_ run mochitest devtools non-e10s for them, but these tests require e10s, so the state is unclear for those platforms.

Anyway, looking at Windows 7 debug which has docshell failures, my debug logging suggests that this step is not the issue.  The XBL constructor is called before we pass through this waiting step.

So, I will tentitively consider this okay for now, but I am certainly open to better approaches.  I'll need to find the real cause of the docshell leaks.

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=15eee943e104
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> https://reviewboard.mozilla.org/r/57136/#review54022
> 
> > That won't work with mozbrowser iframe.
> > At least not without your hacks.
> > Could you at least fallback to use windowsUtils to fetch the outerWindowID if browser.outerWindowID isn't set?
> 
> Is that a real use case that we support today?  This code starts by calling
> `tab.linkedBrowser` first.  Is there really a configuration that uses a
> mozbrowser frame as a tab's linkedBrowser?

Not any "in production", but the about:devtools-toolbox?target mode depends on that.
It creates a { linkedBrowser: iframe } object to fake the tab.
I may introduce a test against this usecase.

> I can change it as you asked, but it does not seem like a case that's
> actually used.

It sounds unfortunate to break arbitrary mozbrowser iframe support when you are introducing first usage of it. You will most likely need such codepath or something similar when debugging frames within the RDM.
https://reviewboard.mozilla.org/r/57134/#review54736

::: devtools/client/responsive.html/browser/tunnel.js:120
(Diff revision 1)
> +      // thing we need to route to the inner browser (the messages), instead of
> +      // having to tweak many different browser properties.  It is safe to alter
> +      // a XBL binding dynamically.  The content within is not reloaded.
> +      outer.setAttribute("style",
> +                         "-moz-binding: url(chrome://browser/content/" +
> +                         "tabbrowser.xml#tabbrowser-remote-browser);");

Hooking on an overriden function is weak. You have to know first when the binding is applied otherwise I imagine the binding and the override may race.

But you are right, knowing when the constructor runs is the only safe way to handle XBL.
There is two typical ways to detect that. Wait for an even dispatched by the constructor. remote-browser isn't dispatching any so that's a dead end.
Another typical solution is to look for an attribute to be available on the node:
```
while(!outer._remoteWebNavigation) 
  waitForTick();
```

::: devtools/client/responsive.html/browser/tunnel.js:136
(Diff revision 1)
> +
> +      // Now that we've flipped to the remote browser XBL binding, add
> +      // `progressListener` onto the remote version of `webProgress`.
> +      let tab = gBrowser.getTabForBrowser(outer);
> +      let progressListener = gBrowser._tabListeners.get(tab);
> +      outer.webProgress.addProgressListener(progressListener);

But wasn't there already a webProgress before we started messing with the <browser> ?

::: devtools/client/responsive.html/browser/tunnel.js:147
(Diff revision 1)
> +        "canGoForward",
> +        "_currentURI",
> +      ];
> +      for (let property of remoteWebNavigationState) {
> +        webNavigation[property] = inner._remoteWebNavigationImpl[property];
> +      }

I'm still confused here. Actually I'm even more confused.
Now. I don't get where does `_remoteWebNavigationImpl` comes from??
It seems to be set by remote-browser xbl. Which shouldn't be set on `innet` right? `inner` is mozbrowser remote iframe, right?

And to get back to my previous comment about this code.
Can't `BrowserElementWebNavigation`, do:
```
canGoBack() {
  return this.browser._remoteWebNavigationImpl.canGoBack();
}
```
?

::: devtools/client/responsive.html/browser/tunnel.js:269
(Diff revision 1)
> +}
> +
> +/**
> + * This module allows specific messages of interest to be directed from the
> + * outer browser to the inner browser (and vice versa) in a targetted fashion
> + * without having to touch the original code paths that use them.

Oh right, makes sense.
Iteration: 49.3 - Jun 6 → 50.1
https://reviewboard.mozilla.org/r/57134/#review54736

> Hooking on an overriden function is weak. You have to know first when the binding is applied otherwise I imagine the binding and the override may race.
> 
> But you are right, knowing when the constructor runs is the only safe way to handle XBL.
> There is two typical ways to detect that. Wait for an even dispatched by the constructor. remote-browser isn't dispatching any so that's a dead end.
> Another typical solution is to look for an attribute to be available on the node:
> ```
> while(!outer._remoteWebNavigation) 
>   waitForTick();
> ```

Thanks, I decided to look for a property being set like you suggest, but I chose to spin an event loop (`processNextEvent`) rather than a raw while loop, which should avoid burning CPU cycles.

> But wasn't there already a webProgress before we started messing with the <browser> ?

There was yes.  Actually, once the tunnel stops, that same webProgess becomes accessible again (with its listener still attached), because it's part of the outer docshell.

I tried removing from it first and then adding to the new one, but it adds a lot of complexity, especially during stopping.  I did realize I was adding the wrong listener though, so I've corrected that.

> I'm still confused here. Actually I'm even more confused.
> Now. I don't get where does `_remoteWebNavigationImpl` comes from??
> It seems to be set by remote-browser xbl. Which shouldn't be set on `innet` right? `inner` is mozbrowser remote iframe, right?
> 
> And to get back to my previous comment about this code.
> Can't `BrowserElementWebNavigation`, do:
> ```
> canGoBack() {
>   return this.browser._remoteWebNavigationImpl.canGoBack();
> }
> ```
> ?

It's set on the inner browser by `swapDocShells`.  Check the list of swapped objects in browser-swap.md.  The entire `_remoteWebNavigationImpl` is moved to the inner browser during the swap.

I am not sure what you mean about `canGoBack`.  `BrowserElementWebNavigation` is not really aware of the swap going on, it just sends meassage or calls mozbrowser to implment the interface.

I'll post an updated version soon with my reply to your last comment about move the swap into `BrowserElementWebNavigation`.
Attachment #8759049 - Attachment is obsolete: true
Attachment #8759049 - Flags: review?(poirot.alex)
The primary browser navigational UI should now behave as if it's connected to
the page content in the viewport, including things like:

* Content page's URL is displayed in location bar
* Content page's title is displayed on the tab
* Back / forward navigates the viewport
* Entering a location navigates the viewport
* Page loading progress is displayed in the status bar as usual

Review commit: https://reviewboard.mozilla.org/r/58128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58128/
Attachment #8760554 - Flags: review?(poirot.alex)
Attachment #8760555 - Flags: review?(poirot.alex)
Comment on attachment 8760554 [details]
Bug 1240900 - Connect primary browser UI to the viewport.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58128/diff/1-2/
Comment on attachment 8760555 [details]
Bug 1240900 - Use outerWindowID directly from browser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58130/diff/1-2/
Comment on attachment 8760555 [details]
Bug 1240900 - Use outerWindowID directly from browser.

https://reviewboard.mozilla.org/r/58130/#review55046
Attachment #8760555 - Flags: review?(poirot.alex) → review+
Comment on attachment 8760554 [details]
Bug 1240900 - Connect primary browser UI to the viewport.

https://reviewboard.mozilla.org/r/58128/#review55036

You may have one leak in your pirate ship's hold. One last pass may be required.

Works great! My report may sound alarming, but many things do work. History/navigation work like a charm without seing any particular race/exception.

When you close a tab with RDM turned ON. It doesn't really close the tab, it just turns off RDM. I haven't tried without your patches but it looks like a regression? I think we should fix that in this bug.

Also you see the responsive.html document url briefly blink in the url bar.
It is especially visible on https://css-tricks.com/examples/State/
Do you think that's something this patch may address?

Random comments while testing, but I don't think any are directly related to these patches:
* Still see various steps while switching on/off RDM. Feels junky. Very junky on some sites.
Like http://edition.cnn.com/ 
When switching it on, you first see the website with desktop layout appear in the RDM, but then after a bit, it switches to a mobile layout (without the menu label on the top black header).
Is there a resize handler? or is this just CSS being recomputed? If that's the later it would be great to wait for it to be recomputed before showing the website. There is no point in showing the Desktop one with such a blinking effect.
Same thing when switching it off, you see the mobile layout in a full tab, and then, only after a bit it resizes.

* nit: just wondering if CTRL +/- shortcuts behavior is expected? It zomm in/out the whole RDM interface?
* The menu item: "View > Page Styles > No Style" destroy RDM badly when selected while RDM is On.
* nit: Ctrl+A or menu "Edit > Select All" doesn't work if the inner browser isn't selected.
* Menu "File > Save file" saves the RDM index.xhtml instead of the web page.
* Context menu > Select all never works, nor Copy.
* Context menu > View selected source only shows mozbrowser source (sounds like a complex story :/)
* Geolocation API doesn't work in RDM: http://html5demos.com/geo The permission prompt doesn't appear.

::: devtools/client/responsive.html/browser/tunnel.js:97
(Diff revision 2)
> +        },
> +        configurable: true,
> +        enumerable: true,
> +      });
> +
> +      // Clear out cached any cached state that references the current non-remote XBL

Clear out cached any cached
=>
Clear out any cached
?

::: devtools/client/responsive.html/browser/tunnel.js:112
(Diff revision 2)
> +                               "#tabbrowser-remote-browser)";
> +
> +      // The constructor of the new XBL binding is run asynchronously and there is no
> +      // event to signal its completion.  Spin an event loop to watch for properties that
> +      // are set by the contructor.
> +      while (outer.mDestroyed) {

mDestroyed isn't set by the constructor but the binding. So either watch for something set by the constructor like _remoteWebNavigation, or update the comment.

::: devtools/client/responsive.html/browser/tunnel.js:231
(Diff revision 2)
> +
> +exports.tunnelToInnerBrowser = tunnelToInnerBrowser;
> +
> +function copyPermanentKey(outer, inner) {
> +  // When we're in the process of swapping content around, we end up receiving a
> +  // SessionStore:update message which lists the special tool UI that is loaded

I'm still confused about this code.
What "special tool UI" are/is?

Here you are listening for any next SessionStore:update event, but aren't they specific to a given tab/browser? Shouldn't we filter to prevent mixing events between tabs?

And I really dont get the executeSoon part.
Don't we want SessionStore to immediately map to the inner browser?

::: devtools/client/responsive.html/browser/tunnel.js:339
(Diff revision 2)
> +  },
> +
> +  get outerChildMM() {
> +    // This is only possible because we require the outer browser to be
> +    // non-remote, so we're able to reach into its window and use the child
> +    // side message mananger there.

mananger => manager

::: devtools/client/responsive.html/browser/tunnel.js:364
(Diff revision 2)
> +    this.innerParentMM.sendAsyncMessage(name, ...args);
> +  },
> +
> +  init() {
> +    for (let method of this.PASS_THROUGH_METHODS) {
> +      let _method = method;

Why do you need that let _method = method;?
can't you use method?

::: devtools/client/responsive.html/browser/web-navigation.js:18
(Diff revision 2)
> +  return NetUtil.readInputStreamToString(stream, stream.available());
> +}
> +
> +/**
> + * This object aims to provide the nsIWebNavigation interface for mozbrowser
> + * elements.  nsIWebNavigation is one of interfaces expected on <xul:browser>s,

/of // ?
or /of/of the/ ?

::: devtools/client/responsive.html/browser/web-navigation.js:22
(Diff revision 2)
> + * This object aims to provide the nsIWebNavigation interface for mozbrowser
> + * elements.  nsIWebNavigation is one of interfaces expected on <xul:browser>s,
> + * so this wrapper helps mozbrowser elements support this.
> + *
> + * It attempts to use the mozbrowser API wherever possible, however some methods
> + * don't exist yet, so we fallback to messaging the WebNavigation frame script

s/WebNavigation/RemoveWebNavigation/ ?
Attachment #8760554 - Flags: review?(poirot.alex)
https://reviewboard.mozilla.org/r/58128/#review55036

> When you close a tab with RDM turned ON. It doesn't really close the tab, it just turns off RDM. I haven't tried without your patches but it looks like a regression? I think we should fix that in this bug.

Yes, that's for catching this.  There were a few more messages I needed to tunnel to support the browser's unload sequence, so I've added those.

> Also you see the responsive.html document url briefly blink in the url bar.
> It is especially visible on https://css-tricks.com/examples/State/
> Do you think that's something this patch may address?

Good point, I've added several measures to address this.  On entering RDM, I freeze the state related to navigation until the swap completes.  This avoids the blinking location bar on entering.  (The blinking is caused by `swapBrowserAndCloseOther` updating the browser UI before we're done changing everything.)

This also made me realize that some browser state is out of date when the tunnel stops for pages you navigate to inside RDM, for example the page title would be missing in the tab after exit (only the URL would show instead).  For this case, I'm now pushing the browser state back down to the inner browser when the tunnel stops.

> * Still see various steps while switching on/off RDM. Feels junky. Very junky on some sites.

I see what you mean here, not sure what the main issue is yet.  Filed bug 1278757 for this.

> nit: just wondering if CTRL +/- shortcuts behavior is expected? It zomm in/out the whole RDM interface?

I would guess we want to zoom the content instead.  Filed bug 1278758.

> The menu item: "View > Page Styles > No Style" destroy RDM badly when selected while RDM is On.

I am not worried about this yet.  Let's wait to see if this is an issue in real use.  (I did not even know this option existed...) :)

> nit: Ctrl+A or menu "Edit > Select All" doesn't work if the inner browser isn't selected.

That's probably okay for now.  We can adjust if needed.

> Menu "File > Save file" saves the RDM index.xhtml instead of the web page.

The page saving machinery can get pretty complex.  Filed bug 1278761.

> Context menu > Select all never works, nor Copy.
> Context menu > View selected source only shows mozbrowser source

We're also missing some items entirely.  Filed bug 1278762.

> Geolocation API doesn't work in RDM: http://html5demos.com/geo The permission prompt doesn't appear.

Filed bug 1278763.

Thanks for all of this testing, it's great to have these issues tracked!

> Clear out cached any cached
> =>
> Clear out any cached
> ?

Fixed.

> mDestroyed isn't set by the constructor but the binding. So either watch for something set by the constructor like _remoteWebNavigation, or update the comment.

Ah, that's true.  Okay, I'll watch `_remoteWebNavigation` then.

> I'm still confused about this code.
> What "special tool UI" are/is?
> 
> Here you are listening for any next SessionStore:update event, but aren't they specific to a given tab/browser? Shouldn't we filter to prevent mixing events between tabs?
> 
> And I really dont get the executeSoon part.
> Don't we want SessionStore to immediately map to the inner browser?

> I'm still confused about this code.
> What "special tool UI" are/is?

The special tool UI is the container page with the RDM UI, the thing loaded into the outer browser which then contains the viewport within in.  I've reworded this to say "container page in the outer browser".

> Here you are listening for any next SessionStore:update event, but aren't they specific to a given tab/browser? Shouldn't we filter to prevent mixing events between tabs?

Yes, they are specific to a particular browser.  However, we are listening for a message coming through the outer browser's message manager, so we'll only receive messages for that specific browser.  There's no need to filter to the right browser in this case.

> And I really dont get the executeSoon part.
> Don't we want SessionStore to immediately map to the inner browser?

It's because we _want_ SessionStore to effectively trash the current message, since it would otherwise store the container page (the RDM tool UI) in your session history, which we don't want at all.  I added more detail about this in comments.

The cause of all our troubles here is that the outer browser, like any browser, is running the session store frame script which dutifully reports back up to the parent.  But we don't actually want to record data about the outer browser!  It's the thing we want to hide.  Since there's no way to "shut off" a frame script, we're doing a wacky workaround to cause its messages to be ignored for this specific case.

> mananger => manager

Fixed.

> Why do you need that let _method = method;?
> can't you use method?

This is bug 449811.  SM doesn't create a fresh binding in for-of loops yet.  I'll mention the bug in a comment.

> /of // ?
> or /of/of the/ ?

Fixed.

> s/WebNavigation/RemoveWebNavigation/ ?

Here I am trying to reference the name of the messages actually, so I've tweaked this slightly to reflect that.
Comment on attachment 8760554 [details]
Bug 1240900 - Connect primary browser UI to the viewport.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58128/diff/2-3/
Attachment #8760554 - Flags: review?(poirot.alex)
Comment on attachment 8760555 [details]
Bug 1240900 - Use outerWindowID directly from browser.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58130/diff/2-3/
Attachment #8760554 - Flags: review?(poirot.alex) → review+
Comment on attachment 8760554 [details]
Bug 1240900 - Connect primary browser UI to the viewport.

https://reviewboard.mozilla.org/r/58128/#review56742

<p>I see one crash, again, I haven't tested without these patches.<br />
Open Private browser window, go on google, wait for end of load, toggle RDM, untoggle RDM, the child process crashes.</p>
<p>Another thing that has nothing to do with you patch, when you grab the RDM scrollbar, and release outside of the viewport, the viewport is going to keep scrolling on mouseover on the viewport.</p>

If the crashes relates to this patches, it would be great to fix that first.
Otherwise I'm interested to see the new canons used by more than the two of us.

::: devtools/client/responsive.html/browser/swap.js:41
(Diff revision 3)
> +  let tunnel;
>  
>    return {
>  
>      start: Task.async(function* () {
> +      // Freeze navigation temporarily to avoid "blinking" in the location bar.

I also see back/forward blinking. It is especially visible if the forward button is visible. I'm wondering if you could apply the same trick?

::: devtools/client/responsive.html/browser/tunnel.js:261
(Diff revision 3)
> +    // record it in a map keyed by the outer browser's `permanentKey`.  Since the outer
> +    // browser has a nonsense value as its `permanentKey` until the copy takes place
> +    // below, this means SessionStore drops this session update as invalid data, which is
> +    // great for us, since we don't want it to be known anyway.  We then copy the
> +    // `permanentKey` across in the next tick so that all future session updates (which
> +    // will be about actual content page navigation we _want_ to record) will be stored.

These two comments start to be too long.
I think you can merge the two into something like this:
* a SessionStore:update message is sent for the RDM outer document load, that we can't cancel
* but session store code matches the messages against a permanentKey
* so we let it pass through, it won't match any browser in its map as we delay setting permanentKey after it receives the message

::: devtools/client/responsive.html/browser/web-navigation.js:55
(Diff revision 3)
> +  LOAD_FLAGS_FROM_EXTERNAL: 4096,
> +  LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP: 8192,
> +  LOAD_FLAGS_FIRST_LOAD: 16384,
> +  LOAD_FLAGS_ALLOW_POPUPS: 32768,
> +  LOAD_FLAGS_BYPASS_CLASSIFIER: 65536,
> +  LOAD_FLAGS_FORCE_ALLOW_COOKIES: 131072,

Instead of hardcoding values like this, could you refer to the interface properties like this:
```
LOAD_FLAGS_FORCE_ALLOW_COOKIES:  CinsIWebNavigation.LOAD_FLAGS_FORCE_ALLOW_COOKIES
```
Iteration: 50.1 → 50.2
https://reviewboard.mozilla.org/r/58128/#review56742

I wasn't able to reproduce this private browsing crash with these patches (or without them).  I'll keep this in mind and if we get better STR / more people report it, we can go from there.

For the scrollbar issue, we have bug 1264974 filed for this.

Thanks for all of your testing and review comments!

> I also see back/forward blinking. It is especially visible if the forward button is visible. I'm wondering if you could apply the same trick?

I tried to work on this for a bit, but it seems like everything was breaking forward navigation.  But then I realized it was broken even without my patches!  It seems there is a platform regression in general forward navigation for pages with redirects at the moment, I filed bug 1282162 about it.

So, I think you are right that I could do something similar, but I'd like to wait until the platform is at least back to working first.  Filed bug 1281950 for this.

> These two comments start to be too long.
> I think you can merge the two into something like this:
> * a SessionStore:update message is sent for the RDM outer document load, that we can't cancel
> * but session store code matches the messages against a permanentKey
> * so we let it pass through, it won't match any browser in its map as we delay setting permanentKey after it receives the message

Okay, I've tried to summarizes the comments like you suggest.

> Instead of hardcoding values like this, could you refer to the interface properties like this:
> ```
> LOAD_FLAGS_FORCE_ALLOW_COOKIES:  CinsIWebNavigation.LOAD_FLAGS_FORCE_ALLOW_COOKIES
> ```

Makes sense, I've changed to copy them from the interface.
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/6b750a2dae7d
Connect primary browser UI to the viewport. r=ochameau
https://hg.mozilla.org/integration/fx-team/rev/59bc5cd1caa6
Use outerWindowID directly from browser. r=ochameau
https://hg.mozilla.org/mozilla-central/rev/6b750a2dae7d
https://hg.mozilla.org/mozilla-central/rev/59bc5cd1caa6
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
I managed to reproduce this issue on Firefox 50.0a1 (2016-06-20) and on Windows 10 x64.
I covered the areas from Comment 15 and found a few potential issues:
- the mute/unmute tab button is not working while the RDM is enabled. It is also nod visible if the audio/video page is accessed while RDM is enabled.
- a blank page is displayed if the RDM is enabled. This is an intermittent issue. It is easy to reproduce if RDM is enabled while a webpage is loading. (See https://goo.gl/SLzYR2)
- if the RDM is enabled and a device is selected, and after that the RDM is closed and reopened, the selected device is no remembered. Is this expected?
The tests were performed on Firefox 50.0a1 (2016-07-13) and on Windows 10 x64, Ubuntu 16.04 x64, Mac OS X 10.9.5. 
Should I log new bugs for the issues described above?
QA Whiteboard: [qe-rdm]
Flags: needinfo?(jryans)
(In reply to Mihai Boldan, QA [:mboldan] from comment #31)
> I managed to reproduce this issue on Firefox 50.0a1 (2016-06-20) and on
> Windows 10 x64.
> I covered the areas from Comment 15 and found a few potential issues:
> - the mute/unmute tab button is not working while the RDM is enabled. It is
> also nod visible if the audio/video page is accessed while RDM is enabled.

Please file an issue for this, thanks for noticing it!

> - a blank page is displayed if the RDM is enabled. This is an intermittent
> issue. It is easy to reproduce if RDM is enabled while a webpage is loading.
> (See https://goo.gl/SLzYR2)

Hmm, that seems like a tricky one.  Were you able to isolate a good set of steps to reproduce?  I tried to duplicate what I saw in the video, but it didn't seem to happen for me.  If you have a good way to make it happen consistently, please file a bug.

> - if the RDM is enabled and a device is selected, and after that the RDM is
> closed and reopened, the selected device is no remembered. Is this expected?

Yes, that's expected.  For the moment, the entire RDM UI resets each time you enter.  We have bug 1248619 to preserve the RDM settings like the selected device.
Flags: needinfo?(jryans) → needinfo?(mihai.boldan)
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #32)
> > I managed to reproduce this issue on Firefox 50.0a1 (2016-06-20) and on
> > Windows 10 x64.
> > I covered the areas from Comment 15 and found a few potential issues:
> > - the mute/unmute tab button is not working while the RDM is enabled. It is
> > also nod visible if the audio/video page is accessed while RDM is enabled.
> 
> Please file an issue for this, thanks for noticing it!

I logged Bug 1287808 for the issue described above.

> 
> > - a blank page is displayed if the RDM is enabled. This is an intermittent
> > issue. It is easy to reproduce if RDM is enabled while a webpage is loading.
> > (See https://goo.gl/SLzYR2)
> 
> Hmm, that seems like a tricky one.  Were you able to isolate a good set of
> steps to reproduce?  I tried to duplicate what I saw in the video, but it
> didn't seem to happen for me.  If you have a good way to make it happen
> consistently, please file a bug.

I will investigate this issue and I will log a bug when I'll find more reliable STR. 

> > - if the RDM is enabled and a device is selected, and after that the RDM is
> > closed and reopened, the selected device is no remembered. Is this expected?
> 
> Yes, that's expected.  For the moment, the entire RDM UI resets each time
> you enter.  We have bug 1248619 to preserve the RDM settings like the
> selected device.

Thanks for the answers. 

Taking in consideration that no other issues were found related to this fix, I am marking this Bug Verified Fixed.
Status: RESOLVED → VERIFIED
status-firefox50: fixed → verified
Flags: needinfo?(mihai.boldan)
It seems that the issue from below is no longer reproducible on Firefox 50.0a1 (2016-07-20). I will keep an eye on this issue.
The issue was still reproducible on Firefox 50.0a1 (2016-07-19), but it seems that it was fixed along the way.

>> - a blank page is displayed if the RDM is enabled. This is an intermittent
> > issue. It is easy to reproduce if RDM is enabled while a webpage is loading.
> > (See https://goo.gl/SLzYR2)
> 
> Hmm, that seems like a tricky one.  Were you able to isolate a good set of
> steps to reproduce?  I tried to duplicate what I saw in the video, but it
> didn't seem to happen for me.  If you have a good way to make it happen
> consistently, please file a bug.
Flags: qe-verify+
Depends on: 1299499
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: