Closed Bug 1340181 Opened 8 years ago Closed 8 years ago

Hide Activity Stream URL in URLbar

Categories

(Firefox Graveyard :: Activity Streams: General, defect)

Product:
Firefox Graveyard
Component:
Activity Streams: General
Platform:
All
Unspecified
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox54 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: ursula, Assigned: ursula)

References

Details

Attachments

(1 file)

Bug 1340181 - Hide Activity Stream URL in URLbar
59 bytes, text/x-review-board-request
mconley
: review+
franziskus
: review+
Details 
In the test pilot version of Activity Stream we implement our own version of url hiding in the awesome bar (https://github.com/mozilla/activity-stream/blob/master/addon/AppURLHider.js). However when we take over about:newtab we should instead hook into Firefox's built in way of hiding a url and get rid of our own url hiding methods. 

Eventually about:newtab will map to an Activity Stream url and we can just make use of the already implemented way of hiding about:newtab. Until then though (since Activity Stream will be a system add-on which overrides the new tab page), we need to implement url hiding.
Comment on attachment 8838111 [details]
Bug 1340181 - Hide Activity Stream URL in URLbar

https://reviewboard.mozilla.org/r/113094/#review114612

::: browser/base/content/browser.js:251
(Diff revision 1)
> -
> +const ACTIVITY_STREAM_URL = gPrefService.getCharPref("browser.newtabpage.activity-stream");
>  var gInitialPages = [
> +  ACTIVITY_STREAM_URL,
>    "about:blank",

So I'll note that we already have an nsIAboutNewTabService that's already be checked alongside gInitialPages [here](http://searchfox.org/mozilla-central/rev/cac6cb6a10afb8ebb2ecfbeeedaff7c66f57dd75/browser/base/content/browser.js#2468).

A lot of nsIAboutNewTabService is for the (now dead) remotely hosted newtab project. I suspect a lot of it can be gutted, but perhaps the service itself can be re-purposes to suit the incoming Activity Stream stuff.

What I recommend is the following:

1) Don't augment gInitialPages. This also means we can avoid doing a pref read on window start-up for each window.
2) Modify nsIAboutNewTabService.idl to return a read-only bool, "activityStreamEnabled" and read-only string "activityStreamURL".
3) Then, alter aboutNewTabService.js to return values for those read-only attributes. Prefs are probably fine for now.
4) Alter Activity Stream to set the enabled pref when the add-on is enabled / disabled.
5) Alter the browser.js code to check the activityStreamEnabled and activityStreamURL properties when checking whether to display the URL in the URL bar.

