Last Comment Bug 795490 - panel.contentURL change doesn't swap out the visible page in a panel
: panel.contentURL change doesn't swap out the visible page in a panel
Status: REOPENED
:
Product: Add-on SDK
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: P1 normal with 1 vote (vote)
: ---
Assigned To: Irakli Gozalishvili [:irakli] [:gozala] [@gozala]
:
Mentors:
: 859007 (view as bug list)
Depends on:
Blocks: sdk/panel
  Show dependency treegraph
 
Reported: 2012-09-28 15:15 PDT by Bryan Clark (DevTools PM) [@clarkbw]
Modified: 2013-08-27 03:42 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments

Description Bryan Clark (DevTools PM) [@clarkbw] 2012-09-28 15:15:25 PDT
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) 2012-10-04 12:16:17 PDT
related bug 574868
Comment 2 Erik Vold [:erikvold] (please needinfo? me) 2012-10-04 12:51:02 PDT
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] 2012-10-05 02:47:07 PDT
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) (:⚡︎) 2012-12-14 09:45:03 PST
I updated the PR.
Comment 5 [github robot] 2012-12-21 17:07:59 PST
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) 2013-04-06 19:10:21 PDT
we should always have a test guys.. I don't think this was actually fixed.
Comment 7 Erik Vold [:erikvold] (please needinfo? me) 2013-04-06 19:10:45 PDT
*** Bug 859007 has been marked as a duplicate of this bug. ***
Comment 8 Wes Kocher (:KWierso) 2013-08-27 01:03:31 PDT
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) 2013-08-27 03:42:22 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.