Closed Bug 1309980 Opened 8 years ago Closed 8 years ago

Disable Simplify Print feature on non-Nightly Linux, and late beta / release.

Categories

(Toolkit :: Printing, defect)

49 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla52
Tracking Status
firefox50 --- verified
firefox51 --- verified
firefox52 --- verified

People

(Reporter: mconley, Assigned: mconley)

Details

Attachments

(2 files)

We've identified some bugs in the Simplify Print feature (bug 962433) that we'd like to fix before the feature is shipped to our release population.

Those bugs (by priority) are:
bug 1306295
bug 1306294
bug 1306299

I've coordinated with relman, and the plan is to disable the feature for Linux for all channels except for Nightly until bug 1306295 is fixed.

We will also only enable the feature on Windows for EARLY_BETA_OR_EARLIER until the other two bugs are addressed. The hope is that we can uplift fixes such that the feature can roll out in Firefox 51 (after which we can remove the EARLY_BETA_OR_EARLIER condition).
Comment on attachment 8800880 [details]
Bug 1309980 - Only enable Simplify Print on early Betas for Windows, and on Nightly for Linux.

https://reviewboard.mozilla.org/r/85696/#review84692

::: browser/app/profile/firefox.js:1526
(Diff revision 1)
>  
> -// Enable the "Simplify Page" feature in Print Preview
> -pref("print.use_simplify_page", true);
> +// "Simplify Page" feature in Print Preview. This feature is disabled by default
> +// in toolkit.
> +//
> +// This feature is only enabled on Nightly for Linux until bug 1306295 is fixed.
> +#if defined(XP_LINUX) && defined(NIGHTLY_BUILD)

What is the difference between XP_LINUX and UNIX_BUT_NOT_MAC?

I think we always preferred UNIX_BUT_NOT_MAC as it covered distros that are not considered "linux".

Then you could refactor the if/else to remove the !defined check.

#ifdef UNIX_BUT_NOT_MAC
#if defined(NIGHTLY_BUILD)
pref("print.user_simplify_page", true);
#endif
#else
#if defined(EARLY_BETA_OR_EARLIER)
pref("print.user_simplify_page", true);
#endif
#endif

I didn't test it, but can we use parens in the #if syntax?

#if (defined(UNIX_BUT_NOT_MAC) && defined(NIGHTLY_BUILD)) || (!defined(XP_LINUX) && defined(EARLY_BETA_OR_EARLIER))
pref("print.user_simplify_page", true);
#endif
Attachment #8800880 - Flags: review?(jaws) → review+
Comment on attachment 8800880 [details]
Bug 1309980 - Only enable Simplify Print on early Betas for Windows, and on Nightly for Linux.

https://reviewboard.mozilla.org/r/85696/#review84692

> What is the difference between XP_LINUX and UNIX_BUT_NOT_MAC?
> 
> I think we always preferred UNIX_BUT_NOT_MAC as it covered distros that are not considered "linux".
> 
> Then you could refactor the if/else to remove the !defined check.
> 
> #ifdef UNIX_BUT_NOT_MAC
> #if defined(NIGHTLY_BUILD)
> pref("print.user_simplify_page", true);
> #endif
> #else
> #if defined(EARLY_BETA_OR_EARLIER)
> pref("print.user_simplify_page", true);
> #endif
> #endif
> 
> I didn't test it, but can we use parens in the #if syntax?
> 
> #if (defined(UNIX_BUT_NOT_MAC) && defined(NIGHTLY_BUILD)) || (!defined(XP_LINUX) && defined(EARLY_BETA_OR_EARLIER))
> pref("print.user_simplify_page", true);
> #endif

> I didn't test it, but can we use parens in the #if syntax?

Unfortunately, it looks like &&'s within parens is not allowed by the pre-processor for some reason:

