Daniel Holbert [:dholbert] Reporter
	Description • 8 years ago

STR:
 1. Visit some simple web page, e.g. https://archive.mozilla.org/
 2. Enter Responsive Design Mode, Ctrl Shift M
 3. File|Save
 4. Load your saved file

EXPECTED RESULTS:
I should've saved a serialization of the page that I was viewing.

ACTUAL RESULTS:
My saved file is actually a saved copy of the Responsive Design Mode UI itself!

	Daniel Holbert [:dholbert] Reporter
	Comment 1 • 8 years ago

This is a regression in the new Responsive Design UI, as compared to the older Responsive Design UI.

(i.e. I get EXPECTED RESULTS if I toggle devtools.responsive.html.enabled to "false" and restart Firefox)

	Daniel Holbert [:dholbert] Reporter
	Comment 2 • 8 years ago

(In reply to Daniel Holbert [:dholbert] from comment #1)
> This is a regression in the new Responsive Design UI, as compared to the
> older Responsive Design UI.

(IMO this should block shipping the new UI past Nightly.  At least for me, this regression breaks my workflow for debugging issues in mobile websites and generating reduced testcases.  Not sure what bug-dependency I should set up to reflect that -- for now, I'll mark it as a regression from bug 1296498, which is where we enabled the new UI on Nightly.)

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 3 • 8 years ago

Thanks for filing, I noticed this recently as well.  We'll take a look!

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 5 • 8 years ago

The attached change should get this working for both paths to "Save page as...":

* Menu bar -> File -> Save page as... (Cmd-S / Ctrl-S)
* Context menu -> Save page as...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=244b2602bc1ae562e7afa87d7794827a899e084c

	Alexandre Poirot [:ochameau]
	Comment 6 • 8 years ago
mozreview-review

Comment on attachment 8804064 [details]
Bug 1311605 - Support save page for RDM content.

https://reviewboard.mozilla.org/r/88208/#review87338

I'm feeling anxious to do such thing. I'm wondering if that's the line we shouldn't cross.

Couldn't we overload <xul:browser>.QueryInterface to return an object with a modified frameLoader attribute?
Would that be any better than this? At least it wouldn't hack a method used elsewhere than just the RDM tab.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 7 • 8 years ago
mozreview-review-reply

Comment on attachment 8804064 [details]
Bug 1311605 - Support save page for RDM content.

https://reviewboard.mozilla.org/r/88208/#review87338

I understand...  I considered `QueryInterface` as well, and it may work, but I just thought that would seem even more random and mysterious than targeting the actual problem area.  It just so happens that this particular `QueryInterface` call is actually a no op and could be removed, so it seemed like building a fix around something that doesn't even need to be there isn't the greatest.

I am open to other options if you see them!

	Alexandre Poirot [:ochameau]
	Comment 8 • 8 years ago

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Comment on attachment 8804064 [details]
> Bug 1311605 - Support save page for RDM content.
> 
> https://reviewboard.mozilla.org/r/88208/#review87338
> 
> I understand...  I considered `QueryInterface` as well, and it may work, but
> I just thought that would seem even more random and mysterious than
> targeting the actual problem area.  It just so happens that this particular
> `QueryInterface` call is actually a no op and could be removed, so it seemed
> like building a fix around something that doesn't even need to be there
> isn't the greatest.
> 
> I am open to other options if you see them!

Remove this useless QueryInterface in browser code?

I don't see any other alternative. Hack save function, hack frameLoader usages, tweak browser code.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 9 • 8 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #8)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> > Comment on attachment 8804064 [details]
> > Bug 1311605 - Support save page for RDM content.
> > 
> > https://reviewboard.mozilla.org/r/88208/#review87338
> > 
> > I understand...  I considered `QueryInterface` as well, and it may work, but
> > I just thought that would seem even more random and mysterious than
> > targeting the actual problem area.  It just so happens that this particular
> > `QueryInterface` call is actually a no op and could be removed, so it seemed
> > like building a fix around something that doesn't even need to be there
> > isn't the greatest.
> > 
> > I am open to other options if you see them!
> 
> Remove this useless QueryInterface in browser code?

Right, this could be done, but doesn't help us solve the problem here...?

> I don't see any other alternative. Hack save function, hack frameLoader
> usages, tweak browser code.

Since I can't alter the frameLoader property, altering the saveBrowser function seemed like the only option.  Even if we did decide to tweak browser code directly, I am not sure what that change would look like.  It seems like it would need to be something RDM specific since the nested browser concept doesn't really exist elsewhere, and I'm not sure we want to maintain an RDM specific change like that in core browser code at this time.

	Alexandre Poirot [:ochameau]
	Comment 10 • 8 years ago

Can't you alter it like this?
  Object.defineProperty(browser, "frameLoader", {value: viewFrameLoader});

Would that break other things to always refer to the content one?

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 11 • 8 years ago

(In reply to Alexandre Poirot [:ochameau] from comment #10)
> Can't you alter it like this?
>   Object.defineProperty(browser, "frameLoader", {value: viewFrameLoader});
> 
> Would that break other things to always refer to the content one?

Hmm, that does seem to work...  Okay, I'll investigate this approach.  I think I only tried `browser.frameLoader = ...` at first, which fails.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 13 • 8 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b8ffd398775

	Alexandre Poirot [:ochameau]
	Comment 14 • 8 years ago
mozreview-review

Comment on attachment 8804375 [details]
Bug 1311605 - Support save page for RDM content.

https://reviewboard.mozilla.org/r/88376/#review87638

Works great even if that's still hacky, but now less that Tree Style Tab!

::: devtools/client/responsive.html/browser/tunnel.js:101
(Diff revision 1)
> +          // One exception is `receiveMessage` from SessionStore.jsm.  SessionStore
> +          // expects data updates to come in as messages targeted to a <xul:browser>.
> +          // In addition, it verifies correctness by checking that the received message's
> +          // `targetFrameLoader` property matches the `frameLoader` of the <xul:browser>.
> +          // To keep SessionStore functioning as expected, we give it the outer
> +          // `frameLoader` as if nothing has changed.

Ouch. This looks quite ugly as well.
But I feel slightly more confortable with that rather than saveBrowser overload.
Do you feel the same?

Do you have a link to the session restore that has these limiting expectations?
I'm not sure I can suggest any better alternative this time.

	J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow) Assignee
	Comment 16 • 8 years ago
mozreview-review-reply

Comment on attachment 8804375 [details]
Bug 1311605 - Support save page for RDM content.

https://reviewboard.mozilla.org/r/88376/#review87638

Yes, I think this version is much better overall.  Plus, it allowed a few previous overrides of things that used `frameLoader` to be removed, so it's more generic.

> Ouch. This looks quite ugly as well.
> But I feel slightly more confortable with that rather than saveBrowser overload.
> Do you feel the same?
> 
> Do you have a link to the session restore that has these limiting expectations?
> I'm not sure I can suggest any better alternative this time.

I agree it's not great, but it's probably the best choice short term.  We can investigate a better integration with `SessionStore.jsm` for the future, but that sounds too risky to try right now.

I'll add a link to the check I'm referencing.

	Pulsebot
	Comment 17 • 8 years ago

Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/fca61eb35df0
Support save page for RDM content. r=ochameau

	Phil Ringnalda (:philor)
	Comment 19 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/fca61eb35df0

	Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact mboldan@mozilla.com
	Comment 20 • 8 years ago

I reproduced this bug using Fx 50.0a1, build ID: 20161019030208, on Windows 10 x64.
I can confirm this issue is fixed, I verified using Fx 52.0a1, build ID: 20161111030203 on Windows 10 x64, Mac OS X 10.11 and Ubuntu 16.04 LTS.

Cheers!