Closed
Bug 640081
Opened 13 years ago
Closed 13 years ago
Add "reload" method to tabs in the tabs module.
Categories
(Add-on SDK Graveyard :: General, enhancement, P2)
Add-on SDK Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
1.2
People
(Reporter: KWierso, Assigned: KWierso)
Details
Attachments
(1 file, 3 obsolete files)
3.52 KB,
patch
|
myk
:
review+
|
Details | Diff | Splinter Review |
It'd be nice to be able to trigger a tab reload from within the SDK.
Assignee | ||
Comment 1•13 years ago
|
||
This adds a reload method to a tab from the Tabs module, exposing the tabbrowser "reloadTab(tab)" function to the SDK.
Assignee | ||
Updated•13 years ago
|
Attachment #517973 -
Flags: review?(dietrich)
Comment 2•13 years ago
|
||
You could do this from a page-mod, but that seems way too heavy for this. I'm ok with this. Let's start with API-R from Myk though, before I do code review.
Updated•13 years ago
|
Attachment #517973 -
Flags: review?(dietrich) → review?(myk)
Comment 3•13 years ago
|
||
Comment on attachment 517973 [details] [diff] [review] add "reload" method This feels fine to me, although I'm curious to hear more about the use case that prompted the request! That's the kind of thing it would be useful to include in the documentation of the method.
Attachment #517973 -
Flags: review?(myk) → review+
Assignee | ||
Comment 4•13 years ago
|
||
(In reply to comment #3) > Comment on attachment 517973 [details] [diff] [review] > add "reload" method > > This feels fine to me, although I'm curious to hear more about the use case > that prompted the request! That's the kind of thing it would be useful to > include in the documentation of the method. Well, I mainly wanted to be able to make this without having to modify the SDK: https://addons.mozilla.org/en-US/firefox/addon/reload-non-pinned-tabs/ But the tabs module already has open and close functions, and there's really only one more function related to tabs that isn't in the SDK: reload.
Comment 5•13 years ago
|
||
Well, I don't know if I found a bug or what, but if you re-assign a tab url to the same url, it gets reloaded: `tab.url = tab.url`. That was how I managed to reload opened pages. Hernán
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to comment #5) > Well, I don't know if I found a bug or what, but if you re-assign a tab url to > the same url, it gets reloaded: `tab.url = tab.url`. That was how I managed to > reload opened pages. > > Hernán That probably is just triggering the location changing event, even though the URL's the same.
Updated•13 years ago
|
Assignee: nobody → kwierso
Keywords: checkin-needed
OS: Windows 7 → All
Priority: -- → P3
Hardware: x86 → All
Target Milestone: --- → 1.0
Assignee | ||
Comment 7•13 years ago
|
||
I updated the patch to include the documentation of the method in tabs.md. It also has some grammar/spelling fixes for comments that I found while looking for where to update the patch. Myk, this still wanted?
Attachment #517973 -
Attachment is obsolete: true
Attachment #532039 -
Flags: review?(myk)
Comment 8•13 years ago
|
||
Comment on attachment 532039 [details] [diff] [review] updated patch with the method added to documentation (In reply to comment #7) > Myk, this still wanted? Yes, I do still want it! And the code and docs look good. The only thing it's missing is a test, so r- for that; will be r+ once accompanied by a test. Note: at this late stage of the 1.0 release, it's best to avoid checking in changes like this, so let's land this for the next release after 1.0.
Attachment #532039 -
Flags: review?(myk) → review-
Comment 9•13 years ago
|
||
delete my name from your mailing list
Updated•13 years ago
|
Comment 10•13 years ago
|
||
Larry: I'm not sure what you mean by mailing list, as this is a bug tracking system, but in any case you can disable all mail from this system on the Email Preferences page: https://bugzilla.mozilla.org/userprefs.cgi?tab=email If, on the other hand, you only want to receive certain messages, then use that page to specify which messages you want to receive, and add/remove yourself from the cc: field of specific bugs accordingly. See the Bugzilla Guide for more information: http://www.bugzilla.org/docs/4.0/en/html/using.html
Assignee | ||
Comment 11•13 years ago
|
||
Okay, so I added a test now. I wasn't quite sure how to test that the tab actually does reload, so the test I added is kind of odd. When the tabs module opens the test URL, the page's script changes the title to the Date object's getTime() output. The test then calls the tab's reload() method, which causes the page's script to change the title again. If the page really did reload, the two titles should be different, since the time between the reloads would be different. I'm open to a less weird option, though... :)
Attachment #532039 -
Attachment is obsolete: true
Attachment #542371 -
Flags: review?(myk)
Comment 12•13 years ago
|
||
Comment on attachment 542371 [details] [diff] [review] patch with documentation and test I think it would be better to watch for an actual second load event, i.e. something like this: exports.testTabReload = function(test) { test.waitUntilDone(); openBrowserWindow(function(window, browser) { let tabs = require("tabs"); let url = "data:text/html,<!doctype%20html><title></title>"; tabs.open({ url: url, onReady: function onReady(tab) { tab.removeListener("ready", onReady); browser.addEventListener( "load", function onLoad() { browser.removeEventListener("load", onLoad, true); browser.addEventListener( "load", function onReload() { browser.removeEventListener("load", onReload, true); test.pass("the tab was loaded again"); test.assertEqual(tab.url, url, "the tab has the same URL"); closeBrowserWindow(window, function() test.done()); }, true ); tab.reload(); }, true ); }}); }); };
Attachment #542371 -
Flags: review?(myk) → review-
Assignee | ||
Comment 13•13 years ago
|
||
Like this?
Attachment #542371 -
Attachment is obsolete: true
Attachment #551571 -
Flags: review?(myk)
Assignee | ||
Comment 14•13 years ago
|
||
Re-prioritizing all 1.1-targeted feature requests to 1.2.
Target Milestone: 1.1 → 1.2
Comment 15•13 years ago
|
||
Comment on attachment 551571 [details] [diff] [review] reload patch (+doc +test) (In reply to Wes Kocher (:KWierso) (Jetpack Bugmaster) from comment #13) > Like this? Yes, exactly! r=myk
Attachment #551571 -
Flags: review?(myk) → review+
Comment 16•13 years ago
|
||
https://github.com/mozilla/addon-sdk/commit/470abdb9519906914b494279015c4d43efd405b8
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•