```
 3:05.69 Traceback (most recent call last):
 3:05.69   File "c:\mozilla-build\python\Lib\runpy.py", line 162, in _run_module_as_main
 3:05.69     "__main__", fname, loader, pkg_name)
 3:05.69   File "c:\mozilla-build\python\Lib\runpy.py", line 72, in _run_code
 3:05.69     exec code in run_globals
 3:05.69   File "c:\Users\mconley\mozilla\mozilla-central\python\mozbuild\mozbuild\action\preprocessor.py", line 18, in <module>
 3:05.69     main(sys.argv[1:])
 3:05.69   File "c:\Users\mconley\mozilla\mozilla-central\python\mozbuild\mozbuild\action\preprocessor.py", line 14, in main
 3:05.69     pp.handleCommandLine(args, True)
 3:05.69   File "c:\Users\mconley\mozilla\mozilla-central\python\mozbuild\mozbuild\preprocessor.py", line 481, in handleCommandLine
 3:05.69     self.processFile(input=input, output=out)
 3:05.69   File "c:\Users\mconley\mozilla\mozilla-central\python\mozbuild\mozbuild\preprocessor.py", line 387, in processFile
 3:05.69     self.do_include(input, False)
 3:05.69   File "c:\Users\mconley\mozilla\mozilla-central\python\mozbuild\mozbuild\preprocessor.py", line 776, in do_include
 3:05.69     self.handleLine(l)
 3:05.69   File "c:\Users\mconley\mozilla\mozilla-central\python\mozbuild\mozbuild\preprocessor.py", line 550, in handleLine
 3:05.69     cmd(args)
 3:05.69   File "c:\Users\mconley\mozilla\mozilla-central\python\mozbuild\mozbuild\preprocessor.py", line 592, in do_if
 3:05.69     raise Preprocessor.Error(self, 'SYNTAX_ERR', args)
 3:05.69 mozbuild.preprocessor.Error: ('c:\\Users\\mconley\\mozilla\\mozilla-central\\browser\\app\\profile\\firefox.js', 1527, 'SYNTAX_ERR', '(defined(UNIX_BUT_NOT_MAC) && defined(NIGHTLY_BUILD)) ||
(!defined(XP_LINUX) && defined(EARLY_BETA_OR_EARLIER))')
 3:05.70 c:/Users/mconley/mozilla/mozilla-central/config/rules.mk:1433: recipe for target '../../dist/bin/browser/defaults/preferences/firefox.js' failed
 3:05.70 mozmake.EXE[5]: *** [../../dist/bin/browser/defaults/preferences/firefox.js] Error 1
 3:05.70 mozmake.EXE[5]: *** Deleting file '../../dist/bin/browser/defaults/preferences/firefox.js'
 3:05.70 c:/Users/mconley/mozilla/mozilla-central/config/recurse.mk:79: recipe for target 'browser/app/misc' failed
 3:05.70 mozmake.EXE[4]: *** [browser/app/misc] Error 2
 3:05.70 mozmake.EXE[4]: *** Waiting for unfinished jobs....
```

```
#ifdef UNIX_BUT_NOT_MAC
#if defined(NIGHTLY_BUILD)
pref("print.user_simplify_page", true);
#endif
#else
#if defined(EARLY_BETA_OR_EARLIER)
pref("print.user_simplify_page", true);
#endif
#endif
```

Hm, yeah, this should work. This feature isn't even exposed on OS X since there's no print preview there, but flipping the pref should be harmless there. Yeah, this should do it. Thanks. :)
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d34cb41dab63
Only enable Simplify Print on early Betas for Windows, and on Nightly for Linux. r=jaws
https://hg.mozilla.org/mozilla-central/rev/d34cb41dab63
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment on attachment 8800880 [details]
Bug 1309980 - Only enable Simplify Print on early Betas for Windows, and on Nightly for Linux.