Then, we should file a follow-up bug to start gutting the remotely hosted newtab stuff.
Attachment #8838111 - Flags: review?(mconley) → review-
Updated patch addresses review comments above (https://bugzilla.mozilla.org/show_bug.cgi?id=1340181#c2). 

One thing to notice here is that there is a bunch of logic for content signing that was put in place for remote about:newtab (https://bugzilla.mozilla.org/show_bug.cgi?id=1226928). I think it's out of the scope of this ticket to remove all that stuff, since we're going to be filing a ticket to properly remove all the remote about:newtab code that currently lives in central, and removing the content signing stuff would be part of that work. I did remove the parts that were hitting any checks against content signing code, and I also disabled the content signing tests found in dom/security/test/contentverifier/.
Comment on attachment 8838111 [details]
Bug 1340181 - Hide Activity Stream URL in URLbar

https://reviewboard.mozilla.org/r/113094/#review116448

This looks okay to me - and I'm super glad to see the unused remote newtab page stuff get torn out. Good stuff. :)

You might want to get someone from Necko (or whoever reviewed the original remote newtab low-level networking stuff) to review this removal as well.

::: browser/app/profile/firefox.js:1199
(Diff revision 2)
>  
>  // endpoint to send newtab click and view pings
>  pref("browser.newtabpage.directory.ping", "https://tiles.services.mozilla.com/v3/links/");
>  
> -// activates the remote-hosted newtab page
> -pref("browser.newtabpage.remote", false);
> +// activates Activity Stream
> +pref("browser.newtabpage.activity-stream", false);

Are you likely to have other prefs for activity-stream? If so, this maybe should be:

browser.newtabpage.activity-stream.enabled

::: browser/base/content/browser.js:2467
(Diff revision 2)
>  
>      // Replace initial page URIs with an empty string
>      // 1. only if there's no opener (bug 370555).
> -    // 2. if remote newtab is enabled and it's the default remote newtab page
> -    let defaultRemoteURL = gAboutNewTabService.remoteEnabled &&
> +    // 2. if activity stream is enabled and it's the default newtab page
> +    let defaultActivityStreamURL = gAboutNewTabService.activityStreamEnabled &&
>                             uri.spec === gAboutNewTabService.newTabURL;

Should this be checking activityStreamURL instead?

::: browser/components/newtab/aboutNewTabService.js:118
(Diff revision 2)
> -      this._remoteEnabled = false;
> -    }
> -    if (!csTest) {
> -      this._newTabURL = ABOUT_URL;
>      }
> +     this._newtabURL = ABOUT_URL;

Busted indentation.

So, just so we're clear, this is saying that "if the activity stream state is toggled, reset the new tab URL no matter what".

Why are we doing that, out of curiosity?

::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:105
(Diff revision 2)
>  add_task(function* test_updates() {
>    /*
>     * Simulates a "cold-boot" situation, with some pref already set before testing a series
>     * of changes.
>     */
> -  Preferences.set("browser.newtabpage.remote", true);
> +  Preferences.set("browser.newtabpage.activity-stream", true);

See my note about SpecialPowers.pushPrefEnv elsewhere (this ensures we clean-up properly).

::: browser/components/newtab/tests/xpcshell/test_NewTabURL.js:20
(Diff revision 2)
>                                     "@mozilla.org/browser/aboutnewtab-service;1",
>                                     "nsIAboutNewTabService");
>  
>  add_task(function*() {
>    let defaultURL = aboutNewTabService.newTabURL;
> -  Services.prefs.setBoolPref("browser.newtabpage.remote", false);
> +  Services.prefs.setBoolPref("browser.newtabpage.activity-stream", false);

Huh - these tests should be using SpecialPowers.pushPrefEnv so that we clear up the pref change after the test file exits.

Example usage: http://searchfox.org/mozilla-central/rev/b1044cf7c2000c3e75e8181e893236a940c8b6d2/browser/base/content/test/captivePortal/browser_CaptivePortalWatcher.js#18-21

::: browser/components/newtab/tests/xpcshell/test_NewTabURL.js:38
(Diff revision 2)
>    Assert.ok(!NewTabURL.overridden, "Newtab URL should not be overridden");
>    Assert.equal(NewTabURL.get(), defaultURL, "Newtab URL should be the default");
>  
> -  // change newtab page to remote
> +  // change newtab page to activity stream
>    NewTabPrefsProvider.prefs.init();
> -  Services.prefs.setBoolPref("browser.newtabpage.remote", true);
> +  Services.prefs.setBoolPref("browser.newtabpage.activity-stream", true);

Same as above.

::: netwerk/protocol/http/nsHttpHandler.cpp
(Diff revision 2)
> -        nsAutoCString channel;
> -        prefs->GetCharPref(NEW_TAB_REMOTE_MODE, getter_Copies(channel));
> -        if (channel.EqualsLiteral("test") ||
> -            channel.EqualsLiteral("test2") ||
> -            channel.EqualsLiteral("dev")) {
> -            mNewTabContentSignaturesDisabled = true;

Probably should get someone from the Necko team to review this. Is the mNewTabContentSignaturesDisabled even necessary now?
Attachment #8838111 - Flags: review?(mconley) → review-
Comment on attachment 8838111 [details]
Bug 1340181 - Hide Activity Stream URL in URLbar

https://reviewboard.mozilla.org/r/113094/#review117242

::: browser/components/newtab/aboutNewTabService.js:113
(Diff revision 5)
>        return false;
>      }
> -
> -    let csTest = Services.prefs.getBoolPref(PREF_REMOTE_CS_TEST);
>      if (stateEnabled) {
> -      if (!csTest) {
> +      this._activityStreamURL = this.activityStreamURL;

Wait, hold on... the getter for this is just `return this._activityStreamURL`. So we're essentially setting `this._activityStreamURL` to itself. Is this supposed to be ACTIVITY_STREAM_URL ?

::: browser/components/newtab/aboutNewTabService.js:118
(Diff revision 5)
> -      this._remoteEnabled = false;
> -    }
> -    if (!csTest) {
> -      this._newTabURL = ABOUT_URL;
>      }
> +    this._newtabURL = ABOUT_URL;

Same question as last review: changing the enabled state of Activity Stream is going to change the newtabURL as well. Is that known / desirable?

Suppose I install an add-on that sets a new newtab URL. Suppose I then install and Activity Stream, which overrides it. Suppose I then disable / remove Activity Stream. At this point, the newtab URL will be set to ABOUT_URL rather than the one that the first add-on is supposed to send me to. I suspect that's accidental, right?

::: browser/components/newtab/aboutNewTabService.js:182
(Diff revision 5)
>    },
>  
>    resetNewTabURL() {
>      this._overridden = false;
>      this._newTabURL = ABOUT_URL;
> -    this.toggleRemote(Services.prefs.getBoolPref(PREF_REMOTE_ENABLED), true);
> +    this.toggleActivityStream(Services.prefs.getBoolPref(PREF_ACTIVITY_STREAM_ENABLED), true);

So if I understand correctly, we're storing two states for Activity Stream enabled-ness... like:

1) Pref state
2) State inside nsIAboutNewTabService

Is it ever possible for these two to go out of sync? If I call toggleActivityStream(true), does the pref somehow get set as well?
Attachment #8838111 - Flags: review?(mconley) → review-
Comment on attachment 8838111 [details]
Bug 1340181 - Hide Activity Stream URL in URLbar

https://reviewboard.mozilla.org/r/113094/#review117342

::: netwerk/protocol/http/nsHttpChannel.cpp:1740
(Diff revision 5)
>  
>      return NS_OK;
>  }
>  
>  nsresult
>  nsHttpChannel::ProcessContentSignatureHeader(nsHttpResponseHead *aResponseHead)

So we want to keep the general processing of content signature headers? We shold probably ping a necko peer about that. AFAIK remote newtab was the only thing using this.
Comment on attachment 8838111 [details]
Bug 1340181 - Hide Activity Stream URL in URLbar

https://reviewboard.mozilla.org/r/113094/#review117342

> So we want to keep the general processing of content signature headers? We shold probably ping a necko peer about that. AFAIK remote newtab was the only thing using this.

There will be a follow up bug filed to remove a bunch of the remote newtab related code, it was just out of the scope of this ticket. Eventually the content signing bits will all be removed if it's true that remote newtab is the only consumer of it.
Comment on attachment 8838111 [details]
Bug 1340181 - Hide Activity Stream URL in URLbar

https://reviewboard.mozilla.org/r/113094/#review117242

> Wait, hold on... the getter for this is just `return this._activityStreamURL`. So we're essentially setting `this._activityStreamURL` to itself. Is this supposed to be ACTIVITY_STREAM_URL ?

Looking at it now, if the getter returns ACTIVITY_STREAM_URL, the internal variable ```this._activityStreamURL``` is essentially useless, so I'll just remove it altogether make a call to the getter directly.

In the remote case, we were generating a remote URL based on stuff like channel, version, and locale, and storing that in a local variable ```this._remoteURL```. But for our purposes the activity stream URL won't change, so returning ACTIVITY_STREAM_URL should be fine.

> Same question as last review: changing the enabled state of Activity Stream is going to change the newtabURL as well. Is that known / desirable?
> 
> Suppose I install an add-on that sets a new newtab URL. Suppose I then install and Activity Stream, which overrides it. Suppose I then disable / remove Activity Stream. At this point, the newtab URL will be set to ABOUT_URL rather than the one that the first add-on is supposed to send me to. I suspect that's accidental, right?

I would *assume* that when the newtab page gets overriden the flag ```this._overriden``` will be true (triggered by the ```set newtabURL``` function), in which case we bail out of this function before setting the newtab URL to ABOUT_URL

> So if I understand correctly, we're storing two states for Activity Stream enabled-ness... like:
> 
> 1) Pref state
> 2) State inside nsIAboutNewTabService
> 
> Is it ever possible for these two to go out of sync? If I call toggleActivityStream(true), does the pref somehow get set as well?

