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

RESOLVED INVALID

Status

Add-on SDK
General
P1
normal
RESOLVED INVALID
5 years ago
2 months ago

People

(Reporter: clarkbw, Assigned: irakli)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

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!

Updated

5 years ago
Assignee: nobody → evold
Priority: -- → P1
related bug 574868
Priority: P1 → --
Priority: -- → P1
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.
Flags: needinfo?(rFobic)
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.
Flags: needinfo?(rFobic)
Assignee: evold → rFobic
I updated the PR.
Blocks: 821476

Comment 5

5 years ago
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

Updated

5 years ago
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
we should always have a test guys.. I don't think this was actually fixed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Duplicate of this bug: 859007
I'm going through the list of open bugs that github robot has commented in. Is this bug fixed, Erik?
Flags: needinfo?(evold)
(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.
Flags: needinfo?(evold)
(Reporter)

Comment 10

2 months ago
This is SDK and isn't going to get fixed.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago2 months ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.