Last Comment Bug 783410 - Stop the social sidebar from interfering w/ awesomebar and NewTab
: Stop the social sidebar from interfering w/ awesomebar and NewTab
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: 17 Branch
: All All
: -- normal (vote)
: Firefox 17
Assigned To: Mark Hammond [:markh]
:
: Shane Caraveo (:mixedpuppy)
Mentors:
: 783318 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-16 16:21 PDT by Jared Wein [:jaws] (please needinfo? me)
Modified: 2012-10-24 18:05 PDT (History)
6 users (show)
markh: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Prevent sidebar from being added to global history (3.96 KB, patch)
2012-08-20 00:16 PDT, Mark Hammond [:markh]
jaws: review+
Details | Diff | Splinter Review
Now handles service window too (6.17 KB, patch)
2012-08-20 19:08 PDT, Mark Hammond [:markh]
no flags Details | Diff | Splinter Review
Updated - gBrowser.docShell is a shortcut for gBrowser.selectedBrowser.docShell (6.15 KB, patch)
2012-08-20 19:17 PDT, Mark Hammond [:markh]
gavin.sharp: review+
Details | Diff | Splinter Review
minor changes to tests (6.02 KB, patch)
2012-08-22 23:36 PDT, Mark Hammond [:markh]
markh: review+
Details | Diff | Splinter Review

Description Jared Wein [:jaws] (please needinfo? me) 2012-08-16 16:21:07 PDT
On my New Tab page, I'm seeing a link to the same location as that of the sidebar page. We probably want to make sure that the sidebar doesn't affect the New Tab page specifically, and likely the awesomebar, etc.
Comment 1 Mark Hammond [:markh] 2012-08-16 16:50:42 PDT
and also "History" (where it also appears).  Hopefully this will be as simple as adding disableglobalhistory="true" to our <browser> elements...
Comment 2 Mark Hammond [:markh] 2012-08-20 00:16:34 PDT
Created attachment 653278 [details] [diff] [review]
Prevent sidebar from being added to global history

This patch really just adds disableglobalhistory="true" to the sidebar window - the rests are all tests for this, including testing that the toolbar panels also don't end up in history - they weren't, but I figured the tests are worth keeping.
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-20 10:28:34 PDT
Seems like we'll need to do something similar for the social panels, right? Those are just iframes so it may be a bit more complicated...

(Why use e.data.data.location instead of just e.data.location, for got-panel-message?)
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-08-20 13:44:21 PDT
Comment on attachment 653278 [details] [diff] [review]
Prevent sidebar from being added to global history

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

This looks fine to me, but can you file another bug for the panels, service windows, and chatboxes? Since at least panels and chatboxes use xul:iframes, we'll want to add the disableglobalhistory attribute to xul:iframe.

The service window can get the disableglobalhistory attribute on its embedded browser.

r+ with the below change.

::: browser/base/content/test/social_worker.js
@@ +39,5 @@
>          if (testPort && event.data.result == "ok")
> +          testPort.postMessage({topic:"got-panel-message",
> +                                data: {
> +                                  location: event.data.location
> +                                }

Should just be {topic:"...", location: event.data.location} as Gavin suggested.
Comment 5 Mark Hammond [:markh] 2012-08-20 16:55:47 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #3)
> Seems like we'll need to do something similar for the social panels, right?
> Those are just iframes so it may be a bit more complicated...

I expected to need to change the panels, but the test attempts to verify that the content in the panels doesn't end up in history.  I should revisit that and check it is doing what I think it is.  Certainly the "service window" and chat windows will need to be considered.

> (Why use e.data.data.location instead of just e.data.location, for
> got-panel-message?)

*shrug* - we generally seem to use a convention of using just 2 elements - data.topic and data.data - but yeah, as the test already used data.result, I probably should have just ignored that convention in this case.
Comment 6 Mark Hammond [:markh] 2012-08-20 19:08:15 PDT
Created attachment 653625 [details] [diff] [review]
Now handles service window too

This new patch:

* Ensures the service window URL doesn't end up in global history (it did).

* Doesn't use 'event.data.data.location' as requested.

I also manually verified that the "panels" don't end up in history - they never did, so this patch takes no action to avoid it, but does test it.

Re-requesting review so:

* The (new) code to prevent the service window being stored in history can be checked.
* You can confirm the test in place for the panels is appropriate, so no further followup on that is necessary.

I also opened bug 784238 to ensure the chat windows don't end up in history once they land.
Comment 7 Mark Hammond [:markh] 2012-08-20 19:17:00 PDT
Created attachment 653628 [details] [diff] [review]
Updated - gBrowser.docShell is a shortcut for gBrowser.selectedBrowser.docShell

Gavin pointed out in IRC that gBrowser.docShell is a shortcut for gBrowser.selectedBrowser.docShell, so updating patch accordingly...
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-20 19:34:28 PDT
Comment on attachment 653628 [details] [diff] [review]
Updated - gBrowser.docShell is a shortcut for gBrowser.selectedBrowser.docShell

(You should bump up the context in exported patch to have 8 lines of context, helps for reviewing: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_customize_the_format_of_the_patches_Mercurial_creates.3F )

IIRC we require that all manifest URIs be absolute, so I'm not sure that ensureSocialUrlNotRemembered needs an origin parameter?
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-20 19:34:52 PDT
We'll probably also need to do something similar for the social panels, right?
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-08-21 08:13:43 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #9)
> We'll probably also need to do something similar for the social panels,
> right?

We discussed this on IRC - those are just iframes, and so don't have global history enabled by default, so no need to do anything there.
Comment 11 Mark Hammond [:markh] 2012-08-22 23:36:09 PDT
Created attachment 654528 [details] [diff] [review]
minor changes to tests

This new version of the patch removes the 'origin' arg from ensureSocialUrlNotRemembered but is otherwise identical, so I'll carry the r+ forward.  Also, FWIW, this patch is part of the try run at https://tbpl.mozilla.org/?tree=Try&rev=1e0b679f0a6b
Comment 13 Ryan VanderMeulen [:RyanVM] 2012-08-25 08:46:09 PDT
https://hg.mozilla.org/mozilla-central/rev/c9068173a7da
Comment 14 Randell Jesup [:jesup] 2012-08-28 23:55:56 PDT
I'm seeing odd failures to add to history on m-c in a build made today; for example selecting a full build log in tbpl it didn't push a history frame (i.e. back remained disabled).  This is the only "interesting" checkin recently in the area
Comment 15 Randell Jesup [:jesup] 2012-08-29 00:18:35 PDT
An updated build seems to have fixed this; never mind.
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-24 18:05:54 PDT
*** Bug 783318 has been marked as a duplicate of this bug. ***

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