Last Comment Bug 682507 - Expose tab.linkedBrowser.__SS_restoreState as a tab attribute to help style unloaded tabs
: Expose tab.linkedBrowser.__SS_restoreState as a tab attribute to help style u...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: Firefox 9
Assigned To: ithinc
:
Mentors:
Depends on:
Blocks: 716279
  Show dependency treegraph
 
Reported: 2011-08-26 21:49 PDT by ithinc
Modified: 2012-08-27 05:18 PDT (History)
4 users (show)
emorley: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.62 KB, patch)
2011-09-17 07:48 PDT, ithinc
dietrich: feedback+
Details | Diff | Review
patch v2 (4.21 KB, patch)
2011-09-21 10:16 PDT, ithinc
dietrich: review+
Details | Diff | Review
patch for checkin (4.54 KB, patch)
2011-09-25 08:16 PDT, ithinc
no flags Details | Diff | Review

Description ithinc 2011-08-26 21:49:25 PDT
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:6.0) Gecko/20100101 Firefox/6.0
Build ID: 20110811165603

Steps to reproduce:

1. Set browser.sessionstore.max_concurrent_tabs to 0 in about:config
2. Restart Firefox




Expected results:

I want to style the unloaded tabs with a lower opacity. Adding an attribute like "restoring" to reflect __SS_restoreState would help.
Comment 1 ithinc 2011-09-17 07:48:27 PDT
Created attachment 560710 [details] [diff] [review]
patch
Comment 2 Dietrich Ayala (:dietrich) 2011-09-19 14:53:55 PDT
Comment on attachment 560710 [details] [diff] [review]
patch

looks fine to me, f+. needs a test though. once you have a test for it, ask for final review.
Comment 3 ithinc 2011-09-20 02:48:12 PDT
Is there any guideline to write a testcase? Thanks.
Comment 4 ithinc 2011-09-21 10:16:31 PDT
Created attachment 561507 [details] [diff] [review]
patch v2
Comment 5 Dietrich Ayala (:dietrich) 2011-09-21 10:28:08 PDT
Sorry, I wasn't cc'd on the bug, so didn't see the follow-up comment! For future reference, check out the tests directories where the sessionstore code is - lots of examples there. Though, I presume that's what you did :)
Comment 6 Dietrich Ayala (:dietrich) 2011-09-21 10:31:09 PDT
Comment on attachment 561507 [details] [diff] [review]
patch v2

Review of attachment 561507 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/test/browser/browser_682507.js
@@ +4,5 @@
> +function test() {
> +  waitForExplicitFinish();
> +
> +  Services.prefs.setBoolPref("browser.sessionstore.restore_on_demand", true);
> +  gBrowser.addTab("about:mozilla");

looks ok, r=me.

my one concern was that in the case where you actually depend on the web content loading in the tab, you'd want to listen for some event before continuing on with the test. however, in this case, it doesn't really matter.
Comment 7 ithinc 2011-09-25 08:16:12 PDT
Created attachment 562304 [details] [diff] [review]
patch for checkin
Comment 8 Dão Gottwald [:dao] 2011-09-27 01:36:14 PDT
Comment on attachment 562304 [details] [diff] [review]
patch for checkin

>+++ b/browser/components/sessionstore/test/browser/browser_682507.js

>+  let ss = Cc['@mozilla.org/browser/sessionstore;1'].getService(Ci.nsISessionStore);

ss should already be available, head.js sets it.

>+  executeSoon(finish);

executeSoon shouldn't be needed here.
Comment 9 Dão Gottwald [:dao] 2011-09-27 01:38:20 PDT
In fact, finish should be removed altogether along with waitForExplicitFinish...
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) (Away until 6/13) 2011-09-27 05:38:04 PDT
https://hg.mozilla.org/mozilla-central/rev/e1be3e4479c3
Comment 12 ithinc 2011-09-27 18:46:00 PDT
(In reply to Dão Gottwald [:dao] from comment #9)
> In fact, finish should be removed altogether along with
> waitForExplicitFinish...

Thanks for the corrections. This is my first testcase.

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