Approval Request Comment
[Feature/regressing bug #]:

Bug 1285607

[User impact if declined]:

Users on release will have the Simplify Page feature available to them by default, when it's not really ready for full release.

[Describe test coverage new/current, TreeHerder]:

None. This patch just flips a pref to a certain value for certain build-time configurations.

[Risks and why]: 

Very low risk. This was discussed with relman (Ritu), and was agreed upon as the correct course of action.

[String/UUID change made/needed]:

None.
Attachment #8800880 - Flags: approval-mozilla-beta?
Attachment #8800880 - Flags: approval-mozilla-aurora?
Comment on attachment 8800880 [details]
Bug 1309980 - Only enable Simplify Print on early Betas for Windows, and on Nightly for Linux.

Sounds like a good plan, Aurora51+, Beta50+
Attachment #8800880 - Flags: approval-mozilla-beta?
Attachment #8800880 - Flags: approval-mozilla-beta+
Attachment #8800880 - Flags: approval-mozilla-aurora?
Attachment #8800880 - Flags: approval-mozilla-aurora+
I could verify that using Firefox 50 beta 9 and latest Developer Edition 51.0a2, simplify page feature is disabled ('print.use_simplify_page' set to 'false') but also latest Nightly 52.0a1 as well.
I noticed that in about:config I have:
'print.use_simplify_page' set to 'false' - which does disable/enable the feature
'print.user_simplify_page' set to 'true' - which does nothing (visible at least)

I also noticed this in the attached patch so I think that's the problem here 'user' instead of 'use' in pref name (typo maybe). ni? Mike for confirmation.
Flags: needinfo?(mconley)
(In reply to Matheus Longaray from comment #12)
> Good catch, Bogdan! Here's the code that checks the pref name:
> http://searchfox.org/mozilla-central/source/toolkit/components/printing/
> content/printPreviewBindings.xml#153

Thanks Matheus!

Forgot to mention the platforms that I tested with a bit more clarification on the simplify pref:

- Windows 10 64-bit and Windows 7 64-bit:
 - Firefox 50 beta 9
  - print.use_simplify_page - false
 - latest Developer Edition 51.0a2
  - print.use_simplify_page - false
  - print.user_simplify_page - true
 - latest Nightly 52.0a1
  - print.use_simplify_page - false
  - print.user_simplify_page - true

- Ubuntu 16.04 64-bit and Fedora 22.0 64-bit.
 - Firefox 50 beta 9
  - print.use_simplify_page - false
 - latest Developer Edition 51.0a2
  - print.use_simplify_page - false
 - latest Nightly 52.0a1
  - print.use_simplify_page - false
  - print.user_simplify_page - true
Gah - how embarrassing. Thank you for catching that Bogdan. I'll have a corrected patch up shortly.
Flags: needinfo?(mconley)
MozReview-Commit-ID: 7qL95EQcASv
Comment on attachment 8803343 [details] [diff] [review]
Follow-up to fix a typo in a pref name

I'm afraid I had a typo in the pref that I set in the original patch. This effectively disabled Simplify Page for all users.

Approval Request Comment
[Feature/regressing bug #]:

Bug 1285607

[User impact if declined]:

Simplify Page feature in Print Preview will be disabled for all users.

[Describe test coverage new/current, TreeHerder]:

None.

[Risks and why]: 

Low.

[String/UUID change made/needed]:

None.
Attachment #8803343 - Flags: approval-mozilla-beta?
Attachment #8803343 - Flags: approval-mozilla-aurora?
Attachment #8800880 - Flags: checkin+
checkin-needed for attachment 8803343 [details] [diff] [review]

Author: Mike Conley <mconley@mozilla.com>
Bug number: 1309980
Commit message: "Bug 1309980 - Follow-up: fix typo in pref name for Simplify Page feature. r=jaws."
Keywords: checkin-needed
Comment on attachment 8803343 [details] [diff] [review]
Follow-up to fix a typo in a pref name

Glad SoftVision caught this issue before 50 hit the release channel, Aurora51+, Beta50+
Attachment #8803343 - Flags: approval-mozilla-beta?
Attachment #8803343 - Flags: approval-mozilla-beta+
Attachment #8803343 - Flags: approval-mozilla-aurora?
Attachment #8803343 - Flags: approval-mozilla-aurora+
Hi Wes, this patch needs to land before I gtb 50.b10. Please and thanks! :)
Flags: needinfo?(wkocher)
Note that the typo did not put release at risk, since release isn't supposed to have the feature in the first place. The feature is definitely disabled where we need it to be disabled - but it's also disabled where it doesn't need to be (Nightly on Linux, everything up to early Beta for Windows). So it was accidentally disabled across the board.
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb91b69b2334
Follow-up to fix a typo in a pref name. r=jaws a=ritu
Keywords: checkin-needed
I just verified using Firefox 50 beta 10, latest Developer Edition and latest Nightly on Windows 10 64bit, Windows 7 32bit and Ubuntu 16.04 32bit that 'Simplify page' feature is disabled in Fx50 across platforms, disabled in Fx51 on Ubuntu but enabled in Fx51 on Windows and enabled in Fx52 across platforms.
You need to log in before you can comment on or make changes to this bug.