Closed Bug 1199765 Opened 6 years ago Closed 6 years ago

Add support for querying active state of remote browsers

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: jimm, Assigned: jimm)

References

Details

Attachments

(2 files, 3 obsolete files)

We need this so we can enable tests that check active state.
Attached patch cpp bits v.1 (obsolete) — Splinter Review
Adds caching for active state and support for returning the value from remote-browser.
Attachment #8655478 - Flags: review?(dtownsend)
Attached patch browser bits v.1 (obsolete) — Splinter Review
The scary thing about this is that without the tabbrowser fixes this test fails.
Attachment #8655481 - Flags: review?(dao)
Comment on attachment 8655478 [details] [diff] [review]
cpp bits v.1

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

::: dom/interfaces/base/nsITabParent.idl
@@ +29,5 @@
> +    * Returns the doc shell active state. Note this does not query the
> +    * actual docShell on the child side, it returns a value cached in
> +    * SetIsDocShellActive.
> +    */
> +  readonly attribute boolean docShellIsActive;

Any reason not to make this a read/write attribute and get rid of the method above?
Attachment #8655478 - Flags: review?(dtownsend) → review+
(In reply to Dave Townsend [:mossop] from comment #4)
> Comment on attachment 8655478 [details] [diff] [review]
> cpp bits v.1
> 
> Review of attachment 8655478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/interfaces/base/nsITabParent.idl
> @@ +29,5 @@
> > +    * Returns the doc shell active state. Note this does not query the
> > +    * actual docShell on the child side, it returns a value cached in
> > +    * SetIsDocShellActive.
> > +    */
> > +  readonly attribute boolean docShellIsActive;
> 
> Any reason not to make this a read/write attribute and get rid of the method
> above?

Yeah I can try that I guess. Let me search around, make sure I have a handle on all the uses of the older api.
Doesn't look bad, and I get zero hits in addons code.
Attached patch cpp bits v.2 (obsolete) — Splinter Review
Updated some naming, so requesting follow up review.
Attachment #8655478 - Attachment is obsolete: true
Attachment #8655592 - Flags: review?(dtownsend)
Attached patch cpp bits v.2Splinter Review
- updated a comment.
Attachment #8655592 - Attachment is obsolete: true
Attachment #8655592 - Flags: review?(dtownsend)
Attachment #8655594 - Flags: review?(dtownsend)
Comment on attachment 8655594 [details] [diff] [review]
cpp bits v.2

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

Didn't spot the name collision there, still this looks good
Attachment #8655594 - Flags: review?(dtownsend) → review+
Attached patch browser bits v.2Splinter Review
Expanded version of this test that includes checking to make sure the cached c++ value is correct.
Attachment #8655481 - Attachment is obsolete: true
Attachment #8655481 - Flags: review?(dao)
Attachment #8655917 - Flags: review?(dao)
Attachment #8655917 - Flags: review?(dao) → review+
The push didn't include the tabbrowser.xml changes. It turns out that having two tabs active is by design with e10s, we manage active state through the tabswitcher defined in tabbrowser. Basically we need to turn the new tab on so that it will paint, and keep the old tab active briefly since we keep it visible until layer tree updates arrive. So the second patch just updates the test.
You need to log in before you can comment on or make changes to this bug.