795490 - panel.contentURL change doesn't swap out the visible page in a panel

Status: RESOLVED INVALID

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Bryan Clark (DevTools PM) [:clarkbw]]

Changing the contentURL of a panel does change the location (your content script will be notified of a location change) however it doesn't seem to change the visible page.  While there is a test for the panel location changing a panel I don't think it actually tests the change in frame contents.

Here's a simple test case of changing contentURL on timeouts
https://builder.addons.mozilla.org/package/154462/latest/

Similar question on Stack Overflow
http://stackoverflow.com/questions/12564758/changing-the-contenturl-of-a-panel-based-on-the-actions-on-the-current-page-rend

And here's the change I found fixed the issue by swapping out the frame loader before the _initFrame call.

diff --git a/packages/addon-kit/lib/panel.js b/packages/addon-kit/lib/panel.js
index edc5581..9832c27 100644
--- a/packages/addon-kit/lib/panel.js
+++ b/packages/addon-kit/lib/panel.js
@@ -354,6 +354,7 @@ const Panel = Symbiont.resolve({
 
   _onChange: function _onChange(e) {
     if ('contentURL' in e && this._frame) {
+      this._frameLoadersSwapped = false;
       // Cleanup the worker before injecting the content script in the new
       // document
       this._workerCleanup();


Original Message Here:
https://groups.google.com/forum/?fromgroups=#!topic/mozilla-labs-jetpack/a5wjP1clkCo

Thanks!

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

related bug 574868

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

Thanks for the patch Bryan!  So far your patch seems to work for me on your test case, but I think that there may be a better fix for the problem.  Afaict you are switching the panel frame and a hidden frame it uses, but I think this might mean that changing the contentURL is changing the url of the wrong frame.

Alex or Irakli can either of you take a look at this, it seems like a follow up to bug 574868 and I'm not very familiar enough with the panel implementation yet I think.  I could use a explanation of how the panel uses it's hidden frame.  As I recall it is supposed to use the hidden frame only when the panel is hidden, and swap it when the panel is showing.  If that is correct then the target frame for changes might be getting mixed up.

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Once new document is loaded `_onShow` is called:
https://github.com/mozilla/addon-sdk/blob/master/packages/addon-kit/lib/panel.js#L321

That would cause swapping frame loaders back, so this patch will not break anything.
More interesting question is why swapping frame loaders is required in first place. My guess would be platform bug / limitation in frame loaders that won't draw new page after location change.

That being said I think we should land Bryan's patch and also investigate why swapping loaders is actually required in a separate bug. We should also add comment to this change to mention that frame loaders have to be swapped to workaround platform limitation / bug pointing to that other bug.

Comment 4

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I updated the PR.

Comment 5

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/16bc7b10344538474618ec5e76530245c3ab00c4
fix for bug #795490 - panel.contentURL change doesn't swap out the visible page in a panel

https://github.com/mozilla/addon-sdk/commit/41d3fd8903dc290471b05c6e28191d33bce4b46c
Merge pull request #594 from canuckistani/bug_795490

fix Bug 795490 r=@gozala

Comment 6

[Erik Vold [:erikvold] (please needinfo? me)]

we should always have a test guys.. I don't think this was actually fixed.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm going through the list of open bugs that github robot has commented in. Is this bug fixed, Erik?

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Wes Kocher (:KWierso) from comment #8)
> I'm going through the list of open bugs that github robot has commented in.
> Is this bug fixed, Erik?

Doesn't look like it.

Comment 10

[Bryan Clark (DevTools PM) [:clarkbw]]

This is SDK and isn't going to get fixed.