Closed Bug 1428108 Opened 2 years ago Closed 2 years ago

set browser.newtabpage.activity-stream.debug by default in local builds

Categories

(Firefox :: Messaging System, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Iteration:
60.3 - Feb 26
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: dmose, Assigned: andreio)

References

(Blocks 1 open bug)

Details

(Whiteboard: [AS60MVP])

Attachments

(1 file, 1 obsolete file)

One highly likely reason that bug 1428106 was not noticed sooner is because we don't have warnings turned on by default in local builds.  We should do that.  :-)
Iteration: --- → 1.27
Priority: -- → P2
Assignee: nobody → andrei.br92
Iteration: 1.27 → 60.1 - Jan 29
Iteration: 60.1 - Jan 29 → 60.2 - Feb 12
Severity: normal → enhancement
Whiteboard: [AS60MVP]
Iteration: 60.2 - Feb 12 → 60.3 - Feb 26
Priority: P2 → P1
Attachment #8941157 - Attachment is obsolete: true
Comment on attachment 8950708 [details]
Bug 1428108 - Set browser.newtabpage.activity-stream.debug by default in local builds.

https://reviewboard.mozilla.org/r/219956/#review225778
Attachment #8950708 - Flags: review?(edilee) → review+
Keywords: checkin-needed
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/301951ce868f
Set browser.newtabpage.activity-stream.debug by default in local builds. r=Mardak
Keywords: checkin-needed
Attachment #8950708 - Attachment is obsolete: true
Flags: needinfo?(andrei.br92)
Comment on attachment 8950708 [details]
Bug 1428108 - Set browser.newtabpage.activity-stream.debug by default in local builds.

https://reviewboard.mozilla.org/r/219956/#review226586

::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:88
(Diff revision 4)
>  
>  add_task(async function test_override_activity_stream_enabled() {
>    let notificationPromise = await setupASPrerendered();
>  
> +  if (!IS_RELEASE_OR_BETA && !MOZILLA_OFFICIAL) { // Check if local build
> +    Assert.equal(aboutNewTabService.defaultURL, ACTIVITY_STREAM_PRERENDER_DEBUG_URL, "Newtab URL should be the debug activity stream prerendered URL");

Instead of doing this check everywhere, I think it would be better that pretty much all tests run assuming debug is false. One exception is for a new test to check if the false value is the default for MOZILLA_OFFICIAL or not.
Comment on attachment 8950708 [details]
Bug 1428108 - Set browser.newtabpage.activity-stream.debug by default in local builds.

https://reviewboard.mozilla.org/r/219956/#review228148

We have less coverage with this change. My suggestion was to make most tests run with pref debug = false even for non-OFFICIAL builds. That way the existing assertions can be left untouched. For the newly added test, it can check if the default value of the debug pref is true for non-OFFICIAL.

::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js
(Diff revision 5)
>  });
>  
>  add_task(async function test_override_activity_stream_enabled() {
>    let notificationPromise = await setupASPrerendered();
>  
> -  Assert.equal(aboutNewTabService.defaultURL, ACTIVITY_STREAM_PRERENDER_URL, "Newtab URL should be the default activity stream prerendered URL");

Now there are no tests checking that `.defaultURL` is `ACTIVITY_STREAM_PRERENDER_URL`.
Attachment #8950708 - Flags: review+ → review-
Comment on attachment 8950708 [details]
Bug 1428108 - Set browser.newtabpage.activity-stream.debug by default in local builds.

https://reviewboard.mozilla.org/r/219956/#review228354

You should also update/comment the !issues in mozreview

::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:79
(Diff revision 6)
>  });
>  
>  add_task(async function test_override_activity_stream_enabled() {
>    let notificationPromise = await setupASPrerendered();
> +  // Don't run in debug mode regardless of build type
> +  Services.prefs.setBoolPref(ACTIVITY_STREAM_DEBUG_PREF, false);

All of these come immediately after a `setupASPrerendered()`, and setting `DEBUG_PREF` to `false` seems reasonable to do there as it does control which prerendered endpoint to use.

::: browser/components/newtab/tests/xpcshell/test_AboutNewTabService.js:142
(Diff revision 6)
>    Assert.equal(aboutNewTabService.activityStreamLocale, "en-US",
>      "The locale for testing should be en-US");
>  });
>  
> +add_task(async function test_debug_mode() {
> +  await setupASPrerendered();

Does this need to be called for testing debug mode?
Comment on attachment 8950708 [details]
Bug 1428108 - Set browser.newtabpage.activity-stream.debug by default in local builds.

https://reviewboard.mozilla.org/r/219956/#review228412
Attachment #8950708 - Flags: review?(edilee) → review+
Pushed by edilee@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/e7d8e299023a
Set browser.newtabpage.activity-stream.debug by default in local builds. r=Mardak
https://hg.mozilla.org/mozilla-central/rev/e7d8e299023a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Blocks: 1442067
Component: Activity Streams: Newtab → Messaging System
You need to log in before you can comment on or make changes to this bug.