From my understanding, the only time they can go out of sync is if I toggle the pref to enable activity stream, but then override my newtab page with a different URL. In that case, "technically" activity stream is enabled (as in the pref returns true), but it's not being shown because we've overriden the URL. So the pref would show it's enabled, but the state internally (```this._activityStreamEnabled```) would be false. 

So I guess the question is, if we override the newtab page, should we change the value of the pref to be false so that they stay in sync?
Comment on attachment 8838111 [details]
Bug 1340181 - Hide Activity Stream URL in URLbar

https://reviewboard.mozilla.org/r/113094/#review117242

> From my understanding, the only time they can go out of sync is if I toggle the pref to enable activity stream, but then override my newtab page with a different URL. In that case, "technically" activity stream is enabled (as in the pref returns true), but it's not being shown because we've overriden the URL. So the pref would show it's enabled, but the state internally (```this._activityStreamEnabled```) would be false. 
> 
> So I guess the question is, if we override the newtab page, should we change the value of the pref to be false so that they stay in sync?

Ah, I think I know why we're not changing the value of the pref when we override the newtab URL...this little bit of logic here: https://dxr.mozilla.org/mozilla-central/source/browser/components/newtab/aboutNewTabService.js?q=path%3A%2Faboutnewtabservice&redirect_type=single#250-260, depends on the original value of the pref to determine if we should reset the URL to activity stream. So I'm wondering if the fact that they're out of sync was in fact intentional in the first place
Comment on attachment 8838111 [details]
Bug 1340181 - Hide Activity Stream URL in URLbar

