Closed Bug 640081 Opened 10 years ago Closed 9 years ago

Add "reload" method to tabs in the tabs module.

Categories

(Add-on SDK Graveyard :: General, enhancement, P2)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: KWierso, Assigned: KWierso)

Details

Attachments

(1 file, 3 obsolete files)

It'd be nice to be able to trigger a tab reload from within the SDK.
Attached patch add "reload" method (obsolete) — Splinter Review
This adds a reload method to a tab from the Tabs module, exposing the tabbrowser "reloadTab(tab)" function to the SDK.
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.
Attachment #517973 - Flags: review?(dietrich) → review?(myk)
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+
(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.
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
(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.
Assignee: nobody → kwierso
Keywords: checkin-needed
OS: Windows 7 → All
Priority: -- → P3
Hardware: x86 → All
Target Milestone: --- → 1.0
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 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-
delete my name from your mailing list
Keywords: checkin-needed
Priority: P3 → P2
Target Milestone: 1.0 → 1.1
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
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 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-
Like this?
Attachment #542371 - Attachment is obsolete: true
Attachment #551571 - Flags: review?(myk)
Re-prioritizing all 1.1-targeted feature requests to 1.2.
Target Milestone: 1.1 → 1.2
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+
https://github.com/mozilla/addon-sdk/commit/470abdb9519906914b494279015c4d43efd405b8
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.