The default bug view has changed. See this FAQ.

Panorama telemetry gatherer checks global PB state

RESOLVED FIXED in Firefox 16

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: jdm, Assigned: sawrubh)

Tracking

Trunk
Firefox 16

Details

(Whiteboard: [mentor=jdm][lang=js], URL)

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

5 years ago
We should change this check to use something like

window.getInterface(Ci.nsIWebNavigation)
             .QueryInterface(Ci.nsIDocShellTreeItem)
             .treeOwner
             .QueryInterface(Ci.nsIInterfaceRequestor)
             .getInterface(Ci.nsIXULWindow)
             .docShell.QueryInterface(Ci.nsILoadContext).usePrivateBrowsing;

to determine if the window is private or not.
(Reporter)

Updated

5 years ago
Whiteboard: [mentor=jdm][lang=js]
(Reporter)

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 1

5 years ago
Created attachment 633912 [details] [diff] [review]
Patch v1

Why has Josh advised to use the QI goop he says and not             

let temp = window.QueryInterface(Ci.nsIInterfaceRequestor)
                 .getInterface(Ci.nsIWebNavigation)
                 .QueryInterface(Ci.nsIDocShellTreeItem)
                 .rootTreeItem
                 .QueryInterface(Ci.nsIInterfaceRequestor)
                 .getInterface(Ci.nsIDOMWindow)
                 .wrappedJSObject;
and then just accesing |temp.gPrivateBrowsingUI.privateWindow|. Is there any difference in what I propose (although I feel both will give the same result) ?
Assignee: nobody → saurabhanandiit
Status: NEW → ASSIGNED
Attachment #633912 - Flags: feedback?(ttaubert)
Attachment #633912 - Flags: feedback?(ehsan)
I think Josh's suggestion predates the privateWindow API.  Have you run this through the try server?
Comment on attachment 633912 [details] [diff] [review]
Patch v1

Please use the privateWindow API.
Attachment #633912 - Flags: feedback?(ttaubert)
Attachment #633912 - Flags: feedback?(ehsan)
Attachment #633912 - Flags: feedback-
(Reporter)

Comment 4

5 years ago
Rereading the code now, I can't imagine that using window works here. I think we need to modify the collection code to avoid private browsing windows and remove the check from the observe method.
(Assignee)

Comment 5

5 years ago
@Ehsan, one silly question :P, is the privateWindow API something special that I don't know about or is it just the process that I have written in my comment ?

@Josh, do you mean that the QI goop that I've said in my comment(which uses window to QI and get chrome window) will not work ?

@Both, for the try run should I do |try: -b do -p all -u all -t none| or all the talos suites also ?
(Assignee)

Comment 6

5 years ago
Created attachment 633953 [details] [diff] [review]
Patch v2

Tried to combine Ehsan's and Josh's comments. Still have a doubt(as Josh said) that using window might not work. So please check if the QI goop that I used is correct or not.
Attachment #633912 - Attachment is obsolete: true
Attachment #633953 - Flags: feedback?(ehsan)
(Reporter)

Comment 7

5 years ago
Comment on attachment 633953 [details] [diff] [review]
Patch v2

This still won't work, since there's no window object to manipulate. http://mxr.mozilla.org/mozilla-central/source/browser/components/tabview/groupitems.js defines the groupItems that is being manipulated in _collect - we should iterate through the result of groupItem.getChildren and obtain window objects from each one, and only then can we do the QI dance. We'll also want to have counter values that hold the totals of non-private windows we encounter.
Attachment #633953 - Flags: feedback?(ehsan) → feedback-
(Reporter)

Comment 8

5 years ago
With regards to tests, running mochitest-browser-chrome should cover it.
Use gWindow.gPrivateBrowsingUI.privateWindow.
(In reply to Saurabh Anand [:sawrubh] from comment #5)
> @Ehsan, one silly question :P, is the privateWindow API something special
> that I don't know about or is it just the process that I have written in my
> comment ?

What you wrote in your comment.

> @Both, for the try run should I do |try: -b do -p all -u all -t none| or all
> the talos suites also ?

No, you generally don't need to run talos tests, unless you're trying to measure performance.
(Assignee)

Comment 11

5 years ago
@Josh, About the counter that you say is needed , what do you want me to do with it, set up a telemetry attribute for it or just return it ?
Please disregard comment 7, you don't need a counter. Your patch is on the right track, except that it should use gWindow.gPrivateBrowsingUI.privateWindow.
(Assignee)

Comment 13

5 years ago
Created attachment 637300 [details] [diff] [review]
Patch v3

Is there anything else that needs to be done in this patch ?
Attachment #633953 - Attachment is obsolete: true
Attachment #637300 - Flags: feedback?(ehsan)
Comment on attachment 637300 [details] [diff] [review]
Patch v3

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

Looks good with the nit below.  Please ask Tim Taubert to review this, though, since it's been a while since I've looked at the Panorama code.

::: browser/components/tabview/telemetry.js
@@ +30,5 @@
>      let childCounts = [];
> +    // Assuming the default state is Normal mode
> +    let pbFlag = false;
> +    if (gWindow && ("gPrivateBrowsingUI" in gWindow))
> +      pbFlag = gWindow.gPrivateBrowsingUI.privateWindow;

Nit: instead of storing the result in pbFlag, just move this code down and check directly in the if condition.
Attachment #637300 - Flags: feedback?(ehsan) → feedback+
(Assignee)

Comment 15

5 years ago
Created attachment 637632 [details] [diff] [review]
Patch v4

Fixed the nit pointed by Ehsan.
Attachment #637300 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=a2deb9e92927
(Assignee)

Comment 17

5 years ago
Comment on attachment 637632 [details] [diff] [review]
Patch v4

The tree is green.
Attachment #637632 - Flags: review?(ttaubert)
Comment on attachment 637632 [details] [diff] [review]
Patch v4

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

Thanks Saurabh, that looks good.

::: browser/components/tabview/telemetry.js
@@ +28,4 @@
>    _collect: function Telemetry_collect() {
>      let stackedGroupsCount = 0;
>      let childCounts = [];
> +    

Nit: please just revert this change.

@@ +46,5 @@
>        let middle = Math.floor(aChildCounts.length / 2);
>        return aChildCounts[middle];
>      }
> +    
> +    if (gWindow && ("gPrivateBrowsingUI" in gWindow) && !gWindow.gPrivateBrowsingUI.privateWindow) {

You don't need to check for 'gWindow', that'll always exist. Please move this check back to its original position - the observe method.
Attachment #637632 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 19

5 years ago
Created attachment 637830 [details] [diff] [review]
Patch v5

Fixed the nits and comments.
Attachment #637632 - Attachment is obsolete: true
Attachment #637830 - Flags: review?(ttaubert)
Comment on attachment 637830 [details] [diff] [review]
Patch v5

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

::: browser/components/tabview/telemetry.js
@@ +56,4 @@
>     * Observes for gather telemetry topic.
>     */
>    observe: function Telemetry_observe(aSubject, aTopic, aData) {
> +    if (("gPrivateBrowsingUI" in gWindow) && !gWindow.gPrivateBrowsingUI.privateWindow)

This must be:

> if (!("gPrivateBrowsingUI" in gWindow) || !gWindow.gPrivateBrowsingUI.privateWindow)

because we'll want to collect when there's no private browsing UI.
(Assignee)

Comment 21

5 years ago
Created attachment 637834 [details] [diff] [review]
Patch v6

Sorry for the wrong logic.
Attachment #637830 - Attachment is obsolete: true
Attachment #637830 - Flags: review?(ttaubert)
Attachment #637834 - Flags: review?(ttaubert)
Comment on attachment 637834 [details] [diff] [review]
Patch v6

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

Please also remove the 'gPrivateBrowsing' getter defined in browser/components/tabview/tabview.js, it's not used anymore. r=me with that change. Thank you!
Attachment #637834 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 23

5 years ago
Created attachment 637837 [details] [diff] [review]
Patch v7
Attachment #637834 - Attachment is obsolete: true
Attachment #637837 - Flags: review?(ttaubert)
Comment on attachment 637837 [details] [diff] [review]
Patch v7

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

That should be r++ now. Can you please push it to try before we land it?
Attachment #637837 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 25

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=ed38fbf4c316
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ad21d37ab91
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/6ad21d37ab91
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(In reply to Tim Taubert [:ttaubert] from comment #20)
> Comment on attachment 637830 [details] [diff] [review]
> Patch v5
> 
> Review of attachment 637830 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/tabview/telemetry.js
> @@ +56,4 @@
> >     * Observes for gather telemetry topic.
> >     */
> >    observe: function Telemetry_observe(aSubject, aTopic, aData) {
> > +    if (("gPrivateBrowsingUI" in gWindow) && !gWindow.gPrivateBrowsingUI.privateWindow)
> 
> This must be:
> 
> > if (!("gPrivateBrowsingUI" in gWindow) || !gWindow.gPrivateBrowsingUI.privateWindow)
> 
> because we'll want to collect when there's no private browsing UI.

gPrivateBrowsingUI must exist here. What's the point of the check?
(In reply to Dão Gottwald [:dao] from comment #28)
> > > if (!("gPrivateBrowsingUI" in gWindow) || !gWindow.gPrivateBrowsingUI.privateWindow)
> > 
> > because we'll want to collect when there's no private browsing UI.
> 
> gPrivateBrowsingUI must exist here. What's the point of the check?

Yeah, in retrospect, that's unnecessary. Panorama is of course active in browser windows only. Doesn't really hurt but can be removed in the future.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.