https://reviewboard.mozilla.org/r/113094/#review118126

I'm just leaving this here regading the content-signature bits. All other uses of content-signatures are well outisde this scope so nothing in here should break them. There might be more code to remove if we want to. But that's out of scope of this patch as I understand.
Attachment #8838111 - Flags: review?(franziskuskiefer) → review+
Blocks: 1344319
Comment on attachment 8838111 [details]
Bug 1340181 - Hide Activity Stream URL in URLbar

https://reviewboard.mozilla.org/r/113094/#review118822

This looks good to me, minus the below nits. Go ahead and fix those and land away! Thanks!

::: browser/components/newtab/aboutNewTabService.js:52
(Diff revision 6)
>   *
>   * 2. Redirector Access:
>   *
>   * When the URL loaded is about:newtab, the default behavior, or when entered in the
>   * URL bar, the redirector is hit. The service is then called to return either of
> - * two URLs, a chrome or remote one, based on the browser.newtabpage.remote pref.
> + * two URLs, a chrome or remote one, based on the browser.newtabpage.activity-stream.enabled pref.

Nit: "or remote one" is no longer true.

::: browser/components/newtab/aboutNewTabService.js:118
(Diff revision 6)
>     * Returns the default URL.
>     *
> -   * This URL only depends on the browser.newtabpage.remote pref. Overriding
> +   * This URL only depends on the browser.newtabpage.activity-stream.enabled pref. Overriding
>     * the newtab page has no effect on the result of this function.
>     *
> -   * The result is also the remote URL if this is in a test (PREF_REMOTE_CS_TEST)
> +   * @returns {String} the default newtab URL, activity-stream or regular  depending on browser.newtabpage.activity-stream.enabled

Nit: extra space after "regular"
Attachment #8838111 - Flags: review?(mconley) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e393e6c239cd
Hide Activity Stream URL in URLbar r=fkiefer,mconley
https://hg.mozilla.org/mozilla-central/rev/e393e6c239cd
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
https://dxr.mozilla.org/mozilla-central/search?q=browser.newtabpage.remote&redirect=false

7 results from the mozilla-central tree

Are they now hidden prefs by design?
Blocks: 1262719
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: